* [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; +Cc: lorenzo.pieralisi, Busch, Keith, linux-pci
[-- 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.