Re: [PATCH 05/15] tools/libs/light: Increase nr_spi to 160

[Julien Grall <julien@xxxxxxx>]

Hi,

On 06/05/2024 06:17, Henry Wang wrote:
> On 5/1/2024 9:58 PM, Anthony PERARD wrote:
> > On Wed, Apr 24, 2024 at 11:34:39AM +0800, Henry Wang wrote:
> > > Increase number of spi to 160 i.e. gic_number_lines() for Xilinx 
> > > ZynqMP - 32.
> > > This was done to allocate and assign IRQs to a running domain.
> > >
> > > Signed-off-by: Vikram Garhwal <fnu.vikram@xxxxxxxxxx>
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> > > Signed-off-by: Henry Wang <xin.wang2@xxxxxxx>
> > > ---
> > >   tools/libs/light/libxl_arm.c | 3 ++-
> > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> > > index dd5c9f4917..50dbd0f2a9 100644
> > > --- a/tools/libs/light/libxl_arm.c
> > > +++ b/tools/libs/light/libxl_arm.c
> > > @@ -181,7 +181,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> > >       LOG(DEBUG, "Configure the domain");
> > > -    config->arch.nr_spis = nr_spis;
> > > +    /* gic_number_lines() is 192 for Xilinx ZynqMP. min nr_spis = 
> > > 192 - 32. */
> > > +    config->arch.nr_spis = MAX(nr_spis, 160);
> > Is there a way that that Xen or libxl could find out what the minimum
> > number of SPI needs to be?
> I am afraid currently there is none.
>
> > Are we going to have to increase that minimum
> > number every time a new platform comes along?
> >
> > It doesn't appear that libxl is using that `nr_spis` value and it is
> > probably just given to Xen. So my guess is that Xen could simply take
> > care of the minimum value, gic_number_lines() seems to be a Xen
> > function.
> Xen will take care of the value of nr_spis for dom0 in create_dom0()
> dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32;
> and also for dom0less domUs in create_domUs().
>
> However, it looks like Xen will not take care of the mininum value for 
> libxl guests, the value from config->arch.nr_spis in guest config file 
> will be directly passed to the domain_vgic_init() function from 
> arch_domain_create().
> I agree with you that we shouldn't just bump the number everytime when 
> we have a new platform. Therefore, would it be a good idea to move the 
> logic in this patch to arch_sanitise_domain_config()?
Xen domains are supposed to be platform agnostics and therefore the 
numbers of SPIs should not be based on the HW.
Furthermore, with your proposal we would end up to allocate data 
structure for N SPIs when a domain may never needs any SPIs (such as if 
passthrough is not in-use). This is more likely for domain created by 
the toolstack than from Xen directly.
Instead, we should introduce a new XL configuration to let the user 
decide the number of SPIs. I would suggest to name "nr_spis" to match 
the DT bindings.
Cheers,

--
Julien Grall