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 <js...@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.
pgpWFxkL30_7K.pgp
Description: PGP signature
