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

[Ladi Prosek]

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

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.

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