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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 19/05/2026 7:02 am, Jan Beulich wrote:
> 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?

With an API like this, the bus number does not need passing back.  The
caller just accesses pci_mmcfg_base(sbdf) + reg (after the NULL check of
course).

Also, I'm pretty sure that it will be cleaner to merge the two functions
than to leave them split.

>
>>> 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.

I did see you elsewhere on the PR which merged this, but not on this
patch specifically.

I have it on good authority that AMD CPUs can't trap MMIO into SMM. 
(I'm not aware of Intel CPUs being able to trap MMIO like this either,
whereas both Intel and AMD explicitly can trap IO ports into SMM.)

Hence the aformentioned enquiries.
>>> +        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?

Despite returning -EINVAL, the single callers don't look at the return
value, and only take the *value value, which is generally -1.

For a bad length, 0 is returned (indistinguishable from success if
anyone were to care) and *value is left uninitialised.

So yes, -1 on the failsafe real error path is fine; after all it can
occur for many other non-error reasons too in PCI Config Space accesses.

~Andrew