Re: [PATCH] virtio: Define and use per-architecture "pfn shift" constants

Re: [PATCH] virtio: Define and use per-architecture "pfn shift" constants

[Hollis Blanchard]

On Wednesday 05 November 2008 22:49:42 Hollis Blanchard wrote:
> virtio: Define and use per-architecture "pfn shift" constants
> 
> Both sides of the virtio interface must agree about how big a pfn really is.
> This is particularly an issue on architectures where the page size is
> configurable (e.g. PowerPC, IA64) -- the interface must be independent of
> PAGE_SHIFT.

I forgot to mention that with this patch and the corresponding qemu patch, I 
can successfully use virtio with qemu TARGET_PAGE_BITS\x12 and Linux 
PAGE_SHIFT\x16. (OK, actually I have other problems once I get to userspace, 
but I don't believe it's a virtio issue.)

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [PATCH] virtio: Define and use per-architecture "pfn shift"

[Mark McLoughlin]

On Wed, 2008-11-05 at 22:49 -0600, Hollis Blanchard wrote:

> -       info->queue = kzalloc(PAGE_ALIGN(vring_size(num,PAGE_SIZE)), GFP_KERNEL);
> +       vring_bytes = PAGE_ALIGN(vring_size(num, VRING_PAGE_SIZE));
> +       info->queue = kzalloc(vring_bytes, GFP_KERNEL);

You're still aligning the size to PAGE_SIZE rather than VRING_PAGE_SIZE?

But actually, why do we align the size anyway?

Also might make sense for vring_init() and vring_size() not to take a
pagesize argument and hard-code them to use VRING_PAGE_SIZE.

Cheers,
Mark.

Re: [PATCH] virtio: Define and use per-architecture "pfn shift" constants

[Anthony Liguori]

Hollis Blanchard wrote:
> # HG changeset patch
> # User Hollis Blanchard <hollisb@us.ibm.com>
> # Date 1225946980 21600
> # Node ID f58566dfe20e841604e1377ff41e9e0501c1cf18
> # Parent  f776b102380286dd173a3b89f7dc976140812517
> virtio: Define and use per-architecture "pfn shift" constants
>
> Both sides of the virtio interface must agree about how big a pfn really is.
> This is particularly an issue on architectures where the page size is
> configurable (e.g. PowerPC, IA64) -- the interface must be independent of
> PAGE_SHIFT.
>
> This patch should have no functional effect on x86 or ia64, but an ack from the IA64 guys
> would be good because I'm not clear on their page size requirements.
>
> Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>
>   

Acked-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

Re: [PATCH] virtio: Define and use per-architecture "pfn shift"

[Hollis Blanchard]

On Thu, 2008-11-06 at 10:49 +0000, Mark McLoughlin wrote:
> On Wed, 2008-11-05 at 22:49 -0600, Hollis Blanchard wrote:
> 
> > -       info->queue = kzalloc(PAGE_ALIGN(vring_size(num,PAGE_SIZE)), GFP_KERNEL);
> > +       vring_bytes = PAGE_ALIGN(vring_size(num, VRING_PAGE_SIZE));
> > +       info->queue = kzalloc(vring_bytes, GFP_KERNEL);
> 
> You're still aligning the size to PAGE_SIZE rather than VRING_PAGE_SIZE?

The original code was calling kzalloc with a multiple of PAGE_SIZE, so I
kept that behavior here...

> But actually, why do we align the size anyway?

I assume it's so that the last page in the ring (containing the "used"
fields) could be safely mapped into another guest's address space,
without fear of exposing other data.

I don't know how valuable that is, but that's not really my concern so I
preserved the behavior.

> Also might make sense for vring_init() and vring_size() not to take a
> pagesize argument and hard-code them to use VRING_PAGE_SIZE.

I think that's a good idea. Anthony mentioned earlier the code was done
this way so it could be copied to userspace, where PAGE_SIZE is
unavailable, but that isn't an issue if we switch to VRING_PAGE_SIZE.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [PATCH] virtio: Define and use per-architecture "pfn shift" constants

[Anthony Liguori]

Hollis Blanchard wrote:
> On Thu, 2008-11-06 at 10:49 +0000, Mark McLoughlin wrote:
>   
>> But actually, why do we align the size anyway?
>>     
>
> I assume it's so that the last page in the ring (containing the "used"
> fields) could be safely mapped into another guest's address space,
> without fear of exposing other data.
>
> I don't know how valuable that is, but that's not really my concern so I
> preserved the behavior.
>
>   
>> Also might make sense for vring_init() and vring_size() not to take a
>> pagesize argument and hard-code them to use VRING_PAGE_SIZE.
>>     
>
> I think that's a good idea. Anthony mentioned earlier the code was done
> this way so it could be copied to userspace, where PAGE_SIZE is
> unavailable, but that isn't an issue if we switch to VRING_PAGE_SIZE.
>   

Agreed.

Regards,

Anthony Liguori

Re: [PATCH] virtio: Define and use per-architecture "pfn shift" constants

[Anthony Liguori]

Hollis Blanchard wrote:
> # HG changeset patch
> # User Hollis Blanchard <hollisb@us.ibm.com>
> # Date 1225946980 21600
> # Node ID f58566dfe20e841604e1377ff41e9e0501c1cf18
> # Parent  f776b102380286dd173a3b89f7dc976140812517
> virtio: Define and use per-architecture "pfn shift" constants
>
> Both sides of the virtio interface must agree about how big a pfn really is.
> This is particularly an issue on architectures where the page size is
> configurable (e.g. PowerPC, IA64) -- the interface must be independent of
> PAGE_SHIFT.
>
> This patch should have no functional effect on x86 or ia64, but an ack from the IA64 guys
> would be good because I'm not clear on their page size requirements.
>
> Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>
>
>   

This is missing a fix for virtio-balloon.

Regards,

Anthony Liguori