| Message ID | 1447349448-22610-2-git-send-email-stefano.stabellini@eu.citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Stefano, On 12/11/15 17:30, Stefano Stabellini wrote: > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > --- > > Changes in v2: > - rename dom0_op to platform_op > --- > arch/arm/include/asm/xen/hypercall.h | 2 ++ > arch/arm/xen/enlighten.c | 1 + > arch/arm/xen/hypercall.S | 1 + > arch/arm64/xen/hypercall.S | 1 + > 4 files changed, 5 insertions(+) > > diff --git a/arch/arm/include/asm/xen/hypercall.h b/arch/arm/include/asm/xen/hypercall.h > index 712b50e..c3e00d0 100644 > --- a/arch/arm/include/asm/xen/hypercall.h > +++ b/arch/arm/include/asm/xen/hypercall.h > @@ -35,6 +35,7 @@ > > #include <xen/interface/xen.h> > #include <xen/interface/sched.h> > +#include <xen/interface/platform.h> > > long privcmd_call(unsigned call, unsigned long a1, > unsigned long a2, unsigned long a3, > @@ -49,6 +50,7 @@ int HYPERVISOR_memory_op(unsigned int cmd, void *arg); > int HYPERVISOR_physdev_op(int cmd, void *arg); > int HYPERVISOR_vcpu_op(int cmd, int vcpuid, void *extra_args); > int HYPERVISOR_tmem_op(void *arg); > +int HYPERVISOR_platform_op(void *arg); int HYPERVISOR_platform_op(struct xen_platform_op *platform_op) to allow compiler type checking and match the x86 version. Also, the implementation of the helper differ from x86. On x86, the helper takes care of setting the interface_version while here you request the caller to do it. It's better if we have similar requirement across the architecture as this helpers may be called from common code. > int HYPERVISOR_multicall(struct multicall_entry *calls, uint32_t nr); > > static inline int Regards,
On Fri, 13 Nov 2015, Julien Grall wrote: > Hi Stefano, > > On 12/11/15 17:30, Stefano Stabellini wrote: > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > > > --- > > > > Changes in v2: > > - rename dom0_op to platform_op > > --- > > arch/arm/include/asm/xen/hypercall.h | 2 ++ > > arch/arm/xen/enlighten.c | 1 + > > arch/arm/xen/hypercall.S | 1 + > > arch/arm64/xen/hypercall.S | 1 + > > 4 files changed, 5 insertions(+) > > > > diff --git a/arch/arm/include/asm/xen/hypercall.h b/arch/arm/include/asm/xen/hypercall.h > > index 712b50e..c3e00d0 100644 > > --- a/arch/arm/include/asm/xen/hypercall.h > > +++ b/arch/arm/include/asm/xen/hypercall.h > > @@ -35,6 +35,7 @@ > > > > #include <xen/interface/xen.h> > > #include <xen/interface/sched.h> > > +#include <xen/interface/platform.h> > > > > long privcmd_call(unsigned call, unsigned long a1, > > unsigned long a2, unsigned long a3, > > @@ -49,6 +50,7 @@ int HYPERVISOR_memory_op(unsigned int cmd, void *arg); > > int HYPERVISOR_physdev_op(int cmd, void *arg); > > int HYPERVISOR_vcpu_op(int cmd, int vcpuid, void *extra_args); > > int HYPERVISOR_tmem_op(void *arg); > > +int HYPERVISOR_platform_op(void *arg); > > int HYPERVISOR_platform_op(struct xen_platform_op *platform_op) to allow > compiler type checking and match the x86 version. Yeah, I am just following the same pattern as the others > Also, the implementation of the helper differ from x86. On x86, the > helper takes care of setting the interface_version while here you > request the caller to do it. > > It's better if we have similar requirement across the architecture as > this helpers may be called from common code. I agree with your point (I thought about it myself) but the current assembly scheme for hypercalls doesn't work well with that. I would have to introduce, and maintain going forward, two special hypercall implementations in assembly, one for arm and another for arm64, just to set interface_version. I don't think it is worth it; I prefer to have to maintain the explicit interface_version setting at the call sites (that today is just one).
(CC David, Boris and Konrad) On 13/11/15 18:10, Stefano Stabellini wrote: > On Fri, 13 Nov 2015, Julien Grall wrote: >> Hi Stefano, >> >> On 12/11/15 17:30, Stefano Stabellini wrote: >>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> >>> >>> --- >>> >>> Changes in v2: >>> - rename dom0_op to platform_op >>> --- >>> arch/arm/include/asm/xen/hypercall.h | 2 ++ >>> arch/arm/xen/enlighten.c | 1 + >>> arch/arm/xen/hypercall.S | 1 + >>> arch/arm64/xen/hypercall.S | 1 + >>> 4 files changed, 5 insertions(+) >>> >>> diff --git a/arch/arm/include/asm/xen/hypercall.h b/arch/arm/include/asm/xen/hypercall.h >>> index 712b50e..c3e00d0 100644 >>> --- a/arch/arm/include/asm/xen/hypercall.h >>> +++ b/arch/arm/include/asm/xen/hypercall.h >>> @@ -35,6 +35,7 @@ >>> >>> #include <xen/interface/xen.h> >>> #include <xen/interface/sched.h> >>> +#include <xen/interface/platform.h> >>> >>> long privcmd_call(unsigned call, unsigned long a1, >>> unsigned long a2, unsigned long a3, >>> @@ -49,6 +50,7 @@ int HYPERVISOR_memory_op(unsigned int cmd, void *arg); >>> int HYPERVISOR_physdev_op(int cmd, void *arg); >>> int HYPERVISOR_vcpu_op(int cmd, int vcpuid, void *extra_args); >>> int HYPERVISOR_tmem_op(void *arg); >>> +int HYPERVISOR_platform_op(void *arg); >> >> int HYPERVISOR_platform_op(struct xen_platform_op *platform_op) to allow >> compiler type checking and match the x86 version. > > Yeah, I am just following the same pattern as the others It's not necessary to repeat a mistake just because the other does. If it's possible to use struct xen_platform_op, then please do it. Having extra safety from the compiler could be useful to spot stupid mistake. > >> Also, the implementation of the helper differ from x86. On x86, the >> helper takes care of setting the interface_version while here you >> request the caller to do it. >> >> It's better if we have similar requirement across the architecture as >> this helpers may be called from common code. > > I agree with your point (I thought about it myself) but the current > assembly scheme for hypercalls doesn't work well with that. I would have > to introduce, and maintain going forward, two special hypercall > implementations in assembly, one for arm and another for arm64, just to > set interface_version. I don't think it is worth it; I prefer to have to > maintain the explicit interface_version setting at the call sites (that > today is just one). Sooner of later we will support EFI runtime and co. There are ~10 references of this platform op hypercall in the common code ([1]), and I wouldn't be surprised if it continues to grow. So every single time someone is using the hypercall in the code we would have to remember that the x86 and ARM implementation is different. Are you ready to review every future patches to check that this will work as expected? IHMO, this will be a pain compare to maintaining a different set of hypercall. I really think we need to have the same behavior for all the hypercalls helpers accross all the architectures. This is really helpful when implementation arch-agnostic driver. Regards, [1] http://lxr.free-electrons.com/ident?i=HYPERVISOR_dom0_op
On Fri, 2015-11-13 at 18:10 +0000, Stefano Stabellini wrote: > > I agree with your point (I thought about it myself) but the current > assembly scheme for hypercalls doesn't work well with that. I would have > to introduce, and maintain going forward, two special hypercall > implementations in assembly, one for arm and another for arm64, just to > set interface_version. I don't think it is worth it; I prefer to have to > maintain the explicit interface_version setting at the call sites (that > today is just one). You could give the bare assembly stub a different name (append _core or _raw or something) and make HYPERVISOR_platform_op a C wrapper for it which DTRT. Ian.
diff --git a/arch/arm/include/asm/xen/hypercall.h b/arch/arm/include/asm/xen/hypercall.h index 712b50e..c3e00d0 100644 --- a/arch/arm/include/asm/xen/hypercall.h +++ b/arch/arm/include/asm/xen/hypercall.h @@ -35,6 +35,7 @@ #include <xen/interface/xen.h> #include <xen/interface/sched.h> +#include <xen/interface/platform.h> long privcmd_call(unsigned call, unsigned long a1, unsigned long a2, unsigned long a3, @@ -49,6 +50,7 @@ int HYPERVISOR_memory_op(unsigned int cmd, void *arg); int HYPERVISOR_physdev_op(int cmd, void *arg); int HYPERVISOR_vcpu_op(int cmd, int vcpuid, void *extra_args); int HYPERVISOR_tmem_op(void *arg); +int HYPERVISOR_platform_op(void *arg); int HYPERVISOR_multicall(struct multicall_entry *calls, uint32_t nr); static inline int diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c index 15621b1..2f57ba3 100644 --- a/arch/arm/xen/enlighten.c +++ b/arch/arm/xen/enlighten.c @@ -328,5 +328,6 @@ EXPORT_SYMBOL_GPL(HYPERVISOR_memory_op); EXPORT_SYMBOL_GPL(HYPERVISOR_physdev_op); EXPORT_SYMBOL_GPL(HYPERVISOR_vcpu_op); EXPORT_SYMBOL_GPL(HYPERVISOR_tmem_op); +EXPORT_SYMBOL_GPL(HYPERVISOR_platform_op); EXPORT_SYMBOL_GPL(HYPERVISOR_multicall); EXPORT_SYMBOL_GPL(privcmd_call); diff --git a/arch/arm/xen/hypercall.S b/arch/arm/xen/hypercall.S index 10fd99c..d4539f4 100644 --- a/arch/arm/xen/hypercall.S +++ b/arch/arm/xen/hypercall.S @@ -89,6 +89,7 @@ HYPERCALL2(memory_op); HYPERCALL2(physdev_op); HYPERCALL3(vcpu_op); HYPERCALL1(tmem_op); +HYPERCALL1(platform_op); HYPERCALL2(multicall); ENTRY(privcmd_call) diff --git a/arch/arm64/xen/hypercall.S b/arch/arm64/xen/hypercall.S index 8bbe940..f7d5724 100644 --- a/arch/arm64/xen/hypercall.S +++ b/arch/arm64/xen/hypercall.S @@ -80,6 +80,7 @@ HYPERCALL2(memory_op); HYPERCALL2(physdev_op); HYPERCALL3(vcpu_op); HYPERCALL1(tmem_op); +HYPERCALL1(platform_op); HYPERCALL2(multicall); ENTRY(privcmd_call)
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> --- Changes in v2: - rename dom0_op to platform_op --- arch/arm/include/asm/xen/hypercall.h | 2 ++ arch/arm/xen/enlighten.c | 1 + arch/arm/xen/hypercall.S | 1 + arch/arm64/xen/hypercall.S | 1 + 4 files changed, 5 insertions(+)