* [PATCH] VMD firmware first @ 2018-08-31 0:30 Jon Derrick 2018-08-31 0:30 ` [PATCH] PCI/VMD: Set up firmware first if capable Jon Derrick 0 siblings, 1 reply; 5+ messages in thread From: Jon Derrick @ 2018-08-31 0:30 UTC (permalink / raw) To: Lorenzo Pieralisi; +Cc: Bjorn Helgaas, Keith Busch, linux-pci, Jon Derrick Hi Lorenzo, Keith, I was hoping to wait for the host_bridge->add_device extensions but had to expedite this. Could we see if we can get this into 4.19? Jon Derrick (1): PCI/VMD: Set up firmware first if capable arch/x86/pci/common.c | 17 ++++++++++++++++- drivers/pci/controller/vmd.c | 25 ++++++++++++++++++++++++- 2 files changed, 40 insertions(+), 2 deletions(-) -- 1.8.3.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] PCI/VMD: Set up firmware first if capable 2018-08-31 0:30 [PATCH] VMD firmware first Jon Derrick @ 2018-08-31 0:30 ` Jon Derrick 2018-08-31 14:47 ` Keith Busch 2018-09-04 13:08 ` Bjorn Helgaas 0 siblings, 2 replies; 5+ messages in thread From: Jon Derrick @ 2018-08-31 0:30 UTC (permalink / raw) To: Lorenzo Pieralisi; +Cc: Bjorn Helgaas, Keith Busch, linux-pci, Jon Derrick Some VMD devices will want to use firmware first error-handling on the entire domain. This is detected by the BIOS setting the VMD endpoint's interface to 0x1. Detect this condition and propogate it to the entire domain. Signed-off-by: Jon Derrick <jonathan.derrick@intel.com> --- arch/x86/pci/common.c | 17 ++++++++++++++++- drivers/pci/controller/vmd.c | 25 ++++++++++++++++++++++++- 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c index d4ec117..f07f2e4 100644 --- a/arch/x86/pci/common.c +++ b/arch/x86/pci/common.c @@ -663,8 +663,23 @@ static void set_dma_domain_ops(struct pci_dev *pdev) {} static void set_dev_domain_options(struct pci_dev *pdev) { - if (is_vmd(pdev->bus)) + if (is_vmd(pdev->bus)) { + struct pci_host_bridge *hb; + struct pci_dev *vmd; + pdev->hotplug_user_indicators = 1; + + /* + * The VMD endpoint is not PCIe, so will fail + * pcie_aer_get_firmware_first(). Set and get the raw member + * instead. + */ + hb = pci_find_host_bridge(pdev->bus); + vmd = to_pci_dev(hb->dev.parent); + + pdev->__aer_firmware_first = vmd->__aer_firmware_first; + pdev->__aer_firmware_first_valid = vmd->__aer_firmware_first_valid; + } } int pcibios_add_device(struct pci_dev *dev) diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c index fd2dbd7..74a1a04 100644 --- a/drivers/pci/controller/vmd.c +++ b/drivers/pci/controller/vmd.c @@ -44,6 +44,11 @@ enum vmd_features { * bus numbering */ VMD_FEAT_HAS_BUS_RESTRICTIONS = (1 << 1), + + /* + * Device may request firmware first error-handling on the domain + */ + VMD_FEAT_HAS_FIRMWARE_FIRST = (1 << 2), }; /* @@ -633,6 +638,23 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features) busn_start = 128; } + /* + * Certain VMD devices may request firmware first error-handling + * support on the domain. These domains are virtual and not described + * by ACPI, so we must set it explicitly. This sets firmware first on + * the endpoint and has a corresponding domain setting in + * arch/x86/pci/common.c + */ + if (features & VMD_FEAT_HAS_FIRMWARE_FIRST) { + u8 interface; + + pci_read_config_byte(vmd->dev, PCI_CLASS_PROG, &interface); + if (interface == 0x1) { + vmd->dev->__aer_firmware_first = 1; + vmd->dev->__aer_firmware_first_valid = 1; + } + } + res = &vmd->dev->resource[VMD_CFGBAR]; vmd->resources[0] = (struct resource) { .name = "VMD CFGBAR", @@ -860,7 +882,8 @@ static int vmd_resume(struct device *dev) {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_VMD_201D),}, {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_VMD_28C0), .driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW | - VMD_FEAT_HAS_BUS_RESTRICTIONS,}, + VMD_FEAT_HAS_BUS_RESTRICTIONS | + VMD_FEAT_HAS_FIRMWARE_FIRST,}, {0,} }; MODULE_DEVICE_TABLE(pci, vmd_ids); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] PCI/VMD: Set up firmware first if capable 2018-08-31 0:30 ` [PATCH] PCI/VMD: Set up firmware first if capable Jon Derrick @ 2018-08-31 14:47 ` Keith Busch 2018-09-04 13:08 ` Bjorn Helgaas 1 sibling, 0 replies; 5+ messages in thread From: Keith Busch @ 2018-08-31 14:47 UTC (permalink / raw) To: Jon Derrick; +Cc: Lorenzo Pieralisi, Bjorn Helgaas, linux-pci On Thu, Aug 30, 2018 at 06:30:03PM -0600, Jon Derrick wrote: > Some VMD devices will want to use firmware first error-handling on the > entire domain. This is detected by the BIOS setting the VMD endpoint's > interface to 0x1. > > Detect this condition and propogate it to the entire domain. > > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com> Just curious, do you know what happens if you encounter a fatal error in a VMD domain with FF enabled? Will this create generic hw error source event for the OS, and if so, does the error record point to the VMD device as the source, or the device within the domain that actually sent the ERR_FATAL? The implementation looks fine to me. Reviewed-by: Keith Busch <keith.busch@intel.com> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] PCI/VMD: Set up firmware first if capable 2018-08-31 0:30 ` [PATCH] PCI/VMD: Set up firmware first if capable Jon Derrick 2018-08-31 14:47 ` Keith Busch @ 2018-09-04 13:08 ` Bjorn Helgaas 2018-09-04 23:04 ` Derrick, Jonathan 1 sibling, 1 reply; 5+ messages in thread From: Bjorn Helgaas @ 2018-09-04 13:08 UTC (permalink / raw) To: Jon Derrick; +Cc: Lorenzo Pieralisi, Keith Busch, linux-pci On Thu, Aug 30, 2018 at 06:30:03PM -0600, Jon Derrick wrote: > Some VMD devices will want to use firmware first error-handling on the > entire domain. This is detected by the BIOS setting the VMD endpoint's > interface to 0x1. I assume there's some spec that codifies this "programming interface 0x1 means firmware-first". Can you reference that section of the spec? Even if it's not public, it will help people who maintain this in the future. I also have questions along the lines of Keith's: how are errors reported in the non-firmware-first case, and in the firmware-first case, how are the errors passed on to the OS? > Detect this condition and propogate it to the entire domain. > > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com> > --- > arch/x86/pci/common.c | 17 ++++++++++++++++- > drivers/pci/controller/vmd.c | 25 ++++++++++++++++++++++++- > 2 files changed, 40 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c > index d4ec117..f07f2e4 100644 > --- a/arch/x86/pci/common.c > +++ b/arch/x86/pci/common.c > @@ -663,8 +663,23 @@ static void set_dma_domain_ops(struct pci_dev *pdev) {} > > static void set_dev_domain_options(struct pci_dev *pdev) > { > - if (is_vmd(pdev->bus)) > + if (is_vmd(pdev->bus)) { > + struct pci_host_bridge *hb; > + struct pci_dev *vmd; > + > pdev->hotplug_user_indicators = 1; > + > + /* > + * The VMD endpoint is not PCIe, so will fail > + * pcie_aer_get_firmware_first(). Set and get the raw member > + * instead. > + */ > + hb = pci_find_host_bridge(pdev->bus); > + vmd = to_pci_dev(hb->dev.parent); > + > + pdev->__aer_firmware_first = vmd->__aer_firmware_first; > + pdev->__aer_firmware_first_valid = vmd->__aer_firmware_first_valid; > + } > } > > int pcibios_add_device(struct pci_dev *dev) > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c > index fd2dbd7..74a1a04 100644 > --- a/drivers/pci/controller/vmd.c > +++ b/drivers/pci/controller/vmd.c > @@ -44,6 +44,11 @@ enum vmd_features { > * bus numbering > */ > VMD_FEAT_HAS_BUS_RESTRICTIONS = (1 << 1), > + > + /* > + * Device may request firmware first error-handling on the domain > + */ > + VMD_FEAT_HAS_FIRMWARE_FIRST = (1 << 2), > }; > > /* > @@ -633,6 +638,23 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features) > busn_start = 128; > } > > + /* > + * Certain VMD devices may request firmware first error-handling > + * support on the domain. These domains are virtual and not described > + * by ACPI, so we must set it explicitly. This sets firmware first on > + * the endpoint and has a corresponding domain setting in > + * arch/x86/pci/common.c > + */ > + if (features & VMD_FEAT_HAS_FIRMWARE_FIRST) { > + u8 interface; > + > + pci_read_config_byte(vmd->dev, PCI_CLASS_PROG, &interface); > + if (interface == 0x1) { This test of the programming interface should have a spec reference so it doesn't look magical. > + vmd->dev->__aer_firmware_first = 1; > + vmd->dev->__aer_firmware_first_valid = 1; > + } > + } > + > res = &vmd->dev->resource[VMD_CFGBAR]; > vmd->resources[0] = (struct resource) { > .name = "VMD CFGBAR", > @@ -860,7 +882,8 @@ static int vmd_resume(struct device *dev) > {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_VMD_201D),}, > {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_VMD_28C0), > .driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW | > - VMD_FEAT_HAS_BUS_RESTRICTIONS,}, > + VMD_FEAT_HAS_BUS_RESTRICTIONS | > + VMD_FEAT_HAS_FIRMWARE_FIRST,}, Why is this connected to specific hardware versions? The non-VMD firmware-first functionality is determined by firmware, independent of any particular PCI device. If there's a spec that says "programming interface 0x1 means something other than firmware-first" on some devices, you could cite it here. If early devices are an exception and programming interface will have a consistent meaning in the future, you might be able to invert this so you only have to mark the early devices instead of every new version. > {0,} > }; > MODULE_DEVICE_TABLE(pci, vmd_ids); > -- > 1.8.3.1 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] PCI/VMD: Set up firmware first if capable 2018-09-04 13:08 ` Bjorn Helgaas @ 2018-09-04 23:04 ` Derrick, Jonathan 0 siblings, 0 replies; 5+ messages in thread From: Derrick, Jonathan @ 2018-09-04 23:04 UTC (permalink / raw) To: helgaas@kernel.org Cc: lorenzo.pieralisi@arm.com, Busch, Keith, linux-pci@vger.kernel.org [-- Attachment #1: Type: text/plain, Size: 4735 bytes --] Thanks for the comments. I'll try to get those answers On Tue, 2018-09-04 at 08:08 -0500, Bjorn Helgaas wrote: > On Thu, Aug 30, 2018 at 06:30:03PM -0600, Jon Derrick wrote: > > Some VMD devices will want to use firmware first error-handling on > > the > > entire domain. This is detected by the BIOS setting the VMD > > endpoint's > > interface to 0x1. > > I assume there's some spec that codifies this "programming interface > 0x1 means firmware-first". Can you reference that section of the > spec? Even if it's not public, it will help people who maintain this > in the future. > > I also have questions along the lines of Keith's: how are errors > reported in the non-firmware-first case, and in the firmware-first > case, how are the errors passed on to the OS? > > > Detect this condition and propogate it to the entire domain. > > > > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com> > > --- > > arch/x86/pci/common.c | 17 ++++++++++++++++- > > drivers/pci/controller/vmd.c | 25 ++++++++++++++++++++++++- > > 2 files changed, 40 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c > > index d4ec117..f07f2e4 100644 > > --- a/arch/x86/pci/common.c > > +++ b/arch/x86/pci/common.c > > @@ -663,8 +663,23 @@ static void set_dma_domain_ops(struct pci_dev > > *pdev) {} > > > > static void set_dev_domain_options(struct pci_dev *pdev) > > { > > - if (is_vmd(pdev->bus)) > > + if (is_vmd(pdev->bus)) { > > + struct pci_host_bridge *hb; > > + struct pci_dev *vmd; > > + > > pdev->hotplug_user_indicators = 1; > > + > > + /* > > + * The VMD endpoint is not PCIe, so will fail > > + * pcie_aer_get_firmware_first(). Set and get the > > raw member > > + * instead. > > + */ > > + hb = pci_find_host_bridge(pdev->bus); > > + vmd = to_pci_dev(hb->dev.parent); > > + > > + pdev->__aer_firmware_first = vmd- > > >__aer_firmware_first; > > + pdev->__aer_firmware_first_valid = vmd- > > >__aer_firmware_first_valid; > > + } > > } > > > > int pcibios_add_device(struct pci_dev *dev) > > diff --git a/drivers/pci/controller/vmd.c > > b/drivers/pci/controller/vmd.c > > index fd2dbd7..74a1a04 100644 > > --- a/drivers/pci/controller/vmd.c > > +++ b/drivers/pci/controller/vmd.c > > @@ -44,6 +44,11 @@ enum vmd_features { > > * bus numbering > > */ > > VMD_FEAT_HAS_BUS_RESTRICTIONS = (1 << 1), > > + > > + /* > > + * Device may request firmware first error-handling on the > > domain > > + */ > > + VMD_FEAT_HAS_FIRMWARE_FIRST = (1 << 2), > > }; > > > > /* > > @@ -633,6 +638,23 @@ static int vmd_enable_domain(struct vmd_dev > > *vmd, unsigned long features) > > busn_start = 128; > > } > > > > + /* > > + * Certain VMD devices may request firmware first error- > > handling > > + * support on the domain. These domains are virtual and > > not described > > + * by ACPI, so we must set it explicitly. This sets > > firmware first on > > + * the endpoint and has a corresponding domain setting in > > + * arch/x86/pci/common.c > > + */ > > + if (features & VMD_FEAT_HAS_FIRMWARE_FIRST) { > > + u8 interface; > > + > > + pci_read_config_byte(vmd->dev, PCI_CLASS_PROG, > > &interface); > > + if (interface == 0x1) { > > This test of the programming interface should have a spec reference > so > it doesn't look magical. > > > + vmd->dev->__aer_firmware_first = 1; > > + vmd->dev->__aer_firmware_first_valid = 1; > > + } > > + } > > + > > res = &vmd->dev->resource[VMD_CFGBAR]; > > vmd->resources[0] = (struct resource) { > > .name = "VMD CFGBAR", > > @@ -860,7 +882,8 @@ static int vmd_resume(struct device *dev) > > {PCI_DEVICE(PCI_VENDOR_ID_INTEL, > > PCI_DEVICE_ID_INTEL_VMD_201D),}, > > {PCI_DEVICE(PCI_VENDOR_ID_INTEL, > > PCI_DEVICE_ID_INTEL_VMD_28C0), > > .driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW | > > - VMD_FEAT_HAS_BUS_RESTRICTIONS,}, > > + VMD_FEAT_HAS_BUS_RESTRICTIONS | > > + VMD_FEAT_HAS_FIRMWARE_FIRST,}, > > Why is this connected to specific hardware versions? The non-VMD > firmware-first functionality is determined by firmware, independent > of > any particular PCI device. > > If there's a spec that says "programming interface 0x1 means > something > other than firmware-first" on some devices, you could cite it here. > If early devices are an exception and programming interface will have > a consistent meaning in the future, you might be able to invert this > so you only have to mark the early devices instead of every new > version. > > > {0,} > > }; > > MODULE_DEVICE_TABLE(pci, vmd_ids); > > -- > > 1.8.3.1 > > [-- Attachment #2: smime.p7s --] [-- Type: application/x-pkcs7-signature, Size: 3278 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-09-04 23:04 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-08-31 0:30 [PATCH] VMD firmware first Jon Derrick 2018-08-31 0:30 ` [PATCH] PCI/VMD: Set up firmware first if capable Jon Derrick 2018-08-31 14:47 ` Keith Busch 2018-09-04 13:08 ` Bjorn Helgaas 2018-09-04 23:04 ` Derrick, Jonathan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).