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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 5 Feb 2019, Julien Grall wrote:
> Sorry for the formatting.
> 
> On Tue, 5 Feb 2019, 20:04 Stefano Stabellini, <sstabellini@xxxxxxxxxx> wrote:
>       On Tue, 5 Feb 2019, Jan Beulich wrote:
>       > >>> On 05.02.19 at 01:39, <sstabellini@xxxxxxxxxx> 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.
> 
> 
> 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. 
> 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 still reasons for choosing unsigned int for cases like this
where unsigned long is not actually necessary, but not a strong as I
previously thought. For example, it could be natural to introduce a
value for a cmd or a flag parameter not available to 32-bit guests (i.e.
0xff00000000000000) by mistake, although I admit that the related Xen
code should throw a warning when compiled for arm32.

In conclusion, if you and other maintainers prefer unsigned long I'll
drop my reservation.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel