Re: [PATCH v2 1/3] xen/arm: Add way to disable traps on accesses to unmapped addresses

["Edgar E. Iglesias" <edgar.iglesias@xxxxxxxxx>]

On Thu, May 29, 2025 at 04:59:21PM +0100, Julien Grall wrote:
> Hi Edgar,

Hi Julien,


> 
> On 29/05/2025 16:50, Edgar E. Iglesias wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xxxxxxx>
> > 
> > Add a per-domain way to optionally disable traps for accesses
> > to unmapped addresses.
> > 
> > The domain flag is general but it's only implemented for ARM for now.
> > 
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xxxxxxx>
> > ---
> >   tools/libs/light/libxl_arm.c  |  3 +++
> >   xen/arch/arm/dom0less-build.c |  3 +++
> >   xen/arch/arm/domain.c         |  3 ++-
> >   xen/arch/arm/domain_build.c   |  3 ++-
> >   xen/arch/arm/io.c             | 36 +++++++++++++++++++++++++++++++++--
> >   xen/common/domain.c           |  3 ++-
> >   xen/include/public/domctl.h   |  4 +++-
> 
> Looking at the changelog, I saw you removed the go bindings (although, they
> were in patch 3). But I don't quite understand why.

I got a little confused. The file tools/golang/xenlight/helpers.gen.go
has the following at the top:
// Code generated by gengotypes.py. DO NOT EDIT.
// source: libxl_types.idl


So I got the impression that we shouldn't be editing it.
Should I edit it manually? Or should I try to rerun gengotypes.py
to generate these bindings?


> 
> Also, I think you need to update the OCaml bindings.

I see, I'll have a look.

> 
> 
> >   7 files changed, 49 insertions(+), 6 deletions(-)
> > 
> > diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> > index 75c811053c..9530996e72 100644
> > --- a/tools/libs/light/libxl_arm.c
> > +++ b/tools/libs/light/libxl_arm.c
> > @@ -233,6 +233,9 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> >           config->arch.sve_vl = d_config->b_info.arch_arm.sve_vl / 128U;
> >       }
> > +    /* Trap accesses to unmapped areas. */
> > +    config->flags |= XEN_DOMCTL_CDF_trap_unmapped_accesses;
> > +
> >       return 0;
> >   }
> > diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> > index a49764f0ad..a4e0a33632 100644
> > --- a/xen/arch/arm/dom0less-build.c
> > +++ b/xen/arch/arm/dom0less-build.c
> > @@ -343,6 +343,9 @@ void __init arch_create_domUs(struct dt_device_node 
> > *node,
> >           panic("'sve' property found, but CONFIG_ARM64_SVE not 
> > selected\n");
> >   #endif
> >       }
> > +
> > +    /* Trap accesses to unmapped areas. */
> > +    d_cfg->flags |= XEN_DOMCTL_CDF_trap_unmapped_accesses;
> >   }
> >   int __init init_intc_phandle(struct kernel_info *kinfo, const char *name,
> > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > index 45aeb8bddc..be58a23dd7 100644
> > --- a/xen/arch/arm/domain.c
> > +++ b/xen/arch/arm/domain.c
> > @@ -612,7 +612,8 @@ int arch_sanitise_domain_config(struct 
> > xen_domctl_createdomain *config)
> >       unsigned int max_vcpus;
> >       unsigned int flags_required = (XEN_DOMCTL_CDF_hvm | 
> > XEN_DOMCTL_CDF_hap);
> >       unsigned int flags_optional = (XEN_DOMCTL_CDF_iommu | 
> > XEN_DOMCTL_CDF_vpmu |
> > -                                   XEN_DOMCTL_CDF_xs_domain );
> > +                                   XEN_DOMCTL_CDF_xs_domain |
> > +                                   XEN_DOMCTL_CDF_trap_unmapped_accesses );
> >       unsigned int sve_vl_bits = sve_decode_vl(config->arch.sve_vl);
> >       if ( (config->flags & ~flags_optional) != flags_required )
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index b189a7cfae..7ff9c1b584 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -2003,7 +2003,8 @@ void __init create_dom0(void)
> >   {
> >       struct domain *dom0;
> >       struct xen_domctl_createdomain dom0_cfg = {
> > -        .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
> > +        .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap |
> > +                 XEN_DOMCTL_CDF_trap_unmapped_accesses,
> >           .max_evtchn_port = -1,
> >           .max_grant_frames = gnttab_dom0_frames(),
> >           .max_maptrack_frames = -1,
> > diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> > index 5a4b0e8f25..adfc822e7e 100644
> > --- a/xen/arch/arm/io.c
> > +++ b/xen/arch/arm/io.c
> > @@ -21,6 +21,32 @@
> >   #include "decode.h"
> > +/* Handler for unmapped ranges. Writes ignored, reads return all ones.  */
> > +static int unmapped_read(struct vcpu *v, mmio_info_t *info, register_t *r,
> > +                         void *priv)
> > +{
> > +    uint64_t mask = GENMASK_ULL((1U << info->dabt.size) * 8 - 1, 0);
> 
> NIT: Looking at the other part of io.c, we are using GENMASK. Any reason to
> not use the same?

Looking closer at it now and no, there's no good reason. I'll change
to GENMASK in v3.


> 
> > +
> > +    /* Mask off upper bits.  */
> > +    *r = UINT64_MAX & mask;
> > +    return 1;
> > +}
> > +
> > +static int unmapped_write(struct vcpu *v, mmio_info_t *info, register_t r,
> > +                          void *priv)
> > +{
> > +    return 1;
> > +}
> > +
> > +static const struct mmio_handler_ops unmapped_ops = {
> > +    .read = unmapped_read,
> > +    .write = unmapped_write
> > +};
> > +
> > +static const struct mmio_handler unmapped_handler = {
> > +    .ops = &unmapped_ops
> > +};
> > +
> >   static enum io_state handle_read(const struct mmio_handler *handler,
> >                                    struct vcpu *v,
> >                                    mmio_info_t *info)
> > @@ -175,11 +201,17 @@ enum io_state try_handle_mmio(struct cpu_user_regs 
> > *regs,
> >       handler = find_mmio_handler(v->domain, info->gpa);
> >       if ( !handler )
> >       {
> > +        bool trap_unmapped = v->domain->options &
> > +                                         
> > XEN_DOMCTL_CDF_trap_unmapped_accesses;
> >           rc = try_fwd_ioserv(regs, v, info);
> >           if ( rc == IO_HANDLED )
> >               return handle_ioserv(regs, v);
> > -
> > -        return rc;
> > +        else if ( rc == IO_UNHANDLED && !trap_unmapped )
> > +        {
> > +            /* Fallback to the unmapped handler. */
> > +            handler = &unmapped_handler;
> > +        } else
> 
> Style:
> 
> else if ( ... )
> {
> }
> else
> {
> }

Will fix for v3.

Thanks,
Edgar


> 
> > +            return rc;
> >       }
> >       /*
> > diff --git a/xen/common/domain.c b/xen/common/domain.c
> > index abf1969e60..ac4f58f638 100644
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -721,7 +721,8 @@ static int sanitise_domain_config(struct 
> > xen_domctl_createdomain *config)
> >            ~(XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap |
> >              XEN_DOMCTL_CDF_s3_integrity | XEN_DOMCTL_CDF_oos_off |
> >              XEN_DOMCTL_CDF_xs_domain | XEN_DOMCTL_CDF_iommu |
> > -           XEN_DOMCTL_CDF_nested_virt | XEN_DOMCTL_CDF_vpmu) )
> > +           XEN_DOMCTL_CDF_nested_virt | XEN_DOMCTL_CDF_vpmu |
> > +           XEN_DOMCTL_CDF_trap_unmapped_accesses) )
> >       {
> >           dprintk(XENLOG_INFO, "Unknown CDF flags %#x\n", config->flags);
> >           return -EINVAL;
> > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> > index 5b2063eed9..be19ab5e26 100644
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -66,9 +66,11 @@ struct xen_domctl_createdomain {
> >   #define XEN_DOMCTL_CDF_nested_virt    (1U << _XEN_DOMCTL_CDF_nested_virt)
> >   /* Should we expose the vPMU to the guest? */
> >   #define XEN_DOMCTL_CDF_vpmu           (1U << 7)
> > +/* Should we trap guest accesses to unmapped addresses? */
> > +#define XEN_DOMCTL_CDF_trap_unmapped_accesses  (1U << 8)
> >   /* Max XEN_DOMCTL_CDF_* constant.  Used for ABI checking. */
> > -#define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_vpmu
> > +#define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_trap_unmapped_accesses
> >       uint32_t flags;
> 
> -- 
> Julien Grall
>