Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1268131 > unrolled thread

[PATCH v4 2/7] xen/arm: introduce HYPERVISOR_platform_op on arm and arm64

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v4 2/7] xen/arm: introduce HYPERVISOR_platform_op on arm and arm64 Stefano Stabellini <stefano.stabellini@eu.citrix.com> - 2015-11-12 18:40 +0100
    Re: [Xen-devel] [PATCH v4 2/7] xen/arm: introduce HYPERVISOR_platform_op  on arm and arm64 Julien Grall <julien.grall@citrix.com> - 2015-11-13 14:50 +0100
      Re: [Xen-devel] [PATCH v4 2/7] xen/arm: introduce HYPERVISOR_platform_op  on arm and arm64 Stefano Stabellini <stefano.stabellini@eu.citrix.com> - 2015-11-13 19:20 +0100
        Re: [Xen-devel] [PATCH v4 2/7] xen/arm: introduce HYPERVISOR_platform_op  on arm and arm64 Julien Grall <julien.grall@citrix.com> - 2015-11-13 19:40 +0100
        Re: [Xen-devel] [PATCH v4 2/7] xen/arm: introduce  HYPERVISOR_platform_op on arm and arm64 Ian Campbell <ian.campbell@citrix.com> - 2015-11-16 10:50 +0100
          Re: [Xen-devel] [PATCH v4 2/7] xen/arm: introduce HYPERVISOR_platform_op  on arm and arm64 Stefano Stabellini <stefano.stabellini@eu.citrix.com> - 2015-11-20 13:00 +0100
            Re: [Xen-devel] [PATCH v4 2/7] xen/arm: introduce  HYPERVISOR_platform_op on arm and arm64 Ian Campbell <ian.campbell@citrix.com> - 2015-11-20 13:20 +0100

#1268131 — [PATCH v4 2/7] xen/arm: introduce HYPERVISOR_platform_op on arm and arm64

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_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)
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1268947 — Re: [Xen-devel] [PATCH v4 2/7] xen/arm: introduce HYPERVISOR_platform_op on arm and arm64

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,

-- 
Julien Grall
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1269129 — Re: [Xen-devel] [PATCH v4 2/7] xen/arm: introduce HYPERVISOR_platform_op on arm and arm64

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).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1269137 — Re: [Xen-devel] [PATCH v4 2/7] xen/arm: introduce HYPERVISOR_platform_op on arm and arm64

(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

-- 
Julien Grall
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1269954 — Re: [Xen-devel] [PATCH v4 2/7] xen/arm: introduce HYPERVISOR_platform_op on arm and arm64

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1274004 — Re: [Xen-devel] [PATCH v4 2/7] xen/arm: introduce HYPERVISOR_platform_op on arm and arm64

On Mon, 16 Nov 2015, Ian Campbell wrote:
> 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.

I had an idea. I just need to 

#define HYPERVISOR_platform_op_raw HYPERVISOR_platform_op

then the small wrapper can work:


diff --git a/arch/arm/include/asm/xen/hypercall.h b/arch/arm/include/asm/xen/hypercall.h
index c3e00d0..d769972 100644
--- a/arch/arm/include/asm/xen/hypercall.h
+++ b/arch/arm/include/asm/xen/hypercall.h
@@ -50,7 +50,12 @@ 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_raw(void *arg);
+static inline int HYPERVISOR_platform_op(struct xen_platform_op *op)
+{
+	op->interface_version = XENPF_INTERFACE_VERSION;
+	return HYPERVISOR_platform_op_raw(op);
+}
 int HYPERVISOR_multicall(struct multicall_entry *calls, uint32_t nr);
 
 static inline int
diff --git a/arch/arm/include/asm/xen/interface.h b/arch/arm/include/asm/xen/interface.h
index 5b150d7..75d5968 100644
--- a/arch/arm/include/asm/xen/interface.h
+++ b/arch/arm/include/asm/xen/interface.h
@@ -27,6 +27,8 @@
 		(hnd).p = val;				\
 	} while (0)
 
+#define __HYPERVISOR_platform_op_raw __HYPERVISOR_platform_op
+
 #ifndef __ASSEMBLY__
 /* Explicitly size integers that represent pfns in the interface with
  * Xen so that we can have one ABI that works for 32 and 64 bit guests.
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index f964238..0f7be84 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -141,7 +141,6 @@ static int xen_pvclock_gtod_notify(struct notifier_block *nb,
 	if (!was_set && timespec64_compare(&now, &next_sync) < 0)
 		return NOTIFY_OK;
 
-	op.interface_version = XENPF_INTERFACE_VERSION;
 	op.cmd = XENPF_settime64;
 	op.u.settime64.mbz = 0;
 	op.u.settime64.secs = now.tv_sec;
diff --git a/arch/arm/xen/hypercall.S b/arch/arm/xen/hypercall.S
index d4539f4..9a36f4f 100644
--- a/arch/arm/xen/hypercall.S
+++ b/arch/arm/xen/hypercall.S
@@ -89,7 +89,7 @@ HYPERCALL2(memory_op);
 HYPERCALL2(physdev_op);
 HYPERCALL3(vcpu_op);
 HYPERCALL1(tmem_op);
-HYPERCALL1(platform_op);
+HYPERCALL1(platform_op_raw);
 HYPERCALL2(multicall);
 
 ENTRY(privcmd_call)
diff --git a/arch/arm64/xen/hypercall.S b/arch/arm64/xen/hypercall.S
index f7d5724..70df80e 100644
--- a/arch/arm64/xen/hypercall.S
+++ b/arch/arm64/xen/hypercall.S
@@ -80,7 +80,7 @@ HYPERCALL2(memory_op);
 HYPERCALL2(physdev_op);
 HYPERCALL3(vcpu_op);
 HYPERCALL1(tmem_op);
-HYPERCALL1(platform_op);
+HYPERCALL1(platform_op_raw);
 HYPERCALL2(multicall);
 
 ENTRY(privcmd_call)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1274019 — Re: [Xen-devel] [PATCH v4 2/7] xen/arm: introduce HYPERVISOR_platform_op on arm and arm64

On Fri, 2015-11-20 at 11:58 +0000, Stefano Stabellini wrote:
> On Mon, 16 Nov 2015, Ian Campbell wrote:
> > 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.
> 
> I had an idea. I just need to 
> 
> #define HYPERVISOR_platform_op_raw HYPERVISOR_platform_op

The need for this #define is a bit unfortunate, but the alternatives (e.g.
a suffix argument to the HYPERCALL*() macros or a RAWHYPERCALL variant)
would seem to suck more, so I say go for it.

Ian.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web