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

[Stefano Stabellini <sstabellini@kernel.org>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 7113 bytes --]

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, <sstabellini@kernel.org>
> > > 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 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.

Good point.


> > 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...).

That's true. In this case, Christopher was telling me the arguments
could easily be 32bit only.


> > 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? You are
*not* saying that unsigned long should only be used with 64-bit
guests/hypervisor?


>     - 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.

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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