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, <sstabell...@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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
