Re: [PATCH v2 2/2] drivers: gpio: add virtio-gpio guest driver
On 03-07-21, 04:05, Michael S. Tsirkin wrote: > On Wed, May 26, 2021 at 09:02:06AM +0530, Viresh Kumar wrote: > > On 25-05-21, 14:59, Enrico Weigelt, metux IT consult wrote: > > > On 24.05.21 13:27, Viresh Kumar wrote: > > > > > > Hi, > > > > > > > > > > We (Linaro's Project Stratos > > > > https://linaro.atlassian.net/wiki/spaces/STR/overview) > > > > are interested in this stuff. I was trying to look at the last status > > > > of all this. Few > > > > questions for you: > > > > > > > > - Was the spec ever posted to virtio-dev list ? I thought that's the > > > > very first step before > > > > we merge the code. > > > > > > I had posted some spec quite some time ago, but it wasn't in the form > > > of patches against the .tex documentation files yet. It's been laying > > > aside for quite a while, since I've been busy w/ other things. > > > > Will you be fine if I take that up and restart the thread ? > > It's been a while - why not right? Yeah, we went past that and here is the last version I posted. https://lists.oasis-open.org/archives/virtio-comment/202106/msg00033.html which I took back later on to let Enrico do it, as he wanted to. And here is the last version from Enrico: https://lists.oasis-open.org/archives/virtio-comment/202106/msg00048.html Lets see how this goes in coming days. -- viresh ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 2/2] drivers: gpio: add virtio-gpio guest driver
On Wed, May 26, 2021 at 09:02:06AM +0530, Viresh Kumar wrote: > On 25-05-21, 14:59, Enrico Weigelt, metux IT consult wrote: > > On 24.05.21 13:27, Viresh Kumar wrote: > > > > Hi, > > > > > > > We (Linaro's Project Stratos > > > https://linaro.atlassian.net/wiki/spaces/STR/overview) > > > are interested in this stuff. I was trying to look at the last status > > > of all this. Few > > > questions for you: > > > > > > - Was the spec ever posted to virtio-dev list ? I thought that's the > > > very first step before > > > we merge the code. > > > > I had posted some spec quite some time ago, but it wasn't in the form > > of patches against the .tex documentation files yet. It's been laying > > aside for quite a while, since I've been busy w/ other things. > > Will you be fine if I take that up and restart the thread ? It's been a while - why not right? > -- > viresh ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 2/2] drivers: gpio: add virtio-gpio guest driver
"Enrico Weigelt, metux IT consult" writes: > On 04.12.20 04:35, Jason Wang wrote: > > Hi, > >> Is the plan to keep this doc synced with the one in the virtio >> specification? > > Yes, of course. I'm still in progress of doing the beaurocratic stuff w/ > virtio-tc folks (ID registration, ...) - yet have to see whether they > wanna add it to their spec documents ... > > BTW: if you feel, sometings not good w/ the current spec, please raise > your voice now. > >> I think it's better to use u8 ot uint8_t here.Git grep told me the >> former is more popular under Documentation/. > > thx, I'll fix that > >>> +- for version field currently only value 1 supported. >>> +- the line names block holds a stream of zero-terminated strings, >>> + holding the individual line names. >> >> I'm not sure but does this mean we don't have a fixed length of config >> space? Need to check whether it can bring any trouble to >> migration(compatibility). > > Yes, it depends on how many gpio lines are present and how much space > their names take up. > > A fixed size would either put unpleasent limits on the max number of > lines or waste a lot space when only few lines present. > > Not that virtio-gpio is also meant for small embedded workloads running > under some hypervisor. > >>> +- unspecified fields are reserved for future use and should be zero. >>> + >>> + >>> +Virtqueues and messages: >>> + >>> + >>> +- Queue #0: transmission from host to guest >>> +- Queue #1: transmission from guest to host >> >> >> Virtio became more a popular in the area without virtualization. So I >> think it's better to use "device/driver" instead of "host/guest" here. > > Good point. But I'd prefer "cpu" instead of "driver" in that case. I think you are going to tie yourself up in knots if you don't move this to the OASIS spec. The reason being the VirtIO spec has definitions for what a "Device" and a "Driver" is that are clear and unambiguous. The upstream spec should be considered the canonical source of truth for any implementation (Linux or otherwise). By all means have the distilled documentation for the driver in the kernel source tree but trying to upstream an implementation before starting the definition in the standard is a little back to front IMHO*. * that's not to say these things can't be done in parallel as the spec is reviewed and worked on and the kinks worked out but you want the final order of upstreaming to start with the spec. -- Alex Bennée ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: Howto listen to/handle gpio state changes ? Re: [PATCH v2 2/2] drivers: gpio: add virtio-gpio guest driver
On Wed, Dec 9, 2020 at 12:19 PM Arnd Bergmann wrote: > On Wed, Dec 9, 2020 at 9:51 AM Linus Walleij wrote: > > On Tue, Dec 8, 2020 at 3:07 PM Enrico Weigelt, metux IT consult > > wrote: > > > What we need to understand is if your new usecase is an outlier > > so it is simplest modeled by a "mock" irq_chip or we have to design > > something new altogether like notifications on changes. I suspect > > irq_chip would be best because all drivers using GPIOs for interrupts > > are expecting interrupts, and it would be an enormous task to > > change them all and really annoying to create a new mechanism > > on the side. > > I would expect the platform abstraction to actually be close enough > to a chained irqchip that it actually works: the notification should > come in via vring_interrupt(), which is a normal interrupt handler > that calls vq->vq.callback(), calling generic_handle_irq() (and > possibly chained_irq_enter()/chained_irq_exit() around it) like the > other gpio drivers do should just work here I think, and if it did > not, then I would expect this to be just a bug in the driver rather > than something missing in the gpio framework. Performance/latency-wise that would also be strongly encouraged. Tglx isn't super-happy about the chained interrupts at times, as they can create really nasty bugs, but a pure IRQ in fastpath of some kinde is preferable and intuitive either way. Yours, Linus Walleij ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 2/2] drivers: gpio: add virtio-gpio guest driver
On 2020/12/8 下午3:02, Enrico Weigelt, metux IT consult wrote: On 08.12.20 03:36, Jason Wang wrote: Hi, So we endup with two solutions (without a prompt): 1) using select, user may end up with driver without transport IMHO not an entirely unusual situation in other places of the kernel, eg. one can enable USB devices, w/o having an usb host adapter enabled. And even if some USB-HA driver is enabled, the actualy machine doesn't necessarily have the corresponding device. Ok, then select works for me. 2) using depends, user need to enable at least one transport 2) looks a little bit better I admit. So, all virtio devices should depend on TRANSPORT_A || TRANSPORT_B || TRANSPORT_C || ... ? (and also change all these places if another transport is added) ? I think not. The idea is, if none of the transport (select VIRTIO) is enabled, user can not enable any virtio drivers (depends on VIRTIO). Thanks --mtx ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: Howto listen to/handle gpio state changes ? Re: [PATCH v2 2/2] drivers: gpio: add virtio-gpio guest driver
On Tue, Dec 8, 2020 at 3:07 PM Enrico Weigelt, metux IT consult wrote: > I've been looking for some more direct notification callback for gpio > consumers: here the consumer would register itself as a listener on > some gpio_desc and called back when something changes (with data what > exactly changed, eg. "gpio #3 input switched to high"). > > Seems we currently just have the indirect path via interrupts. I don't know how indirect it is, it seems pretty direct to me. The subsystem was designed in response to how the hardware in front of the developers worked. So far we have had: - Cascaded interrupts - Dedicated (hieararchical) interrupts - Message Signalled Interrupts And if you now bring something else to the show then it's not like the subsystem was designed for some abstract quality such as generic notification of events that occurred, all practical instances have been around actual IRQs and that is why it is using struct irq_chip. What we need to understand is if your new usecase is an outlier so it is simplest modeled by a "mock" irq_chip or we have to design something new altogether like notifications on changes. I suspect irq_chip would be best because all drivers using GPIOs for interrupts are expecting interrupts, and it would be an enormous task to change them all and really annoying to create a new mechanism on the side. Yours, Linus Walleij ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: Howto listen to/handle gpio state changes ? Re: [PATCH v2 2/2] drivers: gpio: add virtio-gpio guest driver
On Sat, Dec 5, 2020 at 9:15 PM Enrico Weigelt, metux IT consult wrote: > The virtio-gpio device/host can raise a signal on line state change. > Kinda IRQ, but not actually running through real IRQs, instead by a > message running though queue. (hmm, kida MSI ? :o). > > I've tried allocating an IRQ range and calling generic_handle_irq(), > but then I'm getting unhanled IRQ trap. This is Bartosz territory, but the gpio-mockup.c driver will insert IRQs into the system, he went and added really core stuff into kernel/irq to make this happen. Notice that in Kconfig it does: select IRQ_SIM Then this is used: include/linux/irq_sim.h This is intended for simulating IRQs and both GPIO and IIO use it. I think this inserts IRQs from debugfs and I have no idea how flexible that is. If it is suitable for what you want to do I don't know but it's virtio so... Yours, Linus Walleij ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 2/2] drivers: gpio: add virtio-gpio guest driver
On 2020/12/7 下午5:33, Enrico Weigelt, metux IT consult wrote: On 07.12.20 04:48, Jason Wang wrote: Hi, Not a native speaker but event sounds like something driver read from device. Looking at the below lists, most of them except for VIRTIO_GPIO_EV_HOST_LEVEL looks more like a command. okay, shall I name it "message" ? It might be better. Okay, renamed to messages in v3. #define VIRTIO_NET_OK 0 #define VIRTIO_NET_ERR 1 hmm, so I'd need to define all the error codes that possibly could happen ? Yes, I think you need. Okay, going to do it in the next version. If I read the code correctly, this expects there will be at most a single type of event that can be processed at the same time. E.g can upper layer want to read from different lines in parallel? If yes, we need to deal with that. @Linus @Bartosz: can that happen or does gpio subsys already serialize requests ? Initially, I tried to protect it by spinlock (so, only one request may run at a time, other calls just wait until the first is finished), but it crashed when gpio cdev registration calls into the driver (fetches the status) while still in bootup. Don't recall the exact error anymore, but something like an inconsistency in the spinlock calls. Did I just use the wrong type of lock ? I'm not sure since I am not familiar with GPIO. But a question is, if at most one request is allowed, I'm not sure virtio is the best choice here since we don't even need a queue(virtqueue) here. I guess, I should add locks to the gpio callback functions (where gpio subsys calls in). That way, requests are requests are strictly ordered. The locks didn't work in my previous attempts, but probably because I've missed to set the can_sleep flag (now fixed in v3). The gpio ops are already waiting for reply of the corresponding type, so the only bad thing that could happen is the same operation being called twice (when coming from different threads) and replies mixed up between first and second one. OTOH I don't see much problem w/ that. This can be fixed by adding a global lock. I think it's still about whether or not we need allow a batch of requests via a queue. Consider you've submitted two request A and B, and if B is done first, current code won't work. This is because, the reply is transported via rxq buffers not just reuse the txq buffer if I read the code correctly. Meanwhile I've changed it to allocate a new rx buffer for the reply (done right before the request is sent), so everything should be processed in the order it had been sent. Assuming virtio keeps the order of the buffers in the queues. Unfortunately, there's no guarantee that virtio will keep the order or it needs to advertise VIRTIO_F_IN_ORDER. (see 2.6.9 in the virtio spec). Btw, if possible, it's better to add a link to the userspace code here. Could you please give an example how bi-directional transmission within the same queue could look like ? You can check how virtio-blk did this in: https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html#x1-256 hmm, still don't see how the code would actually look like. (in qemu as well as kernel). Just add the fetched inbuf as an outbuf (within the same queue) ? Yes, virtio allows adding IN buffers after OUT buffer through descriptor chaining. Thanks Maybe add one new buffer per request and one new per received async signal ? It would be safe to fill the whole rxq and do the refill e.g when half of the queue is used. Okay, doing that now in v3: there's always at least one rx buffer, and requests as well as the intr receiver always add a new one. (they get removed on fetching, IMHO). --mtx ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 2/2] drivers: gpio: add virtio-gpio guest driver
On 2020/12/7 下午9:53, Michael S. Tsirkin wrote: On Mon, Dec 07, 2020 at 11:12:50AM +0800, Jason Wang wrote: On 2020/12/6 上午3:32, Michael S. Tsirkin wrote: On Sat, Dec 05, 2020 at 08:59:55AM +0100, Enrico Weigelt, metux IT consult wrote: On 04.12.20 04:35, Jason Wang wrote: --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -1615,6 +1615,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 Let's use select, since there's no prompt for VIRTIO and it doesn't have any dependencies. whoops, it's not that simple: make: Entering directory '/home/nekrad/src/apu2-dev/pkg/kernel.apu2.git' make[1]: Entering directory '/home/nekrad/src/dk/DistroKit/platform-x86_64/build-target/linux-5.8.9-build' GEN Makefile drivers/gpu/drm/Kconfig:74:error: recursive dependency detected! drivers/gpu/drm/Kconfig:74: symbol DRM_KMS_HELPER is selected by DRM_VIRTIO_GPU drivers/gpu/drm/virtio/Kconfig:2: symbol DRM_VIRTIO_GPU depends on VIRTIO drivers/virtio/Kconfig:2: symbol VIRTIO is selected by GPIO_VIRTIO drivers/gpio/Kconfig:1618: symbol GPIO_VIRTIO depends on GPIOLIB drivers/gpio/Kconfig:14:symbol GPIOLIB is selected by I2C_MUX_LTC4306 drivers/i2c/muxes/Kconfig:47: symbol I2C_MUX_LTC4306 depends on I2C drivers/i2c/Kconfig:8: symbol I2C is selected by FB_DDC drivers/video/fbdev/Kconfig:63: symbol FB_DDC depends on FB drivers/video/fbdev/Kconfig:12: symbol FB is selected by DRM_KMS_FB_HELPER drivers/gpu/drm/Kconfig:80: symbol DRM_KMS_FB_HELPER depends on DRM_KMS_HELPER Seems that we can only depend on or select some symbol - we run into huge trouble if thats mixed. Just changed DRM_VIRTIO_GPU to just select VIRIO instead of depending on it, and now it works. I've posted another patch for fixing drivers/gpu/drm/virtio/Kconfig to use 'select' instead of 'depends on'. It seems a bit of a mess, at this point I'm not entirely sure when should drivers select VIRTIO and when depend on it. The text near it says: # SPDX-License-Identifier: GPL-2.0-only config VIRTIO tristate help This option is selected by any driver which implements the virtio bus, such as CONFIG_VIRTIO_PCI, CONFIG_VIRTIO_MMIO, CONFIG_RPMSG or CONFIG_S390_GUEST. Which seems clear enough and would indicate drivers for devices *behind* the bus should not select VIRTIO and thus presumably should "depend on" it. This is violated in virtio console and virtio fs drivers. For console it says: commit 9f30eb29c514589e16f2999ea070598583d1f6ec Author: Michal Suchanek Date: Mon Aug 31 18:58:50 2020 +0200 char: virtio: Select VIRTIO from VIRTIO_CONSOLE. Make it possible to have virtio console built-in when other virtio drivers are modular. Signed-off-by: Michal Suchanek Reviewed-by: Amit Shah Link: https://lore.kernel.org/r/20200831165850.26163-1-msucha...@suse.de Signed-off-by: Greg Kroah-Hartman which seems kind of bogus - why do we care about allowing a builtin virtio console driver if the pci virtio bus driver is a module? There won't be any devices on the bus to attach to ... For testing like switching bus from pci to MMIO? Not sure I understand ... can you give an example? E.g testing modprobe -r virtio_mmio modprobe virtio_pci ? And for virtio fs it was like this from the beginning. I am inclined to fix console and virtio fs to depend on VIRTIO: select is harder to use correctly ... Jason? I think it works, but we need a prompt for VIRTIO otherwise there's no way to enable it. Thanks That's even messier. No one needs VIRTIO core by itself - it's only used by transports and drivers. So we endup with two solutions (without a prompt): 1) using select, user may end up with driver without transport 2) using depends, user need to enable at least one transport 2) looks a little bit better I admit. Thanks -- --- 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 i...@metux.net -- +49-151-27565287 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 2/2] drivers: gpio: add virtio-gpio guest driver
On Mon, Dec 07, 2020 at 11:12:50AM +0800, Jason Wang wrote: > > On 2020/12/6 上午3:32, Michael S. Tsirkin wrote: > > On Sat, Dec 05, 2020 at 08:59:55AM +0100, Enrico Weigelt, metux IT consult > > wrote: > > > On 04.12.20 04:35, Jason Wang wrote: > > > > > > > > --- a/drivers/gpio/Kconfig > > > > > +++ b/drivers/gpio/Kconfig > > > > > @@ -1615,6 +1615,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 > > > > > > > > Let's use select, since there's no prompt for VIRTIO and it doesn't have > > > > any dependencies. > > > whoops, it's not that simple: > > > > > > make: Entering directory '/home/nekrad/src/apu2-dev/pkg/kernel.apu2.git' > > > make[1]: Entering directory > > > '/home/nekrad/src/dk/DistroKit/platform-x86_64/build-target/linux-5.8.9-build' > > >GEN Makefile > > > drivers/gpu/drm/Kconfig:74:error: recursive dependency detected! > > > drivers/gpu/drm/Kconfig:74: symbol DRM_KMS_HELPER is selected by > > > DRM_VIRTIO_GPU > > > drivers/gpu/drm/virtio/Kconfig:2: symbol DRM_VIRTIO_GPU depends on VIRTIO > > > drivers/virtio/Kconfig:2: symbol VIRTIO is selected by GPIO_VIRTIO > > > drivers/gpio/Kconfig:1618:symbol GPIO_VIRTIO depends on GPIOLIB > > > drivers/gpio/Kconfig:14: symbol GPIOLIB is selected by I2C_MUX_LTC4306 > > > drivers/i2c/muxes/Kconfig:47: symbol I2C_MUX_LTC4306 depends on I2C > > > drivers/i2c/Kconfig:8:symbol I2C is selected by FB_DDC > > > drivers/video/fbdev/Kconfig:63: symbol FB_DDC depends on FB > > > drivers/video/fbdev/Kconfig:12: symbol FB is selected by > > > DRM_KMS_FB_HELPER > > > drivers/gpu/drm/Kconfig:80: symbol DRM_KMS_FB_HELPER depends on > > > DRM_KMS_HELPER > > > > > > Seems that we can only depend on or select some symbol - we run into > > > huge trouble if thats mixed. Just changed DRM_VIRTIO_GPU to just select > > > VIRIO instead of depending on it, and now it works. > > > > > > I've posted another patch for fixing drivers/gpu/drm/virtio/Kconfig > > > to use 'select' instead of 'depends on'. > > It seems a bit of a mess, at this point I'm not entirely sure when > > should drivers select VIRTIO and when depend on it. > > > > The text near it says: > > > > # SPDX-License-Identifier: GPL-2.0-only > > config VIRTIO > > tristate > > help > >This option is selected by any driver which implements the virtio > >bus, such as CONFIG_VIRTIO_PCI, CONFIG_VIRTIO_MMIO, CONFIG_RPMSG > >or CONFIG_S390_GUEST. > > > > Which seems clear enough and would indicate drivers for devices *behind* > > the bus should not select VIRTIO and thus presumably should "depend on" it. > > This is violated in virtio console and virtio fs drivers. > > > > For console it says: > > > > commit 9f30eb29c514589e16f2999ea070598583d1f6ec > > Author: Michal Suchanek > > Date: Mon Aug 31 18:58:50 2020 +0200 > > > > char: virtio: Select VIRTIO from VIRTIO_CONSOLE. > > Make it possible to have virtio console built-in when > > other virtio drivers are modular. > > Signed-off-by: Michal Suchanek > > Reviewed-by: Amit Shah > > Link: > > https://lore.kernel.org/r/20200831165850.26163-1-msucha...@suse.de > > Signed-off-by: Greg Kroah-Hartman > > > > which seems kind of bogus - why do we care about allowing a builtin > > virtio console driver if the pci virtio bus driver is a module? > > There won't be any devices on the bus to attach to ... > > > For testing like switching bus from pci to MMIO? Not sure I understand ... can you give an example? > > > And for virtio fs it was like this from the beginning. > > > > I am inclined to fix console and virtio fs to depend on VIRTIO: > > select is harder to use correctly ... > > > > Jason? > > > I think it works, but we need a prompt for VIRTIO otherwise there's no way > to enable it. > > Thanks That's even messier. No one needs VIRTIO core by itself - it's only used by transports and drivers. > > > > > > > > -- > > > --- > > > 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 > > > i...@metux.net -- +49-151-27565287 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 2/2] drivers: gpio: add virtio-gpio guest driver
On Sat, Dec 05, 2020 at 09:05:16PM +0100, Enrico Weigelt, metux IT consult wrote: > On 05.12.20 20:32, Michael S. Tsirkin wrote: > > Hi, > > > It seems a bit of a mess, at this point I'm not entirely sure when > > should drivers select VIRTIO and when depend on it. > > if VIRTIO just enables something that could be seen as library > functions, then select should be right, IMHO. > > > The text near it says: > > > > # SPDX-License-Identifier: GPL-2.0-only > > config VIRTIO > > tristate > > oh, wait, doesn't have an menu text, so we can't even explicitly enable > it (not shown in menu) - only implicitly. Which means that some other > option must select it, in order to become availe at all, and in order > to make others depending on it becoming available. > > IMHO, therefore select is the correct approach. > > > > help > > This option is selected by any driver which implements the virtio > > bus, such as CONFIG_VIRTIO_PCI, CONFIG_VIRTIO_MMIO, CONFIG_RPMSG > > or CONFIG_S390_GUEST. > > > > Which seems clear enough and would indicate drivers for devices *behind* > > the bus should not select VIRTIO and thus presumably should "depend on" it. > > This is violated in virtio console and virtio fs drivers. > > See above: NAK. because it can't even be enabled directly (by the user). > If it wasn't meant otherwise, we'd have to add an menu text. The point is that user enables one of the bindings. That in turn enables drivers. If we merely select VIRTIO there's a chance user won't remember to select any bindings and will be surprised not to see any devices. > > For console it says: > > > > commit 9f30eb29c514589e16f2999ea070598583d1f6ec > > Author: Michal Suchanek > > Date: Mon Aug 31 18:58:50 2020 +0200 > > > > char: virtio: Select VIRTIO from VIRTIO_CONSOLE. > > > > Make it possible to have virtio console built-in when > > other virtio drivers are modular. > > > > Signed-off-by: Michal Suchanek > > Reviewed-by: Amit Shah > > Link: https://lore.kernel.org/r/20200831165850.26163-1-msucha...@suse.de > > Signed-off-by: Greg Kroah-Hartman > > > > which seems kind of bogus - why do we care about allowing a builtin > > virtio console driver if the pci virtio bus driver is a module? > > There won't be any devices on the bus to attach to ... > > When using other transports ? Any transport selects VIRTIO so if you enable that, you get VIRTIO and thus it's enough to depend on it. > In my current project, eg. I'm using mmio - my kernel has pci completely > disabled. > > > I am inclined to fix console and virtio fs to depend on VIRTIO: > > select is harder to use correctly ... > > I don't thinkt that would be good - instead everybody should just select > VIRTIO, never depend on it (maybe depend on VIRTIO_MENU instead) GPU depends on VIRTIO and on VIRTIO_MENU ... which seems even messier ... > > --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 > i...@metux.net -- +49-151-27565287 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 2/2] drivers: gpio: add virtio-gpio guest driver
On 2020/12/4 下午5:36, Enrico Weigelt, metux IT consult wrote:
On 04.12.20 04:35, Jason Wang wrote:
Hi,
Is the plan to keep this doc synced with the one in the virtio
specification?
Yes, of course. I'm still in progress of doing the beaurocratic stuff w/
virtio-tc folks (ID registration, ...) - yet have to see whether they
wanna add it to their spec documents ...
BTW: if you feel, sometings not good w/ the current spec, please raise
your voice now.
But, has the spec path posted?
I think it's better to use u8 ot uint8_t here.Git grep told me the
former is more popular under Documentation/.
thx, I'll fix that
+- for version field currently only value 1 supported.
+- the line names block holds a stream of zero-terminated strings,
+ holding the individual line names.
I'm not sure but does this mean we don't have a fixed length of config
space? Need to check whether it can bring any trouble to
migration(compatibility).
Yes, it depends on how many gpio lines are present and how much space
their names take up.
A fixed size would either put unpleasent limits on the max number of
lines or waste a lot space when only few lines present.
Not that virtio-gpio is also meant for small embedded workloads running
under some hypervisor.
+- unspecified fields are reserved for future use and should be zero.
+
+
+Virtqueues and messages:
+
+
+- Queue #0: transmission from host to guest
+- Queue #1: transmission from guest to host
Virtio became more a popular in the area without virtualization. So I
think it's better to use "device/driver" instead of "host/guest" here.
Good point. But I'd prefer "cpu" instead of "driver" in that case.
Not a native speaker but event sounds like something driver read from
device. Looking at the below lists, most of them except for
VIRTIO_GPIO_EV_HOST_LEVEL looks more like a command.
okay, shall I name it "message" ?
It might be better.
Another question is, what's the benefit of unifying the message format
of the two queues. E.g VIRTIO_GPIO_EV_HOST_LEVEL can only works fro rxq.
Simplicity. Those fields that aren't really relevant (eg. replies also
carry the line id), can just be ignored.
Not familiar with GPIO but I wonder the value of a standalone
VIRTIO_GPIO_EV_GUEST_DIRECTION_INPUT/OUTPUT. Can we simply imply them in
SET/GET_VALUE?
Would introduce more complexity. Somewhere I'd have to fit in some extra
bit for differenciating between line state and line direction. The
direction tells whether the line currently acts as input or output. The
"value" (hmm, maybe I should rethink terminology here) is the current
line level (high/low or active/inactive).
Ok.
+--
+Data flow:
+--
+
+- all operations, except ``VIRTIO_GPIO_EV_HOST_LEVEL``, are
guest-initiated
+- host replies ``VIRTIO_GPIO_EV_HOST_LEVEL`` OR'ed to the ``type`` field
+- ``VIRTIO_GPIO_EV_HOST_LEVEL`` is only sent asynchronically from
host to guest
+- in replies, a negative ``value`` field denotes an unix-style errno
code
Virtio is in a different scope, so we need to define the error code on
our own.
E.g for virtio-net we define:
#define VIRTIO_NET_OK 0
#define VIRTIO_NET_ERR 1
hmm, so I'd need to define all the error codes that possibly could happen ?
Yes, I think you need.
+config GPIO_VIRTIO
+ tristate "VirtIO GPIO support"
+ depends on VIRTIO
Let's use select, since there's no prompt for VIRTIO and it doesn't have
any dependencies.
Ok. I just was under the impression that subsystems and busses should
not be select'ed, but depends on (eg. some time ago tried that w/ gpio
subsys and failed).
+ help
+ Say Y here to enable guest support for virtio-based GPIOs.
+
+ These virtual GPIOs can be routed to real GPIOs or attached to
+ simulators on the host (qemu).
It's better to avoid talking host and qemu here for new virtio devices.
Ok, dropped that line.
+static int virtio_gpio_xmit(struct virtio_gpio_priv *priv, int type,
+ int pin, int value, struct virtio_gpio_event *ev)
+{
+ struct scatterlist sg[1];
+ int ret;
+ unsigned long flags;
+
+ WARN_ON(!ev);
+
+ ev->type = type;
+ ev->pin = pin;
+ ev->value = value;
+
+ sg_init_table(sg, 1);
+ sg_set_buf([0], ev, sizeof(struct virtio_gpio_event));
+
+ spin_lock_irqsave(>vq_lock, flags);
+ ret = virtqueue_add_outbuf(priv->vq_tx, sg, ARRAY_SIZE(sg),
+ priv, GFP_KERNEL);
+ if (ret < 0) {
+ dev_err(>vdev->dev,
+ "virtqueue_add_outbuf() failed: %d\n", ret);
+ goto out;
So except for the error log, the failure is silently ignored by the
caller. Is this intended?
ups, I've forgotten the error handling in the caller. fixed in v3.
+static int virtio_gpio_req(struct virtio_gpio_priv *priv, int type,
+ int pin, int value)
+{
+ struct virtio_gpio_event *ev
+ = kzalloc(>vdev->dev, sizeof(struct
Re: [PATCH v2 2/2] drivers: gpio: add virtio-gpio guest driver
On 2020/12/6 上午4:05, Enrico Weigelt, metux IT consult wrote: On 05.12.20 20:32, Michael S. Tsirkin wrote: Hi, It seems a bit of a mess, at this point I'm not entirely sure when should drivers select VIRTIO and when depend on it. if VIRTIO just enables something that could be seen as library functions, then select should be right, IMHO. The text near it says: # SPDX-License-Identifier: GPL-2.0-only config VIRTIO tristate oh, wait, doesn't have an menu text, so we can't even explicitly enable it (not shown in menu) - only implicitly. Which means that some other option must select it, in order to become availe at all, and in order to make others depending on it becoming available. IMHO, therefore select is the correct approach. help This option is selected by any driver which implements the virtio bus, such as CONFIG_VIRTIO_PCI, CONFIG_VIRTIO_MMIO, CONFIG_RPMSG or CONFIG_S390_GUEST. Which seems clear enough and would indicate drivers for devices *behind* the bus should not select VIRTIO and thus presumably should "depend on" it. This is violated in virtio console and virtio fs drivers. See above: NAK. because it can't even be enabled directly (by the user). If it wasn't meant otherwise, we'd have to add an menu text. For console it says: commit 9f30eb29c514589e16f2999ea070598583d1f6ec Author: Michal Suchanek Date: Mon Aug 31 18:58:50 2020 +0200 char: virtio: Select VIRTIO from VIRTIO_CONSOLE. Make it possible to have virtio console built-in when other virtio drivers are modular. Signed-off-by: Michal Suchanek Reviewed-by: Amit Shah Link: https://lore.kernel.org/r/20200831165850.26163-1-msucha...@suse.de Signed-off-by: Greg Kroah-Hartman which seems kind of bogus - why do we care about allowing a builtin virtio console driver if the pci virtio bus driver is a module? There won't be any devices on the bus to attach to ... When using other transports ? In my current project, eg. I'm using mmio - my kernel has pci completely disabled. I am inclined to fix console and virtio fs to depend on VIRTIO: select is harder to use correctly ... I don't thinkt that would be good - instead everybody should just select VIRTIO, never depend on it (maybe depend on VIRTIO_MENU instead) I'm fine with either. Though I prefer to use select but it looks to me adding a prompt and use enable would be easier. Thanks --mtx ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 2/2] drivers: gpio: add virtio-gpio guest driver
On 2020/12/6 上午3:32, Michael S. Tsirkin wrote: On Sat, Dec 05, 2020 at 08:59:55AM +0100, Enrico Weigelt, metux IT consult wrote: On 04.12.20 04:35, Jason Wang wrote: --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -1615,6 +1615,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 Let's use select, since there's no prompt for VIRTIO and it doesn't have any dependencies. whoops, it's not that simple: make: Entering directory '/home/nekrad/src/apu2-dev/pkg/kernel.apu2.git' make[1]: Entering directory '/home/nekrad/src/dk/DistroKit/platform-x86_64/build-target/linux-5.8.9-build' GEN Makefile drivers/gpu/drm/Kconfig:74:error: recursive dependency detected! drivers/gpu/drm/Kconfig:74: symbol DRM_KMS_HELPER is selected by DRM_VIRTIO_GPU drivers/gpu/drm/virtio/Kconfig:2: symbol DRM_VIRTIO_GPU depends on VIRTIO drivers/virtio/Kconfig:2: symbol VIRTIO is selected by GPIO_VIRTIO drivers/gpio/Kconfig:1618: symbol GPIO_VIRTIO depends on GPIOLIB drivers/gpio/Kconfig:14:symbol GPIOLIB is selected by I2C_MUX_LTC4306 drivers/i2c/muxes/Kconfig:47: symbol I2C_MUX_LTC4306 depends on I2C drivers/i2c/Kconfig:8: symbol I2C is selected by FB_DDC drivers/video/fbdev/Kconfig:63: symbol FB_DDC depends on FB drivers/video/fbdev/Kconfig:12: symbol FB is selected by DRM_KMS_FB_HELPER drivers/gpu/drm/Kconfig:80: symbol DRM_KMS_FB_HELPER depends on DRM_KMS_HELPER Seems that we can only depend on or select some symbol - we run into huge trouble if thats mixed. Just changed DRM_VIRTIO_GPU to just select VIRIO instead of depending on it, and now it works. I've posted another patch for fixing drivers/gpu/drm/virtio/Kconfig to use 'select' instead of 'depends on'. It seems a bit of a mess, at this point I'm not entirely sure when should drivers select VIRTIO and when depend on it. The text near it says: # SPDX-License-Identifier: GPL-2.0-only config VIRTIO tristate help This option is selected by any driver which implements the virtio bus, such as CONFIG_VIRTIO_PCI, CONFIG_VIRTIO_MMIO, CONFIG_RPMSG or CONFIG_S390_GUEST. Which seems clear enough and would indicate drivers for devices *behind* the bus should not select VIRTIO and thus presumably should "depend on" it. This is violated in virtio console and virtio fs drivers. For console it says: commit 9f30eb29c514589e16f2999ea070598583d1f6ec Author: Michal Suchanek Date: Mon Aug 31 18:58:50 2020 +0200 char: virtio: Select VIRTIO from VIRTIO_CONSOLE. Make it possible to have virtio console built-in when other virtio drivers are modular. Signed-off-by: Michal Suchanek Reviewed-by: Amit Shah Link: https://lore.kernel.org/r/20200831165850.26163-1-msucha...@suse.de Signed-off-by: Greg Kroah-Hartman which seems kind of bogus - why do we care about allowing a builtin virtio console driver if the pci virtio bus driver is a module? There won't be any devices on the bus to attach to ... For testing like switching bus from pci to MMIO? And for virtio fs it was like this from the beginning. I am inclined to fix console and virtio fs to depend on VIRTIO: select is harder to use correctly ... Jason? I think it works, but we need a prompt for VIRTIO otherwise there's no way to enable it. Thanks -- --- 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 i...@metux.net -- +49-151-27565287 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 2/2] drivers: gpio: add virtio-gpio guest driver
On Sat, Dec 05, 2020 at 08:59:55AM +0100, Enrico Weigelt, metux IT consult wrote: > On 04.12.20 04:35, Jason Wang wrote: > > >> --- a/drivers/gpio/Kconfig > >> +++ b/drivers/gpio/Kconfig > >> @@ -1615,6 +1615,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 > > > > > > Let's use select, since there's no prompt for VIRTIO and it doesn't have > > any dependencies. > > whoops, it's not that simple: > > make: Entering directory '/home/nekrad/src/apu2-dev/pkg/kernel.apu2.git' > make[1]: Entering directory > '/home/nekrad/src/dk/DistroKit/platform-x86_64/build-target/linux-5.8.9-build' > GEN Makefile > drivers/gpu/drm/Kconfig:74:error: recursive dependency detected! > drivers/gpu/drm/Kconfig:74: symbol DRM_KMS_HELPER is selected by > DRM_VIRTIO_GPU > drivers/gpu/drm/virtio/Kconfig:2: symbol DRM_VIRTIO_GPU depends on VIRTIO > drivers/virtio/Kconfig:2: symbol VIRTIO is selected by GPIO_VIRTIO > drivers/gpio/Kconfig:1618:symbol GPIO_VIRTIO depends on GPIOLIB > drivers/gpio/Kconfig:14: symbol GPIOLIB is selected by I2C_MUX_LTC4306 > drivers/i2c/muxes/Kconfig:47: symbol I2C_MUX_LTC4306 depends on I2C > drivers/i2c/Kconfig:8:symbol I2C is selected by FB_DDC > drivers/video/fbdev/Kconfig:63: symbol FB_DDC depends on FB > drivers/video/fbdev/Kconfig:12: symbol FB is selected by > DRM_KMS_FB_HELPER > drivers/gpu/drm/Kconfig:80: symbol DRM_KMS_FB_HELPER depends on > DRM_KMS_HELPER > > Seems that we can only depend on or select some symbol - we run into > huge trouble if thats mixed. Just changed DRM_VIRTIO_GPU to just select > VIRIO instead of depending on it, and now it works. > > I've posted another patch for fixing drivers/gpu/drm/virtio/Kconfig > to use 'select' instead of 'depends on'. It seems a bit of a mess, at this point I'm not entirely sure when should drivers select VIRTIO and when depend on it. The text near it says: # SPDX-License-Identifier: GPL-2.0-only config VIRTIO tristate help This option is selected by any driver which implements the virtio bus, such as CONFIG_VIRTIO_PCI, CONFIG_VIRTIO_MMIO, CONFIG_RPMSG or CONFIG_S390_GUEST. Which seems clear enough and would indicate drivers for devices *behind* the bus should not select VIRTIO and thus presumably should "depend on" it. This is violated in virtio console and virtio fs drivers. For console it says: commit 9f30eb29c514589e16f2999ea070598583d1f6ec Author: Michal Suchanek Date: Mon Aug 31 18:58:50 2020 +0200 char: virtio: Select VIRTIO from VIRTIO_CONSOLE. Make it possible to have virtio console built-in when other virtio drivers are modular. Signed-off-by: Michal Suchanek Reviewed-by: Amit Shah Link: https://lore.kernel.org/r/20200831165850.26163-1-msucha...@suse.de Signed-off-by: Greg Kroah-Hartman which seems kind of bogus - why do we care about allowing a builtin virtio console driver if the pci virtio bus driver is a module? There won't be any devices on the bus to attach to ... And for virtio fs it was like this from the beginning. I am inclined to fix console and virtio fs to depend on VIRTIO: select is harder to use correctly ... Jason? > -- > --- > 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 > i...@metux.net -- +49-151-27565287 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 2/2] drivers: gpio: add virtio-gpio guest driver
On 2020/12/4 上午3:11, Enrico Weigelt, metux IT consult wrote: Introducing new gpio driver for virtual GPIO devices via virtio. The driver allows routing gpio control into VM guests, eg. brigding virtual gpios to specific host gpios, or attaching simulators for automatic application testing. Changes v2: * fixed uapi header license * sorted include's * fixed formatting * fixed unneeded devm allocation - plain kzalloc/kfree is enough * fixed missing devm_kzalloc fail check * use devm_kcalloc() for array allocation * added virtio-gpio protocol specification Signed-off-by: Enrico Weigelt, metux IT consult --- Documentation/gpio/virtio-gpio.rst | 176 MAINTAINERS| 6 + drivers/gpio/Kconfig | 9 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-virtio.c | 332 + include/uapi/linux/virtio_gpio.h | 39 + include/uapi/linux/virtio_ids.h| 1 + 7 files changed, 564 insertions(+) create mode 100644 Documentation/gpio/virtio-gpio.rst create mode 100644 drivers/gpio/gpio-virtio.c create mode 100644 include/uapi/linux/virtio_gpio.h diff --git a/Documentation/gpio/virtio-gpio.rst b/Documentation/gpio/virtio-gpio.rst new file mode 100644 index ..04642be07b96 --- /dev/null +++ b/Documentation/gpio/virtio-gpio.rst @@ -0,0 +1,176 @@ +" +Virtio-GPIO protocol specification +" +... +Specification for virtio-based virtiual GPIO devices +... + Is the plan to keep this doc synced with the one in the virtio specification? ++ ++Version_ 1.0 ++ + +=== +General +=== + +The virtio-gpio protocol provides access to general purpose IO devices +to virtual machine guests. These virtualized GPIOs could be either provided +by some simulator (eg. virtual HIL), routed to some external device or +routed to real GPIOs on the host (eg. virtualized embedded applications). + +Instead of simulating some existing real GPIO chip within an VMM, this +protocol provides an hardware independent interface between host and guest +that solely relies on an active virtio connection (no matter which transport +actually used), no other buses or additional platform driver logic required. + +=== +Protocol layout +=== + +-- +Configuration space +-- + +++--+---+ +| Offset | Type | Description | +++==+===+ +| 0x00 | uint8| version | +++--+---+ +| 0x02 | uint16 | number of GPIO lines | +++--+---+ +| 0x04 | uint32 | size of gpio name block | +++--+---+ +| 0x20 | char[32] | device name (0-terminated)| +++--+---+ +| 0x40 | char[] | line names block | +++--+---+ + I think it's better to use u8 ot uint8_t here.Git grep told me the former is more popular under Documentation/. +- for version field currently only value 1 supported. +- the line names block holds a stream of zero-terminated strings, + holding the individual line names. I'm not sure but does this mean we don't have a fixed length of config space? Need to check whether it can bring any trouble to migration(compatibility). +- unspecified fields are reserved for future use and should be zero. + + +Virtqueues and messages: + + +- Queue #0: transmission from host to guest +- Queue #1: transmission from guest to host Virtio became more a popular in the area without virtualization. So I think it's better to use "device/driver" instead of "host/guest" here. + +The queues transport messages of the struct virtio_gpio_event: + +Message format: +--- + +++--+---+ +| Offset | Type | Description | +++==+===+ +| 0x00 | uint16 | event type| +++--+---+ +| 0x02 | uint16 | line id | +++--+---+ +| 0x04 | uint32 | value | +++--+---+ Not a native speaker but event sounds like something driver read from device. Looking at the below lists, most of them except for VIRTIO_GPIO_EV_HOST_LEVEL looks more like a command. Another question is, what's the benefit of unifying the message format of the two queues. E.g VIRTIO_GPIO_EV_HOST_LEVEL can only works fro rxq. + +Message types: +-- + ++---+---+-+ +| Code | Symbol
