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

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

[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;
-- 
2.8.3

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

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 993 bytes --]

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.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[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

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

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

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

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

[Michael S. Tsirkin]

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

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

-- 
MST

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