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

[Stefan Hajnoczi]

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

pgpylNg6KLBR3.pgp
Description: PGP signature