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

[John Snow]

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.

--js