Re: [Qemu-block] [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 <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.
> 

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