[Qemu-devel] [PATCH 0/2] build QEMU with Xen support on ARM

[Qemu-devel] [PATCH 0/2] build QEMU with Xen support on ARM

[Stefano Stabellini]

Hi all,
the xenpv machine provides Xen paravirtualized backends for console,
disk and framebuffer. xenfb in particular is the only open source
framebuffer backend available.
On ARM we don't need QEMU to emulate any hardware but xenpv would still
be useful at least to provide xenfb.
This patch series allows QEMU to build and run (xenpv) with Xen support
on ARM.


Stefano Stabellini (2):
      xen_backend: introduce xenstore_read_uint64 and xenstore_read_fe_uint64
      xen: build on ARM

 hw/display/xenfb.c           |   17 +++++++++--------
 hw/xen/xen_backend.c         |   18 ++++++++++++++++++
 include/hw/xen/xen_backend.h |    2 ++
 xen-all.c                    |    2 +-
 xen-mapcache.c               |    4 ++--
 5 files changed, 32 insertions(+), 11 deletions(-)

[Qemu-devel] [PATCH 1/2] xen_backend: introduce xenstore_read_uint64 and xenstore_read_fe_uint64

[Stefano Stabellini]

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/xen/xen_backend.c         |   18 ++++++++++++++++++
 include/hw/xen/xen_backend.h |    2 ++
 2 files changed, 20 insertions(+)

diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
index 197795f..5f9be5a 100644
--- a/hw/xen/xen_backend.c
+++ b/hw/xen/xen_backend.c
@@ -112,6 +112,19 @@ int xenstore_read_int(const char *base, const char *node, int *ival)
     return rc;
 }
 
+int xenstore_read_uint64(const char *base, const char *node, uint64_t *uval)
+{
+    char *val;
+    int rc = -1;
+
+    val = xenstore_read_str(base, node);
+    if (val && 1 == sscanf(val, "%"PRIu64, uval)) {
+        rc = 0;
+    }
+    g_free(val);
+    return rc;
+}
+
 int xenstore_write_be_str(struct XenDevice *xendev, const char *node, const char *val)
 {
     return xenstore_write_str(xendev->be, node, val);
@@ -147,6 +160,11 @@ int xenstore_read_fe_int(struct XenDevice *xendev, const char *node, int *ival)
     return xenstore_read_int(xendev->fe, node, ival);
 }
 
+int xenstore_read_fe_uint64(struct XenDevice *xendev, const char *node, uint64_t *uval)
+{
+    return xenstore_read_uint64(xendev->fe, node, uval);
+}
+
 /* ------------------------------------------------------------- */
 
 const char *xenbus_strstate(enum xenbus_state state)
diff --git a/include/hw/xen/xen_backend.h b/include/hw/xen/xen_backend.h
index 3b7d96d..3b4125e 100644
--- a/include/hw/xen/xen_backend.h
+++ b/include/hw/xen/xen_backend.h
@@ -74,6 +74,8 @@ char *xenstore_read_be_str(struct XenDevice *xendev, const char *node);
 int xenstore_read_be_int(struct XenDevice *xendev, const char *node, int *ival);
 char *xenstore_read_fe_str(struct XenDevice *xendev, const char *node);
 int xenstore_read_fe_int(struct XenDevice *xendev, const char *node, int *ival);
+int xenstore_read_uint64(const char *base, const char *node, uint64_t *uval);
+int xenstore_read_fe_uint64(struct XenDevice *xendev, const char *node, uint64_t *uval);
 
 const char *xenbus_strstate(enum xenbus_state state);
 struct XenDevice *xen_be_find_xendev(const char *type, int dom, int dev);
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 1/2] xen_backend: introduce xenstore_read_uint64 and xenstore_read_fe_uint64

[Peter Maydell]

On 17 December 2013 17:30, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  hw/xen/xen_backend.c         |   18 ++++++++++++++++++
>  include/hw/xen/xen_backend.h |    2 ++
>  2 files changed, 20 insertions(+)
>
> diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
> index 197795f..5f9be5a 100644
> --- a/hw/xen/xen_backend.c
> +++ b/hw/xen/xen_backend.c
> @@ -112,6 +112,19 @@ int xenstore_read_int(const char *base, const char *node, int *ival)
>      return rc;
>  }
>
> +int xenstore_read_uint64(const char *base, const char *node, uint64_t *uval)
> +{
> +    char *val;
> +    int rc = -1;
> +
> +    val = xenstore_read_str(base, node);
> +    if (val && 1 == sscanf(val, "%"PRIu64, uval)) {

PRIxxx are printf format macros, not scanf format ones. You want
SCNu64

http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/inttypes.h.html

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] xen_backend: introduce xenstore_read_uint64 and xenstore_read_fe_uint64

[Stefano Stabellini]

On Tue, 17 Dec 2013, Peter Maydell wrote:
> On 17 December 2013 17:30, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > ---
> >  hw/xen/xen_backend.c         |   18 ++++++++++++++++++
> >  include/hw/xen/xen_backend.h |    2 ++
> >  2 files changed, 20 insertions(+)
> >
> > diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
> > index 197795f..5f9be5a 100644
> > --- a/hw/xen/xen_backend.c
> > +++ b/hw/xen/xen_backend.c
> > @@ -112,6 +112,19 @@ int xenstore_read_int(const char *base, const char *node, int *ival)
> >      return rc;
> >  }
> >
> > +int xenstore_read_uint64(const char *base, const char *node, uint64_t *uval)
> > +{
> > +    char *val;
> > +    int rc = -1;
> > +
> > +    val = xenstore_read_str(base, node);
> > +    if (val && 1 == sscanf(val, "%"PRIu64, uval)) {
> 
> PRIxxx are printf format macros, not scanf format ones. You want
> SCNu64
> 
> http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/inttypes.h.html

Well spotted, thanks.

[Qemu-devel] [PATCH 2/2] xen: build on ARM

[Stefano Stabellini]

Collection of fixes to build QEMU with Xen support on ARM:
- use xenstore_read_fe_uint64 to retrieve the page-ref (xenfb);
- use xen_pfn_t instead of unsigned long in xenfb;
- unsigned long/xenpfn_t in xen_remove_from_physmap;
- add __arm__ and __aarch64__ cases in xen-mapcache.c.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/display/xenfb.c |   17 +++++++++--------
 xen-all.c          |    2 +-
 xen-mapcache.c     |    4 ++--
 3 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
index f0333a0..cbc2901 100644
--- a/hw/display/xenfb.c
+++ b/hw/display/xenfb.c
@@ -93,10 +93,11 @@ struct XenFB {
 
 static int common_bind(struct common *c)
 {
-    int mfn;
+    uint64_t mfn;
 
-    if (xenstore_read_fe_int(&c->xendev, "page-ref", &mfn) == -1)
+    if (xenstore_read_fe_uint64(&c->xendev, "page-ref", &mfn) == -1)
 	return -1;
+
     if (xenstore_read_fe_int(&c->xendev, "event-channel", &c->xendev.remote_port) == -1)
 	return -1;
 
@@ -107,7 +108,7 @@ static int common_bind(struct common *c)
 	return -1;
 
     xen_be_bind_evtchn(&c->xendev);
-    xen_be_printf(&c->xendev, 1, "ring mfn %d, remote-port %d, local-port %d\n",
+    xen_be_printf(&c->xendev, 1, "ring mfn %"PRIx64", remote-port %d, local-port %d\n",
 		  mfn, c->xendev.remote_port, c->xendev.local_port);
 
     return 0;
@@ -409,7 +410,7 @@ static void input_event(struct XenDevice *xendev)
 
 /* -------------------------------------------------------------------- */
 
-static void xenfb_copy_mfns(int mode, int count, unsigned long *dst, void *src)
+static void xenfb_copy_mfns(int mode, int count, xen_pfn_t *dst, void *src)
 {
     uint32_t *src32 = src;
     uint64_t *src64 = src;
@@ -424,8 +425,8 @@ static int xenfb_map_fb(struct XenFB *xenfb)
     struct xenfb_page *page = xenfb->c.page;
     char *protocol = xenfb->c.xendev.protocol;
     int n_fbdirs;
-    unsigned long *pgmfns = NULL;
-    unsigned long *fbmfns = NULL;
+    xen_pfn_t *pgmfns = NULL;
+    xen_pfn_t *fbmfns = NULL;
     void *map, *pd;
     int mode, ret = -1;
 
@@ -483,8 +484,8 @@ static int xenfb_map_fb(struct XenFB *xenfb)
     n_fbdirs = xenfb->fbpages * mode / 8;
     n_fbdirs = (n_fbdirs + (XC_PAGE_SIZE - 1)) / XC_PAGE_SIZE;
 
-    pgmfns = g_malloc0(sizeof(unsigned long) * n_fbdirs);
-    fbmfns = g_malloc0(sizeof(unsigned long) * xenfb->fbpages);
+    pgmfns = g_malloc0(sizeof(xen_pfn_t) * n_fbdirs);
+    fbmfns = g_malloc0(sizeof(xen_pfn_t) * xenfb->fbpages);
 
     xenfb_copy_mfns(mode, n_fbdirs, pgmfns, pd);
     map = xc_map_foreign_pages(xen_xc, xenfb->c.xendev.dom,
diff --git a/xen-all.c b/xen-all.c
index 4a594bd..774c80d 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -376,7 +376,7 @@ static int xen_remove_from_physmap(XenIOState *state,
     start_addr >>= TARGET_PAGE_BITS;
     phys_offset >>= TARGET_PAGE_BITS;
     for (i = 0; i < size; i++) {
-        unsigned long idx = start_addr + i;
+        xen_pfn_t idx = start_addr + i;
         xen_pfn_t gpfn = phys_offset + i;
 
         rc = xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn);
diff --git a/xen-mapcache.c b/xen-mapcache.c
index eda914a..79af55c 100644
--- a/xen-mapcache.c
+++ b/xen-mapcache.c
@@ -33,10 +33,10 @@
 #  define DPRINTF(fmt, ...) do { } while (0)
 #endif
 
-#if defined(__i386__)
+#if defined(__i386__) || defined(__arm__)
 #  define MCACHE_BUCKET_SHIFT 16
 #  define MCACHE_MAX_SIZE     (1UL<<31) /* 2GB Cap */
-#elif defined(__x86_64__)
+#elif defined(__x86_64__) || defined(__aarch64__)
 #  define MCACHE_BUCKET_SHIFT 20
 #  define MCACHE_MAX_SIZE     (1UL<<35) /* 32GB Cap */
 #endif
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 2/2] xen: build on ARM

[Peter Maydell]

On 17 December 2013 17:31, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> - add __arm__ and __aarch64__ cases in xen-mapcache.c.
> --- a/xen-mapcache.c
> +++ b/xen-mapcache.c
> @@ -33,10 +33,10 @@
>  #  define DPRINTF(fmt, ...) do { } while (0)
>  #endif
>
> -#if defined(__i386__)
> +#if defined(__i386__) || defined(__arm__)
>  #  define MCACHE_BUCKET_SHIFT 16
>  #  define MCACHE_MAX_SIZE     (1UL<<31) /* 2GB Cap */
> -#elif defined(__x86_64__)
> +#elif defined(__x86_64__) || defined(__aarch64__)
>  #  define MCACHE_BUCKET_SHIFT 20
>  #  define MCACHE_MAX_SIZE     (1UL<<35) /* 32GB Cap */
>  #endif

Per-host-architecture ifdef ladders are kind of nasty. What's this
code actually trying to do ? (looks like maybe "64 bit host addresses
vs 32 bit host addresses" ?)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/2] xen: build on ARM

[Stefano Stabellini]

On Tue, 17 Dec 2013, Peter Maydell wrote:
> On 17 December 2013 17:31, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
> > - add __arm__ and __aarch64__ cases in xen-mapcache.c.
> > --- a/xen-mapcache.c
> > +++ b/xen-mapcache.c
> > @@ -33,10 +33,10 @@
> >  #  define DPRINTF(fmt, ...) do { } while (0)
> >  #endif
> >
> > -#if defined(__i386__)
> > +#if defined(__i386__) || defined(__arm__)
> >  #  define MCACHE_BUCKET_SHIFT 16
> >  #  define MCACHE_MAX_SIZE     (1UL<<31) /* 2GB Cap */
> > -#elif defined(__x86_64__)
> > +#elif defined(__x86_64__) || defined(__aarch64__)
> >  #  define MCACHE_BUCKET_SHIFT 20
> >  #  define MCACHE_MAX_SIZE     (1UL<<35) /* 32GB Cap */
> >  #endif
> 
> Per-host-architecture ifdef ladders are kind of nasty. What's this
> code actually trying to do ? (looks like maybe "64 bit host addresses
> vs 32 bit host addresses" ?)

Almost.
It is trying to decide how much stuff we can map into QEMU's address
space (that is what the "Cap" is referring to). In particular consider
the case where we are running on an ARMv8 system, dom0 and QEMU are
32-bit and the guest is 64-bit.

Re: [Qemu-devel] [PATCH 2/2] xen: build on ARM

[Peter Maydell]

On 17 December 2013 18:20, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Tue, 17 Dec 2013, Peter Maydell wrote:
>> Per-host-architecture ifdef ladders are kind of nasty. What's this
>> code actually trying to do ? (looks like maybe "64 bit host addresses
>> vs 32 bit host addresses" ?)
>
> Almost.
> It is trying to decide how much stuff we can map into QEMU's address
> space (that is what the "Cap" is referring to). In particular consider
> the case where we are running on an ARMv8 system, dom0 and QEMU are
> 32-bit and the guest is 64-bit.

Right, so you essentially want to know the size of a host address
(that's 32 bits in a 32 bit QEMU even if the h/w is ARMv8).

#if HOST_LONG_BITS == 32
[stuff for 32 bit address space]
#else
[stuff for 64 bit address space]
#endif

thanks
-- PMM

Re: [Qemu-devel] [Xen-devel] [PATCH 2/2] xen: build on ARM

[Ian Campbell]

On Tue, 2013-12-17 at 17:31 +0000, Stefano Stabellini wrote:
> Collection of fixes to build QEMU with Xen support on ARM:
> - use xenstore_read_fe_uint64 to retrieve the page-ref (xenfb);
> - use xen_pfn_t instead of unsigned long in xenfb;
> - unsigned long/xenpfn_t in xen_remove_from_physmap;
> - add __arm__ and __aarch64__ cases in xen-mapcache.c.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  hw/display/xenfb.c |   17 +++++++++--------
>  xen-all.c          |    2 +-
>  xen-mapcache.c     |    4 ++--
>  3 files changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> index f0333a0..cbc2901 100644
> --- a/hw/display/xenfb.c
> +++ b/hw/display/xenfb.c
> @@ -93,10 +93,11 @@ struct XenFB {
>  
>  static int common_bind(struct common *c)
>  {
> -    int mfn;
> +    uint64_t mfn;

The commit message says you are switching to use xen_pfn_t in this
file...

Somewhere along the line this will need to be cast to a xen_pfn_t, which
would potentially involve a loss of the upper bits if xen_pfn_t is only
32-bits.

You might want an if (mfn != (xen_pfn_t)mfn) style check somewhere.

Ian.

Re: [Qemu-devel] [Xen-devel] [PATCH 2/2] xen: build on ARM

[Stefano Stabellini]

On Wed, 18 Dec 2013, Ian Campbell wrote:
> On Tue, 2013-12-17 at 17:31 +0000, Stefano Stabellini wrote:
> > Collection of fixes to build QEMU with Xen support on ARM:
> > - use xenstore_read_fe_uint64 to retrieve the page-ref (xenfb);
> > - use xen_pfn_t instead of unsigned long in xenfb;
> > - unsigned long/xenpfn_t in xen_remove_from_physmap;
> > - add __arm__ and __aarch64__ cases in xen-mapcache.c.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > ---
> >  hw/display/xenfb.c |   17 +++++++++--------
> >  xen-all.c          |    2 +-
> >  xen-mapcache.c     |    4 ++--
> >  3 files changed, 12 insertions(+), 11 deletions(-)
> > 
> > diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> > index f0333a0..cbc2901 100644
> > --- a/hw/display/xenfb.c
> > +++ b/hw/display/xenfb.c
> > @@ -93,10 +93,11 @@ struct XenFB {
> >  
> >  static int common_bind(struct common *c)
> >  {
> > -    int mfn;
> > +    uint64_t mfn;
> 
> The commit message says you are switching to use xen_pfn_t in this
> file...
> 
> Somewhere along the line this will need to be cast to a xen_pfn_t, which
> would potentially involve a loss of the upper bits if xen_pfn_t is only
> 32-bits.
> 
> You might want an if (mfn != (xen_pfn_t)mfn) style check somewhere.

Good idea, I'll add an assert.