| Message ID | 20250110140152.27624-3-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: fix usage of devices behind a VMD bridge | expand |
Match historical subject line style for prefix and capitalization: PCI: vmd: Set devices to D0 before enabling PM L1 Substates PCI: vmd: Add DID 8086:B06F and 8086:B60B for Intel client SKUs PCI: vmd: Fix indentation issue in vmd_shutdown() On Fri, Jan 10, 2025 at 03:01:49PM +0100, Roger Pau Monne wrote: > MSI remapping bypass (directly configuring MSI entries for devices on the VMD > bus) won't work under Xen, as Xen is not aware of devices in such bus, and > hence cannot configure the entries using the pIRQ interface in the PV case, and > in the PVH case traps won't be setup for MSI entries for such devices. > > Until Xen is aware of devices in the VMD bus prevent the > VMD_FEAT_CAN_BYPASS_MSI_REMAP capability from being used when running as any > kind of Xen guest. Wrap to fit in 75 columns. Can you include a hint about *why* Xen is not aware of devices below VMD? That will help to know whether it's a permanent unfixable situation or something that could be done eventually. > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > drivers/pci/controller/vmd.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c > index 264a180403a0..d9b7510ace29 100644 > --- a/drivers/pci/controller/vmd.c > +++ b/drivers/pci/controller/vmd.c > @@ -965,6 +965,15 @@ static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id) > struct vmd_dev *vmd; > int err; > > + if (xen_domain()) > + /* > + * Xen doesn't have knowledge about devices in the VMD bus. Also here. > + * Bypass of MSI remapping won't work in that case as direct > + * write to the MSI entries won't result in functional > + * interrupts. > + */ > + features &= ~VMD_FEAT_CAN_BYPASS_MSI_REMAP; > + > if (resource_size(&dev->resource[VMD_CFGBAR]) < (1 << 20)) > return -ENOMEM; > > -- > 2.46.0 >
Hi Bjorn, On 1/10/25 3:25 PM, Bjorn Helgaas wrote: > Match historical subject line style for prefix and capitalization: > > PCI: vmd: Set devices to D0 before enabling PM L1 Substates > PCI: vmd: Add DID 8086:B06F and 8086:B60B for Intel client SKUs > PCI: vmd: Fix indentation issue in vmd_shutdown() > > On Fri, Jan 10, 2025 at 03:01:49PM +0100, Roger Pau Monne wrote: >> MSI remapping bypass (directly configuring MSI entries for devices on the VMD >> bus) won't work under Xen, as Xen is not aware of devices in such bus, and >> hence cannot configure the entries using the pIRQ interface in the PV case, and >> in the PVH case traps won't be setup for MSI entries for such devices. >> >> Until Xen is aware of devices in the VMD bus prevent the >> VMD_FEAT_CAN_BYPASS_MSI_REMAP capability from being used when running as any >> kind of Xen guest. > > Wrap to fit in 75 columns. > > Can you include a hint about *why* Xen is not aware of devices below > VMD? That will help to know whether it's a permanent unfixable > situation or something that could be done eventually. > I wasn't aware of the Xen issue with VMD but if I had to guess it's probably due to the special handling of the downstream device into the dmar table. [1] 4fda230ecddc2 [2] 9b4a824b889e1 >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >> --- >> drivers/pci/controller/vmd.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c >> index 264a180403a0..d9b7510ace29 100644 >> --- a/drivers/pci/controller/vmd.c >> +++ b/drivers/pci/controller/vmd.c >> @@ -965,6 +965,15 @@ static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id) >> struct vmd_dev *vmd; >> int err; >> >> + if (xen_domain()) >> + /* >> + * Xen doesn't have knowledge about devices in the VMD bus. > > Also here. > >> + * Bypass of MSI remapping won't work in that case as direct >> + * write to the MSI entries won't result in functional >> + * interrupts. >> + */ >> + features &= ~VMD_FEAT_CAN_BYPASS_MSI_REMAP; >> + >> if (resource_size(&dev->resource[VMD_CFGBAR]) < (1 << 20)) >> return -ENOMEM; >> >> -- >> 2.46.0 >>
Hi Roger, kernel test robot noticed the following build errors: [auto build test ERROR on pci/next] [also build test ERROR on pci/for-linus tip/irq/core linus/master v6.13-rc6 next-20250110] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Roger-Pau-Monne/xen-pci-do-not-register-devices-outside-of-PCI-segment-scope/20250110-220331 base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next patch link: https://lore.kernel.org/r/20250110140152.27624-3-roger.pau%40citrix.com patch subject: [PATCH 2/3] vmd: disable MSI remapping bypass under Xen config: x86_64-buildonly-randconfig-001-20250112 (https://download.01.org/0day-ci/archive/20250112/202501121029.dJk0TBPr-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250112/202501121029.dJk0TBPr-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202501121029.dJk0TBPr-lkp@intel.com/ All errors (new ones prefixed by >>): drivers/pci/controller/vmd.c: In function 'vmd_probe': >> drivers/pci/controller/vmd.c:973:13: error: implicit declaration of function 'xen_domain' [-Werror=implicit-function-declaration] 973 | if (xen_domain()) | ^~~~~~~~~~ cc1: some warnings being treated as errors vim +/xen_domain +973 drivers/pci/controller/vmd.c 966 967 static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id) 968 { 969 unsigned long features = (unsigned long) id->driver_data; 970 struct vmd_dev *vmd; 971 int err; 972 > 973 if (xen_domain()) 974 /* 975 * Xen doesn't have knowledge about devices in the VMD bus. 976 * Bypass of MSI remapping won't work in that case as direct 977 * write to the MSI entries won't result in functional 978 * interrupts. 979 */ 980 features &= ~VMD_FEAT_CAN_BYPASS_MSI_REMAP; 981 982 if (resource_size(&dev->resource[VMD_CFGBAR]) < (1 << 20)) 983 return -ENOMEM; 984 985 vmd = devm_kzalloc(&dev->dev, sizeof(*vmd), GFP_KERNEL); 986 if (!vmd) 987 return -ENOMEM; 988 989 vmd->dev = dev; 990 vmd->instance = ida_alloc(&vmd_instance_ida, GFP_KERNEL); 991 if (vmd->instance < 0) 992 return vmd->instance; 993 994 vmd->name = devm_kasprintf(&dev->dev, GFP_KERNEL, "vmd%d", 995 vmd->instance); 996 if (!vmd->name) { 997 err = -ENOMEM; 998 goto out_release_instance; 999 } 1000 1001 err = pcim_enable_device(dev); 1002 if (err < 0) 1003 goto out_release_instance; 1004 1005 vmd->cfgbar = pcim_iomap(dev, VMD_CFGBAR, 0); 1006 if (!vmd->cfgbar) { 1007 err = -ENOMEM; 1008 goto out_release_instance; 1009 } 1010 1011 pci_set_master(dev); 1012 if (dma_set_mask_and_coherent(&dev->dev, DMA_BIT_MASK(64)) && 1013 dma_set_mask_and_coherent(&dev->dev, DMA_BIT_MASK(32))) { 1014 err = -ENODEV; 1015 goto out_release_instance; 1016 } 1017 1018 if (features & VMD_FEAT_OFFSET_FIRST_VECTOR) 1019 vmd->first_vec = 1; 1020 1021 spin_lock_init(&vmd->cfg_lock); 1022 pci_set_drvdata(dev, vmd); 1023 err = vmd_enable_domain(vmd, features); 1024 if (err) 1025 goto out_release_instance; 1026 1027 dev_info(&vmd->dev->dev, "Bound to PCI domain %04x\n", 1028 vmd->sysdata.domain); 1029 return 0; 1030 1031 out_release_instance: 1032 ida_free(&vmd_instance_ida, vmd->instance); 1033 return err; 1034 } 1035
On Fri, Jan 10, 2025 at 04:25:25PM -0600, Bjorn Helgaas wrote: > Match historical subject line style for prefix and capitalization: > > PCI: vmd: Set devices to D0 before enabling PM L1 Substates > PCI: vmd: Add DID 8086:B06F and 8086:B60B for Intel client SKUs > PCI: vmd: Fix indentation issue in vmd_shutdown() > > On Fri, Jan 10, 2025 at 03:01:49PM +0100, Roger Pau Monne wrote: > > MSI remapping bypass (directly configuring MSI entries for devices on the VMD > > bus) won't work under Xen, as Xen is not aware of devices in such bus, and > > hence cannot configure the entries using the pIRQ interface in the PV case, and > > in the PVH case traps won't be setup for MSI entries for such devices. > > > > Until Xen is aware of devices in the VMD bus prevent the > > VMD_FEAT_CAN_BYPASS_MSI_REMAP capability from being used when running as any > > kind of Xen guest. > > Wrap to fit in 75 columns. Hm, OK, but isn't the limit 80 columns according to the kernel coding style (Documentation/process/coding-style.rst)? I don't mind adjusting, but if you are going to ask every submitter to limit to 75 columns then the coding style document should be updated to reflect that. > Can you include a hint about *why* Xen is not aware of devices below > VMD? That will help to know whether it's a permanent unfixable > situation or something that could be done eventually. Xen would need to be made aware of the devices exposed behind the VMD bridge, so it can manage them. For example Xen is the entity that controls the local APICs, and hence interrupts must be configured by Xen. Xen needs knowledge about the devices behind the VMD bridge, and how to access those devices PCI config space to at least configure MSI or MSI-X capabilities. It could possibly be exposed similarly to how Xen currently deals with ECAM areas. None of this is present at the moment, could always be added later and Linux be made aware that the limitation no longer applies. That would require changes in both Xen and Linux to propagate the VMD information into Xen. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > --- > > drivers/pci/controller/vmd.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c > > index 264a180403a0..d9b7510ace29 100644 > > --- a/drivers/pci/controller/vmd.c > > +++ b/drivers/pci/controller/vmd.c > > @@ -965,6 +965,15 @@ static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id) > > struct vmd_dev *vmd; > > int err; > > > > + if (xen_domain()) > > + /* > > + * Xen doesn't have knowledge about devices in the VMD bus. > > Also here. Would you be OK with something like: "Xen doesn't have knowledge about devices in the VMD bus because the config space of devices behind the VMD bridge is not known to Xen, and hence Xen cannot discover or configure them in any way. Bypass of MSI remapping won't work in that case as direct write by Linux to the MSI entries won't result in functional interrupts, as it's Xen the entity that manages the local APIC and must configure interrupts." Thanks, Roger.
On Fri, Jan 10, 2025 at 10:02:00PM -0700, Jonathan Derrick wrote: > Hi Bjorn, > > On 1/10/25 3:25 PM, Bjorn Helgaas wrote: > > Match historical subject line style for prefix and capitalization: > > > > PCI: vmd: Set devices to D0 before enabling PM L1 Substates > > PCI: vmd: Add DID 8086:B06F and 8086:B60B for Intel client SKUs > > PCI: vmd: Fix indentation issue in vmd_shutdown() > > > > On Fri, Jan 10, 2025 at 03:01:49PM +0100, Roger Pau Monne wrote: > > > MSI remapping bypass (directly configuring MSI entries for devices on the VMD > > > bus) won't work under Xen, as Xen is not aware of devices in such bus, and > > > hence cannot configure the entries using the pIRQ interface in the PV case, and > > > in the PVH case traps won't be setup for MSI entries for such devices. > > > > > > Until Xen is aware of devices in the VMD bus prevent the > > > VMD_FEAT_CAN_BYPASS_MSI_REMAP capability from being used when running as any > > > kind of Xen guest. > > > > Wrap to fit in 75 columns. > > > > Can you include a hint about *why* Xen is not aware of devices below > > VMD? That will help to know whether it's a permanent unfixable > > situation or something that could be done eventually. > > > I wasn't aware of the Xen issue with VMD but if I had to guess it's probably > due to the special handling of the downstream device into the dmar table. Nothing to do with DMAR or IOMMUs, it's just that on a Xen system it must be Xen the one that configures the MSI entries, and that requires Xen being aware of the VMD devices and it's MSI or MSI-X capabilities. None of this is currently done, as Xen has no visibility at all of devices behind a VMD bridge because is doesn't even know about VMD bridges, neither about the exposed ECAM-like region on those devices. Thanks, Roger.
On Mon, Jan 13, 2025 at 11:03:58AM +0100, Roger Pau Monné wrote: > > Hm, OK, but isn't the limit 80 columns according to the kernel coding > style (Documentation/process/coding-style.rst)? That's the coding style. The commit message style is described in a different doc: https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format
On Mon, Jan 13, 2025 at 08:11:19AM -0700, Keith Busch wrote: > On Mon, Jan 13, 2025 at 11:03:58AM +0100, Roger Pau Monné wrote: > > > > Hm, OK, but isn't the limit 80 columns according to the kernel coding > > style (Documentation/process/coding-style.rst)? > > That's the coding style. The commit message style is described in a > different doc: > > https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format It's quite confusing to have different line length for code vs commit messages, but my fault for not reading those. Will adjust to 75 columns them. Regards, Roger.
On Mon, Jan 13, 2025 at 05:45:20PM +0100, Roger Pau Monné wrote: > On Mon, Jan 13, 2025 at 08:11:19AM -0700, Keith Busch wrote: > > On Mon, Jan 13, 2025 at 11:03:58AM +0100, Roger Pau Monné wrote: > > > > > > Hm, OK, but isn't the limit 80 columns according to the kernel coding > > > style (Documentation/process/coding-style.rst)? > > > > That's the coding style. The commit message style is described in a > > different doc: > > > > https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format > > It's quite confusing to have different line length for code vs commit > messages, but my fault for not reading those. Will adjust to 75 > columns them. The output of 'git log' prepends spaces to the subject and body of the listed commits. The lower limit for commit messages vs. code makes the change log look readable in an 80-char terminal.
On Mon, Jan 13, 2025 at 09:53:21AM -0700, Keith Busch wrote: > On Mon, Jan 13, 2025 at 05:45:20PM +0100, Roger Pau Monné wrote: > > On Mon, Jan 13, 2025 at 08:11:19AM -0700, Keith Busch wrote: > > > On Mon, Jan 13, 2025 at 11:03:58AM +0100, Roger Pau Monné wrote: > > > > > > > > Hm, OK, but isn't the limit 80 columns according to the kernel coding > > > > style (Documentation/process/coding-style.rst)? > > > > > > That's the coding style. The commit message style is described in a > > > different doc: > > > > > > https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format > > > > It's quite confusing to have different line length for code vs commit > > messages, but my fault for not reading those. Will adjust to 75 > > columns them. > > The output of 'git log' prepends spaces to the subject and body of the > listed commits. The lower limit for commit messages vs. code makes the > change log look readable in an 80-char terminal. Oh, I see, thanks for explaining. The 80 column limit for code mean the resulting diff (and `git log` output) could have a maximum width of 81 characters (because of the prepended +,-, ), but since Linux uses hard tabs for indentation this is not really an issue as it would be if using spaces. Regards, Roger.
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c index 264a180403a0..d9b7510ace29 100644 --- a/drivers/pci/controller/vmd.c +++ b/drivers/pci/controller/vmd.c @@ -965,6 +965,15 @@ static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id) struct vmd_dev *vmd; int err; + if (xen_domain()) + /* + * Xen doesn't have knowledge about devices in the VMD bus. + * Bypass of MSI remapping won't work in that case as direct + * write to the MSI entries won't result in functional + * interrupts. + */ + features &= ~VMD_FEAT_CAN_BYPASS_MSI_REMAP; + if (resource_size(&dev->resource[VMD_CFGBAR]) < (1 << 20)) return -ENOMEM;
MSI remapping bypass (directly configuring MSI entries for devices on the VMD bus) won't work under Xen, as Xen is not aware of devices in such bus, and hence cannot configure the entries using the pIRQ interface in the PV case, and in the PVH case traps won't be setup for MSI entries for such devices. Until Xen is aware of devices in the VMD bus prevent the VMD_FEAT_CAN_BYPASS_MSI_REMAP capability from being used when running as any kind of Xen guest. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- drivers/pci/controller/vmd.c | 9 +++++++++ 1 file changed, 9 insertions(+)