Re: [PATCH 5/5] RFC: pci: Migrate pci_mmcfg_{read,write} to pci.c

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.05.2026 19:35, Andrew Cooper wrote:
> On 18/05/2026 4:21 pm, Teddy Astie wrote:
>> --- a/xen/arch/x86/x86_64/mmconfig_64.c
>> +++ b/xen/arch/x86/x86_64/mmconfig_64.c
>> @@ -133,6 +46,25 @@ static void __iomem *mcfg_ioremap(const struct 
>> acpi_mcfg_allocation *cfg,
>>      return (void __iomem *) virt;
>>  }
>>  
>> +char __iomem *pci_mmcfg_base(unsigned int seg, unsigned int *bus)
>> +{
>> +    struct acpi_mcfg_allocation *cfg;
>> +    int cfg_num;
>> +
>> +    for (cfg_num = 0; cfg_num < pci_mmcfg_config_num; cfg_num++) {
>> +        cfg = pci_mmcfg_virt[cfg_num].cfg;
>> +        if (cfg->pci_segment == seg &&
>> +            (cfg->start_bus_number <= *bus) &&
>> +            (cfg->end_bus_number >= *bus)) {
>> +            *bus -= cfg->start_bus_number;
>> +            return pci_mmcfg_virt[cfg_num].virt;
>> +        }
>> +    }
>> +
>> +    /* Fall back to type 0 */
>> +    return NULL;
>> +}
> 
> This is a horrid function.  Accessing and modifying bus like that causes
> poor code generation, and by now having this in a separate translation
> unit, the optimiser can't fold it into it's single caller and undo the
> poor decisions which went into writing this function.
> 
> Instead, you want:
> 
> void __iomem *pci_mmcfg_base(pci_sbdf_t sbdf)
> {
>     ...
> }
> 
> base which takes care of the bus adjustment internally.

If the updated bus number need passing back to the caller, what do you
mean by this? With two values to pass back, and without resorting to
returning a larger struct by value, one pointer parameter is going to
be needed, isn't it?

>> diff --git a/xen/arch/x86/x86_64/pci.c b/xen/arch/x86/x86_64/pci.c
>> index 8d33429103..c37e3edade 100644
>> --- a/xen/arch/x86/x86_64/pci.c
>> +++ b/xen/arch/x86/x86_64/pci.c
>> @@ -11,13 +11,123 @@
>>  #define PCI_CONF_ADDRESS(sbdf, reg) \
>>      (0x80000000U | ((sbdf).bdf << 8) | ((reg) & ~3))
>>  
>> +/*
>> + * AMD Fam10h CPUs are buggy, and cannot access MMIO config space
>> + * on their northbrige except through the * %eax register. As such, you MUST
>> + * NOT use normal IOMEM accesses, you need to only use the magic mmio-config
>> + * accessor functions.
>> + * In fact just use pci_config_*, nothing else please.
> 
> I know you're just copying an existing comment, but it's mostly an
> opinion and not terribly helpful in place.
> 
> "AMD Fam10h CPUs can only make MMCFG accesses via MOV %eax/%ax/%al",
> would be better, except...
> 
> ... this claim cannot be true.  It's been made since the K8 RevF BKWG
> and exists even into the latest PPRs, but that's simply not how
> load/store ops work in the pipeline.

How do you know what special casing there exists (or has existed), or
what (e.g.) bogus(?) SMM intercepts there may be? I'm pretty sure the
Linux change was in response to things indeed not working otherwise.

>> +static inline
>> +int pci_mmcfg_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int len, u32 
>> *value)
>> +{
>> +    char __iomem *addr;
>> +
>> +    /* Why do we have this when nobody checks it. How about a BUG()!? -AK */
>> +    if (unlikely(reg > 4095)) {
>> +err:        *value = -1;

Nit: Style. Yet as the comment suggests: Perhaps time to drop or replace
by ASSERT() / BUG_ON()?

>> +        return -EINVAL;
>> +    }
>> +
>> +    addr = pci_dev_base(sbdf.seg, sbdf.bus, sbdf.devfn);
>> +    if (!addr)
>> +        goto err;
>> +
>> +    switch (len) {
>> +    case 1:
>> +        *value = mmio_config_readb(addr + reg);
>> +        break;
>> +    case 2:
>> +        *value = mmio_config_readw(addr + reg);
>> +        break;
>> +    case 4:
>> +        *value = mmio_config_readl(addr + reg);
>> +        break;
>> +    }
>> +
>> +    return 0;
>> +}
> 
> Again, for this patch or a later cleanup, drop the output-by-pointer and
> return value directly.  The optimiser is hopefully doing this already
> but it also makes the function simpler.
> 
> At best, we want ASSERT_UNREACHBLE()'s in the error paths (including no
> mapping), and to consistently return -1.  Returning 0 for a bad length
> is bogus.

This looks to contradict the earlier paragraph: Do you want to return the
value, or do you want to return a success indicator?

Jan