* [Qemu-devel] [PATCH] ahci: advertise HOST_CAP_64 @ 2017-01-13 11:02 Ladi Prosek 2017-01-13 17:23 ` John Snow 0 siblings, 1 reply; 9+ messages in thread From: Ladi Prosek @ 2017-01-13 11:02 UTC (permalink / raw) To: qemu-block, qemu-devel; +Cc: jsnow 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; -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] ahci: advertise HOST_CAP_64 2017-01-13 11:02 [Qemu-devel] [PATCH] ahci: advertise HOST_CAP_64 Ladi Prosek @ 2017-01-13 17:23 ` John Snow 2017-01-13 18:12 ` Ladi Prosek 0 siblings, 1 reply; 9+ messages in thread From: John Snow @ 2017-01-13 17:23 UTC (permalink / raw) To: Ladi Prosek, qemu-block, qemu-devel 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? 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> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] ahci: advertise HOST_CAP_64 2017-01-13 17:23 ` John Snow @ 2017-01-13 18:12 ` Ladi Prosek 2017-01-13 18:31 ` John Snow 0 siblings, 1 reply; 9+ messages in thread From: Ladi Prosek @ 2017-01-13 18:12 UTC (permalink / raw) To: John Snow; +Cc: qemu-block, qemu-devel 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 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 <jsnow@redhat.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] ahci: advertise HOST_CAP_64 2017-01-13 18:12 ` Ladi Prosek @ 2017-01-13 18:31 ` John Snow 2017-01-13 19:01 ` Ladi Prosek 0 siblings, 1 reply; 9+ messages in thread From: John Snow @ 2017-01-13 18:31 UTC (permalink / raw) To: Ladi Prosek; +Cc: qemu-block, qemu-devel, qemu-stable 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] ahci: advertise HOST_CAP_64 2017-01-13 18:31 ` John Snow @ 2017-01-13 19:01 ` Ladi Prosek 2017-01-13 19:15 ` John Snow 0 siblings, 1 reply; 9+ messages in thread From: Ladi Prosek @ 2017-01-13 19:01 UTC (permalink / raw) To: John Snow; +Cc: qemu-block, qemu-devel, qemu-stable On Fri, Jan 13, 2017 at 7:31 PM, John Snow <jsnow@redhat.com> wrote: > > > 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 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 <jsnow@redhat.com> > > Thanks, applied to my IDE tree: > > https://github.com/jnsnow/qemu/commits/ide > https://github.com/jnsnow/qemu.git > > --js ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] ahci: advertise HOST_CAP_64 2017-01-13 19:01 ` Ladi Prosek @ 2017-01-13 19:15 ` John Snow 2017-01-16 7:49 ` Ladi Prosek 0 siblings, 1 reply; 9+ messages in thread From: John Snow @ 2017-01-13 19:15 UTC (permalink / raw) To: Ladi Prosek; +Cc: qemu-block, qemu-devel, qemu-stable On 01/13/2017 02:01 PM, Ladi Prosek wrote: > On Fri, Jan 13, 2017 at 7:31 PM, John Snow <jsnow@redhat.com> wrote: >> >> >> 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 > > 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? >>> 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] ahci: advertise HOST_CAP_64 2017-01-13 19:15 ` John Snow @ 2017-01-16 7:49 ` Ladi Prosek 2017-01-31 12:56 ` John Snow 0 siblings, 1 reply; 9+ messages in thread From: Ladi Prosek @ 2017-01-16 7:49 UTC (permalink / raw) To: John Snow; +Cc: qemu-block, qemu-devel, qemu-stable On Fri, Jan 13, 2017 at 8:15 PM, John Snow <jsnow@redhat.com> wrote: > > > On 01/13/2017 02:01 PM, Ladi Prosek wrote: >> On Fri, Jan 13, 2017 at 7:31 PM, John Snow <jsnow@redhat.com> wrote: >>> >>> >>> 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 >> >> 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. >>>> 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] ahci: advertise HOST_CAP_64 2017-01-16 7:49 ` Ladi Prosek @ 2017-01-31 12:56 ` John Snow 2017-01-31 13:02 ` Ladi Prosek 0 siblings, 1 reply; 9+ messages in thread From: John Snow @ 2017-01-31 12:56 UTC (permalink / raw) To: Ladi Prosek; +Cc: qemu-block, qemu-devel, qemu-stable On 01/16/2017 02:49 AM, Ladi Prosek wrote: > On Fri, Jan 13, 2017 at 8:15 PM, John Snow <jsnow@redhat.com> wrote: >> >> >> On 01/13/2017 02:01 PM, Ladi Prosek wrote: >>> On Fri, Jan 13, 2017 at 7:31 PM, John Snow <jsnow@redhat.com> wrote: >>>> >>>> >>>> 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 >>> >>> 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] ahci: advertise HOST_CAP_64 2017-01-31 12:56 ` John Snow @ 2017-01-31 13:02 ` Ladi Prosek 0 siblings, 0 replies; 9+ messages in thread From: Ladi Prosek @ 2017-01-31 13:02 UTC (permalink / raw) To: John Snow; +Cc: qemu-block, qemu-devel, qemu-stable On Tue, Jan 31, 2017 at 1:56 PM, John Snow <jsnow@redhat.com> wrote: > > > On 01/16/2017 02:49 AM, Ladi Prosek wrote: >> On Fri, Jan 13, 2017 at 8:15 PM, John Snow <jsnow@redhat.com> wrote: >>> >>> >>> On 01/13/2017 02:01 PM, Ladi Prosek wrote: >>>> On Fri, Jan 13, 2017 at 7:31 PM, John Snow <jsnow@redhat.com> wrote: >>>>> >>>>> >>>>> 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 >>>> >>>> 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-01-31 13:03 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-01-13 11:02 [Qemu-devel] [PATCH] ahci: advertise HOST_CAP_64 Ladi Prosek 2017-01-13 17:23 ` John Snow 2017-01-13 18:12 ` Ladi Prosek 2017-01-13 18:31 ` John Snow 2017-01-13 19:01 ` Ladi Prosek 2017-01-13 19:15 ` John Snow 2017-01-16 7:49 ` Ladi Prosek 2017-01-31 12:56 ` John Snow 2017-01-31 13:02 ` Ladi Prosek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).