Re: [PATCH] xen/arm: XEN_DOMCTL_get_address_size hypercall support

[Milan Djokic <milan_djokic@xxxxxxxx>]

Hi Julien,

On 10/2/25 14:43, Julien Grall wrote:
> Hi,
>
> On 02/10/2025 12:10, Milan Djokic wrote:
> > Hello Andrew,
> >
> > On 10/2/25 12:10, Andrew Cooper wrote:
> > > On 01/10/2025 9:01 pm, Milan Djokic wrote:
> > > > Signed-off-by: Milan Djokic <milan_djokic@xxxxxxxx>
> > > >
> > > > ---
> > > > XEN_DOMCTL_get_address_size hypercall is not implemented for arm
> > > > (only for x86)
> > > > It would be useful to have this hypercall supported for arm64, in
> > > > order to get
> > > > current guest addressing mode and also to verify that
> > > > XEN_DOMCTL_set_address_size
> > > > performs switch to target addressing mode (instead of relying on its
> > > > returned error code only).
> > > Please don't copy this misfeature of x86 PV guests into ARM.
> > >
> > > Letting domains be of variable bitness after domain create leads to a
> > > whole lot of bugs, many security relevant.
> > >
> > > 32bit vs 64bit should be an input to domain_create(), not something that
> > > is edited after the domain has been constructed.
> > Yes, the idea behind this patch is not to introduce variable bitness,
> > just to have the ability to get current addressing mode through
> > hypercall. In our case, we have used it only in domain creation path
> > (after domain_create(), to verify that target addressing mode is set).
> > Of course, whether this hypercall support would be useful in mainline is
> > open for discussion.
> We already have a series under review [1] that follow what Andrew is
> suggesting. I would rather focus on getting it committed rather than
> trying to workaround it.
Thank you for clarification. In that case, I will abandon this patch.
You are referring to path series from Grygorii [1]? This means that 
XEN_DOMCTL_set_address_size hypercall will be removed and its function 
integrated as part of domain_create() and XEN_DOMCTL_createdomain (for 
toolstack) hypercall?
> > > > diff --git a/xen/arch/arm/arm64/domctl.c b/xen/arch/arm/arm64/domctl.c
> > > > index 8720d126c9..f227309e06 100644
> > > > --- a/xen/arch/arm/arm64/domctl.c
> > > > +++ b/xen/arch/arm/arm64/domctl.c
> > > > @@ -33,6 +34,37 @@ static long switch_mode(struct domain *d, enum
> > > > domain_type type)
> > > >        return 0;
> > > >    }
> > > > +static long get_address_size(struct domain *d, uint32_t *address_size)
> > > > +{
> > > > +    long rc = 0;
> > > > +    struct vcpu *v;
> > > > +    /* Check invalid arguments */
> > > > +    if ( d == NULL || address_size == NULL) {
> > > > +        rc = -EINVAL;
> > > > +    }
> > > > +    /* Domain structure type field and actual vcpu mode must be
> > > > aligned */
> > > > +    if(rc == 0) {
> > > > +        for_each_vcpu(d, v) {
> > > > +            if(vcpu_get_mode(v) != d->arch.type) {
> > > > +                rc = -EFAULT;
> > > > +            }
> > > > +        }
> > > This is deeply suspicious.
> > >
> > > Under what circumstances can the vCPU setting be different from the
> > > domain setting?
> > It should never happen, this is more of a sanity check. Alternative
> > would be to use only one of vCPU or domain settings, checking against
> > both seemed more complete to me.
> This would be a logical error in Xen rather than something that could be
> triggered by the toolstack. I feel it could mislead people using release
> build because the fault is not due to the input provided.
>
> Cheers,
[1] 
https://patchwork.kernel.org/project/xen-devel/cover/20250911082034.1326377-1-grygorii_strashko@xxxxxxxx/
BR,
Milan