Re: [Xen-devel] [PATCH v7 02/15] argo: introduce the argo_op hypercall boilerplate
Hi Stefano, On 2/6/19 7:35 PM, Stefano Stabellini wrote: On Wed, 6 Feb 2019, Julien Grall wrote: However, I think we should add some sanity check in arch_set_info_guest for our peace of mind. For guest entry/exit, rather than zero the upper 32-bits I would also add sanity check in enter_hypervisor_head and leave_hypervisor_tail but only in debug build. Any opinions? Definitely we should have sanity checks. I will add this one and ... In the official public headers, I can't find anything telling you the size of each arguments and the number of arguments. Instead you have to look at Xen code to know the exact number of arguments and the size. Did I miss anything? No, it is like you wrote. I think I should have pushed the discussion further and added more information to the Xen public headers back in those days. ... and this one in my list of things to improve. In conclusion, if you and other maintainers prefer unsigned long I'll drop my reservation. The summary of my e-mail is: - we need to add sanity check (or zero) upper-bits for 32-bit guest - unsigned long should be fine for 64-bit only features I take that you mean that unsigned long should be fine, and would also allow us to introduce 64-bit only features in the future, right? that's correct. You are *not* saying that unsigned long should only be used with 64-bit guests/hypervisor? unsigned long == uint32_t on 32-bit. So it would be silly of me to suggest that :). - we need to document the behavior of each hypercall and provide guidelines for new one. None of this is specific to Argo and I would be happy to defer this as a follow-up series. Assuming my understanding is right, I agree with you. You understood right. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v7 02/15] argo: introduce the argo_op hypercall boilerplate
On Wed, 6 Feb 2019, Julien Grall wrote: > Hi Stefano, > > On 2/5/19 9:34 PM, Stefano Stabellini wrote: > > On Tue, 5 Feb 2019, Julien Grall wrote: > > > Sorry for the formatting. > > > > > > On Tue, 5 Feb 2019, 20:04 Stefano Stabellini, > > > wrote: > > >On Tue, 5 Feb 2019, Jan Beulich wrote: > > > > But I am afraid this is not correct. Upper 32-bit of the register will be > > > zeroed when writing a 32-bit value. So we never rely on > > > the register to be zeroed on boot. > > > > Thank you for checking your emails. I found the reference in the ARM > > ARM, although it took me several minutes! > > > >"The upper 32 bits of the destination register are set to zero." > > > > from C6.1.1 (ID092916). > > Actually, you were right and I was wrong. This paragraph only talks about > 32-bit access from AArch64. I found a paragraph on the Arm Arm (D1.20.2 in DDI > 0487D.a) stating that the upper 32-bits can either "be zeroed, or hold the > value that the same architectural register held before any AArch32". Understanding the ARM ARM is really difficult, I am glad we clarified this (and that my memory didn't completely fail me yet). > So we do rely the upper bits are zeroed when the vCPU is created. The > registers are set by arch_set_info_guest via a context. The context can be set > by either virtual PSCI call CPU_ON or via DOMCTL setvcpucontext (so the > tools). > > We fully control PSCI call CPU_ON and zero the registers. So no question here. > > For the DOMCTL, per XSA-77, we trust how operation will be used. The toolstack > (vcpu_arm32 libxc/xc_dom_arm.c) will zero the context before. So I think we > should be safe here. > > However, I think we should add some sanity check in arch_set_info_guest for > our peace of mind. For guest entry/exit, rather than zero the upper 32-bits I > would also add sanity check in enter_hypervisor_head and leave_hypervisor_tail > but only in debug build. Any opinions? Definitely we should have sanity checks. > > >FYI do_memory_op is declared as follows on the Linux side for arm32 > > > and > > >arm64: > > > > > > int HYPERVISOR_memory_op(unsigned int cmd, void *arg); > > > > > >When I went through all existing hypercalls to introduce them on > > > arm32, > > >I checked that we didn't actually need 64bit parameters, especially > > > for > > >cmd. I introduced them as int instead of long on the Linux side > > > when > > >possible (see include/xen/arm/hypercall.h), but I didn't attempt to > > >modify all the existing Xen headers. > > > > > > > > > I don't understand your concern with unsigned long. We use them in > > > __DEFINE_XEN_GUEST_HANDLE that are in turn to describe guest > > > pointer. > > > > __DEFINE_XEN_GUEST_HANDLE is for pointers inside memory structs, and we > > defined it as: > > * XEN_GUEST_HANDLE represents a guest pointer, when passed as a field > > * in a struct in memory. On ARM is always 8 bytes sizes and 8 bytes > > * aligned. > > > > You probably meant XEN_GUEST_HANDLE_PARAM which is the one for pointers > > when passed as hypercall parameters, that is defined as: > > * XEN_GUEST_HANDLE_PARAM represents a guest pointer, when passed as an > > * hypercall argument. It is 4 bytes on aarch32 and 8 bytes on aarch64. > > Yes, I meant XEN_GUEST_HANDLE_PARAM. I should have waited to have my laptop in > hand rather looking on my phone :). > > > > > Yes, pointers as hypercalls parameters are the exception to the > > single-ABI rule and we introduced XEN_GUEST_HANDLE_PARAM purposely to > > handle them. However, I am not sure we took into account zero-extension > > when we discussed hypercalls parameters for arm back in the day when I > > wrote include/xen/arm/hypercall.h. > > I am not sure to follow your thoughts about taking into account zero-extension > in the Linux header. Could you expand it? > > In the official public headers, I can't find anything telling you the size of > each arguments and the number of arguments. Instead you have to look at Xen > code to know the exact number of arguments and the size. Did I miss anything? No, it is like you wrote. I think I should have pushed the discussion further and added more information to the Xen public headers back in those days. > Regarding Argo, there seem to have some kind of documentation per-hypercalls > although some does not specify the size. But I can't find anything telling you > the command number is in arg0. The mapping to from argN the hypercalls > arguments is also not that clear. > > But I guess this kind of improvement can be done afterwards. > > > > The problem with explicitly sized (i.e 32-bit) is you ignore the top > > > 32-bit. This means you can't check the upper bits are always > > > 0 and would prevent extension. > > > > That is true. I implicitly assumed that our desire for a common > > 32-bit/64-bit ABI would not apply just to structs in memory (where we > > always define
Re: [Xen-devel] [PATCH v7 02/15] argo: introduce the argo_op hypercall boilerplate
Hi Stefano, On 2/5/19 9:34 PM, Stefano Stabellini wrote: On Tue, 5 Feb 2019, Julien Grall wrote: Sorry for the formatting. On Tue, 5 Feb 2019, 20:04 Stefano Stabellini, wrote: On Tue, 5 Feb 2019, Jan Beulich wrote: But I am afraid this is not correct. Upper 32-bit of the register will be zeroed when writing a 32-bit value. So we never rely on the register to be zeroed on boot. Hi Julien, Hi Stefano, Thank you for checking your emails. I found the reference in the ARM ARM, although it took me several minutes! "The upper 32 bits of the destination register are set to zero." from C6.1.1 (ID092916). Actually, you were right and I was wrong. This paragraph only talks about 32-bit access from AArch64. I found a paragraph on the Arm Arm (D1.20.2 in DDI 0487D.a) stating that the upper 32-bits can either "be zeroed, or hold the value that the same architectural register held before any AArch32". So we do rely the upper bits are zeroed when the vCPU is created. The registers are set by arch_set_info_guest via a context. The context can be set by either virtual PSCI call CPU_ON or via DOMCTL setvcpucontext (so the tools). We fully control PSCI call CPU_ON and zero the registers. So no question here. For the DOMCTL, per XSA-77, we trust how operation will be used. The toolstack (vcpu_arm32 libxc/xc_dom_arm.c) will zero the context before. So I think we should be safe here. However, I think we should add some sanity check in arch_set_info_guest for our peace of mind. For guest entry/exit, rather than zero the upper 32-bits I would also add sanity check in enter_hypervisor_head and leave_hypervisor_tail but only in debug build. Any opinions? FYI do_memory_op is declared as follows on the Linux side for arm32 and arm64: int HYPERVISOR_memory_op(unsigned int cmd, void *arg); When I went through all existing hypercalls to introduce them on arm32, I checked that we didn't actually need 64bit parameters, especially for cmd. I introduced them as int instead of long on the Linux side when possible (see include/xen/arm/hypercall.h), but I didn't attempt to modify all the existing Xen headers. I don't understand your concern with unsigned long. We use them in __DEFINE_XEN_GUEST_HANDLE that are in turn to describe guest pointer. __DEFINE_XEN_GUEST_HANDLE is for pointers inside memory structs, and we defined it as: * XEN_GUEST_HANDLE represents a guest pointer, when passed as a field * in a struct in memory. On ARM is always 8 bytes sizes and 8 bytes * aligned. You probably meant XEN_GUEST_HANDLE_PARAM which is the one for pointers when passed as hypercall parameters, that is defined as: * XEN_GUEST_HANDLE_PARAM represents a guest pointer, when passed as an * hypercall argument. It is 4 bytes on aarch32 and 8 bytes on aarch64. Yes, I meant XEN_GUEST_HANDLE_PARAM. I should have waited to have my laptop in hand rather looking on my phone :). Yes, pointers as hypercalls parameters are the exception to the single-ABI rule and we introduced XEN_GUEST_HANDLE_PARAM purposely to handle them. However, I am not sure we took into account zero-extension when we discussed hypercalls parameters for arm back in the day when I wrote include/xen/arm/hypercall.h. I am not sure to follow your thoughts about taking into account zero-extension in the Linux header. Could you expand it? In the official public headers, I can't find anything telling you the size of each arguments and the number of arguments. Instead you have to look at Xen code to know the exact number of arguments and the size. Did I miss anything? Regarding Argo, there seem to have some kind of documentation per-hypercalls although some does not specify the size. But I can't find anything telling you the command number is in arg0. The mapping to from argN the hypercalls arguments is also not that clear. But I guess this kind of improvement can be done afterwards. The problem with explicitly sized (i.e 32-bit) is you ignore the top 32-bit. This means you can't check the upper bits are always 0 and would prevent extension. That is true. I implicitly assumed that our desire for a common 32-bit/64-bit ABI would not apply just to structs in memory (where we always define unsigned long and pointers as 64-bit) but also seamlessly apply to hypercalls parameters (except for pointers as per the above). There are no documentation in the public interface about the size of each arguments. When looking at traps.c, we assume that hypercalls arguments are the size of a register: typedef register_t (*arm_hypercall_fn_t)(register_t, register_t, register_t, register_t, register_t). So for 64-bit Xen, the hypercalls arguments will be 64-bit. If we want to impose 32-bit value, then we need to update the callback, add sanity check (?) and probably document it. There are still reasons for choosing unsigned int for cases like this where
Re: [Xen-devel] [PATCH v7 02/15] argo: introduce the argo_op hypercall boilerplate
On Tue, 5 Feb 2019, Julien Grall wrote:
> Sorry for the formatting.
>
> On Tue, 5 Feb 2019, 20:04 Stefano Stabellini, wrote:
> On Tue, 5 Feb 2019, Jan Beulich wrote:
> > >>> On 05.02.19 at 01:39, wrote:
> > > On Wed, 30 Jan 2019, Christopher Clark wrote:
> > >> +#include
> > >> +#include
> > >> +
> > >> +long
> > >> +do_argo_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg1,
> > >> + XEN_GUEST_HANDLE_PARAM(void) arg2, unsigned long arg3,
> > >> + unsigned long arg4)
> > >> +{
> > >> + return -ENOSYS;
> > >> +}
> > >> +
> > >> +#ifdef CONFIG_COMPAT
> > >> +long
> > >> +compat_argo_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void)
> arg1,
> > >> + XEN_GUEST_HANDLE_PARAM(void) arg2, unsigned long
> arg3,
> > >> + unsigned long arg4)
> > >> +{
> > >> + return -ENOSYS;
> > >> +}
> > >> +#endif
> > >
> > > From an ARM perspective, it is not a good idea to use unsigned long
> as
> > > hypercall parameters because they are going to be of different size
> on
> > > arm32 and arm64. On ARM, there is no COMPAT code, and we try to
> keep a
> > > single stable ABI across 32bit and 64bit hypervisors (pointers size
> > > being the only exception and we deal with that using
> > > XEN_GUEST_HANDLE_PARAM).
> > >
> > > For this reason, given that we don't need arg3 and arg4 to actually
> be
> > > 64bit, it would be best to use explicitly sized integers instead. I
> > > would use uint32_t or unsigned int for arg3 and arg4. That way,
> there
> > > are not going to be any ABI compatibility issues between arm32 and
> arm64
> > > and we could run, and even migrate, 32bit guests to a 64bit
> hypervisor
> > > without problems.
> > >
> > > I know that Andrew expressed concerns about using unsigned int
> before,
> > > but don't we just need to make sure we are properly ignoring the top
> > > 32bit of arg3 and arg4 when the hypervisor is compiled 64bit?
> >
> > Are you saying that hypercall arguments made by a 32-bit guest on a
> > 64-bit hypervisor do not get zero-extended before reaching the C layer
> > (or more specifically the individual handlers, since on x86 we deal
> with
> > the necessary zero-extension in C nowadays)? What about
> > do_memory_op()'s first parameter then?
>
> If I remember right, there is no zero-extension, however, they should
> still be zero because they have always been zero -- nothing should
> change them in the VM lifetime. However, it is not great to rely on
> that, that is why I suggested to clear them on entry as an alternative,
> and also to have a single ABI between 32bit and 64bit.
>
>
> I can't find the wording again on the Arm Arm (the pdf reader on the phone is
> not great).
>
> But I am afraid this is not correct. Upper 32-bit of the register will be
> zeroed when writing a 32-bit value. So we never rely on
> the register to be zeroed on boot.
Hi Julien,
Thank you for checking your emails. I found the reference in the ARM
ARM, although it took me several minutes!
"The upper 32 bits of the destination register are set to zero."
from C6.1.1 (ID092916).
> FYI do_memory_op is declared as follows on the Linux side for arm32 and
> arm64:
>
> int HYPERVISOR_memory_op(unsigned int cmd, void *arg);
>
> When I went through all existing hypercalls to introduce them on arm32,
> I checked that we didn't actually need 64bit parameters, especially for
> cmd. I introduced them as int instead of long on the Linux side when
> possible (see include/xen/arm/hypercall.h), but I didn't attempt to
> modify all the existing Xen headers.
>
>
> I don't understand your concern with unsigned long. We use them in
> __DEFINE_XEN_GUEST_HANDLE that are in turn to describe guest
> pointer.
__DEFINE_XEN_GUEST_HANDLE is for pointers inside memory structs, and we
defined it as:
* XEN_GUEST_HANDLE represents a guest pointer, when passed as a field
* in a struct in memory. On ARM is always 8 bytes sizes and 8 bytes
* aligned.
You probably meant XEN_GUEST_HANDLE_PARAM which is the one for pointers
when passed as hypercall parameters, that is defined as:
* XEN_GUEST_HANDLE_PARAM represents a guest pointer, when passed as an
* hypercall argument. It is 4 bytes on aarch32 and 8 bytes on aarch64.
Yes, pointers as hypercalls parameters are the exception to the
single-ABI rule and we introduced XEN_GUEST_HANDLE_PARAM purposely to
handle them. However, I am not sure we took into account zero-extension
when we discussed hypercalls parameters for arm back in the day when I
wrote include/xen/arm/hypercall.h.
> The problem with explicitly sized (i.e 32-bit) is you ignore the top 32-bit.
Re: [Xen-devel] [PATCH v7 02/15] argo: introduce the argo_op hypercall boilerplate
Sorry for the formatting.
On Tue, 5 Feb 2019, 20:04 Stefano Stabellini,
wrote:
> On Tue, 5 Feb 2019, Jan Beulich wrote:
> > >>> On 05.02.19 at 01:39, wrote:
> > > On Wed, 30 Jan 2019, Christopher Clark wrote:
> > >> +#include
> > >> +#include
> > >> +
> > >> +long
> > >> +do_argo_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg1,
> > >> + XEN_GUEST_HANDLE_PARAM(void) arg2, unsigned long arg3,
> > >> + unsigned long arg4)
> > >> +{
> > >> +return -ENOSYS;
> > >> +}
> > >> +
> > >> +#ifdef CONFIG_COMPAT
> > >> +long
> > >> +compat_argo_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg1,
> > >> + XEN_GUEST_HANDLE_PARAM(void) arg2, unsigned long arg3,
> > >> + unsigned long arg4)
> > >> +{
> > >> +return -ENOSYS;
> > >> +}
> > >> +#endif
> > >
> > > From an ARM perspective, it is not a good idea to use unsigned long as
> > > hypercall parameters because they are going to be of different size on
> > > arm32 and arm64. On ARM, there is no COMPAT code, and we try to keep a
> > > single stable ABI across 32bit and 64bit hypervisors (pointers size
> > > being the only exception and we deal with that using
> > > XEN_GUEST_HANDLE_PARAM).
> > >
> > > For this reason, given that we don't need arg3 and arg4 to actually be
> > > 64bit, it would be best to use explicitly sized integers instead. I
> > > would use uint32_t or unsigned int for arg3 and arg4. That way, there
> > > are not going to be any ABI compatibility issues between arm32 and
> arm64
> > > and we could run, and even migrate, 32bit guests to a 64bit hypervisor
> > > without problems.
> > >
> > > I know that Andrew expressed concerns about using unsigned int before,
> > > but don't we just need to make sure we are properly ignoring the top
> > > 32bit of arg3 and arg4 when the hypervisor is compiled 64bit?
> >
> > Are you saying that hypercall arguments made by a 32-bit guest on a
> > 64-bit hypervisor do not get zero-extended before reaching the C layer
> > (or more specifically the individual handlers, since on x86 we deal with
> > the necessary zero-extension in C nowadays)? What about
> > do_memory_op()'s first parameter then?
>
> If I remember right, there is no zero-extension, however, they should
> still be zero because they have always been zero -- nothing should
> change them in the VM lifetime. However, it is not great to rely on
> that, that is why I suggested to clear them on entry as an alternative,
> and also to have a single ABI between 32bit and 64bit.
I can't find the wording again on the Arm Arm (the pdf reader on the phone
is not great).
But I am afraid this is not correct. Upper 32-bit of the register will be
zeroed when writing a 32-bit value. So we never rely on the register to be
zeroed on boot.
> FYI do_memory_op is declared as follows on the Linux side for arm32 and
> arm64:
>
> int HYPERVISOR_memory_op(unsigned int cmd, void *arg);
>
> When I went through all existing hypercalls to introduce them on arm32,
> I checked that we didn't actually need 64bit parameters, especially for
> cmd. I introduced them as int instead of long on the Linux side when
> possible (see include/xen/arm/hypercall.h), but I didn't attempt to
> modify all the existing Xen headers.
>
I don't understand your concern with unsigned long. We use them in
__DEFINE_XEN_GUEST_HANDLE that are in turn to describe guest pointer.
The problem with explicitly sized (i.e 32-bit) is you ignore the top
32-bit. This means you can't check the upper bits are always 0 and would
prevent extension.
Cheers,
> ___
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v7 02/15] argo: introduce the argo_op hypercall boilerplate
On Tue, 5 Feb 2019, Jan Beulich wrote:
> >>> On 05.02.19 at 01:39, wrote:
> > On Wed, 30 Jan 2019, Christopher Clark wrote:
> >> +#include
> >> +#include
> >> +
> >> +long
> >> +do_argo_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg1,
> >> + XEN_GUEST_HANDLE_PARAM(void) arg2, unsigned long arg3,
> >> + unsigned long arg4)
> >> +{
> >> +return -ENOSYS;
> >> +}
> >> +
> >> +#ifdef CONFIG_COMPAT
> >> +long
> >> +compat_argo_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg1,
> >> + XEN_GUEST_HANDLE_PARAM(void) arg2, unsigned long arg3,
> >> + unsigned long arg4)
> >> +{
> >> +return -ENOSYS;
> >> +}
> >> +#endif
> >
> > From an ARM perspective, it is not a good idea to use unsigned long as
> > hypercall parameters because they are going to be of different size on
> > arm32 and arm64. On ARM, there is no COMPAT code, and we try to keep a
> > single stable ABI across 32bit and 64bit hypervisors (pointers size
> > being the only exception and we deal with that using
> > XEN_GUEST_HANDLE_PARAM).
> >
> > For this reason, given that we don't need arg3 and arg4 to actually be
> > 64bit, it would be best to use explicitly sized integers instead. I
> > would use uint32_t or unsigned int for arg3 and arg4. That way, there
> > are not going to be any ABI compatibility issues between arm32 and arm64
> > and we could run, and even migrate, 32bit guests to a 64bit hypervisor
> > without problems.
> >
> > I know that Andrew expressed concerns about using unsigned int before,
> > but don't we just need to make sure we are properly ignoring the top
> > 32bit of arg3 and arg4 when the hypervisor is compiled 64bit?
>
> Are you saying that hypercall arguments made by a 32-bit guest on a
> 64-bit hypervisor do not get zero-extended before reaching the C layer
> (or more specifically the individual handlers, since on x86 we deal with
> the necessary zero-extension in C nowadays)? What about
> do_memory_op()'s first parameter then?
If I remember right, there is no zero-extension, however, they should
still be zero because they have always been zero -- nothing should
change them in the VM lifetime. However, it is not great to rely on
that, that is why I suggested to clear them on entry as an alternative,
and also to have a single ABI between 32bit and 64bit.
FYI do_memory_op is declared as follows on the Linux side for arm32 and
arm64:
int HYPERVISOR_memory_op(unsigned int cmd, void *arg);
When I went through all existing hypercalls to introduce them on arm32,
I checked that we didn't actually need 64bit parameters, especially for
cmd. I introduced them as int instead of long on the Linux side when
possible (see include/xen/arm/hypercall.h), but I didn't attempt to
modify all the existing Xen headers.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v7 02/15] argo: introduce the argo_op hypercall boilerplate
>>> On 05.02.19 at 01:39, wrote:
> On Wed, 30 Jan 2019, Christopher Clark wrote:
>> +#include
>> +#include
>> +
>> +long
>> +do_argo_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg1,
>> + XEN_GUEST_HANDLE_PARAM(void) arg2, unsigned long arg3,
>> + unsigned long arg4)
>> +{
>> +return -ENOSYS;
>> +}
>> +
>> +#ifdef CONFIG_COMPAT
>> +long
>> +compat_argo_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg1,
>> + XEN_GUEST_HANDLE_PARAM(void) arg2, unsigned long arg3,
>> + unsigned long arg4)
>> +{
>> +return -ENOSYS;
>> +}
>> +#endif
>
> From an ARM perspective, it is not a good idea to use unsigned long as
> hypercall parameters because they are going to be of different size on
> arm32 and arm64. On ARM, there is no COMPAT code, and we try to keep a
> single stable ABI across 32bit and 64bit hypervisors (pointers size
> being the only exception and we deal with that using
> XEN_GUEST_HANDLE_PARAM).
>
> For this reason, given that we don't need arg3 and arg4 to actually be
> 64bit, it would be best to use explicitly sized integers instead. I
> would use uint32_t or unsigned int for arg3 and arg4. That way, there
> are not going to be any ABI compatibility issues between arm32 and arm64
> and we could run, and even migrate, 32bit guests to a 64bit hypervisor
> without problems.
>
> I know that Andrew expressed concerns about using unsigned int before,
> but don't we just need to make sure we are properly ignoring the top
> 32bit of arg3 and arg4 when the hypervisor is compiled 64bit?
Are you saying that hypercall arguments made by a 32-bit guest on a
64-bit hypervisor do not get zero-extended before reaching the C layer
(or more specifically the individual handlers, since on x86 we deal with
the necessary zero-extension in C nowadays)? What about
do_memory_op()'s first parameter then?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v7 02/15] argo: introduce the argo_op hypercall boilerplate
On Wed, 30 Jan 2019, Christopher Clark wrote:
> +#include
> +#include
> +
> +long
> +do_argo_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg1,
> + XEN_GUEST_HANDLE_PARAM(void) arg2, unsigned long arg3,
> + unsigned long arg4)
> +{
> +return -ENOSYS;
> +}
> +
> +#ifdef CONFIG_COMPAT
> +long
> +compat_argo_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg1,
> + XEN_GUEST_HANDLE_PARAM(void) arg2, unsigned long arg3,
> + unsigned long arg4)
> +{
> +return -ENOSYS;
> +}
> +#endif
From an ARM perspective, it is not a good idea to use unsigned long as
hypercall parameters because they are going to be of different size on
arm32 and arm64. On ARM, there is no COMPAT code, and we try to keep a
single stable ABI across 32bit and 64bit hypervisors (pointers size
being the only exception and we deal with that using
XEN_GUEST_HANDLE_PARAM).
For this reason, given that we don't need arg3 and arg4 to actually be
64bit, it would be best to use explicitly sized integers instead. I
would use uint32_t or unsigned int for arg3 and arg4. That way, there
are not going to be any ABI compatibility issues between arm32 and arm64
and we could run, and even migrate, 32bit guests to a 64bit hypervisor
without problems.
I know that Andrew expressed concerns about using unsigned int before,
but don't we just need to make sure we are properly ignoring the top
32bit of arg3 and arg4 when the hypervisor is compiled 64bit?
I am really sorry for pointing this out so late in the review cycle, but
I only spotted it now.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v7 02/15] argo: introduce the argo_op hypercall boilerplate
Hi Christopher, On 2/4/19 8:32 PM, Christopher Clark wrote: On Wed, Jan 30, 2019 at 8:14 PM Christopher Clark wrote: On Fri, Jan 25, 2019 at 10:55 AM Christopher Clark wrote: On Thu, Jan 24, 2019 at 2:08 AM Julien Grall wrote: [...] Sorry for noticing quite late in the process. Don't you need to add the hypercall in xen/arch/arm/traps.c? Adding this looked fine, so I've added the ARM hypercall table entry to this patch. Julien, do you have any further feedback on the latest version of this patch? I will not be able to look at the series this week. I defer the Arm part to Stefano. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v7 02/15] argo: introduce the argo_op hypercall boilerplate
On Wed, Jan 30, 2019 at 8:14 PM Christopher Clark
wrote:
>
> On Fri, Jan 25, 2019 at 10:55 AM Christopher Clark
> wrote:
> >
> > On Thu, Jan 24, 2019 at 2:08 AM Julien Grall wrote:
> > > [...]
> > > Sorry for noticing quite late in the process. Don't you need to add the
> > > hypercall in xen/arch/arm/traps.c?
>
> Adding this looked fine, so I've added the ARM hypercall table entry to
> this patch.
Julien, do you have any further feedback on the latest version of this patch?
Christopher
On Wed, Jan 30, 2019 at 8:28 PM Christopher Clark
wrote:
>
> Presence is gated upon CONFIG_ARGO.
>
> Registers the hypercall previously reserved for this.
> Takes 5 arguments, does nothing and returns -ENOSYS.
>
> Implementation will provide a compat ABI so COMPAT_CALL is the
> macro used in the hypercall tables.
>
> Signed-off-by: Christopher Clark
> ---
> v6 dropped Jan Acked-by due to change of implementation and commit msg
> v6 switched to COMPAT_CALL and provides compat_argo_op
> v6 feedback #3 Julien: add argo_op to the ARM hypercall table
> v2 Copyright line: add 2019
> v2 feedback #3 Jan: drop "message" from argo_message_op
> v2 feedback #3 Jan: add Acked-by
> v1 feedback #15 Jan: handle upper-halves of hypercall args
> v1 feedback #15 Jan: use unsigned where negative values impossible
>
> xen/arch/arm/traps.c| 3 +++
> xen/arch/x86/guest/hypercall_page.S | 2 +-
> xen/arch/x86/hvm/hypercall.c| 3 +++
> xen/arch/x86/hypercall.c| 3 +++
> xen/arch/x86/pv/hypercall.c | 3 +++
> xen/common/Makefile | 1 +
> xen/common/argo.c | 38
> +
> xen/include/public/xen.h| 2 +-
> xen/include/xen/hypercall.h | 18 ++
> 9 files changed, 71 insertions(+), 2 deletions(-)
> create mode 100644 xen/common/argo.c
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 221c762..e1e8ac9 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1397,6 +1397,9 @@ static arm_hypercall_t arm_hypercall_table[] = {
> HYPERCALL(platform_op, 1),
> HYPERCALL_ARM(vcpu_op, 3),
> HYPERCALL(vm_assist, 2),
> +#ifdef CONFIG_ARGO
> +HYPERCALL(argo_op, 5),
> +#endif
> };
>
> #ifndef NDEBUG
> diff --git a/xen/arch/x86/guest/hypercall_page.S
> b/xen/arch/x86/guest/hypercall_page.S
> index fdd2e72..26afabf 100644
> --- a/xen/arch/x86/guest/hypercall_page.S
> +++ b/xen/arch/x86/guest/hypercall_page.S
> @@ -59,7 +59,7 @@ DECLARE_HYPERCALL(sysctl)
> DECLARE_HYPERCALL(domctl)
> DECLARE_HYPERCALL(kexec_op)
> DECLARE_HYPERCALL(tmem_op)
> -DECLARE_HYPERCALL(xc_reserved_op)
> +DECLARE_HYPERCALL(argo_op)
> DECLARE_HYPERCALL(xenpmu_op)
>
> DECLARE_HYPERCALL(arch_0)
> diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
> index 19d1263..5bb1750 100644
> --- a/xen/arch/x86/hvm/hypercall.c
> +++ b/xen/arch/x86/hvm/hypercall.c
> @@ -134,6 +134,9 @@ static const hypercall_table_t hvm_hypercall_table[] = {
> #ifdef CONFIG_TMEM
> HYPERCALL(tmem_op),
> #endif
> +#ifdef CONFIG_ARGO
> +COMPAT_CALL(argo_op),
> +#endif
> COMPAT_CALL(platform_op),
> #ifdef CONFIG_PV
> COMPAT_CALL(mmuext_op),
> diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c
> index 032de8f..93e7860 100644
> --- a/xen/arch/x86/hypercall.c
> +++ b/xen/arch/x86/hypercall.c
> @@ -64,6 +64,9 @@ const hypercall_args_t hypercall_args_table[NR_hypercalls] =
> ARGS(domctl, 1),
> ARGS(kexec_op, 2),
> ARGS(tmem_op, 1),
> +#ifdef CONFIG_ARGO
> +ARGS(argo_op, 5),
> +#endif
> ARGS(xenpmu_op, 2),
> #ifdef CONFIG_HVM
> ARGS(hvm_op, 2),
> diff --git a/xen/arch/x86/pv/hypercall.c b/xen/arch/x86/pv/hypercall.c
> index 5d11911..f452dd5 100644
> --- a/xen/arch/x86/pv/hypercall.c
> +++ b/xen/arch/x86/pv/hypercall.c
> @@ -77,6 +77,9 @@ const hypercall_table_t pv_hypercall_table[] = {
> #ifdef CONFIG_TMEM
> HYPERCALL(tmem_op),
> #endif
> +#ifdef CONFIG_ARGO
> +COMPAT_CALL(argo_op),
> +#endif
> HYPERCALL(xenpmu_op),
> #ifdef CONFIG_HVM
> HYPERCALL(hvm_op),
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index 56fc201..59ac7de 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -1,3 +1,4 @@
> +obj-$(CONFIG_ARGO) += argo.o
> obj-y += bitmap.o
> obj-y += bsearch.o
> obj-$(CONFIG_CORE_PARKING) += core_parking.o
> diff --git a/xen/common/argo.c b/xen/common/argo.c
> new file mode 100644
> index 000..ddc48f1
> --- /dev/null
> +++ b/xen/common/argo.c
> @@ -0,0 +1,38 @@
> +/**
> + * Argo : Hypervisor-Mediated data eXchange
> + *
> + * Derived from v4v, the version 2 of v2v.
> + *
> + * Copyright (c) 2010, Citrix Systems
> + * Copyright (c) 2018-2019 BAE Systems
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *
Re: [Xen-devel] [PATCH v7 02/15] argo: introduce the argo_op hypercall boilerplate
>>> On 31.01.19 at 05:28, wrote: > Presence is gated upon CONFIG_ARGO. > > Registers the hypercall previously reserved for this. > Takes 5 arguments, does nothing and returns -ENOSYS. > > Implementation will provide a compat ABI so COMPAT_CALL is the > macro used in the hypercall tables. > > Signed-off-by: Christopher Clark > --- > v6 dropped Jan Acked-by due to change of implementation and commit msg Feel free to re-instate. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
