Re: [PATCH v2] x86/PCI: Prefer MMIO over PIO on all hypervisor

[Ajay Kaher]

Note: Corrected the Subject.

> On 07/09/22, 8:50 PM, "Vitaly Kuznetsov" <vkuznets@xxxxxxxxxx> wrote:
>
>> diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
>> index ddb7986..1e5a8f7 100644
>> --- a/arch/x86/pci/common.c
>> +++ b/arch/x86/pci/common.c
>> @@ -20,6 +20,7 @@
>>  #include <asm/pci_x86.h>
>>  #include <asm/setup.h>
>>  #include <asm/irqdomain.h>
>> +#include <asm/hypervisor.h>
>>
>>  unsigned int pci_probe = PCI_PROBE_BIOS | PCI_PROBE_CONF1 | PCI_PROBE_CONF2 |
>>                               PCI_PROBE_MMCONF;
>> @@ -57,14 +58,58 @@ int raw_pci_write(unsigned int domain, unsigned int bus, unsigned int devfn,
>>       return -EINVAL;
>>  }
>>
>> +#ifdef CONFIG_HYPERVISOR_GUEST
>> +static int vm_raw_pci_read(unsigned int domain, unsigned int bus, unsigned int devfn,
>> +                                             int reg, int len, u32 *val)
>> +{
>> +     if (raw_pci_ext_ops)
>> +             return raw_pci_ext_ops->read(domain, bus, devfn, reg, len, val);
>> +     if (domain == 0 && reg < 256 && raw_pci_ops)
>> +             return raw_pci_ops->read(domain, bus, devfn, reg, len, val);
>> +     return -EINVAL;
>> +}
>> +
>> +static int vm_raw_pci_write(unsigned int domain, unsigned int bus, unsigned int devfn,
>> +                                             int reg, int len, u32 val)
>> +{
>> +     if (raw_pci_ext_ops)
>> +             return raw_pci_ext_ops->write(domain, bus, devfn, reg, len, val);
>> +     if (domain == 0 && reg < 256 && raw_pci_ops)
>> +             return raw_pci_ops->write(domain, bus, devfn, reg, len, val);
>> +     return -EINVAL;
>> +}
>
> These look exactly like raw_pci_read()/raw_pci_write() but with inverted
> priority. We could've added a parameter but to be more flexible, I'd
> suggest we add a 'priority' field to 'struct pci_raw_ops' and make
> raw_pci_read()/raw_pci_write() check it before deciding what to use
> first. To be on the safe side, you can leave raw_pci_ops's priority
> higher than raw_pci_ext_ops's by default and only tweak it in
> arch/x86/kernel/cpu/vmware.c

Thanks Vitaly for your response.

1. we have multiple objects of struct pci_raw_ops, 2. adding 'priority' field to struct pci_raw_ops
doesn't seems to be appropriate as need to take decision which object of struct pci_raw_ops has
to be used, not something with-in struct pci_raw_ops.

It's a generic solution for all hypervisor (sorry for earlier wrong Subject), not specific to VMware.

Further looking for feedback if it's impacting to any hypervisor.

-Ajay