Re: [PATCH] qemu: define and use VIRTIO_PFN_SHIFT

Re: [PATCH] qemu: define and use VIRTIO_PFN_SHIFT

[Mark McLoughlin]

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

> diff --git a/qemu/hw/virtio.c b/qemu/hw/virtio.c
> --- a/qemu/hw/virtio.c
> +++ b/qemu/hw/virtio.c
> @@ -56,6 +56,10 @@
>   */
>  #define wmb() do { } while (0)
>  
> +#define VRING_PAGE_SIZE (1<<12)
> +
> +#define ALIGN(x, a)  (((x)+(a)-1) & ~((a)-1))

Might be better to do:

#define VRING_PAGE_SIZE (1<<12)
#define VRING_PAGE_MASK ~(VRING_PAGE_SIZE - 1)
#define VRING_PAGE_ALIGN(x) (((x) + VRING_PAGE_SIZE - 1) & VRING_PAGE_MASK)

Cheers,
Mark

Re: [PATCH] qemu: define and use VIRTIO_PFN_SHIFT

[Mark McLoughlin]

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

> diff --git a/qemu/hw/virtio.c b/qemu/hw/virtio.c
> --- a/qemu/hw/virtio.c
> +++ b/qemu/hw/virtio.c
> @@ -56,6 +56,10 @@
>   */
>  #define wmb() do { } while (0)
>  
> +#define VRING_PAGE_SIZE (1<<12)
> +
> +#define ALIGN(x, a)  (((x)+(a)-1) & ~((a)-1))

Might be better to do:

#define VRING_PAGE_SIZE (1<<12)
#define VRING_PAGE_MASK ~(VRING_PAGE_SIZE - 1)
#define VRING_PAGE_ALIGN(x) (((x) + VRING_PAGE_SIZE - 1) & VRING_PAGE_MASK)

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] qemu: define and use VIRTIO_PFN_SHIFT

[Mark McLoughlin]

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

> diff --git a/qemu/hw/virtio.c b/qemu/hw/virtio.c
> --- a/qemu/hw/virtio.c
> +++ b/qemu/hw/virtio.c
> @@ -56,6 +56,10 @@
>   */
>  #define wmb() do { } while (0)
>  
> +#define VRING_PAGE_SIZE (1<<12)
> +
> +#define ALIGN(x, a)  (((x)+(a)-1) & ~((a)-1))

Might be better to do:

#define VRING_PAGE_SIZE (1<<12)
#define VRING_PAGE_MASK ~(VRING_PAGE_SIZE - 1)
#define VRING_PAGE_ALIGN(x) (((x) + VRING_PAGE_SIZE - 1) & VRING_PAGE_MASK)

Cheers,
Mark

Re: [PATCH] qemu: define and use VIRTIO_PFN_SHIFT

[Anthony Liguori]

Hollis Blanchard wrote:
> # HG changeset patch
> # User Hollis Blanchard <hollisb@us.ibm.com>
> # Date 1225946837 21600
> # Node ID 43a111ea61b542d3823e2a11d017e7b06b7ec254
> # Parent  b63967268af119e0faa4adc3086cdef857815548
> qemu: define and use VIRTIO_PFN_SHIFT
>
> The virtio front and back ends must agree about how big a pfn really is. Since
> qemu has no idea what "page size" the guest may be using, it must be
> independent of TARGET_PAGE_BITS.
>
> This patch should have no functional effect on x86 or ia64, but I'd like an ack from the
> ia64 guys.
>   

Would be better to add a new header in target-XXX instead of using 
cpu.h.  Virtio is not part of the CPU ISA.

> Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>
>
> diff --git a/qemu/hw/virtio.c b/qemu/hw/virtio.c
> --- a/qemu/hw/virtio.c
> +++ b/qemu/hw/virtio.c
> @@ -56,6 +56,10 @@
>   */
>  #define wmb() do { } while (0)
>  
> +#define VRING_PAGE_SIZE (1<<12)
> +
> +#define ALIGN(x, a)  (((x)+(a)-1) & ~((a)-1))
> +
>  /* virt queue functions */

Why is VRING_PAGE_SIZE not architecture specific?

Regards,

Anthony Liguori


>  
>  static void *virtio_map_gpa(target_phys_addr_t addr, size_t size)
> @@ -95,8 +99,8 @@ static void *virtio_map_gpa(target_phys_
>  
>  static size_t virtqueue_size(int num)
>  {
> -    return TARGET_PAGE_ALIGN((sizeof(VRingDesc) * num) +
> -			     (sizeof(VRingAvail) + sizeof(uint16_t) * num)) +
> +    return ALIGN((sizeof(VRingDesc) * num) + (sizeof(VRingAvail) +
> +		 sizeof(uint16_t) * num), VRING_PAGE_SIZE) +
>  	(sizeof(VRingUsed) + sizeof(VRingUsedElem) * num);
>  }
>  
> @@ -104,7 +108,7 @@ static void virtqueue_init(VirtQueue *vq
>  {
>      vq->vring.desc = p;
>      vq->vring.avail = p + vq->vring.num * sizeof(VRingDesc);
> -    vq->vring.used = (void *)TARGET_PAGE_ALIGN((unsigned long)&vq->vring.avail->ring[vq->vring.num]);
> +    vq->vring.used = (void *)ALIGN((unsigned long)&vq->vring.avail->ring[vq->vring.num], VRING_PAGE_SIZE);
>  }
>  
>  static unsigned virtqueue_next_desc(VirtQueue *vq, unsigned int i)
> @@ -241,7 +245,7 @@ static void virtio_ioport_write(void *op
>  	vdev->features = val;
>  	break;
>      case VIRTIO_PCI_QUEUE_PFN:
> -	pa = (ram_addr_t)val << TARGET_PAGE_BITS;
> +	pa = (ram_addr_t)val << VIRTIO_PFN_SHIFT;
>  	vdev->vq[vdev->queue_sel].pfn = val;
>  	if (pa = 0) {
>              virtio_reset(vdev);
> @@ -519,7 +523,7 @@ void virtio_load(VirtIODevice *vdev, QEM
>  	    size_t size;
>  	    target_phys_addr_t pa;
>  
> -	    pa = (ram_addr_t)vdev->vq[i].pfn << TARGET_PAGE_BITS;
> +	    pa = (ram_addr_t)vdev->vq[i].pfn << VIRTIO_PFN_SHIFT;
>  	    size = virtqueue_size(vdev->vq[i].vring.num);
>  	    virtqueue_init(&vdev->vq[i], virtio_map_gpa(pa, size));
>  	}
> diff --git a/qemu/target-i386/cpu.h b/qemu/target-i386/cpu.h
> --- a/qemu/target-i386/cpu.h
> +++ b/qemu/target-i386/cpu.h
> @@ -751,6 +751,8 @@ static inline int cpu_get_time_fast(void
>  
>  #define TARGET_PAGE_BITS 12
>  
> +#define VIRTIO_PFN_SHIFT 12
> +
>  #define CPUState CPUX86State
>  #define cpu_init cpu_x86_init
>  #define cpu_exec cpu_x86_exec
> diff --git a/qemu/target-ia64/cpu.h b/qemu/target-ia64/cpu.h
> --- a/qemu/target-ia64/cpu.h
> +++ b/qemu/target-ia64/cpu.h
> @@ -30,6 +30,8 @@
>  #define TARGET_LONG_BITS 64
>  
>  #define TARGET_PAGE_BITS 16
> +
> +#define VIRTIO_PFN_SHIFT 16
>  
>  #define ELF_MACHINE	EM_IA_64
>  
> diff --git a/qemu/target-ppc/cpu.h b/qemu/target-ppc/cpu.h
> --- a/qemu/target-ppc/cpu.h
> +++ b/qemu/target-ppc/cpu.h
> @@ -54,6 +54,8 @@
>  #endif /* defined(TARGET_PPCEMB) */
>  
>  #endif /* defined (TARGET_PPC64) */
> +
> +#define VIRTIO_PFN_SHIFT 10
>  
>  #include "cpu-defs.h"
>  
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   

Re: [PATCH] qemu: define and use VIRTIO_PFN_SHIFT

[Anthony Liguori]

Hollis Blanchard wrote:
> # HG changeset patch
> # User Hollis Blanchard <hollisb@us.ibm.com>
> # Date 1225946837 21600
> # Node ID 43a111ea61b542d3823e2a11d017e7b06b7ec254
> # Parent  b63967268af119e0faa4adc3086cdef857815548
> qemu: define and use VIRTIO_PFN_SHIFT
>
> The virtio front and back ends must agree about how big a pfn really is. Since
> qemu has no idea what "page size" the guest may be using, it must be
> independent of TARGET_PAGE_BITS.
>
> This patch should have no functional effect on x86 or ia64, but I'd like an ack from the
> ia64 guys.
>   

Would be better to add a new header in target-XXX instead of using 
cpu.h.  Virtio is not part of the CPU ISA.

> Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>
>
> diff --git a/qemu/hw/virtio.c b/qemu/hw/virtio.c
> --- a/qemu/hw/virtio.c
> +++ b/qemu/hw/virtio.c
> @@ -56,6 +56,10 @@
>   */
>  #define wmb() do { } while (0)
>  
> +#define VRING_PAGE_SIZE (1<<12)
> +
> +#define ALIGN(x, a)  (((x)+(a)-1) & ~((a)-1))
> +
>  /* virt queue functions */

Why is VRING_PAGE_SIZE not architecture specific?

Regards,

Anthony Liguori


>  
>  static void *virtio_map_gpa(target_phys_addr_t addr, size_t size)
> @@ -95,8 +99,8 @@ static void *virtio_map_gpa(target_phys_
>  
>  static size_t virtqueue_size(int num)
>  {
> -    return TARGET_PAGE_ALIGN((sizeof(VRingDesc) * num) +
> -			     (sizeof(VRingAvail) + sizeof(uint16_t) * num)) +
> +    return ALIGN((sizeof(VRingDesc) * num) + (sizeof(VRingAvail) +
> +		 sizeof(uint16_t) * num), VRING_PAGE_SIZE) +
>  	(sizeof(VRingUsed) + sizeof(VRingUsedElem) * num);
>  }
>  
> @@ -104,7 +108,7 @@ static void virtqueue_init(VirtQueue *vq
>  {
>      vq->vring.desc = p;
>      vq->vring.avail = p + vq->vring.num * sizeof(VRingDesc);
> -    vq->vring.used = (void *)TARGET_PAGE_ALIGN((unsigned long)&vq->vring.avail->ring[vq->vring.num]);
> +    vq->vring.used = (void *)ALIGN((unsigned long)&vq->vring.avail->ring[vq->vring.num], VRING_PAGE_SIZE);
>  }
>  
>  static unsigned virtqueue_next_desc(VirtQueue *vq, unsigned int i)
> @@ -241,7 +245,7 @@ static void virtio_ioport_write(void *op
>  	vdev->features = val;
>  	break;
>      case VIRTIO_PCI_QUEUE_PFN:
> -	pa = (ram_addr_t)val << TARGET_PAGE_BITS;
> +	pa = (ram_addr_t)val << VIRTIO_PFN_SHIFT;
>  	vdev->vq[vdev->queue_sel].pfn = val;
>  	if (pa == 0) {
>              virtio_reset(vdev);
> @@ -519,7 +523,7 @@ void virtio_load(VirtIODevice *vdev, QEM
>  	    size_t size;
>  	    target_phys_addr_t pa;
>  
> -	    pa = (ram_addr_t)vdev->vq[i].pfn << TARGET_PAGE_BITS;
> +	    pa = (ram_addr_t)vdev->vq[i].pfn << VIRTIO_PFN_SHIFT;
>  	    size = virtqueue_size(vdev->vq[i].vring.num);
>  	    virtqueue_init(&vdev->vq[i], virtio_map_gpa(pa, size));
>  	}
> diff --git a/qemu/target-i386/cpu.h b/qemu/target-i386/cpu.h
> --- a/qemu/target-i386/cpu.h
> +++ b/qemu/target-i386/cpu.h
> @@ -751,6 +751,8 @@ static inline int cpu_get_time_fast(void
>  
>  #define TARGET_PAGE_BITS 12
>  
> +#define VIRTIO_PFN_SHIFT 12
> +
>  #define CPUState CPUX86State
>  #define cpu_init cpu_x86_init
>  #define cpu_exec cpu_x86_exec
> diff --git a/qemu/target-ia64/cpu.h b/qemu/target-ia64/cpu.h
> --- a/qemu/target-ia64/cpu.h
> +++ b/qemu/target-ia64/cpu.h
> @@ -30,6 +30,8 @@
>  #define TARGET_LONG_BITS 64
>  
>  #define TARGET_PAGE_BITS 16
> +
> +#define VIRTIO_PFN_SHIFT 16
>  
>  #define ELF_MACHINE	EM_IA_64
>  
> diff --git a/qemu/target-ppc/cpu.h b/qemu/target-ppc/cpu.h
> --- a/qemu/target-ppc/cpu.h
> +++ b/qemu/target-ppc/cpu.h
> @@ -54,6 +54,8 @@
>  #endif /* defined(TARGET_PPCEMB) */
>  
>  #endif /* defined (TARGET_PPC64) */
> +
> +#define VIRTIO_PFN_SHIFT 10
>  
>  #include "cpu-defs.h"
>  
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   

Re: [PATCH] qemu: define and use VIRTIO_PFN_SHIFT

[Anthony Liguori]

Hollis Blanchard wrote:
> # HG changeset patch
> # User Hollis Blanchard <hollisb@us.ibm.com>
> # Date 1225946837 21600
> # Node ID 43a111ea61b542d3823e2a11d017e7b06b7ec254
> # Parent  b63967268af119e0faa4adc3086cdef857815548
> qemu: define and use VIRTIO_PFN_SHIFT
>
> The virtio front and back ends must agree about how big a pfn really is. Since
> qemu has no idea what "page size" the guest may be using, it must be
> independent of TARGET_PAGE_BITS.
>
> This patch should have no functional effect on x86 or ia64, but I'd like an ack from the
> ia64 guys.
>   

Would be better to add a new header in target-XXX instead of using 
cpu.h.  Virtio is not part of the CPU ISA.

> Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>
>
> diff --git a/qemu/hw/virtio.c b/qemu/hw/virtio.c
> --- a/qemu/hw/virtio.c
> +++ b/qemu/hw/virtio.c
> @@ -56,6 +56,10 @@
>   */
>  #define wmb() do { } while (0)
>  
> +#define VRING_PAGE_SIZE (1<<12)
> +
> +#define ALIGN(x, a)  (((x)+(a)-1) & ~((a)-1))
> +
>  /* virt queue functions */

Why is VRING_PAGE_SIZE not architecture specific?

Regards,

Anthony Liguori


>  
>  static void *virtio_map_gpa(target_phys_addr_t addr, size_t size)
> @@ -95,8 +99,8 @@ static void *virtio_map_gpa(target_phys_
>  
>  static size_t virtqueue_size(int num)
>  {
> -    return TARGET_PAGE_ALIGN((sizeof(VRingDesc) * num) +
> -			     (sizeof(VRingAvail) + sizeof(uint16_t) * num)) +
> +    return ALIGN((sizeof(VRingDesc) * num) + (sizeof(VRingAvail) +
> +		 sizeof(uint16_t) * num), VRING_PAGE_SIZE) +
>  	(sizeof(VRingUsed) + sizeof(VRingUsedElem) * num);
>  }
>  
> @@ -104,7 +108,7 @@ static void virtqueue_init(VirtQueue *vq
>  {
>      vq->vring.desc = p;
>      vq->vring.avail = p + vq->vring.num * sizeof(VRingDesc);
> -    vq->vring.used = (void *)TARGET_PAGE_ALIGN((unsigned long)&vq->vring.avail->ring[vq->vring.num]);
> +    vq->vring.used = (void *)ALIGN((unsigned long)&vq->vring.avail->ring[vq->vring.num], VRING_PAGE_SIZE);
>  }
>  
>  static unsigned virtqueue_next_desc(VirtQueue *vq, unsigned int i)
> @@ -241,7 +245,7 @@ static void virtio_ioport_write(void *op
>  	vdev->features = val;
>  	break;
>      case VIRTIO_PCI_QUEUE_PFN:
> -	pa = (ram_addr_t)val << TARGET_PAGE_BITS;
> +	pa = (ram_addr_t)val << VIRTIO_PFN_SHIFT;
>  	vdev->vq[vdev->queue_sel].pfn = val;
>  	if (pa = 0) {
>              virtio_reset(vdev);
> @@ -519,7 +523,7 @@ void virtio_load(VirtIODevice *vdev, QEM
>  	    size_t size;
>  	    target_phys_addr_t pa;
>  
> -	    pa = (ram_addr_t)vdev->vq[i].pfn << TARGET_PAGE_BITS;
> +	    pa = (ram_addr_t)vdev->vq[i].pfn << VIRTIO_PFN_SHIFT;
>  	    size = virtqueue_size(vdev->vq[i].vring.num);
>  	    virtqueue_init(&vdev->vq[i], virtio_map_gpa(pa, size));
>  	}
> diff --git a/qemu/target-i386/cpu.h b/qemu/target-i386/cpu.h
> --- a/qemu/target-i386/cpu.h
> +++ b/qemu/target-i386/cpu.h
> @@ -751,6 +751,8 @@ static inline int cpu_get_time_fast(void
>  
>  #define TARGET_PAGE_BITS 12
>  
> +#define VIRTIO_PFN_SHIFT 12
> +
>  #define CPUState CPUX86State
>  #define cpu_init cpu_x86_init
>  #define cpu_exec cpu_x86_exec
> diff --git a/qemu/target-ia64/cpu.h b/qemu/target-ia64/cpu.h
> --- a/qemu/target-ia64/cpu.h
> +++ b/qemu/target-ia64/cpu.h
> @@ -30,6 +30,8 @@
>  #define TARGET_LONG_BITS 64
>  
>  #define TARGET_PAGE_BITS 16
> +
> +#define VIRTIO_PFN_SHIFT 16
>  
>  #define ELF_MACHINE	EM_IA_64
>  
> diff --git a/qemu/target-ppc/cpu.h b/qemu/target-ppc/cpu.h
> --- a/qemu/target-ppc/cpu.h
> +++ b/qemu/target-ppc/cpu.h
> @@ -54,6 +54,8 @@
>  #endif /* defined(TARGET_PPCEMB) */
>  
>  #endif /* defined (TARGET_PPC64) */
> +
> +#define VIRTIO_PFN_SHIFT 10
>  
>  #include "cpu-defs.h"
>  
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   

Re: [PATCH] qemu: define and use VIRTIO_PFN_SHIFT

[Hollis Blanchard]

On Thu, 2008-11-06 at 08:01 -0600, Anthony Liguori wrote:
> Hollis Blanchard wrote:
> > # HG changeset patch
> > # User Hollis Blanchard <hollisb@us.ibm.com>
> > # Date 1225946837 21600
> > # Node ID 43a111ea61b542d3823e2a11d017e7b06b7ec254
> > # Parent  b63967268af119e0faa4adc3086cdef857815548
> > qemu: define and use VIRTIO_PFN_SHIFT
> >
> > The virtio front and back ends must agree about how big a pfn really
> is. Since
> > qemu has no idea what "page size" the guest may be using, it must be
> > independent of TARGET_PAGE_BITS.
> >
> > This patch should have no functional effect on x86 or ia64, but I'd
> like an ack from the
> > ia64 guys.
> >   
> 
> Would be better to add a new header in target-XXX instead of using 
> cpu.h.  Virtio is not part of the CPU ISA.

OK.

> > Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>
> >
> > diff --git a/qemu/hw/virtio.c b/qemu/hw/virtio.c
> > --- a/qemu/hw/virtio.c
> > +++ b/qemu/hw/virtio.c
> > @@ -56,6 +56,10 @@
> >   */
> >  #define wmb() do { } while (0)
> >  
> > +#define VRING_PAGE_SIZE (1<<12)
> > +
> > +#define ALIGN(x, a)  (((x)+(a)-1) & ~((a)-1))
> > +
> >  /* virt queue functions */
> 
> Why is VRING_PAGE_SIZE not architecture specific?

I wanted to make sure people on non-x86 architectures couldn't run into
vring-size related problems that didn't also appear on x86.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [PATCH] qemu: define and use VIRTIO_PFN_SHIFT

[Hollis Blanchard]

On Thu, 2008-11-06 at 08:01 -0600, Anthony Liguori wrote:
> Hollis Blanchard wrote:
> > # HG changeset patch
> > # User Hollis Blanchard <hollisb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> > # Date 1225946837 21600
> > # Node ID 43a111ea61b542d3823e2a11d017e7b06b7ec254
> > # Parent  b63967268af119e0faa4adc3086cdef857815548
> > qemu: define and use VIRTIO_PFN_SHIFT
> >
> > The virtio front and back ends must agree about how big a pfn really
> is. Since
> > qemu has no idea what "page size" the guest may be using, it must be
> > independent of TARGET_PAGE_BITS.
> >
> > This patch should have no functional effect on x86 or ia64, but I'd
> like an ack from the
> > ia64 guys.
> >   
> 
> Would be better to add a new header in target-XXX instead of using 
> cpu.h.  Virtio is not part of the CPU ISA.

OK.

> > Signed-off-by: Hollis Blanchard <hollisb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> >
> > diff --git a/qemu/hw/virtio.c b/qemu/hw/virtio.c
> > --- a/qemu/hw/virtio.c
> > +++ b/qemu/hw/virtio.c
> > @@ -56,6 +56,10 @@
> >   */
> >  #define wmb() do { } while (0)
> >  
> > +#define VRING_PAGE_SIZE (1<<12)
> > +
> > +#define ALIGN(x, a)  (((x)+(a)-1) & ~((a)-1))
> > +
> >  /* virt queue functions */
> 
> Why is VRING_PAGE_SIZE not architecture specific?

I wanted to make sure people on non-x86 architectures couldn't run into
vring-size related problems that didn't also appear on x86.

-- 
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] qemu: define and use VIRTIO_PFN_SHIFT

[Hollis Blanchard]

On Thu, 2008-11-06 at 08:01 -0600, Anthony Liguori wrote:
> Hollis Blanchard wrote:
> > # HG changeset patch
> > # User Hollis Blanchard <hollisb@us.ibm.com>
> > # Date 1225946837 21600
> > # Node ID 43a111ea61b542d3823e2a11d017e7b06b7ec254
> > # Parent  b63967268af119e0faa4adc3086cdef857815548
> > qemu: define and use VIRTIO_PFN_SHIFT
> >
> > The virtio front and back ends must agree about how big a pfn really
> is. Since
> > qemu has no idea what "page size" the guest may be using, it must be
> > independent of TARGET_PAGE_BITS.
> >
> > This patch should have no functional effect on x86 or ia64, but I'd
> like an ack from the
> > ia64 guys.
> >   
> 
> Would be better to add a new header in target-XXX instead of using 
> cpu.h.  Virtio is not part of the CPU ISA.

OK.

> > Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>
> >
> > diff --git a/qemu/hw/virtio.c b/qemu/hw/virtio.c
> > --- a/qemu/hw/virtio.c
> > +++ b/qemu/hw/virtio.c
> > @@ -56,6 +56,10 @@
> >   */
> >  #define wmb() do { } while (0)
> >  
> > +#define VRING_PAGE_SIZE (1<<12)
> > +
> > +#define ALIGN(x, a)  (((x)+(a)-1) & ~((a)-1))
> > +
> >  /* virt queue functions */
> 
> Why is VRING_PAGE_SIZE not architecture specific?

I wanted to make sure people on non-x86 architectures couldn't run into
vring-size related problems that didn't also appear on x86.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [PATCH] qemu: define and use VIRTIO_PFN_SHIFT

[Anthony Liguori]

Hollis Blanchard wrote:
> I wanted to make sure people on non-x86 architectures couldn't run into
> vring-size related problems that didn't also appear on x86.
>   

Having a VRING_SHIFT and a VRING_PAGE_SIZE where VRING_PAGE_SIZE != (1 
<< VRING_SHIFT) is almost certainly going to break things in unexpected 
ways because it's going against common understanding of the relationship 
between shift and page_size.

Regards,

Anthony Liguori

Re: [PATCH] qemu: define and use VIRTIO_PFN_SHIFT

[Anthony Liguori]

Hollis Blanchard wrote:
> I wanted to make sure people on non-x86 architectures couldn't run into
> vring-size related problems that didn't also appear on x86.
>   

Having a VRING_SHIFT and a VRING_PAGE_SIZE where VRING_PAGE_SIZE != (1 
<< VRING_SHIFT) is almost certainly going to break things in unexpected 
ways because it's going against common understanding of the relationship 
between shift and page_size.

Regards,

Anthony Liguori

Re: [PATCH] qemu: define and use VIRTIO_PFN_SHIFT

[Anthony Liguori]

Hollis Blanchard wrote:
> I wanted to make sure people on non-x86 architectures couldn't run into
> vring-size related problems that didn't also appear on x86.
>   

Having a VRING_SHIFT and a VRING_PAGE_SIZE where VRING_PAGE_SIZE != (1 
<< VRING_SHIFT) is almost certainly going to break things in unexpected 
ways because it's going against common understanding of the relationship 
between shift and page_size.

Regards,

Anthony Liguori

RE: [PATCH] qemu: define and use VIRTIO_PFN_SHIFT

[Zhang, Xiantao]

Hollis Blanchard wrote:
> On Thu, 2008-11-06 at 08:01 -0600, Anthony Liguori wrote:
>> Hollis Blanchard wrote:
>>> # HG changeset patch
>>> # User Hollis Blanchard <hollisb@us.ibm.com>
>>> # Date 1225946837 21600
>>> # Node ID 43a111ea61b542d3823e2a11d017e7b06b7ec254
>>> # Parent  b63967268af119e0faa4adc3086cdef857815548
>>> qemu: define and use VIRTIO_PFN_SHIFT
>>> 
>>> The virtio front and back ends must agree about how big a pfn
>>> really is. Since qemu has no idea what "page size" the guest may be
>>> using, it must be independent of TARGET_PAGE_BITS.
>>> 
>>> This patch should have no functional effect on x86 or ia64, but I'd
>>> like an ack from the ia64 guys. 
>>> 
>> 
>> Would be better to add a new header in target-XXX instead of using
>> cpu.h.  Virtio is not part of the CPU ISA.
> 
> OK.
> 
>>> Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>
>>> 
>>> diff --git a/qemu/hw/virtio.c b/qemu/hw/virtio.c
>>> --- a/qemu/hw/virtio.c
>>> +++ b/qemu/hw/virtio.c
>>> @@ -56,6 +56,10 @@
>>>   */
>>>  #define wmb() do { } while (0)
>>> 
>>> +#define VRING_PAGE_SIZE (1<<12)
>>> +
>>> +#define ALIGN(x, a)  (((x)+(a)-1) & ~((a)-1))
>>> +
>>>  /* virt queue functions */
>> 
>> Why is VRING_PAGE_SIZE not architecture specific?
> 
> I wanted to make sure people on non-x86 architectures couldn't run
> into vring-size related problems that didn't also appear on x86.

Hi, Hollis
	Currenlty, kvm-qemu only supports the only case which is host page_size = qemu's target page size for ia64. Does your patch meets the requirement ? For ia64, current linux support 4K, 16K and 64k page size, and 1M 16M 64M or bigger page will be supported in future, so if your patch consider the case, it should work for ia64.  Thanks! 
Thanks
Xiantao

RE: [PATCH] qemu: define and use VIRTIO_PFN_SHIFT

[Zhang, Xiantao]

Hollis Blanchard wrote:
> On Thu, 2008-11-06 at 08:01 -0600, Anthony Liguori wrote:
>> Hollis Blanchard wrote:
>>> # HG changeset patch
>>> # User Hollis Blanchard <hollisb@us.ibm.com>
>>> # Date 1225946837 21600
>>> # Node ID 43a111ea61b542d3823e2a11d017e7b06b7ec254
>>> # Parent  b63967268af119e0faa4adc3086cdef857815548
>>> qemu: define and use VIRTIO_PFN_SHIFT
>>> 
>>> The virtio front and back ends must agree about how big a pfn
>>> really is. Since qemu has no idea what "page size" the guest may be
>>> using, it must be independent of TARGET_PAGE_BITS.
>>> 
>>> This patch should have no functional effect on x86 or ia64, but I'd
>>> like an ack from the ia64 guys. 
>>> 
>> 
>> Would be better to add a new header in target-XXX instead of using
>> cpu.h.  Virtio is not part of the CPU ISA.
> 
> OK.
> 
>>> Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>
>>> 
>>> diff --git a/qemu/hw/virtio.c b/qemu/hw/virtio.c
>>> --- a/qemu/hw/virtio.c
>>> +++ b/qemu/hw/virtio.c
>>> @@ -56,6 +56,10 @@
>>>   */
>>>  #define wmb() do { } while (0)
>>> 
>>> +#define VRING_PAGE_SIZE (1<<12)
>>> +
>>> +#define ALIGN(x, a)  (((x)+(a)-1) & ~((a)-1))
>>> +
>>>  /* virt queue functions */
>> 
>> Why is VRING_PAGE_SIZE not architecture specific?
> 
> I wanted to make sure people on non-x86 architectures couldn't run
> into vring-size related problems that didn't also appear on x86.

Hi, Hollis
	Currenlty, kvm-qemu only supports the only case which is host page_size = qemu's target page size for ia64. Does your patch meets the requirement ? For ia64, current linux support 4K, 16K and 64k page size, and 1M 16M 64M or bigger page will be supported in future, so if your patch consider the case, it should work for ia64.  Thanks! 
Thanks
Xiantao

RE: [PATCH] qemu: define and use VIRTIO_PFN_SHIFT

[Zhang, Xiantao]

Hollis Blanchard wrote:
> On Thu, 2008-11-06 at 08:01 -0600, Anthony Liguori wrote:
>> Hollis Blanchard wrote:
>>> # HG changeset patch
>>> # User Hollis Blanchard <hollisb@us.ibm.com>
>>> # Date 1225946837 21600
>>> # Node ID 43a111ea61b542d3823e2a11d017e7b06b7ec254
>>> # Parent  b63967268af119e0faa4adc3086cdef857815548
>>> qemu: define and use VIRTIO_PFN_SHIFT
>>> 
>>> The virtio front and back ends must agree about how big a pfn
>>> really is. Since qemu has no idea what "page size" the guest may be
>>> using, it must be independent of TARGET_PAGE_BITS.
>>> 
>>> This patch should have no functional effect on x86 or ia64, but I'd
>>> like an ack from the ia64 guys. 
>>> 
>> 
>> Would be better to add a new header in target-XXX instead of using
>> cpu.h.  Virtio is not part of the CPU ISA.
> 
> OK.
> 
>>> Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>
>>> 
>>> diff --git a/qemu/hw/virtio.c b/qemu/hw/virtio.c
>>> --- a/qemu/hw/virtio.c
>>> +++ b/qemu/hw/virtio.c
>>> @@ -56,6 +56,10 @@
>>>   */
>>>  #define wmb() do { } while (0)
>>> 
>>> +#define VRING_PAGE_SIZE (1<<12)
>>> +
>>> +#define ALIGN(x, a)  (((x)+(a)-1) & ~((a)-1))
>>> +
>>>  /* virt queue functions */
>> 
>> Why is VRING_PAGE_SIZE not architecture specific?
> 
> I wanted to make sure people on non-x86 architectures couldn't run
> into vring-size related problems that didn't also appear on x86.

Hi, Hollis
	Currenlty, kvm-qemu only supports the only case which is host page_size = qemu's target page size for ia64. Does your patch meets the requirement ? For ia64, current linux support 4K, 16K and 64k page size, and 1M 16M 64M or bigger page will be supported in future, so if your patch consider the case, it should work for ia64.  Thanks! 
Thanks
Xiantao

Re: [PATCH] qemu: define and use VIRTIO_PFN_SHIFT

[Hollis Blanchard]

On Thursday 06 November 2008 19:38:40 Zhang, Xiantao wrote:
> 
> Hi, Hollis
> 	Currenlty, kvm-qemu only supports the only case which is host page_size = 
qemu's target page size for ia64. Does your patch meets the requirement ? For 
ia64, current linux support 4K, 16K and 64k page size, and 1M 16M 64M or 
bigger page will be supported in future, so if your patch consider the case, 
it should work for ia64.  Thanks! 

Take a look at the patch and you tell me. :) I've hardcoded the PFN shift as 
16, which matches your TARGET_PAGE_BITS, but does it *have* to be the same? I 
don't believe it does.

Out of curiosity, if you had 4K Linux host pages, and you redefined 
TARGET_PAGE_BITS to be 12, would everything just work?

Also, as long as I have you... can we use getpagesize() instead of 65536/4096 
in qemu_vmalloc()?

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [PATCH] qemu: define and use VIRTIO_PFN_SHIFT

[Hollis Blanchard]

On Thursday 06 November 2008 19:38:40 Zhang, Xiantao wrote:
> 
> Hi, Hollis
> 	Currenlty, kvm-qemu only supports the only case which is host page_size = 
qemu's target page size for ia64. Does your patch meets the requirement ? For 
ia64, current linux support 4K, 16K and 64k page size, and 1M 16M 64M or 
bigger page will be supported in future, so if your patch consider the case, 
it should work for ia64.  Thanks! 

Take a look at the patch and you tell me. :) I've hardcoded the PFN shift as 
16, which matches your TARGET_PAGE_BITS, but does it *have* to be the same? I 
don't believe it does.

Out of curiosity, if you had 4K Linux host pages, and you redefined 
TARGET_PAGE_BITS to be 12, would everything just work?

Also, as long as I have you... can we use getpagesize() instead of 65536/4096 
in qemu_vmalloc()?

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [PATCH] qemu: define and use VIRTIO_PFN_SHIFT

[Hollis Blanchard]

On Thursday 06 November 2008 19:38:40 Zhang, Xiantao wrote:
> 
> Hi, Hollis
> 	Currenlty, kvm-qemu only supports the only case which is host page_size = 
qemu's target page size for ia64. Does your patch meets the requirement ? For 
ia64, current linux support 4K, 16K and 64k page size, and 1M 16M 64M or 
bigger page will be supported in future, so if your patch consider the case, 
it should work for ia64.  Thanks! 

Take a look at the patch and you tell me. :) I've hardcoded the PFN shift as 
16, which matches your TARGET_PAGE_BITS, but does it *have* to be the same? I 
don't believe it does.

Out of curiosity, if you had 4K Linux host pages, and you redefined 
TARGET_PAGE_BITS to be 12, would everything just work?

Also, as long as I have you... can we use getpagesize() instead of 65536/4096 
in qemu_vmalloc()?

-- 
Hollis Blanchard
IBM Linux Technology Center

RE: [PATCH] qemu: define and use VIRTIO_PFN_SHIFT

[Zhang, Xiantao]

Hollis Blanchard wrote:
> On Thursday 06 November 2008 19:38:40 Zhang, Xiantao wrote:
>> 
>> Hi, Hollis
>> 	Currenlty, kvm-qemu only supports the only case which is host
>> page_size = 
> qemu's target page size for ia64. Does your patch meets the
> requirement ? For ia64, current linux support 4K, 16K and 64k page
> size, and 1M 16M 64M or bigger page will be supported in future, so
> if your patch consider the case, it should work for ia64.  Thanks!
> 
> Take a look at the patch and you tell me. :) I've hardcoded the PFN
> shift as 16, which matches your TARGET_PAGE_BITS, but does it *have*
> to be the same? I don't believe it does.
> Out of curiosity, if you had 4K Linux host pages, and you redefined
> TARGET_PAGE_BITS to be 12, would everything just work?

Yes, if the host linux use 4K page size, we should define TARGET_PAGE_BITS to 12 when compiling qemu. 

> Also, as long as I have you... can we use getpagesize() instead of
> 65536/4096 in qemu_vmalloc()?

We tried it at early stage, seems it can't work, because current code very depens on TARGET_PAGE_BITS. 
Xiantao

RE: [PATCH] qemu: define and use VIRTIO_PFN_SHIFT

[Zhang, Xiantao]

Hollis Blanchard wrote:
> On Thursday 06 November 2008 19:38:40 Zhang, Xiantao wrote:
>> 
>> Hi, Hollis
>> 	Currenlty, kvm-qemu only supports the only case which is host
>> page_size = 
> qemu's target page size for ia64. Does your patch meets the
> requirement ? For ia64, current linux support 4K, 16K and 64k page
> size, and 1M 16M 64M or bigger page will be supported in future, so
> if your patch consider the case, it should work for ia64.  Thanks!
> 
> Take a look at the patch and you tell me. :) I've hardcoded the PFN
> shift as 16, which matches your TARGET_PAGE_BITS, but does it *have*
> to be the same? I don't believe it does.
> Out of curiosity, if you had 4K Linux host pages, and you redefined
> TARGET_PAGE_BITS to be 12, would everything just work?

Yes, if the host linux use 4K page size, we should define TARGET_PAGE_BITS to 12 when compiling qemu. 

> Also, as long as I have you... can we use getpagesize() instead of
> 65536/4096 in qemu_vmalloc()?

We tried it at early stage, seems it can't work, because current code very depens on TARGET_PAGE_BITS. 
Xiantao
--
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] qemu: define and use VIRTIO_PFN_SHIFT

[Zhang, Xiantao]

Hollis Blanchard wrote:
> On Thursday 06 November 2008 19:38:40 Zhang, Xiantao wrote:
>> 
>> Hi, Hollis
>> 	Currenlty, kvm-qemu only supports the only case which is host
>> page_size = 
> qemu's target page size for ia64. Does your patch meets the
> requirement ? For ia64, current linux support 4K, 16K and 64k page
> size, and 1M 16M 64M or bigger page will be supported in future, so
> if your patch consider the case, it should work for ia64.  Thanks!
> 
> Take a look at the patch and you tell me. :) I've hardcoded the PFN
> shift as 16, which matches your TARGET_PAGE_BITS, but does it *have*
> to be the same? I don't believe it does.
> Out of curiosity, if you had 4K Linux host pages, and you redefined
> TARGET_PAGE_BITS to be 12, would everything just work?

Yes, if the host linux use 4K page size, we should define TARGET_PAGE_BITS to 12 when compiling qemu. 

> Also, as long as I have you... can we use getpagesize() instead of
> 65536/4096 in qemu_vmalloc()?

We tried it at early stage, seems it can't work, because current code very depens on TARGET_PAGE_BITS. 
Xiantao

Re: dynamic virtio page size

[Hollis Blanchard]

On Thu, 2008-11-06 at 14:02 -0600, Anthony Liguori wrote:
> Hollis Blanchard wrote:
> > I wanted to make sure people on non-x86 architectures couldn't run into
> > vring-size related problems that didn't also appear on x86.
> >   
> 
> Having a VRING_SHIFT and a VRING_PAGE_SIZE where VRING_PAGE_SIZE != (1 
> << VRING_SHIFT) is almost certainly going to break things in unexpected 
> ways because it's going against common understanding of the relationship 
> between shift and page_size.

In the patch I sent, they are named VIRTIO_PFN_SHIFT and
VRING_PAGE_SIZE. In fact, the first could really be named
VIRTIO_PCI_PFN_SHIFT or even more specific, which hopefully would
alleviate the confusion.

Anyways, I've been trying to implement your other idea, which was to
advertise a "virtio page size" from the host to the guest. Since the
virtio IO space is only 20 bytes long, and it's full, we need to offer a
feature bit to notify the guest that the IO space has been expanded. 

       virtio's PCI IO resource
------------------------------------
|    20 bytes    |                 |
------------------------------------
   virtio-pci       virtio-<device>
    specific          specific
      data              data

If guests don't acknowledge the feature, the host pretends the
VIRTIO_PAGE_SIZE register (which would be the new IO offset 20) doesn't
exist, and reads to offset 20 instead return the device-specific config
data.

Problem: the guest has not yet acknowledged the advertised features
before accessing device-specific data. So in the virtio-blk/virtio-pci
case, virtio_dev_probe() calls virtblk_probe() *before*
vp_finalize_features(). virtblk_probe() uses virtio_config_val(), which
returns the wrong data because the new yet unknown feature flag is still
set. (Of course, we can't just fix the ordering guest-side, because that
breaks backwards compatibility.)

It seems to me that virtio-pci configuration is not very flexible. I
think I'm going to continue revising the hardcoded page size patches I
sent earlier...

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: dynamic virtio page size

[Hollis Blanchard]

On Thu, 2008-11-06 at 14:02 -0600, Anthony Liguori wrote:
> Hollis Blanchard wrote:
> > I wanted to make sure people on non-x86 architectures couldn't run into
> > vring-size related problems that didn't also appear on x86.
> >   
> 
> Having a VRING_SHIFT and a VRING_PAGE_SIZE where VRING_PAGE_SIZE != (1 
> << VRING_SHIFT) is almost certainly going to break things in unexpected 
> ways because it's going against common understanding of the relationship 
> between shift and page_size.

In the patch I sent, they are named VIRTIO_PFN_SHIFT and
VRING_PAGE_SIZE. In fact, the first could really be named
VIRTIO_PCI_PFN_SHIFT or even more specific, which hopefully would
alleviate the confusion.

Anyways, I've been trying to implement your other idea, which was to
advertise a "virtio page size" from the host to the guest. Since the
virtio IO space is only 20 bytes long, and it's full, we need to offer a
feature bit to notify the guest that the IO space has been expanded. 

       virtio's PCI IO resource
------------------------------------
|    20 bytes    |                 |
------------------------------------
   virtio-pci       virtio-<device>
    specific          specific
      data              data

If guests don't acknowledge the feature, the host pretends the
VIRTIO_PAGE_SIZE register (which would be the new IO offset 20) doesn't
exist, and reads to offset 20 instead return the device-specific config
data.

Problem: the guest has not yet acknowledged the advertised features
before accessing device-specific data. So in the virtio-blk/virtio-pci
case, virtio_dev_probe() calls virtblk_probe() *before*
vp_finalize_features(). virtblk_probe() uses virtio_config_val(), which
returns the wrong data because the new yet unknown feature flag is still
set. (Of course, we can't just fix the ordering guest-side, because that
breaks backwards compatibility.)

It seems to me that virtio-pci configuration is not very flexible. I
think I'm going to continue revising the hardcoded page size patches I
sent earlier...

-- 
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: dynamic virtio page size

[Hollis Blanchard]

On Thu, 2008-11-06 at 14:02 -0600, Anthony Liguori wrote:
> Hollis Blanchard wrote:
> > I wanted to make sure people on non-x86 architectures couldn't run into
> > vring-size related problems that didn't also appear on x86.
> >   
> 
> Having a VRING_SHIFT and a VRING_PAGE_SIZE where VRING_PAGE_SIZE != (1 
> << VRING_SHIFT) is almost certainly going to break things in unexpected 
> ways because it's going against common understanding of the relationship 
> between shift and page_size.

In the patch I sent, they are named VIRTIO_PFN_SHIFT and
VRING_PAGE_SIZE. In fact, the first could really be named
VIRTIO_PCI_PFN_SHIFT or even more specific, which hopefully would
alleviate the confusion.

Anyways, I've been trying to implement your other idea, which was to
advertise a "virtio page size" from the host to the guest. Since the
virtio IO space is only 20 bytes long, and it's full, we need to offer a
feature bit to notify the guest that the IO space has been expanded. 

       virtio's PCI IO resource
------------------------------------
|    20 bytes    |                 |
------------------------------------
   virtio-pci       virtio-<device>
    specific          specific
      data              data

If guests don't acknowledge the feature, the host pretends the
VIRTIO_PAGE_SIZE register (which would be the new IO offset 20) doesn't
exist, and reads to offset 20 instead return the device-specific config
data.

Problem: the guest has not yet acknowledged the advertised features
before accessing device-specific data. So in the virtio-blk/virtio-pci
case, virtio_dev_probe() calls virtblk_probe() *before*
vp_finalize_features(). virtblk_probe() uses virtio_config_val(), which
returns the wrong data because the new yet unknown feature flag is still
set. (Of course, we can't just fix the ordering guest-side, because that
breaks backwards compatibility.)

It seems to me that virtio-pci configuration is not very flexible. I
think I'm going to continue revising the hardcoded page size patches I
sent earlier...

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: dynamic virtio page size

[Hollis Blanchard]

On Thu, 2008-11-06 at 17:27 -0600, Hollis Blanchard wrote:
> 
> In the patch I sent, they are named VIRTIO_PFN_SHIFT and
> VRING_PAGE_SIZE. In fact, the first could really be named
> VIRTIO_PCI_PFN_SHIFT or even more specific, which hopefully would
> alleviate the confusion.
> 
> Anyways, I've been trying to implement your other idea, which was to
> advertise a "virtio page size" from the host to the guest. Since the
> virtio IO space is only 20 bytes long, and it's full, we need to offer a
> feature bit to notify the guest that the IO space has been expanded. 
> 
>        virtio's PCI IO resource
> ------------------------------------
> |    20 bytes    |                 |
> ------------------------------------
>    virtio-pci       virtio-<device>
>     specific          specific
>       data              data
> 
> If guests don't acknowledge the feature, the host pretends the
> VIRTIO_PAGE_SIZE register (which would be the new IO offset 20) doesn't
> exist, and reads to offset 20 instead return the device-specific config
> data.

FWIW, here's my current (broken) patch. Aside from being broken, it also
doesn't seem like this approach will scale well should we need to extend
the IO space again in the future.


diff --git a/qemu/hw/virtio.c b/qemu/hw/virtio.c
--- a/qemu/hw/virtio.c
+++ b/qemu/hw/virtio.c
@@ -45,7 +45,11 @@
  * a read-and-acknowledge. */
 #define VIRTIO_PCI_ISR			19
 
-#define VIRTIO_PCI_CONFIG		20
+/* A 32-bit r/o register specifying the size of a "page", in bytes, in the
+ * virtio interface. */
+#define VIRTIO_PCI_PAGESIZE		20
+
+#define VIRTIO_PCI_CONFIG		24
 
 /* Virtio ABI version, if we increment this, we break the guest driver. */
 #define VIRTIO_PCI_ABI_VERSION		0
@@ -55,6 +59,12 @@
  * KVM or if kqemu gets SMP support.
  */
 #define wmb() do { } while (0)
+
+#define VIRTIO_PAGE_SHIFT		12
+#define VIRTIO_PAGE_SIZE		(1<<VIRTIO_PAGE_SHIFT)
+#define VIRTIO_PAGE_MASK		(~(VIRTIO_PAGE_SIZE - 1))
+#define VIRTIO_PAGE_ALIGN(x)		(((x) + VIRTIO_PAGE_SIZE - 1) \
+					 & VIRTIO_PAGE_MASK)
 
 /* virt queue functions */
 
@@ -95,7 +105,7 @@ static void *virtio_map_gpa(target_phys_
 
 static size_t virtqueue_size(int num)
 {
-    return TARGET_PAGE_ALIGN((sizeof(VRingDesc) * num) +
+    return VIRTIO_PAGE_ALIGN((sizeof(VRingDesc) * num) +
 			     (sizeof(VRingAvail) + sizeof(uint16_t) * num)) +
 	(sizeof(VRingUsed) + sizeof(VRingUsedElem) * num);
 }
@@ -104,7 +114,7 @@ static void virtqueue_init(VirtQueue *vq
 {
     vq->vring.desc = p;
     vq->vring.avail = p + vq->vring.num * sizeof(VRingDesc);
-    vq->vring.used = (void *)TARGET_PAGE_ALIGN((unsigned long)&vq->vring.avail->ring[vq->vring.num]);
+    vq->vring.used = (void *)VIRTIO_PAGE_ALIGN((unsigned long)&vq->vring.avail->ring[vq->vring.num]);
 }
 
 static unsigned virtqueue_next_desc(VirtQueue *vq, unsigned int i)
@@ -234,6 +244,9 @@ static uint32_t virtio_config_readb(void
 
     vdev->get_config(vdev, vdev->config);
 
+    if (!(vdev->features & VIRTIO_F_EXPLICIT_PAGE_SIZE))
+	addr += 4;
+
     addr -= vdev->addr + VIRTIO_PCI_CONFIG;
     if (addr > (vdev->config_len - sizeof(val)))
 	return (uint32_t)-1;
@@ -248,6 +261,9 @@ static uint32_t virtio_config_readw(void
     uint16_t val;
 
     vdev->get_config(vdev, vdev->config);
+
+    if (!(vdev->features & VIRTIO_F_EXPLICIT_PAGE_SIZE))
+	addr += 4;
 
     addr -= vdev->addr + VIRTIO_PCI_CONFIG;
     if (addr > (vdev->config_len - sizeof(val)))
@@ -264,6 +280,9 @@ static uint32_t virtio_config_readl(void
 
     vdev->get_config(vdev, vdev->config);
 
+    if (!(vdev->features & VIRTIO_F_EXPLICIT_PAGE_SIZE))
+	addr += 4;
+
     addr -= vdev->addr + VIRTIO_PCI_CONFIG;
     if (addr > (vdev->config_len - sizeof(val)))
 	return (uint32_t)-1;
@@ -276,6 +295,9 @@ static void virtio_config_writeb(void *o
 {
     VirtIODevice *vdev = opaque;
     uint8_t val = data;
+
+    if (!(vdev->features & VIRTIO_F_EXPLICIT_PAGE_SIZE))
+	addr += 4;
 
     addr -= vdev->addr + VIRTIO_PCI_CONFIG;
     if (addr > (vdev->config_len - sizeof(val)))
@@ -292,6 +314,9 @@ static void virtio_config_writew(void *o
     VirtIODevice *vdev = opaque;
     uint16_t val = data;
 
+    if (!(vdev->features & VIRTIO_F_EXPLICIT_PAGE_SIZE))
+	addr += 4;
+
     addr -= vdev->addr + VIRTIO_PCI_CONFIG;
     if (addr > (vdev->config_len - sizeof(val)))
 	return;
@@ -306,6 +331,9 @@ static void virtio_config_writel(void *o
 {
     VirtIODevice *vdev = opaque;
     uint32_t val = data;
+
+    if (!(vdev->features & VIRTIO_F_EXPLICIT_PAGE_SIZE))
+	addr += 4;
 
     addr -= vdev->addr + VIRTIO_PCI_CONFIG;
     if (addr > (vdev->config_len - sizeof(val)))
@@ -329,9 +357,10 @@ static void virtio_ioport_write(void *op
 	if (vdev->set_features)
 	    vdev->set_features(vdev, val);
 	vdev->features = val;
+	printf("%s: features 0x%x\n", __func__, val);
 	break;
     case VIRTIO_PCI_QUEUE_PFN:
-	pa = (ram_addr_t)val << TARGET_PAGE_BITS;
+	pa = (ram_addr_t)val << VIRTIO_PAGE_SHIFT;
 	vdev->vq[vdev->queue_sel].pfn = val;
 	if (pa = 0) {
             virtio_reset(vdev);
@@ -368,6 +397,7 @@ static uint32_t virtio_ioport_read(void 
     case VIRTIO_PCI_HOST_FEATURES:
 	ret = vdev->get_features(vdev);
 	ret |= (1 << VIRTIO_F_NOTIFY_ON_EMPTY);
+	ret |= (1 << VIRTIO_F_EXPLICIT_PAGE_SIZE);
 	break;
     case VIRTIO_PCI_GUEST_FEATURES:
 	ret = vdev->features;
@@ -390,6 +420,16 @@ static uint32_t virtio_ioport_read(void 
 	vdev->isr = 0;
 	virtio_update_irq(vdev);
 	break;
+    case VIRTIO_PCI_PAGESIZE:
+	if (vdev->features & VIRTIO_F_EXPLICIT_PAGE_SIZE) {
+	    /* Guest must have acknowledged this feature in order to enable
+	     * this IO port. */
+	    ret = VIRTIO_PAGE_SIZE;
+	} else {
+	    /* If it hasn't, return device config space instead. */
+	    ret = virtio_config_readl(vdev, addr);
+	}
+	break;
     default:
 	break;
     }
@@ -405,22 +445,24 @@ static void virtio_map(PCIDevice *pci_de
 
     vdev->addr = addr;
     for (i = 0; i < 3; i++) {
-	register_ioport_write(addr, 20, 1 << i, virtio_ioport_write, vdev);
-	register_ioport_read(addr, 20, 1 << i, virtio_ioport_read, vdev);
+	register_ioport_write(addr, VIRTIO_PCI_CONFIG, 1 << i,
+	                      virtio_ioport_write, vdev);
+	register_ioport_read(addr, VIRTIO_PCI_CONFIG, 1 << i,
+	                     virtio_ioport_read, vdev);
     }
 
     if (vdev->config_len) {
-	register_ioport_write(addr + 20, vdev->config_len, 1,
+	register_ioport_write(addr + VIRTIO_PCI_CONFIG, vdev->config_len, 1,
 			      virtio_config_writeb, vdev);
-	register_ioport_write(addr + 20, vdev->config_len, 2,
+	register_ioport_write(addr + VIRTIO_PCI_CONFIG, vdev->config_len, 2,
 			      virtio_config_writew, vdev);
-	register_ioport_write(addr + 20, vdev->config_len, 4,
+	register_ioport_write(addr + VIRTIO_PCI_CONFIG, vdev->config_len, 4,
 			      virtio_config_writel, vdev);
-	register_ioport_read(addr + 20, vdev->config_len, 1,
+	register_ioport_read(addr + VIRTIO_PCI_CONFIG, vdev->config_len, 1,
 			     virtio_config_readb, vdev);
-	register_ioport_read(addr + 20, vdev->config_len, 2,
+	register_ioport_read(addr + VIRTIO_PCI_CONFIG, vdev->config_len, 2,
 			     virtio_config_readw, vdev);
-	register_ioport_read(addr + 20, vdev->config_len, 4,
+	register_ioport_read(addr + VIRTIO_PCI_CONFIG, vdev->config_len, 4,
 			     virtio_config_readl, vdev);
 
 	vdev->get_config(vdev, vdev->config);
@@ -519,7 +561,7 @@ void virtio_load(VirtIODevice *vdev, QEM
 	    size_t size;
 	    target_phys_addr_t pa;
 
-	    pa = (ram_addr_t)vdev->vq[i].pfn << TARGET_PAGE_BITS;
+	    pa = (ram_addr_t)vdev->vq[i].pfn << VIRTIO_PAGE_SHIFT;
 	    size = virtqueue_size(vdev->vq[i].vring.num);
 	    virtqueue_init(&vdev->vq[i], virtio_map_gpa(pa, size));
 	}
diff --git a/qemu/hw/virtio.h b/qemu/hw/virtio.h
--- a/qemu/hw/virtio.h
+++ b/qemu/hw/virtio.h
@@ -33,6 +33,9 @@
 /* We notify when the ring is completely used, even if the guest is supressing
  * callbacks */
 #define VIRTIO_F_NOTIFY_ON_EMPTY        24
+
+/* Device advertises the size of a "page". */
+#define VIRTIO_F_EXPLICIT_PAGE_SIZE	28
 
 /* from Linux's linux/virtio_ring.h */
 


-- 
Hollis Blanchard
IBM Linux Technology Center

Re: dynamic virtio page size

[Hollis Blanchard]

On Thu, 2008-11-06 at 17:27 -0600, Hollis Blanchard wrote:
> 
> In the patch I sent, they are named VIRTIO_PFN_SHIFT and
> VRING_PAGE_SIZE. In fact, the first could really be named
> VIRTIO_PCI_PFN_SHIFT or even more specific, which hopefully would
> alleviate the confusion.
> 
> Anyways, I've been trying to implement your other idea, which was to
> advertise a "virtio page size" from the host to the guest. Since the
> virtio IO space is only 20 bytes long, and it's full, we need to offer a
> feature bit to notify the guest that the IO space has been expanded. 
> 
>        virtio's PCI IO resource
> ------------------------------------
> |    20 bytes    |                 |
> ------------------------------------
>    virtio-pci       virtio-<device>
>     specific          specific
>       data              data
> 
> If guests don't acknowledge the feature, the host pretends the
> VIRTIO_PAGE_SIZE register (which would be the new IO offset 20) doesn't
> exist, and reads to offset 20 instead return the device-specific config
> data.

FWIW, here's my current (broken) patch. Aside from being broken, it also
doesn't seem like this approach will scale well should we need to extend
the IO space again in the future.


diff --git a/qemu/hw/virtio.c b/qemu/hw/virtio.c
--- a/qemu/hw/virtio.c
+++ b/qemu/hw/virtio.c
@@ -45,7 +45,11 @@
  * a read-and-acknowledge. */
 #define VIRTIO_PCI_ISR			19
 
-#define VIRTIO_PCI_CONFIG		20
+/* A 32-bit r/o register specifying the size of a "page", in bytes, in the
+ * virtio interface. */
+#define VIRTIO_PCI_PAGESIZE		20
+
+#define VIRTIO_PCI_CONFIG		24
 
 /* Virtio ABI version, if we increment this, we break the guest driver. */
 #define VIRTIO_PCI_ABI_VERSION		0
@@ -55,6 +59,12 @@
  * KVM or if kqemu gets SMP support.
  */
 #define wmb() do { } while (0)
+
+#define VIRTIO_PAGE_SHIFT		12
+#define VIRTIO_PAGE_SIZE		(1<<VIRTIO_PAGE_SHIFT)
+#define VIRTIO_PAGE_MASK		(~(VIRTIO_PAGE_SIZE - 1))
+#define VIRTIO_PAGE_ALIGN(x)		(((x) + VIRTIO_PAGE_SIZE - 1) \
+					 & VIRTIO_PAGE_MASK)
 
 /* virt queue functions */
 
@@ -95,7 +105,7 @@ static void *virtio_map_gpa(target_phys_
 
 static size_t virtqueue_size(int num)
 {
-    return TARGET_PAGE_ALIGN((sizeof(VRingDesc) * num) +
+    return VIRTIO_PAGE_ALIGN((sizeof(VRingDesc) * num) +
 			     (sizeof(VRingAvail) + sizeof(uint16_t) * num)) +
 	(sizeof(VRingUsed) + sizeof(VRingUsedElem) * num);
 }
@@ -104,7 +114,7 @@ static void virtqueue_init(VirtQueue *vq
 {
     vq->vring.desc = p;
     vq->vring.avail = p + vq->vring.num * sizeof(VRingDesc);
-    vq->vring.used = (void *)TARGET_PAGE_ALIGN((unsigned long)&vq->vring.avail->ring[vq->vring.num]);
+    vq->vring.used = (void *)VIRTIO_PAGE_ALIGN((unsigned long)&vq->vring.avail->ring[vq->vring.num]);
 }
 
 static unsigned virtqueue_next_desc(VirtQueue *vq, unsigned int i)
@@ -234,6 +244,9 @@ static uint32_t virtio_config_readb(void
 
     vdev->get_config(vdev, vdev->config);
 
+    if (!(vdev->features & VIRTIO_F_EXPLICIT_PAGE_SIZE))
+	addr += 4;
+
     addr -= vdev->addr + VIRTIO_PCI_CONFIG;
     if (addr > (vdev->config_len - sizeof(val)))
 	return (uint32_t)-1;
@@ -248,6 +261,9 @@ static uint32_t virtio_config_readw(void
     uint16_t val;
 
     vdev->get_config(vdev, vdev->config);
+
+    if (!(vdev->features & VIRTIO_F_EXPLICIT_PAGE_SIZE))
+	addr += 4;
 
     addr -= vdev->addr + VIRTIO_PCI_CONFIG;
     if (addr > (vdev->config_len - sizeof(val)))
@@ -264,6 +280,9 @@ static uint32_t virtio_config_readl(void
 
     vdev->get_config(vdev, vdev->config);
 
+    if (!(vdev->features & VIRTIO_F_EXPLICIT_PAGE_SIZE))
+	addr += 4;
+
     addr -= vdev->addr + VIRTIO_PCI_CONFIG;
     if (addr > (vdev->config_len - sizeof(val)))
 	return (uint32_t)-1;
@@ -276,6 +295,9 @@ static void virtio_config_writeb(void *o
 {
     VirtIODevice *vdev = opaque;
     uint8_t val = data;
+
+    if (!(vdev->features & VIRTIO_F_EXPLICIT_PAGE_SIZE))
+	addr += 4;
 
     addr -= vdev->addr + VIRTIO_PCI_CONFIG;
     if (addr > (vdev->config_len - sizeof(val)))
@@ -292,6 +314,9 @@ static void virtio_config_writew(void *o
     VirtIODevice *vdev = opaque;
     uint16_t val = data;
 
+    if (!(vdev->features & VIRTIO_F_EXPLICIT_PAGE_SIZE))
+	addr += 4;
+
     addr -= vdev->addr + VIRTIO_PCI_CONFIG;
     if (addr > (vdev->config_len - sizeof(val)))
 	return;
@@ -306,6 +331,9 @@ static void virtio_config_writel(void *o
 {
     VirtIODevice *vdev = opaque;
     uint32_t val = data;
+
+    if (!(vdev->features & VIRTIO_F_EXPLICIT_PAGE_SIZE))
+	addr += 4;
 
     addr -= vdev->addr + VIRTIO_PCI_CONFIG;
     if (addr > (vdev->config_len - sizeof(val)))
@@ -329,9 +357,10 @@ static void virtio_ioport_write(void *op
 	if (vdev->set_features)
 	    vdev->set_features(vdev, val);
 	vdev->features = val;
+	printf("%s: features 0x%x\n", __func__, val);
 	break;
     case VIRTIO_PCI_QUEUE_PFN:
-	pa = (ram_addr_t)val << TARGET_PAGE_BITS;
+	pa = (ram_addr_t)val << VIRTIO_PAGE_SHIFT;
 	vdev->vq[vdev->queue_sel].pfn = val;
 	if (pa == 0) {
             virtio_reset(vdev);
@@ -368,6 +397,7 @@ static uint32_t virtio_ioport_read(void 
     case VIRTIO_PCI_HOST_FEATURES:
 	ret = vdev->get_features(vdev);
 	ret |= (1 << VIRTIO_F_NOTIFY_ON_EMPTY);
+	ret |= (1 << VIRTIO_F_EXPLICIT_PAGE_SIZE);
 	break;
     case VIRTIO_PCI_GUEST_FEATURES:
 	ret = vdev->features;
@@ -390,6 +420,16 @@ static uint32_t virtio_ioport_read(void 
 	vdev->isr = 0;
 	virtio_update_irq(vdev);
 	break;
+    case VIRTIO_PCI_PAGESIZE:
+	if (vdev->features & VIRTIO_F_EXPLICIT_PAGE_SIZE) {
+	    /* Guest must have acknowledged this feature in order to enable
+	     * this IO port. */
+	    ret = VIRTIO_PAGE_SIZE;
+	} else {
+	    /* If it hasn't, return device config space instead. */
+	    ret = virtio_config_readl(vdev, addr);
+	}
+	break;
     default:
 	break;
     }
@@ -405,22 +445,24 @@ static void virtio_map(PCIDevice *pci_de
 
     vdev->addr = addr;
     for (i = 0; i < 3; i++) {
-	register_ioport_write(addr, 20, 1 << i, virtio_ioport_write, vdev);
-	register_ioport_read(addr, 20, 1 << i, virtio_ioport_read, vdev);
+	register_ioport_write(addr, VIRTIO_PCI_CONFIG, 1 << i,
+	                      virtio_ioport_write, vdev);
+	register_ioport_read(addr, VIRTIO_PCI_CONFIG, 1 << i,
+	                     virtio_ioport_read, vdev);
     }
 
     if (vdev->config_len) {
-	register_ioport_write(addr + 20, vdev->config_len, 1,
+	register_ioport_write(addr + VIRTIO_PCI_CONFIG, vdev->config_len, 1,
 			      virtio_config_writeb, vdev);
-	register_ioport_write(addr + 20, vdev->config_len, 2,
+	register_ioport_write(addr + VIRTIO_PCI_CONFIG, vdev->config_len, 2,
 			      virtio_config_writew, vdev);
-	register_ioport_write(addr + 20, vdev->config_len, 4,
+	register_ioport_write(addr + VIRTIO_PCI_CONFIG, vdev->config_len, 4,
 			      virtio_config_writel, vdev);
-	register_ioport_read(addr + 20, vdev->config_len, 1,
+	register_ioport_read(addr + VIRTIO_PCI_CONFIG, vdev->config_len, 1,
 			     virtio_config_readb, vdev);
-	register_ioport_read(addr + 20, vdev->config_len, 2,
+	register_ioport_read(addr + VIRTIO_PCI_CONFIG, vdev->config_len, 2,
 			     virtio_config_readw, vdev);
-	register_ioport_read(addr + 20, vdev->config_len, 4,
+	register_ioport_read(addr + VIRTIO_PCI_CONFIG, vdev->config_len, 4,
 			     virtio_config_readl, vdev);
 
 	vdev->get_config(vdev, vdev->config);
@@ -519,7 +561,7 @@ void virtio_load(VirtIODevice *vdev, QEM
 	    size_t size;
 	    target_phys_addr_t pa;
 
-	    pa = (ram_addr_t)vdev->vq[i].pfn << TARGET_PAGE_BITS;
+	    pa = (ram_addr_t)vdev->vq[i].pfn << VIRTIO_PAGE_SHIFT;
 	    size = virtqueue_size(vdev->vq[i].vring.num);
 	    virtqueue_init(&vdev->vq[i], virtio_map_gpa(pa, size));
 	}
diff --git a/qemu/hw/virtio.h b/qemu/hw/virtio.h
--- a/qemu/hw/virtio.h
+++ b/qemu/hw/virtio.h
@@ -33,6 +33,9 @@
 /* We notify when the ring is completely used, even if the guest is supressing
  * callbacks */
 #define VIRTIO_F_NOTIFY_ON_EMPTY        24
+
+/* Device advertises the size of a "page". */
+#define VIRTIO_F_EXPLICIT_PAGE_SIZE	28
 
 /* from Linux's linux/virtio_ring.h */
 


-- 
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: dynamic virtio page size

[Hollis Blanchard]

On Thu, 2008-11-06 at 17:27 -0600, Hollis Blanchard wrote:
> 
> In the patch I sent, they are named VIRTIO_PFN_SHIFT and
> VRING_PAGE_SIZE. In fact, the first could really be named
> VIRTIO_PCI_PFN_SHIFT or even more specific, which hopefully would
> alleviate the confusion.
> 
> Anyways, I've been trying to implement your other idea, which was to
> advertise a "virtio page size" from the host to the guest. Since the
> virtio IO space is only 20 bytes long, and it's full, we need to offer a
> feature bit to notify the guest that the IO space has been expanded. 
> 
>        virtio's PCI IO resource
> ------------------------------------
> |    20 bytes    |                 |
> ------------------------------------
>    virtio-pci       virtio-<device>
>     specific          specific
>       data              data
> 
> If guests don't acknowledge the feature, the host pretends the
> VIRTIO_PAGE_SIZE register (which would be the new IO offset 20) doesn't
> exist, and reads to offset 20 instead return the device-specific config
> data.

FWIW, here's my current (broken) patch. Aside from being broken, it also
doesn't seem like this approach will scale well should we need to extend
the IO space again in the future.


diff --git a/qemu/hw/virtio.c b/qemu/hw/virtio.c
--- a/qemu/hw/virtio.c
+++ b/qemu/hw/virtio.c
@@ -45,7 +45,11 @@
  * a read-and-acknowledge. */
 #define VIRTIO_PCI_ISR			19
 
-#define VIRTIO_PCI_CONFIG		20
+/* A 32-bit r/o register specifying the size of a "page", in bytes, in the
+ * virtio interface. */
+#define VIRTIO_PCI_PAGESIZE		20
+
+#define VIRTIO_PCI_CONFIG		24
 
 /* Virtio ABI version, if we increment this, we break the guest driver. */
 #define VIRTIO_PCI_ABI_VERSION		0
@@ -55,6 +59,12 @@
  * KVM or if kqemu gets SMP support.
  */
 #define wmb() do { } while (0)
+
+#define VIRTIO_PAGE_SHIFT		12
+#define VIRTIO_PAGE_SIZE		(1<<VIRTIO_PAGE_SHIFT)
+#define VIRTIO_PAGE_MASK		(~(VIRTIO_PAGE_SIZE - 1))
+#define VIRTIO_PAGE_ALIGN(x)		(((x) + VIRTIO_PAGE_SIZE - 1) \
+					 & VIRTIO_PAGE_MASK)
 
 /* virt queue functions */
 
@@ -95,7 +105,7 @@ static void *virtio_map_gpa(target_phys_
 
 static size_t virtqueue_size(int num)
 {
-    return TARGET_PAGE_ALIGN((sizeof(VRingDesc) * num) +
+    return VIRTIO_PAGE_ALIGN((sizeof(VRingDesc) * num) +
 			     (sizeof(VRingAvail) + sizeof(uint16_t) * num)) +
 	(sizeof(VRingUsed) + sizeof(VRingUsedElem) * num);
 }
@@ -104,7 +114,7 @@ static void virtqueue_init(VirtQueue *vq
 {
     vq->vring.desc = p;
     vq->vring.avail = p + vq->vring.num * sizeof(VRingDesc);
-    vq->vring.used = (void *)TARGET_PAGE_ALIGN((unsigned long)&vq->vring.avail->ring[vq->vring.num]);
+    vq->vring.used = (void *)VIRTIO_PAGE_ALIGN((unsigned long)&vq->vring.avail->ring[vq->vring.num]);
 }
 
 static unsigned virtqueue_next_desc(VirtQueue *vq, unsigned int i)
@@ -234,6 +244,9 @@ static uint32_t virtio_config_readb(void
 
     vdev->get_config(vdev, vdev->config);
 
+    if (!(vdev->features & VIRTIO_F_EXPLICIT_PAGE_SIZE))
+	addr += 4;
+
     addr -= vdev->addr + VIRTIO_PCI_CONFIG;
     if (addr > (vdev->config_len - sizeof(val)))
 	return (uint32_t)-1;
@@ -248,6 +261,9 @@ static uint32_t virtio_config_readw(void
     uint16_t val;
 
     vdev->get_config(vdev, vdev->config);
+
+    if (!(vdev->features & VIRTIO_F_EXPLICIT_PAGE_SIZE))
+	addr += 4;
 
     addr -= vdev->addr + VIRTIO_PCI_CONFIG;
     if (addr > (vdev->config_len - sizeof(val)))
@@ -264,6 +280,9 @@ static uint32_t virtio_config_readl(void
 
     vdev->get_config(vdev, vdev->config);
 
+    if (!(vdev->features & VIRTIO_F_EXPLICIT_PAGE_SIZE))
+	addr += 4;
+
     addr -= vdev->addr + VIRTIO_PCI_CONFIG;
     if (addr > (vdev->config_len - sizeof(val)))
 	return (uint32_t)-1;
@@ -276,6 +295,9 @@ static void virtio_config_writeb(void *o
 {
     VirtIODevice *vdev = opaque;
     uint8_t val = data;
+
+    if (!(vdev->features & VIRTIO_F_EXPLICIT_PAGE_SIZE))
+	addr += 4;
 
     addr -= vdev->addr + VIRTIO_PCI_CONFIG;
     if (addr > (vdev->config_len - sizeof(val)))
@@ -292,6 +314,9 @@ static void virtio_config_writew(void *o
     VirtIODevice *vdev = opaque;
     uint16_t val = data;
 
+    if (!(vdev->features & VIRTIO_F_EXPLICIT_PAGE_SIZE))
+	addr += 4;
+
     addr -= vdev->addr + VIRTIO_PCI_CONFIG;
     if (addr > (vdev->config_len - sizeof(val)))
 	return;
@@ -306,6 +331,9 @@ static void virtio_config_writel(void *o
 {
     VirtIODevice *vdev = opaque;
     uint32_t val = data;
+
+    if (!(vdev->features & VIRTIO_F_EXPLICIT_PAGE_SIZE))
+	addr += 4;
 
     addr -= vdev->addr + VIRTIO_PCI_CONFIG;
     if (addr > (vdev->config_len - sizeof(val)))
@@ -329,9 +357,10 @@ static void virtio_ioport_write(void *op
 	if (vdev->set_features)
 	    vdev->set_features(vdev, val);
 	vdev->features = val;
+	printf("%s: features 0x%x\n", __func__, val);
 	break;
     case VIRTIO_PCI_QUEUE_PFN:
-	pa = (ram_addr_t)val << TARGET_PAGE_BITS;
+	pa = (ram_addr_t)val << VIRTIO_PAGE_SHIFT;
 	vdev->vq[vdev->queue_sel].pfn = val;
 	if (pa = 0) {
             virtio_reset(vdev);
@@ -368,6 +397,7 @@ static uint32_t virtio_ioport_read(void 
     case VIRTIO_PCI_HOST_FEATURES:
 	ret = vdev->get_features(vdev);
 	ret |= (1 << VIRTIO_F_NOTIFY_ON_EMPTY);
+	ret |= (1 << VIRTIO_F_EXPLICIT_PAGE_SIZE);
 	break;
     case VIRTIO_PCI_GUEST_FEATURES:
 	ret = vdev->features;
@@ -390,6 +420,16 @@ static uint32_t virtio_ioport_read(void 
 	vdev->isr = 0;
 	virtio_update_irq(vdev);
 	break;
+    case VIRTIO_PCI_PAGESIZE:
+	if (vdev->features & VIRTIO_F_EXPLICIT_PAGE_SIZE) {
+	    /* Guest must have acknowledged this feature in order to enable
+	     * this IO port. */
+	    ret = VIRTIO_PAGE_SIZE;
+	} else {
+	    /* If it hasn't, return device config space instead. */
+	    ret = virtio_config_readl(vdev, addr);
+	}
+	break;
     default:
 	break;
     }
@@ -405,22 +445,24 @@ static void virtio_map(PCIDevice *pci_de
 
     vdev->addr = addr;
     for (i = 0; i < 3; i++) {
-	register_ioport_write(addr, 20, 1 << i, virtio_ioport_write, vdev);
-	register_ioport_read(addr, 20, 1 << i, virtio_ioport_read, vdev);
+	register_ioport_write(addr, VIRTIO_PCI_CONFIG, 1 << i,
+	                      virtio_ioport_write, vdev);
+	register_ioport_read(addr, VIRTIO_PCI_CONFIG, 1 << i,
+	                     virtio_ioport_read, vdev);
     }
 
     if (vdev->config_len) {
-	register_ioport_write(addr + 20, vdev->config_len, 1,
+	register_ioport_write(addr + VIRTIO_PCI_CONFIG, vdev->config_len, 1,
 			      virtio_config_writeb, vdev);
-	register_ioport_write(addr + 20, vdev->config_len, 2,
+	register_ioport_write(addr + VIRTIO_PCI_CONFIG, vdev->config_len, 2,
 			      virtio_config_writew, vdev);
-	register_ioport_write(addr + 20, vdev->config_len, 4,
+	register_ioport_write(addr + VIRTIO_PCI_CONFIG, vdev->config_len, 4,
 			      virtio_config_writel, vdev);
-	register_ioport_read(addr + 20, vdev->config_len, 1,
+	register_ioport_read(addr + VIRTIO_PCI_CONFIG, vdev->config_len, 1,
 			     virtio_config_readb, vdev);
-	register_ioport_read(addr + 20, vdev->config_len, 2,
+	register_ioport_read(addr + VIRTIO_PCI_CONFIG, vdev->config_len, 2,
 			     virtio_config_readw, vdev);
-	register_ioport_read(addr + 20, vdev->config_len, 4,
+	register_ioport_read(addr + VIRTIO_PCI_CONFIG, vdev->config_len, 4,
 			     virtio_config_readl, vdev);
 
 	vdev->get_config(vdev, vdev->config);
@@ -519,7 +561,7 @@ void virtio_load(VirtIODevice *vdev, QEM
 	    size_t size;
 	    target_phys_addr_t pa;
 
-	    pa = (ram_addr_t)vdev->vq[i].pfn << TARGET_PAGE_BITS;
+	    pa = (ram_addr_t)vdev->vq[i].pfn << VIRTIO_PAGE_SHIFT;
 	    size = virtqueue_size(vdev->vq[i].vring.num);
 	    virtqueue_init(&vdev->vq[i], virtio_map_gpa(pa, size));
 	}
diff --git a/qemu/hw/virtio.h b/qemu/hw/virtio.h
--- a/qemu/hw/virtio.h
+++ b/qemu/hw/virtio.h
@@ -33,6 +33,9 @@
 /* We notify when the ring is completely used, even if the guest is supressing
  * callbacks */
 #define VIRTIO_F_NOTIFY_ON_EMPTY        24
+
+/* Device advertises the size of a "page". */
+#define VIRTIO_F_EXPLICIT_PAGE_SIZE	28
 
 /* from Linux's linux/virtio_ring.h */
 


-- 
Hollis Blanchard
IBM Linux Technology Center

[PATCH] qemu: define and use VIRTIO_PFN_SHIFT

[Hollis Blanchard]

# HG changeset patch
# User Hollis Blanchard <hollisb@us.ibm.com>
# Date 1225946837 21600
# Node ID 43a111ea61b542d3823e2a11d017e7b06b7ec254
# Parent  b63967268af119e0faa4adc3086cdef857815548
qemu: define and use VIRTIO_PFN_SHIFT

The virtio front and back ends must agree about how big a pfn really is. Since
qemu has no idea what "page size" the guest may be using, it must be
independent of TARGET_PAGE_BITS.

This patch should have no functional effect on x86 or ia64, but I'd like an ack from the
ia64 guys.

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

diff --git a/qemu/hw/virtio.c b/qemu/hw/virtio.c
--- a/qemu/hw/virtio.c
+++ b/qemu/hw/virtio.c
@@ -56,6 +56,10 @@
  */
 #define wmb() do { } while (0)
 
+#define VRING_PAGE_SIZE (1<<12)
+
+#define ALIGN(x, a)  (((x)+(a)-1) & ~((a)-1))
+
 /* virt queue functions */
 
 static void *virtio_map_gpa(target_phys_addr_t addr, size_t size)
@@ -95,8 +99,8 @@ static void *virtio_map_gpa(target_phys_
 
 static size_t virtqueue_size(int num)
 {
-    return TARGET_PAGE_ALIGN((sizeof(VRingDesc) * num) +
-			     (sizeof(VRingAvail) + sizeof(uint16_t) * num)) +
+    return ALIGN((sizeof(VRingDesc) * num) + (sizeof(VRingAvail) +
+		 sizeof(uint16_t) * num), VRING_PAGE_SIZE) +
 	(sizeof(VRingUsed) + sizeof(VRingUsedElem) * num);
 }
 
@@ -104,7 +108,7 @@ static void virtqueue_init(VirtQueue *vq
 {
     vq->vring.desc = p;
     vq->vring.avail = p + vq->vring.num * sizeof(VRingDesc);
-    vq->vring.used = (void *)TARGET_PAGE_ALIGN((unsigned long)&vq->vring.avail->ring[vq->vring.num]);
+    vq->vring.used = (void *)ALIGN((unsigned long)&vq->vring.avail->ring[vq->vring.num], VRING_PAGE_SIZE);
 }
 
 static unsigned virtqueue_next_desc(VirtQueue *vq, unsigned int i)
@@ -241,7 +245,7 @@ static void virtio_ioport_write(void *op
 	vdev->features = val;
 	break;
     case VIRTIO_PCI_QUEUE_PFN:
-	pa = (ram_addr_t)val << TARGET_PAGE_BITS;
+	pa = (ram_addr_t)val << VIRTIO_PFN_SHIFT;
 	vdev->vq[vdev->queue_sel].pfn = val;
 	if (pa == 0) {
             virtio_reset(vdev);
@@ -519,7 +523,7 @@ void virtio_load(VirtIODevice *vdev, QEM
 	    size_t size;
 	    target_phys_addr_t pa;
 
-	    pa = (ram_addr_t)vdev->vq[i].pfn << TARGET_PAGE_BITS;
+	    pa = (ram_addr_t)vdev->vq[i].pfn << VIRTIO_PFN_SHIFT;
 	    size = virtqueue_size(vdev->vq[i].vring.num);
 	    virtqueue_init(&vdev->vq[i], virtio_map_gpa(pa, size));
 	}
diff --git a/qemu/target-i386/cpu.h b/qemu/target-i386/cpu.h
--- a/qemu/target-i386/cpu.h
+++ b/qemu/target-i386/cpu.h
@@ -751,6 +751,8 @@ static inline int cpu_get_time_fast(void
 
 #define TARGET_PAGE_BITS 12
 
+#define VIRTIO_PFN_SHIFT 12
+
 #define CPUState CPUX86State
 #define cpu_init cpu_x86_init
 #define cpu_exec cpu_x86_exec
diff --git a/qemu/target-ia64/cpu.h b/qemu/target-ia64/cpu.h
--- a/qemu/target-ia64/cpu.h
+++ b/qemu/target-ia64/cpu.h
@@ -30,6 +30,8 @@
 #define TARGET_LONG_BITS 64
 
 #define TARGET_PAGE_BITS 16
+
+#define VIRTIO_PFN_SHIFT 16
 
 #define ELF_MACHINE	EM_IA_64
 
diff --git a/qemu/target-ppc/cpu.h b/qemu/target-ppc/cpu.h
--- a/qemu/target-ppc/cpu.h
+++ b/qemu/target-ppc/cpu.h
@@ -54,6 +54,8 @@
 #endif /* defined(TARGET_PPCEMB) */
 
 #endif /* defined (TARGET_PPC64) */
+
+#define VIRTIO_PFN_SHIFT 10
 
 #include "cpu-defs.h"