Re: [Qemu-devel] [PATCH 2/2] ahci: fix signature generation

[John Snow]

On 07/07/2015 04:49 AM, Stefan Hajnoczi wrote:
> On Mon, Jul 06, 2015 at 05:49:52PM -0400, John Snow wrote:
>> The initial register device-to-host FIS no longer needs to specially
>> set certain fields, as these can be handled generically by setting those
>> fields explicitly with the signatures we want at port reset time.
>>
>> (1) Signatures are decomposed into their four component registers and
>>     set upon (AHCI) port reset.
>> (2) the signature cache register is no longer set manually per-each
>>     device type, but instead just once during ahci_init_d2h.
>>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>>  hw/ide/ahci.c | 33 ++++++++++++++++++++-------------
>>  1 file changed, 20 insertions(+), 13 deletions(-)
> 
> I see two code paths that call ahci_init_d2h().  Either
> ahci_reset_port() does it (if a block device is attached) or it's called
> when the guest writes to the PORT_CMD register.
> 
> I'm not sure the latter works.  The signature doesn't seem to be set
> anywhere.
> 
> Any ideas?
> 

I've never been super clear on this, but the reset functions are called
during bringup and not (apparently) the result of explicit guest
interaction with the HBA or ICH9, and as far as I can tell we've always
been relying on it:

#0  0x0000555555828501 in ahci_reset (s=0x5555576b55a0) at
/home/bos/jhuston/src/qemu/hw/ide/ahci.c:1498
#1  0x0000555555828cb7 in pci_ich9_reset (dev=0x5555576b4d20) at
/home/bos/jhuston/src/qemu/hw/ide/ich.c:98
#2  0x00005555557dc0c4 in device_reset (dev=0x5555576b4d20) at
/home/bos/jhuston/src/qemu/hw/core/qdev.c:1269
#3  0x00005555557d996f in qdev_reset_one (dev=0x5555576b4d20,
opaque=0x0) at /home/bos/jhuston/src/qemu/hw/core/qdev.c:309
#4  0x00005555557da5b1 in qdev_walk_children (dev=0x5555576b4d20,
pre_devfn=0x0, pre_busfn=0x0, post_devfn=0x5555557d9953
<qdev_reset_one>, post_busfn=0x5555557d9976 <qbus_reset_one>,
opaque=0x0) at /home/bos/jhuston/src/qemu/hw/core/qdev.c:637
#5  0x00005555557da4a4 in qbus_walk_children (bus=0x555556626890,
pre_devfn=0x0, pre_busfn=0x0, post_devfn=0x5555557d9953
<qdev_reset_one>, post_busfn=0x5555557d9976 <qbus_reset_one>,
opaque=0x0) at /home/bos/jhuston/src/qemu/hw/core/qdev.c:595
#6  0x00005555557da575 in qdev_walk_children (dev=0x5555565cc5c0,
pre_devfn=0x0, pre_busfn=0x0, post_devfn=0x5555557d9953
<qdev_reset_one>, post_busfn=0x5555557d9976 <qbus_reset_one>,
opaque=0x0) at /home/bos/jhuston/src/qemu/hw/core/qdev.c:629
#7  0x00005555557da4a4 in qbus_walk_children (bus=0x555556451a30,
pre_devfn=0x0, pre_busfn=0x0, post_devfn=0x5555557d9953
<qdev_reset_one>, post_busfn=0x5555557d9976 <qbus_reset_one>,
opaque=0x0) at /home/bos/jhuston/src/qemu/hw/core/qdev.c:595
#8  0x00005555557d9a4b in qbus_reset_all (bus=0x555556451a30) at
/home/bos/jhuston/src/qemu/hw/core/qdev.c:330
#9  0x00005555557d9a6d in qbus_reset_all_fn (opaque=0x555556451a30) at
/home/bos/jhuston/src/qemu/hw/core/qdev.c:336
#10 0x0000555555749f81 in qemu_devices_reset () at
/home/bos/jhuston/src/qemu/vl.c:1703
#11 0x000055555574a01f in qemu_system_reset (report=false) at
/home/bos/jhuston/src/qemu/vl.c:1716
#12 0x00005555557524c3 in main (argc=10, argv=0x7fffffffdd88,
envp=0x7fffffffdde0) at /home/bos/jhuston/src/qemu/vl.c:4595

So on initial boot, we call ahci_init_d2h and set pr->sig, then call
ahci_write_fis_d2h. However, since the FIS RX engine (PxFRE) is off, we
don't actually generate the FIS because there's nowhere to store it.

Later, when the guest activates the PxFRE bit, we call init_d2h again
and succeed this time.

As the original comment notes, this is kind of hacky. I decided not to
interfere with this for now because "it works" and is reasonably close
to the real behavior.

As far as I can tell, what's /supposed/ to happen is this:

(A) "START is off, but we received the Hello from the device" (Start
being off implies that FRE /should/ also be off, but this is not required.)

P:Init --> P:NotRunning --> NDR:Entry --> NDR:Accept --> P:RegFisUpdate
--> [P:NotRunning / P:RegFisPostToMem]

i.e. receive the FIS, update PxSIG, then trash the FIS or, if FRE is on
for whatever reason, go ahead and post it to memory. If FRE is off,
stash the D2H Reg FIS in the FIFO queue.

(B) "START is on, and we received the Hello FIS from the device"

P:Init --> P:NotRunning --> P:Idle --> NDR:Entry --> NDR:Accept -->
RegFIS:Entry --> ... -->
RegFIS:UpdateSig --> RegFIS:SetSig --> PM:Aggr ... --> P:Idle

This is a normal path to encounter during a COMRESET, perhaps, but not
likely to occur in QEMU because the disk and the HBA are not really
independent. The Initial D2H Register FIS gets posted immediately as
normal, taking care to update the signature.

(C) "PxFRE was enabled, but there's a D2H Register FIS in the FIFO"

P:Init --> P:NotRunning --> P:RegFisPostToMem

i.e, the device said hello before the guest told us where we can store
the FIS (quite probable) so once the FRE bit is turned on, we reveal the
FIS and update the shadow registers here.




It looks like what should happen in QEMU, effectively, is:

(1) Generate the Initial FIS and update pr->sig regardless of the
current HBA state.
(2) Wait for the guest to enable PxFRE
(3) Send the initial FIS and update shadow registers.

What we do now is pretty close, with the exception of updating the
shadow registers whether or not PxFRE is enabled during the
ahci_port_reset phase, but it's been like that for quite some time.

--js

>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>> index bb6a92f..f352dd7 100644
>> --- a/hw/ide/ahci.c
>> +++ b/hw/ide/ahci.c
>> @@ -537,20 +537,31 @@ static void ahci_init_d2h(AHCIDevice *ad)
>>  {
>>      uint8_t init_fis[20];
>>      IDEState *ide_state = &ad->port.ifs[0];
>> +    AHCIPortRegs *pr = &ad->port_regs;
>>  
>>      memset(init_fis, 0, sizeof(init_fis));
>>  
>> -    init_fis[4] = 1;
>> -    init_fis[12] = 1;
>> -
>> -    if (ide_state->drive_kind == IDE_CD) {
>> -        init_fis[5] = ide_state->lcyl;
>> -        init_fis[6] = ide_state->hcyl;
>> -    }
>> +    /* We're emulating receiving the first Reg H2D Fis from the device;
>> +     * Update the SIG register, but otherwise procede as normal. */
>> +    pr->sig = (ide_state->hcyl << 24) |
>> +        (ide_state->lcyl << 16) |
>> +        (ide_state->sector << 8) |
>> +        (ide_state->nsector & 0xFF);
>>  
>>      ahci_write_fis_d2h(ad, init_fis);
>>  }
>>  
>> +static void ahci_set_signature(AHCIDevice *ad, uint32_t sig)
>> +{
>> +    IDEState *s = &ad->port.ifs[0];
>> +    s->hcyl = sig >> 24 & 0xFF;
>> +    s->lcyl = sig >> 16 & 0xFF;
>> +    s->sector = sig >> 8 & 0xFF;
>> +    s->nsector = sig & 0xFF;
>> +
>> +    DPRINTF(ad->port_no, "set hcyl:lcyl:sect:nsect = 0x%08x\n", sig);
>> +}
>> +
>>  static void ahci_reset_port(AHCIState *s, int port)
>>  {
>>      AHCIDevice *d = &s->dev[port];
>> @@ -600,16 +611,12 @@ static void ahci_reset_port(AHCIState *s, int port)
>>  
>>      s->dev[port].port_state = STATE_RUN;
>>      if (!ide_state->blk) {
>> -        pr->sig = 0;
>>          ide_state->status = SEEK_STAT | WRERR_STAT;
>>      } else if (ide_state->drive_kind == IDE_CD) {
>> -        pr->sig = SATA_SIGNATURE_CDROM;
>> -        ide_state->lcyl = 0x14;
>> -        ide_state->hcyl = 0xeb;
>> -        DPRINTF(port, "set lcyl = %d\n", ide_state->lcyl);
>> +        ahci_set_signature(d, SATA_SIGNATURE_CDROM);
>>          ide_state->status = SEEK_STAT | WRERR_STAT | READY_STAT;
>>      } else {
>> -        pr->sig = SATA_SIGNATURE_DISK;
>> +        ahci_set_signature(d, SATA_SIGNATURE_DISK);
>>          ide_state->status = SEEK_STAT | WRERR_STAT;
>>      }
>>  
>> -- 
>> 2.1.0
>>