[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/2] ahci: fix signature generation
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH 2/2] ahci: fix signature generation |
Date: |
Wed, 8 Jul 2015 13:56:03 +0100 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
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.
pgpH656FUXv8H.pgp
Description: PGP signature