diff mbox series

[V3,1/3] gpio: Add virtio-gpio driver

Message ID 10442926ae8a65f716bfc23f32339a6b35e51d5a.1623326176.git.viresh.kumar@linaro.org
State New
Headers show
Series gpio: Add virtio based driver | expand

Commit Message

Viresh Kumar June 10, 2021, 12:16 p.m. UTC
From: "Enrico Weigelt, metux IT consult" <info@metux.net>


This patch adds a new driver for Virtio based GPIO devices.

This allows a guest VM running Linux to access GPIO device provided by
the host. It supports all basic operations for now, except interrupts
for the GPIO lines.

Signed-off-by: "Enrico Weigelt, metux IT consult" <info@metux.net>

Co-developed-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

---
 drivers/gpio/Kconfig             |   9 +
 drivers/gpio/Makefile            |   1 +
 drivers/gpio/gpio-virtio.c       | 326 +++++++++++++++++++++++++++++++
 include/uapi/linux/virtio_gpio.h |  41 ++++
 include/uapi/linux/virtio_ids.h  |   1 +
 5 files changed, 378 insertions(+)
 create mode 100644 drivers/gpio/gpio-virtio.c
 create mode 100644 include/uapi/linux/virtio_gpio.h

-- 
2.31.1.272.g89b43f80a514

Comments

Arnd Bergmann June 10, 2021, 1:22 p.m. UTC | #1
On Thu, Jun 10, 2021 at 2:18 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>

> From: "Enrico Weigelt, metux IT consult" <info@metux.net>

>

> This patch adds a new driver for Virtio based GPIO devices.

>

> This allows a guest VM running Linux to access GPIO device provided by

> the host. It supports all basic operations for now, except interrupts

> for the GPIO lines.

>

> Signed-off-by: "Enrico Weigelt, metux IT consult" <info@metux.net>

> Co-developed-by: Viresh Kumar <viresh.kumar@linaro.org>

> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>


Can you give an example of how this would be hooked up to other drivers
using those gpios. Can you give an example of how using the "gpio-keys" or
"gpio-leds" drivers in combination with virtio-gpio looks like in the DT?

Would qemu simply add the required DT properties to the device node that
corresponds to the virtio device in this case?

From what I can tell, both the mmio and pci variants of virtio can have their
dev->of_node populated, but I don't see the logic in register_virtio_device()
that looks up the of_node of the virtio_device that the of_gpio code then
tries to refer to.

        Arnd
Enrico Weigelt, metux IT consult June 10, 2021, 3:54 p.m. UTC | #2
On 10.06.21 14:16, Viresh Kumar wrote:
> From: "Enrico Weigelt, metux IT consult" <info@metux.net>

> 

> This patch adds a new driver for Virtio based GPIO devices.

> 

> This allows a guest VM running Linux to access GPIO device provided by

> the host. It supports all basic operations for now, except interrupts

> for the GPIO lines.


What exactly did you change here from my original driver ?

Your already changed the spec havily (w/o consulting me first), so I
guess this driver hasn't so much in common w/ my original design.

Note that I made my original design decisions for good reaons
(see virtio-dev list). It already support async notifications
(IOW: irqs), had an easy upgrade path for future additions, etc.

Note #2: it's already implemented and running in the field (different
kernels, different hypervisors, ...) - it just lacked the going through
virtio comitte's formal specification process, which blocked mainlining.

Is there anything fundamentally wrong w/ my original design, why you
invented a completely new one ?


--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
Enrico Weigelt, metux IT consult June 10, 2021, 4 p.m. UTC | #3
On 10.06.21 15:22, Arnd Bergmann wrote:

> Can you give an example of how this would be hooked up to other drivers

> using those gpios. Can you give an example of how using the "gpio-keys" or

> "gpio-leds" drivers in combination with virtio-gpio looks like in the DT?


Connecting between self-probing bus'es and DT is generally tricky. IMHO
we don't have any generic mechanism for that.

I've made a few attempts, but nothing practically useful, which would be
accepted by the corresponding maintainers, yet. We'd either need some
very special logic in DT probing or pseudo-bus'es for the mapping.
(DT wants to do those connections via phandle's, which in turn need the
referenced nodes to be present in the DT).

>  From what I can tell, both the mmio and pci variants of virtio can have their

> dev->of_node populated, but I don't see the logic in register_virtio_device()

> that looks up the of_node of the virtio_device that the of_gpio code then

> tries to refer to.


Have you ever successfully bound a virtio device via DT ?


--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
Viresh Kumar June 10, 2021, 4:57 p.m. UTC | #4
Hi Enrico,

On Thu, 10 Jun 2021 at 21:24, Enrico Weigelt, metux IT consult
<lkml@metux.net> wrote:
> On 10.06.21 14:16, Viresh Kumar wrote:

> > From: "Enrico Weigelt, metux IT consult" <info@metux.net>

> >

> > This patch adds a new driver for Virtio based GPIO devices.

> >

> > This allows a guest VM running Linux to access GPIO device provided by

> > the host. It supports all basic operations for now, except interrupts

> > for the GPIO lines.

>

> What exactly did you change here from my original driver ?


I didn't write it from scratch and used your patch only to start with, and so
you are still the Author of this particular patch.

This just followed the specification updates and so changes only the stuff
that was different from your original specs. Apart from that as you know,
the irqs weren't working in your case as they needed to be implemented
differently (patch 2 does that) here.

> Your already changed the spec havily (w/o consulting me first),


Isn't that what we are doing on the list? I believe that's why the lists
exist, so people don't need to discuss in person, offline. I am happy to
make changes in spec if you want to suggest something and if something
breaks it for you.

> so I guess this driver hasn't so much in common w/ my original design.


It actually has as I said earlier, it is still based on your patch.

> Note that I made my original design decisions for good reaons

> (see virtio-dev list).


I tried to follow your patches from December on the Linux kernel list
and the ID allocation one on virtio-comments list. I wasn't able to search
for any other attempt at sending the specification, so may have missed
something.

I do understand that there were reasons why you (and me) chose a
particular way of doing things and if there is a good reason of following
that, then we can still modify the spec and fix things for everyone.
We just need to discuss our reasoning on the list and get through with
a specfication which works for everyone as this will become a standard
interface for many in the future and it needs to be robust and efficient
for everyone.

> It already support async notifications

> (IOW: irqs), had an easy upgrade path for future additions, etc.


I haven't changed irqs path, we still have a separate virtqueue
(named "interrupt" vq) which handles just the interrupts now. And so
will be faster than what you initially suggested.

In your original design you also received the responses for the requests
on this virtqueue, wihch I have changed to get the response synchronously
on the "command" virtqueue only.

This is from one of your earlier replies:

"
I've been under the impression that queues only work in only one
direction. (at least that's what my web research was telling).

Could you please give an example how bi-directional transmission within
the same queue could look like ?
"

It is actually possible and the right thing to do here as we aren't starting
multiple requests together. The operation needs to be synchronous anyway
and both request/response on the same channel work better there. Also that
makes the interrupts reach faster, without additional delay due to responses.

> Note #2: it's already implemented and running in the field (different

> kernels, different hypervisors, ...) - it just lacked the going through

> virtio comitte's formal specification process, which blocked mainlining.


I understand the pain you would be reqiured to go through because of this,
but this is how any open source community will work, like Linux.

There will be changes in specification and code once you post it and any
solutions already working in the field won't really matter during the
discussion.
That is why it is always the right thing to _upstream first_, so one can avoid
these problems later on. Though I understand that the real world
needs to move faster than community. But no one can really help in that.

> Is there anything fundamentally wrong w/ my original design, why you

> invented a completely new one ?


Not much, and I haven't changed a lot as well.

Lemme point out few things which have changed in specification since
your earlier
version (the code just followed the specification, that's it).

- config structure
  - version information is replaced with virtio-features.
  - Irq support is added as a feature, as you suggested.
  - extra padding space (24 bytes) removed. If you see this patch we can
    now read the entire config structure in a single go. Why should
anyone be required
    to copy extra 24 bytes? If we need more fields later, we can
always do that with help
    of another feature-flag. So this is still extendable.

- Virtqueues: we still have two virtqueues (command and interrupt),
just responses are
  moved to the command virtqueue only, as that is more efficient.

- Request/response: Request layout is same, added a new layout for response as
  the same layout is inefficient.

- IRQ support: Initial specification had no interface for configuring
irqs from the guest,
  I added that.

That's all I can gather right now.

I don't think that's a lot and it is mostly improvements only. But if
there is a good reason
on why we should do things differently, we can still discuss that. I
am open to suggestions.

Thanks

--
Viresh
Jean-Philippe Brucker June 10, 2021, 5:03 p.m. UTC | #5
On Thu, Jun 10, 2021 at 04:00:39PM +0000, Enrico Weigelt, metux IT consult via Stratos-dev wrote:
> On 10.06.21 15:22, Arnd Bergmann wrote:

> 

> > Can you give an example of how this would be hooked up to other drivers

> > using those gpios. Can you give an example of how using the "gpio-keys" or

> > "gpio-leds" drivers in combination with virtio-gpio looks like in the DT?

> 

> Connecting between self-probing bus'es and DT is generally tricky. IMHO

> we don't have any generic mechanism for that.


DT does have a generic description of PCI endpoints, which virtio-iommu
relies on to express the relation between IOMMU and endpoint nodes [1].
I think the problem here is similar: the client node needs a phandle to
the GPIO controller which may use virtio-pci transport?

Note that it mostly works if the device is on the root PCI bus. Behind a
bridge the OS may change the device's bus number as needed, so the BDF
reference in DT is only valid if the software providing the DT description
(VMM or firmware) initializes bus numbers accordingly (and I don't
remember if Linux supports this case well).

Thanks,
Jean

[1] Documentation/devicetree/bindings/virtio/iommu.txt

> 

> I've made a few attempts, but nothing practically useful, which would be

> accepted by the corresponding maintainers, yet. We'd either need some

> very special logic in DT probing or pseudo-bus'es for the mapping.

> (DT wants to do those connections via phandle's, which in turn need the

> referenced nodes to be present in the DT).

> 

> >  From what I can tell, both the mmio and pci variants of virtio can have their

> > dev->of_node populated, but I don't see the logic in register_virtio_device()

> > that looks up the of_node of the virtio_device that the of_gpio code then

> > tries to refer to.

> 

> Have you ever successfully bound a virtio device via DT ?

> 

> 

> --mtx

> 

> -- 

> ---

> Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert

> werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren

> GPG/PGP-Schlüssel zu.

> ---

> Enrico Weigelt, metux IT consult

> Free software and Linux embedded engineering

> info@metux.net -- +49-151-27565287

> -- 

> Stratos-dev mailing list

> Stratos-dev@op-lists.linaro.org

> https://op-lists.linaro.org/mailman/listinfo/stratos-dev
Arnd Bergmann June 10, 2021, 7:41 p.m. UTC | #6
On Thu, Jun 10, 2021 at 7:03 PM Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>

> On Thu, Jun 10, 2021 at 04:00:39PM +0000, Enrico Weigelt, metux IT consult via Stratos-dev wrote:

> > On 10.06.21 15:22, Arnd Bergmann wrote:

> >

> > > Can you give an example of how this would be hooked up to other drivers

> > > using those gpios. Can you give an example of how using the "gpio-keys" or

> > > "gpio-leds" drivers in combination with virtio-gpio looks like in the DT?

> >

> > Connecting between self-probing bus'es and DT is generally tricky. IMHO

> > we don't have any generic mechanism for that.

>

> DT does have a generic description of PCI endpoints, which virtio-iommu

> relies on to express the relation between IOMMU and endpoint nodes [1].

> I think the problem here is similar: the client node needs a phandle to

> the GPIO controller which may use virtio-pci transport?


Right, the code to set dev->of_node is fairly simple, the device probe
just needs to scan for child nodes. Aside from PCI, similar code exists
for USB and MMC/SDIO, which are usually discoverable but sometimes
need additional properties.

> Note that it mostly works if the device is on the root PCI bus. Behind a

> bridge the OS may change the device's bus number as needed, so the BDF

> reference in DT is only valid if the software providing the DT description

> (VMM or firmware) initializes bus numbers accordingly (and I don't

> remember if Linux supports this case well).


I think you can mark the host bridge as "probe-only" to prevent the OS
(at least Linux) from renumbering the buses.

The part I did not find though is assigning dev->of_node in the virtio_device
to a child of the PCI device node.

      Arnd
Linus Walleij June 10, 2021, 8:46 p.m. UTC | #7
Hi Viresh!

thanks for working on this, it's a really interesting driver.

My first question is conceptual:

We previously have Geerts driver for virtualization:
drivers/gpio/gpio-aggregator.c

The idea with the aggregator is that a host script sets up a
unique gpiochip for the virtualized instance using some poking
in sysfs and pass that to the virtual machine.
So this is Linux acting as virtualization host by definition.

I think virtio is more abstract and intended for the usecase
where the hypervisor is not Linux, so this should be mentioned
in the commit, possibly also in Kconfig so users immediately
know what usecases the two different drivers are for.

Possibly both could be used: aggregator to pick out the GPIOs
you want into a synthetic GPIO chip, and the actual talk
between the hypervisor/host and the guest using virtio, even
with linux-on-linux.

Yet another usecase would be to jit this with remoteproc/rpmsg
and let a specific signal processor or real-time executive on
another CPU with a few GPIOs around present these to
Linux using this mechanism. Well that would certainly interest
Bjorn and other rpmsg stakeholders, so they should have
a look so that this provides what they need they day they
need it. (CCed Bjorn and also Google who may want this for
their Android emulators.)

On Thu, Jun 10, 2021 at 2:16 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:

> +static const char **parse_gpio_names(struct virtio_device *vdev,

> +                              struct virtio_gpio_config *config)


I really like this end-to-end plug-and-play that even provides
the names over virtio.

I think my patch to the gpiolib to make it mandatory for names to
be unique per-chip made it in, but please make that part of the spec
so that we don't get the problem with non-unique names here.

I suppose the spec can be augmented later to also accept config
settings like open drain pull up/down etc but no need to specify
more than the basic for now.

But to be able to add more in the future, the client needs some
kind of query mechanism or version number so the driver can
adapt and not announce something the underlying virtio device
cannot do. Do we have this? A bitmask for features, a version
number that increase monotonically for new features to be
presented or similar?

Because otherwise we have to bump this:
+#define VIRTIO_ID_GPIO                 41 /* virtio GPIO */

every time we add something new (and we will).

Yours,
Linus Walleij
Viresh Kumar June 11, 2021, 3:56 a.m. UTC | #8
Hi Linus,

On 10-06-21, 22:46, Linus Walleij wrote:
> Hi Viresh!

> 

> thanks for working on this, it's a really interesting driver.

> 

> My first question is conceptual:

> 

> We previously have Geerts driver for virtualization:

> drivers/gpio/gpio-aggregator.c

> 

> The idea with the aggregator is that a host script sets up a

> unique gpiochip for the virtualized instance using some poking

> in sysfs and pass that to the virtual machine.

> So this is Linux acting as virtualization host by definition.

> 

> I think virtio is more abstract and intended for the usecase

> where the hypervisor is not Linux, so this should be mentioned

> in the commit, possibly also in Kconfig so users immediately

> know what usecases the two different drivers are for.


Well, not actually.

The host can actually be anything. It can be a Xen based dom0, which
runs some proprietary firmware, or Qemu running over Linux.

It is left for the host to decide how it wants to club together the
GPIO pins from host and access them, with Linux host userspace it
would be playing with /dev/gpiochipN, while for a raw one it may
be accessing registers directly.

And so the backend running at host, needs to pass the gpiochip
configurations and only the host understand it.

The way I test it for now is by running this with Qemu over my x86
box, so my host side is indeed playing with sysfs Linux.

> Possibly both could be used: aggregator to pick out the GPIOs

> you want into a synthetic GPIO chip, and the actual talk

> between the hypervisor/host and the guest using virtio, even

> with linux-on-linux.


Not sure if I understand the aggregator thing for now, but we see the
backend running at host (which talks to this Linux driver at guest) as
a userspace thing and not a kernel driver. Not sure if aggregator can
be used like that, but anyway..

> Yet another usecase would be to jit this with remoteproc/rpmsg

> and let a specific signal processor or real-time executive on

> another CPU with a few GPIOs around present these to

> Linux using this mechanism. Well that would certainly interest

> Bjorn and other rpmsg stakeholders, so they should have

> a look so that this provides what they need they day they

> need it. (CCed Bjorn and also Google who may want this for

> their Android emulators.)


I am not very clear on the rpmsg thing, I know couple of folks at
project Stratos were talking about it :)

@Alex, want to chime in here for me ? :)

> On Thu, Jun 10, 2021 at 2:16 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:

> 

> > +static const char **parse_gpio_names(struct virtio_device *vdev,

> > +                              struct virtio_gpio_config *config)

> 

> I really like this end-to-end plug-and-play that even provides

> the names over virtio.


The credit goes to Enrico for this :)

> I think my patch to the gpiolib to make it mandatory for names to

> be unique per-chip made it in, but please make that part of the spec

> so that we don't get the problem with non-unique names here.


Oh, that's nice. I will surely do that.

> I suppose the spec can be augmented later to also accept config

> settings like open drain pull up/down etc but no need to specify

> more than the basic for now.


That's the plan.

> But to be able to add more in the future, the client needs some

> kind of query mechanism or version number so the driver can

> adapt and not announce something the underlying virtio device

> cannot do. Do we have this? A bitmask for features, a version

> number that increase monotonically for new features to be

> presented or similar?

> 

> Because otherwise we have to bump this:

> +#define VIRTIO_ID_GPIO                 41 /* virtio GPIO */

> 

> every time we add something new (and we will).


Yes, Virtio presents features for this. The patch 2/3 already uses one
for IRQs. We won't need to bump up the IDs :)

-- 
viresh
Geert Uytterhoeven June 11, 2021, 7:42 a.m. UTC | #9
Hi Viresh, Linus,

On Fri, Jun 11, 2021 at 5:56 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 10-06-21, 22:46, Linus Walleij wrote:

> > thanks for working on this, it's a really interesting driver.

> >

> > My first question is conceptual:

> >

> > We previously have Geerts driver for virtualization:

> > drivers/gpio/gpio-aggregator.c

> >

> > The idea with the aggregator is that a host script sets up a

> > unique gpiochip for the virtualized instance using some poking

> > in sysfs and pass that to the virtual machine.

> > So this is Linux acting as virtualization host by definition.


The gpio-aggregator is running on the host...

> > I think virtio is more abstract and intended for the usecase

> > where the hypervisor is not Linux, so this should be mentioned

> > in the commit, possibly also in Kconfig so users immediately

> > know what usecases the two different drivers are for.


... while the virtio-gpio driver is meant for the guest kernel.

I my PoC "[PATCH QEMU v2 0/5] Add a GPIO backend"[1], I didn't have
a virtio transport, but just hooked into the PL061 GPIO emulation
in QEMU.  The PL061 QEMU driver talked to the GPIO backend, which
talked to /dev/gpiochipN on the host.

> Well, not actually.

>

> The host can actually be anything. It can be a Xen based dom0, which

> runs some proprietary firmware, or Qemu running over Linux.

>

> It is left for the host to decide how it wants to club together the

> GPIO pins from host and access them, with Linux host userspace it

> would be playing with /dev/gpiochipN, while for a raw one it may

> be accessing registers directly.

>

> And so the backend running at host, needs to pass the gpiochip

> configurations and only the host understand it.


So QEMU has to translate the virtio-gpio communication to e.g.
/dev/gpiochipN on the host (or a different backend on non-Linux or
bare-metal HV).

> The way I test it for now is by running this with Qemu over my x86

> box, so my host side is indeed playing with sysfs Linux.


Can you please share a link to the QEMU patches?

> > Possibly both could be used: aggregator to pick out the GPIOs

> > you want into a synthetic GPIO chip, and the actual talk

> > between the hypervisor/host and the guest using virtio, even

> > with linux-on-linux.

>

> Not sure if I understand the aggregator thing for now, but we see the

> backend running at host (which talks to this Linux driver at guest) as

> a userspace thing and not a kernel driver. Not sure if aggregator can

> be used like that, but anyway..


The GPIO aggregator came into play after talking to Alexander Graf and
Peter Maydell.  To reduce the attack surface, they didn't want QEMU
to be responsible for exporting to the guest a subset of all GPIOs of
a gpiochip, only a full gpiochip.  However, the full gpiochip may
contain critical GPIOs you do not want the guest to tamper with.
Hence the GPIO aggregator was born, to take care of aggregating all
GPIOs you want to export to a guest into a new virtual gpiochip.

You can find more information about the GPIO Aggregator's use cases in
"[PATCH v7 0/6] gpio: Add GPIO Aggregator"[2].

[1] https://lore.kernel.org/linux-gpio/20200423090118.11199-1-geert+renesas@glider.be
[2] https://lore.kernel.org/linux-doc/20200511145257.22970-1-geert+renesas@glider.be/

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Viresh Kumar June 11, 2021, 8:01 a.m. UTC | #10
On 11-06-21, 09:42, Geert Uytterhoeven wrote:
> Hi Viresh, Linus,

> 

> On Fri, Jun 11, 2021 at 5:56 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:

> > On 10-06-21, 22:46, Linus Walleij wrote:

> > > thanks for working on this, it's a really interesting driver.

> > >

> > > My first question is conceptual:

> > >

> > > We previously have Geerts driver for virtualization:

> > > drivers/gpio/gpio-aggregator.c

> > >

> > > The idea with the aggregator is that a host script sets up a

> > > unique gpiochip for the virtualized instance using some poking

> > > in sysfs and pass that to the virtual machine.

> > > So this is Linux acting as virtualization host by definition.

> 

> The gpio-aggregator is running on the host...

> 

> > > I think virtio is more abstract and intended for the usecase

> > > where the hypervisor is not Linux, so this should be mentioned

> > > in the commit, possibly also in Kconfig so users immediately

> > > know what usecases the two different drivers are for.

> 

> ... while the virtio-gpio driver is meant for the guest kernel.

> 

> I my PoC "[PATCH QEMU v2 0/5] Add a GPIO backend"[1], I didn't have

> a virtio transport, but just hooked into the PL061 GPIO emulation

> in QEMU.  The PL061 QEMU driver talked to the GPIO backend, which

> talked to /dev/gpiochipN on the host.


Hmm, interesting.

> > Well, not actually.

> >

> > The host can actually be anything. It can be a Xen based dom0, which

> > runs some proprietary firmware, or Qemu running over Linux.

> >

> > It is left for the host to decide how it wants to club together the

> > GPIO pins from host and access them, with Linux host userspace it

> > would be playing with /dev/gpiochipN, while for a raw one it may

> > be accessing registers directly.

> >

> > And so the backend running at host, needs to pass the gpiochip

> > configurations and only the host understand it.

> 

> So QEMU has to translate the virtio-gpio communication to e.g.

> /dev/gpiochipN on the host (or a different backend on non-Linux or

> bare-metal HV).


No, QEMU passes the raw messages to the backend daemon running in host
userspace (which shares a socket with qemu). The backend understands
the virtio/vhost protocols and so won't be required to change at all
if we move from Qemu to something else. And that's what we (Linaro)
are looking to do here with Project Stratos.

Create virtio based hypervisor agnostic backends.

> > The way I test it for now is by running this with Qemu over my x86

> > box, so my host side is indeed playing with sysfs Linux.

> 

> Can you please share a link to the QEMU patches?


Unfortunately, they aren't in good shape right now and the backend is
a bit hacky (Just checking the data paths, but not touching
/dev/gpiochipN at all for now).

I didn't implement one as I am going to implement the backend in Rust
and not Qemu. So it doesn't depend on Qemu at all.

To give you an idea of the whole thing, here is what we have done for
I2c for example, GPIO one will look very similar.

The Qemu patches:

https://yhbt.net/lore/all/cover.1617278395.git.viresh.kumar@linaro.org/T/

The stuff from tools/vhost-user-i2c/ directory (or patch 4/6) isn't
used anymore and the following Rust implementation replaces it:

https://github.com/vireshk/vhost-device/tree/master/src/i2c

I can share the GPIO code once I have the Rust implementation ready.

> The GPIO aggregator came into play after talking to Alexander Graf and

> Peter Maydell.  To reduce the attack surface, they didn't want QEMU

> to be responsible for exporting to the guest a subset of all GPIOs of

> a gpiochip, only a full gpiochip.  However, the full gpiochip may

> contain critical GPIOs you do not want the guest to tamper with.

> Hence the GPIO aggregator was born, to take care of aggregating all

> GPIOs you want to export to a guest into a new virtual gpiochip.

> 

> You can find more information about the GPIO Aggregator's use cases in

> "[PATCH v7 0/6] gpio: Add GPIO Aggregator"[2].


So I was actually looking to do some kind of aggregation on the host
side's backend daemon to share only a subset of GPIO pins, I will see
if that is something I can reuse. Thanks for sharing details.

-- 
viresh
Geert Uytterhoeven June 11, 2021, 8:22 a.m. UTC | #11
Hi Viresh,

On Fri, Jun 11, 2021 at 10:01 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 11-06-21, 09:42, Geert Uytterhoeven wrote:

> > On Fri, Jun 11, 2021 at 5:56 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:

> > > On 10-06-21, 22:46, Linus Walleij wrote:

> > > > thanks for working on this, it's a really interesting driver.

> > > >

> > > > My first question is conceptual:

> > > >

> > > > We previously have Geerts driver for virtualization:

> > > > drivers/gpio/gpio-aggregator.c

> > > >

> > > > The idea with the aggregator is that a host script sets up a

> > > > unique gpiochip for the virtualized instance using some poking

> > > > in sysfs and pass that to the virtual machine.

> > > > So this is Linux acting as virtualization host by definition.

> >

> > The gpio-aggregator is running on the host...

> >

> > > > I think virtio is more abstract and intended for the usecase

> > > > where the hypervisor is not Linux, so this should be mentioned

> > > > in the commit, possibly also in Kconfig so users immediately

> > > > know what usecases the two different drivers are for.

> >

> > ... while the virtio-gpio driver is meant for the guest kernel.

> >

> > I my PoC "[PATCH QEMU v2 0/5] Add a GPIO backend"[1], I didn't have

> > a virtio transport, but just hooked into the PL061 GPIO emulation

> > in QEMU.  The PL061 QEMU driver talked to the GPIO backend, which

> > talked to /dev/gpiochipN on the host.

>

> Hmm, interesting.

>

> > > Well, not actually.

> > >

> > > The host can actually be anything. It can be a Xen based dom0, which

> > > runs some proprietary firmware, or Qemu running over Linux.

> > >

> > > It is left for the host to decide how it wants to club together the

> > > GPIO pins from host and access them, with Linux host userspace it

> > > would be playing with /dev/gpiochipN, while for a raw one it may

> > > be accessing registers directly.

> > >

> > > And so the backend running at host, needs to pass the gpiochip

> > > configurations and only the host understand it.

> >

> > So QEMU has to translate the virtio-gpio communication to e.g.

> > /dev/gpiochipN on the host (or a different backend on non-Linux or

> > bare-metal HV).

>

> No, QEMU passes the raw messages to the backend daemon running in host

> userspace (which shares a socket with qemu). The backend understands

> the virtio/vhost protocols and so won't be required to change at all

> if we move from Qemu to something else. And that's what we (Linaro)

> are looking to do here with Project Stratos.

>

> Create virtio based hypervisor agnostic backends.


OK, IC.

> > > The way I test it for now is by running this with Qemu over my x86

> > > box, so my host side is indeed playing with sysfs Linux.

> >

> > Can you please share a link to the QEMU patches?

>

> Unfortunately, they aren't in good shape right now and the backend is

> a bit hacky (Just checking the data paths, but not touching

> /dev/gpiochipN at all for now).

>

> I didn't implement one as I am going to implement the backend in Rust

> and not Qemu. So it doesn't depend on Qemu at all.


OK.

> To give you an idea of the whole thing, here is what we have done for

> I2c for example, GPIO one will look very similar.


Oh, you also have i2c. Nice!

> The Qemu patches:

>

> https://yhbt.net/lore/all/cover.1617278395.git.viresh.kumar@linaro.org/T/

>

> The stuff from tools/vhost-user-i2c/ directory (or patch 4/6) isn't

> used anymore and the following Rust implementation replaces it:

>

> https://github.com/vireshk/vhost-device/tree/master/src/i2c


Thanks for the links!

> > The GPIO aggregator came into play after talking to Alexander Graf and

> > Peter Maydell.  To reduce the attack surface, they didn't want QEMU

> > to be responsible for exporting to the guest a subset of all GPIOs of

> > a gpiochip, only a full gpiochip.  However, the full gpiochip may

> > contain critical GPIOs you do not want the guest to tamper with.

> > Hence the GPIO aggregator was born, to take care of aggregating all

> > GPIOs you want to export to a guest into a new virtual gpiochip.

> >

> > You can find more information about the GPIO Aggregator's use cases in

> > "[PATCH v7 0/6] gpio: Add GPIO Aggregator"[2].

>

> So I was actually looking to do some kind of aggregation on the host

> side's backend daemon to share only a subset of GPIO pins, I will see

> if that is something I can reuse. Thanks for sharing details.


The same reasoning can apply to your backend daemon, so when using
the GPIO aggregator, you can just control a full gpiochip, without
having to implement access control on individual GPIO lines.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Enrico Weigelt, metux IT consult June 14, 2021, 8:03 a.m. UTC | #12
On 11.06.21 09:42, Geert Uytterhoeven wrote:

hi,

> I my PoC "[PATCH QEMU v2 0/5] Add a GPIO backend"[1], I didn't have

> a virtio transport, but just hooked into the PL061 GPIO emulation

> in QEMU.  The PL061 QEMU driver talked to the GPIO backend, which

> talked to /dev/gpiochipN on the host.


for qemu side you might be interested in my patch queue from last year
(including the virtio-gpio implementation) - it also introduces an
gpio backend subsys that allows attaching simulation gpio's to various
backends. so far just implemented a dummy backend (that can be
manipulated by qemu console) and using it just in the virtio-gpio
device emulation.

https://github.com/oss-qm/qemu/tree/wip/gpio-v2

> So QEMU has to translate the virtio-gpio communication to e.g.

> /dev/gpiochipN on the host (or a different backend on non-Linux or

> bare-metal HV).


For qemu case, yes, depending on your actual configuration. You can
attach the virtual device to any gpio backend you like (once it's 
actually implemented). Yet only implemented the dummy, which doesn't
speak to a real hosts gpio, but can be used simulations like HIL.


--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
Enrico Weigelt, metux IT consult June 14, 2021, 8:07 a.m. UTC | #13
On 11.06.21 10:01, Viresh Kumar wrote:

> No, QEMU passes the raw messages to the backend daemon running in host

> userspace (which shares a socket with qemu). The backend understands

> the virtio/vhost protocols and so won't be required to change at all

> if we move from Qemu to something else. And that's what we (Linaro)

> are looking to do here with Project Stratos.


Note that this is completely different from my approach that I've posted
in autumn last year. Viresh's protocol hasn't much in common with mine.


--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
Andy Shevchenko June 14, 2021, 8:12 a.m. UTC | #14
On Mon, Jun 14, 2021 at 11:08 AM Enrico Weigelt, metux IT consult
<lkml@metux.net> wrote:
>

> On 11.06.21 10:01, Viresh Kumar wrote:

>

> > No, QEMU passes the raw messages to the backend daemon running in host

> > userspace (which shares a socket with qemu). The backend understands

> > the virtio/vhost protocols and so won't be required to change at all

> > if we move from Qemu to something else. And that's what we (Linaro)

> > are looking to do here with Project Stratos.

>

> Note that this is completely different from my approach that I've posted

> in autumn last year. Viresh's protocol hasn't much in common with mine.


That's why we have a thing called standard. And AFAIU virtio API/ABIs
should be officially registered and standardized.

-- 
With Best Regards,
Andy Shevchenko
Viresh Kumar June 14, 2021, 9:12 a.m. UTC | #15
On 14-06-21, 10:07, Enrico Weigelt, metux IT consult wrote:
> On 11.06.21 10:01, Viresh Kumar wrote:

> 

> > No, QEMU passes the raw messages to the backend daemon running in host

> > userspace (which shares a socket with qemu). The backend understands

> > the virtio/vhost protocols and so won't be required to change at all

> > if we move from Qemu to something else. And that's what we (Linaro)

> > are looking to do here with Project Stratos.


This is one of the ways of using it, via a backend running in host
userspace, maybe there are other ways of making this work which I may
not have explored, like handling this within qemu.

> Note that this is completely different from my approach that I've posted

> in autumn last year. Viresh's protocol hasn't much in common with mine.


The protocol hasn't changed in a sense on how it can be used. Yes few
things have moved around, new operations were introduced, but nothing
that will make a change at this level. We are trying to use the
protocol in one of the possible ways and yours can be different.

-- 
viresh
Viresh Kumar June 14, 2021, 9:14 a.m. UTC | #16
On 14-06-21, 11:12, Andy Shevchenko wrote:
> On Mon, Jun 14, 2021 at 11:08 AM Enrico Weigelt, metux IT consult

> <lkml@metux.net> wrote:

> >

> > On 11.06.21 10:01, Viresh Kumar wrote:

> >

> > > No, QEMU passes the raw messages to the backend daemon running in host

> > > userspace (which shares a socket with qemu). The backend understands

> > > the virtio/vhost protocols and so won't be required to change at all

> > > if we move from Qemu to something else. And that's what we (Linaro)

> > > are looking to do here with Project Stratos.

> >

> > Note that this is completely different from my approach that I've posted

> > in autumn last year. Viresh's protocol hasn't much in common with mine.

> 

> That's why we have a thing called standard. And AFAIU virtio API/ABIs

> should be officially registered and standardized.


Yes and here is the latest version (which is based on the work done by
Enrico earlier). It isn't accepted yet and is under review.

https://lists.oasis-open.org/archives/virtio-comment/202106/msg00022.html

-- 
viresh
Enrico Weigelt, metux IT consult June 14, 2021, 9:17 a.m. UTC | #17
On 14.06.21 10:12, Andy Shevchenko wrote:

> That's why we have a thing called standard. And AFAIU virtio API/ABIs

> should be officially registered and standardized.


Absolutely.

I've submitted my spec to virtio folks last nov, but this wasn't in form
of patch against their tex-based documentation yet (just ascii text),
thus laying around in the pipeline.

(meanwhile the actual implementation is running in the field for over
6 month now)

Viresh picked it up, but made something totally different out of it.
I wonder why he didn't consult me first. All that needed to be done was
translated the ascii documentation into tex and rephrase a bit in order
to match the formal terminology and structure used in virtio specs.

This makes me very unhappy.


--mtx


-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
Viresh Kumar June 14, 2021, 9:24 a.m. UTC | #18
On 14-06-21, 10:03, Enrico Weigelt, metux IT consult wrote:
> for qemu side you might be interested in my patch queue from last year

> (including the virtio-gpio implementation) - it also introduces an

> gpio backend subsys that allows attaching simulation gpio's to various

> backends. so far just implemented a dummy backend (that can be

> manipulated by qemu console) and using it just in the virtio-gpio

> device emulation.

> 

> https://github.com/oss-qm/qemu/tree/wip/gpio-v2


Interesting, so this is a qemu-specific backend you have here.

The way we are looking to write the backend (in Project Stratos at
Linaro) is to make it hypervisor agnostic. So any hypervisor that
understands the vhost protocol can use our backend without changing a
single line of code. The backend will not be part of any
hypervisor's/VMM's source code. FWIW, this doesn't add anything
special to the virtio protocol (like GPIO).

Here is a C based backend we have for I2C:

https://yhbt.net/lore/all/cover.1617278395.git.viresh.kumar@linaro.org/T/#m3b5044bad9769b170f505e63bd081eb27cef8db2

I started with keeping code in QEMU itself but now replaced it with
one in RUST.

https://github.com/vireshk/vhost-device/tree/master/src/i2c

-- 
viresh
Enrico Weigelt, metux IT consult June 14, 2021, 9:29 a.m. UTC | #19
On 14.06.21 11:12, Viresh Kumar wrote:
>> Note that this is completely different from my approach that I've posted

>> in autumn last year. Viresh's protocol hasn't much in common with mine.

> 

> The protocol hasn't changed in a sense on how it can be used. 


Sorry, but that's not correct. It's an entirely different protocol,
entirely different signal flow, different data structures, different
queue layout, ... in no way compatible.

The only thing they've got in common is they're both do gpios via virtio.


--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
Viresh Kumar June 14, 2021, 9:52 a.m. UTC | #20
On 14-06-21, 11:17, Enrico Weigelt, metux IT consult wrote:
> On 14.06.21 10:12, Andy Shevchenko wrote:

> 

> > That's why we have a thing called standard. And AFAIU virtio API/ABIs

> > should be officially registered and standardized.

> 

> Absolutely.

> 

> I've submitted my spec to virtio folks last nov, but this wasn't in form

> of patch against their tex-based documentation yet (just ascii text),

> thus laying around in the pipeline.

> 

> (meanwhile the actual implementation is running in the field for over

> 6 month now)

> 

> Viresh picked it up, but made something totally different out of it.

> I wonder why he didn't consult me first.


Here I asked you on 26th of May, if you would be fine if I take it
forward as you didn't do anything with it formally in the last 5-6
months (Yes I know you were busy with other stuff).

https://lore.kernel.org/linux-doc/20210526033206.5v362hdywb55msve@vireshk-i7/

You chose not to reply.

I did the same before picking up the kernel code. You chose not to
reply.

I am not inclined to do offlist discussions as they aren't fruitful
eventually, and it is better to do these discussions over open lists,
so others can chime in as well.

> All that needed to be done was

> translated the ascii documentation into tex and rephrase a bit in order

> to match the formal terminology and structure used in virtio specs.


Not really. There were things lagging, which are normally caught in
reviews and fixed/updated. But that never happened in this case. You
only sent the specs once to virtio list, that too as an attachment and
it never made it to the virtio guys there (as the list doesn't take
attachments).

https://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg06946.html

> This makes me very unhappy.


I know you have solutions running in products which depend on the
layout of the config structure or request/responses, based on your
original write-up, but unless something is merged in open source code
or specification, it is going to get changed, normally because of
reviews.

And you will be required to change things based on reviews, you just
can't say that I have products running the code and so I won't change
it. That is why people say, Upstream first. So you don't get into this
mess. Yes, this is tough and that's the way it is.

You keep saying that I have changed the original specification too
much, which is incorrect, I tried to point out earlier what all is
changed and why.

https://lore.kernel.org/linux-gpio/CAKohpokxSoSVtAJkL-T_OOoS8dW-gYG1Gs5=_DwebvJETE48Xw@mail.gmail.com/

You chose not to reply to that.

Lemme say this again. This is a generic protocol we are trying to
define here. It needs to take everyone's view into account and their
use cases. We are required here to collaborate. A solution thought to
be good enough for one person or use-case, isn't going to fly.

The changes I did to it are required to make it useful for the generic
solution for a protocol.

I am sure there would be shortcomings, like the one identified by
Jean-Philippe Brucker, where he asked to add offset of the gpios-name
thing. He made a sensible suggestion, which I am required to
incorporate. I just can't reply to him and say I won't change it
because I have already designed my product based on this.

Lets try to improve the code and specification here and make it work
for both of us by giving very specific feedback on wherever you think
the protocol or code is not efficient or correct. Being unhappy isn't
going make anything better.

-- 
viresh
Viresh Kumar June 14, 2021, 10:21 a.m. UTC | #21
On 10-06-21, 15:22, Arnd Bergmann wrote:
> Can you give an example of how this would be hooked up to other drivers

> using those gpios. Can you give an example of how using the "gpio-keys" or

> "gpio-leds" drivers in combination with virtio-gpio looks like in the DT?

> 

> Would qemu simply add the required DT properties to the device node that

> corresponds to the virtio device in this case?

> 

> From what I can tell, both the mmio and pci variants of virtio can have their

> dev->of_node populated, but I don't see the logic in register_virtio_device()

> that looks up the of_node of the virtio_device that the of_gpio code then

> tries to refer to.


To be honest, I haven't tried this yet and I was expecting it to be
already taken care of. I was relying on the DTB automatically
generated by Qemu to get the driver probed and didn't have a look at
it as well.

I now understand that it won't be that straight forward. The same must
be true for adding an i2c device to an i2c bus over virtio (The way I
tested that earlier was by using the sysfs file to add a device to a
bus).

This may be something lacking generally for virtio-pci thing, not
sure though.

-- 
viresh
Arnd Bergmann June 14, 2021, 12:31 p.m. UTC | #22
On Mon, Jun 14, 2021 at 12:23 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 10-06-21, 15:22, Arnd Bergmann wrote:

> > Can you give an example of how this would be hooked up to other drivers

> > using those gpios. Can you give an example of how using the "gpio-keys" or

> > "gpio-leds" drivers in combination with virtio-gpio looks like in the DT?

> >

> > Would qemu simply add the required DT properties to the device node that

> > corresponds to the virtio device in this case?

> >

> > From what I can tell, both the mmio and pci variants of virtio can have their

> > dev->of_node populated, but I don't see the logic in register_virtio_device()

> > that looks up the of_node of the virtio_device that the of_gpio code then

> > tries to refer to.

>

> To be honest, I haven't tried this yet and I was expecting it to be

> already taken care of. I was relying on the DTB automatically

> generated by Qemu to get the driver probed and didn't have a look at

> it as well.

>

> I now understand that it won't be that straight forward. The same must

> be true for adding an i2c device to an i2c bus over virtio (The way I

> tested that earlier was by using the sysfs file to add a device to a

> bus).


Yes, correct, we had the same discussion about i2c.  Again, this is
relatively straightforward when the controller and the device attached
to it (i2c controller/client or gpio controller/function) are both emulated
by qemu, but a lot harder when the controller and device are
implemented in different programs.

> This may be something lacking generally for virtio-pci thing, not

> sure though.


I think most importantly we need a DT binding to describe what device
nodes are supposed to look like underneath a virtio-mmio or
virtio-pci device in order for a hypervisor to pass down the
information to a guest OS in a generic way. We can probably borrow
the USB naming, and replace compatible="usbVID,PID" with
compatible="virtioDID", with the device ID in hexadecimal digits,
such as "virtio22" for I2C (virtio device ID 34 == 0x22) if we decide
to have a sub-node under the device, or we just point dev->of_node
of the virtio device to the platform/pci device that is its parent
in Linux.

Adding the Linux guest code to the virtio layer should be fairly
straightforward, and I suppose it could be mostly copied from the
corresponding code that added this for mmc in commit 25185f3f31c9
("mmc: Add SDIO function devicetree subnode parsing") and for USB
in commit 69bec7259853 ("USB: core: let USB device know device
node") and 1a7e3948cb9f ("USB: add device-tree support for
interfaces").

        Arnd
Vincent Guittot June 14, 2021, 12:49 p.m. UTC | #23
On Mon, 14 Jun 2021 at 14:33, Arnd Bergmann <arnd@kernel.org> wrote:
>

> On Mon, Jun 14, 2021 at 12:23 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:

> > On 10-06-21, 15:22, Arnd Bergmann wrote:

> > > Can you give an example of how this would be hooked up to other drivers

> > > using those gpios. Can you give an example of how using the "gpio-keys" or

> > > "gpio-leds" drivers in combination with virtio-gpio looks like in the DT?

> > >

> > > Would qemu simply add the required DT properties to the device node that

> > > corresponds to the virtio device in this case?

> > >

> > > From what I can tell, both the mmio and pci variants of virtio can have their

> > > dev->of_node populated, but I don't see the logic in register_virtio_device()

> > > that looks up the of_node of the virtio_device that the of_gpio code then

> > > tries to refer to.

> >

> > To be honest, I haven't tried this yet and I was expecting it to be

> > already taken care of. I was relying on the DTB automatically

> > generated by Qemu to get the driver probed and didn't have a look at

> > it as well.

> >

> > I now understand that it won't be that straight forward. The same must

> > be true for adding an i2c device to an i2c bus over virtio (The way I

> > tested that earlier was by using the sysfs file to add a device to a

> > bus).

>

> Yes, correct, we had the same discussion about i2c.  Again, this is

> relatively straightforward when the controller and the device attached

> to it (i2c controller/client or gpio controller/function) are both emulated

> by qemu, but a lot harder when the controller and device are

> implemented in different programs.

>

> > This may be something lacking generally for virtio-pci thing, not

> > sure though.

>

> I think most importantly we need a DT binding to describe what device

> nodes are supposed to look like underneath a virtio-mmio or

> virtio-pci device in order for a hypervisor to pass down the

> information to a guest OS in a generic way. We can probably borrow

> the USB naming, and replace compatible="usbVID,PID" with

> compatible="virtioDID", with the device ID in hexadecimal digits,

> such as "virtio22" for I2C (virtio device ID 34 == 0x22) if we decide

> to have a sub-node under the device, or we just point dev->of_node

> of the virtio device to the platform/pci device that is its parent

> in Linux.

>

> Adding the Linux guest code to the virtio layer should be fairly

> straightforward, and I suppose it could be mostly copied from the

> corresponding code that added this for mmc in commit 25185f3f31c9

> ("mmc: Add SDIO function devicetree subnode parsing") and for USB

> in commit 69bec7259853 ("USB: core: let USB device know device

> node") and 1a7e3948cb9f ("USB: add device-tree support for

> interfaces").


And something similar is also done with SCMI protocols which are
defined in a SCMI node. A  typical example:

    cpu@0 {
        ...
        clocks = <&scmi_dvfs 0>;
        ...
    };

    deviceX: deviceX@YYYYYYY {
        ...
        clocks = <&scmi_clk 0>;
        ...
    };

    scmi: scmi {
        compatible = "arm,scmi-virtio";
        #address-cells = <1>;
        #size-cells = <0>;

        scmi_devpd: protocol@11 {
            reg = <0x11>;
            #power-domain-cells = <1>;
        };

        scmi_clk: protocol@14 {
            reg = <0x14>;
            #clock-cells = <1>;
        };

        scmi_sensors: protocol@15 {
            reg = <0x15>;
            #thermal-sensor-cells = <1>;
        };

        scmi_dvfs: protocol@13 {
            reg = <0x13>;
            #clock-cells = <1>;
        };
    };

>

>         Arnd
Arnd Bergmann June 14, 2021, 12:58 p.m. UTC | #24
On Mon, Jun 14, 2021 at 2:50 PM Vincent Guittot via Stratos-dev
<stratos-dev@op-lists.linaro.org> wrote:>
> On Mon, 14 Jun 2021 at 14:33, Arnd Bergmann <arnd@kernel.org> wrote:

> > On Mon, Jun 14, 2021 at 12:23 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:

> >

> > I think most importantly we need a DT binding to describe what device

> > nodes are supposed to look like underneath a virtio-mmio or

> > virtio-pci device in order for a hypervisor to pass down the

> > information to a guest OS in a generic way. We can probably borrow

> > the USB naming, and replace compatible="usbVID,PID" with

> > compatible="virtioDID", with the device ID in hexadecimal digits,

> > such as "virtio22" for I2C (virtio device ID 34 == 0x22) if we decide

> > to have a sub-node under the device, or we just point dev->of_node

> > of the virtio device to the platform/pci device that is its parent

> > in Linux.

> >

> > Adding the Linux guest code to the virtio layer should be fairly

> > straightforward, and I suppose it could be mostly copied from the

> > corresponding code that added this for mmc in commit 25185f3f31c9

> > ("mmc: Add SDIO function devicetree subnode parsing") and for USB

> > in commit 69bec7259853 ("USB: core: let USB device know device

> > node") and 1a7e3948cb9f ("USB: add device-tree support for

> > interfaces").

>

> And something similar is also done with SCMI protocols which are

> defined in a SCMI node. A  typical example:

>

>     cpu@0 {

>         ...

>         clocks = <&scmi_dvfs 0>;

>         ...

>     };

>

>     deviceX: deviceX@YYYYYYY {

>         ...

>         clocks = <&scmi_clk 0>;

>         ...

>     };

>

>     scmi: scmi {

>         compatible = "arm,scmi-virtio";

>         #address-cells = <1>;

>         #size-cells = <0>;

>

>         scmi_devpd: protocol@11 {

>             reg = <0x11>;

>             #power-domain-cells = <1>;

>         };

>

>         scmi_clk: protocol@14 {

>             reg = <0x14>;

>             #clock-cells = <1>;

>         };

>

>         scmi_sensors: protocol@15 {

>             reg = <0x15>;

>             #thermal-sensor-cells = <1>;

>         };

>

>         scmi_dvfs: protocol@13 {

>             reg = <0x13>;

>             #clock-cells = <1>;

>         };

>     };


But this example seem to be completely different from the ones I mentioned:
The scmi node that you have here looks like it shows up under the root of the
device tree, not below the virtio device that implements the scmi transport.

         Arnd
Vincent Guittot June 14, 2021, 1:24 p.m. UTC | #25
On Mon, 14 Jun 2021 at 15:00, Arnd Bergmann <arnd@kernel.org> wrote:
>

> On Mon, Jun 14, 2021 at 2:50 PM Vincent Guittot via Stratos-dev

> <stratos-dev@op-lists.linaro.org> wrote:>

> > On Mon, 14 Jun 2021 at 14:33, Arnd Bergmann <arnd@kernel.org> wrote:

> > > On Mon, Jun 14, 2021 at 12:23 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:

> > >

> > > I think most importantly we need a DT binding to describe what device

> > > nodes are supposed to look like underneath a virtio-mmio or

> > > virtio-pci device in order for a hypervisor to pass down the

> > > information to a guest OS in a generic way. We can probably borrow

> > > the USB naming, and replace compatible="usbVID,PID" with

> > > compatible="virtioDID", with the device ID in hexadecimal digits,

> > > such as "virtio22" for I2C (virtio device ID 34 == 0x22) if we decide

> > > to have a sub-node under the device, or we just point dev->of_node

> > > of the virtio device to the platform/pci device that is its parent

> > > in Linux.

> > >

> > > Adding the Linux guest code to the virtio layer should be fairly

> > > straightforward, and I suppose it could be mostly copied from the

> > > corresponding code that added this for mmc in commit 25185f3f31c9

> > > ("mmc: Add SDIO function devicetree subnode parsing") and for USB

> > > in commit 69bec7259853 ("USB: core: let USB device know device

> > > node") and 1a7e3948cb9f ("USB: add device-tree support for

> > > interfaces").

> >

> > And something similar is also done with SCMI protocols which are

> > defined in a SCMI node. A  typical example:

> >

> >     cpu@0 {

> >         ...

> >         clocks = <&scmi_dvfs 0>;

> >         ...

> >     };

> >

> >     deviceX: deviceX@YYYYYYY {

> >         ...

> >         clocks = <&scmi_clk 0>;

> >         ...

> >     };

> >

> >     scmi: scmi {

> >         compatible = "arm,scmi-virtio";

> >         #address-cells = <1>;

> >         #size-cells = <0>;

> >

> >         scmi_devpd: protocol@11 {

> >             reg = <0x11>;

> >             #power-domain-cells = <1>;

> >         };

> >

> >         scmi_clk: protocol@14 {

> >             reg = <0x14>;

> >             #clock-cells = <1>;

> >         };

> >

> >         scmi_sensors: protocol@15 {

> >             reg = <0x15>;

> >             #thermal-sensor-cells = <1>;

> >         };

> >

> >         scmi_dvfs: protocol@13 {

> >             reg = <0x13>;

> >             #clock-cells = <1>;

> >         };

> >     };

>

> But this example seem to be completely different from the ones I mentioned:

> The scmi node that you have here looks like it shows up under the root of the

> device tree, not below the virtio device that implements the scmi transport.


I was thinking of something like below:

    deviceX: deviceX@YYYYYYY {
        ...
        gpio = <&virtio_gpio 0>;
        ...
    };

    virtio_mmio@a000000 {
        dma-coherent;
        interrupts = <0x0 0x10 0x1>;
        reg = <0x0 0xa000000 0x0 0x200>;
        compatible = "virtio,mmio";

        virtio_gpio: protocol@22 {
            reg = <0x22>;
        };
    };

>

>          Arnd
Arnd Bergmann June 14, 2021, 8:54 p.m. UTC | #26
On Mon, Jun 14, 2021 at 3:24 PM Vincent Guittot
<vincent.guittot@linaro.org> wrote:
> On Mon, 14 Jun 2021 at 15:00, Arnd Bergmann <arnd@kernel.org> wrote:

> > On Mon, Jun 14, 2021 at 2:50 PM Vincent Guittot via Stratos-dev <stratos-dev@op-lists.linaro.org> wrote:>

> >

> > But this example seem to be completely different from the ones I mentioned:

> > The scmi node that you have here looks like it shows up under the root of the

> > device tree, not below the virtio device that implements the scmi transport.

>

> I was thinking of something like below:

>

>     deviceX: deviceX@YYYYYYY {

>         ...

>         gpio = <&virtio_gpio 0>;

>         ...

>     };

>

>     virtio_mmio@a000000 {

>         dma-coherent;

>         interrupts = <0x0 0x10 0x1>;

>         reg = <0x0 0xa000000 0x0 0x200>;

>         compatible = "virtio,mmio";

>

>         virtio_gpio: protocol@22 {

>             reg = <0x22>;

>         };


Encoding the device ID as "reg" seems somewhat odd, especially since there
can only be one child for each virtio device. The other bus types use the
"compatible" property instead of "reg" for this purpose. This is still
redundant,
since the type is also known from the contents, but it seems less unusual.

The gpio node in the example is usually called "gpio" or "gpio-controller", and
it would then need the "gpio-controller" and "#gpio-cells" properties so other
nodes can refer to it by phandle.

       Arnd
Vincent Guittot June 15, 2021, 7:30 a.m. UTC | #27
On Mon, 14 Jun 2021 at 22:56, Arnd Bergmann <arnd@kernel.org> wrote:
>

> On Mon, Jun 14, 2021 at 3:24 PM Vincent Guittot

> <vincent.guittot@linaro.org> wrote:

> > On Mon, 14 Jun 2021 at 15:00, Arnd Bergmann <arnd@kernel.org> wrote:

> > > On Mon, Jun 14, 2021 at 2:50 PM Vincent Guittot via Stratos-dev <stratos-dev@op-lists.linaro.org> wrote:>

> > >

> > > But this example seem to be completely different from the ones I mentioned:

> > > The scmi node that you have here looks like it shows up under the root of the

> > > device tree, not below the virtio device that implements the scmi transport.

> >

> > I was thinking of something like below:

> >

> >     deviceX: deviceX@YYYYYYY {

> >         ...

> >         gpio = <&virtio_gpio 0>;

> >         ...

> >     };

> >

> >     virtio_mmio@a000000 {

> >         dma-coherent;

> >         interrupts = <0x0 0x10 0x1>;

> >         reg = <0x0 0xa000000 0x0 0x200>;

> >         compatible = "virtio,mmio";

> >

> >         virtio_gpio: protocol@22 {

> >             reg = <0x22>;

> >         };

>

> Encoding the device ID as "reg" seems somewhat odd, especially since there

> can only be one child for each virtio device. The other bus types use the

> "compatible" property instead of "reg" for this purpose. This is still

> redundant,

> since the type is also known from the contents, but it seems less unusual.


At least this ensures to match directly the protocol id instead of
mapping a compatible string with the protocol id.

>

> The gpio node in the example is usually called "gpio" or "gpio-controller", and

> it would then need the "gpio-controller" and "#gpio-cells" properties so other

> nodes can refer to it by phandle.


yes, This short example is just to show what I mean.



>

>        Arnd
Viresh Kumar June 15, 2021, 11:15 a.m. UTC | #28
On 11-06-21, 10:22, Geert Uytterhoeven wrote:
> The same reasoning can apply to your backend daemon, so when using

> the GPIO aggregator, you can just control a full gpiochip, without

> having to implement access control on individual GPIO lines.


I tried to look at it and it surely looks very temping and may fit
well and reduce size of my backend :)

I am now wondering how interrupts can be made to work here. Do you
have anything in mind for that ?

GPIO sysfs already supports interrupts, just that you need to register
irq for the specific GPIO pins inside the aggregator ?

-- 
viresh
Geert Uytterhoeven June 15, 2021, 11:37 a.m. UTC | #29
Hi Viresh,

On Tue, Jun 15, 2021 at 1:15 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 11-06-21, 10:22, Geert Uytterhoeven wrote:

> > The same reasoning can apply to your backend daemon, so when using

> > the GPIO aggregator, you can just control a full gpiochip, without

> > having to implement access control on individual GPIO lines.

>

> I tried to look at it and it surely looks very temping and may fit

> well and reduce size of my backend :)

>

> I am now wondering how interrupts can be made to work here. Do you

> have anything in mind for that ?

>

> GPIO sysfs already supports interrupts, just that you need to register

> irq for the specific GPIO pins inside the aggregator ?


So far I hadn't considered interrupts.
Will think about it...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Linus Walleij June 15, 2021, 8:03 p.m. UTC | #30
On Tue, Jun 15, 2021 at 1:15 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:

> I am now wondering how interrupts can be made to work here. Do you

> have anything in mind for that ?


The aggregator does not aggregate interrupts only lines for now.

> GPIO sysfs already supports interrupts, (...)


I hope that doesn't make you tempted to use sysfs to get interrupts,
those are awful, they use sysfs_notify_dirent() which means that
if two IRQs happen in  fast succession you will miss one of them
and think it was only one.

The GPIO character device supports low latency events forwarding
interrupts to userspace including QEMU & similar, timestamps the
events as close in time as possible to when they actually happen
(which is necessary for any kind of industrial control) and will never
miss an event if the hardware can register it. See:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/gpio/gpio-event-mon.c

Yours,
Linus Walleij
Viresh Kumar June 16, 2021, 1:45 a.m. UTC | #31
On 15-06-21, 22:03, Linus Walleij wrote:
> On Tue, Jun 15, 2021 at 1:15 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:

> 

> > I am now wondering how interrupts can be made to work here. Do you

> > have anything in mind for that ?

> 

> The aggregator does not aggregate interrupts only lines for now.

> 

> > GPIO sysfs already supports interrupts, (...)

> 

> I hope that doesn't make you tempted to use sysfs to get interrupts,

> those are awful, they use sysfs_notify_dirent() which means that

> if two IRQs happen in  fast succession you will miss one of them

> and think it was only one.

> 

> The GPIO character device supports low latency events forwarding

> interrupts to userspace including QEMU & similar, timestamps the

> events as close in time as possible to when they actually happen

> (which is necessary for any kind of industrial control) and will never

> miss an event if the hardware can register it. See:

> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/gpio/gpio-event-mon.c


The current version of backend (I am working on) will be Linux
userspace based, so I would be required to get an interrupt there.

I was planning to use /dev/gpiochipN only for now and not a sysfs
file, so yes it should be fine.

-- 
viresh
Bjorn Andersson June 16, 2021, 3:30 a.m. UTC | #32
On Thu 10 Jun 15:46 CDT 2021, Linus Walleij wrote:
[..]
> Yet another usecase would be to jit this with remoteproc/rpmsg

> and let a specific signal processor or real-time executive on

> another CPU with a few GPIOs around present these to

> Linux using this mechanism. Well that would certainly interest

> Bjorn and other rpmsg stakeholders, so they should have

> a look so that this provides what they need they day they

> need it. (CCed Bjorn and also Google who may want this for

> their Android emulators.)

> 


Right, your typical Qualcomm platform has a dedicated sensor subsystem,
with some CPU core with dedicated I2C controllers and GPIOs for
processing sensor input while the rest of the SoC is in deep sleep.

Combined with the virtio-i2c effort this could provide an alternative by
simply tunneling the busses and GPIOs into Linux and use standard iio
drivers, for cases where this suits your product requirements better.


And I've seen similar interest from others in the community as well.

Regards,
Bjorn
Enrico Weigelt, metux IT consult June 16, 2021, 3:52 p.m. UTC | #33
On 16.06.21 05:30, Bjorn Andersson wrote:

> Combined with the virtio-i2c effort this could provide an alternative by

> simply tunneling the busses and GPIOs into Linux and use standard iio

> drivers, for cases where this suits your product requirements better.


So, you wanna use virtio as logical interface between the two CPUs ?
Interesting idea. Usually folks use rpmsg for those things.

What is running on the secondary CPU ? Some OS like Linux or some bare
metal stuff ? What kind of CPU is that anyways ?

--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
Linus Walleij June 18, 2021, 9:13 a.m. UTC | #34
On Wed, Jun 16, 2021 at 5:52 PM Enrico Weigelt, metux IT consult
<lkml@metux.net> wrote:
> On 16.06.21 05:30, Bjorn Andersson wrote:

>

> > Combined with the virtio-i2c effort this could provide an alternative by

> > simply tunneling the busses and GPIOs into Linux and use standard iio

> > drivers, for cases where this suits your product requirements better.

>

> So, you wanna use virtio as logical interface between the two CPUs ?

> Interesting idea. Usually folks use rpmsg for those things.


rpmsg is using shared memory as transport mechanism and virtio
is providing this. rpmsg is conceptually a child of virtio: when the
subsystem was proposed by TI Arnd noted that virtio has large
similarities in shared memory transport and as the potential reuse
for buffer handling etc was excellent virtio was used as
a base for rpmsg/remoteproc work.

Yours,
Linus Walleij
Bjorn Andersson June 21, 2021, 5:25 p.m. UTC | #35
On Wed 16 Jun 10:52 CDT 2021, Enrico Weigelt, metux IT consult wrote:

> On 16.06.21 05:30, Bjorn Andersson wrote:

> 

> > Combined with the virtio-i2c effort this could provide an alternative by

> > simply tunneling the busses and GPIOs into Linux and use standard iio

> > drivers, for cases where this suits your product requirements better.

> 

> So, you wanna use virtio as logical interface between the two CPUs ?

> Interesting idea. Usually folks use rpmsg for those things.

> 


rpmsg is a layer on top of virtio, so this would be an extension of the
existing model.

There's been discussions (and I believe some implementations) related to
bridging I2C requests over rpmsg, but I think it's preferable to
standardize around the virtio based bearer directly.

> What is running on the secondary CPU ? Some OS like Linux or some bare

> metal stuff ? What kind of CPU is that anyways ?

> 


These ideas revolves around platforms that implements something like the
"Android Sensor Hub", which provides some resource constraint
co-processor that deals with sensor device interaction and processing of
the data without waking up the power-hungry ARM cores.

Given the focus on power consumption I would guess that these are not
going to run Linux. Core-wise I've seen this implemented using primarily
ARM and Hexagon cores.

Regards,
Bjorn
diff mbox series

Patch

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index e3607ec4c2e8..7f12fb65d130 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1633,6 +1633,15 @@  config GPIO_MOCKUP
 	  tools/testing/selftests/gpio/gpio-mockup.sh. Reference the usage in
 	  it.
 
+config GPIO_VIRTIO
+	tristate "VirtIO GPIO support"
+	depends on VIRTIO
+	help
+	  Say Y here to enable guest support for virtio-based GPIO controllers.
+
+	  These virtual GPIOs can be routed to real GPIOs or attached to
+	  simulators on the host (like QEMU).
+
 endmenu
 
 endif
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index c58a90a3c3b1..ace004c80871 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -162,6 +162,7 @@  obj-$(CONFIG_GPIO_UCB1400)		+= gpio-ucb1400.o
 obj-$(CONFIG_GPIO_UNIPHIER)		+= gpio-uniphier.o
 obj-$(CONFIG_GPIO_VF610)		+= gpio-vf610.o
 obj-$(CONFIG_GPIO_VIPERBOARD)		+= gpio-viperboard.o
+obj-$(CONFIG_GPIO_VIRTIO)		+= gpio-virtio.o
 obj-$(CONFIG_GPIO_VISCONTI)		+= gpio-visconti.o
 obj-$(CONFIG_GPIO_VR41XX)		+= gpio-vr41xx.o
 obj-$(CONFIG_GPIO_VX855)		+= gpio-vx855.o
diff --git a/drivers/gpio/gpio-virtio.c b/drivers/gpio/gpio-virtio.c
new file mode 100644
index 000000000000..1ba8ddf4693a
--- /dev/null
+++ b/drivers/gpio/gpio-virtio.c
@@ -0,0 +1,326 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * GPIO driver for virtio-based virtual GPIO controllers
+ *
+ * Copyright (C) 2021 metux IT consult
+ * Enrico Weigelt, metux IT consult <info@metux.net>
+ *
+ * Copyright (C) 2021 Linaro.
+ * Viresh Kumar <viresh.kumar@linaro.org>
+ */
+
+#include <linux/completion.h>
+#include <linux/err.h>
+#include <linux/gpio/driver.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/virtio_config.h>
+#include <uapi/linux/virtio_gpio.h>
+#include <uapi/linux/virtio_ids.h>
+
+struct virtio_gpio {
+	struct virtio_device *vdev;
+	struct mutex lock;
+	struct gpio_chip gc;
+
+	struct completion completion;
+	struct virtqueue *command_vq;
+	struct virtio_gpio_request creq;
+	struct virtio_gpio_response cres;
+
+	/* This must be kept as the last entry here, hint: gpio_names[0] */
+	struct virtio_gpio_config config;
+};
+
+#define gpio_chip_to_vgpio(chip) container_of(chip, struct virtio_gpio, gc)
+
+static int virtio_gpio_req(struct virtio_gpio *vgpio, u16 type, u16 gpio,
+			   u8 txdata, u8 *rxdata)
+{
+	struct virtio_gpio_response *res = &vgpio->cres;
+	struct virtio_gpio_request *req = &vgpio->creq;
+	struct scatterlist *sgs[2], req_sg, res_sg;
+	struct device *dev = &vgpio->vdev->dev;
+	unsigned long time_left;
+	unsigned int len;
+	int ret;
+
+	req->type = cpu_to_le16(type);
+	req->gpio = cpu_to_le16(gpio);
+	req->data = txdata;
+
+	sg_init_one(&req_sg, req, sizeof(*req));
+	sg_init_one(&res_sg, res, sizeof(*res));
+	sgs[0] = &req_sg;
+	sgs[1] = &res_sg;
+
+	mutex_lock(&vgpio->lock);
+	ret = virtqueue_add_sgs(vgpio->command_vq, sgs, 1, 1, res, GFP_KERNEL);
+	if (ret) {
+		dev_err(dev, "failed to add request to vq\n");
+		goto out;
+	}
+
+	reinit_completion(&vgpio->completion);
+	virtqueue_kick(vgpio->command_vq);
+
+	time_left = wait_for_completion_timeout(&vgpio->completion, HZ / 10);
+	if (!time_left) {
+		dev_err(dev, "virtio GPIO backend timeout\n");
+		return -ETIMEDOUT;
+	}
+
+	WARN_ON(res != virtqueue_get_buf(vgpio->command_vq, &len));
+	if (unlikely(res->status != VIRTIO_GPIO_STATUS_OK)) {
+		dev_err(dev, "virtio GPIO request failed: %d\n", gpio);
+		return -EINVAL;
+	}
+
+	if (rxdata)
+		*rxdata = res->data;
+
+out:
+	mutex_unlock(&vgpio->lock);
+
+	return ret;
+}
+
+static int virtio_gpio_request(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc);
+
+	return virtio_gpio_req(vgpio, VIRTIO_GPIO_REQ_ACTIVATE, gpio, 0, NULL);
+}
+
+static void virtio_gpio_free(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc);
+
+	virtio_gpio_req(vgpio, VIRTIO_GPIO_REQ_DEACTIVATE, gpio, 0, NULL);
+}
+
+static int virtio_gpio_get_direction(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc);
+	u8 direction;
+	int ret;
+
+	ret = virtio_gpio_req(vgpio, VIRTIO_GPIO_REQ_GET_DIRECTION, gpio, 0,
+			      &direction);
+	if (ret)
+		return ret;
+
+	return direction;
+}
+
+static int virtio_gpio_direction_input(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc);
+
+	return virtio_gpio_req(vgpio, VIRTIO_GPIO_REQ_DIRECTION_IN, gpio, 0,
+			       NULL);
+}
+
+static int virtio_gpio_direction_output(struct gpio_chip *gc, unsigned int gpio,
+					int value)
+{
+	struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc);
+
+	return virtio_gpio_req(vgpio, VIRTIO_GPIO_REQ_DIRECTION_OUT, gpio, (u8)
+			       value, NULL);
+}
+
+static int virtio_gpio_get(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc);
+	u8 value;
+	int ret;
+
+	ret = virtio_gpio_req(vgpio, VIRTIO_GPIO_REQ_GET_VALUE, gpio, 0, &value);
+	if (ret)
+		return ret;
+
+	return value;
+}
+
+static void virtio_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value)
+{
+	struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc);
+
+	virtio_gpio_req(vgpio, VIRTIO_GPIO_REQ_SET_VALUE, gpio, value, NULL);
+}
+
+static void virtio_gpio_command(struct virtqueue *vq)
+{
+	struct virtio_gpio *vgpio = vq->vdev->priv;
+
+	complete(&vgpio->completion);
+}
+
+static int virtio_gpio_alloc_vqs(struct virtio_device *vdev)
+{
+	struct virtio_gpio *vgpio = vdev->priv;
+	const char * const names[] = { "command" };
+	vq_callback_t *cbs[] = {
+		virtio_gpio_command,
+	};
+	struct virtqueue *vqs[1] = {NULL};
+	int ret;
+
+	ret = virtio_find_vqs(vdev, 1, vqs, cbs, names, NULL);
+	if (ret) {
+		dev_err(&vdev->dev, "failed to allocate vqs: %d\n", ret);
+		return ret;
+	}
+
+	vgpio->command_vq = vqs[0];
+
+	/* Mark the device ready to perform operations from within probe() */
+	virtio_device_ready(vgpio->vdev);
+	return 0;
+}
+
+static void virtio_gpio_free_vqs(struct virtio_device *vdev)
+{
+	vdev->config->reset(vdev);
+	vdev->config->del_vqs(vdev);
+}
+
+static const char **parse_gpio_names(struct virtio_device *vdev,
+			       struct virtio_gpio_config *config)
+{
+	struct device *dev = &vdev->dev;
+	char *str = config->gpio_names;
+	const char **names;
+	int i, len;
+
+	if (!config->gpio_names_size)
+		return NULL;
+
+	names = devm_kcalloc(dev, config->ngpio, sizeof(names), GFP_KERNEL);
+	if (!names)
+		return ERR_PTR(-ENOMEM);
+
+	/* NULL terminate the string instead of checking it */
+	config->gpio_names[config->gpio_names_size - 1] = '\0';
+
+	for (i = 0; i < config->ngpio; i++) {
+		/*
+		 * We have already marked the last byte with '\0'
+		 * earlier, this shall be enough here.
+		 */
+		if (str == config->gpio_names + config->gpio_names_size) {
+			dev_err(dev, "Invalid gpio_names\n");
+			return ERR_PTR(-EINVAL);
+		}
+
+		len = strlen(str);
+		if (!len) {
+			dev_err(dev, "Missing GPIO name: %d\n", i);
+			return ERR_PTR(-EINVAL);
+		}
+
+		names[i] = str;
+		str += len + 1;
+	}
+
+	return names;
+}
+
+static int virtio_gpio_probe(struct virtio_device *vdev)
+{
+	struct virtio_gpio_config *config;
+	struct device *dev = &vdev->dev;
+	struct virtio_gpio *vgpio;
+	u32 size;
+	int ret;
+
+	virtio_cread(vdev, struct virtio_gpio_config, gpio_names_size, &size);
+	size = cpu_to_le32(size);
+
+	vgpio = devm_kzalloc(dev, sizeof(*vgpio) + size, GFP_KERNEL);
+	if (!vgpio)
+		return -ENOMEM;
+
+	config = &vgpio->config;
+
+	/* Read configuration */
+	virtio_cread_bytes(vdev, 0, config, sizeof(*config) + size);
+
+	/* NULL terminate the string instead of checking it */
+	config->name[sizeof(config->name) - 1] = '\0';
+	config->ngpio = cpu_to_le16(config->ngpio);
+	config->gpio_names_size = cpu_to_le32(config->gpio_names_size);
+
+	if (!config->ngpio) {
+		dev_err(dev, "Number of GPIOs can't be zero\n");
+		return -EINVAL;
+	}
+
+	vgpio->vdev			= vdev;
+	vgpio->gc.request		= virtio_gpio_request;
+	vgpio->gc.free			= virtio_gpio_free;
+	vgpio->gc.get_direction		= virtio_gpio_get_direction;
+	vgpio->gc.direction_input	= virtio_gpio_direction_input;
+	vgpio->gc.direction_output	= virtio_gpio_direction_output;
+	vgpio->gc.get			= virtio_gpio_get;
+	vgpio->gc.set			= virtio_gpio_set;
+	vgpio->gc.ngpio			= config->ngpio;
+	vgpio->gc.base			= -1; /* Allocate base dynamically */
+	vgpio->gc.label			= config->name[0] ?
+					  config->name : dev_name(dev);
+	vgpio->gc.parent		= dev;
+	vgpio->gc.owner			= THIS_MODULE;
+	vgpio->gc.can_sleep		= true;
+	vgpio->gc.names			= parse_gpio_names(vdev, config);
+	if (IS_ERR(vgpio->gc.names))
+		return PTR_ERR(vgpio->gc.names);
+
+	vdev->priv = vgpio;
+	mutex_init(&vgpio->lock);
+	init_completion(&vgpio->completion);
+
+	ret = virtio_gpio_alloc_vqs(vdev);
+	if (ret)
+		return ret;
+
+	ret = gpiochip_add(&vgpio->gc);
+	if (ret) {
+		virtio_gpio_free_vqs(vdev);
+		dev_err(dev, "Failed to add virtio-gpio controller\n");
+	}
+
+	return ret;
+}
+
+static void virtio_gpio_remove(struct virtio_device *vdev)
+{
+	struct virtio_gpio *vgpio = vdev->priv;
+
+	gpiochip_remove(&vgpio->gc);
+	virtio_gpio_free_vqs(vdev);
+}
+
+static const struct virtio_device_id id_table[] = {
+	{ VIRTIO_ID_GPIO, VIRTIO_DEV_ANY_ID },
+	{},
+};
+MODULE_DEVICE_TABLE(virtio, id_table);
+
+static struct virtio_driver virtio_gpio_driver = {
+	.id_table		= id_table,
+	.probe			= virtio_gpio_probe,
+	.remove			= virtio_gpio_remove,
+	.driver			= {
+		.name		= KBUILD_MODNAME,
+		.owner		= THIS_MODULE,
+	},
+};
+module_virtio_driver(virtio_gpio_driver);
+
+MODULE_AUTHOR("Enrico Weigelt, metux IT consult <info@metux.net>");
+MODULE_AUTHOR("Viresh Kumar <viresh.kumar@linaro.org>");
+MODULE_DESCRIPTION("VirtIO GPIO driver");
+MODULE_LICENSE("GPL");
diff --git a/include/uapi/linux/virtio_gpio.h b/include/uapi/linux/virtio_gpio.h
new file mode 100644
index 000000000000..e805e33a79cb
--- /dev/null
+++ b/include/uapi/linux/virtio_gpio.h
@@ -0,0 +1,41 @@ 
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+
+#ifndef _LINUX_VIRTIO_GPIO_H
+#define _LINUX_VIRTIO_GPIO_H
+
+#include <linux/types.h>
+
+/* Virtio GPIO request types */
+#define VIRTIO_GPIO_REQ_ACTIVATE		0x01
+#define VIRTIO_GPIO_REQ_DEACTIVATE		0x02
+#define VIRTIO_GPIO_REQ_GET_DIRECTION		0x03
+#define VIRTIO_GPIO_REQ_DIRECTION_IN		0x04
+#define VIRTIO_GPIO_REQ_DIRECTION_OUT		0x05
+#define VIRTIO_GPIO_REQ_GET_VALUE		0x06
+#define VIRTIO_GPIO_REQ_SET_VALUE		0x07
+
+/* Possible values of the status field */
+#define VIRTIO_GPIO_STATUS_OK			0x0
+#define VIRTIO_GPIO_STATUS_ERR			0x1
+
+struct virtio_gpio_config {
+	char name[32];
+	__u16 ngpio;
+	__u16 padding;
+	__u32 gpio_names_size;
+	char gpio_names[0];
+} __packed;
+
+/* Virtio GPIO Request / Response */
+struct virtio_gpio_request {
+	__u16 type;
+	__u16 gpio;
+	__u8 data;
+};
+
+struct virtio_gpio_response {
+	__u8 status;
+	__u8 data;
+};
+
+#endif /* _LINUX_VIRTIO_GPIO_H */
diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
index b89391dff6c9..0c24e8ae2499 100644
--- a/include/uapi/linux/virtio_ids.h
+++ b/include/uapi/linux/virtio_ids.h
@@ -55,5 +55,6 @@ 
 #define VIRTIO_ID_PMEM			27 /* virtio pmem */
 #define VIRTIO_ID_MAC80211_HWSIM	29 /* virtio mac80211-hwsim */
 #define VIRTIO_ID_I2C_ADAPTER		34 /* virtio i2c adapter */
+#define VIRTIO_ID_GPIO			41 /* virtio GPIO */
 
 #endif /* _LINUX_VIRTIO_IDS_H */