[PATCH 0/5] vfio-pci: Misc enhancements

[PATCH 0/5] vfio-pci: Misc enhancements

[Alex Williamson]

Each of these patches stands on it's own, but they all touch the
same file and therefore build on each other to some extent.  The first
patch allows vfio-pci to accept a list of IDs as a module parameter,
exactly like pci-stub.  This is really useful to get vfio-pci bound to
devices that are intended to be used exclusively by userspace without
more complex scripts (particuarly with modprobe.d softdep, pre
loading).

The 2nd patch allows a module option to disable VGA access if vfio-pci
is configured with VGA support.  By itself this doesn't do much, but
with patch 5, we add a VGA arbiter client that will opt-out devices
from arbitration if the user cannot access VGA regions of the device.
If you have extra GPUs in your system exclusively for use via vfio-pci
and don't depend on VGA resource access, this opt-out can allow host
graphics to enable DRI2 support as if the additional GPUs weren't
there.  This depends on https://lkml.org/lkml/2015/2/24/430 or else
vga arbiter support gets ugly (still looking for an Ack there).

The other interesting feature here is idle power management.  It's
possible that there are extra devices in a host that are used
exclusively via vfio-pci and that those devices are not used 100% of
the time.  To be power conscious we can put devices into a low power
state when they're not actively being used.  This doesn't always save
much, or necessarily any power, but we can at least try.

I'll also note that the last patch makes use of pci_bus_max_busnr()
which seems to be in flux with some current upstream patches.  I don't
really care if we use pci_bus_max_busnr() or some replacement for it,
so long as pci-core allows a way to determine the bus number range
below a bridge so I can determine if there's more than one VGA device
below it.  Comments welcome.  Thanks,

Alex

---

Alex Williamson (5):
      vfio-pci: Allow PCI IDs to be specified as module options
      vfio-pci: Add module option to disable VGA region access
      vfio-pci: Remove warning if try-reset fails
      vfio-pci: Move idle devices to D3hot power state
      vfio-pci: Add VGA arbiter client


 drivers/vfio/pci/vfio_pci.c |  174 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 157 insertions(+), 17 deletions(-)

[PATCH 1/5] vfio-pci: Allow PCI IDs to be specified as module options

[Alex Williamson]

This copies the same support from pci-stub for exactly the same
purpose, enabling a set of PCI IDs to be automatically added to the
driver's dynamic ID table at module load time.  The code here is
pretty simple and both vfio-pci and pci-stub are fairly unique in
being meta drivers, capable of attaching to any device, so there's no
attempt made to generalize the code into pci-core.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 drivers/vfio/pci/vfio_pci.c |   46 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index f8a1863..b3bae4c 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -32,6 +32,10 @@
 #define DRIVER_AUTHOR   "Alex Williamson <alex.williamson@redhat.com>"
 #define DRIVER_DESC     "VFIO PCI - User Level meta-driver"
 
+static char ids[1024] __initdata;
+module_param_string(ids, ids, sizeof(ids), 0);
+MODULE_PARM_DESC(ids, "Initial PCI IDs to add to the vfio driver, format is \"vendor:device[:subvendor[:subdevice[:class[:class_mask]]]]\" and multiple comma separated entries can be specified");
+
 static bool nointxmask;
 module_param_named(nointxmask, nointxmask, bool, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(nointxmask,
@@ -1034,6 +1038,46 @@ static void __exit vfio_pci_cleanup(void)
 	vfio_pci_uninit_perm_bits();
 }
 
+static void __init vfio_pci_fill_ids(void)
+{
+	char *p, *id;
+	int rc;
+
+	/* no ids passed actually */
+	if (ids[0] == '\0')
+		return;
+
+	/* add ids specified in the module parameter */
+	p = ids;
+	while ((id = strsep(&p, ","))) {
+		unsigned int vendor, device, subvendor = PCI_ANY_ID,
+			subdevice = PCI_ANY_ID, class = 0, class_mask = 0;
+		int fields;
+
+		if (!strlen(id))
+			continue;
+
+		fields = sscanf(id, "%x:%x:%x:%x:%x:%x",
+				&vendor, &device, &subvendor, &subdevice,
+				&class, &class_mask);
+
+		if (fields < 2) {
+			pr_warn("vfio-pci: invalid id string \"%s\"\n", id);
+			continue;
+		}
+
+		pr_info("vfio-pci: add %04X:%04X sub=%04X:%04X cls=%08X/%08X\n",
+			vendor, device, subvendor, subdevice,
+			class, class_mask);
+
+		rc = pci_add_dynid(&vfio_pci_driver, vendor, device,
+				   subvendor, subdevice, class, class_mask, 0);
+		if (rc)
+			pr_warn("vfio-pci: failed to add dynamic id (%d)\n",
+				rc);
+	}
+}
+
 static int __init vfio_pci_init(void)
 {
 	int ret;
@@ -1053,6 +1097,8 @@ static int __init vfio_pci_init(void)
 	if (ret)
 		goto out_driver;
 
+	vfio_pci_fill_ids();
+
 	return 0;
 
 out_driver:

[PATCH 2/5] vfio-pci: Add module option to disable VGA region access

[Alex Williamson]

Add a module option so that we don't require a CONFIG change and
kernel rebuild to disable VGA support.  Not only can VGA support be
troublesome in itself, but by disabling it we can reduce the impact
to host devices by doing a VGA arbitration opt-out.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 drivers/vfio/pci/vfio_pci.c |   20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index b3bae4c..9c854b0 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -41,8 +41,24 @@ module_param_named(nointxmask, nointxmask, bool, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(nointxmask,
 		  "Disable support for PCI 2.3 style INTx masking.  If this resolves problems for specific devices, report lspci -vvvxxx to linux-pci@vger.kernel.org so the device can be fixed automatically via the broken_intx_masking flag.");
 
+#ifdef CONFIG_VFIO_PCI_VGA
+static bool disable_vga;
+module_param_named(disable_vga, disable_vga, bool, S_IRUGO);
+MODULE_PARM_DESC(disable_vga,
+		 "Disable VGA resource access for VGA-capable devices");
+#endif
+
 static DEFINE_MUTEX(driver_lock);
 
+static inline bool vfio_vga_disabled(void)
+{
+#ifdef CONFIG_VFIO_PCI_VGA
+	return disable_vga;
+#else
+	return true;
+#endif
+}
+
 static void vfio_pci_try_bus_reset(struct vfio_pci_device *vdev);
 
 static int vfio_pci_enable(struct vfio_pci_device *vdev)
@@ -97,10 +113,8 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev)
 	} else
 		vdev->msix_bar = 0xFF;
 
-#ifdef CONFIG_VFIO_PCI_VGA
-	if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
+	if (!vfio_vga_disabled() && (pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
 		vdev->has_vga = true;
-#endif
 
 	return 0;
 }

[PATCH 3/5] vfio-pci: Remove warning if try-reset fails

[Alex Williamson]

As indicated in the comment, this is not entirely uncommon and
causes user concern for no reason.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 drivers/vfio/pci/vfio_pci.c |   10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 9c854b0..e5eb2e6 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -171,14 +171,8 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev)
 	 * Try to reset the device.  The success of this is dependent on
 	 * being able to lock the device, which is not always possible.
 	 */
-	if (vdev->reset_works) {
-		int ret = pci_try_reset_function(pdev);
-		if (ret)
-			pr_warn("%s: Failed to reset device %s (%d)\n",
-				__func__, dev_name(&pdev->dev), ret);
-		else
-			vdev->needs_reset = false;
-	}
+	if (vdev->reset_works && !pci_try_reset_function(pdev))
+		vdev->needs_reset = false;
 
 	pci_restore_state(pdev);
 out:

[PATCH 4/5] vfio-pci: Move idle devices to D3hot power state

[Alex Williamson]

We can save some power by putting devices that are bound to vfio-pci
but not in use by the user in the D3hot power state.  Devices get
woken into D0 when opened by the user.  Resets return the device to
D0, so we need to re-apply the low power state after a bus reset.
It's tempting to try to use D3cold, but we have no reason to inhibit
hotplug of idle devices and we might get into a loop of having the
device disappear before we have a chance to try to use it.

A new module parameter "disable_idle_pm" allows this feature to be
disabled if there are devices that misbehave as a result of this
change.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 drivers/vfio/pci/vfio_pci.c |   33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index e5eb2e6..2133b74 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -48,6 +48,11 @@ MODULE_PARM_DESC(disable_vga,
 		 "Disable VGA resource access for VGA-capable devices");
 #endif
 
+static bool disable_idle_pm;
+module_param_named(disable_idle_pm, disable_idle_pm, bool, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(disable_idle_pm,
+		 "Disable support for trying to move idle, unused devices to a low power state.  This might be necessary if devices do not behave properly moving into or out of low power states.");
+
 static DEFINE_MUTEX(driver_lock);
 
 static inline bool vfio_vga_disabled(void)
@@ -68,6 +73,8 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev)
 	u16 cmd;
 	u8 msix_pos;
 
+	pci_set_power_state(pdev, PCI_D0);
+
 	/* Don't allow our initial saved state to include busmaster */
 	pci_clear_master(pdev);
 
@@ -179,6 +186,9 @@ out:
 	pci_disable_device(pdev);
 
 	vfio_pci_try_bus_reset(vdev);
+
+	if (!disable_idle_pm)
+		pci_set_power_state(pdev, PCI_D3hot);
 }
 
 static void vfio_pci_release(void *device_data)
@@ -899,6 +909,17 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		kfree(vdev);
 	}
 
+	if (!disable_idle_pm) {
+		/*
+		 * pci-core sets the device power state to an unknown value at
+		 * bootup and after being removed from a driver.  The only
+		 * transition it allows from this unknown state is to D0.  We
+		 * therefore first do a D0 transition before going to D3.
+		 */
+		pci_set_power_state(pdev, PCI_D0);
+		pci_set_power_state(pdev, PCI_D3hot);
+	}
+
 	return ret;
 }
 
@@ -911,6 +932,9 @@ static void vfio_pci_remove(struct pci_dev *pdev)
 		iommu_group_put(pdev->dev.iommu_group);
 		kfree(vdev);
 	}
+
+	if (!disable_idle_pm)
+		pci_set_power_state(pdev, PCI_D0);
 }
 
 static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev,
@@ -1029,10 +1053,13 @@ static void vfio_pci_try_bus_reset(struct vfio_pci_device *vdev)
 
 put_devs:
 	for (i = 0; i < devs.cur_index; i++) {
-		if (!ret) {
-			tmp = vfio_device_data(devs.devices[i]);
+		tmp = vfio_device_data(devs.devices[i]);
+		if (!ret)
 			tmp->needs_reset = false;
-		}
+
+		if (!tmp->refcnt && !disable_idle_pm)
+			pci_set_power_state(tmp->pdev, PCI_D3hot);
+
 		vfio_device_put(devs.devices[i]);
 	}
 

[PATCH 5/5] vfio-pci: Add VGA arbiter client

[Alex Williamson]

If VFIO VGA access is disabled for the user, either by CONFIG option
or module parameter, we can often opt-out of VGA arbitration.  We can
do this when PCI bridge control of VGA routing is possible.  This
means that we must have a parent bridge and there must only be a
single VGA device below that bridge.  Fortunately this is the typical
case for discrete GPUs.

Doing this allows us to minimize the impact of additional GPUs, in
terms of VGA arbitration, when they are only used via vfio-pci for
non-VGA applications.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 drivers/vfio/pci/vfio_pci.c |   67 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 63 insertions(+), 4 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 2133b74..2522d7c 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -25,6 +25,7 @@
 #include <linux/types.h>
 #include <linux/uaccess.h>
 #include <linux/vfio.h>
+#include <linux/vgaarb.h>
 
 #include "vfio_pci_private.h"
 
@@ -64,6 +65,50 @@ static inline bool vfio_vga_disabled(void)
 #endif
 }
 
+/*
+ * Our VGA arbiter participation is limited since we don't know anything
+ * about the device itself.  However, if the device is the only VGA device
+ * downstream of a bridge and VFIO VGA support is disabled, then we can
+ * safely return legacy VGA IO and memory as not decoded since the user
+ * has no way to get to it and routing can be disabled externally at the
+ * bridge.
+ */
+static unsigned int vfio_pci_set_vga_decode(void *opaque, bool single_vga)
+{
+	struct vfio_pci_device *vdev = opaque;
+	struct pci_dev *tmp = NULL, *pdev = vdev->pdev;
+	unsigned char max_busnr;
+	unsigned int decodes;
+
+	if (single_vga || !vfio_vga_disabled() || pci_is_root_bus(pdev->bus))
+		return VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM |
+		       VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM;
+
+	max_busnr = pci_bus_max_busnr(pdev->bus);
+	decodes = VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM;
+
+	while ((tmp = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, tmp)) != NULL) {
+		if (tmp == pdev ||
+		    pci_domain_nr(tmp->bus) != pci_domain_nr(pdev->bus) ||
+		    pci_is_root_bus(tmp->bus))
+			continue;
+
+		if (tmp->bus->number >= pdev->bus->number &&
+		    tmp->bus->number <= max_busnr) {
+			pci_dev_put(tmp);
+			decodes |= VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM;
+			break;
+		}
+	}
+
+	return decodes;
+}
+
+static inline bool vfio_pci_is_vga(struct pci_dev *pdev)
+{
+	return (pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA;
+}
+
 static void vfio_pci_try_bus_reset(struct vfio_pci_device *vdev);
 
 static int vfio_pci_enable(struct vfio_pci_device *vdev)
@@ -120,7 +165,7 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev)
 	} else
 		vdev->msix_bar = 0xFF;
 
-	if (!vfio_vga_disabled() && (pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
+	if (!vfio_vga_disabled() && vfio_pci_is_vga(pdev))
 		vdev->has_vga = true;
 
 	return 0;
@@ -909,6 +954,12 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		kfree(vdev);
 	}
 
+	if (vfio_pci_is_vga(pdev)) {
+		vga_client_register(pdev, vdev, NULL, vfio_pci_set_vga_decode);
+		vga_set_legacy_decoding(pdev,
+					vfio_pci_set_vga_decode(vdev, false));
+	}
+
 	if (!disable_idle_pm) {
 		/*
 		 * pci-core sets the device power state to an unknown value at
@@ -928,9 +979,17 @@ static void vfio_pci_remove(struct pci_dev *pdev)
 	struct vfio_pci_device *vdev;
 
 	vdev = vfio_del_group_dev(&pdev->dev);
-	if (vdev) {
-		iommu_group_put(pdev->dev.iommu_group);
-		kfree(vdev);
+	if (!vdev)
+		return;
+
+	iommu_group_put(pdev->dev.iommu_group);
+	kfree(vdev);
+
+	if (vfio_pci_is_vga(pdev)) {
+		vga_client_register(pdev, NULL, NULL, NULL);
+		vga_set_legacy_decoding(pdev,
+				VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM |
+				VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM);
 	}
 
 	if (!disable_idle_pm)

Re: [PATCH 1/5] vfio-pci: Allow PCI IDs to be specified as module options

[Bandan Das]

Hi Alex,

Alex Williamson <alex.williamson@redhat.com> writes:
...
> +		if (fields < 2) {
> +			pr_warn("vfio-pci: invalid id string \"%s\"\n", id);
> +			continue;
> +		}
> +
> +		pr_info("vfio-pci: add %04X:%04X sub=%04X:%04X cls=%08X/%08X\n",
> +			vendor, device, subvendor, subdevice,
> +			class, class_mask);
> +
> +		rc = pci_add_dynid(&vfio_pci_driver, vendor, device,
> +				   subvendor, subdevice, class, class_mask, 0);
> +		if (rc)
> +			pr_warn("vfio-pci: failed to add dynamic id (%d)\n",
> +				rc);
Just a minor observation - maybe we should print out the
vendor/device/subvendor/subdevice as part of the failure message too. The info
message could potentially get lost in a system with high syslog traffic.

Bandan

> +	}
> +}
> +
>  static int __init vfio_pci_init(void)
>  {
>  	int ret;
> @@ -1053,6 +1097,8 @@ static int __init vfio_pci_init(void)
>  	if (ret)
>  		goto out_driver;
>  
> +	vfio_pci_fill_ids();
> +
>  	return 0;
>  
>  out_driver:
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] vfio-pci: Allow PCI IDs to be specified as module options

[Alex Williamson]

On Wed, 2015-03-04 at 15:49 -0500, Bandan Das wrote:
> Hi Alex,
> 
> Alex Williamson <alex.williamson@redhat.com> writes:
> ...
> > +		if (fields < 2) {
> > +			pr_warn("vfio-pci: invalid id string \"%s\"\n", id);
> > +			continue;
> > +		}
> > +
> > +		pr_info("vfio-pci: add %04X:%04X sub=%04X:%04X cls=%08X/%08X\n",
> > +			vendor, device, subvendor, subdevice,
> > +			class, class_mask);
> > +
> > +		rc = pci_add_dynid(&vfio_pci_driver, vendor, device,
> > +				   subvendor, subdevice, class, class_mask, 0);
> > +		if (rc)
> > +			pr_warn("vfio-pci: failed to add dynamic id (%d)\n",
> > +				rc);
> Just a minor observation - maybe we should print out the
> vendor/device/subvendor/subdevice as part of the failure message too. The info
> message could potentially get lost in a system with high syslog traffic.

Thanks for the comment.  I don't think we want to duplicate the pr_info
above it, so are you thinking something like this?

                if (fields < 2) {
                        pr_warn("vfio-pci: invalid id string \"%s\"\n", id);
                        continue;
                }

                rc = pci_add_dynid(&vfio_pci_driver, vendor, device,
                                   subvendor, subdevice, class, class_mask, 0);
                if (rc)
                        pr_warn("vfio-pci: failed to add dynamic id: %04X:%04X sub=%04X:%04X cls=%08X/%08X (%d)\n",
                                vendor, device, subvendor, subdevice,
                                class, class_mask, rc);
                else
                        pr_info("vfio-pci: add %04X:%04X sub=%04X:%04X cls=%08X/%08X\n",
                                vendor, device, subvendor, subdevice,
                                class, class_mask);
        }


> > +	}
> > +}
> > +
> >  static int __init vfio_pci_init(void)
> >  {
> >  	int ret;
> > @@ -1053,6 +1097,8 @@ static int __init vfio_pci_init(void)
> >  	if (ret)
> >  		goto out_driver;
> >  
> > +	vfio_pci_fill_ids();
> > +
> >  	return 0;
> >  
> >  out_driver:
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] vfio-pci: Allow PCI IDs to be specified as module options

[Bandan Das]

Alex Williamson <alex.williamson@redhat.com> writes:

> On Wed, 2015-03-04 at 15:49 -0500, Bandan Das wrote:
>> Hi Alex,
>> 
>> Alex Williamson <alex.williamson@redhat.com> writes:
>> ...
>> > +		if (fields < 2) {
>> > +			pr_warn("vfio-pci: invalid id string \"%s\"\n", id);
>> > +			continue;
>> > +		}
>> > +
>> > +		pr_info("vfio-pci: add %04X:%04X sub=%04X:%04X cls=%08X/%08X\n",
>> > +			vendor, device, subvendor, subdevice,
>> > +			class, class_mask);
>> > +
>> > +		rc = pci_add_dynid(&vfio_pci_driver, vendor, device,
>> > +				   subvendor, subdevice, class, class_mask, 0);
>> > +		if (rc)
>> > +			pr_warn("vfio-pci: failed to add dynamic id (%d)\n",
>> > +				rc);
>> Just a minor observation - maybe we should print out the
>> vendor/device/subvendor/subdevice as part of the failure message too. The info
>> message could potentially get lost in a system with high syslog traffic.
>
> Thanks for the comment.  I don't think we want to duplicate the pr_info
> above it, so are you thinking something like this?
>
>                 if (fields < 2) {
>                         pr_warn("vfio-pci: invalid id string \"%s\"\n", id);
>                         continue;
>                 }
>
>                 rc = pci_add_dynid(&vfio_pci_driver, vendor, device,
>                                    subvendor, subdevice, class, class_mask, 0);
>                 if (rc)
>                         pr_warn("vfio-pci: failed to add dynamic id: %04X:%04X sub=%04X:%04X cls=%08X/%08X (%d)\n",
>                                 vendor, device, subvendor, subdevice,
>                                 class, class_mask, rc);
>                 else
>                         pr_info("vfio-pci: add %04X:%04X sub=%04X:%04X cls=%08X/%08X\n",
>                                 vendor, device, subvendor, subdevice,
>                                 class, class_mask);
>         }

Yep, this is good. (BTW, we can define pr_fmt at the top of the file and avoid
the "vfio-pci" prefix)

>
>> > +	}
>> > +}
>> > +
>> >  static int __init vfio_pci_init(void)
>> >  {
>> >  	int ret;
>> > @@ -1053,6 +1097,8 @@ static int __init vfio_pci_init(void)
>> >  	if (ret)
>> >  		goto out_driver;
>> >  
>> > +	vfio_pci_fill_ids();
>> > +
>> >  	return 0;
>> >  
>> >  out_driver:
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe kvm" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] vfio-pci: Allow PCI IDs to be specified as module options

[Alex Williamson]

On Wed, 2015-03-04 at 17:32 -0500, Bandan Das wrote:
> Alex Williamson <alex.williamson@redhat.com> writes:
> 
> > On Wed, 2015-03-04 at 15:49 -0500, Bandan Das wrote:
> >> Hi Alex,
> >> 
> >> Alex Williamson <alex.williamson@redhat.com> writes:
> >> ...
> >> > +		if (fields < 2) {
> >> > +			pr_warn("vfio-pci: invalid id string \"%s\"\n", id);
> >> > +			continue;
> >> > +		}
> >> > +
> >> > +		pr_info("vfio-pci: add %04X:%04X sub=%04X:%04X cls=%08X/%08X\n",
> >> > +			vendor, device, subvendor, subdevice,
> >> > +			class, class_mask);
> >> > +
> >> > +		rc = pci_add_dynid(&vfio_pci_driver, vendor, device,
> >> > +				   subvendor, subdevice, class, class_mask, 0);
> >> > +		if (rc)
> >> > +			pr_warn("vfio-pci: failed to add dynamic id (%d)\n",
> >> > +				rc);
> >> Just a minor observation - maybe we should print out the
> >> vendor/device/subvendor/subdevice as part of the failure message too. The info
> >> message could potentially get lost in a system with high syslog traffic.
> >
> > Thanks for the comment.  I don't think we want to duplicate the pr_info
> > above it, so are you thinking something like this?
> >
> >                 if (fields < 2) {
> >                         pr_warn("vfio-pci: invalid id string \"%s\"\n", id);
> >                         continue;
> >                 }
> >
> >                 rc = pci_add_dynid(&vfio_pci_driver, vendor, device,
> >                                    subvendor, subdevice, class, class_mask, 0);
> >                 if (rc)
> >                         pr_warn("vfio-pci: failed to add dynamic id: %04X:%04X sub=%04X:%04X cls=%08X/%08X (%d)\n",
> >                                 vendor, device, subvendor, subdevice,
> >                                 class, class_mask, rc);
> >                 else
> >                         pr_info("vfio-pci: add %04X:%04X sub=%04X:%04X cls=%08X/%08X\n",
> >                                 vendor, device, subvendor, subdevice,
> >                                 class, class_mask);
> >         }
> 
> Yep, this is good. (BTW, we can define pr_fmt at the top of the file and avoid
> the "vfio-pci" prefix)

Cool, I'll remove the "vfio-pci: " prefixes and define the standard:

#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

Thanks!

> >> > +	}
> >> > +}
> >> > +
> >> >  static int __init vfio_pci_init(void)
> >> >  {
> >> >  	int ret;
> >> > @@ -1053,6 +1097,8 @@ static int __init vfio_pci_init(void)
> >> >  	if (ret)
> >> >  		goto out_driver;
> >> >  
> >> > +	vfio_pci_fill_ids();
> >> > +
> >> >  	return 0;
> >> >  
> >> >  out_driver:
> >> >
> >> > --
> >> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> >> > the body of a message to majordomo@vger.kernel.org
> >> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/5] vfio-pci: Allow PCI IDs to be specified as module options

[Bjorn Helgaas]

On Wed, Mar 04, 2015 at 01:02:43PM -0700, Alex Williamson wrote:
> This copies the same support from pci-stub for exactly the same
> purpose, enabling a set of PCI IDs to be automatically added to the
> driver's dynamic ID table at module load time.  The code here is
> pretty simple and both vfio-pci and pci-stub are fairly unique in
> being meta drivers, capable of attaching to any device, so there's no
> attempt made to generalize the code into pci-core.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>  drivers/vfio/pci/vfio_pci.c |   46 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index f8a1863..b3bae4c 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -32,6 +32,10 @@
>  #define DRIVER_AUTHOR   "Alex Williamson <alex.williamson@redhat.com>"
>  #define DRIVER_DESC     "VFIO PCI - User Level meta-driver"
>  
> +static char ids[1024] __initdata;
> +module_param_string(ids, ids, sizeof(ids), 0);
> +MODULE_PARM_DESC(ids, "Initial PCI IDs to add to the vfio driver, format is \"vendor:device[:subvendor[:subdevice[:class[:class_mask]]]]\" and multiple comma separated entries can be specified");
> +
>  static bool nointxmask;
>  module_param_named(nointxmask, nointxmask, bool, S_IRUGO | S_IWUSR);
>  MODULE_PARM_DESC(nointxmask,
> @@ -1034,6 +1038,46 @@ static void __exit vfio_pci_cleanup(void)
>  	vfio_pci_uninit_perm_bits();
>  }
>  
> +static void __init vfio_pci_fill_ids(void)
> +{
> +	char *p, *id;
> +	int rc;
> +
> +	/* no ids passed actually */
> +	if (ids[0] == '\0')
> +		return;
> +
> +	/* add ids specified in the module parameter */
> +	p = ids;
> +	while ((id = strsep(&p, ","))) {
> +		unsigned int vendor, device, subvendor = PCI_ANY_ID,
> +			subdevice = PCI_ANY_ID, class = 0, class_mask = 0;
> +		int fields;
> +
> +		if (!strlen(id))
> +			continue;
> +
> +		fields = sscanf(id, "%x:%x:%x:%x:%x:%x",
> +				&vendor, &device, &subvendor, &subdevice,
> +				&class, &class_mask);
> +
> +		if (fields < 2) {
> +			pr_warn("vfio-pci: invalid id string \"%s\"\n", id);
> +			continue;
> +		}
> +
> +		pr_info("vfio-pci: add %04X:%04X sub=%04X:%04X cls=%08X/%08X\n",

pci_setup_device() uses "[%04x:%04x] ... class %#08x".  Maybe they should
be the same, at least as far as using upper/lower case, spelling out
"class", and "0x" prefix.  Maybe there's other precedent that you're
following and pci_setup_device() is not?

> +			vendor, device, subvendor, subdevice,
> +			class, class_mask);
> +
> +		rc = pci_add_dynid(&vfio_pci_driver, vendor, device,
> +				   subvendor, subdevice, class, class_mask, 0);
> +		if (rc)
> +			pr_warn("vfio-pci: failed to add dynamic id (%d)\n",
> +				rc);
> +	}
> +}
> +
>  static int __init vfio_pci_init(void)
>  {
>  	int ret;
> @@ -1053,6 +1097,8 @@ static int __init vfio_pci_init(void)
>  	if (ret)
>  		goto out_driver;
>  
> +	vfio_pci_fill_ids();
> +
>  	return 0;
>  
>  out_driver:
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/5] vfio-pci: Allow PCI IDs to be specified as module options

[Alex Williamson]

On Fri, 2015-03-06 at 16:11 -0600, Bjorn Helgaas wrote:
> On Wed, Mar 04, 2015 at 01:02:43PM -0700, Alex Williamson wrote:
> > This copies the same support from pci-stub for exactly the same
> > purpose, enabling a set of PCI IDs to be automatically added to the
> > driver's dynamic ID table at module load time.  The code here is
> > pretty simple and both vfio-pci and pci-stub are fairly unique in
> > being meta drivers, capable of attaching to any device, so there's no
> > attempt made to generalize the code into pci-core.
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> >  drivers/vfio/pci/vfio_pci.c |   46 +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 46 insertions(+)
> > 
> > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > index f8a1863..b3bae4c 100644
> > --- a/drivers/vfio/pci/vfio_pci.c
> > +++ b/drivers/vfio/pci/vfio_pci.c
> > @@ -32,6 +32,10 @@
> >  #define DRIVER_AUTHOR   "Alex Williamson <alex.williamson@redhat.com>"
> >  #define DRIVER_DESC     "VFIO PCI - User Level meta-driver"
> >  
> > +static char ids[1024] __initdata;
> > +module_param_string(ids, ids, sizeof(ids), 0);
> > +MODULE_PARM_DESC(ids, "Initial PCI IDs to add to the vfio driver, format is \"vendor:device[:subvendor[:subdevice[:class[:class_mask]]]]\" and multiple comma separated entries can be specified");
> > +
> >  static bool nointxmask;
> >  module_param_named(nointxmask, nointxmask, bool, S_IRUGO | S_IWUSR);
> >  MODULE_PARM_DESC(nointxmask,
> > @@ -1034,6 +1038,46 @@ static void __exit vfio_pci_cleanup(void)
> >  	vfio_pci_uninit_perm_bits();
> >  }
> >  
> > +static void __init vfio_pci_fill_ids(void)
> > +{
> > +	char *p, *id;
> > +	int rc;
> > +
> > +	/* no ids passed actually */
> > +	if (ids[0] == '\0')
> > +		return;
> > +
> > +	/* add ids specified in the module parameter */
> > +	p = ids;
> > +	while ((id = strsep(&p, ","))) {
> > +		unsigned int vendor, device, subvendor = PCI_ANY_ID,
> > +			subdevice = PCI_ANY_ID, class = 0, class_mask = 0;
> > +		int fields;
> > +
> > +		if (!strlen(id))
> > +			continue;
> > +
> > +		fields = sscanf(id, "%x:%x:%x:%x:%x:%x",
> > +				&vendor, &device, &subvendor, &subdevice,
> > +				&class, &class_mask);
> > +
> > +		if (fields < 2) {
> > +			pr_warn("vfio-pci: invalid id string \"%s\"\n", id);
> > +			continue;
> > +		}
> > +
> > +		pr_info("vfio-pci: add %04X:%04X sub=%04X:%04X cls=%08X/%08X\n",
> 
> pci_setup_device() uses "[%04x:%04x] ... class %#08x".  Maybe they should
> be the same, at least as far as using upper/lower case, spelling out
> "class", and "0x" prefix.  Maybe there's other precedent that you're
> following and pci_setup_device() is not?

Sorry for not responding to this right away.  The precedent I was
following is pci_stub_init(), but I don't know that it's any sort of
canonical reference.  PCI output in dmesg seems pretty consistent in
using lower case hex, so I agree we should make that change.  I also
note that the default value for sub IDs is PCI_ANY_ID, which ends up
getting printed as 8 characters since we don't specify the length.  I'm
also not a fan of the lack of consistency in printing the ID vs the sub
ID nor how the line wraps in dmesg w/ timestamps enabled.  What do you
think about this:

[%04hx:%04hx[%04hx:%04hx]] class %#08x/%08x

Which gives us this and doesn't wrap:

[   92.731809] vfio_pci: add [10de:11fa[ffff:ffff]] class 0x000000/00000000
[   92.738530] vfio_pci: add [10de:0e0b[ffff:ffff]] class 0x000000/00000000

I'm open to adding 0x on the class mask as well if anyone wants it.
Thanks,
Alex

> > +			vendor, device, subvendor, subdevice,
> > +			class, class_mask);
> > +
> > +		rc = pci_add_dynid(&vfio_pci_driver, vendor, device,
> > +				   subvendor, subdevice, class, class_mask, 0);
> > +		if (rc)
> > +			pr_warn("vfio-pci: failed to add dynamic id (%d)\n",
> > +				rc);
> > +	}
> > +}
> > +
> >  static int __init vfio_pci_init(void)
> >  {
> >  	int ret;
> > @@ -1053,6 +1097,8 @@ static int __init vfio_pci_init(void)
> >  	if (ret)
> >  		goto out_driver;
> >  
> > +	vfio_pci_fill_ids();
> > +
> >  	return 0;
> >  
> >  out_driver:
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/5] vfio-pci: Allow PCI IDs to be specified as module options

[Bjorn Helgaas]

On Wed, Mar 11, 2015 at 3:14 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Fri, 2015-03-06 at 16:11 -0600, Bjorn Helgaas wrote:
>> On Wed, Mar 04, 2015 at 01:02:43PM -0700, Alex Williamson wrote:
>> > This copies the same support from pci-stub for exactly the same
>> > purpose, enabling a set of PCI IDs to be automatically added to the
>> > driver's dynamic ID table at module load time.  The code here is
>> > pretty simple and both vfio-pci and pci-stub are fairly unique in
>> > being meta drivers, capable of attaching to any device, so there's no
>> > attempt made to generalize the code into pci-core.
>> >
>> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>> > ---
>> >  drivers/vfio/pci/vfio_pci.c |   46 +++++++++++++++++++++++++++++++++++++++++++
>> >  1 file changed, 46 insertions(+)
>> >
>> > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
>> > index f8a1863..b3bae4c 100644
>> > --- a/drivers/vfio/pci/vfio_pci.c
>> > +++ b/drivers/vfio/pci/vfio_pci.c
>> > @@ -32,6 +32,10 @@
>> >  #define DRIVER_AUTHOR   "Alex Williamson <alex.williamson@redhat.com>"
>> >  #define DRIVER_DESC     "VFIO PCI - User Level meta-driver"
>> >
>> > +static char ids[1024] __initdata;
>> > +module_param_string(ids, ids, sizeof(ids), 0);
>> > +MODULE_PARM_DESC(ids, "Initial PCI IDs to add to the vfio driver, format is \"vendor:device[:subvendor[:subdevice[:class[:class_mask]]]]\" and multiple comma separated entries can be specified");
>> > +
>> >  static bool nointxmask;
>> >  module_param_named(nointxmask, nointxmask, bool, S_IRUGO | S_IWUSR);
>> >  MODULE_PARM_DESC(nointxmask,
>> > @@ -1034,6 +1038,46 @@ static void __exit vfio_pci_cleanup(void)
>> >     vfio_pci_uninit_perm_bits();
>> >  }
>> >
>> > +static void __init vfio_pci_fill_ids(void)
>> > +{
>> > +   char *p, *id;
>> > +   int rc;
>> > +
>> > +   /* no ids passed actually */
>> > +   if (ids[0] == '\0')
>> > +           return;
>> > +
>> > +   /* add ids specified in the module parameter */
>> > +   p = ids;
>> > +   while ((id = strsep(&p, ","))) {
>> > +           unsigned int vendor, device, subvendor = PCI_ANY_ID,
>> > +                   subdevice = PCI_ANY_ID, class = 0, class_mask = 0;
>> > +           int fields;
>> > +
>> > +           if (!strlen(id))
>> > +                   continue;
>> > +
>> > +           fields = sscanf(id, "%x:%x:%x:%x:%x:%x",
>> > +                           &vendor, &device, &subvendor, &subdevice,
>> > +                           &class, &class_mask);
>> > +
>> > +           if (fields < 2) {
>> > +                   pr_warn("vfio-pci: invalid id string \"%s\"\n", id);
>> > +                   continue;
>> > +           }
>> > +
>> > +           pr_info("vfio-pci: add %04X:%04X sub=%04X:%04X cls=%08X/%08X\n",
>>
>> pci_setup_device() uses "[%04x:%04x] ... class %#08x".  Maybe they should
>> be the same, at least as far as using upper/lower case, spelling out
>> "class", and "0x" prefix.  Maybe there's other precedent that you're
>> following and pci_setup_device() is not?
>
> Sorry for not responding to this right away.  The precedent I was
> following is pci_stub_init(), but I don't know that it's any sort of
> canonical reference.  PCI output in dmesg seems pretty consistent in
> using lower case hex, so I agree we should make that change.  I also
> note that the default value for sub IDs is PCI_ANY_ID, which ends up
> getting printed as 8 characters since we don't specify the length.  I'm
> also not a fan of the lack of consistency in printing the ID vs the sub
> ID nor how the line wraps in dmesg w/ timestamps enabled.  What do you
> think about this:
>
> [%04hx:%04hx[%04hx:%04hx]] class %#08x/%08x
>
> Which gives us this and doesn't wrap:
>
> [   92.731809] vfio_pci: add [10de:11fa[ffff:ffff]] class 0x000000/00000000
> [   92.738530] vfio_pci: add [10de:0e0b[ffff:ffff]] class 0x000000/00000000

Looks OK to me.