[Qemu-devel] [PATCH 0/2] ahci: Fix CD-ROM signature

[Qemu-devel] [PATCH 0/2] ahci: Fix CD-ROM signature

[John Snow]

As reported by Hannes Reinecke, the signature generation
in AHCI is a little goofy. Let's fix it up.

Patch 1 should be considered a bugfix for 2.4.
Patch 2 is mostly a tidying effort prompted by the first patch.

________________________________________________________________________________

For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch ahci-sig
https://github.com/jnsnow/qemu/tree/ahci-sig

This version is tagged ahci-sig-v1:
https://github.com/jnsnow/qemu/releases/tag/ahci-sig-v1

Hannes Reinecke (1):
  ahci: Fix CD-ROM signature

John Snow (1):
  ahci: fix signature generation

 hw/ide/ahci.c | 33 ++++++++++++++++++++-------------
 hw/ide/ahci.h |  2 +-
 2 files changed, 21 insertions(+), 14 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/2] ahci: Fix CD-ROM signature

[John Snow]

From: Hannes Reinecke <hare@suse.de>

The CD-ROM signature is 0xeb140101, not 0xeb140000.
Without this change OVMF/Duet runs into a timeout trying
to detect a SATA cdrom.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index 9f5b4d2..68d5074 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -166,7 +166,7 @@
 #define AHCI_CMD_HDR_CMD_FIS_LEN           0x1f
 #define AHCI_CMD_HDR_PRDT_LEN              16
 
-#define SATA_SIGNATURE_CDROM               0xeb140000
+#define SATA_SIGNATURE_CDROM               0xeb140101
 #define SATA_SIGNATURE_DISK                0x00000101
 
 #define AHCI_GENERIC_HOST_CONTROL_REGS_MAX_ADDR 0x20
-- 
2.1.0

Re: [Qemu-devel] [PATCH 1/2] ahci: Fix CD-ROM signature

[John Snow]

On 07/06/2015 05:49 PM, John Snow wrote:
> From: Hannes Reinecke <hare@suse.de>
> 
> The CD-ROM signature is 0xeb140101, not 0xeb140000.
> Without this change OVMF/Duet runs into a timeout trying
> to detect a SATA cdrom.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  hw/ide/ahci.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
> index 9f5b4d2..68d5074 100644
> --- a/hw/ide/ahci.h
> +++ b/hw/ide/ahci.h
> @@ -166,7 +166,7 @@
>  #define AHCI_CMD_HDR_CMD_FIS_LEN           0x1f
>  #define AHCI_CMD_HDR_PRDT_LEN              16
>  
> -#define SATA_SIGNATURE_CDROM               0xeb140000
> +#define SATA_SIGNATURE_CDROM               0xeb140101
>  #define SATA_SIGNATURE_DISK                0x00000101
>  
>  #define AHCI_GENERIC_HOST_CONTROL_REGS_MAX_ADDR 0x20
> 

FWIW for review purposes, this is based on ATA8 AC3, Table 184 "Device
Signatures for Normal Output" and is very straightforward.

For how the component fields there (LBA and Count) become a single 4
byte signature, see AHCI 1.3 section 3.3.9 (PxSIG) and this is the value
we are emulating here with the #define.

I gave this patch a soft ACK in the past, but it feels wrong to give it
an R-B when I'm sending it out myself :)

--js

Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] ahci: Fix CD-ROM signature

[Kevin Wolf]

Am 07.07.2015 um 19:19 hat John Snow geschrieben:
> 
> 
> On 07/06/2015 05:49 PM, John Snow wrote:
> > From: Hannes Reinecke <hare@suse.de>
> > 
> > The CD-ROM signature is 0xeb140101, not 0xeb140000.
> > Without this change OVMF/Duet runs into a timeout trying
> > to detect a SATA cdrom.
> > 
> > Signed-off-by: Hannes Reinecke <hare@suse.de>
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  hw/ide/ahci.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
> > index 9f5b4d2..68d5074 100644
> > --- a/hw/ide/ahci.h
> > +++ b/hw/ide/ahci.h
> > @@ -166,7 +166,7 @@
> >  #define AHCI_CMD_HDR_CMD_FIS_LEN           0x1f
> >  #define AHCI_CMD_HDR_PRDT_LEN              16
> >  
> > -#define SATA_SIGNATURE_CDROM               0xeb140000
> > +#define SATA_SIGNATURE_CDROM               0xeb140101
> >  #define SATA_SIGNATURE_DISK                0x00000101
> >  
> >  #define AHCI_GENERIC_HOST_CONTROL_REGS_MAX_ADDR 0x20
> > 
> 
> FWIW for review purposes, this is based on ATA8 AC3, Table 184 "Device
> Signatures for Normal Output" and is very straightforward.

It is. The only thing that confuses me about this is that I thought it
had been fixed for a while already. My AHCI driver has a comment "Broken
value in qemu < 2.2" at the workaround, and I seem to remember that I
didn't need it for a newer qemu version indeed. Strange. Or perhaps I
only fixed it locally and then forgot to send the patch?

Anyway, the fix is obviously correct:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

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

[John Snow]

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 <jsnow@redhat.com>
---
 hw/ide/ahci.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

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

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3166 bytes --]

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 <jsnow@redhat.com>
> ---
>  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?

> 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
> 

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

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 <jsnow@redhat.com>
>> ---
>>  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
>>

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2112 bytes --]

On Tue, Jul 07, 2015 at 01:15:02PM -0400, John Snow wrote:
> 
> 
> 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 <jsnow@redhat.com>
> >> ---
> >>  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?
...
> 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.

My question is about the ide_state->blk == NULL case:

ahci_reset_port() is contradictory:

static void ahci_reset_port(AHCIState *s, int port)
{
    ...
    ide_state = &s->dev[port].port.ifs[0];
    if (!ide_state->blk) {
        return;
    }

    ...

    s->dev[port].port_state = STATE_RUN;
    if (!ide_state->blk) { <-- deadcode?
        pr->sig = 0;
        ide_state->status = SEEK_STAT | WRERR_STAT;
    }

Does code after the first "if (!ide_state->blk)" in ahci_reset_port()
ever execute in a drive hotplug scenario?

If it doesn't execute then sig is never filled in.

Your patch does not include a regression but either something is broken
here or I don't understand the code.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

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

[John Snow]

On 07/08/2015 08:56 AM, Stefan Hajnoczi wrote:
> On Tue, Jul 07, 2015 at 01:15:02PM -0400, John Snow wrote:
>>
>>
>> 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 <jsnow@redhat.com>
>>>> ---
>>>>  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?
> ...
>> 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.
> 
> My question is about the ide_state->blk == NULL case:
> 
> ahci_reset_port() is contradictory:
> 
> static void ahci_reset_port(AHCIState *s, int port)
> {
>     ...
>     ide_state = &s->dev[port].port.ifs[0];
>     if (!ide_state->blk) {
>         return;
>     }
> 
>     ...
> 
>     s->dev[port].port_state = STATE_RUN;
>     if (!ide_state->blk) { <-- deadcode?
>         pr->sig = 0;
>         ide_state->status = SEEK_STAT | WRERR_STAT;
>     }
> 
> Does code after the first "if (!ide_state->blk)" in ahci_reset_port()
> ever execute in a drive hotplug scenario?
> 
> If it doesn't execute then sig is never filled in.
> 
> Your patch does not include a regression but either something is broken
> here or I don't understand the code.
> 

I'm sorry, I misunderstood you...

Haven't really played with the hotplugging much so I don't know it to
work. I'll throw it on the list.

Actually, since I need to start focusing on non-legacy devices, I'll
start a wiki page of all the bugs and quirks I know about and that way
if I forget to get back to it (or my plane disappears over the Bermuda
Triangle) my understanding of existing problems will be documented.

I'll stage patch #1 here (Reviewed by Kevin) for inclusion in 2.4, #2 is
also a bug fix but it's more subtle and isn't known to break anything
and could possibly benefit from a more comprehensive fix so I'll leave
it for now.

Thanks,
--js