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