Re: [Qemu-devel] [PATCH v2] ahci: fix FIS I bit and PIO Setup FIS interrupt

[John Snow]

On 06/22/2018 05:06 PM, Paolo Bonzini wrote:
> On 22/06/2018 21:07, John Snow wrote:
>>> @@ -1251,6 +1249,7 @@ static void handle_reg_h2d_fis(AHCIState *s, int port,
>>>  
>>>      /* Copy the ACMD field (ATAPI packet, if any) from the AHCI command
>>>       * table to ide_state->io_buffer */
>>> +    s->dev[port].done_first_drq = false;
>>
>> If you don't mind I'm going to shift this down so that it's beneath the
>> ATAPI section, just before the ide_exec_cmd call alongside all of the
>> other reset/init calls.
> 
> It's okay if the tests pass. :)
> 

They do! Thanks for solving two of the mysteries I had for the AHCI
emulation.

>>> @@ -883,19 +879,24 @@ AHCICommand *ahci_command_create(uint8_t command_name)
>>>      return cmd;
>>>  }
>>>  
>>> -AHCICommand *ahci_atapi_command_create(uint8_t scsi_cmd, uint16_t bcl)
>>> +AHCICommand *ahci_atapi_command_create(uint8_t scsi_cmd, uint16_t bcl, 
>>> bool dma)
>>>  {
>>>      AHCICommand *cmd = ahci_command_create(CMD_PACKET);
>>>      cmd->atapi_cmd = g_malloc0(16);
>>>      cmd->atapi_cmd[0] = scsi_cmd;
>>>      stw_le_p(&cmd->fis.lba_lo[1], bcl);
>>> +    if (dma) {
>>> +        ahci_command_enable_atapi_dma(cmd);
>>> +    } else {
>>> +        cmd->interrupts |= bcl ? AHCI_PX_IS_PSS : 0;
>>> +    }
>>
>> Why are we gating on the DRQ byte count limit?
>>
>> (Oh, I guess it CAN'T be zero if we expect to transfer any data, so this
>> assumption is good to test if we expect to see even a single DRQ block.)
> 
> This is ATAPI, so "having a PIO Setup FIS with I=1" is the same as
> "having >1 PIO Setup FIS's" (data direction doesn't matter) and in turn
> that is the same as "having some bytes to transfer while DMA is off".
> 

Right, but "byte count limit" being the same as "having some bytes to
transfer" took me an extra minute. It's okay in the end because BCL is
not allowed to be zero if there is any data to transfer.

> Thanks,
> 
> Paolo
>