Re: [PATCH] [v2] linux: virtio: Standardize virtio's concept of "page size"

Re: [PATCH] [v2] linux: virtio: Standardize virtio's concept of "page size"

[Hollis Blanchard]

On Tue, 2008-11-11 at 23:47 +1030, Rusty Russell wrote:
> On Tuesday 11 November 2008 10:07:09 Hollis Blanchard wrote:
> 
> > 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.
> 
> Thanks Hollis! Applied, with the following overpatch:

Avi, will you commit the matching qemu patch? (I'm not clear on the
patch flow for kvm-userspace qemu virtio patches.)

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [PATCH] [v2] linux: virtio: Standardize virtio's concept of "page size"

[Avi Kivity]

Hollis Blanchard wrote:
> Avi, will you commit the matching qemu patch? (I'm not clear on the
> patch flow for kvm-userspace qemu virtio patches.)
>   

At this time, I'm accepting qemu/virtio patches (and will apply this 
one); once virtio is merged in qemu upstream, patches will have to go 
there as well.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] [v2] linux: virtio: Standardize virtio's concept of "page size"

[Rusty Russell]

On Tuesday 11 November 2008 10:07:09 Hollis Blanchard wrote:
> 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.
>
> Currently there are three distinct problems:
> * The shift count used when passing the physical address of the ring to a
>   PCI-based back end.
> * The ring layout itself is padded to span at least two "pages".
> * The balloon driver operates in units of "pages".

Hi Hollis,

   The more I thought about this, the more I think we're not solving this
as neatly as we could.  The trigger was noting that we're breaking the
userspace API (vring_size and vring_init are exposed to userspace): I
know that qemu cut & pastes, but that's no excuse.

   So instead, I've introduced separate constants for each use.  Yes,
all these constants are 12/4096.  But just to be contrary, at the end
is a patch to change lguest to 128.  And there's no reason this
couldn't change in future using some guest detection scheme.

Patch stream to follow...
Rusty.

Re: [PATCH] [v2] linux: virtio: Standardize virtio's concept of "page size"

[Hollis Blanchard]

On Wed, 2008-11-12 at 22:51 +1030, Rusty Russell wrote:
> On Tuesday 11 November 2008 10:07:09 Hollis Blanchard wrote:
> > 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.
> >
> > Currently there are three distinct problems:
> > * The shift count used when passing the physical address of the ring to a
> >   PCI-based back end.
> > * The ring layout itself is padded to span at least two "pages".
> > * The balloon driver operates in units of "pages".
> 
> Hi Hollis,
> 
>    The more I thought about this, the more I think we're not solving this
> as neatly as we could.  The trigger was noting that we're breaking the
> userspace API (vring_size and vring_init are exposed to userspace): I
> know that qemu cut & pastes, but that's no excuse.
> 
>    So instead, I've introduced separate constants for each use.  Yes,
> all these constants are 12/4096.  But just to be contrary, at the end
> is a patch to change lguest to 128.  And there's no reason this
> couldn't change in future using some guest detection scheme.

OK. I thought it was simpler to just say "4KB everywhere" in all aspects
of the virtio interface, but I'm happy as long as we solve the problem
somehow. :)

-- 
Hollis Blanchard
IBM Linux Technology Center

--
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] [v2] linux: virtio: Standardize virtio's concept of "page size"

[Rusty Russell]

On Thursday 13 November 2008 02:46:31 Hollis Blanchard wrote:
> On Wed, 2008-11-12 at 22:51 +1030, Rusty Russell wrote:
> > On Tuesday 11 November 2008 10:07:09 Hollis Blanchard wrote:
> > > 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.
> > >
> > > Currently there are three distinct problems:
> > > * The shift count used when passing the physical address of the ring to
> > > a PCI-based back end.
> > > * The ring layout itself is padded to span at least two "pages".
> > > * The balloon driver operates in units of "pages".
> >
> > Hi Hollis,
> >
> >    The more I thought about this, the more I think we're not solving this
> > as neatly as we could.  The trigger was noting that we're breaking the
> > userspace API (vring_size and vring_init are exposed to userspace): I
> > know that qemu cut & pastes, but that's no excuse.
> >
> >    So instead, I've introduced separate constants for each use.  Yes,
> > all these constants are 12/4096.  But just to be contrary, at the end
> > is a patch to change lguest to 128.  And there's no reason this
> > couldn't change in future using some guest detection scheme.
>
> OK. I thought it was simpler to just say "4KB everywhere" in all aspects
> of the virtio interface, but I'm happy as long as we solve the problem
> somehow. :)

It is simpler, yes, but we can take this opportunity to deconflate them and 
make things clearer and better than the current code, not just "fix" it.

Note that I still don't have a balloon patch: want to send me one?

Thanks,
Rusty.

Re: [PATCH] [v2] linux: virtio: Standardize virtio's concept of "page size"

[Hollis Blanchard]

On Thu, 2008-11-13 at 08:44 +1030, Rusty Russell wrote:
> 
> Note that I still don't have a balloon patch: want to send me one?

linux: virtio-balloon: avoid implicit use of Linux page size in balloon interface

Make the balloon interface always use 4K pages, and convert Linux pfns if
necessary. This patch assumes that Linux's PAGE_SHIFT will never be less than
12.

Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>
---
Only build-tested.

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -56,6 +56,14 @@ static struct virtio_device_id id_table[
 	{ 0 },
 };
 
+static u32 page_to_balloon_pfn(struct page *page)
+{
+    unsigned long pfn = page_to_pfn(page);
+
+    /* Convert pfn from Linux page size to balloon page size. */
+    return pfn >> (PAGE_SHIFT - VIRTIO_BALLOON_PFN_SHIFT);
+}
+
 static void balloon_ack(struct virtqueue *vq)
 {
 	struct virtio_balloon *vb;
@@ -99,7 +107,7 @@ static void fill_balloon(struct virtio_b
 			msleep(200);
 			break;
 		}
-		vb->pfns[vb->num_pfns] = page_to_pfn(page);
+		vb->pfns[vb->num_pfns] = page_to_balloon_pfn(page);
 		totalram_pages--;
 		vb->num_pages++;
 		list_add(&page->lru, &vb->pages);
@@ -132,7 +140,7 @@ static void leak_balloon(struct virtio_b
 	for (vb->num_pfns = 0; vb->num_pfns < num; vb->num_pfns++) {
 		page = list_first_entry(&vb->pages, struct page, lru);
 		list_del(&page->lru);
-		vb->pfns[vb->num_pfns] = page_to_pfn(page);
+		vb->pfns[vb->num_pfns] = page_to_balloon_pfn(page);
 		vb->num_pages--;
 	}
 
diff --git a/include/linux/virtio_balloon.h b/include/linux/virtio_balloon.h
--- a/include/linux/virtio_balloon.h
+++ b/include/linux/virtio_balloon.h
@@ -10,6 +10,9 @@
 /* The feature bitmap for virtio balloon */
 #define VIRTIO_BALLOON_F_MUST_TELL_HOST	0 /* Tell before reclaiming pages */
 
+/* Size of a PFN in the balloon interface. */
+#define VIRTIO_BALLOON_PFN_SHIFT 12
+
 struct virtio_balloon_config
 {
 	/* Number of pages host wants Guest to give up. */


-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [PATCH] [v2] linux: virtio: Standardize virtio's concept of "page size"

[Rusty Russell]

On Friday 14 November 2008 08:18:33 Hollis Blanchard wrote:
> On Thu, 2008-11-13 at 08:44 +1030, Rusty Russell wrote:
> > Note that I still don't have a balloon patch: want to send me one?
>
> linux: virtio-balloon: avoid implicit use of Linux page size in balloon
> interface

Thanks, applied with following diff:

Use tabs to indent, and put BUILD_BUG_ON pagesize assumption.

Signed-off-by: Rusty Russell <rusty-8n+1lVoiYb80n/F98K4Iww@public.gmane.org>

diff -r 50e970613233 drivers/virtio/virtio_balloon.c
--- a/drivers/virtio/virtio_balloon.c	Fri Nov 14 11:40:38 2008 +1030
+++ b/drivers/virtio/virtio_balloon.c	Fri Nov 14 11:41:19 2008 +1030
@@ -58,10 +58,11 @@
 
 static u32 page_to_balloon_pfn(struct page *page)
 {
-    unsigned long pfn = page_to_pfn(page);
+	unsigned long pfn = page_to_pfn(page);
 
-    /* Convert pfn from Linux page size to balloon page size. */
-    return pfn >> (PAGE_SHIFT - VIRTIO_BALLOON_PFN_SHIFT);
+	BUILD_BUG_ON(PAGE_SHIFT < VIRTIO_BALLOON_PFN_SHIFT);
+	/* Convert pfn from Linux page size to balloon page size. */
+	return pfn >> (PAGE_SHIFT - VIRTIO_BALLOON_PFN_SHIFT);
 }
 
 static void balloon_ack(struct virtqueue *vq)

--
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] [v2] linux: virtio: Standardize virtio's concept of "page size"

[Hollis Blanchard]

On Thu, 2008-11-13 at 08:44 +1030, Rusty Russell wrote:
> 
> Note that I still don't have a balloon patch: want to send me one?

qemu: virtio-balloon: don't use TARGET_PAGE_BITS in balloon interface

Make the balloon interface always use 4K pages.

Signed-off-by: Hollis Blanchard <hollisb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
Only build-tested.

diff --git a/qemu/hw/virtio-balloon.c b/qemu/hw/virtio-balloon.c
--- a/qemu/hw/virtio-balloon.c
+++ b/qemu/hw/virtio-balloon.c
@@ -87,7 +87,7 @@ static void virtio_balloon_handle_output
 	    ram_addr_t pa;
 	    ram_addr_t addr;
 
-	    pa = (ram_addr_t)ldl_p(&pfn) << TARGET_PAGE_BITS;
+	    pa = (ram_addr_t)ldl_p(&pfn) << VIRTIO_BALLOON_PFN_SHIFT;
 	    offset += 4;
 
 	    addr = cpu_get_physical_page_desc(pa);
@@ -135,11 +135,11 @@ static ram_addr_t virtio_balloon_to_targ
 	target = ram_size;
 
     if (target) {
-	dev->num_pages = (ram_size - target) >> TARGET_PAGE_BITS;
+	dev->num_pages = (ram_size - target) >> VIRTIO_BALLOON_PFN_SHIFT;
 	virtio_notify_config(&dev->vdev);
     }
 
-    return ram_size - (dev->actual << TARGET_PAGE_BITS);
+    return ram_size - (dev->actual << VIRTIO_BALLOON_PFN_SHIFT);
 }
 
 static void virtio_balloon_save(QEMUFile *f, void *opaque)
diff --git a/qemu/hw/virtio-balloon.h b/qemu/hw/virtio-balloon.h
--- a/qemu/hw/virtio-balloon.h
+++ b/qemu/hw/virtio-balloon.h
@@ -23,6 +23,9 @@
 /* The feature bitmap for virtio balloon */
 #define VIRTIO_BALLOON_F_MUST_TELL_HOST	0 /* Tell before reclaiming pages */
 
+/* Size of a PFN in the balloon interface. */
+#define VIRTIO_BALLOON_PFN_SHIFT 12
+
 struct virtio_balloon_config
 {
     /* Number of pages host wants Guest to give up. */

-- 
Hollis Blanchard
IBM Linux Technology Center

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