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

[John Snow <jsnow@redhat.com>]

On 01/13/2017 01:12 PM, Ladi Prosek wrote:
> On Fri, Jan 13, 2017 at 6:23 PM, John Snow <jsnow@redhat.com> 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 <lprosek@redhat.com>
>>> ---
>>>  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

> 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 <jsnow@redhat.com>

Thanks, applied to my IDE tree:

https://github.com/jnsnow/qemu/commits/ide
https://github.com/jnsnow/qemu.git

--js