Re: [Qemu-devel] [PATCH] ahci: advertise HOST_CAP_64

[Ladi Prosek]

On Fri, Jan 13, 2017 at 8:15 PM, John Snow <address@hidden> wrote:
>
>
> On 01/13/2017 02:01 PM, Ladi Prosek wrote:
>> On Fri, Jan 13, 2017 at 7:31 PM, John Snow <address@hidden> wrote:
>>>
>>>
>>> On 01/13/2017 01:12 PM, Ladi Prosek wrote:
>>>> On Fri, Jan 13, 2017 at 6:23 PM, John Snow <address@hidden> wrote:
>>>>> On 01/13/2017 06:02 AM, Ladi Prosek wrote:
>>>>>> The AHCI emulation code supports 64-bit addressing and should advertise 
>>>>>> this
>>>>>> fact in the Host Capabilities register. Both Linux and Windows drivers 
>>>>>> test
>>>>>> this bit to decide if the upper 32 bits of various registers may be 
>>>>>> written
>>>>>> to, and at least some versions of Windows have a bug where DMA is 
>>>>>> attempted
>>>>>> with an address above 4GB but, in the absence of HOST_CAP_64, the upper 
>>>>>> 32
>>>>>> bits are left unititialized which leads to a memory corruption.
>>>>>>
>>>>>> Signed-off-by: Ladi Prosek <address@hidden>
>>>>>> ---
>>>>>>  hw/ide/ahci.c | 2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>>>>>> index 3c19bda..6a17acf 100644
>>>>>> --- a/hw/ide/ahci.c
>>>>>> +++ b/hw/ide/ahci.c
>>>>>> @@ -488,7 +488,7 @@ static void ahci_reg_init(AHCIState *s)
>>>>>>      s->control_regs.cap = (s->ports - 1) |
>>>>>>                            (AHCI_NUM_COMMAND_SLOTS << 8) |
>>>>>>                            (AHCI_SUPPORTED_SPEED_GEN1 << 
>>>>>> AHCI_SUPPORTED_SPEED) |
>>>>>> -                          HOST_CAP_NCQ | HOST_CAP_AHCI;
>>>>>> +                          HOST_CAP_NCQ | HOST_CAP_AHCI | HOST_CAP_64;
>>>>>>
>>>>>>      s->control_regs.impl = (1 << s->ports) - 1;
>>>>>>
>>>>>>
>>>>>
>>>>> Was this tested? What was the use case that prompted this patch, and do
>>>>> you have a public bugzilla to point to?
>>>>
>>>> Sorry, I thought you were aware. Here's the BZ with details:
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1411105
>>>>
>>>
>>> It's for the public record :)
>>>
>>> I'm going to amend your commit message, OK?
>>>
>>> https://github.com/jnsnow/qemu/commit/f037be92fc0c4580b846134e0355a69d0eccd0d9
>>
>> Minor nit: the OS we know for sure is affected is Windows Server 2008,
>> without the "R2". Thanks!
>>
>
> I misunderstood. It looked like the initial report was for "SP2," though
> I see you saying that the R2 version actually worked okay, but by
> coincidence.
>
> I didn't think there *was* an "SP2," for Windows Server, so I
> interpreted that to mean R2.
>
> So this only manifests with vanilla 2008?

We know it manifests on 2008 SP2, which is very different from 2008
R2. 2008 is the server variant of Vista, 2008 R2 is the server variant
of Win7 (yay for marketing names!)

I believe that 2008 SP2 32-bit is the only OS where it's been confirmed so far.

>>>> In short, Windows guests run into issues in certain virtual HW
>>>> configurations if the bit is not set. I have tested the patch and
>>>> verified that it fixes the failing cases.
>>>>
>>>
>>> Great. I'm CCing qemu-stable as this should probably be included in
>>> 2.7.2 / 2.8.1 / etc.
>>>
>>>>> It looks correct via the spec, thank you for finding this oversight. It
>>>>> looks like everywhere this bit would make a difference we already do the
>>>>> correct thing for having this bit set.
>>>>>
>>>>> From what I can gather from the spec, it looks as if even 32bit guests
>>>>> must ensure that the upper 32bit registers are cleared, so I think we're
>>>>> all set here.
>>>>>
>>>>> Reviewed-by: John Snow <address@hidden>
>>>
>>> Thanks, applied to my IDE tree:
>>>
>>> https://github.com/jnsnow/qemu/commits/ide
>>> https://github.com/jnsnow/qemu.git
>>>
>>> --js