| Message ID | 20170324231943.18768-1-Andrew.Baumann@microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 25.03.2017 um 00:19 schrieb Andrew Baumann: > long is 32-bits on 64-bit windows, which caused the top half of the > address to be truncated; this patch changes it to use the > QEMU_ALIGN_UP macro which does not suffer the same problem > > Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > --- > include/hw/virtio/virtio.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > index 15efcf2..7b6edba 100644 > --- a/include/hw/virtio/virtio.h > +++ b/include/hw/virtio/virtio.h > @@ -34,7 +34,7 @@ struct VirtQueue; > static inline hwaddr vring_align(hwaddr addr, > unsigned long align) > { > - return (addr + align - 1) & ~(align - 1); > + return QEMU_ALIGN_UP(addr, align); > } > > typedef struct VirtQueue VirtQueue; > Eric added "for-2.9" to the subject line of v2, but now it was missing again for v3. Is this needed for 2.9? I wonder why I never before noticed a problem or got a bug report for this issue. Regards Stefan
On 03/28/2017 01:38 PM, Stefan Weil wrote: > Am 25.03.2017 um 00:19 schrieb Andrew Baumann: >> long is 32-bits on 64-bit windows, which caused the top half of the >> address to be truncated; this patch changes it to use the >> QEMU_ALIGN_UP macro which does not suffer the same problem >> >> Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> --- > Eric added "for-2.9" to the subject line of v2, but now it was > missing again for v3. > > Is this needed for 2.9? Yes, it's a correctness bug that avoids miscompilation on 64-bit targets where long is 32 bits (which, at the moment, is really just Windows). > I wonder why I never before noticed > a problem or got a bug report for this issue. Probably because so few people are testing on native Windows, and it doesn't affect other platforms.
> From: Eric Blake [mailto:eblake@redhat.com] > Sent: Tuesday, 28 March 2017 11:52 > > On 03/28/2017 01:38 PM, Stefan Weil wrote: > > Am 25.03.2017 um 00:19 schrieb Andrew Baumann: > >> long is 32-bits on 64-bit windows, which caused the top half of the > >> address to be truncated; this patch changes it to use the > >> QEMU_ALIGN_UP macro which does not suffer the same problem > >> > >> Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com> > >> Reviewed-by: Eric Blake <eblake@redhat.com> > >> --- > > > Eric added "for-2.9" to the subject line of v2, but now it was > > missing again for v3. > > > > Is this needed for 2.9? > > Yes, it's a correctness bug that avoids miscompilation on 64-bit targets > where long is 32 bits (which, at the moment, is really just Windows). I agree, this should be in 2.9. I dropped the tag by accident. > > I wonder why I never before noticed > > a problem or got a bug report for this issue. > > Probably because so few people are testing on native Windows, and it > doesn't affect other platforms. In addition to that, you only notice it on virtio devices mapped above the 32-bit limit... Andrew
Hi, I never received Andrew Baumann's mail via the ML... On 03/28/2017 03:38 PM, Stefan Weil wrote: > Am 25.03.2017 um 00:19 schrieb QEMU_ALIGN_UP: >> long is 32-bits on 64-bit windows, which caused the top half of the >> address to be truncated; this patch changes it to use the >> QEMU_ALIGN_UP macro which does not suffer the same problem >> >> Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> include/hw/virtio/virtio.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h >> index 15efcf2..7b6edba 100644 >> --- a/include/hw/virtio/virtio.h >> +++ b/include/hw/virtio/virtio.h >> @@ -34,7 +34,7 @@ struct VirtQueue; >> static inline hwaddr vring_align(hwaddr addr, >> unsigned long align) >> { >> - return (addr + align - 1) & ~(align - 1); >> + return QEMU_ALIGN_UP(addr, align); >> } >> >> typedef struct VirtQueue VirtQueue; >> > > Eric added "for-2.9" to the subject line of v2, but now it was > missing again for v3. > > Is this needed for 2.9? I wonder why I never before noticed > a problem or got a bug report for this issue. > > Regards > Stefan > >
Am 28.03.2017 um 20:56 schrieb Andrew Baumann: >> From: Eric Blake [mailto:eblake@redhat.com] >>> Is this needed for 2.9? >> >> Yes, it's a correctness bug that avoids miscompilation on 64-bit targets >> where long is 32 bits (which, at the moment, is really just Windows). > > I agree, this should be in 2.9. I dropped the tag by accident. > >>> I wonder why I never before noticed >>> a problem or got a bug report for this issue. >> >> Probably because so few people are testing on native Windows, and it >> doesn't affect other platforms. > > In addition to that, you only notice it on virtio devices mapped above the 32-bit limit... I think that is the reason why most people don't get that problem. I also think that only a few people are testing on Windows, but there seem to be more people than expected who simply use it. Most of them will never complain when they have a problem, but sometimes I also get e-mails which report an issue. By the way: I expect that more Windows users will be attracted as soon as the HAXM acceleration works better (Intel is just preparing a new HAXM version which fixes CPUID, something which was reported to me by a Windows user). Stefan
Am 28.03.2017 um 20:56 schrieb Andrew Baumann: >> From: Eric Blake [mailto:eblake@redhat.com] >> Sent: Tuesday, 28 March 2017 11:52 >> >> On 03/28/2017 01:38 PM, Stefan Weil wrote: >>> Am 25.03.2017 um 00:19 schrieb Andrew Baumann: >>>> long is 32-bits on 64-bit windows, which caused the top half of the >>>> address to be truncated; this patch changes it to use the >>>> QEMU_ALIGN_UP macro which does not suffer the same problem >>>> >>>> Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com> >>>> Reviewed-by: Eric Blake <eblake@redhat.com> >>>> --- >> >>> Eric added "for-2.9" to the subject line of v2, but now it was >>> missing again for v3. >>> >>> Is this needed for 2.9? >> >> Yes, it's a correctness bug that avoids miscompilation on 64-bit targets >> where long is 32 bits (which, at the moment, is really just Windows). > > I agree, this should be in 2.9. I dropped the tag by accident. > >>> I wonder why I never before noticed >>> a problem or got a bug report for this issue. >> >> Probably because so few people are testing on native Windows, and it >> doesn't affect other platforms. > > In addition to that, you only notice it on virtio devices mapped above the 32-bit limit... > > Andrew > Reviewed-by: Stefan Weil <sw@weilnetz.de> I added this patch to my queue. Peter, do you still accept pull requests for 2.9? I'm still waiting for a review of another bug fix for Windows (http://patchwork.ozlabs.org/patch/743416/). How long do I have time to get bug fixes for Windows into 2.9? Of course I would not mind if you pulled this one directly (see http://patchwork.ozlabs.org/patch/743410/). Stefan
On Fri, Mar 24, 2017 at 04:19:43PM -0700, Andrew Baumann wrote: > long is 32-bits on 64-bit windows, which caused the top half of the > address to be truncated; this patch changes it to use the > QEMU_ALIGN_UP macro which does not suffer the same problem > > Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com> > Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > --- > include/hw/virtio/virtio.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > index 15efcf2..7b6edba 100644 > --- a/include/hw/virtio/virtio.h > +++ b/include/hw/virtio/virtio.h > @@ -34,7 +34,7 @@ struct VirtQueue; > static inline hwaddr vring_align(hwaddr addr, > unsigned long align) > { > - return (addr + align - 1) & ~(align - 1); > + return QEMU_ALIGN_UP(addr, align); > } > > typedef struct VirtQueue VirtQueue; > -- > 2.8.3
On Tue, Mar 28, 2017 at 10:02:10PM +0200, Stefan Weil wrote: > Am 28.03.2017 um 20:56 schrieb Andrew Baumann: > > > From: Eric Blake [mailto:eblake@redhat.com] > > > Sent: Tuesday, 28 March 2017 11:52 > > > > > > On 03/28/2017 01:38 PM, Stefan Weil wrote: > > > > Am 25.03.2017 um 00:19 schrieb Andrew Baumann: > > > > > long is 32-bits on 64-bit windows, which caused the top half of the > > > > > address to be truncated; this patch changes it to use the > > > > > QEMU_ALIGN_UP macro which does not suffer the same problem > > > > > > > > > > Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com> > > > > > Reviewed-by: Eric Blake <eblake@redhat.com> > > > > > --- > > > > > > > Eric added "for-2.9" to the subject line of v2, but now it was > > > > missing again for v3. > > > > > > > > Is this needed for 2.9? > > > > > > Yes, it's a correctness bug that avoids miscompilation on 64-bit targets > > > where long is 32 bits (which, at the moment, is really just Windows). > > > > I agree, this should be in 2.9. I dropped the tag by accident. > > > > > > I wonder why I never before noticed > > > > a problem or got a bug report for this issue. > > > > > > Probably because so few people are testing on native Windows, and it > > > doesn't affect other platforms. > > > > In addition to that, you only notice it on virtio devices mapped above the 32-bit limit... > > > > Andrew > > > > Reviewed-by: Stefan Weil <sw@weilnetz.de> > > I added this patch to my queue. Peter, do you still accept pull requests > for 2.9? I'm still waiting for a review of another bug fix for Windows > (http://patchwork.ozlabs.org/patch/743416/). How long do I have time > to get bug fixes for Windows into 2.9? > > Of course I would not mind if you pulled this one directly (see > http://patchwork.ozlabs.org/patch/743410/). > > Stefan I'm doing a pull request a bit later today - I can pick this one up if you prefer. If yes, pls send your ack.
Am 28.03.2017 um 22:11 schrieb Michael S. Tsirkin: > I'm doing a pull request a bit later today - I can pick this one up > if you prefer. If yes, pls send your ack. > Yes, please. Thanks a lot Stefan
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index 15efcf2..7b6edba 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -34,7 +34,7 @@ struct VirtQueue; static inline hwaddr vring_align(hwaddr addr, unsigned long align) { - return (addr + align - 1) & ~(align - 1); + return QEMU_ALIGN_UP(addr, align); } typedef struct VirtQueue VirtQueue;