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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.04.2025 21:19, Andrii Sultanov wrote:
> @@ -748,6 +746,7 @@ static u16 __init parse_ivhd_device_special(
>  {
>      u16 dev_length, bdf;
>      unsigned int apic, idx;
> +    pci_sbdf_t sbdf;

Why does bdf need keeping around here?

> @@ -335,20 +336,18 @@ void cf_check amd_iommu_ioapic_update_ire(
>      new_rte.raw = rte;
>  
>      /* get device id of ioapic devices */
> -    bdf = ioapic_sbdf[idx].bdf;
> -    seg = ioapic_sbdf[idx].seg;
> -    iommu = find_iommu_for_device(PCI_SBDF(seg, bdf));
> +    sbdf = ioapic_sbdf[idx].sbdf;
> +    iommu = find_iommu_for_device(sbdf);
>      if ( !iommu )
>      {
> -        AMD_IOMMU_WARN("failed to find IOMMU for IO-APIC @ %04x:%04x\n",
> -                       seg, bdf);
> +        AMD_IOMMU_WARN("failed to find IOMMU for IO-APIC @ %pp\n", &sbdf);

Hmm, this one's somewhat questionable: An IO-APIC isn't a PCI device, so
making its "coordinates" look like one can be confusing.

> @@ -369,7 +368,8 @@ unsigned int cf_check amd_iommu_read_ioapic_from_ire(
>      unsigned int offset;
>      unsigned int val = __io_apic_read(apic, reg);
>      unsigned int pin = (reg - 0x10) / 2;
> -    uint16_t seg, bdf, req_id;
> +    pci_sbdf_t sbdf;
> +    uint16_t req_id;
>      const struct amd_iommu *iommu;
>      union irte_ptr entry;
>  
> @@ -381,12 +381,11 @@ unsigned int cf_check amd_iommu_read_ioapic_from_ire(
>      if ( offset >= INTREMAP_MAX_ENTRIES )
>          return val;
>  
> -    seg = ioapic_sbdf[idx].seg;
> -    bdf = ioapic_sbdf[idx].bdf;
> -    iommu = find_iommu_for_device(PCI_SBDF(seg, bdf));
> +    sbdf = ioapic_sbdf[idx].sbdf;
> +    iommu = find_iommu_for_device(sbdf);
>      if ( !iommu )
>          return val;
> -    req_id = get_intremap_requestor_id(seg, bdf);
> +    req_id = get_intremap_requestor_id(sbdf.seg, sbdf.bdf);
>      entry = get_intremap_entry(iommu, req_id, offset);
>  
>      if ( !(reg & 1) )

Is a local variable here warranted in the first place?

Jan