Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used ring
On Thu, Oct 29, 2020 at 2:35 PM Dixit, Ashutosh wrote: > On Thu, 29 Oct 2020 04:53:09 -0700, Arnd Bergmann wrote: > > > > - PCIe endpoint, with the endpoint controlling the virtio configuration > > - PCIe endpoint, with the host (the side that has the pci_driver) > > controlling > > the virtio configuration > > - NTB connections > > - your loopback mode > > - Virtio tunnels between VM guests (see > > https://www.linaro.org/projects/#STR) > > - Intel MIC (to be removed, but it would be wrong to make assumptions that > > cannot be made on that type of hardware) > > A virtio interface being one between host and guest is inherently > asymmetric. The whole innovation of the VOP design was to treat Linux on a > PCIe device as a guest, there was even talk at some point of the "guest" > being managed via libvirt. So here host and guest retain their specific > role/personality. The host "inserts" devices which appear in the guest > e.g. So I am not sure how this asymmetry plays in the scenarios mentioned > above. This is the reason I listed the PCIe endpoint mode twice. I expect that some use cases require the same setup as MIC, with Linux on some kind of PCIe add-on card using devices that are implemented on the host. In other cases we may need the opposite: you may have a PCIe add-on card that provides arbitrary services to the host in the same way that most PCIe endpoint devices work. An example might be a smart NIC running a standalone Linux, but implementing virtio-net to avoid the need for custom drivers on the host. In the endpoint framework, it is always the endpoint that decides what PCI devices it wants to implement, but in case of VOP that device could be either the side that configures and implements the virtio devices, or the side that probes and uses them. Arnd
Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used ring
On Thu, 29 Oct 2020 04:53:09 -0700, Arnd Bergmann wrote: > > On Thu, Oct 29, 2020 at 11:07 AM Vincent Whitchurch > wrote: > > > > On Wed, Oct 28, 2020 at 04:50:36PM +0100, Arnd Bergmann wrote: > > > I think we should try to do something on top of the PCIe endpoint > > > subsystem > > > to make it work across arbitrary combinations of host and device > > > implementations, > > > and provide a superset of what the MIC driver, (out-of-tree) Bluefield > > > endpoint > > > driver, and the NTB subsystem as well as a couple of others used to do, > > > each of them tunneling block/network/serial/... over a PCIe link of some > > > sort, usually with virtio. > > > > VOP is not PCIe-specific (as demonstrated by the vop-loopback patches I > > posted a while ago [1]), and it would be a shame for a replacement to be > > tied to the PCIe endpoint subsystem. There are many SOCs out there > > which have multiple Linux-capable processors without cache-coherency > > between them. VOP is (or should I say was since I guess it's being > > deleted) the closest we have in mainline to easily get generic virtio > > (and not just rpmsg) running between these kind of Linux instances. If > > a new replacement framework were to be PCIe-exclusive then we'd have to > > invent one more framework for non-PCIe links to do pretty much the same > > thing. > > > > [1] > > https://lore.kernel.org/lkml/20190403104746.16063-1-vincent.whitchu...@axis.com/ > > Right, sorry I forgot about that. I think this means we should keep having > an abstraction between VOP (under whichever name) and the lower levels, > and be aware that it might run on any number of these: > > - PCIe endpoint, with the endpoint controlling the virtio configuration > - PCIe endpoint, with the host (the side that has the pci_driver) controlling > the virtio configuration > - NTB connections > - your loopback mode > - Virtio tunnels between VM guests (see https://www.linaro.org/projects/#STR) > - Intel MIC (to be removed, but it would be wrong to make assumptions that > cannot be made on that type of hardware) A virtio interface being one between host and guest is inherently asymmetric. The whole innovation of the VOP design was to treat Linux on a PCIe device as a guest, there was even talk at some point of the "guest" being managed via libvirt. So here host and guest retain their specific role/personality. The host "inserts" devices which appear in the guest e.g. So I am not sure how this asymmetry plays in the scenarios mentioned above.
Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used ring
On Thu, 29 Oct 2020 03:07:27 -0700, Vincent Whitchurch wrote: > > On Wed, Oct 28, 2020 at 04:50:36PM +0100, Arnd Bergmann wrote: > > I think we should try to do something on top of the PCIe endpoint subsystem > > to make it work across arbitrary combinations of host and device > > implementations, > > and provide a superset of what the MIC driver, (out-of-tree) Bluefield > > endpoint > > driver, and the NTB subsystem as well as a couple of others used to do, > > each of them tunneling block/network/serial/... over a PCIe link of some > > sort, usually with virtio. > > VOP is not PCIe-specific (as demonstrated by the vop-loopback patches I > posted a while ago [1]), and it would be a shame for a replacement to be > tied to the PCIe endpoint subsystem. There are many SOCs out there > which have multiple Linux-capable processors without cache-coherency > between them. VOP is (or should I say was since I guess it's being > deleted) the closest we have in mainline to easily get generic virtio > (and not just rpmsg) running between these kind of Linux instances. If > a new replacement framework were to be PCIe-exclusive then we'd have to > invent one more framework for non-PCIe links to do pretty much the same > thing. > > [1] > https://lore.kernel.org/lkml/20190403104746.16063-1-vincent.whitchu...@axis.com/ It may be possible to use VOP in other instances but it was optimized for PCIe. The rings were specifically placed to avoid doing reads across PCIe and only do writes across PCIe.
Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used ring
On Thu, Oct 29, 2020 at 11:07 AM Vincent Whitchurch wrote: > > On Wed, Oct 28, 2020 at 04:50:36PM +0100, Arnd Bergmann wrote: > > I think we should try to do something on top of the PCIe endpoint subsystem > > to make it work across arbitrary combinations of host and device > > implementations, > > and provide a superset of what the MIC driver, (out-of-tree) Bluefield > > endpoint > > driver, and the NTB subsystem as well as a couple of others used to do, > > each of them tunneling block/network/serial/... over a PCIe link of some > > sort, usually with virtio. > > VOP is not PCIe-specific (as demonstrated by the vop-loopback patches I > posted a while ago [1]), and it would be a shame for a replacement to be > tied to the PCIe endpoint subsystem. There are many SOCs out there > which have multiple Linux-capable processors without cache-coherency > between them. VOP is (or should I say was since I guess it's being > deleted) the closest we have in mainline to easily get generic virtio > (and not just rpmsg) running between these kind of Linux instances. If > a new replacement framework were to be PCIe-exclusive then we'd have to > invent one more framework for non-PCIe links to do pretty much the same > thing. > > [1] > https://lore.kernel.org/lkml/20190403104746.16063-1-vincent.whitchu...@axis.com/ Right, sorry I forgot about that. I think this means we should keep having an abstraction between VOP (under whichever name) and the lower levels, and be aware that it might run on any number of these: - PCIe endpoint, with the endpoint controlling the virtio configuration - PCIe endpoint, with the host (the side that has the pci_driver) controlling the virtio configuration - NTB connections - your loopback mode - Virtio tunnels between VM guests (see https://www.linaro.org/projects/#STR) - Intel MIC (to be removed, but it would be wrong to make assumptions that cannot be made on that type of hardware) - ... The existing VOP codebase does look like a reasonable start, though I think we need to discuss whether the ioctl interface should be replaced with a configfs interface, and what other changes would be needed to make it support the generalized hardware case. Arnd
Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used ring
On Thu, Oct 29, 2020 at 11:07:27AM +0100, Vincent Whitchurch wrote: > On Wed, Oct 28, 2020 at 04:50:36PM +0100, Arnd Bergmann wrote: > > I think we should try to do something on top of the PCIe endpoint subsystem > > to make it work across arbitrary combinations of host and device > > implementations, > > and provide a superset of what the MIC driver, (out-of-tree) Bluefield > > endpoint > > driver, and the NTB subsystem as well as a couple of others used to do, > > each of them tunneling block/network/serial/... over a PCIe link of some > > sort, usually with virtio. > > VOP is not PCIe-specific (as demonstrated by the vop-loopback patches I > posted a while ago [1]), and it would be a shame for a replacement to be > tied to the PCIe endpoint subsystem. There are many SOCs out there > which have multiple Linux-capable processors without cache-coherency > between them. VOP is (or should I say was since I guess it's being > deleted) the closest we have in mainline to easily get generic virtio > (and not just rpmsg) running between these kind of Linux instances. If > a new replacement framework were to be PCIe-exclusive then we'd have to > invent one more framework for non-PCIe links to do pretty much the same > thing. > > [1] > https://lore.kernel.org/lkml/20190403104746.16063-1-vincent.whitchu...@axis.com/ If this works well, please restore the existing code and move it to a new directory and we can take it from there, right? thanks, greg k-h
Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used ring
On Wed, Oct 28, 2020 at 04:50:36PM +0100, Arnd Bergmann wrote: > I think we should try to do something on top of the PCIe endpoint subsystem > to make it work across arbitrary combinations of host and device > implementations, > and provide a superset of what the MIC driver, (out-of-tree) Bluefield > endpoint > driver, and the NTB subsystem as well as a couple of others used to do, > each of them tunneling block/network/serial/... over a PCIe link of some > sort, usually with virtio. VOP is not PCIe-specific (as demonstrated by the vop-loopback patches I posted a while ago [1]), and it would be a shame for a replacement to be tied to the PCIe endpoint subsystem. There are many SOCs out there which have multiple Linux-capable processors without cache-coherency between them. VOP is (or should I say was since I guess it's being deleted) the closest we have in mainline to easily get generic virtio (and not just rpmsg) running between these kind of Linux instances. If a new replacement framework were to be PCIe-exclusive then we'd have to invent one more framework for non-PCIe links to do pretty much the same thing. [1] https://lore.kernel.org/lkml/20190403104746.16063-1-vincent.whitchu...@axis.com/
Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used ring
On Thu, Oct 29, 2020 at 3:42 AM Sherry Sun wrote: > > Subject: Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the > > used ring > > Thanks for your detailed reply. > Yes, the PCIe endpoint subsystem is the base code, actually we have > implemented a set > of pci endpoint code similar to pci-epf-test.c, combine with vop (Virtio Over > PCIe). > > But now the vop code is going to be removed, we planned to change to NTB > framework, > I saw Kishon has done some jobs based on NTB and PCIe endpoint subsystem, > will get > a deep look. Maybe it is a good solution. Ok, great. Regarding the VoP code, I think nothing stops you from reusing anything you find useful in there and build on top of it, but we should consider it as a new submission then, which means that you are free to change the interfaces without worrying about backwards compatibility. On the flip side, this also means we need to carefully review the interface to make sure we can cover the requirements of as many users as possible without adding too much complexity for cases that we do not care about. Arnd
RE: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used ring
Hi Arnd, > Subject: Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used > ring > > (resending from the kernel.org address after getting bounces again) > > On Wed, Oct 28, 2020 at 7:29 AM Sherry Sun wrote: > > > Subject: Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign > > > the used > > > > > > Both Ashutosh and I have moved on to other projects. The MIC devices > > > have been discontinued. I have just sent across a patch to remove > > > the MIC drivers from the kernel tree. > > > > > > We are very glad to see that Sherry is able to reuse some of the VOP > > > logic and it is working well. It is best if the MIC drivers are > > > removed so Sherry can add the specific VOP logic required for imx8qm > > > subsequently without having to worry about other driver dependencies. > > > Hoping this results in a cleaner imx8qm driver moving forward. > > > > I'm ok with your patch. > > Since you have deprecated the MIC related code, may I ask do you have > > a better solution instead of vop/scif? > > I think we should try to do something on top of the PCIe endpoint subsystem > to make it work across arbitrary combinations of host and device > implementations, and provide a superset of what the MIC driver, (out-of- > tree) Bluefield endpoint driver, and the NTB subsystem as well as a couple of > others used to do, each of them tunneling block/network/serial/... over a > PCIe link of some sort, usually with virtio. > > At the moment, there is only one driver for the endpoint framework in the > kernel, in drivers/pci/endpoint/functions/pci-epf-test.c, but I think this can > serve as a starting point. > Thanks for your detailed reply. Yes, the PCIe endpoint subsystem is the base code, actually we have implemented a set of pci endpoint code similar to pci-epf-test.c, combine with vop (Virtio Over PCIe). But now the vop code is going to be removed, we planned to change to NTB framework, I saw Kishon has done some jobs based on NTB and PCIe endpoint subsystem, will get a deep look. Maybe it is a good solution. Best regards Sherry > The PCI endpoint subsystem already uses configfs for configuring the > available devices, and this seems like a good fit for making it work in > general. > However, there are a number of use cases that have somewhat conflicting > requirements, so the first step would be to figure out what everyone actually > needs for virtio communication. > > These are some of the main differences that I have noticed in the > past: > > - The simple case would be to use one PCIe endpoint device > for each virtio device, but I think this needs to be multiplexed > so that hardware that only supports a single PCIe endpoint > can still have multiple virtio devices tunneled through it. > > - While sometimes the configuration is hardcoded in the driver, ideally > the type of virtio device(s) that is tunneled over the PCIe link should > be configurable. The configuration of the endpoint device itself is > done on the machine running on the endpoint side, but for the > virtio devices, this might be either on the host or the endpoint. > Not sure if one of the two ways is common enough, or we have to > allow both. > > - When the link is configured, you still need one side to provide a > virtio device host implementation, while the other side would > run the normal virtio device driver. Again, these could be done > either way, and it is independent of which side has configured > the link, and we might want to only allow one of the two options, > or do both, or tie it to who configures it (e.g. the side that creates > the device must be the virtio device host, while the other side > just sees the device pop up and uses a virtio driver). > >Arnd
RE: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used ring
Hi Vincent, > Subject: Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used > ring > > On Tue, Oct 27, 2020 at 08:05:43AM +0100, Sherry Sun wrote: > > Can you help test the patch about removing the codes of reassign used > > ring, and comment on the impact for Intel MIC platform? Thanks for > > any help. > > I don't have access to MIC hardware myself, either. > > But this patch is quite certainly going to break it since guests using a > kernel > without the patch will not work with hosts with a kernel with the patch. Thanks for your reply. This patch can be used by both guests and hosts. I have tested it on imx8qm platform(both guest and host are ARM64 architecture), and it works well. So I guess Intel MIC won't meet big problems when both guest and host apply this patch. But it is best if it can be tested. Best regards Sherry
RE: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used ring
Hi Sudeep, > Subject: Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used > ring > > On Wed, 2020-10-28 at 01:47 +, Sherry Sun wrote: > > Hi Vincent, > > > > > Subject: Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign > > > the used ring > > > > > > On Tue, Oct 27, 2020 at 08:05:43AM +0100, Sherry Sun wrote: > > > > Can you help test the patch about removing the codes of reassign > > > > used ring, and comment on the impact for Intel MIC platform? > > > > Thanks for any help. > > > > > > I don't have access to MIC hardware myself, either. > > > > > > But this patch is quite certainly going to break it since guests > > > using a kernel without the patch will not work with hosts with a > > > kernel with the patch. > > > > Thanks for your reply. > > This patch can be used by both guests and hosts. > > I have tested it on imx8qm platform(both guest and host are ARM64 > > architecture), and it works well. > > So I guess Intel MIC won't meet big problems when both guest and > > Hi Greg/Sherry, > > Both Ashutosh and I have moved on to other projects. The MIC devices have > been discontinued. I have just sent across a patch to remove the MIC drivers > from the kernel tree. > > We are very glad to see that Sherry is able to reuse some of the VOP logic > and it is working well. It is best if the MIC drivers are removed so Sherry > can > add the specific VOP logic required for imx8qm subsequently without having > to worry about other driver dependencies. > Hoping this results in a cleaner imx8qm driver moving forward. I'm ok with your patch. Since you have deprecated the MIC related code, may I ask do you have a better solution instead of vop/scif? Best regards Sherry > > Thanks, > Sudeep Dutt
Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used ring
(resending from the kernel.org address after getting bounces again) On Wed, Oct 28, 2020 at 7:29 AM Sherry Sun wrote: > > Subject: Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used > > > > Both Ashutosh and I have moved on to other projects. The MIC devices have > > been discontinued. I have just sent across a patch to remove the MIC drivers > > from the kernel tree. > > > > We are very glad to see that Sherry is able to reuse some of the VOP logic > > and it is working well. It is best if the MIC drivers are removed so Sherry > > can > > add the specific VOP logic required for imx8qm subsequently without having > > to worry about other driver dependencies. > > Hoping this results in a cleaner imx8qm driver moving forward. > > I'm ok with your patch. > Since you have deprecated the MIC related code, may I ask do you have > a better solution instead of vop/scif? I think we should try to do something on top of the PCIe endpoint subsystem to make it work across arbitrary combinations of host and device implementations, and provide a superset of what the MIC driver, (out-of-tree) Bluefield endpoint driver, and the NTB subsystem as well as a couple of others used to do, each of them tunneling block/network/serial/... over a PCIe link of some sort, usually with virtio. At the moment, there is only one driver for the endpoint framework in the kernel, in drivers/pci/endpoint/functions/pci-epf-test.c, but I think this can serve as a starting point. The PCI endpoint subsystem already uses configfs for configuring the available devices, and this seems like a good fit for making it work in general. However, there are a number of use cases that have somewhat conflicting requirements, so the first step would be to figure out what everyone actually needs for virtio communication. These are some of the main differences that I have noticed in the past: - The simple case would be to use one PCIe endpoint device for each virtio device, but I think this needs to be multiplexed so that hardware that only supports a single PCIe endpoint can still have multiple virtio devices tunneled through it. - While sometimes the configuration is hardcoded in the driver, ideally the type of virtio device(s) that is tunneled over the PCIe link should be configurable. The configuration of the endpoint device itself is done on the machine running on the endpoint side, but for the virtio devices, this might be either on the host or the endpoint. Not sure if one of the two ways is common enough, or we have to allow both. - When the link is configured, you still need one side to provide a virtio device host implementation, while the other side would run the normal virtio device driver. Again, these could be done either way, and it is independent of which side has configured the link, and we might want to only allow one of the two options, or do both, or tie it to who configures it (e.g. the side that creates the device must be the virtio device host, while the other side just sees the device pop up and uses a virtio driver). Arnd
Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used ring
On Wed, Oct 28, 2020 at 02:47:49AM +0100, Sherry Sun wrote: > > Subject: Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used > > ring > > > > On Tue, Oct 27, 2020 at 08:05:43AM +0100, Sherry Sun wrote: > > > Can you help test the patch about removing the codes of reassign used > > > ring, and comment on the impact for Intel MIC platform? Thanks for > > > any help. > > > > I don't have access to MIC hardware myself, either. > > > > But this patch is quite certainly going to break it since guests using a > > kernel > > without the patch will not work with hosts with a kernel with the patch. > > Thanks for your reply. > This patch can be used by both guests and hosts. > I have tested it on imx8qm platform(both guest and host are ARM64 > architecture), and it works well. > So I guess Intel MIC won't meet big problems when both guest and host > apply this patch. But it is best if it can be tested. The guest and host are different systems and it should be possible to upgrade the host kernel without being forced to upgrade the guest kernel, and vice-versa. This patch breaks that. If MIC hardware has no users and the drivers are being deleted then of course backward compatibility doesn't matter.
Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used ring
On Tue, Oct 27, 2020 at 08:05:43AM +0100, Sherry Sun wrote: > Can you help test the patch about removing the codes of reassign used > ring, and comment on the impact for Intel MIC platform? Thanks for > any help. I don't have access to MIC hardware myself, either. But this patch is quite certainly going to break it since guests using a kernel without the patch will not work with hosts with a kernel with the patch.
RE: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used ring
Hi Sudeep & Ashutosh & Vincent, Can you help test the patch about removing the codes of reassign used ring, and comment on the impact for Intel MIC platform? Thanks for any help. Best regards Sherry > Subject: Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used > ring > > On Mon, Oct 26, 2020 at 03:04:45AM +, Sherry Sun wrote: > > Hi Greg & Christoph, > > > > > Subject: Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign > > > the used ring > > > > > > On Thu, Oct 22, 2020 at 01:06:36PM +0800, Sherry Sun wrote: > > > > We don't need to allocate and reassign the used ring here and > > > > remove the used_address_updated flag.Since RC has allocated the > > > > entire vring, including the used ring. Simply add the > > > > corresponding offset can get the used ring address. > > > > > > Someone needs to verify this vs the existing intel implementations. > > > > Hi Greg, @gre...@linuxfoundation.org, sorry I don't have the Intel MIC > devices so cannot test it, do you know anyone who can help test this patch > changes on the Intel MIC platform? Thanks. > > Why not ask the authors/maintainers of that code? > > greg k-h
Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used ring
On Mon, Oct 26, 2020 at 03:04:45AM +, Sherry Sun wrote: > Hi Greg & Christoph, > > > Subject: Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used > > ring > > > > On Thu, Oct 22, 2020 at 01:06:36PM +0800, Sherry Sun wrote: > > > We don't need to allocate and reassign the used ring here and remove > > > the used_address_updated flag.Since RC has allocated the entire vring, > > > including the used ring. Simply add the corresponding offset can get > > > the used ring address. > > > > Someone needs to verify this vs the existing intel implementations. > > Hi Greg, @gre...@linuxfoundation.org, sorry I don't have the Intel MIC > devices so cannot test it, do you know anyone who can help test this patch > changes on the Intel MIC platform? Thanks. Why not ask the authors/maintainers of that code? greg k-h
RE: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used ring
Hi Greg & Christoph, > Subject: Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used > ring > > On Thu, Oct 22, 2020 at 01:06:36PM +0800, Sherry Sun wrote: > > We don't need to allocate and reassign the used ring here and remove > > the used_address_updated flag.Since RC has allocated the entire vring, > > including the used ring. Simply add the corresponding offset can get > > the used ring address. > > Someone needs to verify this vs the existing intel implementations. Hi Greg, @gre...@linuxfoundation.org, sorry I don't have the Intel MIC devices so cannot test it, do you know anyone who can help test this patch changes on the Intel MIC platform? Thanks. > > > - used = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, > > - get_order(vdev->used_size[index])); > > + used = va + PAGE_ALIGN(sizeof(struct vring_desc) * > > +le16_to_cpu(config.num) + > > This adds an over 80 char line. Hi Christoph, will fix it in V4, thanks. > > > + vdev->used[index] = config.address + PAGE_ALIGN(sizeof(struct > > +vring_desc) * le16_to_cpu(config.num) + > > Again. Best regards Sherry
Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used ring
On Thu, Oct 22, 2020 at 01:06:36PM +0800, Sherry Sun wrote: > We don't need to allocate and reassign the used ring here and remove the > used_address_updated flag.Since RC has allocated the entire vring, > including the used ring. Simply add the corresponding offset can get the > used ring address. Someone needs to verify this vs the existing intel implementations. > - used = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, > - get_order(vdev->used_size[index])); > + used = va + PAGE_ALIGN(sizeof(struct vring_desc) * > le16_to_cpu(config.num) + This adds an over 80 char line. > + vdev->used[index] = config.address + PAGE_ALIGN(sizeof(struct > vring_desc) * le16_to_cpu(config.num) + Again.
Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used ring
Hi Sherry, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on char-misc/char-misc-testing] [also build test WARNING on soc/for-next linus/master v5.9 next-20201022] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Sherry-Sun/Change-vring-space-from-nomal-memory-to-dma-coherent-memory/20201022-131008 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git f3277cbfba763cd2826396521b9296de67cf1bbc config: i386-randconfig-s002-20201022 (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 reproduce: # apt-get install sparse # sparse version: v0.6.3-dirty # https://github.com/0day-ci/linux/commit/6ae9d6d36b63c7bb8170f4c0409470d8e7101880 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Sherry-Sun/Change-vring-space-from-nomal-memory-to-dma-coherent-memory/20201022-131008 git checkout 6ae9d6d36b63c7bb8170f4c0409470d8e7101880 # save the attached .config to linux build tree make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot "sparse warnings: (new ones prefixed by >>)" >> drivers/misc/mic/vop/vop_main.c:339:14: sparse: sparse: incorrect type in >> assignment (different address spaces) @@ expected void *used @@ got >> void [noderef] __iomem * @@ >> drivers/misc/mic/vop/vop_main.c:339:14: sparse: expected void *used >> drivers/misc/mic/vop/vop_main.c:339:14: sparse: got void [noderef] >> __iomem * >> drivers/misc/mic/vop/vop_main.c:357:35: sparse: sparse: restricted __le64 >> degrades to integer vim +339 drivers/misc/mic/vop/vop_main.c 289 290 /* 291 * This routine will assign vring's allocated in host/io memory. Code in 292 * virtio_ring.c however continues to access this io memory as if it were local 293 * memory without io accessors. 294 */ 295 static struct virtqueue *vop_find_vq(struct virtio_device *dev, 296 unsigned index, 297 void (*callback)(struct virtqueue *vq), 298 const char *name, bool ctx) 299 { 300 struct _vop_vdev *vdev = to_vopvdev(dev); 301 struct vop_device *vpdev = vdev->vpdev; 302 struct mic_vqconfig __iomem *vqconfig; 303 struct mic_vqconfig config; 304 struct virtqueue *vq; 305 void __iomem *va; 306 struct _mic_vring_info __iomem *info; 307 void *used; 308 int vr_size, _vr_size, err, magic; 309 u8 type = ioread8(>desc->type); 310 311 if (index >= ioread8(>desc->num_vq)) 312 return ERR_PTR(-ENOENT); 313 314 if (!name) 315 return ERR_PTR(-ENOENT); 316 317 /* First assign the vring's allocated in host memory */ 318 vqconfig = _vop_vq_config(vdev->desc) + index; 319 memcpy_fromio(, vqconfig, sizeof(config)); 320 _vr_size = round_up(vring_size(le16_to_cpu(config.num), MIC_VIRTIO_RING_ALIGN), 4); 321 vr_size = PAGE_ALIGN(_vr_size + sizeof(struct _mic_vring_info)); 322 va = vpdev->hw_ops->remap(vpdev, le64_to_cpu(config.address), vr_size); 323 if (!va) 324 return ERR_PTR(-ENOMEM); 325 vdev->vr[index] = va; 326 memset_io(va, 0x0, _vr_size); 327 328 info = va + _vr_size; 329 magic = ioread32(>magic); 330 331 if (WARN(magic != MIC_MAGIC + type + index, "magic mismatch")) { 332 err = -EIO; 333 goto unmap; 334 } 335 336 vdev->used_size[index] = PAGE_ALIGN(sizeof(__u16) * 3 + 337 sizeof(struct vring_used_elem) * 338 le16_to_cpu(config.num)); > 339 used = va + PAGE_ALIGN(sizeof(struct vring_desc) * > le16_to_cpu(config.num) + 340 sizeof(__u16) * (3 + le16_to_cpu(config.num))); 341 vdev->used_virt[index] = used; 342 if (!used) { 343 err = -ENOMEM; 344 dev_err(_vop_dev(vdev), "%s %d err %d\n", 345 __func__, __LINE__, err); 346 goto unmap; 347 } 348 349 vq = vop_new_virtqueue(index, le16_to_cpu(config.num), dev, ctx, 350 (void __force *)va, vop_notify, callback, 351 name,
[PATCH V3 2/4] misc: vop: do not allocate and reassign the used ring
We don't need to allocate and reassign the used ring here and remove the used_address_updated flag.Since RC has allocated the entire vring, including the used ring. Simply add the corresponding offset can get the used ring address. If following the orginal way to reassign the used ring, will encounter a problem. When host finished with descriptor, it will update the used ring with putused_kern api, if reassign used ring at EP side, used ring will be io device memory for RC, use memcpy in putused_kern will cause kernel panic. Signed-off-by: Sherry Sun Signed-off-by: Joakim Zhang --- drivers/misc/mic/vop/vop_debugfs.c | 2 -- drivers/misc/mic/vop/vop_main.c| 48 -- drivers/misc/mic/vop/vop_vringh.c | 31 --- include/uapi/linux/mic_common.h| 5 ++-- 4 files changed, 8 insertions(+), 78 deletions(-) diff --git a/drivers/misc/mic/vop/vop_debugfs.c b/drivers/misc/mic/vop/vop_debugfs.c index 9d4f175f4dd1..05eca4a98585 100644 --- a/drivers/misc/mic/vop/vop_debugfs.c +++ b/drivers/misc/mic/vop/vop_debugfs.c @@ -79,8 +79,6 @@ static int vop_dp_show(struct seq_file *s, void *pos) seq_printf(s, "Vdev reset %d\n", dc->vdev_reset); seq_printf(s, "Guest Ack %d ", dc->guest_ack); seq_printf(s, "Host ack %d\n", dc->host_ack); - seq_printf(s, "Used address updated %d ", - dc->used_address_updated); seq_printf(s, "Vdev 0x%llx\n", dc->vdev); seq_printf(s, "c2h doorbell %d ", dc->c2h_vdev_db); seq_printf(s, "h2c doorbell %d\n", dc->h2c_vdev_db); diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c index 714b94f42d38..1ccc94dfd6ac 100644 --- a/drivers/misc/mic/vop/vop_main.c +++ b/drivers/misc/mic/vop/vop_main.c @@ -250,10 +250,6 @@ static void vop_del_vq(struct virtqueue *vq, int n) struct _vop_vdev *vdev = to_vopvdev(vq->vdev); struct vop_device *vpdev = vdev->vpdev; - dma_unmap_single(>dev, vdev->used[n], -vdev->used_size[n], DMA_BIDIRECTIONAL); - free_pages((unsigned long)vdev->used_virt[n], - get_order(vdev->used_size[n])); vring_del_virtqueue(vq); vpdev->hw_ops->unmap(vpdev, vdev->vr[n]); vdev->vr[n] = NULL; @@ -340,8 +336,8 @@ static struct virtqueue *vop_find_vq(struct virtio_device *dev, vdev->used_size[index] = PAGE_ALIGN(sizeof(__u16) * 3 + sizeof(struct vring_used_elem) * le16_to_cpu(config.num)); - used = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, - get_order(vdev->used_size[index])); + used = va + PAGE_ALIGN(sizeof(struct vring_desc) * le16_to_cpu(config.num) + + sizeof(__u16) * (3 + le16_to_cpu(config.num))); vdev->used_virt[index] = used; if (!used) { err = -ENOMEM; @@ -355,27 +351,15 @@ static struct virtqueue *vop_find_vq(struct virtio_device *dev, name, used); if (!vq) { err = -ENOMEM; - goto free_used; + goto unmap; } - vdev->used[index] = dma_map_single(>dev, used, - vdev->used_size[index], - DMA_BIDIRECTIONAL); - if (dma_mapping_error(>dev, vdev->used[index])) { - err = -ENOMEM; - dev_err(_vop_dev(vdev), "%s %d err %d\n", - __func__, __LINE__, err); - goto del_vq; - } + vdev->used[index] = config.address + PAGE_ALIGN(sizeof(struct vring_desc) * le16_to_cpu(config.num) + + sizeof(__u16) * (3 + le16_to_cpu(config.num))); writeq(vdev->used[index], >used_address); vq->priv = vdev; return vq; -del_vq: - vring_del_virtqueue(vq); -free_used: - free_pages((unsigned long)used, - get_order(vdev->used_size[index])); unmap: vpdev->hw_ops->unmap(vpdev, vdev->vr[index]); return ERR_PTR(err); @@ -388,9 +372,7 @@ static int vop_find_vqs(struct virtio_device *dev, unsigned nvqs, struct irq_affinity *desc) { struct _vop_vdev *vdev = to_vopvdev(dev); - struct vop_device *vpdev = vdev->vpdev; - struct mic_device_ctrl __iomem *dc = vdev->dc; - int i, err, retry, queue_idx = 0; + int i, err, queue_idx = 0; /* We must have this many virtqueues. */ if (nvqs > ioread8(>desc->num_vq)) @@ -412,24 +394,6 @@ static int vop_find_vqs(struct virtio_device *dev, unsigned nvqs, } } - iowrite8(1, >used_address_updated); - /* -* Send an interrupt to the host to inform it that used -* rings have been re-assigned. -*/ -