Re: [PATCH 05/23] xen/arm: Add capabilities to dom0less

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2025-03-07 04:01, Julien Grall wrote:
> Hi,
>
> On 06/03/2025 22:03, Jason Andryuk wrote:
> > Add capabilities property to dom0less to allow building a
> > disaggregated system.
> >
> > Introduce bootfdt.h to contain these constants.
> >
> > When using the hardware or xenstore capabilities, adjust the grant and
> > event channel limits similar to dom0.
>  > > Also for the hardware domain, set directmap and iommu.  This brings 
> its
> > configuration in line with a dom0.
> Looking the device tree bindings, a user would be allowed to disable 
> "passthrough" or even "directmap". This means, we would never be able to 
> disable "directmap" for the hardware domain in the future with the 
> existing property (this is to avoid break backwards compatibility).
> Instead, I think we should check what the user provided and confirm this 
> is matching our expectation for an hardware domain.
>
> That said, I am not entirely sure why we should force directmap for the 
> HW domain. We are starting from a clean slate, so I think it would be 
> better to have by default no directmap and imposing the presence of an 
> IOMMU in the system.
Ok, it seems like directmap is not necessary.  It was helpful early on 
to get things booting, but I think it's no longer necessary after 
factoring out construct_hwdom().
What exactly do you mean by imposing with respect to the iommu?  Require 
one, or mirror the dom0 code and set it for the hwdom?
    if ( iommu_enabled )
        dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;

> Lastly, can you provide an example of what the hardware domain DT node 
> would looke like?
I've attached a dump of /sys/firmware/fdt from hwdom.  (This is direct 
mapped).
> > --- a/xen/arch/arm/dom0less-build.c
> > +++ b/xen/arch/arm/dom0less-build.c
> > @@ -12,6 +12,7 @@
> >   #include <xen/sizes.h>
> >   #include <xen/vmap.h>
> > +#include <public/bootfdt.h>
> >   #include <public/io/xs_wire.h>
> >   #include <asm/arm64/sve.h>
> > @@ -994,6 +995,34 @@ void __init create_domUs(void)
> >           if ( (max_init_domid + 1) >= DOMID_FIRST_RESERVED )
> >               panic("No more domain IDs available\n");
> > +        if ( dt_property_read_u32(node, "capabilities", &val) )
> > +        {
> > +            if ( val & ~DOMAIN_CAPS_MASK )
> > +                panic("Invalid capabilities (%"PRIx32")\n", val);
> > +
> > +            if ( val & DOMAIN_CAPS_CONTROL )
> > +                flags |= CDF_privileged;
> > +
> > +            if ( val & DOMAIN_CAPS_HARDWARE )
> > +            {
> > +                if ( hardware_domain )
> > +                    panic("Only 1 hardware domain can be specified! 
> > (%pd)\n",
> > +                           hardware_domain);
> > +
> > +                d_cfg.max_grant_frames = gnttab_dom0_frames();
> > +                d_cfg.max_evtchn_port = -1;
> What about d_cfg.arch.nr_spis? Are we expecting the user to pass "nr_spis"?
Further down, when nr_spis isn't specified in the DT, it defaults to:
    d_cfg.arch.nr_spis = gic_number_lines() - 32;

Dom0 does:
    /*
     * Xen vGIC supports a maximum of 992 interrupt lines.
     * 32 are substracted to cover local IRQs.
     */
    dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) 
- 32;
    if ( gic_number_lines() > 992 )
        printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n");

So I think it's fine as-is?  I could add the min() and warning if you 
think it's necessary.
Regards,
Jason

Attachment: hwdom-dts
Description: Text document