| Message ID | 20210712141833.6628-1-cristian.marussi@arm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce SCMI transport based on VirtIO | expand |
On 12.07.21 16:18, Cristian Marussi wrote: > Hi all, > Hi Cristian, thanks for your update. Please find some additional comments in this reply and the following. Best regards, Peter > While reworking this series starting from the work done up to V3 by > OpenSynergy, I am keeping the original autorship and list distribution > unchanged. > > The main aim of this rework, as said, is to simplify where possible the > SCMI VirtIO support added in V3 by adding at first some new general > mechanisms in the SCMI Transport layer. > > Indeed, after some initial small fixes, patches 05/06/07/08 add such new > additional mechanisms to the SCMI core to ease implementation of more > complex transports like virtio, while also addressing a few general issues > already potentially affecting existing transports. > > In terms of rework I dropped original V3 patches 05/06/07/08/12 as no more > needed, and modified where needed the remaining original patches to take > advantage of the above mentioned new SCMI transport features. > > DT bindings patch has been ported on top of freshly YAML converted arm,scmi > bindings. > > Moreover, since V5 I dropped support for polling mode from the virtio-scmi > transport, since it is an optional general mechanism provided by the core > to allow transports lacking a completion IRQ to work and it seemed a > needless addition/complication in the context of virtio transport. > Just for correctness, in my understanding polling is not completely optional ATM. Polling would be required by scmi_cpufreq_fast_switch(). But that requirement might be irrelevant for now. > Additionally, in V5 I could also simplify a bit the virtio transport > probing sequence starting from the observation that, by the VirtIO spec, > in fact, only one single SCMI VirtIO device can possibly exist on a system. > I wouldn't say that the virtio spec restricts the # of virtio-scmi devices to one. But I do think the one device limitation in the kernel is acceptable. > The series has been tested using an emulated fake SCMI device and also a > proper SCP-fw stack running through QEMU vhost-users, with the SCMI stack > compiled, in both cases, as builtin and as a loadable module, running tests > against mocked SCMI Sensors using HWMON and IIO interfaces to check the > functionality of notifications and sync/async commands. > > Virtio-scmi support has been exercised in the following testing scenario > on a JUNO board: > > - normal sync/async command transfers > - notifications > - concurrent delivery of correlated response and delayed responses > - out-of-order delivery of delayed responses before related responses > - unexpected delayed response delivery for sync commands > - late delivery of timed-out responses and delayed responses > > Some basic regression testing against mailbox transport has been performed > for commands and notifications too. > > No sensible overhead in total handling time of commands and notifications > has been observed, even though this series do indeed add a considerable > amount of code to execute on TX path. > More test and measurements could be needed in these regards. > > This series is based on top of v5.14-rc1. > > Any feedback/testing is welcome :D > > Thanks, > Cristian > --- > V5 --> V6: > - removed delegated xfers and its usage > - add and use *priv optional parameter in scmi_rx_callback() > - made .poll_done and .clear_channel ops optional > > V4 --> V5: > - removed msg raw_payload helpers > - reworked msg helpers to use xfer->priv reference > - simplified SCMI device probe sequence (one static device) > - added new SCMI Kconfig layout > - removed SCMI virtio polling support > > V3 --> V4: > - using new delegated xfers support and monotonically increasing tokens > in virtio transport > - ported SCMI virtio transport DT bindings to YAML format > - added virtio-scmi polling support > - added delegated xfers support > > Cristian Marussi (11): > firmware: arm_scmi: Avoid padding in sensor message structure > firmware: arm_scmi: Fix max pending messages boundary check > firmware: arm_scmi: Add support for type handling in common functions > firmware: arm_scmi: Remove scmi_dump_header_dbg() helper > firmware: arm_scmi: Add transport optional init/exit support > firmware: arm_scmi: Introduce monotonically increasing tokens > firmware: arm_scmi: Handle concurrent and out-of-order messages > firmware: arm_scmi: Add priv parameter to scmi_rx_callback > firmware: arm_scmi: Make .clear_channel optional > firmware: arm_scmi: Make polling mode optional > firmware: arm_scmi: Make SCMI transports configurable > > Igor Skalkin (4): > firmware: arm_scmi: Make shmem support optional for transports > firmware: arm_scmi: Add method to override max message number > dt-bindings: arm: Add virtio transport for SCMI > firmware: arm_scmi: Add virtio transport > > Peter Hilber (2): > firmware: arm_scmi: Add message passing abstractions for transports > firmware: arm_scmi: Add optional link_supplier() transport op > > .../bindings/firmware/arm,scmi.yaml | 8 +- > MAINTAINERS | 1 + > drivers/firmware/Kconfig | 34 +- > drivers/firmware/arm_scmi/Kconfig | 97 +++ > drivers/firmware/arm_scmi/Makefile | 8 +- > drivers/firmware/arm_scmi/common.h | 94 ++- > drivers/firmware/arm_scmi/driver.c | 651 +++++++++++++++--- > drivers/firmware/arm_scmi/mailbox.c | 2 +- > drivers/firmware/arm_scmi/msg.c | 113 +++ > drivers/firmware/arm_scmi/sensors.c | 6 +- > drivers/firmware/arm_scmi/smc.c | 3 +- > drivers/firmware/arm_scmi/virtio.c | 491 +++++++++++++ > include/uapi/linux/virtio_ids.h | 1 + > include/uapi/linux/virtio_scmi.h | 24 + > 14 files changed, 1389 insertions(+), 144 deletions(-) > create mode 100644 drivers/firmware/arm_scmi/Kconfig > create mode 100644 drivers/firmware/arm_scmi/msg.c > create mode 100644 drivers/firmware/arm_scmi/virtio.c > create mode 100644 include/uapi/linux/virtio_scmi.h >
On Thu, Jul 15, 2021 at 06:35:38PM +0200, Peter Hilber wrote: > On 12.07.21 16:18, Cristian Marussi wrote: > > Hi all, > > > > Hi Cristian, > > thanks for your update. Please find some additional comments in this reply > and the following. > > Best regards, > > Peter Hi Peter, thanks for the feedback. > > > While reworking this series starting from the work done up to V3 by > > OpenSynergy, I am keeping the original autorship and list distribution > > unchanged. > > > > The main aim of this rework, as said, is to simplify where possible the > > SCMI VirtIO support added in V3 by adding at first some new general > > mechanisms in the SCMI Transport layer. > > > > Indeed, after some initial small fixes, patches 05/06/07/08 add such new > > additional mechanisms to the SCMI core to ease implementation of more > > complex transports like virtio, while also addressing a few general issues > > already potentially affecting existing transports. > > > > In terms of rework I dropped original V3 patches 05/06/07/08/12 as no more > > needed, and modified where needed the remaining original patches to take > > advantage of the above mentioned new SCMI transport features. > > > > DT bindings patch has been ported on top of freshly YAML converted arm,scmi > > bindings. > > > > Moreover, since V5 I dropped support for polling mode from the virtio-scmi > > transport, since it is an optional general mechanism provided by the core > > to allow transports lacking a completion IRQ to work and it seemed a > > needless addition/complication in the context of virtio transport. > > > > Just for correctness, in my understanding polling is not completely optional > ATM. Polling would be required by scmi_cpufreq_fast_switch(). But that > requirement might be irrelevant for now. > Cpufreq core can use .fast_switch (scmi_cpufreq_fast_switch) op only if policy->fast_switch_enabled is true which in turn reported as true by the SCMI cpufreq driver iff SCMI FastChannels are supported by Perf implementation server side, but the SCMI Device VirtIO spec (5.17) explicitly does NOT support SCMI FastChannels as of now. Anyway, even though we should support in the future SCMI FastChannels on VirtIO SCMI transport, fastchannels are by defintion per-protocol/per-command/ per-domain-id specific, based on sharedMem or MMIO, unidirectional and do not even allow for a response from the platform (SCMIV3.0 4.1.1 5.3) so polling won't be a thing anyway unless I'm missing something. BUT you made a good point in fact anyway, because the generic perf->freq_set/get API CAN be indeed invoked in polling mode, and, even though we do not use them in polling as of now (if not in the FastChannel scenario above) this could be a potential problem in general if when the underlying transport do not support poll the core just drop any poll_completion=true messages. So, while I still think it is not sensible to enable poll mode in SCMI Virtio, because would be a sort of faked polling and increases complexity, I'm now considering the fact that maybe the right behaviour of the SCMI core in such a scenario would be to warn the user as it does now AND then fallback to use non-polling, probably better if such a behavior is made condtional on some transport config desc flag that allow such fallback behavior. Any thought ? > > Additionally, in V5 I could also simplify a bit the virtio transport > > probing sequence starting from the observation that, by the VirtIO spec, > > in fact, only one single SCMI VirtIO device can possibly exist on a system. > > > > I wouldn't say that the virtio spec restricts the # of virtio-scmi devices > to one. But I do think the one device limitation in the kernel is > acceptable. > Indeed it is not that VirtIO spec explicitly forbids multiple devices, but the fact that SCMI devices are not identifiable from VirtIO layer (if not by vendor mmaybe) nor from DT config (as of now) leads to the fact that only one single device can be possibly used; if you define multiple MMIO devices (or multiple PCI are discovered) there is now way to tell which is which and, say, assign those to distinct channels per different protocols. Having said that, this upcoming series by Viresh, may be useful in the future: https://lore.kernel.org/lkml/aa4bf68fdd13b885a6dc1b98f88834916d51d97d.1626173013.git.viresh.kumar@linaro.org/ since it would add the capability to 'link' an SCMI device user (SCMI virtio transport instance or specific channel) to a specific virtio-mmio node. Once this is in, I could try to multi SCMI VirtIO device support, but not within this series probably. Thanks, Cristian
On 19.07.21 13:36, Cristian Marussi wrote: > On Thu, Jul 15, 2021 at 06:35:38PM +0200, Peter Hilber wrote: >> On 12.07.21 16:18, Cristian Marussi wrote: >>> Hi all, >>> >> >> Hi Cristian, >> >> thanks for your update. Please find some additional comments in this reply >> and the following. >> >> Best regards, >> >> Peter > > Hi Peter, > > thanks for the feedback. > >> >>> While reworking this series starting from the work done up to V3 by >>> OpenSynergy, I am keeping the original autorship and list distribution >>> unchanged. >>> >>> The main aim of this rework, as said, is to simplify where possible the >>> SCMI VirtIO support added in V3 by adding at first some new general >>> mechanisms in the SCMI Transport layer. >>> >>> Indeed, after some initial small fixes, patches 05/06/07/08 add such new >>> additional mechanisms to the SCMI core to ease implementation of more >>> complex transports like virtio, while also addressing a few general issues >>> already potentially affecting existing transports. >>> >>> In terms of rework I dropped original V3 patches 05/06/07/08/12 as no more >>> needed, and modified where needed the remaining original patches to take >>> advantage of the above mentioned new SCMI transport features. >>> >>> DT bindings patch has been ported on top of freshly YAML converted arm,scmi >>> bindings. >>> >>> Moreover, since V5 I dropped support for polling mode from the virtio-scmi >>> transport, since it is an optional general mechanism provided by the core >>> to allow transports lacking a completion IRQ to work and it seemed a >>> needless addition/complication in the context of virtio transport. >>> >> >> Just for correctness, in my understanding polling is not completely optional >> ATM. Polling would be required by scmi_cpufreq_fast_switch(). But that >> requirement might be irrelevant for now. >> > > Cpufreq core can use .fast_switch (scmi_cpufreq_fast_switch) op only if > policy->fast_switch_enabled is true which in turn reported as true by > the SCMI cpufreq driver iff SCMI FastChannels are supported by Perf > implementation server side, but the SCMI Device VirtIO spec (5.17) > explicitly does NOT support SCMI FastChannels as of now. > > Anyway, even though we should support in the future SCMI FastChannels on > VirtIO SCMI transport, fastchannels are by defintion per-protocol/per-command/ > per-domain-id specific, based on sharedMem or MMIO, unidirectional and do not > even allow for a response from the platform (SCMIV3.0 4.1.1 5.3) so polling > won't be a thing anyway unless I'm missing something. > > BUT you made a good point in fact anyway, because the generic perf->freq_set/get > API CAN be indeed invoked in polling mode, and, even though we do not use them > in polling as of now (if not in the FastChannel scenario above) this could be a > potential problem in general if when the underlying transport do not support poll > the core just drop any poll_completion=true messages. > > So, while I still think it is not sensible to enable poll mode in SCMI Virtio, > because would be a sort of faked polling and increases complexity, I'm now > considering the fact that maybe the right behaviour of the SCMI core in such a > scenario would be to warn the user as it does now AND then fallback to use > non-polling, probably better if such a behavior is made condtional on some > transport config desc flag that allow such fallback behavior. > > Any thought ? > Maybe the SCMI protocols should request "atomic" instead of "polling"? That semantics are the actual intent in my understanding. So the "Introduce atomic support for SCMI transports" patch series [1] could potentially address this? Best regards, Peter [1] https://lkml.org/lkml/2021/7/12/3089
Hi Cristian, I am currently working on an interface for VMs to communicate their performance requirements to the hosts by passing through cpu frequency adjustments. Your patch looks very interesting but I have some questions: On Mon, Jul 12, 2021 at 03:18:16PM +0100, Cristian Marussi wrote: > > The series has been tested using an emulated fake SCMI device and also a > proper SCP-fw stack running through QEMU vhost-users, with the SCMI stack > compiled, in both cases, as builtin and as a loadable module, running tests > against mocked SCMI Sensors using HWMON and IIO interfaces to check the > functionality of notifications and sync/async commands. > > Virtio-scmi support has been exercised in the following testing scenario > on a JUNO board: > > - normal sync/async command transfers > - notifications > - concurrent delivery of correlated response and delayed responses > - out-of-order delivery of delayed responses before related responses > - unexpected delayed response delivery for sync commands > - late delivery of timed-out responses and delayed responses > > Some basic regression testing against mailbox transport has been performed > for commands and notifications too. > > No sensible overhead in total handling time of commands and notifications > has been observed, even though this series do indeed add a considerable > amount of code to execute on TX path. > More test and measurements could be needed in these regards. > Can you share any data and benchmarks using you fake SCMI device. Also, could you provide the emulated device code so that the results can be reproduced. Cheers, Floris
On Wed, Aug 11, 2021 at 09:31:21AM +0000, Floris Westermann wrote: > Hi Cristian, > Hi Floris, > I am currently working on an interface for VMs to communicate their > performance requirements to the hosts by passing through cpu frequency > adjustments. > So something like looking up SCMI requests from VMs in the hypervisor and act on VM underlying hw accordingly ? Where the SCMI server is meant to live ? > Your patch looks very interesting but I have some questions: > Happy to hear that, a new V7 (with minor cleanups) which is (hopefully) being pulled these days is at: https://lore.kernel.org/linux-arm-kernel/20210803131024.40280-1-cristian.marussi@arm.com/ > > On Mon, Jul 12, 2021 at 03:18:16PM +0100, Cristian Marussi wrote: > > > > The series has been tested using an emulated fake SCMI device and also a > > proper SCP-fw stack running through QEMU vhost-users, with the SCMI stack > > compiled, in both cases, as builtin and as a loadable module, running tests > > against mocked SCMI Sensors using HWMON and IIO interfaces to check the > > functionality of notifications and sync/async commands. > > > > Virtio-scmi support has been exercised in the following testing scenario > > on a JUNO board: > > > > - normal sync/async command transfers > > - notifications > > - concurrent delivery of correlated response and delayed responses > > - out-of-order delivery of delayed responses before related responses > > - unexpected delayed response delivery for sync commands > > - late delivery of timed-out responses and delayed responses > > > > Some basic regression testing against mailbox transport has been performed > > for commands and notifications too. > > > > No sensible overhead in total handling time of commands and notifications > > has been observed, even though this series do indeed add a considerable > > amount of code to execute on TX path. > > More test and measurements could be needed in these regards. > > > > Can you share any data and benchmarks using you fake SCMI device. > Also, could you provide the emulated device code so that the results can > be reproduced. > Not really, because the testing based on the fake SCMI VirtIO device was purely functional, just to exercise some rare limit conditions not easily reproducible with a regular SCMI stack, I've made no benchmark using the fake emulated SCMI virtio device because it mimics VirtIO transfers but there's not even an host/guest in my fake emulation. Moreover is a hacked driver+userspace blob not really in a state to be shared :P While developing this series I needed somehow to be able to let the Kernel SCMI-agent in the guest "speak" some basic SCMI commands and notifs to some SCMI server platform sitting somewhere across the new VirtIO SCMI transport, which basically means that it's not enough to create an SCMI VirtIO device reachable from the VirtIO layer, the SCMI stack itself (or part of it) must live behind such device somehow/somewhere to be able to receive meaningful replies. One proper way to do that is to use some QEMU/SCP-fw vhost-users support cooked by Linaro (not in the upstream for now) so as to basically run a full proper SCP SCMI-platform fw stack in host userspace and let it speak to a guest through the new scmi virtio transport and the vhost-users magic: the drawback of this kind of approach is that it made hard to test limit conditions like stale or out-of-order SCMI replies because to do so you have to patch the official SCP/SCMI stack to behave badly and out-of specs, which is not something is designed to do. (and also the fact that the vhost-users QEMU/SCP-fw solution was only available later during devel). Hence the emulation hack for testing rare limit conditions. Now, the emulation hack, beside really ugly, is clearly a totally fake VirtIO environment (there's not even a guest really...) and, as said, it just served the need to exercise the SCMI virtio transport code enough to test anomalous and bad-behaving SCMI commands flows and notifications and as such made really no sense to be used as a performance testbed. In fact, what I was really meaning (poorly) while saying: > > No sensible overhead in total handling time of commands and notifications > > has been observed, even though this series do indeed add a considerable > > amount of code to execute on TX path. is that I have NOT seen any sensible overhead/slowdown in the context of OTHER real SCMI transports (like mailboxes), because the virtio series contains a number of preliminary SCMI common core changes unrelated to virtio (monotonic tokens/handle concurrent and out-of-order replies) that, even though easing the devel of SCMI virtio, are really needed and used by any other existent SCMI transport, so my fear was to introduce some common slowdown in the core: in those regards only, I said that I have not seen (looking at cmd traces) any slowdown with such additional core changes even though more code is clearly now run in the TX path. (contention can indeed only happen under very rare limit conditions) Having said that (sorry for the flood of words) what I can give you are a few traces (non statistically significant probably) showing the typical round-trip time for some plain SCMI command sensor requests on an idle system (JUNO) In both cases the sensors being read are mocked, so the time is purely related to SCMI core stack and virtio exchanges (i.e. there's no delay introduced by reading real hw sensors) -> Using a proper SCP-fw/QEMU vhost-users stack: root@deb-guest:~# cat /sys/class/hwmon/hwmon0/temp1_input cat-195 [000] .... 7044.614295: scmi_xfer_begin: transfer_id=27 msg_id=6 protocol_id=21 seq=27 poll=0 25000 <idle>-0 [000] d.h3 7044.615342: scmi_rx_done: transfer_id=27 msg_id=6 protocol_id=21 seq=27 msg_type=0 cat-195 [000] .... 7044.615420: scmi_xfer_end: transfer_id=27 msg_id=6 protocol_id=21 seq=27 status=0 root@deb-guest:~# cat /sys/class/hwmon/hwmon0/temp1_input cat-196 [000] .... 7049.200349: scmi_xfer_begin: transfer_id=28 msg_id=6 protocol_id=21 seq=28 poll=0 <idle>-0 [000] d.h3 7049.202053: scmi_rx_done: transfer_id=28 msg_id=6 protocol_id=21 seq=28 msg_type=0 cat-196 [000] .... 7049.202152: scmi_xfer_end: transfer_id=28 msg_id=6 protocol_id=21 seq=28 status=0 25000 root@deb-guest:~# cat /sys/class/hwmon/hwmon0/temp1_input cat-197 [000] .... 7053.699713: scmi_xfer_begin: transfer_id=29 msg_id=6 protocol_id=21 seq=29 poll=0 25000 <idle>-0 [000] d.H3 7053.700366: scmi_rx_done: transfer_id=29 msg_id=6 protocol_id=21 seq=29 msg_type=0 cat-197 [000] .... 7053.700468: scmi_xfer_end: transfer_id=29 msg_id=6 protocol_id=21 seq=29 status=0 root@deb-guest:~# cat /sys/class/hwmon/hwmon0/temp1_input cat-198 [001] .... 7058.944442: scmi_xfer_begin: transfer_id=30 msg_id=6 protocol_id=21 seq=30 poll=0 cat-173 [000] d.h2 7058.944959: scmi_rx_done: transfer_id=30 msg_id=6 protocol_id=21 seq=30 msg_type=0 cat-198 [001] .... 7058.945500: scmi_xfer_end: transfer_id=30 msg_id=6 protocol_id=21 seq=30 status=0 25000 root@deb-guest:~# cat /sys/class/hwmon/hwmon0/temp1_input cat-199 [000] .... 7064.598797: scmi_xfer_begin: transfer_id=31 msg_id=6 protocol_id=21 seq=31 poll=0 25000 <idle>-0 [000] d.h3 7064.599710: scmi_rx_done: transfer_id=31 msg_id=6 protocol_id=21 seq=31 msg_type=0 cat-199 [000] .... 7064.599787: scmi_xfer_end: transfer_id=31 msg_id=6 protocol_id=21 seq=31 status=0 -> Using the fake hack SCMI device that relays packets to userspace: cat-1306 [000] .... 7614.373161: scmi_xfer_begin: transfer_id=78 msg_id=6 protocol_id=21 seq=78 poll=0 scmi_sniffer_ng-342 [000] d.h2 7614.373699: scmi_rx_done: transfer_id=78 msg_id=6 protocol_id=21 seq=78 msg_type=0 cat-1306 [000] .... 7614.377653: scmi_xfer_end: transfer_id=78 msg_id=6 protocol_id=21 seq=78 status=0 cat-1308 [004] .... 7626.677176: scmi_xfer_begin: transfer_id=79 msg_id=6 protocol_id=21 seq=79 poll=0 scmi_sniffer_ng-342 [000] d.h2 7626.677653: scmi_rx_done: transfer_id=79 msg_id=6 protocol_id=21 seq=79 msg_type=0 cat-1308 [004] .... 7626.677705: scmi_xfer_end: transfer_id=79 msg_id=6 protocol_id=21 seq=79 status=0 cat-1309 [004] .... 7631.249412: scmi_xfer_begin: transfer_id=80 msg_id=6 protocol_id=21 seq=80 poll=0 scmi_sniffer_ng-342 [000] d.h2 7631.250182: scmi_rx_done: transfer_id=80 msg_id=6 protocol_id=21 seq=80 msg_type=0 cat-1309 [004] .... 7631.250237: scmi_xfer_end: transfer_id=80 msg_id=6 protocol_id=21 seq=80 status=0 cat-1312 [004] .... 7642.210034: scmi_xfer_begin: transfer_id=81 msg_id=6 protocol_id=21 seq=81 poll=0 scmi_sniffer_ng-342 [000] d.h2 7642.210514: scmi_rx_done: transfer_id=81 msg_id=6 protocol_id=21 seq=81 msg_type=0 cat-1312 [004] .... 7642.210567: scmi_xfer_end: transfer_id=81 msg_id=6 protocol_id=21 seq=81 status=0 cat-1314 [003] .... 7645.810775: scmi_xfer_begin: transfer_id=82 msg_id=6 protocol_id=21 seq=82 poll=0 scmi_sniffer_ng-342 [000] d.h2 7645.811255: scmi_rx_done: transfer_id=82 msg_id=6 protocol_id=21 seq=82 msg_type=0 cat-1314 [003] .... 7645.811307: scmi_xfer_end: transfer_id=82 msg_id=6 protocol_id=21 seq=82 status=0 In both cases SCMI requests are effectively relayed to userspace so that's probably the reason timings are similar. (despite the hackish internals of latter solution) Not sure if all the above madness helped you at all :D Thanks, Cristian