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

[Ladi Prosek]

On Tue, Jan 31, 2017 at 1:56 PM, John Snow <address@hidden> wrote:
>
>
> On 01/16/2017 02:49 AM, Ladi Prosek wrote:
>> 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.
>>
>
> Thanks for clearing that up for me, I re-edited the commit message
> upstream. If it looks OK, I'll likely merge this when I get back home
> after FOSDEM.

Looks good, thanks!

> --js