| Message ID | 20231103171641.1703146-1-lulu@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | vhost-vdpa: add support for iommufd | expand |
On Sat, Nov 4, 2023 at 1:16 AM Cindy Lu <lulu@redhat.com> wrote: > > > Hi All > This code provides the iommufd support for vdpa device > This code fixes the bugs from the last version and also add the asid support. rebase on kernel > v6,6-rc3 > Test passed in the physical device (vp_vdpa), but there are still some problems in the emulated device (vdpa_sim_net), > I will continue working on it > > The kernel code is > https://gitlab.com/lulu6/vhost/-/tree/iommufdRFC_v1 > > Signed-off-by: Cindy Lu <lulu@redhat.com> It would be better to have a change summary here. Thanks > > > Cindy Lu (8): > vhost/iommufd: Add the functions support iommufd > Kconfig: Add the new file vhost/iommufd > vhost: Add 3 new uapi to support iommufd > vdpa: Add new vdpa_config_ops to support iommufd > vdpa_sim :Add support for iommufd > vdpa: change the map/unmap process to support iommufd > vp_vdpa::Add support for iommufd > iommu: expose the function iommu_device_use_default_domain > > drivers/iommu/iommu.c | 2 + > drivers/vdpa/vdpa_sim/vdpa_sim.c | 8 ++ > drivers/vdpa/virtio_pci/vp_vdpa.c | 4 + > drivers/vhost/Kconfig | 1 + > drivers/vhost/Makefile | 1 + > drivers/vhost/iommufd.c | 178 +++++++++++++++++++++++++ > drivers/vhost/vdpa.c | 210 +++++++++++++++++++++++++++++- > drivers/vhost/vhost.h | 21 +++ > include/linux/vdpa.h | 38 +++++- > include/uapi/linux/vhost.h | 66 ++++++++++ > 10 files changed, 525 insertions(+), 4 deletions(-) > create mode 100644 drivers/vhost/iommufd.c > > -- > 2.34.3 >
On 2023/11/6 12:11, Jason Wang wrote: > On Sat, Nov 4, 2023 at 1:16 AM Cindy Lu <lulu@redhat.com> wrote: >> >> >> Hi All >> This code provides the iommufd support for vdpa device >> This code fixes the bugs from the last version and also add the asid support. rebase on kernel >> v6,6-rc3 >> Test passed in the physical device (vp_vdpa), but there are still some problems in the emulated device (vdpa_sim_net), >> I will continue working on it >> >> The kernel code is >> https://gitlab.com/lulu6/vhost/-/tree/iommufdRFC_v1 >> >> Signed-off-by: Cindy Lu <lulu@redhat.com> > > It would be better to have a change summary here. yeah, the version number is also incorrect. > Thanks > >> >> >> Cindy Lu (8): >> vhost/iommufd: Add the functions support iommufd >> Kconfig: Add the new file vhost/iommufd >> vhost: Add 3 new uapi to support iommufd >> vdpa: Add new vdpa_config_ops to support iommufd >> vdpa_sim :Add support for iommufd >> vdpa: change the map/unmap process to support iommufd >> vp_vdpa::Add support for iommufd >> iommu: expose the function iommu_device_use_default_domain >> >> drivers/iommu/iommu.c | 2 + >> drivers/vdpa/vdpa_sim/vdpa_sim.c | 8 ++ >> drivers/vdpa/virtio_pci/vp_vdpa.c | 4 + >> drivers/vhost/Kconfig | 1 + >> drivers/vhost/Makefile | 1 + >> drivers/vhost/iommufd.c | 178 +++++++++++++++++++++++++ >> drivers/vhost/vdpa.c | 210 +++++++++++++++++++++++++++++- >> drivers/vhost/vhost.h | 21 +++ >> include/linux/vdpa.h | 38 +++++- >> include/uapi/linux/vhost.h | 66 ++++++++++ >> 10 files changed, 525 insertions(+), 4 deletions(-) >> create mode 100644 drivers/vhost/iommufd.c >> >> -- >> 2.34.3 >> >
On Sat, Nov 04, 2023 at 01:16:33AM +0800, Cindy Lu wrote: > > Hi All > This code provides the iommufd support for vdpa device > This code fixes the bugs from the last version and also add the asid support. rebase on kernel > v6,6-rc3 > Test passed in the physical device (vp_vdpa), but there are still some problems in the emulated device (vdpa_sim_net), What kind of problems? Understanding that will make it easier to figure out whether this is worth reviewing. > I will continue working on it > > The kernel code is > https://gitlab.com/lulu6/vhost/-/tree/iommufdRFC_v1 > > Signed-off-by: Cindy Lu <lulu@redhat.com> Please also Cc iommufd maintainers: Jason Gunthorpe <jgg@ziepe.ca> (maintainer:IOMMUFD) Kevin Tian <kevin.tian@intel.com> (maintainer:IOMMUFD) Joerg Roedel <joro@8bytes.org> (maintainer:IOMMU SUBSYSTEM) Will Deacon <will@kernel.org> (maintainer:IOMMU SUBSYSTEM) Robin Murphy <robin.murphy@arm.com> (reviewer:IOMMU SUBSYSTEM) iommu@lists.linux.dev (open list:IOMMUFD) linux-kernel@vger.kernel.org (open list)
On Tue, Nov 07, 2023 at 02:30:34AM -0500, Michael S. Tsirkin wrote: > On Sat, Nov 04, 2023 at 01:16:33AM +0800, Cindy Lu wrote: > > > > Hi All > > This code provides the iommufd support for vdpa device > > This code fixes the bugs from the last version and also add the asid support. rebase on kernel > > v6,6-rc3 > > Test passed in the physical device (vp_vdpa), but there are still some problems in the emulated device (vdpa_sim_net), > > What kind of problems? Understanding that will make it easier > to figure out whether this is worth reviewing. IMHO, this patch series needs to spend more time internally to Red Hat before it is presented to the community. It is too far away from something worth reviewing at this point. Jason
On Sat, Nov 04, 2023 at 01:16:33AM +0800, Cindy Lu wrote:
> Test passed in the physical device (vp_vdpa), but there are still some problems in the emulated device (vdpa_sim_net),
I'm not sure there's even value in bothering with iommufd for the
simulator. Just find a way to disable it and fail gracefully.
On Tue, Nov 07, 2023 at 08:49:02AM -0400, Jason Gunthorpe wrote: > On Tue, Nov 07, 2023 at 02:30:34AM -0500, Michael S. Tsirkin wrote: > > On Sat, Nov 04, 2023 at 01:16:33AM +0800, Cindy Lu wrote: > > > > > > Hi All > > > This code provides the iommufd support for vdpa device > > > This code fixes the bugs from the last version and also add the asid support. rebase on kernel > > > v6,6-rc3 > > > Test passed in the physical device (vp_vdpa), but there are still some problems in the emulated device (vdpa_sim_net), > > > > What kind of problems? Understanding that will make it easier > > to figure out whether this is worth reviewing. > > IMHO, this patch series needs to spend more time internally to Red Hat > before it is presented to the community. It is too far away from > something worth reviewing at this point. > > Jason I am always trying to convince people to post RFCs early instead of working for months behind closed doors only to be told to rewrite everything in Rust. Why does it have to be internal to a specific company? I see Yi Liu from Intel is helping Cindy get it into shape and that's classic open source ethos. I know some subsystems ignore the RFC tag but I didn't realize iommu is one of these. Is that really true?
On Tue, Nov 07, 2023 at 08:28:32AM -0500, Michael S. Tsirkin wrote: > On Tue, Nov 07, 2023 at 08:49:02AM -0400, Jason Gunthorpe wrote: > > On Tue, Nov 07, 2023 at 02:30:34AM -0500, Michael S. Tsirkin wrote: > > > On Sat, Nov 04, 2023 at 01:16:33AM +0800, Cindy Lu wrote: > > > > > > > > Hi All > > > > This code provides the iommufd support for vdpa device > > > > This code fixes the bugs from the last version and also add the asid support. rebase on kernel > > > > v6,6-rc3 > > > > Test passed in the physical device (vp_vdpa), but there are still some problems in the emulated device (vdpa_sim_net), > > > > > > What kind of problems? Understanding that will make it easier > > > to figure out whether this is worth reviewing. > > > > IMHO, this patch series needs to spend more time internally to Red Hat > > before it is presented to the community. It is too far away from > > something worth reviewing at this point. > > > > Jason > > I am always trying to convince people to post RFCs early > instead of working for months behind closed doors only > to be told to rewrite everything in Rust. The community has a limited review bandwidth, things should meet a minimum standard before there is an expectation that other people should review it on the list. > Why does it have to be internal to a specific company? > I see Yi Liu from Intel is helping Cindy get it into shape > and that's classic open source ethos. Big company's should take the responsibility to train and provide skill development for their own staff. > I know some subsystems ignore the RFC tag but I didn't realize > iommu is one of these. Is that really true? At least I've looked at this twice now and been disappointed. Neither have been a well thought out RFC, this is a brain dump of unfinished work. Jason
On Tue, Nov 07, 2023 at 10:12:37AM -0400, Jason Gunthorpe wrote: > Big company's should take the responsibility to train and provide > skill development for their own staff. That would result in a beautiful cathedral of a patch. I know this is how some companies work. We are doing more of a bazaar thing here, though. In a bunch of subsystems it seems that you don't get the necessary skills until you have been publically shouted at by maintainers - better to start early ;). Not a nice environment for novices, for sure.
On Tue, Nov 07, 2023 at 08:49:02AM -0400, Jason Gunthorpe wrote: > IMHO, this patch series needs to spend more time internally to Red Hat > before it is presented to the community. Just to add an example why I think this "internal review" is a bad idea I seem to recall that someone internal to nvidia at some point attempted to implement this already. The only output from that work we have is that "it's tough" - no pointers to what's tough, no code to study even as a bad path to follow. And while Red Hat might be big, the virt team is rather smaller.
On Tue, Nov 07, 2023 at 09:55:26AM -0500, Michael S. Tsirkin wrote: > On Tue, Nov 07, 2023 at 08:49:02AM -0400, Jason Gunthorpe wrote: > > IMHO, this patch series needs to spend more time internally to Red Hat > > before it is presented to the community. > > Just to add an example why I think this "internal review" is a bad idea > I seem to recall that someone internal to nvidia at some point > attempted to implement this already. The only output from that > work we have is that "it's tough" - no pointers to what's tough, > no code to study even as a bad path to follow. > And while Red Hat might be big, the virt team is rather smaller. I don't think Nicolin got to a presentable code point. But you can start to see the issues even in this series, like simulator is complicated. mlx5 is complicated. Deciding to omit those is one path. Come with a proposal and justification to take it out, not a patch with an unexplained #ifdef. Again, I'm not talking about big impactful decisions I'm saying RH should take it internally to get a RFC proposal to the level where it is actually an RFC proposal and not a brain dump. Make sure it has logical commit messages, make sure the basic thinking about the idea is done and the proposal is self consistent and explained. Make sure the patches and series construction meet a kernel standard. The purpose of the RFC is to clearly articulate what it is you are asking to do, why you want to do it, and how you intend to get there. There is still alot of basic work to achieve this and properly communicate it. Training to do that should rightly come from the employeer, not the community. We've seen some big blow ups because some companies have been trying to externalize their training to the community. Jason
On Tue, Nov 07, 2023 at 09:30:21AM -0500, Michael S. Tsirkin wrote: > On Tue, Nov 07, 2023 at 10:12:37AM -0400, Jason Gunthorpe wrote: > > Big company's should take the responsibility to train and provide > > skill development for their own staff. > > That would result in a beautiful cathedral of a patch. I know this is > how some companies work. We are doing more of a bazaar thing here, > though. In a bunch of subsystems it seems that you don't get the > necessary skills until you have been publically shouted at by > maintainers - better to start early ;). Not a nice environment for > novices, for sure. In my view the "shouting from maintainers" is harmful to the people buidling skills and it is an unkind thing to dump employees into that kind of situation. They should have help to establish the basic level of competence where they may do the wrong thing, but all the process and presentation of the wrong thing is top notch. You get a much better reception. Jason
On Tue, Nov 07, 2023 at 11:48:48AM -0400, Jason Gunthorpe wrote: > On Tue, Nov 07, 2023 at 09:55:26AM -0500, Michael S. Tsirkin wrote: > > On Tue, Nov 07, 2023 at 08:49:02AM -0400, Jason Gunthorpe wrote: > > > IMHO, this patch series needs to spend more time internally to Red Hat > > > before it is presented to the community. > > > > Just to add an example why I think this "internal review" is a bad idea > > I seem to recall that someone internal to nvidia at some point > > attempted to implement this already. The only output from that > > work we have is that "it's tough" - no pointers to what's tough, > > no code to study even as a bad path to follow. > > And while Red Hat might be big, the virt team is rather smaller. > > I don't think Nicolin got to a presentable code point. > > But you can start to see the issues even in this series, like > simulator is complicated. mlx5 is complicated. Deciding to omit those > is one path. Come with a proposal and justification to take it out, > not a patch with an unexplained #ifdef. Right. Simulator I don't think we need to support, or at least not necessarily to get this merged - it does not really benefit from any iommufd features.
On Tue, 7 Nov 2023 08:28:32 -0500 Michael S. Tsirkin wrote: > I am always trying to convince people to post RFCs early > instead of working for months behind closed doors only > to be told to rewrite everything in Rust. > > Why does it have to be internal to a specific company? > I see Yi Liu from Intel is helping Cindy get it into shape > and that's classic open source ethos. +1, FWIW.
On Tue, Nov 07, 2023 at 11:52:17AM -0400, Jason Gunthorpe wrote: > On Tue, Nov 07, 2023 at 09:30:21AM -0500, Michael S. Tsirkin wrote: > > On Tue, Nov 07, 2023 at 10:12:37AM -0400, Jason Gunthorpe wrote: > > > Big company's should take the responsibility to train and provide > > > skill development for their own staff. > > > > That would result in a beautiful cathedral of a patch. I know this is > > how some companies work. We are doing more of a bazaar thing here, > > though. In a bunch of subsystems it seems that you don't get the > > necessary skills until you have been publically shouted at by > > maintainers - better to start early ;). Not a nice environment for > > novices, for sure. > > In my view the "shouting from maintainers" is harmful to the people > buidling skills and it is an unkind thing to dump employees into that > kind of situation. > > They should have help to establish the basic level of competence where > they may do the wrong thing, but all the process and presentation of > the wrong thing is top notch. You get a much better reception. > > Jason What - like e.g. mechanically fixing checkpatch warnings without understanding? I actually very much dislike it when people take a bad patch and just polish the presentation - it is easier for me if I can judge patch quality quickly from the presentation. Matter of taste I guess.
On Thu, Nov 09, 2023 at 06:48:46PM -0500, Michael S. Tsirkin wrote: > On Tue, Nov 07, 2023 at 11:52:17AM -0400, Jason Gunthorpe wrote: > > On Tue, Nov 07, 2023 at 09:30:21AM -0500, Michael S. Tsirkin wrote: > > > On Tue, Nov 07, 2023 at 10:12:37AM -0400, Jason Gunthorpe wrote: > > > > Big company's should take the responsibility to train and provide > > > > skill development for their own staff. > > > > > > That would result in a beautiful cathedral of a patch. I know this is > > > how some companies work. We are doing more of a bazaar thing here, > > > though. In a bunch of subsystems it seems that you don't get the > > > necessary skills until you have been publically shouted at by > > > maintainers - better to start early ;). Not a nice environment for > > > novices, for sure. > > > > In my view the "shouting from maintainers" is harmful to the people > > buidling skills and it is an unkind thing to dump employees into that > > kind of situation. > > > > They should have help to establish the basic level of competence where > > they may do the wrong thing, but all the process and presentation of > > the wrong thing is top notch. You get a much better reception. > > What - like e.g. mechanically fixing checkpatch warnings without > understanding? No, not at all. I mean actually going through and explaining what the idea is to another person and ensuing that the commit messages convey that idea, that the patches reflect the idea, that everything is convayed, and it isn't obviously internally illogical. Like, why did this series have a giant block of #ifdef 0'd code with no explanation at all? That isn't checkpatch nitpicks, that is not meeting the minimum standard to convey an idea in an RFC. Jason
On Sat, Nov 04, 2023 at 01:16:33AM +0800, Cindy Lu wrote: > > Hi All > This code provides the iommufd support for vdpa device > This code fixes the bugs from the last version and also add the asid support. rebase on kernel > v6,6-rc3 > Test passed in the physical device (vp_vdpa), but there are still some problems in the emulated device (vdpa_sim_net), > I will continue working on it > > The kernel code is > https://gitlab.com/lulu6/vhost/-/tree/iommufdRFC_v1 > > Signed-off-by: Cindy Lu <lulu@redhat.com> Was this abandoned? > > Cindy Lu (8): > vhost/iommufd: Add the functions support iommufd > Kconfig: Add the new file vhost/iommufd > vhost: Add 3 new uapi to support iommufd > vdpa: Add new vdpa_config_ops to support iommufd > vdpa_sim :Add support for iommufd > vdpa: change the map/unmap process to support iommufd > vp_vdpa::Add support for iommufd > iommu: expose the function iommu_device_use_default_domain > > drivers/iommu/iommu.c | 2 + > drivers/vdpa/vdpa_sim/vdpa_sim.c | 8 ++ > drivers/vdpa/virtio_pci/vp_vdpa.c | 4 + > drivers/vhost/Kconfig | 1 + > drivers/vhost/Makefile | 1 + > drivers/vhost/iommufd.c | 178 +++++++++++++++++++++++++ > drivers/vhost/vdpa.c | 210 +++++++++++++++++++++++++++++- > drivers/vhost/vhost.h | 21 +++ > include/linux/vdpa.h | 38 +++++- > include/uapi/linux/vhost.h | 66 ++++++++++ > 10 files changed, 525 insertions(+), 4 deletions(-) > create mode 100644 drivers/vhost/iommufd.c > > -- > 2.34.3
On Thu, Jan 11, 2024 at 6:25 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Sat, Nov 04, 2023 at 01:16:33AM +0800, Cindy Lu wrote: > > > > Hi All > > This code provides the iommufd support for vdpa device > > This code fixes the bugs from the last version and also add the asid support. rebase on kernel > > v6,6-rc3 > > Test passed in the physical device (vp_vdpa), but there are still some problems in the emulated device (vdpa_sim_net), > > I will continue working on it > > > > The kernel code is > > https://gitlab.com/lulu6/vhost/-/tree/iommufdRFC_v1 > > > > Signed-off-by: Cindy Lu <lulu@redhat.com> > > Was this abandoned? > Thanks Micheal. I'm really sorry for the delay. I will continue working on this Thanks Cindy > > > > Cindy Lu (8): > > vhost/iommufd: Add the functions support iommufd > > Kconfig: Add the new file vhost/iommufd > > vhost: Add 3 new uapi to support iommufd > > vdpa: Add new vdpa_config_ops to support iommufd > > vdpa_sim :Add support for iommufd > > vdpa: change the map/unmap process to support iommufd > > vp_vdpa::Add support for iommufd > > iommu: expose the function iommu_device_use_default_domain > > > > drivers/iommu/iommu.c | 2 + > > drivers/vdpa/vdpa_sim/vdpa_sim.c | 8 ++ > > drivers/vdpa/virtio_pci/vp_vdpa.c | 4 + > > drivers/vhost/Kconfig | 1 + > > drivers/vhost/Makefile | 1 + > > drivers/vhost/iommufd.c | 178 +++++++++++++++++++++++++ > > drivers/vhost/vdpa.c | 210 +++++++++++++++++++++++++++++- > > drivers/vhost/vhost.h | 21 +++ > > include/linux/vdpa.h | 38 +++++- > > include/uapi/linux/vhost.h | 66 ++++++++++ > > 10 files changed, 525 insertions(+), 4 deletions(-) > > create mode 100644 drivers/vhost/iommufd.c > > > > -- > > 2.34.3 >
Hi All This code provides the iommufd support for vdpa device This code fixes the bugs from the last version and also add the asid support. rebase on kernel v6,6-rc3 Test passed in the physical device (vp_vdpa), but there are still some problems in the emulated device (vdpa_sim_net), I will continue working on it The kernel code is https://gitlab.com/lulu6/vhost/-/tree/iommufdRFC_v1 Signed-off-by: Cindy Lu <lulu@redhat.com> Cindy Lu (8): vhost/iommufd: Add the functions support iommufd Kconfig: Add the new file vhost/iommufd vhost: Add 3 new uapi to support iommufd vdpa: Add new vdpa_config_ops to support iommufd vdpa_sim :Add support for iommufd vdpa: change the map/unmap process to support iommufd vp_vdpa::Add support for iommufd iommu: expose the function iommu_device_use_default_domain drivers/iommu/iommu.c | 2 + drivers/vdpa/vdpa_sim/vdpa_sim.c | 8 ++ drivers/vdpa/virtio_pci/vp_vdpa.c | 4 + drivers/vhost/Kconfig | 1 + drivers/vhost/Makefile | 1 + drivers/vhost/iommufd.c | 178 +++++++++++++++++++++++++ drivers/vhost/vdpa.c | 210 +++++++++++++++++++++++++++++- drivers/vhost/vhost.h | 21 +++ include/linux/vdpa.h | 38 +++++- include/uapi/linux/vhost.h | 66 ++++++++++ 10 files changed, 525 insertions(+), 4 deletions(-) create mode 100644 drivers/vhost/iommufd.c