Re: [PATCH v7 02/15] argo: introduce the argo_op hypercall boilerplate

[Stefano Stabellini <sstabellini@kernel.org>]

On Tue, 5 Feb 2019, Jan Beulich wrote:
> >>> On 05.02.19 at 01:39, <sstabellini@kernel.org> wrote:
> > On Wed, 30 Jan 2019, Christopher Clark wrote:
> >> +#include <xen/errno.h>
> >> +#include <xen/guest_access.h>
> >> +
> >> +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