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

[Paolo Bonzini <pbonzini@redhat.com>]

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. :)

>> @@ -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".

Thanks,

Paolo