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

[Ladi Prosek]

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!

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