[RFC PATCH v3 0/2] Add platform check for subdevice irq domain

Hi,

Learnt from the discussions in this thread:

https://lore.kernel.org/linux-pci/160408357912.912050.17005584526266191420.stgit@djiang5-desk3.ch.intel.com/

The device IMS (Interrupt Message Storage) should not be enabled in any
virtualization environments unless there is a HYPERCALL domain which
makes the changes in the message store monitored by the hypervisor.

As the initial step, we allow the IMS to be enabled only if we are
running on the bare metal. It's easy to enable IMS in the virtualization
environments if above preconditions are met in the future.

This series is only for comments purpose. We will include it in the Intel
IMS implementation later once we reach a consensus.

Change log:
v2->v3:
 - v2:
   https://lore.kernel.org/linux-pci/20210106022749.2769057-1-baolu.lu@linux.intel.com/
 - Add all identified heuristics so far.

v1->v2:
 - v1:
   https://lore.kernel.org/linux-pci/20201210004624.345282-1-baolu.lu@linux.intel.com/
 - Rename probably_on_bare_metal() with on_bare_metal();
 - Some vendors might use the same name for both bare metal and virtual
   environment. Before we add vendor specific code to distinguish
   between them, let's return false in on_bare_metal(). This won't
   introduce any regression. The only impact is that the coming new
   platform msi feature won't be supported until the vendor specific code
   is provided.

Best regards,
baolu

Lu Baolu (2):
  iommu: Add capability IOMMU_CAP_VIOMMU
  platform-msi: Add platform check for subdevice irq domain

 arch/x86/pci/common.c        | 71 ++++++++++++++++++++++++++++++++++++
 drivers/base/platform-msi.c  |  8 ++++
 drivers/iommu/intel/iommu.c  | 20 ++++++++++
 drivers/iommu/virtio-iommu.c |  9 +++++
 include/linux/iommu.h        |  1 +
 include/linux/msi.h          |  1 +
 6 files changed, 110 insertions(+)

-- 
2.25.1

Some vendor IOMMU drivers are able to declare that it is running in a VM
context. This is very valuable for the features that only want to be
supported on bare metal. Add a capability bit so that it could be used.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel/iommu.c  | 20 ++++++++++++++++++++
 drivers/iommu/virtio-iommu.c |  9 +++++++++
 include/linux/iommu.h        |  1 +
 3 files changed, 30 insertions(+)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index cb205a04fe4c..8eb022d0e8aa 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c

@@ -5738,12 +5738,32 @@ static inline bool nested_mode_support(void)
 	return ret;
 }
 
+static inline bool caching_mode_enabled(void)
+{
+	struct dmar_drhd_unit *drhd;
+	struct intel_iommu *iommu;
+	bool ret = false;
+
+	rcu_read_lock();
+	for_each_active_iommu(iommu, drhd) {
+		if (cap_caching_mode(iommu->cap)) {
+			ret = true;
+			break;
+		}
+	}
+	rcu_read_unlock();
+
+	return ret;
+}
+
 static bool intel_iommu_capable(enum iommu_cap cap)
 {
 	if (cap == IOMMU_CAP_CACHE_COHERENCY)
 		return domain_update_iommu_snooping(NULL) == 1;
 	if (cap == IOMMU_CAP_INTR_REMAP)
 		return irq_remapping_enabled == 1;
+	if (cap == IOMMU_CAP_VIOMMU)
+		return caching_mode_enabled();
 
 	return false;
 }

diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 2bfdd5734844..719793e103db 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c

@@ -931,7 +931,16 @@ static int viommu_of_xlate(struct device *dev, struct of_phandle_args *args)
 	return iommu_fwspec_add_ids(dev, args->args, 1);
 }
 
+static bool viommu_capable(enum iommu_cap cap)
+{
+	if (cap == IOMMU_CAP_VIOMMU)
+		return true;
+
+	return false;
+}
+
 static struct iommu_ops viommu_ops = {
+	.capable		= viommu_capable,
 	.domain_alloc		= viommu_domain_alloc,
 	.domain_free		= viommu_domain_free,
 	.attach_dev		= viommu_attach_dev,

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index b95a6f8db6ff..1d24be667a03 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h

@@ -94,6 +94,7 @@ enum iommu_cap {
 					   transactions */
 	IOMMU_CAP_INTR_REMAP,		/* IOMMU supports interrupt isolation */
 	IOMMU_CAP_NOEXEC,		/* IOMMU_NOEXEC flag */
+	IOMMU_CAP_VIOMMU,		/* IOMMU can declar running in a VM */
 };
 
 /*

-- 
2.25.1

The pci_subdevice_msi_create_irq_domain() should fail if the underlying
platform is not able to support IMS (Interrupt Message Storage). Otherwise,
the isolation of interrupt is not guaranteed.

For x86, IMS is only supported on bare metal for now. We could enable it
in the virtualization environments in the future if interrupt HYPERCALL
domain is supported or the hardware has the capability of interrupt
isolation for subdevices.

Cc: David Woodhouse <redacted>
Cc: Leon Romanovsky <leon@kernel.org>
Cc: Kevin Tian <kevin.tian@intel.com>
Suggested-by: Thomas Gleixner <redacted>
Link: https://lore.kernel.org/linux-pci/87pn4nk7nn.fsf@nanos.tec.linutronix.de/
Link: https://lore.kernel.org/linux-pci/877dqrnzr3.fsf@nanos.tec.linutronix.de/
Link: https://lore.kernel.org/linux-pci/877dqqmc2h.fsf@nanos.tec.linutronix.de/
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 arch/x86/pci/common.c       | 71 +++++++++++++++++++++++++++++++++++++
 drivers/base/platform-msi.c |  8 +++++
 include/linux/msi.h         |  1 +
 3 files changed, 80 insertions(+)

diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
index 3507f456fcd0..9deb826fb242 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c

@@ -12,6 +12,7 @@
 #include <linux/init.h>
 #include <linux/dmi.h>
 #include <linux/slab.h>
+#include <linux/iommu.h>
 
 #include <asm/acpi.h>
 #include <asm/segment.h>

@@ -724,3 +725,73 @@ struct pci_dev *pci_real_dma_dev(struct pci_dev *dev)
 	return dev;
 }
 #endif
+
+/*
+ * We want to figure out which context we are running in. But the hardware
+ * does not introduce a reliable way (instruction, CPUID leaf, MSR, whatever)
+ * which can be manipulated by the VMM to let the OS figure out where it runs.
+ * So we go with the below probably on_bare_metal() function as a replacement
+ * for definitely on_bare_metal() to go forward only for the very simple reason
+ * that this is the only option we have.
+ */
+static const char * const vmm_vendor_name[] = {
+	"QEMU", "Bochs", "KVM", "Xen", "VMware", "VMW", "VMware Inc.",
+	"innotek GmbH", "Oracle Corporation", "Parallels", "BHYVE"
+};
+
+static void read_type0_virtual_machine(const struct dmi_header *dm, void *p)
+{
+	u8 *data = (u8 *)dm + 0x13;
+
+	/* BIOS Information (Type 0) */
+	if (dm->type != 0 || dm->length < 0x14)
+		return;
+
+	/* Bit 4 of BIOS Characteristics Extension Byte 2*/
+	if (*data & BIT(4))
+		*((bool *)p) = true;
+}
+
+static bool smbios_virtual_machine(void)
+{
+	bool bit_present = false;
+
+	dmi_walk(read_type0_virtual_machine, &bit_present);
+
+	return bit_present;
+}
+
+static bool on_bare_metal(struct device *dev)
+{
+	int i;
+
+	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
+		return false;
+
+	if (smbios_virtual_machine())
+		return false;
+
+	if (iommu_capable(dev->bus, IOMMU_CAP_VIOMMU))
+		return false;
+
+	for (i = 0; i < ARRAY_SIZE(vmm_vendor_name); i++)
+		if (dmi_match(DMI_SYS_VENDOR, vmm_vendor_name[i]))
+			return false;
+
+	pr_info("System running on bare metal, report to bugzilla.kernel.org if not the case.");
+
+	return true;
+}
+
+bool arch_support_pci_device_ims(struct pci_dev *pdev)
+{
+	/*
+	 * When we are running in a VMM context, the device IMS could only be
+	 * enabled when the underlying hardware supports interrupt isolation
+	 * of the subdevice, or any mechanism (trap, hypercall) is added so
+	 * that changes in the interrupt message store could be managed by the
+	 * VMM. For now, we only support the device IMS when we are running on
+	 * the bare metal.
+	 */
+	return on_bare_metal(&pdev->dev);
+}

diff --git a/drivers/base/platform-msi.c b/drivers/base/platform-msi.c
index 8432a1bf4e28..88e5fe4dae67 100644
--- a/drivers/base/platform-msi.c
+++ b/drivers/base/platform-msi.c

@@ -512,6 +512,11 @@ struct irq_domain *device_msi_create_irq_domain(struct fwnode_handle *fn,
 #ifdef CONFIG_PCI
 #include <linux/pci.h>
 
+bool __weak arch_support_pci_device_ims(struct pci_dev *pdev)
+{
+	return false;
+}
+
 /**
  * pci_subdevice_msi_create_irq_domain - Create an irq domain for subdevices
  * @pdev:	Pointer to PCI device for which the subdevice domain is created

@@ -523,6 +528,9 @@ struct irq_domain *pci_subdevice_msi_create_irq_domain(struct pci_dev *pdev,
 	struct irq_domain *domain, *pdev_msi;
 	struct fwnode_handle *fn;
 
+	if (!arch_support_pci_device_ims(pdev))
+		return NULL;
+
 	/*
 	 * Retrieve the MSI domain of the underlying PCI device's MSI
 	 * domain. The PCI device domain's parent domain is also the parent

diff --git a/include/linux/msi.h b/include/linux/msi.h
index f319d7c6a4ef..6fda81c4b859 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h

@@ -478,6 +478,7 @@ struct irq_domain *device_msi_create_irq_domain(struct fwnode_handle *fn,
 						struct irq_domain *parent);
 
 # ifdef CONFIG_PCI
+bool arch_support_pci_device_ims(struct pci_dev *pdev);
 struct irq_domain *pci_subdevice_msi_create_irq_domain(struct pci_dev *pdev,
 						       struct msi_domain_info *info);
 # endif

-- 
2.25.1

Thinking more I wonder whether this check is actually useful here. As Leon
and David commented, the same vendor name can be used both for VM
and bare metal instances. It implies that both bare metal and VM might be
misinterpreted with this check. This might not be what we want originally -
find heuristics to indicate a VM environment and tolerate misinterpreting 
VM as bare metal in corner cases (but not vice versa).

Thomas?

Thanks
Kevin

On Thu, Jan 14, 2021 at 09:30:02AM +0800, Lu Baolu wrote:

Some vendor IOMMU drivers are able to declare that it is running in a VM
context. This is very valuable for the features that only want to be
supported on bare metal. Add a capability bit so that it could be used.

And how is it used? Who and how will set it?

Kernel coding style is not in favour of inline functions in *.c files.

Hi Leon,

On 1/14/21 9:26 PM, Leon Romanovsky wrote:

On Thu, Jan 14, 2021 at 09:30:02AM +0800, Lu Baolu wrote:

quoted

Some vendor IOMMU drivers are able to declare that it is running in a VM
context. This is very valuable for the features that only want to be
supported on bare metal. Add a capability bit so that it could be used.

And how is it used? Who and how will set it?

Use the existing iommu_capable(). I should add more descriptions about
who and how to use it.

quoted

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
  drivers/iommu/intel/iommu.c  | 20 ++++++++++++++++++++
  drivers/iommu/virtio-iommu.c |  9 +++++++++
  include/linux/iommu.h        |  1 +
  3 files changed, 30 insertions(+)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index cb205a04fe4c..8eb022d0e8aa 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c

@@ -5738,12 +5738,32 @@ static inline bool nested_mode_support(void)
  	return ret;
  }

+static inline bool caching_mode_enabled(void)
+{

Kernel coding style is not in favour of inline functions in *.c files.

Yes, agreed.

Best regards,
baolu

quoted

+	struct dmar_drhd_unit *drhd;
+	struct intel_iommu *iommu;
+	bool ret = false;
+
+	rcu_read_lock();
+	for_each_active_iommu(iommu, drhd) {
+		if (cap_caching_mode(iommu->cap)) {
+			ret = true;
+			break;
+		}
+	}
+	rcu_read_unlock();
+
+	return ret;
+}
+
  static bool intel_iommu_capable(enum iommu_cap cap)
  {
  	if (cap == IOMMU_CAP_CACHE_COHERENCY)
  		return domain_update_iommu_snooping(NULL) == 1;
  	if (cap == IOMMU_CAP_INTR_REMAP)
  		return irq_remapping_enabled == 1;
+	if (cap == IOMMU_CAP_VIOMMU)
+		return caching_mode_enabled();

  	return false;
  }

diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 2bfdd5734844..719793e103db 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c

@@ -931,7 +931,16 @@ static int viommu_of_xlate(struct device *dev, struct of_phandle_args *args)
  	return iommu_fwspec_add_ids(dev, args->args, 1);
  }

+static bool viommu_capable(enum iommu_cap cap)
+{
+	if (cap == IOMMU_CAP_VIOMMU)
+		return true;
+
+	return false;
+}
+
  static struct iommu_ops viommu_ops = {
+	.capable		= viommu_capable,
  	.domain_alloc		= viommu_domain_alloc,
  	.domain_free		= viommu_domain_free,
  	.attach_dev		= viommu_attach_dev,

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index b95a6f8db6ff..1d24be667a03 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h

@@ -94,6 +94,7 @@ enum iommu_cap {
  					   transactions */
  	IOMMU_CAP_INTR_REMAP,		/* IOMMU supports interrupt isolation */
  	IOMMU_CAP_NOEXEC,		/* IOMMU_NOEXEC flag */
+	IOMMU_CAP_VIOMMU,		/* IOMMU can declar running in a VM */
  };

  /*
--

2.25.1

On 2021/1/14 上午9:30, Lu Baolu wrote:

This part I don't understand. Does it mean Intel IOMMU can't be used in VM without caching mode?

Thanks

On Fri, Jan 15, 2021 at 07:49:47AM +0800, Lu Baolu wrote:

Hi Leon,

On 1/14/21 9:26 PM, Leon Romanovsky wrote:

quoted

On Thu, Jan 14, 2021 at 09:30:02AM +0800, Lu Baolu wrote:

quoted

Some vendor IOMMU drivers are able to declare that it is running in a VM
context. This is very valuable for the features that only want to be
supported on bare metal. Add a capability bit so that it could be used.

And how is it used? Who and how will set it?

Use the existing iommu_capable(). I should add more descriptions about
who and how to use it.

I want to see the code that sets this capability.

Thanks

Hi,

On 2021/1/15 14:31, Leon Romanovsky wrote:

On Fri, Jan 15, 2021 at 07:49:47AM +0800, Lu Baolu wrote:

quoted

Hi Leon,

On 1/14/21 9:26 PM, Leon Romanovsky wrote:

quoted

On Thu, Jan 14, 2021 at 09:30:02AM +0800, Lu Baolu wrote:

quoted

Some vendor IOMMU drivers are able to declare that it is running in a VM
context. This is very valuable for the features that only want to be
supported on bare metal. Add a capability bit so that it could be used.

And how is it used? Who and how will set it?

Use the existing iommu_capable(). I should add more descriptions about
who and how to use it.

I want to see the code that sets this capability.

Currently we have Intel VT-d and the virt-iommu setting this capability.

 static bool intel_iommu_capable(enum iommu_cap cap)
 {
 	if (cap == IOMMU_CAP_CACHE_COHERENCY)
 		return domain_update_iommu_snooping(NULL) == 1;
 	if (cap == IOMMU_CAP_INTR_REMAP)
 		return irq_remapping_enabled == 1;
+	if (cap == IOMMU_CAP_VIOMMU)
+		return caching_mode_enabled();

 	return false;
 }

And,

+static bool viommu_capable(enum iommu_cap cap)
+{
+	if (cap == IOMMU_CAP_VIOMMU)
+		return true;
+
+	return false;
+}

Best regards,
baolu

On Sat, Jan 16, 2021 at 09:20:16AM +0800, Lu Baolu wrote:

Hi,

On 2021/1/15 14:31, Leon Romanovsky wrote:

quoted

On Fri, Jan 15, 2021 at 07:49:47AM +0800, Lu Baolu wrote:

quoted

Hi Leon,

On 1/14/21 9:26 PM, Leon Romanovsky wrote:

quoted

On Thu, Jan 14, 2021 at 09:30:02AM +0800, Lu Baolu wrote:

quoted

Some vendor IOMMU drivers are able to declare that it is running in a VM
context. This is very valuable for the features that only want to be
supported on bare metal. Add a capability bit so that it could be used.

And how is it used? Who and how will set it?

Use the existing iommu_capable(). I should add more descriptions about
who and how to use it.

I want to see the code that sets this capability.

Currently we have Intel VT-d and the virt-iommu setting this capability.

 static bool intel_iommu_capable(enum iommu_cap cap)
 {
 	if (cap == IOMMU_CAP_CACHE_COHERENCY)
 		return domain_update_iommu_snooping(NULL) == 1;
 	if (cap == IOMMU_CAP_INTR_REMAP)
 		return irq_remapping_enabled == 1;
+	if (cap == IOMMU_CAP_VIOMMU)
+		return caching_mode_enabled();

 	return false;
 }

And,

+static bool viommu_capable(enum iommu_cap cap)
+{
+	if (cap == IOMMU_CAP_VIOMMU)
+		return true;
+
+	return false;
+}

These two functions are reading this cap and not setting.
Where can I see code that does "cap = IOMMU_CAP_VIOMMU" and not "=="?

Best regards,
baolu

Hi Leon,

On 2021/1/16 16:39, Leon Romanovsky wrote:

On Sat, Jan 16, 2021 at 09:20:16AM +0800, Lu Baolu wrote:

quoted

Hi,

On 2021/1/15 14:31, Leon Romanovsky wrote:

quoted

On Fri, Jan 15, 2021 at 07:49:47AM +0800, Lu Baolu wrote:

quoted

Hi Leon,

On 1/14/21 9:26 PM, Leon Romanovsky wrote:

quoted

On Thu, Jan 14, 2021 at 09:30:02AM +0800, Lu Baolu wrote:

quoted

Some vendor IOMMU drivers are able to declare that it is running in a VM
context. This is very valuable for the features that only want to be
supported on bare metal. Add a capability bit so that it could be used.

And how is it used? Who and how will set it?

Use the existing iommu_capable(). I should add more descriptions about
who and how to use it.

I want to see the code that sets this capability.

Currently we have Intel VT-d and the virt-iommu setting this capability.

  static bool intel_iommu_capable(enum iommu_cap cap)
  {
  	if (cap == IOMMU_CAP_CACHE_COHERENCY)
  		return domain_update_iommu_snooping(NULL) == 1;
  	if (cap == IOMMU_CAP_INTR_REMAP)
  		return irq_remapping_enabled == 1;
+	if (cap == IOMMU_CAP_VIOMMU)
+		return caching_mode_enabled();

  	return false;
  }

And,

+static bool viommu_capable(enum iommu_cap cap)
+{
+	if (cap == IOMMU_CAP_VIOMMU)
+		return true;
+
+	return false;
+}

These two functions are reading this cap and not setting.
Where can I see code that does "cap = IOMMU_CAP_VIOMMU" and not "=="?

The iommu_capable() is a generic IOMMU interface to query IOMMU
capabilities. It takes @bus and @cap as input, and calls the callback
of vendor iommu. If the vendor iommu driver supports the specific
capability, it returns true. Otherwise, it returns false.

bool iommu_capable(struct bus_type *bus, enum iommu_cap cap)
{
        if (!bus->iommu_ops || !bus->iommu_ops->capable)
                return false;

        return bus->iommu_ops->capable(cap);
}
EXPORT_SYMBOL_GPL(iommu_capable);

In the vendor iommu's callback, it checks the capability and returns a
value according to its capability, just as showed above.

Best regards,
baolu

On Sat, Jan 16, 2021 at 04:47:40PM +0800, Lu Baolu wrote:

Hi Leon,

On 2021/1/16 16:39, Leon Romanovsky wrote:

quoted

On Sat, Jan 16, 2021 at 09:20:16AM +0800, Lu Baolu wrote:

quoted

Hi,

On 2021/1/15 14:31, Leon Romanovsky wrote:

quoted

On Fri, Jan 15, 2021 at 07:49:47AM +0800, Lu Baolu wrote:

quoted

Hi Leon,

On 1/14/21 9:26 PM, Leon Romanovsky wrote:

quoted

On Thu, Jan 14, 2021 at 09:30:02AM +0800, Lu Baolu wrote:

quoted

Some vendor IOMMU drivers are able to declare that it is running in a VM
context. This is very valuable for the features that only want to be
supported on bare metal. Add a capability bit so that it could be used.

And how is it used? Who and how will set it?

Use the existing iommu_capable(). I should add more descriptions about
who and how to use it.

I want to see the code that sets this capability.

Currently we have Intel VT-d and the virt-iommu setting this capability.

  static bool intel_iommu_capable(enum iommu_cap cap)
  {
  	if (cap == IOMMU_CAP_CACHE_COHERENCY)
  		return domain_update_iommu_snooping(NULL) == 1;
  	if (cap == IOMMU_CAP_INTR_REMAP)
  		return irq_remapping_enabled == 1;
+	if (cap == IOMMU_CAP_VIOMMU)
+		return caching_mode_enabled();

  	return false;
  }

And,

+static bool viommu_capable(enum iommu_cap cap)
+{
+	if (cap == IOMMU_CAP_VIOMMU)
+		return true;
+
+	return false;
+}

These two functions are reading this cap and not setting.
Where can I see code that does "cap = IOMMU_CAP_VIOMMU" and not "=="?

The iommu_capable() is a generic IOMMU interface to query IOMMU
capabilities. It takes @bus and @cap as input, and calls the callback
of vendor iommu. If the vendor iommu driver supports the specific
capability, it returns true. Otherwise, it returns false.

bool iommu_capable(struct bus_type *bus, enum iommu_cap cap)
{
        if (!bus->iommu_ops || !bus->iommu_ops->capable)
                return false;

        return bus->iommu_ops->capable(cap);
}
EXPORT_SYMBOL_GPL(iommu_capable);

In the vendor iommu's callback, it checks the capability and returns a
value according to its capability, just as showed above.

Ohh, sorry.
I missed "iommu_capable(dev->bus, IOMMU_CAP_VIOMMU)" from second patch.

Thanks

Best regards,
baolu