Andy Shevchenko <andy.shevchenko@xxxxxxxxx> writes: > On Thu, Jul 30, 2020 at 10:37 PM Julia Suvorova <jusual@xxxxxxxxxx> wrote: >> >> Using MMCONFIG instead of I/O ports cuts the number of config space >> accesses in half, which is faster on KVM and opens the door for >> additional optimizations such as Vitaly's "[PATCH 0/3] KVM: x86: KVM >> MEM_PCI_HOLE memory": > >> https://lore.kernel.org/kvm/20200728143741.2718593-1-vkuznets@xxxxxxxxxx > > You may use Link: tag for this. > >> However, this change will not bring significant performance improvement >> unless it is running on x86 within a hypervisor. Moreover, allowing >> MMCONFIG access for addresses < 256 can be dangerous for some devices: >> see commit a0ca99096094 ("PCI x86: always use conf1 to access config >> space below 256 bytes"). That is why a special feature flag is needed. >> >> Introduce KVM_FEATURE_PCI_GO_MMCONFIG, which can be enabled when the >> configuration is known to be safe (e.g. in QEMU). > > ... > >> +static int __init kvm_pci_arch_init(void) >> +{ >> + if (raw_pci_ext_ops && >> + kvm_para_has_feature(KVM_FEATURE_PCI_GO_MMCONFIG)) { > > Better to use traditional pattern, i.e. > if (not_supported) > return bail_out; > > ...do useful things... > return 0; > >> + pr_info("PCI: Using MMCONFIG for base access\n"); >> + raw_pci_ops = raw_pci_ext_ops; >> + return 0; >> + } > >> + return 1; > > Hmm... I don't remember what positive codes means there. Perhaps you > need to return a rather error code? If I'm reading the code correctly, pci_arch_init() has the following: if (x86_init.pci.arch_init && !x86_init.pci.arch_init()) return 0; so returning '1' here means 'continue' and this seems to be correct. (E.g. Hyper-V's hv_pci_init() does the same). What I'm not sure about is 'return 0' above as this will result in skipping the rest of pci_arch_init(). Was this desired or should we return '1' in both cases? -- Vitaly
