Re: [PATCH v5 3/3] drivers: Make ioapic_sbdf and hpet_sbdf contain pci_sbdf_t

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.07.2025 09:31, Andrii Sultanov wrote:
> Following a similar change to amd_iommu struct, make two more structs
> take pci_sbdf_t directly instead of seg and bdf separately. This lets us
> drop several conversions from the latter to the former and simplifies
> several comparisons and assignments.
> 
> Bloat-o-meter reports:
> add/remove: 0/0 grow/shrink: 1/10 up/down: 256/-320 (-64)
> Function                                     old     new   delta
> _einittext                                 22092   22348    +256
> parse_ivrs_hpet                              248     245      -3
> amd_iommu_detect_one_acpi                    876     868      -8
> iov_supports_xt                              275     264     -11
> amd_iommu_read_ioapic_from_ire               344     332     -12
> amd_setup_hpet_msi                           237     224     -13
> amd_iommu_ioapic_update_ire                  575     555     -20
> reserve_unity_map_for_device                 453     424     -29
> _hvm_dpci_msi_eoi                            160     128     -32
> amd_iommu_get_supported_ivhd_type             86      30     -56
> parse_ivrs_table                            3966    3830    -136
> 
> Signed-off-by: Andrii Sultanov <sultanovandriy@xxxxxxxxx>

Here the unspecific subject prefix is actually a problem: It remains unclear
what component(s) is (are) being altered.

> @@ -746,8 +744,9 @@ static u16 __init parse_ivhd_device_special(
>      const struct acpi_ivrs_device8c *special, u16 seg,
>      u16 header_length, u16 block_length, struct amd_iommu *iommu)
>  {
> -    u16 dev_length, bdf;
> +    u16 dev_length;

Nit: Preferably switch to uint16_t while touching such a line.

With the adjustments (which can be done while committing, I guess):
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

> @@ -780,8 +779,7 @@ static u16 __init parse_ivhd_device_special(
>           */
>          for ( idx = 0; idx < nr_ioapic_sbdf; idx++ )
>          {
> -            if ( ioapic_sbdf[idx].bdf == bdf &&
> -                 ioapic_sbdf[idx].seg == seg &&
> +            if ( ioapic_sbdf[idx].sbdf.sbdf == sbdf.sbdf &&

Just to mention - there are of course a lot of "sbdf" on lines like this now.
We may want to reconsider naming of the two global variables (not here, but
in general).

Jan