Mailing List Archive

>>> On 05.10.12 at 12:38, Ian Campbell <ian.campbell@citrix.com> wrote:
> --- a/tools/python/xen/lowlevel/xc/xc.c
> +++ b/tools/python/xen/lowlevel/xc/xc.c
> @@ -1439,7 +1439,7 @@ static PyObject *pyxc_xeninfo(XcObject *self)
> if ( xc_version(self->xc_handle, XENVER_commandline, &xen_commandline) != 0 )
> return pyxc_error_to_exception(self->xc_handle);
>
> - snprintf(str, sizeof(str), "virt_start=0x%lx", p_parms.virt_start);
> + snprintf(str, sizeof(str), "virt_start=0x%"PRI_xen_ulong, p_parms.virt_start);

Does that build on x86? I ask because ...

>
> xen_pagesize = xc_version(self->xc_handle, XENVER_pagesize, NULL);
> if (xen_pagesize < 0 )
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index e3d4ad9..2ae6548 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -122,7 +122,8 @@ typedef uint64_t xen_pfn_t;
> /* Only one. All other VCPUS must use VCPUOP_register_vcpu_info */
> #define XEN_LEGACY_MAX_VCPUS 1
>
> -typedef uint32_t xen_ulong_t;
> +typedef uint64_t xen_ulong_t;
> +#define PRI_xen_ulong PRIx64

... this doesn't seem to have an x86 counterpart.

Jan

>
> struct vcpu_guest_context {
> #define _VGCF_online 0
> diff --git a/xen/include/public/version.h b/xen/include/public/version.h
> index 8742c2b..c7e6f8c 100644
> --- a/xen/include/public/version.h
> +++ b/xen/include/public/version.h
> @@ -28,6 +28,8 @@
> #ifndef __XEN_PUBLIC_VERSION_H__
> #define __XEN_PUBLIC_VERSION_H__
>
> +#include "xen.h"
> +
> /* NB. All ops return zero on success, except XENVER_{version,pagesize} */
>
> /* arg == NULL; returns major:minor (16:16). */
> @@ -58,7 +60,7 @@ typedef char xen_changeset_info_t[64];
>
> #define XENVER_platform_parameters 5
> struct xen_platform_parameters {
> - unsigned long virt_start;
> + xen_ulong_t virt_start;
> };
> typedef struct xen_platform_parameters xen_platform_parameters_t;
>
> --
> 1.7.9.1




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On Fri, 2012-10-05 at 12:00 +0100, Jan Beulich wrote:
> >>> On 05.10.12 at 12:38, Ian Campbell <ian.campbell@citrix.com> wrote:
> > --- a/tools/python/xen/lowlevel/xc/xc.c
> > +++ b/tools/python/xen/lowlevel/xc/xc.c
> > @@ -1439,7 +1439,7 @@ static PyObject *pyxc_xeninfo(XcObject *self)
> > if ( xc_version(self->xc_handle, XENVER_commandline, &xen_commandline) != 0 )
> > return pyxc_error_to_exception(self->xc_handle);
> >
> > - snprintf(str, sizeof(str), "virt_start=0x%lx", p_parms.virt_start);
> > + snprintf(str, sizeof(str), "virt_start=0x%"PRI_xen_ulong, p_parms.virt_start);
>
> Does that build on x86? I ask because ...

I appear to have forgotten to build test the tools side on x86, which
was remiss of me.

> >
> > xen_pagesize = xc_version(self->xc_handle, XENVER_pagesize, NULL);
> > if (xen_pagesize < 0 )
> > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> > index e3d4ad9..2ae6548 100644
> > --- a/xen/include/public/arch-arm.h
> > +++ b/xen/include/public/arch-arm.h
> > @@ -122,7 +122,8 @@ typedef uint64_t xen_pfn_t;
> > /* Only one. All other VCPUS must use VCPUOP_register_vcpu_info */
> > #define XEN_LEGACY_MAX_VCPUS 1
> >
> > -typedef uint32_t xen_ulong_t;
> > +typedef uint64_t xen_ulong_t;
> > +#define PRI_xen_ulong PRIx64
>
> ... this doesn't seem to have an x86 counterpart.

Indeed, I haven't tried but I'm sure it must be broken on x86.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On Fri, 5 Oct 2012, Jan Beulich wrote:
> >>> On 05.10.12 at 12:38, Ian Campbell <ian.campbell@citrix.com> wrote:
> > --- a/tools/python/xen/lowlevel/xc/xc.c
> > +++ b/tools/python/xen/lowlevel/xc/xc.c
> > @@ -1439,7 +1439,7 @@ static PyObject *pyxc_xeninfo(XcObject *self)
> > if ( xc_version(self->xc_handle, XENVER_commandline, &xen_commandline) != 0 )
> > return pyxc_error_to_exception(self->xc_handle);
> >
> > - snprintf(str, sizeof(str), "virt_start=0x%lx", p_parms.virt_start);
> > + snprintf(str, sizeof(str), "virt_start=0x%"PRI_xen_ulong, p_parms.virt_start);
>
> Does that build on x86? I ask because ...
>
> >
> > xen_pagesize = xc_version(self->xc_handle, XENVER_pagesize, NULL);
> > if (xen_pagesize < 0 )
> > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> > index e3d4ad9..2ae6548 100644
> > --- a/xen/include/public/arch-arm.h
> > +++ b/xen/include/public/arch-arm.h
> > @@ -122,7 +122,8 @@ typedef uint64_t xen_pfn_t;
> > /* Only one. All other VCPUS must use VCPUOP_register_vcpu_info */
> > #define XEN_LEGACY_MAX_VCPUS 1
> >
> > -typedef uint32_t xen_ulong_t;
> > +typedef uint64_t xen_ulong_t;
> > +#define PRI_xen_ulong PRIx64
>
> ... this doesn't seem to have an x86 counterpart.
>

xen/include/public/arch-x86/xen.h defines xen_ulong_t but it looks like
that it is missing PRI_xen_ulong

>
> >
> > struct vcpu_guest_context {
> > #define _VGCF_online 0
> > diff --git a/xen/include/public/version.h b/xen/include/public/version.h
> > index 8742c2b..c7e6f8c 100644
> > --- a/xen/include/public/version.h
> > +++ b/xen/include/public/version.h
> > @@ -28,6 +28,8 @@
> > #ifndef __XEN_PUBLIC_VERSION_H__
> > #define __XEN_PUBLIC_VERSION_H__
> >
> > +#include "xen.h"
> > +
> > /* NB. All ops return zero on success, except XENVER_{version,pagesize} */
> >
> > /* arg == NULL; returns major:minor (16:16). */
> > @@ -58,7 +60,7 @@ typedef char xen_changeset_info_t[64];
> >
> > #define XENVER_platform_parameters 5
> > struct xen_platform_parameters {
> > - unsigned long virt_start;
> > + xen_ulong_t virt_start;
> > };
> > typedef struct xen_platform_parameters xen_platform_parameters_t;
> >
> > --
> > 1.7.9.1
>
>
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On Fri, 2012-10-05 at 11:38 +0100, Ian Campbell wrote:
> From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>
> There is still an unwanted unsigned long in the xen public interface:
> replace it with xen_ulong_t.
>
> Also typedef xen_ulong_t to uint64_t on ARM.

Should this change be applied to the uses of XEN_GUEST_HANDLE(ulong)
too? My main concern is the one in struct gnttab_setup_table but there
are a few others.

I suspect XEN_GUEST_HANDLE(ulong) needs to be removed entirely,
everywhere it is used should be XEN_GUEST_HANDLE(xen_ulong_t) instead?

Ian.



>
> Changes in v2:
>
> - do not replace the unsigned long in x86 specific calls;
> - do not replace the unsigned long in multicall_entry;
> - add missing include "xen.h" in version.h;
> - use proper printf flag for xen_ulong_t.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: keir@xen.org
> Cc: JBeulich@suse.com
> ---
> tools/python/xen/lowlevel/xc/xc.c | 2 +-
> xen/include/public/arch-arm.h | 3 ++-
> xen/include/public/version.h | 4 +++-
> 3 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
> index 7c89756..e220f68 100644
> --- a/tools/python/xen/lowlevel/xc/xc.c
> +++ b/tools/python/xen/lowlevel/xc/xc.c
> @@ -1439,7 +1439,7 @@ static PyObject *pyxc_xeninfo(XcObject *self)
> if ( xc_version(self->xc_handle, XENVER_commandline, &xen_commandline) != 0 )
> return pyxc_error_to_exception(self->xc_handle);
>
> - snprintf(str, sizeof(str), "virt_start=0x%lx", p_parms.virt_start);
> + snprintf(str, sizeof(str), "virt_start=0x%"PRI_xen_ulong, p_parms.virt_start);
>
> xen_pagesize = xc_version(self->xc_handle, XENVER_pagesize, NULL);
> if (xen_pagesize < 0 )
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index e3d4ad9..2ae6548 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -122,7 +122,8 @@ typedef uint64_t xen_pfn_t;
> /* Only one. All other VCPUS must use VCPUOP_register_vcpu_info */
> #define XEN_LEGACY_MAX_VCPUS 1
>
> -typedef uint32_t xen_ulong_t;
> +typedef uint64_t xen_ulong_t;
> +#define PRI_xen_ulong PRIx64
>
> struct vcpu_guest_context {
> #define _VGCF_online 0
> diff --git a/xen/include/public/version.h b/xen/include/public/version.h
> index 8742c2b..c7e6f8c 100644
> --- a/xen/include/public/version.h
> +++ b/xen/include/public/version.h
> @@ -28,6 +28,8 @@
> #ifndef __XEN_PUBLIC_VERSION_H__
> #define __XEN_PUBLIC_VERSION_H__
>
> +#include "xen.h"
> +
> /* NB. All ops return zero on success, except XENVER_{version,pagesize} */
>
> /* arg == NULL; returns major:minor (16:16). */
> @@ -58,7 +60,7 @@ typedef char xen_changeset_info_t[64];
>
> #define XENVER_platform_parameters 5
> struct xen_platform_parameters {
> - unsigned long virt_start;
> + xen_ulong_t virt_start;
> };
> typedef struct xen_platform_parameters xen_platform_parameters_t;
>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On Fri, 2012-10-05 at 15:13 +0100, Stefano Stabellini wrote:
> xen/include/public/arch-x86/xen.h defines xen_ulong_t but it looks like
> that it is missing PRI_xen_ulong

I've added:
#define PRI_xen_ulong lx

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On Fri, 5 Oct 2012, Ian Campbell wrote:
> On Fri, 2012-10-05 at 11:38 +0100, Ian Campbell wrote:
> > From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >
> > There is still an unwanted unsigned long in the xen public interface:
> > replace it with xen_ulong_t.
> >
> > Also typedef xen_ulong_t to uint64_t on ARM.
>
> Should this change be applied to the uses of XEN_GUEST_HANDLE(ulong)
> too? My main concern is the one in struct gnttab_setup_table but there
> are a few others.
>
> I suspect XEN_GUEST_HANDLE(ulong) needs to be removed entirely,
> everywhere it is used should be XEN_GUEST_HANDLE(xen_ulong_t) instead?

It is not necessary, because all the XEN_GUEST_HANDLE(ulong) are already
64 bit on ARM. A 32 bit guest is going to pass a 32 bit unsigned long in a
64 bit field, while a 64 bit guest is going to pass a 64 bit unsigned
long in a 64 bit field. Either way it will work.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On Tue, 2012-10-09 at 13:39 +0100, Stefano Stabellini wrote:
> On Fri, 5 Oct 2012, Ian Campbell wrote:
> > On Fri, 2012-10-05 at 11:38 +0100, Ian Campbell wrote:
> > > From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > >
> > > There is still an unwanted unsigned long in the xen public interface:
> > > replace it with xen_ulong_t.
> > >
> > > Also typedef xen_ulong_t to uint64_t on ARM.
> >
> > Should this change be applied to the uses of XEN_GUEST_HANDLE(ulong)
> > too? My main concern is the one in struct gnttab_setup_table but there
> > are a few others.
> >
> > I suspect XEN_GUEST_HANDLE(ulong) needs to be removed entirely,
> > everywhere it is used should be XEN_GUEST_HANDLE(xen_ulong_t) instead?
>
> It is not necessary, because all the XEN_GUEST_HANDLE(ulong) are already
> 64 bit on ARM. A 32 bit guest is going to pass a 32 bit unsigned long in a
> 64 bit field, while a 64 bit guest is going to pass a 64 bit unsigned
> long in a 64 bit field. Either way it will work.

XEN_GUEST_HANDLE(ulong) is unsigned long on all platforms, see
xen/include/public/xen.h:
__DEFINE_XEN_GUEST_HANDLE(ulong, unsigned long);

The existence of this handle is dangerous since it contains a type which
varies in size but it is (slightly) opaque so you might not notice.

The ulong handle is only really usable/desirable on x86 where retaining
the ABI requires us to use unsigned long for some fields, but we have
already defined xen_ulong_t which has the correct semantics on both ARM
and x86.

I propose the following.

8<---------------------------------------------------

From 9090354c816216d6b9cc462e3e8c380e0001c554 Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Fri, 5 Oct 2012 16:32:56 +0000
Subject: [PATCH] xen: remove XEN_GUEST_HANDLE(ulong)

Having both this and XEN_GUEST_HANDLE(xen_ulong_t) is confusing and
error prone.

Replace the two remaining uses of the ulong handle, in grant set and
x86 set_gdt hypercalls, with xen_ulong_t.

This correctly sizes the grant frame entry as 64 bit on ARM but
leaves it as unsigned long on x86 (therefore no intended change on
x86). Likewise in set_gdt there is no actual change.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
xen/arch/x86/mm.c | 3 ++-
xen/common/grant_table.c | 2 +-
xen/include/asm-x86/hypercall.h | 2 +-
xen/include/public/grant_table.h | 2 +-
xen/include/public/xen.h | 2 --
5 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index f9a41fd..3a11af0 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4100,7 +4100,8 @@ long set_gdt(struct vcpu *v,
}


-long do_set_gdt(XEN_GUEST_HANDLE_PARAM(ulong) frame_list, unsigned int entries)
+long do_set_gdt(XEN_GUEST_HANDLE_PARAM(xen_ulong_t) frame_list,
+ unsigned int entries)
{
int nr_pages = (entries + 511) / 512;
unsigned long frames[16];
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index f4ae9ee..7912769 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1322,7 +1322,7 @@ gnttab_setup_table(
struct domain *d;
struct grant_table *gt;
int i;
- unsigned long gmfn;
+ xen_pfn_t gmfn;

if ( count != 1 )
return -EINVAL;
diff --git a/xen/include/asm-x86/hypercall.h b/xen/include/asm-x86/hypercall.h
index bd14220..afa8ba9 100644
--- a/xen/include/asm-x86/hypercall.h
+++ b/xen/include/asm-x86/hypercall.h
@@ -33,7 +33,7 @@ do_mmu_update(

extern long
do_set_gdt(
- XEN_GUEST_HANDLE_PARAM(ulong) frame_list,
+ XEN_GUEST_HANDLE_PARAM(xen_ulong_t) frame_list,
unsigned int entries);

extern long
diff --git a/xen/include/public/grant_table.h b/xen/include/public/grant_table.h
index 28d9476..13cc559 100644
--- a/xen/include/public/grant_table.h
+++ b/xen/include/public/grant_table.h
@@ -385,7 +385,7 @@ struct gnttab_setup_table {
uint32_t nr_frames;
/* OUT parameters. */
int16_t status; /* => enum grant_status */
- XEN_GUEST_HANDLE(ulong) frame_list;
+ XEN_GUEST_HANDLE(xen_pfn_t) frame_list;
};
typedef struct gnttab_setup_table gnttab_setup_table_t;
DEFINE_XEN_GUEST_HANDLE(gnttab_setup_table_t);
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index e42d01f..9a5b394 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -43,8 +43,6 @@ DEFINE_XEN_GUEST_HANDLE(char);
__DEFINE_XEN_GUEST_HANDLE(uchar, unsigned char);
DEFINE_XEN_GUEST_HANDLE(int);
__DEFINE_XEN_GUEST_HANDLE(uint, unsigned int);
-DEFINE_XEN_GUEST_HANDLE(long);
-__DEFINE_XEN_GUEST_HANDLE(ulong, unsigned long);
DEFINE_XEN_GUEST_HANDLE(void);

DEFINE_XEN_GUEST_HANDLE(uint64_t);
--
1.7.9.1




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On Tue, 9 Oct 2012, Ian Campbell wrote:
> On Tue, 2012-10-09 at 13:39 +0100, Stefano Stabellini wrote:
> > On Fri, 5 Oct 2012, Ian Campbell wrote:
> > > On Fri, 2012-10-05 at 11:38 +0100, Ian Campbell wrote:
> > > > From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > >
> > > > There is still an unwanted unsigned long in the xen public interface:
> > > > replace it with xen_ulong_t.
> > > >
> > > > Also typedef xen_ulong_t to uint64_t on ARM.
> > >
> > > Should this change be applied to the uses of XEN_GUEST_HANDLE(ulong)
> > > too? My main concern is the one in struct gnttab_setup_table but there
> > > are a few others.
> > >
> > > I suspect XEN_GUEST_HANDLE(ulong) needs to be removed entirely,
> > > everywhere it is used should be XEN_GUEST_HANDLE(xen_ulong_t) instead?
> >
> > It is not necessary, because all the XEN_GUEST_HANDLE(ulong) are already
> > 64 bit on ARM. A 32 bit guest is going to pass a 32 bit unsigned long in a
> > 64 bit field, while a 64 bit guest is going to pass a 64 bit unsigned
> > long in a 64 bit field. Either way it will work.
>
> XEN_GUEST_HANDLE(ulong) is unsigned long on all platforms, see
> xen/include/public/xen.h:
> __DEFINE_XEN_GUEST_HANDLE(ulong, unsigned long);
>
> The existence of this handle is dangerous since it contains a type which
> varies in size but it is (slightly) opaque so you might not notice.
>
> The ulong handle is only really usable/desirable on x86 where retaining
> the ABI requires us to use unsigned long for some fields, but we have
> already defined xen_ulong_t which has the correct semantics on both ARM
> and x86.
>
> I propose the following.

It is certainly an improvement. Also I didn't notice the
XEN_GUEST_HANDLE_PARAM(ulong): that is actually an error.
We also need a corresponding patch for Linux.


> 8<---------------------------------------------------
>
> From 9090354c816216d6b9cc462e3e8c380e0001c554 Mon Sep 17 00:00:00 2001
> From: Ian Campbell <ian.campbell@citrix.com>
> Date: Fri, 5 Oct 2012 16:32:56 +0000
> Subject: [PATCH] xen: remove XEN_GUEST_HANDLE(ulong)
>
> Having both this and XEN_GUEST_HANDLE(xen_ulong_t) is confusing and
> error prone.
>
> Replace the two remaining uses of the ulong handle, in grant set and
> x86 set_gdt hypercalls, with xen_ulong_t.
>
> This correctly sizes the grant frame entry as 64 bit on ARM but
> leaves it as unsigned long on x86 (therefore no intended change on
> x86). Likewise in set_gdt there is no actual change.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> xen/arch/x86/mm.c | 3 ++-
> xen/common/grant_table.c | 2 +-
> xen/include/asm-x86/hypercall.h | 2 +-
> xen/include/public/grant_table.h | 2 +-
> xen/include/public/xen.h | 2 --
> 5 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index f9a41fd..3a11af0 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4100,7 +4100,8 @@ long set_gdt(struct vcpu *v,
> }
>
>
> -long do_set_gdt(XEN_GUEST_HANDLE_PARAM(ulong) frame_list, unsigned int entries)
> +long do_set_gdt(XEN_GUEST_HANDLE_PARAM(xen_ulong_t) frame_list,
> + unsigned int entries)
> {
> int nr_pages = (entries + 511) / 512;
> unsigned long frames[16];
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index f4ae9ee..7912769 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1322,7 +1322,7 @@ gnttab_setup_table(
> struct domain *d;
> struct grant_table *gt;
> int i;
> - unsigned long gmfn;
> + xen_pfn_t gmfn;
>
> if ( count != 1 )
> return -EINVAL;
> diff --git a/xen/include/asm-x86/hypercall.h b/xen/include/asm-x86/hypercall.h
> index bd14220..afa8ba9 100644
> --- a/xen/include/asm-x86/hypercall.h
> +++ b/xen/include/asm-x86/hypercall.h
> @@ -33,7 +33,7 @@ do_mmu_update(
>
> extern long
> do_set_gdt(
> - XEN_GUEST_HANDLE_PARAM(ulong) frame_list,
> + XEN_GUEST_HANDLE_PARAM(xen_ulong_t) frame_list,
> unsigned int entries);
>
> extern long
> diff --git a/xen/include/public/grant_table.h b/xen/include/public/grant_table.h
> index 28d9476..13cc559 100644
> --- a/xen/include/public/grant_table.h
> +++ b/xen/include/public/grant_table.h
> @@ -385,7 +385,7 @@ struct gnttab_setup_table {
> uint32_t nr_frames;
> /* OUT parameters. */
> int16_t status; /* => enum grant_status */
> - XEN_GUEST_HANDLE(ulong) frame_list;
> + XEN_GUEST_HANDLE(xen_pfn_t) frame_list;
> };
> typedef struct gnttab_setup_table gnttab_setup_table_t;
> DEFINE_XEN_GUEST_HANDLE(gnttab_setup_table_t);
> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> index e42d01f..9a5b394 100644
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -43,8 +43,6 @@ DEFINE_XEN_GUEST_HANDLE(char);
> __DEFINE_XEN_GUEST_HANDLE(uchar, unsigned char);
> DEFINE_XEN_GUEST_HANDLE(int);
> __DEFINE_XEN_GUEST_HANDLE(uint, unsigned int);
> -DEFINE_XEN_GUEST_HANDLE(long);
> -__DEFINE_XEN_GUEST_HANDLE(ulong, unsigned long);
> DEFINE_XEN_GUEST_HANDLE(void);
>
> DEFINE_XEN_GUEST_HANDLE(uint64_t);
> --
> 1.7.9.1
>
>
>
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On Tue, 2012-10-09 at 13:51 +0100, Stefano Stabellini wrote:
> On Tue, 9 Oct 2012, Ian Campbell wrote:
> > On Tue, 2012-10-09 at 13:39 +0100, Stefano Stabellini wrote:
> > > On Fri, 5 Oct 2012, Ian Campbell wrote:
> > > > On Fri, 2012-10-05 at 11:38 +0100, Ian Campbell wrote:
> > > > > From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > > >
> > > > > There is still an unwanted unsigned long in the xen public interface:
> > > > > replace it with xen_ulong_t.
> > > > >
> > > > > Also typedef xen_ulong_t to uint64_t on ARM.
> > > >
> > > > Should this change be applied to the uses of XEN_GUEST_HANDLE(ulong)
> > > > too? My main concern is the one in struct gnttab_setup_table but there
> > > > are a few others.
> > > >
> > > > I suspect XEN_GUEST_HANDLE(ulong) needs to be removed entirely,
> > > > everywhere it is used should be XEN_GUEST_HANDLE(xen_ulong_t) instead?
> > >
> > > It is not necessary, because all the XEN_GUEST_HANDLE(ulong) are already
> > > 64 bit on ARM. A 32 bit guest is going to pass a 32 bit unsigned long in a
> > > 64 bit field, while a 64 bit guest is going to pass a 64 bit unsigned
> > > long in a 64 bit field. Either way it will work.
> >
> > XEN_GUEST_HANDLE(ulong) is unsigned long on all platforms, see
> > xen/include/public/xen.h:
> > __DEFINE_XEN_GUEST_HANDLE(ulong, unsigned long);
> >
> > The existence of this handle is dangerous since it contains a type which
> > varies in size but it is (slightly) opaque so you might not notice.
> >
> > The ulong handle is only really usable/desirable on x86 where retaining
> > the ABI requires us to use unsigned long for some fields, but we have
> > already defined xen_ulong_t which has the correct semantics on both ARM
> > and x86.
> >
> > I propose the following.
>
> It is certainly an improvement. Also I didn't notice the
> XEN_GUEST_HANDLE_PARAM(ulong): that is actually an error.
> We also need a corresponding patch for Linux.

I need to tesdt both this and the h/v side a bit more but here it is.

8<---------------------------

From c55591bbe3b1d5164641075b95f3c95418bcdf79 Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Fri, 5 Oct 2012 17:39:19 +0100
Subject: [PATCH] xen: grant: use xen_pfn_t type for frame_list.

This correctly sizes it as 64 bit on ARM but leaves it as unsigned
long on x86 (therefore no intended change on x86).

The long and ulong guest handles are now unused (and a bit dangerous)
so remove them.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
arch/arm/include/asm/xen/interface.h | 2 --
arch/x86/include/asm/xen/interface.h | 2 --
include/xen/interface/grant_table.h | 2 +-
3 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/arch/arm/include/asm/xen/interface.h b/arch/arm/include/asm/xen/interface.h
index ad87917..9ac9f4e 100644
--- a/arch/arm/include/asm/xen/interface.h
+++ b/arch/arm/include/asm/xen/interface.h
@@ -35,10 +35,8 @@ typedef uint64_t xen_ulong_t;
/* Guest handles for primitive C types. */
__DEFINE_GUEST_HANDLE(uchar, unsigned char);
__DEFINE_GUEST_HANDLE(uint, unsigned int);
-__DEFINE_GUEST_HANDLE(ulong, unsigned long);
DEFINE_GUEST_HANDLE(char);
DEFINE_GUEST_HANDLE(int);
-DEFINE_GUEST_HANDLE(long);
DEFINE_GUEST_HANDLE(void);
DEFINE_GUEST_HANDLE(uint64_t);
DEFINE_GUEST_HANDLE(uint32_t);
diff --git a/arch/x86/include/asm/xen/interface.h b/arch/x86/include/asm/xen/interface.h
index d67f3c6..ed602f8 100644
--- a/arch/x86/include/asm/xen/interface.h
+++ b/arch/x86/include/asm/xen/interface.h
@@ -55,10 +55,8 @@ typedef unsigned long xen_ulong_t;
/* Guest handles for primitive C types. */
__DEFINE_GUEST_HANDLE(uchar, unsigned char);
__DEFINE_GUEST_HANDLE(uint, unsigned int);
-__DEFINE_GUEST_HANDLE(ulong, unsigned long);
DEFINE_GUEST_HANDLE(char);
DEFINE_GUEST_HANDLE(int);
-DEFINE_GUEST_HANDLE(long);
DEFINE_GUEST_HANDLE(void);
DEFINE_GUEST_HANDLE(uint64_t);
DEFINE_GUEST_HANDLE(uint32_t);
diff --git a/include/xen/interface/grant_table.h b/include/xen/interface/grant_table.h
index f9f8b97..e40fae9 100644
--- a/include/xen/interface/grant_table.h
+++ b/include/xen/interface/grant_table.h
@@ -310,7 +310,7 @@ struct gnttab_setup_table {
uint32_t nr_frames;
/* OUT parameters. */
int16_t status; /* GNTST_* */
- GUEST_HANDLE(ulong) frame_list;
+ GUEST_HANDLE(xen_pfn_t) frame_list;
};
DEFINE_GUEST_HANDLE_STRUCT(gnttab_setup_table);

--
1.7.2.5




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel