Re: [PATCH v2 4/8] tools/arm: Introduce the "nr_spis" xl config entry

[Henry Wang <xin.wang2@xxxxxxx>]

Hi Jason,

On 5/21/2024 3:13 AM, Jason Andryuk wrote:
> > +
> > +=item B<nr_spis="NR_SPIS">
> > +
> > +A 32-bit optional integer parameter specifying the number of SPIs 
> > (Shared
> I'd phrase it "An optional 32-but integer"
>
> > +Peripheral Interrupts) to allocate for the domain. If the `nr_spis` 
> > parameter
> > +is missing, the max number of SPIs calculated by the toolstack based 
> > on the
> > +devices allocated for the domain will be used.
> This text says the maximum only applies if xl.cfg nr_spis is not setup.
>
> > +
> > +=back
> > +
> >   =head3 x86
> >     =over 4
> > diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> > index 1cb89fa584..a4029e3ac8 100644
> > --- a/tools/libs/light/libxl_arm.c
> > +++ b/tools/libs/light/libxl_arm.c
> > @@ -181,8 +181,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> >         LOG(DEBUG, "Configure the domain");
> >   -    config->arch.nr_spis = nr_spis;
> > -    LOG(DEBUG, " - Allocate %u SPIs", nr_spis);
> > +    config->arch.nr_spis = max(nr_spis, 
> > d_config->b_info.arch_arm.nr_spis);
> > +    LOG(DEBUG, " - Allocate %u SPIs", config->arch.nr_spis);
> But this is always taking the max.  Should it instead be:
>
> config->arch.nr_spis = d_config->b_info.arch_arm.nr_spis ?: nr_spis;
>
> However, I don't know if that makes sense for ARM.  Does the hardware 
> nr_spis need to be a minimum for a domain?
> Really, we just want the documentation to match the code.
Before you pointed this out, I didn't realize the ambiguity in the doc 
about the "max". The "max" in the doc have different meanings compared 
to the "max()" in the code. I will drop the "max" in the doc and reword 
the doc to "If the `nr_spis` parameter is missing, the number of SPIs 
calculated by the toolstack based on the  devices allocated for the 
domain will be used.". Thanks for pointing it out.
Kind regards,
Henry

> Thanks,
> Jason