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

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

[Hollis Blanchard]

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

diff --git a/arch/ia64/include/asm/virtio.h b/arch/ia64/include/asm/virtio.h
new file mode 100644
--- /dev/null
+++ b/arch/ia64/include/asm/virtio.h
@@ -0,0 +1,25 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 2, as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ *
+ * Copyright IBM Corp. 2008
+ *
+ * Authors: Hollis Blanchard <hollisb@us.ibm.com>
+ */
+
+#ifndef __ASM_VIRTIO_H__
+#define __ASM_VIRTIO_H__
+
+#define VIRTIO_PFN_SHIFT 16
+
+#endif /* __ASM_VIRTIO_H__ */
diff --git a/arch/powerpc/include/asm/virtio.h b/arch/powerpc/include/asm/virtio.h
new file mode 100644
--- /dev/null
+++ b/arch/powerpc/include/asm/virtio.h
@@ -0,0 +1,25 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 2, as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ *
+ * Copyright IBM Corp. 2008
+ *
+ * Authors: Hollis Blanchard <hollisb@us.ibm.com>
+ */
+
+#ifndef __ASM_VIRTIO_H__
+#define __ASM_VIRTIO_H__
+
+#define VIRTIO_PFN_SHIFT 10
+
+#endif /* __ASM_VIRTIO_H__ */
diff --git a/arch/x86/include/asm/virtio.h b/arch/x86/include/asm/virtio.h
new file mode 100644
--- /dev/null
+++ b/arch/x86/include/asm/virtio.h
@@ -0,0 +1,25 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 2, as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ *
+ * Copyright IBM Corp. 2008
+ *
+ * Authors: Hollis Blanchard <hollisb@us.ibm.com>
+ */
+
+#ifndef __ASM_VIRTIO_H__
+#define __ASM_VIRTIO_H__
+
+#define VIRTIO_PFN_SHIFT 12
+
+#endif /* __ASM_VIRTIO_H__ */
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -24,6 +24,8 @@
 #include <linux/virtio_pci.h>
 #include <linux/highmem.h>
 #include <linux/spinlock.h>
+
+#include <asm/virtio.h>
 
 MODULE_AUTHOR("Anthony Liguori <aliguori@us.ibm.com>");
 MODULE_DESCRIPTION("virtio-pci");
@@ -216,6 +218,7 @@ static struct virtqueue *vp_find_vq(stru
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	struct virtio_pci_vq_info *info;
 	struct virtqueue *vq;
+	unsigned long vring_bytes;
 	unsigned long flags;
 	u16 num;
 	int err;
@@ -237,14 +240,15 @@ static struct virtqueue *vp_find_vq(stru
 	info->queue_index = index;
 	info->num = num;
 
-	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);
 	if (info->queue == NULL) {
 		err = -ENOMEM;
 		goto out_info;
 	}
 
 	/* activate the queue */
-	iowrite32(virt_to_phys(info->queue) >> PAGE_SHIFT,
+	iowrite32(virt_to_phys(info->queue) >> VIRTIO_PFN_SHIFT,
 		  vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN);
 
 	/* create the vring */
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -292,7 +292,7 @@ struct virtqueue *vring_new_virtqueue(un
 	if (!vq)
 		return NULL;
 
-	vring_init(&vq->vring, num, pages, PAGE_SIZE);
+	vring_init(&vq->vring, num, pages, VRING_PAGE_SIZE);
 	vq->vq.callback = callback;
 	vq->vq.vdev = vdev;
 	vq->vq.vq_ops = &vring_vq_ops;
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -102,6 +102,8 @@ static inline void vring_init(struct vri
 			    & ~(pagesize - 1));
 }
 
+#define VRING_PAGE_SIZE (1<<12)
+
 static inline unsigned vring_size(unsigned int num, unsigned long pagesize)
 {
 	return ((sizeof(struct vring_desc) * num + sizeof(__u16) * (2 + num)

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=12 and Linux 
PAGE_SHIFT=16. (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" constants

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

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[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


--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Anthony Liguori]

Hollis Blanchard wrote:
> # HG changeset patch
> # User Hollis Blanchard <hollisb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> # 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-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
>
>   

This is missing a fix for virtio-balloon.

Regards,

Anthony Liguori


--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html