[PATCH] virt-pci: add platform bus support

[PATCH] virt-pci: add platform bus support

[Vincent Whitchurch]

This driver registers PCI busses, but the underlying virtio protocol
could just as easily be used to provide a platform bus instead.  If the
virtio device node in the devicetree indicates that it's compatible with
simple-bus, register platform devices instead of handling it as a PCI
bus.

Only one platform bus is allowed and the logic MMIO region for the
platform bus is placed at an arbitrarily-chosen address away from the
PCI region.

Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---
My first approach to getting platform drivers working on UML was by
adding a minimal PCI-to-platform bridge driver, which worked without
modifications to virt-pci, but that got shot down:

 https://lore.kernel.org/lkml/20230120-simple-mfd-pci-v1-1-c46b3d6601ef@axis.com/
---
 arch/um/drivers/virt-pci.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)

diff --git a/arch/um/drivers/virt-pci.c b/arch/um/drivers/virt-pci.c
index 3ac220dafec4..0ff69d6ad253 100644
--- a/arch/um/drivers/virt-pci.c
+++ b/arch/um/drivers/virt-pci.c
@@ -8,6 +8,7 @@
 #include <linux/virtio.h>
 #include <linux/virtio_config.h>
 #include <linux/logic_iomem.h>
+#include <linux/of_platform.h>
 #include <linux/irqdomain.h>
 #include <linux/virtio_pcidev.h>
 #include <linux/virtio-uml.h>
@@ -39,6 +40,8 @@ struct um_pci_device {
 	unsigned long status;
 
 	int irq;
+
+	bool platform;
 };
 
 struct um_pci_device_reg {
@@ -48,6 +51,7 @@ struct um_pci_device_reg {
 
 static struct pci_host_bridge *bridge;
 static DEFINE_MUTEX(um_pci_mtx);
+static struct um_pci_device *um_pci_platform_device;
 static struct um_pci_device_reg um_pci_devices[MAX_DEVICES];
 static struct fwnode_handle *um_pci_fwnode;
 static struct irq_domain *um_pci_inner_domain;
@@ -480,6 +484,9 @@ static void um_pci_handle_irq_message(struct virtqueue *vq,
 	struct virtio_device *vdev = vq->vdev;
 	struct um_pci_device *dev = vdev->priv;
 
+	if (!dev->irq)
+		return;
+
 	/* we should properly chain interrupts, but on ARCH=um we don't care */
 
 	switch (msg->op) {
@@ -561,6 +568,55 @@ static int um_pci_init_vqs(struct um_pci_device *dev)
 	return 0;
 }
 
+static void __um_pci_virtio_platform_remove(struct virtio_device *vdev,
+					    struct um_pci_device *dev)
+{
+	virtio_reset_device(vdev);
+	vdev->config->del_vqs(vdev);
+
+	mutex_lock(&um_pci_mtx);
+	um_pci_platform_device = NULL;
+	mutex_unlock(&um_pci_mtx);
+
+	kfree(dev);
+}
+
+static int um_pci_virtio_platform_probe(struct virtio_device *vdev,
+					struct um_pci_device *dev)
+{
+	int ret;
+
+	dev->platform = true;
+
+	mutex_lock(&um_pci_mtx);
+
+	if (um_pci_platform_device) {
+		mutex_unlock(&um_pci_mtx);
+		ret = -EBUSY;
+		goto out_free;
+	}
+
+	ret = um_pci_init_vqs(dev);
+	if (ret) {
+		mutex_unlock(&um_pci_mtx);
+		goto out_free;
+	}
+
+	um_pci_platform_device = dev;
+
+	mutex_unlock(&um_pci_mtx);
+
+	ret = of_platform_default_populate(vdev->dev.of_node, NULL, &vdev->dev);
+	if (ret)
+		__um_pci_virtio_platform_remove(vdev, dev);
+
+	return ret;
+
+out_free:
+	kfree(dev);
+	return ret;
+}
+
 static int um_pci_virtio_probe(struct virtio_device *vdev)
 {
 	struct um_pci_device *dev;
@@ -574,6 +630,9 @@ static int um_pci_virtio_probe(struct virtio_device *vdev)
 	dev->vdev = vdev;
 	vdev->priv = dev;
 
+	if (of_device_is_compatible(vdev->dev.of_node, "simple-bus"))
+		return um_pci_virtio_platform_probe(vdev, dev);
+
 	mutex_lock(&um_pci_mtx);
 	for (i = 0; i < MAX_DEVICES; i++) {
 		if (um_pci_devices[i].dev)
@@ -623,6 +682,12 @@ static void um_pci_virtio_remove(struct virtio_device *vdev)
 	struct um_pci_device *dev = vdev->priv;
 	int i;
 
+	if (dev->platform) {
+		of_platform_depopulate(&vdev->dev);
+		__um_pci_virtio_platform_remove(vdev, dev);
+		return;
+	}
+
         /* Stop all virtqueues */
         virtio_reset_device(vdev);
         vdev->config->del_vqs(vdev);
@@ -860,6 +925,30 @@ void *pci_root_bus_fwnode(struct pci_bus *bus)
 	return um_pci_fwnode;
 }
 
+static long um_pci_map_platform(unsigned long offset, size_t size,
+				const struct logic_iomem_ops **ops,
+				void **priv)
+{
+	if (!um_pci_platform_device)
+		return -ENOENT;
+
+	*ops = &um_pci_device_bar_ops;
+	*priv = &um_pci_platform_device->resptr[0];
+
+	return 0;
+}
+
+static const struct logic_iomem_region_ops um_pci_platform_ops = {
+	.map = um_pci_map_platform,
+};
+
+static struct resource virt_platform_resource = {
+	.name = "platform",
+	.start = 0x10000000,
+	.end = 0x1fffffff,
+	.flags = IORESOURCE_MEM,
+};
+
 static int __init um_pci_init(void)
 {
 	int err, i;
@@ -868,6 +957,8 @@ static int __init um_pci_init(void)
 				       &um_pci_cfgspace_ops));
 	WARN_ON(logic_iomem_add_region(&virt_iomem_resource,
 				       &um_pci_iomem_ops));
+	WARN_ON(logic_iomem_add_region(&virt_platform_resource,
+				       &um_pci_platform_ops));
 
 	if (WARN(CONFIG_UML_PCI_OVER_VIRTIO_DEVICE_ID < 0,
 		 "No virtio device ID configured for PCI - no PCI support\n"))

---
base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
change-id: 20230127-uml-pci-platform-ef851ba75f80

Best regards,
-- 
Vincent Whitchurch <vincent.whitchurch@axis.com>


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH] virt-pci: add platform bus support

[Johannes Berg]

On Fri, 2023-01-27 at 15:30 +0100, Vincent Whitchurch wrote:
> This driver registers PCI busses, but the underlying virtio protocol
> could just as easily be used to provide a platform bus instead.  If the
> virtio device node in the devicetree indicates that it's compatible with
> simple-bus, register platform devices instead of handling it as a PCI
> bus.
> 
> Only one platform bus is allowed and the logic MMIO region for the
> platform bus is placed at an arbitrarily-chosen address away from the
> PCI region.

So ... hm. I'm not sure I _like_ this so much. It feels decidedly
strange to put it this way.

But it looks like Richard already applied it, so I suppose look at this
as kind of a retroactive information gathering. :)


> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
> ---
> My first approach to getting platform drivers working on UML was by
> adding a minimal PCI-to-platform bridge driver, which worked without
> modifications to virt-pci, but that got shot down:
> 
>  https://lore.kernel.org/lkml/20230120-simple-mfd-pci-v1-1-c46b3d6601ef@axis.com/

Reading through that ... OK that isn't fun either :-)

Sounds like there's a use case for something else though, but the PCI
IDs issue also makes that thorny.

> @@ -48,6 +51,7 @@ struct um_pci_device_reg {
>  
>  static struct pci_host_bridge *bridge;
>  static DEFINE_MUTEX(um_pci_mtx);
> +static struct um_pci_device *um_pci_platform_device;
>  static struct um_pci_device_reg um_pci_devices[MAX_DEVICES];
>  static struct fwnode_handle *um_pci_fwnode;
>  static struct irq_domain *um_pci_inner_domain;
> @@ -480,6 +484,9 @@ static void um_pci_handle_irq_message(struct virtqueue *vq,
>  	struct virtio_device *vdev = vq->vdev;
>  	struct um_pci_device *dev = vdev->priv;
>  
> +	if (!dev->irq)
> +		return;
> 

Does that mean platform devices don't have interrupts, or does that mean
not all of them must have interrupts?

I'll note that this also would allow the device to send an MSI which
feels a bit wrong? But I guess it doesn't really matter.


So let me ask this: Conceptually, wouldn't the "right" way to handle
this be a new virtio device and protocol and everything, with a new
driver to handle it? I realise that would likely lead to quite a bit of
code duplication, for now I just want to understand the concept here a
bit better.

Such a driver would then define its own messages, requiring (I guess)
the equivalents of
 * VIRTIO_PCIDEV_OP_INT,
 * VIRTIO_PCIDEV_OP_MMIO_READ,
 * VIRTIO_PCIDEV_OP_MMIO_WRITE, and
 * (maybe) VIRTIO_PCIDEV_OP_MMIO_MEMSET,

but not
 * VIRTIO_PCIDEV_OP_CFG_READ,
 * VIRTIO_PCIDEV_OP_CFG_WRITE,
 * VIRTIO_PCIDEV_OP_MSI and
 * VIRTIO_PCIDEV_OP_PME,

right?

How much code would we actually duplicate? Most of virt-pci is dedicated
to the mess of PCI MSI domains, bridges, etc.

The limitation to a single device feels odd, and the fact that you have
to put it into DT under the PCI also seems odd. OTOH, the platform
device has to be _somewhere_ right?

I guess I'm not really sure how to use it, but I suppose that's OK too
:)

Thanks,
johannes

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH] virt-pci: add platform bus support

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Johannes Berg" <johannes@sipsolutions.net>
> An: "Vincent Whitchurch" <vincent.whitchurch@axis.com>, "richard" <richard@nod.at>, "anton ivanov"
> <anton.ivanov@cambridgegreys.com>
> CC: robh@kernel.org, devicetree@lists.infradead.org, "linux-um" <linux-um@lists.infradead.org>, "linux-kernel"
> <linux-kernel@vger.kernel.org>, "kernel" <kernel@axis.com>
> Gesendet: Montag, 13. Februar 2023 18:54:49
> Betreff: Re: [PATCH] virt-pci: add platform bus support

> On Fri, 2023-01-27 at 15:30 +0100, Vincent Whitchurch wrote:
>> This driver registers PCI busses, but the underlying virtio protocol
>> could just as easily be used to provide a platform bus instead.  If the
>> virtio device node in the devicetree indicates that it's compatible with
>> simple-bus, register platform devices instead of handling it as a PCI
>> bus.
>> 
>> Only one platform bus is allowed and the logic MMIO region for the
>> platform bus is placed at an arbitrarily-chosen address away from the
>> PCI region.
> 
> So ... hm. I'm not sure I _like_ this so much. It feels decidedly
> strange to put it this way.
> 
> But it looks like Richard already applied it, so I suppose look at this
> as kind of a retroactive information gathering. :)

I liked the idea, the patch made sense, so yes.
But this does not mean that things can't be changed.
Until we release 6.3 we have time.

Thanks,
//richard

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH] virt-pci: add platform bus support

[Vincent Whitchurch]

On Mon, Feb 13, 2023 at 06:54:49PM +0100, Johannes Berg wrote:
> On Fri, 2023-01-27 at 15:30 +0100, Vincent Whitchurch wrote:
> > My first approach to getting platform drivers working on UML was by
> > adding a minimal PCI-to-platform bridge driver, which worked without
> > modifications to virt-pci, but that got shot down:
> > 
> >  https://lore.kernel.org/lkml/20230120-simple-mfd-pci-v1-1-c46b3d6601ef@axis.com/
> 
> Reading through that ... OK that isn't fun either :-)
> 
> Sounds like there's a use case for something else though, but the PCI
> IDs issue also makes that thorny.

Yes, Greg was initially totally opposed to the idea of putting platform
devices under PCI devices, but in his latest email he seemed to
allow it in some cases.  It's still unclear if he'd be OK with a
"virtual PCI-to-platform bridge" though.  And yes, adding platform
devices support like in this patch removes one layer and also eliminates
the disadvantage of having to wait for user space to specify a PCI ID
for the bridge device.

> > @@ -48,6 +51,7 @@ struct um_pci_device_reg {
> >  
> >  static struct pci_host_bridge *bridge;
> >  static DEFINE_MUTEX(um_pci_mtx);
> > +static struct um_pci_device *um_pci_platform_device;
> >  static struct um_pci_device_reg um_pci_devices[MAX_DEVICES];
> >  static struct fwnode_handle *um_pci_fwnode;
> >  static struct irq_domain *um_pci_inner_domain;
> > @@ -480,6 +484,9 @@ static void um_pci_handle_irq_message(struct virtqueue *vq,
> >  	struct virtio_device *vdev = vq->vdev;
> >  	struct um_pci_device *dev = vdev->priv;
> >  
> > +	if (!dev->irq)
> > +		return;
> > 
> 
> Does that mean platform devices don't have interrupts, or does that mean
> not all of them must have interrupts?

They don't have interrupts via this driver.  There isn't any standard
way for platform devices to handle interrupts since it it all depends on
what interrupt-parent is specified in the devicetree and how that is
implemented.

In my case, I have a separate virtio-gpio and use that as the
interrupt-parent like in the devicetree at the end of this email.  I
actually also did that when I used the platform-on-PCI solution since I
already use virtio-gpio as the interrupt controller for devices on other
busses like I2C and SPI and just reusing that was easier than
implementing MSI support in my virt-pci device.

> I'll note that this also would allow the device to send an MSI which
> feels a bit wrong? But I guess it doesn't really matter.

We could avoid setting up the IRQ/MSI virtqueue when we know we're
dealing with platform devices.

> So let me ask this: Conceptually, wouldn't the "right" way to handle
> this be a new virtio device and protocol and everything, with a new
> driver to handle it? I realise that would likely lead to quite a bit of
> code duplication, for now I just want to understand the concept here a
> bit better.

Yes, that could be a way to do it.  Or there could perhaps be some
feature bits indicating that only MMIO read/write/memset are allowed.

> Such a driver would then define its own messages, requiring (I guess)
> the equivalents of
>  * VIRTIO_PCIDEV_OP_INT,
>  * VIRTIO_PCIDEV_OP_MMIO_READ,
>  * VIRTIO_PCIDEV_OP_MMIO_WRITE, and
>  * (maybe) VIRTIO_PCIDEV_OP_MMIO_MEMSET,
> 
> but not
>  * VIRTIO_PCIDEV_OP_CFG_READ,
>  * VIRTIO_PCIDEV_OP_CFG_WRITE,
>  * VIRTIO_PCIDEV_OP_MSI and
>  * VIRTIO_PCIDEV_OP_PME,
> 
> right?

Yes, just the MMIO* stuff would be enough.

> How much code would we actually duplicate? Most of virt-pci is dedicated
> to the mess of PCI MSI domains, bridges, etc.

Probably not a huge amount, I can try to cook up a patch if you'd like.
But, besides the code duplication, I'm not sure if adding another new
virtio driver without a specification would be OK?

> The limitation to a single device feels odd, and the fact that you have

The limitation to a single device here is not a problem since one can
use simple-bus to instantiate any number of platform devices via the
devicetree.  The devicetree at the end of this email shows how that
looks like.

> to put it into DT under the PCI also seems odd. OTOH, the platform
> device has to be _somewhere_ right?

With this patch, it's not put under PCI in the devicetree, see the
example below.

> I guess I'm not really sure how to use it, but I suppose that's OK too
> :)

With a devicetree like the one below, using it shouldn't be all that
different from using the normal virt-pci except that the register ranges
and IRQ information are in the devicetree rather than coming via the
config space implementation in the virtio device.

With everything hooked up and with the backend code I have in roadtest,
this allows tests like this one which exercises some SoC's DMA
controller (the simplest one I could find in the tree):
 https://github.com/vwax/linux/commit/337e03bb3c1796c96ab8f397ff36e9b543f2c6d5

Here's the devicetree example I mentioned above:

 / {
     #address-cells = <2>;
     #size-cells = <2>;
 
     virtio@0 {
         compatible = "virtio,uml";
         socket-path = ".roadtest/roadtest-work/0/gpio.sock";
         virtio-device-id = <0x29>;
 
         gpio: gpio {
             compatible = "virtio,device29";
 
             gpio-controller;
             #gpio-cells = <2>;
 
             interrupt-controller;
             #interrupt-cells = <2>;
         };
     };
 
     virtio@2 {
         compatible = "virtio,uml";
         socket-path = ".roadtest/roadtest-work/0/platform.sock";
         virtio-device-id = <1234>;
         ranges;
 
         platform: bus@0,0 {
                 compatible = "virtio,device4d2", "simple-bus";
                 reg = <0x00000 0x10000000 0x0 0x10000>;
                 interrupt-parent = <&gpio>;
		 // Could use range translation here to avoid absolute
		 // addresses in reg properties
                 ranges;
         };
     };
 
 };
 
 &platform {
     foo@10001000 {
             compatible = "some-platform-device-complatible";
             reg = <0x00000 0x10001000 0 0x1000>;
             interrupts = <2 4>;
     };

     bar@10002000 {
             compatible = "another-platform-device-complatible";
             reg = <0x00000 0x10002000 0 0x1000>;
             interrupts = <3 4>;
     };
 };


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH] virt-pci: add platform bus support

[Johannes Berg]

On Tue, 2023-02-14 at 13:12 +0100, Vincent Whitchurch wrote:
> 
> Yes, Greg was initially totally opposed to the idea of putting platform
> devices under PCI devices, but in his latest email he seemed to
> allow it in some cases.  It's still unclear if he'd be OK with a
> "virtual PCI-to-platform bridge" though.  And yes, adding platform
> devices support like in this patch removes one layer and also eliminates
> the disadvantage of having to wait for user space to specify a PCI ID
> for the bridge device.

Right.

> > > +	if (!dev->irq)
> > > +		return;
> > > 
> > 
> > Does that mean platform devices don't have interrupts, or does that mean
> > not all of them must have interrupts?
> 
> They don't have interrupts via this driver.  There isn't any standard
> way for platform devices to handle interrupts since it it all depends on
> what interrupt-parent is specified in the devicetree and how that is
> implemented.

Ah, OK.

> > I'll note that this also would allow the device to send an MSI which
> > feels a bit wrong? But I guess it doesn't really matter.
> 
> We could avoid setting up the IRQ/MSI virtqueue when we know we're
> dealing with platform devices.

Not sure it matters then?

> > So let me ask this: Conceptually, wouldn't the "right" way to handle
> > this be a new virtio device and protocol and everything, with a new
> > driver to handle it? I realise that would likely lead to quite a bit of
> > code duplication, for now I just want to understand the concept here a
> > bit better.
> 
> Yes, that could be a way to do it.  Or there could perhaps be some
> feature bits indicating that only MMIO read/write/memset are allowed.

Right.

> > How much code would we actually duplicate? Most of virt-pci is dedicated
> > to the mess of PCI MSI domains, bridges, etc.
> 
> Probably not a huge amount, I can try to cook up a patch if you'd like.
> But, besides the code duplication, I'm not sure if adding another new
> virtio driver without a specification would be OK?

Yeah ... let's not worry. Was mostly trying to understand it better.

I'm not really bothered by it :)

> > The limitation to a single device feels odd, and the fact that you have
> 
> The limitation to a single device here is not a problem since one can
> use simple-bus to instantiate any number of platform devices via the
> devicetree.  The devicetree at the end of this email shows how that
> looks like.

OK cool.

> With a devicetree like the one below, using it shouldn't be all that
> different from using the normal virt-pci except that the register ranges
> and IRQ information are in the devicetree rather than coming via the
> config space implementation in the virtio device.

Makes sense.

Thanks for all the answers & examples! Let's just leave it as is then :)

johannes

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH] virt-pci: add platform bus support

[Rob Herring]

On Tue, Feb 14, 2023 at 01:12:25PM +0100, Vincent Whitchurch wrote:
> On Mon, Feb 13, 2023 at 06:54:49PM +0100, Johannes Berg wrote:
> > On Fri, 2023-01-27 at 15:30 +0100, Vincent Whitchurch wrote:
> > > My first approach to getting platform drivers working on UML was by
> > > adding a minimal PCI-to-platform bridge driver, which worked without
> > > modifications to virt-pci, but that got shot down:
> > > 
> > >  https://lore.kernel.org/lkml/20230120-simple-mfd-pci-v1-1-c46b3d6601ef@axis.com/
> > 
> > Reading through that ... OK that isn't fun either :-)
> > 
> > Sounds like there's a use case for something else though, but the PCI
> > IDs issue also makes that thorny.
> 
> Yes, Greg was initially totally opposed to the idea of putting platform
> devices under PCI devices, but in his latest email he seemed to
> allow it in some cases.  It's still unclear if he'd be OK with a
> "virtual PCI-to-platform bridge" though.  And yes, adding platform
> devices support like in this patch removes one layer and also eliminates
> the disadvantage of having to wait for user space to specify a PCI ID
> for the bridge device.

Like I said in that thread, we have multiple usecases needing something 
similar for non-discoverable MMIO devices behind a PCI device. And I 
convinced Greg a platform device was okay, so please continue that path.

I'm adding you to the thread of other usecases.

Rob

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um