[PATCH-for-10.0,3/5] hw/arm/virt-acpi: Factor its_enabled() helper out

GIC ITS is checked for the MADT and IORT tables.
Factor the checks out to the its_enabled() helper.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/virt-acpi-build.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Hi Phil,

On 3/31/25 19:12, Philippe Mathieu-Daudé wrote:
> GIC ITS is checked for the MADT and IORT tables.
> Factor the checks out to the its_enabled() helper.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/arm/virt-acpi-build.c | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 3ac8f8e1786..fdc08b40883 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -208,6 +208,13 @@ static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState *vms)
>   #define ROOT_COMPLEX_ENTRY_SIZE 36
>   #define IORT_NODE_OFFSET 48
>   
> +static bool its_enabled(VirtMachineState *vms)
> +{
> +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> +
> +    return its_class_name() && !vmc->no_its;
> +}
> +

Isn't its_class_name() always "true"?


Cheers,
Gustavo

>   /*
>    * Append an ID mapping entry as described by "Table 4 ID mapping format" in
>    * "IO Remapping Table System Software on ARM Platforms", Chapter 3.
> @@ -670,7 +677,6 @@ static void
>   build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>   {
>       int i;
> -    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>       const MemMapEntry *memmap = vms->memmap;
>       AcpiTable table = { .sig = "APIC", .rev = 4, .oem_id = vms->oem_id,
>                           .oem_table_id = vms->oem_table_id };
> @@ -741,7 +747,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>                                             memmap[VIRT_HIGH_GIC_REDIST2].size);
>           }
>   
> -        if (its_class_name() && !vmc->no_its) {
> +        if (its_enabled(vms)) {
>               /*
>                * ACPI spec, Revision 6.0 Errata A
>                * (original 6.0 definition has invalid Length)
> @@ -973,7 +979,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>                             vms->oem_table_id);
>       }
>   
> -    if (its_class_name() && !vmc->no_its) {
> +    if (its_enabled(vms)) {
>           acpi_add_table(table_offsets, tables_blob);
>           build_iort(tables_blob, tables->linker, vms);
>       }

On 2/4/25 08:43, Gustavo Romero wrote:
> Hi Phil,
> 
> On 3/31/25 19:12, Philippe Mathieu-Daudé wrote:
>> GIC ITS is checked for the MADT and IORT tables.
>> Factor the checks out to the its_enabled() helper.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/arm/virt-acpi-build.c | 12 +++++++++---
>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 3ac8f8e1786..fdc08b40883 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -208,6 +208,13 @@ static void acpi_dsdt_add_tpm(Aml *scope, 
>> VirtMachineState *vms)
>>   #define ROOT_COMPLEX_ENTRY_SIZE 36
>>   #define IORT_NODE_OFFSET 48
>> +static bool its_enabled(VirtMachineState *vms)
>> +{
>> +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>> +
>> +    return its_class_name() && !vmc->no_its;
>> +}
>> +
> 
> Isn't its_class_name() always "true"?

The method signature is described as:

  /**
   * its_class_name:
   *
   * Return the ITS class name to use depending on whether
   * KVM acceleration  and KVM CAP_SIGNAL_MSI are supported
   *
   * Returns: class name to use or NULL
   */
  const char *its_class_name(void);

So I'd say no.

Indeed since commit cc5e719e2c8 ("kvm: require KVM_CAP_SIGNAL_MSI")
the single implementation doesn't return NULL anymore.

Paolo, can we update the signature and clean code path?

Anyhow Gustavo, while well noticed, this is pre-exising and unrelated
to the code movement in this patch.

> 
> 
> Cheers,
> Gustavo
> 
>>   /*
>>    * Append an ID mapping entry as described by "Table 4 ID mapping 
>> format" in
>>    * "IO Remapping Table System Software on ARM Platforms", Chapter 3.
>> @@ -670,7 +677,6 @@ static void
>>   build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState 
>> *vms)
>>   {
>>       int i;
>> -    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>>       const MemMapEntry *memmap = vms->memmap;
>>       AcpiTable table = { .sig = "APIC", .rev = 4, .oem_id = vms->oem_id,
>>                           .oem_table_id = vms->oem_table_id };
>> @@ -741,7 +747,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, 
>> VirtMachineState *vms)
>>                                             
>> memmap[VIRT_HIGH_GIC_REDIST2].size);
>>           }
>> -        if (its_class_name() && !vmc->no_its) {
>> +        if (its_enabled(vms)) {
>>               /*
>>                * ACPI spec, Revision 6.0 Errata A
>>                * (original 6.0 definition has invalid Length)
>> @@ -973,7 +979,7 @@ void virt_acpi_build(VirtMachineState *vms, 
>> AcpiBuildTables *tables)
>>                             vms->oem_table_id);
>>       }
>> -    if (its_class_name() && !vmc->no_its) {
>> +    if (its_enabled(vms)) {
>>           acpi_add_table(table_offsets, tables_blob);
>>           build_iort(tables_blob, tables->linker, vms);
>>       }
>

Hi Phil,

On 4/2/25 07:27, Philippe Mathieu-Daudé wrote:
> On 2/4/25 08:43, Gustavo Romero wrote:
>> Hi Phil,
>>
>> On 3/31/25 19:12, Philippe Mathieu-Daudé wrote:
>>> GIC ITS is checked for the MADT and IORT tables.
>>> Factor the checks out to the its_enabled() helper.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   hw/arm/virt-acpi-build.c | 12 +++++++++---
>>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>> index 3ac8f8e1786..fdc08b40883 100644
>>> --- a/hw/arm/virt-acpi-build.c
>>> +++ b/hw/arm/virt-acpi-build.c
>>> @@ -208,6 +208,13 @@ static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState *vms)
>>>   #define ROOT_COMPLEX_ENTRY_SIZE 36
>>>   #define IORT_NODE_OFFSET 48
>>> +static bool its_enabled(VirtMachineState *vms)
>>> +{
>>> +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>>> +
>>> +    return its_class_name() && !vmc->no_its;
>>> +}
>>> +
>>
>> Isn't its_class_name() always "true"?
> 
> The method signature is described as:
> 
>   /**
>    * its_class_name:
>    *
>    * Return the ITS class name to use depending on whether
>    * KVM acceleration  and KVM CAP_SIGNAL_MSI are supported
>    *
>    * Returns: class name to use or NULL
>    */
>   const char *its_class_name(void);
> 
> So I'd say no.
> 
> Indeed since commit cc5e719e2c8 ("kvm: require KVM_CAP_SIGNAL_MSI")
> the single implementation doesn't return NULL anymore.
> 
> Paolo, can we update the signature and clean code path?

Updating the signature won't solve the redundancy here. Using its_class_name()
for gating the generation of GIC ITS-related ACPI data is still moot.


> Anyhow Gustavo, while well noticed, this is pre-exising and unrelated
> to the code movement in this patch.

hmm I think the fix is kind simple: just remove its_class_name() from
the predicate in its_enabled(). Is that what you meant by "clean code path"
above?

This seems correct to me because we always have ITS present if
(vmc->no_its == false and vms->its == true). If TCG is used and option
its=on is given ITS is created in create_its(). If KVM accel is used
it's as in the commit message from Paolo you pointed out:

     ARM uses it to detect the presence of the ITS emulation in the kernel,
     introduced in Linux 4.8.  **Assume that it's there and possibly fail when
     realizing the arm-its-kvm device.**

So if kernel does not support in-kernel ITS kvm_arm_its_realize() will
bail out with "error creating in-kernel ITS".

It's up to you if you want to fix it in this series or not :)


>>>   /*
>>>    * Append an ID mapping entry as described by "Table 4 ID mapping format" in
>>>    * "IO Remapping Table System Software on ARM Platforms", Chapter 3.
>>> @@ -670,7 +677,6 @@ static void
>>>   build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>>   {
>>>       int i;
>>> -    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>>>       const MemMapEntry *memmap = vms->memmap;
>>>       AcpiTable table = { .sig = "APIC", .rev = 4, .oem_id = vms->oem_id,
>>>                           .oem_table_id = vms->oem_table_id };
>>> @@ -741,7 +747,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>> memmap[VIRT_HIGH_GIC_REDIST2].size);
>>>           }
>>> -        if (its_class_name() && !vmc->no_its) {
>>> +        if (its_enabled(vms)) {
>>>               /*
>>>                * ACPI spec, Revision 6.0 Errata A
>>>                * (original 6.0 definition has invalid Length)
>>> @@ -973,7 +979,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>>>                             vms->oem_table_id);
>>>       }
>>> -    if (its_class_name() && !vmc->no_its) {
>>> +    if (its_enabled(vms)) {
>>>           acpi_add_table(table_offsets, tables_blob);
>>>           build_iort(tables_blob, tables->linker, vms);
>>>       }

I can't see how that's right. Gating IORT table generation entirely based
on the presence of ITS looks wrong because IORT table has data beyond GIC ITS,
like for SMMUv3 etc.. Maybe open an issue to investigate it later?


FWIW,

Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>


Cheers,
Gustavo