Re: [Qemu-devel] [PATCH v3 for-2.9] virtio: fix vring_align() on 64-bit windows

[Stefan Weil]

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

Re: [Qemu-devel] [PATCH v3 for-2.9] virtio: fix vring_align() on 64-bit windows

[Michael S. Tsirkin]

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:ebl...@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 
> > > > > Reviewed-by: Eric Blake 
> > > > > ---
> > > 
> > > > 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 
> 
> 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.

-- 
MST

Re: [Qemu-devel] [PATCH v3 for-2.9] virtio: fix vring_align() on 64-bit windows

[Stefan Weil]

Am 28.03.2017 um 20:56 schrieb Andrew Baumann:

From: Eric Blake [mailto:ebl...@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 
Reviewed-by: Eric Blake 
---



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 

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

Re: [Qemu-devel] [PATCH v3 for-2.9?] virtio: fix vring_align() on 64-bit windows

[Stefan Weil]

Am 28.03.2017 um 20:56 schrieb Andrew Baumann:

From: Eric Blake [mailto:ebl...@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

Re: [Qemu-devel] [PATCH v3 for-2.9?] virtio: fix vring_align() on 64-bit windows

[Philippe Mathieu-Daudé]

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 
Reviewed-by: Eric Blake 


Reviewed-by: Philippe Mathieu-Daudé 


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

Re: [Qemu-devel] [PATCH v3 for-2.9?] virtio: fix vring_align() on 64-bit windows

[Andrew Baumann via Qemu-devel]

> From: Eric Blake [mailto:ebl...@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 
> >> Reviewed-by: Eric Blake 
> >> ---
> 
> > 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

Re: [Qemu-devel] [PATCH v3 for-2.9?] virtio: fix vring_align() on 64-bit windows

[Eric Blake]

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 
>> Reviewed-by: Eric Blake 
>> ---

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

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v3 for-2.9?] virtio: fix vring_align() on 64-bit windows

[Stefan Weil]

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 
Reviewed-by: Eric Blake 
---
 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