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

[Julien Grall <julien.grall@xxxxxxx>]

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, <sstabellini@xxxxxxxxxx> 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 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.
I think the compiler should catch it. However, I think allowing up to 
64-bit arguments is nice to have if we want to introduce extension for 
64-bit only guest (i.e larger buffer...).
> 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
    - 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.
Cheers,

--
Julien Grall

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