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

[Henry Wang <xin.wang2@amd.com>]

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