Re: refactor the i915 GVT support and move to the modern mdev API v3
On 4/20/22 7:08 AM, Christoph Hellwig wrote: > On Thu, Apr 14, 2022 at 11:38:59AM -0300, Jason Gunthorpe wrote: >> pull requests can flow through more than one tree concurrently. The >> purpose of the topic branch is to allow all the commits to be in all >> the trees they need to be in at once. >> >> So you should send this branch as a PR to the next logical upstream >> tree gvt patches normally go through, in the usual way that you send >> PRs. Especially in this case where there is a small merge conflict >> internal to DRM to resolve. I'm assuming this is the drm-intel-next >> tree? >> >> Once DRM is internally happy then VFIO can merge it as well. You can >> view VFIO as the secondary path to Linus, DRM as primary. Alex will >> mention in his pull request that VFIO has a 'shared branch with DRM >> for gvt'. > > Where do we stand here? The (somewhat misnamed) topic/for-christoph > branch looks fine to me now except for the mіssing "static inline" on > the intel_gvt_iterate_mmio_table stub. > > When can we expect it in the i915 tree and linux-next? > Qur QA finished the test yesterday and I just made a tag. The pull request is going to be sent today. Yes, I will fix that. Thanks, Zhi.
Re: refactor the i915 GVT support and move to the modern mdev API v3
On Thu, Apr 14, 2022 at 02:25:36PM +, Wang, Zhi A wrote: > > So drop the '[DONT PULL]' commit and send a PR to the next DRM tree - > > when that is confirmed send the same PR to vfio, > > I updated the branch again, but I am confused. What is the purpose of sending > the PR to next DRM tree? I suppose all the patches will go through VFIO? If > I understand correctly? pull requests can flow through more than one tree concurrently. The purpose of the topic branch is to allow all the commits to be in all the trees they need to be in at once. So you should send this branch as a PR to the next logical upstream tree gvt patches normally go through, in the usual way that you send PRs. Especially in this case where there is a small merge conflict internal to DRM to resolve. I'm assuming this is the drm-intel-next tree? Once DRM is internally happy then VFIO can merge it as well. You can view VFIO as the secondary path to Linus, DRM as primary. Alex will mention in his pull request that VFIO has a 'shared branch with DRM for gvt'. Jason
Re: refactor the i915 GVT support and move to the modern mdev API v3
On 4/14/22 1:43 PM, Jason Gunthorpe wrote: > On Thu, Apr 14, 2022 at 04:40:11PM +0300, Jani Nikula wrote: > >> git clone https://github.com/intel/gvt-linux -b for-christoph > > There are alot of extra commits on there - is it possible to base this > straight on rc1 not on some kind of existing DRM tree? > > Why did you choose drm/i915/fbc: Call intel_fbc_activate() directly > from frontbuffer flush as a base? > > Jason > Hi Jason: I updated the branch. You can check if those are what you are expecting. :) >>> >>> This is better, except for the first commit: >>> >>> [DON'T PULL] drm/i915/dmc: split out dmc registers to a separate file >>> THIS PATCH WILL GO THROUGH DRM-INTEL-NEXT TO UPSTREAM >>> >>> Clean up the massive i915_reg.h a bit with this isolated set of >>> registers. >>> >>> v2: Remove stale comment (Lucas) >>> >>> Clean the commit message and send that as a proper PR to >>> drm-intel-next, then everything else is OK. >> >> It's already in drm-intel-next, I guess the problem is basing the branch >> on something that doesn't have it. I'd probably just base everything >> cleanly on -rc1, and whoever does the merge between the two will need to >> account for the missing include in the result. It's just adding one line >> in the right place. > > That makes sense to me, especially if you can do the merge fixup > internally in DRM. > > So drop the '[DONT PULL]' commit and send a PR to the next DRM tree - > when that is confirmed send the same PR to vfio, I updated the branch again, but I am confused. What is the purpose of sending the PR to next DRM tree? I suppose all the patches will go through VFIO? If I understand correctly? > > Thanks, > Jason >
Re: refactor the i915 GVT support and move to the modern mdev API v3
On Thu, 14 Apr 2022, Jason Gunthorpe wrote: > On Thu, Apr 14, 2022 at 01:39:11PM +, Wang, Zhi A wrote: >> This one belongs to i915, which has already been queued in drm-intel-next, >> but >> not yet reached to the top. When it is landed in -rc, I will rebase this >> branch >> on it, then we can drop this patch in this branch. > > A commit called 'split out dmc registers' with no Fixes: will be sent > to a rc? Won't. It's in drm-intel-next (and drm-next), headed to v5.19. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
Re: refactor the i915 GVT support and move to the modern mdev API v3
On Thu, Apr 14, 2022 at 04:40:11PM +0300, Jani Nikula wrote: > >> >> git clone https://github.com/intel/gvt-linux -b for-christoph > >> > > >> > There are alot of extra commits on there - is it possible to base this > >> > straight on rc1 not on some kind of existing DRM tree? > >> > > >> > Why did you choose drm/i915/fbc: Call intel_fbc_activate() directly > >> > from frontbuffer flush as a base? > >> > > >> > Jason > >> > > >> > >> Hi Jason: > >> > >> I updated the branch. You can check if those are what you are expecting. :) > > > > This is better, except for the first commit: > > > > [DON'T PULL] drm/i915/dmc: split out dmc registers to a separate file > > THIS PATCH WILL GO THROUGH DRM-INTEL-NEXT TO UPSTREAM > > > > Clean up the massive i915_reg.h a bit with this isolated set of > > registers. > > > > v2: Remove stale comment (Lucas) > > > > Clean the commit message and send that as a proper PR to > > drm-intel-next, then everything else is OK. > > It's already in drm-intel-next, I guess the problem is basing the branch > on something that doesn't have it. I'd probably just base everything > cleanly on -rc1, and whoever does the merge between the two will need to > account for the missing include in the result. It's just adding one line > in the right place. That makes sense to me, especially if you can do the merge fixup internally in DRM. So drop the '[DONT PULL]' commit and send a PR to the next DRM tree - when that is confirmed send the same PR to vfio, Thanks, Jason
Re: refactor the i915 GVT support and move to the modern mdev API v3
On Thu, Apr 14, 2022 at 01:39:11PM +, Wang, Zhi A wrote: > On 4/14/22 1:34 PM, Jason Gunthorpe wrote: > > On Thu, Apr 14, 2022 at 12:20:42PM +, Wang, Zhi A wrote: > >> On 4/13/22 11:20 PM, Jason Gunthorpe wrote: > >>> On Wed, Apr 13, 2022 at 11:13:06PM +, Wang, Zhi A wrote: > Hi folks: > > Thanks so much for the efforts. I prepared a branch which contains all > our patches.The aim of the branch is for the VFIO maintainers to pull > the whole bunch easily after the drm-intel-next got merged through drm > (as one of the MMIO patches depends on a patch in drm-intel-next). > > I dropped patch 4 and patch 5 as they have been covered by Jani's > patches. Some conflicts was solved. > QA is going to test it today. > > You can find it here: > > git clone https://github.com/intel/gvt-linux -b for-christoph > >>> > >>> There are alot of extra commits on there - is it possible to base this > >>> straight on rc1 not on some kind of existing DRM tree? > >>> > >>> Why did you choose drm/i915/fbc: Call intel_fbc_activate() directly > >>> from frontbuffer flush as a base? > >>> > >>> Jason > >>> > >> > >> Hi Jason: > >> > This one belongs to i915, which has already been queued in drm-intel-next, but > not yet reached to the top. When it is landed in -rc, I will rebase this > branch > on it, then we can drop this patch in this branch. A commit called 'split out dmc registers' with no Fixes: will be sent to a rc? Jason
Re: refactor the i915 GVT support and move to the modern mdev API v3
On Thu, 14 Apr 2022, Jason Gunthorpe wrote: > On Thu, Apr 14, 2022 at 12:20:42PM +, Wang, Zhi A wrote: >> On 4/13/22 11:20 PM, Jason Gunthorpe wrote: >> > On Wed, Apr 13, 2022 at 11:13:06PM +, Wang, Zhi A wrote: >> >> Hi folks: >> >> >> >> Thanks so much for the efforts. I prepared a branch which contains all >> >> our patches.The aim of the branch is for the VFIO maintainers to pull the >> >> whole bunch easily after the drm-intel-next got merged through drm (as >> >> one of the MMIO patches depends on a patch in drm-intel-next). >> >> >> >> I dropped patch 4 and patch 5 as they have been covered by Jani's >> >> patches. Some conflicts was solved. >> >> QA is going to test it today. >> >> >> >> You can find it here: >> >> >> >> git clone https://github.com/intel/gvt-linux -b for-christoph >> > >> > There are alot of extra commits on there - is it possible to base this >> > straight on rc1 not on some kind of existing DRM tree? >> > >> > Why did you choose drm/i915/fbc: Call intel_fbc_activate() directly >> > from frontbuffer flush as a base? >> > >> > Jason >> > >> >> Hi Jason: >> >> I updated the branch. You can check if those are what you are expecting. :) > > This is better, except for the first commit: > > [DON'T PULL] drm/i915/dmc: split out dmc registers to a separate file > THIS PATCH WILL GO THROUGH DRM-INTEL-NEXT TO UPSTREAM > > Clean up the massive i915_reg.h a bit with this isolated set of > registers. > > v2: Remove stale comment (Lucas) > > Clean the commit message and send that as a proper PR to > drm-intel-next, then everything else is OK. It's already in drm-intel-next, I guess the problem is basing the branch on something that doesn't have it. I'd probably just base everything cleanly on -rc1, and whoever does the merge between the two will need to account for the missing include in the result. It's just adding one line in the right place. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
Re: refactor the i915 GVT support and move to the modern mdev API v3
On 4/14/22 1:34 PM, Jason Gunthorpe wrote: > On Thu, Apr 14, 2022 at 12:20:42PM +, Wang, Zhi A wrote: >> On 4/13/22 11:20 PM, Jason Gunthorpe wrote: >>> On Wed, Apr 13, 2022 at 11:13:06PM +, Wang, Zhi A wrote: Hi folks: Thanks so much for the efforts. I prepared a branch which contains all our patches.The aim of the branch is for the VFIO maintainers to pull the whole bunch easily after the drm-intel-next got merged through drm (as one of the MMIO patches depends on a patch in drm-intel-next). I dropped patch 4 and patch 5 as they have been covered by Jani's patches. Some conflicts was solved. QA is going to test it today. You can find it here: git clone https://github.com/intel/gvt-linux -b for-christoph >>> >>> There are alot of extra commits on there - is it possible to base this >>> straight on rc1 not on some kind of existing DRM tree? >>> >>> Why did you choose drm/i915/fbc: Call intel_fbc_activate() directly >>> from frontbuffer flush as a base? >>> >>> Jason >>> >> >> Hi Jason: >> This one belongs to i915, which has already been queued in drm-intel-next, but not yet reached to the top. When it is landed in -rc, I will rebase this branch on it, then we can drop this patch in this branch. >> I updated the branch. You can check if those are what you are expecting. :) > > This is better, except for the first commit: > > [DON'T PULL] drm/i915/dmc: split out dmc registers to a separate file > THIS PATCH WILL GO THROUGH DRM-INTEL-NEXT TO UPSTREAM > > Clean up the massive i915_reg.h a bit with this isolated set of > registers. > > v2: Remove stale comment (Lucas) > > Clean the commit message and send that as a proper PR to > drm-intel-next, then everything else is OK. > > Jason >
Re: refactor the i915 GVT support and move to the modern mdev API v3
On Thu, Apr 14, 2022 at 12:20:42PM +, Wang, Zhi A wrote: > On 4/13/22 11:20 PM, Jason Gunthorpe wrote: > > On Wed, Apr 13, 2022 at 11:13:06PM +, Wang, Zhi A wrote: > >> Hi folks: > >> > >> Thanks so much for the efforts. I prepared a branch which contains all our > >> patches.The aim of the branch is for the VFIO maintainers to pull the > >> whole bunch easily after the drm-intel-next got merged through drm (as one > >> of the MMIO patches depends on a patch in drm-intel-next). > >> > >> I dropped patch 4 and patch 5 as they have been covered by Jani's patches. > >> Some conflicts was solved. > >> QA is going to test it today. > >> > >> You can find it here: > >> > >> git clone https://github.com/intel/gvt-linux -b for-christoph > > > > There are alot of extra commits on there - is it possible to base this > > straight on rc1 not on some kind of existing DRM tree? > > > > Why did you choose drm/i915/fbc: Call intel_fbc_activate() directly > > from frontbuffer flush as a base? > > > > Jason > > > > Hi Jason: > > I updated the branch. You can check if those are what you are expecting. :) This is better, except for the first commit: [DON'T PULL] drm/i915/dmc: split out dmc registers to a separate file THIS PATCH WILL GO THROUGH DRM-INTEL-NEXT TO UPSTREAM Clean up the massive i915_reg.h a bit with this isolated set of registers. v2: Remove stale comment (Lucas) Clean the commit message and send that as a proper PR to drm-intel-next, then everything else is OK. Jason
Re: refactor the i915 GVT support and move to the modern mdev API v3
On 4/13/22 11:20 PM, Jason Gunthorpe wrote: > On Wed, Apr 13, 2022 at 11:13:06PM +, Wang, Zhi A wrote: >> Hi folks: >> >> Thanks so much for the efforts. I prepared a branch which contains all our >> patches.The aim of the branch is for the VFIO maintainers to pull the whole >> bunch easily after the drm-intel-next got merged through drm (as one of the >> MMIO patches depends on a patch in drm-intel-next). >> >> I dropped patch 4 and patch 5 as they have been covered by Jani's patches. >> Some conflicts was solved. >> QA is going to test it today. >> >> You can find it here: >> >> git clone https://github.com/intel/gvt-linux -b for-christoph > > There are alot of extra commits on there - is it possible to base this > straight on rc1 not on some kind of existing DRM tree? > > Why did you choose drm/i915/fbc: Call intel_fbc_activate() directly > from frontbuffer flush as a base? > > Jason > Hi Jason: I updated the branch. You can check if those are what you are expecting. :) Thanks, Zhi.
Re: refactor the i915 GVT support and move to the modern mdev API v3
On Wed, Apr 13, 2022 at 11:13:06PM +, Wang, Zhi A wrote: > Hi folks: > > Thanks so much for the efforts. I prepared a branch which contains all our > patches.The aim of the branch is for the VFIO maintainers to pull the whole > bunch easily after the drm-intel-next got merged through drm (as one of the > MMIO patches depends on a patch in drm-intel-next). > > I dropped patch 4 and patch 5 as they have been covered by Jani's patches. > Some conflicts was solved. > QA is going to test it today. > > You can find it here: > > git clone https://github.com/intel/gvt-linux -b for-christoph There are alot of extra commits on there - is it possible to base this straight on rc1 not on some kind of existing DRM tree? Why did you choose drm/i915/fbc: Call intel_fbc_activate() directly from frontbuffer flush as a base? Jason
Re: refactor the i915 GVT support and move to the modern mdev API v3
Hi folks: Thanks so much for the efforts. I prepared a branch which contains all our patches.The aim of the branch is for the VFIO maintainers to pull the whole bunch easily after the drm-intel-next got merged through drm (as one of the MMIO patches depends on a patch in drm-intel-next). I dropped patch 4 and patch 5 as they have been covered by Jani's patches. Some conflicts was solved. QA is going to test it today. You can find it here: git clone https://github.com/intel/gvt-linux -b for-christoph Thanks, Zhi. On 4/13/22 3:58 PM, Jani Nikula wrote: > On Wed, 13 Apr 2022, Christoph Hellwig wrote: >> On Wed, Apr 13, 2022 at 01:47:05PM +, Wang, Zhi A wrote: the GVT code in the i915 is a bit of a mess right now due to strange abstractions and lots of indirect calls. This series refactors various bits to clean that up. The main user visible change is that almost all of the GVT code moves out of the main i915 driver and into the kvmgt module. >>> >>> Hi Christoph: >>> >>> Do you want me to merge the GVT-g patches in this series? Or you want them >>> to get merged from your side? >> >> The two option here are drm tree via gvt and i915 trees or the vfio >> tree, neither of which really is my tree. >> >> We already have a fair bit of vfio changes at the tail end of the series, >> and Jason has some more that should sit on top of it, and I have some >> more that I haven't sent yet. >> >> So if we could get the MMIO table and Makefile cleanups into a topic >> branch that we could pull into the vfio tree and merge it through that >> that would seem easiest to me, assuming that is ok with the i915, drm >> and vfio maintainers. > > AFAICS the changes are mostly to gvt/, and at least I'm fine with the > minor changes to i915 (in this series and in my two patches) being > merged via whichever tree you all see fit. > > Acked-by: Jani Nikula > > Joonas, Tvrtko, Rodrigo, chime in now if you have any issues with that. > > > BR, > Jani. > >
Re: refactor the i915 GVT support and move to the modern mdev API v3
On Wed, 13 Apr 2022, Christoph Hellwig wrote: > On Wed, Apr 13, 2022 at 01:47:05PM +, Wang, Zhi A wrote: >> > the GVT code in the i915 is a bit of a mess right now due to strange >> > abstractions and lots of indirect calls. This series refactors various >> > bits to clean that up. The main user visible change is that almost all >> > of the GVT code moves out of the main i915 driver and into the kvmgt >> > module. >> >> Hi Christoph: >> >> Do you want me to merge the GVT-g patches in this series? Or you want them >> to get merged from your side? > > The two option here are drm tree via gvt and i915 trees or the vfio > tree, neither of which really is my tree. > > We already have a fair bit of vfio changes at the tail end of the series, > and Jason has some more that should sit on top of it, and I have some > more that I haven't sent yet. > > So if we could get the MMIO table and Makefile cleanups into a topic > branch that we could pull into the vfio tree and merge it through that > that would seem easiest to me, assuming that is ok with the i915, drm > and vfio maintainers. AFAICS the changes are mostly to gvt/, and at least I'm fine with the minor changes to i915 (in this series and in my two patches) being merged via whichever tree you all see fit. Acked-by: Jani Nikula Joonas, Tvrtko, Rodrigo, chime in now if you have any issues with that. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
Re: refactor the i915 GVT support and move to the modern mdev API v3
On 4/11/22 2:13 PM, Christoph Hellwig wrote: > Hi all, > > the GVT code in the i915 is a bit of a mess right now due to strange > abstractions and lots of indirect calls. This series refactors various > bits to clean that up. The main user visible change is that almost all > of the GVT code moves out of the main i915 driver and into the kvmgt > module. Hi Christoph: Do you want me to merge the GVT-g patches in this series? Or you want them to get merged from your side? Thanks, Zhi. > > Tested on my Thinkpad with a Kaby Lake CPU and integrated graphics. > > Git tree: > > git://git.infradead.org/users/hch/misc.git i915-gvt > > Gitweb: > > http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/i915-gvt > > Changes since v2: > - rebased on top of Linx 5.18-rc + >"Refactor GVT-g MMIO tracking table and handlers" > - don't fold the gvt Makefile into the main Makefile > - add the mdev patches to remove the legacy interface that is now >unused to the end of the series > > Changes since v1: > - rebased on Linux 5.15 > - allow the kvmgvt module to be loaded at any time and thus solve >the deadlock when both i915 amd kvmgvt are modular > - include the conversion to the modern mdev API > > Note that I do expect to rebased this again against 5.16-rc1 once > released, but I'd like to get this out for review ASAP. > > Diffstat: > b/drivers/gpu/drm/i915/Kconfig | 33 > b/drivers/gpu/drm/i915/Makefile | 31 > b/drivers/gpu/drm/i915/gvt/cfg_space.c | 89 -- > b/drivers/gpu/drm/i915/gvt/cmd_parser.c |4 > b/drivers/gpu/drm/i915/gvt/dmabuf.c | 36 - > b/drivers/gpu/drm/i915/gvt/execlist.c | 12 > b/drivers/gpu/drm/i915/gvt/gtt.c| 55 + > b/drivers/gpu/drm/i915/gvt/gvt.h| 125 ++- > b/drivers/gpu/drm/i915/gvt/interrupt.c | 38 + > b/drivers/gpu/drm/i915/gvt/kvmgt.c | 1099 > +++- > b/drivers/gpu/drm/i915/gvt/mmio.c |4 > b/drivers/gpu/drm/i915/gvt/opregion.c | 148 > b/drivers/gpu/drm/i915/gvt/page_track.c |8 > b/drivers/gpu/drm/i915/gvt/scheduler.c | 37 - > b/drivers/gpu/drm/i915/gvt/trace.h |2 > b/drivers/gpu/drm/i915/gvt/vgpu.c | 22 > b/drivers/gpu/drm/i915/i915_drv.c |7 > b/drivers/gpu/drm/i915/i915_drv.h |1 > b/drivers/gpu/drm/i915/i915_trace.h |1 > b/drivers/gpu/drm/i915/intel_gvt.c | 162 +++- > b/drivers/gpu/drm/i915/intel_gvt.h | 17 > drivers/gpu/drm/i915/gvt/Makefile |9 > drivers/gpu/drm/i915/gvt/gvt.c | 340 - > drivers/gpu/drm/i915/gvt/hypercall.h| 82 -- > drivers/gpu/drm/i915/gvt/mpt.h | 400 --- > 25 files changed, 929 insertions(+), 1833 deletions(-) >
Re: refactor the i915 GVT support and move to the modern mdev API v2
Quoting Christoph Hellwig (2021-11-09 09:59:57) > On Thu, Nov 04, 2021 at 02:59:18PM +0200, Joonas Lahtinen wrote: > > The minimal we should do is to eliminate the double underscore > > prefixed functions. But I would prefer to have the symbol exports by > > default so that we can enable the functionality just by loading the > > module. > > I'm fine with exporting by default, but just loading won't really work > even with that: > > - there are a bunch of IS_ENABLED conditionals in the i915 (although >they look like minor optimizations to me). I'd assume the golden state capture being the one with biggest impact. > - the enable_gvt needs to be set, although after this refactor this >option is completely pointless and should probably be enabled Indeed. Hope is that modprobe/rmmod would be enough to enable/disable. This should help any distros intending to enable the feature, too. So mostly about making sure the IS_ENABLED portions in base i915 operation are not too invasive. > - the enable_guc option needs to be disable for gvt to work. On the GVT supported platforms GuC is disabled by default, so it should be fine. We can change the logic to opposite to disable the feature if the enable_guc unsafe modparam is used. Regards, Joonas
RE: refactor the i915 GVT support and move to the modern mdev API v2
Hi Joonas and Christoph: We were testing the patch series since Monday and planning to reply after we get the test result. Mostly, we are concerned about patch 6 and how it would affect the test result. Patch 6 changes the timing of loading GVT-g. According to the discussion in the last email, this will break our design of golden MMIO snapshot. Also moving GVT-g code into kvmgt.ko requires the discussion of defining and shrinking the interfaces between i915 and kvmgt. I guess the ideal way to take Christoph's patch is: 1) We have to figure out how to deal with golden MMIO snapshot. It's a little bit hard to take the re-factor patch before settling this down. In the previous discussion, we would like to find a way to do the snapshot in intel_gvt.c 2) Shrink and refine the exported interfaces because of moving the code into kvmgt.ko 3) Get patches reviewed and merged. For 1) I was thinking to separated the MMIO handler table from handlers.c and let it build different data structures depends on where it got referenced. 2) That's might require some more discussion. Is it possible to separate the refactor part from the using new mdev API stuff? So that the design opens in the re-factor patches wouldn’t block the process of mdev API improvement? Thanks, Zhi. -Original Message- From: Joonas Lahtinen Sent: Thursday, November 4, 2021 2:59 PM To: Christoph Hellwig ; Jani Nikula ; Vivi, Rodrigo ; Zhenyu Wang ; Wang, Zhi A Cc: Jason Gunthorpe ; intel-...@lists.freedesktop.org; intel-gvt-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-ker...@vger.kernel.org Subject: Re: refactor the i915 GVT support and move to the modern mdev API v2 Hi Zhenyu and Zhi, Can you have somebody from the GVT team to review the patches that are fully contained in gvt/ ? I also started discussion on patch 6 which is about defining the interface between the modules. I remember there is prior work to shrink the interface. Do you have links to such patches? The minimal we should do is to eliminate the double underscore prefixed functions. But I would prefer to have the symbol exports by default so that we can enable the functionality just by loading the module. Regards, Joonas Quoting Christoph Hellwig (2021-11-02 09:05:32) > Hi all, > > the GVT code in the i915 is a bit of a mess right now due to strange > abstractions and lots of indirect calls. This series refactors > various bits to clean that up. The main user visible change is that > almost all of the GVT code moves out of the main i915 driver and into > the kvmgt module. > > Tested on my Thinkpad with a Kaby Lake CPU and integrated graphics. > > Git tree: > > git://git.infradead.org/users/hch/misc.git i915-gvt > > Gitweb: > > > http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/i915-g > vt > > Changes since v1: > - rebased on Linux 5.15 > - allow the kvmgvt module to be loaded at any time and thus solve >the deadlock when both i915 amd kvmgvt are modular > - include the conversion to the modern mdev API > > Note that I do expect to rebased this again against 5.16-rc1 once > released, but I'd like to get this out for review ASAP. > > Diffstat: > b/drivers/gpu/drm/i915/Kconfig | 33 > b/drivers/gpu/drm/i915/Makefile | 31 > b/drivers/gpu/drm/i915/gvt/cfg_space.c | 89 -- > b/drivers/gpu/drm/i915/gvt/cmd_parser.c |4 > b/drivers/gpu/drm/i915/gvt/dmabuf.c | 36 - > b/drivers/gpu/drm/i915/gvt/execlist.c | 12 > b/drivers/gpu/drm/i915/gvt/gtt.c| 55 + > b/drivers/gpu/drm/i915/gvt/gvt.h| 125 ++- > b/drivers/gpu/drm/i915/gvt/interrupt.c | 38 + > b/drivers/gpu/drm/i915/gvt/kvmgt.c | 1099 > +++- > b/drivers/gpu/drm/i915/gvt/mmio.c |4 > b/drivers/gpu/drm/i915/gvt/opregion.c | 148 > b/drivers/gpu/drm/i915/gvt/page_track.c |8 > b/drivers/gpu/drm/i915/gvt/scheduler.c | 37 - > b/drivers/gpu/drm/i915/gvt/trace.h |2 > b/drivers/gpu/drm/i915/gvt/vgpu.c | 22 > b/drivers/gpu/drm/i915/i915_drv.c |7 > b/drivers/gpu/drm/i915/i915_drv.h |1 > b/drivers/gpu/drm/i915/i915_trace.h |1 > b/drivers/gpu/drm/i915/intel_gvt.c | 162 +++- > b/drivers/gpu/drm/i915/intel_gvt.h | 17 > drivers/gpu/drm/i915/gvt/Makefile |9 > drivers/gpu/drm/i915/gvt/gvt.c | 340 - > drivers/gpu/drm/i915/gvt/hypercall.h| 82 -- > drivers/gpu/drm/i915/gvt/mpt.h | 400 --- > 25 files changed, 929 insertions(+), 1833 deletions(-)
Re: refactor the i915 GVT support and move to the modern mdev API v2
Hi Zhenyu and Zhi, Can you have somebody from the GVT team to review the patches that are fully contained in gvt/ ? I also started discussion on patch 6 which is about defining the interface between the modules. I remember there is prior work to shrink the interface. Do you have links to such patches? The minimal we should do is to eliminate the double underscore prefixed functions. But I would prefer to have the symbol exports by default so that we can enable the functionality just by loading the module. Regards, Joonas Quoting Christoph Hellwig (2021-11-02 09:05:32) > Hi all, > > the GVT code in the i915 is a bit of a mess right now due to strange > abstractions and lots of indirect calls. This series refactors various > bits to clean that up. The main user visible change is that almost all > of the GVT code moves out of the main i915 driver and into the kvmgt > module. > > Tested on my Thinkpad with a Kaby Lake CPU and integrated graphics. > > Git tree: > > git://git.infradead.org/users/hch/misc.git i915-gvt > > Gitweb: > > http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/i915-gvt > > Changes since v1: > - rebased on Linux 5.15 > - allow the kvmgvt module to be loaded at any time and thus solve >the deadlock when both i915 amd kvmgvt are modular > - include the conversion to the modern mdev API > > Note that I do expect to rebased this again against 5.16-rc1 once > released, but I'd like to get this out for review ASAP. > > Diffstat: > b/drivers/gpu/drm/i915/Kconfig | 33 > b/drivers/gpu/drm/i915/Makefile | 31 > b/drivers/gpu/drm/i915/gvt/cfg_space.c | 89 -- > b/drivers/gpu/drm/i915/gvt/cmd_parser.c |4 > b/drivers/gpu/drm/i915/gvt/dmabuf.c | 36 - > b/drivers/gpu/drm/i915/gvt/execlist.c | 12 > b/drivers/gpu/drm/i915/gvt/gtt.c| 55 + > b/drivers/gpu/drm/i915/gvt/gvt.h| 125 ++- > b/drivers/gpu/drm/i915/gvt/interrupt.c | 38 + > b/drivers/gpu/drm/i915/gvt/kvmgt.c | 1099 > +++- > b/drivers/gpu/drm/i915/gvt/mmio.c |4 > b/drivers/gpu/drm/i915/gvt/opregion.c | 148 > b/drivers/gpu/drm/i915/gvt/page_track.c |8 > b/drivers/gpu/drm/i915/gvt/scheduler.c | 37 - > b/drivers/gpu/drm/i915/gvt/trace.h |2 > b/drivers/gpu/drm/i915/gvt/vgpu.c | 22 > b/drivers/gpu/drm/i915/i915_drv.c |7 > b/drivers/gpu/drm/i915/i915_drv.h |1 > b/drivers/gpu/drm/i915/i915_trace.h |1 > b/drivers/gpu/drm/i915/intel_gvt.c | 162 +++- > b/drivers/gpu/drm/i915/intel_gvt.h | 17 > drivers/gpu/drm/i915/gvt/Makefile |9 > drivers/gpu/drm/i915/gvt/gvt.c | 340 - > drivers/gpu/drm/i915/gvt/hypercall.h| 82 -- > drivers/gpu/drm/i915/gvt/mpt.h | 400 --- > 25 files changed, 929 insertions(+), 1833 deletions(-)
Re: refactor the i915 GVT support
Hi folks:
It seems we haven't reached a possible solution of this refactor patch
series. The current patch series needs to be re-worked because of the
module/symbol dependency(The root cause has been discussed in another
email). I have to get them off from our gvt-next repo so that we can
continue our development and pull-request to upstream. Thanks so much
for the patch and the discussion.
Thanks,
Zhi.
On 10/1/21 1:01 PM, Wang, Zhi A wrote:
> On 9/29/21 6:55 PM, Jason Gunthorpe wrote:
>> On Wed, Sep 29, 2021 at 06:27:16PM +, Wang, Zhi A wrote:
>>> On 9/28/21 3:05 PM, Jason Gunthorpe wrote:
On Tue, Sep 28, 2021 at 02:35:06PM +, Wang, Zhi A wrote:
> Yes. I was thinking of the possibility of putting off some work
> later so
> that we don't need to make a lot of changes. GVT-g needs to take a
> snapshot of GPU registers as the initial virtual states for other
> vGPUs,
> which requires the initialization happens at a certain early time of
> initialization of i915. I was thinking maybe we can take other
> patches
> from Christoph like "de-virtualize*" except this one because
> currently
> we have to maintain a TEST-ONLY patch on our tree to prevent i915
> built
> as kernel module.
How about just capture these registers in the main module/device and
not try so hard to isolate it to the gvt stuff?
>>> Hi Jason:
>>>
>>> Thanks for the idea. I am not sure i915 guys would take this idea since
>>> that it's only for GVT-g, i915 doesn't use this at all. We need to take
>>> a snapshot of both PCI configuration space and MMIO registers before
>>> i915 driver starts to touch the HW.
>> Given the code is already linked into i915 I don't see there is much
>> to object to here. It can remain conditional on the kernel parameter
>> as today.
>>
>> As a general philosophy this would all be much less strange if the
>> mdev .ko is truely optional. It should be cleanly seperate from its
>> base device and never request_module'd..
>>
>> In this case auxiliary device might be a good option, have i915 create
>> one and the mdev module be loaded against it.
>>
>> In the mean time is there some shortcut to get this series to move
>> ahead? Is patch 4 essential to the rest of the series?
>>
>> A really awful hack would be to push the pci_driver_register into a
>> WQ so that the request_module is guarenteed to not be part of the
>> module_init callchain.
>
> Hi Jason and folks:
>
> Thanks so much for the ideas. That sounds great and I was keeping
> thinking how to make progress on this. How about we do like this: We
> don't do request_module("kvmgt") in i915.ko, which resolves the
> circular module dependency. We keep the code of doing snapshot of
> registers in intel_gvt.c. When i915.enable_gvt=1, we do the snapshot.
> Then we export functions for kvmgt.ko in intel_gvt.c to check if gvt
> in i915 is enabled or not and get the snapshots.
>
> How does that sounds? I just need to write another patch and put it on
> top of Christoph's series.
>
> Thanks,
>
> Zhi.
>
>>> Also I was thinking if moving gvt into kvmgt.ko is the right direction.
>>> It seems the module loading system in kernel is not designed for
>>> "module
>>> A loading module B, which needs symbols from module A, in the
>>> initialization path of module A".
>> Of course not, that is a circular module dependency, it should not be
>> that way. The SW layers need to be clean and orderly - meaning the
>> i915 module needs to have the minimal amount of code to support the
>> mdev module.
>>
>> Jason
>
>
Re: refactor the i915 GVT support
On 9/29/21 6:55 PM, Jason Gunthorpe wrote:
> On Wed, Sep 29, 2021 at 06:27:16PM +, Wang, Zhi A wrote:
>> On 9/28/21 3:05 PM, Jason Gunthorpe wrote:
>>> On Tue, Sep 28, 2021 at 02:35:06PM +, Wang, Zhi A wrote:
>>>
Yes. I was thinking of the possibility of putting off some work later so
that we don't need to make a lot of changes. GVT-g needs to take a
snapshot of GPU registers as the initial virtual states for other vGPUs,
which requires the initialization happens at a certain early time of
initialization of i915. I was thinking maybe we can take other patches
from Christoph like "de-virtualize*" except this one because currently
we have to maintain a TEST-ONLY patch on our tree to prevent i915 built
as kernel module.
>>> How about just capture these registers in the main module/device and
>>> not try so hard to isolate it to the gvt stuff?
>> Hi Jason:
>>
>> Thanks for the idea. I am not sure i915 guys would take this idea since
>> that it's only for GVT-g, i915 doesn't use this at all. We need to take
>> a snapshot of both PCI configuration space and MMIO registers before
>> i915 driver starts to touch the HW.
> Given the code is already linked into i915 I don't see there is much
> to object to here. It can remain conditional on the kernel parameter
> as today.
>
> As a general philosophy this would all be much less strange if the
> mdev .ko is truely optional. It should be cleanly seperate from its
> base device and never request_module'd..
>
> In this case auxiliary device might be a good option, have i915 create
> one and the mdev module be loaded against it.
>
> In the mean time is there some shortcut to get this series to move
> ahead? Is patch 4 essential to the rest of the series?
>
> A really awful hack would be to push the pci_driver_register into a
> WQ so that the request_module is guarenteed to not be part of the
> module_init callchain.
Hi Jason and folks:
Thanks so much for the ideas. That sounds great and I was keeping
thinking how to make progress on this. How about we do like this: We
don't do request_module("kvmgt") in i915.ko, which resolves the circular
module dependency. We keep the code of doing snapshot of registers in
intel_gvt.c. When i915.enable_gvt=1, we do the snapshot. Then we export
functions for kvmgt.ko in intel_gvt.c to check if gvt in i915 is enabled
or not and get the snapshots.
How does that sounds? I just need to write another patch and put it on
top of Christoph's series.
Thanks,
Zhi.
>> Also I was thinking if moving gvt into kvmgt.ko is the right direction.
>> It seems the module loading system in kernel is not designed for "module
>> A loading module B, which needs symbols from module A, in the
>> initialization path of module A".
> Of course not, that is a circular module dependency, it should not be
> that way. The SW layers need to be clean and orderly - meaning the
> i915 module needs to have the minimal amount of code to support the
> mdev module.
>
> Jason
Re: refactor the i915 GVT support
On Wed, Sep 29, 2021 at 06:27:16PM +, Wang, Zhi A wrote: > On 9/28/21 3:05 PM, Jason Gunthorpe wrote: > > On Tue, Sep 28, 2021 at 02:35:06PM +, Wang, Zhi A wrote: > > > >> Yes. I was thinking of the possibility of putting off some work later so > >> that we don't need to make a lot of changes. GVT-g needs to take a > >> snapshot of GPU registers as the initial virtual states for other vGPUs, > >> which requires the initialization happens at a certain early time of > >> initialization of i915. I was thinking maybe we can take other patches > >> from Christoph like "de-virtualize*" except this one because currently > >> we have to maintain a TEST-ONLY patch on our tree to prevent i915 built > >> as kernel module. > > How about just capture these registers in the main module/device and > > not try so hard to isolate it to the gvt stuff? > > Hi Jason: > > Thanks for the idea. I am not sure i915 guys would take this idea since > that it's only for GVT-g, i915 doesn't use this at all. We need to take > a snapshot of both PCI configuration space and MMIO registers before > i915 driver starts to touch the HW. Given the code is already linked into i915 I don't see there is much to object to here. It can remain conditional on the kernel parameter as today. As a general philosophy this would all be much less strange if the mdev .ko is truely optional. It should be cleanly seperate from its base device and never request_module'd.. In this case auxiliary device might be a good option, have i915 create one and the mdev module be loaded against it. In the mean time is there some shortcut to get this series to move ahead? Is patch 4 essential to the rest of the series? A really awful hack would be to push the pci_driver_register into a WQ so that the request_module is guarenteed to not be part of the module_init callchain. > Also I was thinking if moving gvt into kvmgt.ko is the right direction. > It seems the module loading system in kernel is not designed for "module > A loading module B, which needs symbols from module A, in the > initialization path of module A". Of course not, that is a circular module dependency, it should not be that way. The SW layers need to be clean and orderly - meaning the i915 module needs to have the minimal amount of code to support the mdev module. Jason
Re: refactor the i915 GVT support
On 9/28/21 3:05 PM, Jason Gunthorpe wrote:
> On Tue, Sep 28, 2021 at 02:35:06PM +, Wang, Zhi A wrote:
>
>> Yes. I was thinking of the possibility of putting off some work later so
>> that we don't need to make a lot of changes. GVT-g needs to take a
>> snapshot of GPU registers as the initial virtual states for other vGPUs,
>> which requires the initialization happens at a certain early time of
>> initialization of i915. I was thinking maybe we can take other patches
>> from Christoph like "de-virtualize*" except this one because currently
>> we have to maintain a TEST-ONLY patch on our tree to prevent i915 built
>> as kernel module.
> How about just capture these registers in the main module/device and
> not try so hard to isolate it to the gvt stuff?
Hi Jason:
Thanks for the idea. I am not sure i915 guys would take this idea since
that it's only for GVT-g, i915 doesn't use this at all. We need to take
a snapshot of both PCI configuration space and MMIO registers before
i915 driver starts to touch the HW.
One idea is we capture the registers in intel_gvt.c during the early
initialization and do everything else when i915.ko is fully loaded. But
how about dependence between i915.ko and kvmgt.ko? We cannot do
request_module("kvmgt") in i915.ko.
Maybe Christoph can give more input on this and how we can deal with
this? Before we figure out an solution, we have to pick that patch out
since it blocks our pull request schedule.
Also I was thinking if moving gvt into kvmgt.ko is the right direction.
It seems the module loading system in kernel is not designed for "module
A loading module B, which needs symbols from module A, in the
initialization path of module A".
Thanks,
Zhi.
> Jason
Re: refactor the i915 GVT support
On Tue, Sep 28, 2021 at 02:35:06PM +, Wang, Zhi A wrote: > Yes. I was thinking of the possibility of putting off some work later so > that we don't need to make a lot of changes. GVT-g needs to take a > snapshot of GPU registers as the initial virtual states for other vGPUs, > which requires the initialization happens at a certain early time of > initialization of i915. I was thinking maybe we can take other patches > from Christoph like "de-virtualize*" except this one because currently > we have to maintain a TEST-ONLY patch on our tree to prevent i915 built > as kernel module. How about just capture these registers in the main module/device and not try so hard to isolate it to the gvt stuff? Jason
Re: refactor the i915 GVT support
On 9/28/21 2:00 PM, Luis Chamberlain wrote:
> On Tue, Sep 28, 2021 at 07:41:00AM +, Wang, Zhi A wrote:
>> Hey guys:
>>
>> After some investigation, I found the root cause this problem ("i915"
>> module loading will be stuck with Christoph's refactor patches), which
>> can be reproduced by building both i915 and kvmgt as kernel module and
>> the loading i915.
> Thanks for looking into this!
>
>> The root cause is: in Linux kernel loading, before a kernel module
>> loading is finished, its symbols can not be reached by other module when
>> resolving the symbols (even they can be found in /proc/kallsyms).
>> Because the status of the kernel module is MODULE_STATE_COMING and
>> resolve_symbol() from another kernel module will check this and return a
>> -EBUSY.
> Well, it would seem that way but...
>
>> In this case, before i915 loading is finished, the requested module
>> "kvmgt" cannot reach the symbols in module i915. Thus it kept waiting
>> and left message like below in the dmesg:
>>
>> [ 644.152021] kvmgt: gave up waiting for init of module i915.
>> [ 644.152039] kvmgt: Unknown symbol i915_gem_object_set_to_cpu_domain
>> (err -16)
>> [ 674.871409] kvmgt: gave up waiting for init of module i915.
>> [ 674.871427] kvmgt: Unknown symbol intel_ring_begin (err -16)
>> [ 705.590586] kvmgt: gave up waiting for init of module i915.
>> [ 705.590604] kvmgt: Unknown symbol i915_vma_move_to_active (err -16)
>> [ 736.310230] kvmgt: gave up waiting for init of module i915.
>> [ 736.310248] kvmgt: Unknown symbol shmem_unpin_map (err -16)
>> ...
>>
>> The error message is from execution path below:
>>
>> kernel/module.c:
>>
>> [i915 module loading] ->
>> request_module("kvmgt")->[modprobe]->init_module("kvmgt")->load_module()->simplify_symbols()->resolve_symbol_wait():
>>
>> static const struct kernel_symbol *
>> resolve_symbol_wait(struct module *mod,
>> const struct load_info *info,
>> const char *name)
>> {
>> const struct kernel_symbol *ksym;
>> char owner[MODULE_NAME_LEN];
>>
>> if (wait_event_interruptible_timeout(module_wq,
>> !IS_ERR(ksym = resolve_symbol(mod, info, name, owner))
>> || PTR_ERR(ksym) != -EBUSY,
>> 30 * HZ) <= 0) {
>> pr_warn("%s: gave up waiting for init of module %s.\n",
>> mod->name, owner);
>>
>> }
> Commit 9bea7f23952d5 ("module: fix bne2 "gave up waiting for init of
> module libcrc32c") is worth reviewing. It dealt with a similar issue,
> and in particular it addressed the issue with -EBUSY being returned
> by ref_module().
>
> And so, in theory that case should be dealt with in resolve_symbol_wait()
> already. And so can you try this just to verify something:
>
> diff --git a/kernel/module.c b/kernel/module.c
> index 40ec9a030eec..98f87cbb37de 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1459,7 +1459,7 @@ resolve_symbol_wait(struct module *mod,
> if (wait_event_interruptible_timeout(module_wq,
> !IS_ERR(ksym = resolve_symbol(mod, info, name, owner))
> || PTR_ERR(ksym) != -EBUSY,
> - 30 * HZ) <= 0) {
> + 160 * HZ) <= 0) {
> pr_warn("%s: gave up waiting for init of module %s.\n",
> mod->name, owner);
> }
>
Hi Luis:
Thanks so much for the reply and patch.:)
I am afraid that this patch wouldn't work in this case as the
request_module("kvmgt") happens in the init_module of i915. Before the
initialization path is finished in i915, the i915 symbols are not
available to be referenced. Unfortunately, It's matter of sequence, not
of delay. :(
>> code:
>> https://github.com/intel/gvt-linux/blob/bd950a66c7919d7121d2530f30984351534a96dc/kernel/module.c#L1452
>>
>> In resolve_symbol_wait(), it calls resolve_symbol() to resolve the
>> symbols in "i915". In resolve_symbol() -> ref_module() ->
>> strong_try_module_get(), it will check the status of the module which
>> owns the symbol.
>>
>> static inline int strong_try_module_get(struct module *mod)
>> {
>> BUG_ON(mod && mod->state == MODULE_STATE_UNFORMED);
>> if (mod && mod->state == MODULE_STATE_COMING)
>> return -EBUSY;
>> if (try_module_get(mod))
>> return 0;
>> else
>> return -ENOENT;
>> }
>>
>> code:https://github.com/intel/gvt-linux/blob/bd950a66c7919d7121d2530f30984351534a96dc/kernel/module.c#L318
>>
>> But unfortunately, this execution path begins in i915 module loading, at
>> this time, the status of kernel module "i915" is MODULE_STATE_COMING
>> until loading of "kvmgt" is finished. Thus a -EBUSY is always returned
>> when kernel is trying to resolve symbols for "kvmgt".
>>
>>
>> This patch below might need re-work:
> If the above test patch still fails, well.. that might be telling of
> another issue which is perhaps difficult to see at first glance. If
> resolve_sy
Re: refactor the i915 GVT support
On Tue, Sep 28, 2021 at 07:41:00AM +, Wang, Zhi A wrote:
> Hey guys:
>
> After some investigation, I found the root cause this problem ("i915"
> module loading will be stuck with Christoph's refactor patches), which
> can be reproduced by building both i915 and kvmgt as kernel module and
> the loading i915.
Thanks for looking into this!
> The root cause is: in Linux kernel loading, before a kernel module
> loading is finished, its symbols can not be reached by other module when
> resolving the symbols (even they can be found in /proc/kallsyms).
> Because the status of the kernel module is MODULE_STATE_COMING and
> resolve_symbol() from another kernel module will check this and return a
> -EBUSY.
Well, it would seem that way but...
> In this case, before i915 loading is finished, the requested module
> "kvmgt" cannot reach the symbols in module i915. Thus it kept waiting
> and left message like below in the dmesg:
>
> [ 644.152021] kvmgt: gave up waiting for init of module i915.
> [ 644.152039] kvmgt: Unknown symbol i915_gem_object_set_to_cpu_domain
> (err -16)
> [ 674.871409] kvmgt: gave up waiting for init of module i915.
> [ 674.871427] kvmgt: Unknown symbol intel_ring_begin (err -16)
> [ 705.590586] kvmgt: gave up waiting for init of module i915.
> [ 705.590604] kvmgt: Unknown symbol i915_vma_move_to_active (err -16)
> [ 736.310230] kvmgt: gave up waiting for init of module i915.
> [ 736.310248] kvmgt: Unknown symbol shmem_unpin_map (err -16)
> ...
>
> The error message is from execution path below:
>
> kernel/module.c:
>
> [i915 module loading] ->
> request_module("kvmgt")->[modprobe]->init_module("kvmgt")->load_module()->simplify_symbols()->resolve_symbol_wait():
>
> static const struct kernel_symbol *
> resolve_symbol_wait(struct module *mod,
> const struct load_info *info,
> const char *name)
> {
> const struct kernel_symbol *ksym;
> char owner[MODULE_NAME_LEN];
>
> if (wait_event_interruptible_timeout(module_wq,
> !IS_ERR(ksym = resolve_symbol(mod, info, name, owner))
> || PTR_ERR(ksym) != -EBUSY,
> 30 * HZ) <= 0) {
> pr_warn("%s: gave up waiting for init of module %s.\n",
> mod->name, owner);
>
> }
Commit 9bea7f23952d5 ("module: fix bne2 "gave up waiting for init of
module libcrc32c") is worth reviewing. It dealt with a similar issue,
and in particular it addressed the issue with -EBUSY being returned
by ref_module().
And so, in theory that case should be dealt with in resolve_symbol_wait()
already. And so can you try this just to verify something:
diff --git a/kernel/module.c b/kernel/module.c
index 40ec9a030eec..98f87cbb37de 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1459,7 +1459,7 @@ resolve_symbol_wait(struct module *mod,
if (wait_event_interruptible_timeout(module_wq,
!IS_ERR(ksym = resolve_symbol(mod, info, name, owner))
|| PTR_ERR(ksym) != -EBUSY,
-30 * HZ) <= 0) {
+160 * HZ) <= 0) {
pr_warn("%s: gave up waiting for init of module %s.\n",
mod->name, owner);
}
> code:
> https://github.com/intel/gvt-linux/blob/bd950a66c7919d7121d2530f30984351534a96dc/kernel/module.c#L1452
>
> In resolve_symbol_wait(), it calls resolve_symbol() to resolve the
> symbols in "i915". In resolve_symbol() -> ref_module() ->
> strong_try_module_get(), it will check the status of the module which
> owns the symbol.
>
> static inline int strong_try_module_get(struct module *mod)
> {
> BUG_ON(mod && mod->state == MODULE_STATE_UNFORMED);
> if (mod && mod->state == MODULE_STATE_COMING)
> return -EBUSY;
> if (try_module_get(mod))
> return 0;
> else
> return -ENOENT;
> }
>
> code:https://github.com/intel/gvt-linux/blob/bd950a66c7919d7121d2530f30984351534a96dc/kernel/module.c#L318
>
> But unfortunately, this execution path begins in i915 module loading, at
> this time, the status of kernel module "i915" is MODULE_STATE_COMING
> until loading of "kvmgt" is finished. Thus a -EBUSY is always returned
> when kernel is trying to resolve symbols for "kvmgt".
>
>
> This patch below might need re-work:
If the above test patch still fails, well.. that might be telling of
another issue which is perhaps difficult to see at first glance. If
resolve_symbol_wait() won't succeed until request_module("kvmgt")
completes and if this means having kvmgt's init routine complete, that
could end up in some longer chain or in the worst case a sort of
circular dependency which is only implicated by module loading. It'd be
really odd... but I cannot rule it out.
This is one reason I hinted that you should strive to not do much on a
module's init. If you can punt work off for later that's best.
Luis
>
> Author: Christoph Hellwig
> Da
Re: refactor the i915 GVT support
Hey guys:
After some investigation, I found the root cause this problem ("i915"
module loading will be stuck with Christoph's refactor patches), which
can be reproduced by building both i915 and kvmgt as kernel module and
the loading i915.
The root cause is: in Linux kernel loading, before a kernel module
loading is finished, its symbols can not be reached by other module when
resolving the symbols (even they can be found in /proc/kallsyms).
Because the status of the kernel module is MODULE_STATE_COMING and
resolve_symbol() from another kernel module will check this and return a
-EBUSY.
In this case, before i915 loading is finished, the requested module
"kvmgt" cannot reach the symbols in module i915. Thus it kept waiting
and left message like below in the dmesg:
[ 644.152021] kvmgt: gave up waiting for init of module i915.
[ 644.152039] kvmgt: Unknown symbol i915_gem_object_set_to_cpu_domain
(err -16)
[ 674.871409] kvmgt: gave up waiting for init of module i915.
[ 674.871427] kvmgt: Unknown symbol intel_ring_begin (err -16)
[ 705.590586] kvmgt: gave up waiting for init of module i915.
[ 705.590604] kvmgt: Unknown symbol i915_vma_move_to_active (err -16)
[ 736.310230] kvmgt: gave up waiting for init of module i915.
[ 736.310248] kvmgt: Unknown symbol shmem_unpin_map (err -16)
...
The error message is from execution path below:
kernel/module.c:
[i915 module loading] ->
request_module("kvmgt")->[modprobe]->init_module("kvmgt")->load_module()->simplify_symbols()->resolve_symbol_wait():
static const struct kernel_symbol *
resolve_symbol_wait(struct module *mod,
const struct load_info *info,
const char *name)
{
const struct kernel_symbol *ksym;
char owner[MODULE_NAME_LEN];
if (wait_event_interruptible_timeout(module_wq,
!IS_ERR(ksym = resolve_symbol(mod, info, name, owner))
|| PTR_ERR(ksym) != -EBUSY,
30 * HZ) <= 0) {
pr_warn("%s: gave up waiting for init of module %s.\n",
mod->name, owner);
}
code:
https://github.com/intel/gvt-linux/blob/bd950a66c7919d7121d2530f30984351534a96dc/kernel/module.c#L1452
In resolve_symbol_wait(), it calls resolve_symbol() to resolve the
symbols in "i915". In resolve_symbol() -> ref_module() ->
strong_try_module_get(), it will check the status of the module which
owns the symbol.
static inline int strong_try_module_get(struct module *mod)
{
BUG_ON(mod && mod->state == MODULE_STATE_UNFORMED);
if (mod && mod->state == MODULE_STATE_COMING)
return -EBUSY;
if (try_module_get(mod))
return 0;
else
return -ENOENT;
}
code:https://github.com/intel/gvt-linux/blob/bd950a66c7919d7121d2530f30984351534a96dc/kernel/module.c#L318
But unfortunately, this execution path begins in i915 module loading, at
this time, the status of kernel module "i915" is MODULE_STATE_COMING
until loading of "kvmgt" is finished. Thus a -EBUSY is always returned
when kernel is trying to resolve symbols for "kvmgt".
This patch below might need re-work:
Author: Christoph Hellwig
Date: Wed Jul 21 17:53:38 2021 +0200
drm/i915/gvt: move the gvt code into kvmgt.ko
Instead of having an option to build the gvt code into the main i915
module, just move it into the kvmgt.ko module. This only requires
a new struct with three entries that the main i915 module needs to
request before enabling VGPU passthrough operations.
This also conveniently streamlines the GVT initialization and avoids
the need for the global device pointer.
Signed-off-by: Christoph Hellwig
Signed-off-by: Zhenyu Wang
Link:
http://patchwork.freedesktop.org/patch/msgid/20210721155355.173183-5-...@lst.de
Acked-by: Zhenyu Wang
On 8/26/21 6:12 AM, Zhenyu Wang wrote:
> On 2021.08.20 12:56:34 -0700, Luis Chamberlain wrote:
>> On Fri, Aug 20, 2021 at 04:17:24PM +0200, Christoph Hellwig wrote:
>>> On Thu, Aug 19, 2021 at 04:29:29PM +0800, Zhenyu Wang wrote:
I'm working on below patch to resolve this. But I met a weird issue in
case when building i915 as module and also kvmgt module, it caused
busy wait on request_module("kvmgt") when boot, it doesn't happen if
building i915 into kernel. I'm not sure what could be the reason?
>>> Luis, do you know if there is a problem with a request_module from
>>> a driver ->probe routine that is probably called by a module_init
>>> function itself?
>> Generally no, but you can easily foot yourself in the feet by creating
>> cross dependencies and not dealing with them properly. I'd make sure
>> to keep module initialization as simple as possible, and run whatever
>> takes more time asynchronously, then use a state machine to allow
>> you to verify where you are in the initialization phase or query it
>> or wait for a completion with a timeout.
>>
>> It seems the code in question is getting some spring cleaning, and its
>> unclear where the code is I c
Re: refactor the i915 GVT support
On 2021.08.20 12:56:34 -0700, Luis Chamberlain wrote:
> On Fri, Aug 20, 2021 at 04:17:24PM +0200, Christoph Hellwig wrote:
> > On Thu, Aug 19, 2021 at 04:29:29PM +0800, Zhenyu Wang wrote:
> > > I'm working on below patch to resolve this. But I met a weird issue in
> > > case when building i915 as module and also kvmgt module, it caused
> > > busy wait on request_module("kvmgt") when boot, it doesn't happen if
> > > building i915 into kernel. I'm not sure what could be the reason?
> >
> > Luis, do you know if there is a problem with a request_module from
> > a driver ->probe routine that is probably called by a module_init
> > function itself?
>
> Generally no, but you can easily foot yourself in the feet by creating
> cross dependencies and not dealing with them properly. I'd make sure
> to keep module initialization as simple as possible, and run whatever
> takes more time asynchronously, then use a state machine to allow
> you to verify where you are in the initialization phase or query it
> or wait for a completion with a timeout.
>
> It seems the code in question is getting some spring cleaning, and its
> unclear where the code is I can inspect. If there's a tree somewhere I
> can take a peak I'd be happy to review possible oddities that may stick
> out.
I tried to put current patches under test here:
https://github.com/intel/gvt-linux/tree/gvt-staging
The issue can be produced with CONFIG_DRM_I915=m and
CONFIG_DRM_I915_GVT_KVMGT=m.
>
> My goto model for these sorts of problems is to abstract the issue
> *outside* of the driver in question and implement new selftests to
> try to reproduce. This serves two purposes, 1) helps with testing
> 2) may allow you to see the problem more clearly.
>
I'll see if can abstract that.
Thanks, Luis.
signature.asc
Description: PGP signature
Re: refactor the i915 GVT support
On 2021.08.20 16:17:24 +0200, Christoph Hellwig wrote:
> On Thu, Aug 19, 2021 at 04:29:29PM +0800, Zhenyu Wang wrote:
> > I'm working on below patch to resolve this. But I met a weird issue in
> > case when building i915 as module and also kvmgt module, it caused
> > busy wait on request_module("kvmgt") when boot, it doesn't happen if
> > building i915 into kernel. I'm not sure what could be the reason?
>
> Luis, do you know if there is a problem with a request_module from
> a driver ->probe routine that is probably called by a module_init
> function itself?
>
> In the meantime I'll try to reproduce it locally, but I always had a
> hard time getting useful results out of a modular i915, especially
> when combined with module paramters. (no blame on i915, just the problem
> with modules needed early on).
>
> >
> > > But the problem I see is that after moving gvt device model (gvt/*.c
> > > except kvmgt.c) into kvmgt module, we'll have issue with initial mmio
> > > state which current gvt relies on, that is in design supposed to get
> > > initial HW state before i915 driver has taken any operation. Before
> > > we can ensure that, I think we may only remove MPT part first but
> > > still keep gvt device model as part of i915 with config. I'll try to
> > > split that out.
> >
> > Sorry I misread the code that as we always request kvmgt module when
> > gvt init, so it should still apply original method that this isn't a
> > problem. Our current validation result has shown no regression as well.
>
> What does initial mmio state mean? This is something new to me. But
> as you said in this mail unless I missed something very big it should
> work the same as before.
>
It's gvt internal track for all gfx mmio state, and yes with your current
change it should still work as before.
> > -static inline void intel_context_unpin(struct intel_context *ce)
> > +static inline void _intel_context_unpin(struct intel_context *ce)
> > {
> > if (!ce->ops->sched_disable) {
> > __intel_context_do_unpin(ce, 1);
> > @@ -150,6 +150,7 @@ static inline void intel_context_unpin(struct
> > intel_context *ce)
> > }
> > }
> > }
> > +void intel_context_unpin(struct intel_context *ce);
>
> Looking at intel_context_unpin/_intel_context_unpin is there really
> a need to have this inline to start with? It don't see much the compiler
> could optimize by inlining it.
I'll send patch to i915 for this, and get more comments there.
thanks
signature.asc
Description: PGP signature
Re: refactor the i915 GVT support
On 2021.08.19 17:43:43 +0300, Joonas Lahtinen wrote:
> Quoting Zhenyu Wang (2021-08-19 11:29:29)
> > On 2021.08.17 13:22:03 +0800, Zhenyu Wang wrote:
> > > > On 2021.08.16 19:34:58 +0200, Christoph Hellwig wrote:
> > > > > Any updates on this? I'd really hate to miss this merge window.
> > > >
> > > > I'm still waiting for our validation team's report on this. I'm afraid
> > > > it might be missing for next version as i915 merge window is mostly
> > > > till rc5...and for any change outside of gvt, it still needs to be
> > > > acked by i915 maintainers.
> > >
> > > Looks our validation team did have problem against recent i915 change.
> > > If you like to try, we have a gvt-staging branch on
> > > https://github.com/intel/gvt-linux which is generated against drm-tip
> > > with gvt changes for testing, currently it's broken.
> > >
> > > One issue is with i915 export that intel_context_unpin has been
> > > changed into static inline function. Another is that intel_gvt.c
> > > should be part of i915 for gvt interface instead of depending on KVMGT
> > > config.
> >
> > I'm working on below patch to resolve this. But I met a weird issue in
> > case when building i915 as module and also kvmgt module, it caused
> > busy wait on request_module("kvmgt") when boot, it doesn't happen if
> > building i915 into kernel. I'm not sure what could be the reason?
> >
> > > But the problem I see is that after moving gvt device model (gvt/*.c
> > > except kvmgt.c) into kvmgt module, we'll have issue with initial mmio
> > > state which current gvt relies on, that is in design supposed to get
> > > initial HW state before i915 driver has taken any operation.
>
> As mentioned in some past discussions, I think it would be best rely on
> golden MMIO located in /lib/firmware or elsewhere. This way we will better
> isolate the guest system from host system updates/changes.
>
> This should also hopefully allow enabling kvmgt module after i915 has
> already loaded, as the initialization would not be conditional to
> capture the MMIO.
>
I think the concern is that even for same GEN hw there could be many
variant platforms e.g APL with gen9, etc. To verify golden states for
them all might take too much effort...
>
> > > Before
> > > we can ensure that, I think we may only remove MPT part first but
> > > still keep gvt device model as part of i915 with config. I'll try to
> > > split that out.
> >
> > Sorry I misread the code that as we always request kvmgt module when
> > gvt init, so it should still apply original method that this isn't a
> > problem. Our current validation result has shown no regression as well.
> >
> > ---8<---
> > From 58ff84572f1a0f9d79ca1d7ec0cff5ecbe78d280 Mon Sep 17 00:00:00 2001
> > From: Zhenyu Wang
> > Date: Thu, 19 Aug 2021 16:36:33 +0800
> > Subject: [PATCH] TESTONLY:drm/i915/gvt: potential fix for refactor against
> > current tip
> >
> > ---
> > drivers/gpu/drm/i915/Makefile | 4 +++-
> > drivers/gpu/drm/i915/gt/intel_context.c | 5 +
> > drivers/gpu/drm/i915/gt/intel_context.h | 3 ++-
> > drivers/gpu/drm/i915/i915_trace.h | 1 +
> > 4 files changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> > index c4f953837f72..2248574428a1 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -296,7 +296,9 @@ i915-$(CONFIG_DRM_I915_SELFTEST) += \
> >
> > # virtual gpu code
> > i915-y += i915_vgpu.o
> > -i915-$(CONFIG_DRM_I915_GVT_KVMGT) += intel_gvt.o
> > +ifneq ($(CONFIG_DRM_I915_GVT_KVMGT),)
> > +i915-y += intel_gvt.o
> > +endif
> >
> > kvmgt-y += gvt/kvmgt.o \
> > gvt/gvt.o \
> > diff --git a/drivers/gpu/drm/i915/gt/intel_context.c
> > b/drivers/gpu/drm/i915/gt/intel_context.c
> > index 745e84c72c90..20e7522fed84 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_context.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> > @@ -328,6 +328,11 @@ void __intel_context_do_unpin(struct intel_context
> > *ce, int sub)
> > intel_context_put(ce);
> > }
> >
> > +void intel_context_unpin(struct intel_context *ce)
> > +{
> > + _intel_context_unpin(ce);
> > +}
> > +
> > static void __intel_context_retire(struct i915_active *active)
> > {
> > struct intel_context *ce = container_of(active, typeof(*ce),
> > active);
> > diff --git a/drivers/gpu/drm/i915/gt/intel_context.h
> > b/drivers/gpu/drm/i915/gt/intel_context.h
> > index c41098950746..f942cbf6300a 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_context.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_context.h
> > @@ -131,7 +131,7 @@ static inline void
> > intel_context_sched_disable_unpin(struct intel_context *ce)
> > __intel_context_do_unpin(ce, 2);
> > }
> >
> > -static inline void intel_context_unpin(struct intel_context *ce)
> > +static inline void _intel_context_unpin(struct intel_context *ce)
> > {
> > if (!ce->ops->sched_disable) {
> > __intel
Re: refactor the i915 GVT support
On Fri, Aug 20, 2021 at 04:17:24PM +0200, Christoph Hellwig wrote:
> On Thu, Aug 19, 2021 at 04:29:29PM +0800, Zhenyu Wang wrote:
> > I'm working on below patch to resolve this. But I met a weird issue in
> > case when building i915 as module and also kvmgt module, it caused
> > busy wait on request_module("kvmgt") when boot, it doesn't happen if
> > building i915 into kernel. I'm not sure what could be the reason?
>
> Luis, do you know if there is a problem with a request_module from
> a driver ->probe routine that is probably called by a module_init
> function itself?
Generally no, but you can easily foot yourself in the feet by creating
cross dependencies and not dealing with them properly. I'd make sure
to keep module initialization as simple as possible, and run whatever
takes more time asynchronously, then use a state machine to allow
you to verify where you are in the initialization phase or query it
or wait for a completion with a timeout.
It seems the code in question is getting some spring cleaning, and its
unclear where the code is I can inspect. If there's a tree somewhere I
can take a peak I'd be happy to review possible oddities that may stick
out.
My goto model for these sorts of problems is to abstract the issue
*outside* of the driver in question and implement new selftests to
try to reproduce. This serves two purposes, 1) helps with testing
2) may allow you to see the problem more clearly.
Luis
Re: refactor the i915 GVT support
Quoting Zhenyu Wang (2021-08-19 11:29:29)
> On 2021.08.17 13:22:03 +0800, Zhenyu Wang wrote:
> > > On 2021.08.16 19:34:58 +0200, Christoph Hellwig wrote:
> > > > Any updates on this? I'd really hate to miss this merge window.
> > >
> > > I'm still waiting for our validation team's report on this. I'm afraid
> > > it might be missing for next version as i915 merge window is mostly
> > > till rc5...and for any change outside of gvt, it still needs to be
> > > acked by i915 maintainers.
> >
> > Looks our validation team did have problem against recent i915 change.
> > If you like to try, we have a gvt-staging branch on
> > https://github.com/intel/gvt-linux which is generated against drm-tip
> > with gvt changes for testing, currently it's broken.
> >
> > One issue is with i915 export that intel_context_unpin has been
> > changed into static inline function. Another is that intel_gvt.c
> > should be part of i915 for gvt interface instead of depending on KVMGT
> > config.
>
> I'm working on below patch to resolve this. But I met a weird issue in
> case when building i915 as module and also kvmgt module, it caused
> busy wait on request_module("kvmgt") when boot, it doesn't happen if
> building i915 into kernel. I'm not sure what could be the reason?
>
> > But the problem I see is that after moving gvt device model (gvt/*.c
> > except kvmgt.c) into kvmgt module, we'll have issue with initial mmio
> > state which current gvt relies on, that is in design supposed to get
> > initial HW state before i915 driver has taken any operation.
As mentioned in some past discussions, I think it would be best rely on
golden MMIO located in /lib/firmware or elsewhere. This way we will better
isolate the guest system from host system updates/changes.
This should also hopefully allow enabling kvmgt module after i915 has
already loaded, as the initialization would not be conditional to
capture the MMIO.
Regards, Joonas
> > Before
> > we can ensure that, I think we may only remove MPT part first but
> > still keep gvt device model as part of i915 with config. I'll try to
> > split that out.
>
> Sorry I misread the code that as we always request kvmgt module when
> gvt init, so it should still apply original method that this isn't a
> problem. Our current validation result has shown no regression as well.
>
> ---8<---
> From 58ff84572f1a0f9d79ca1d7ec0cff5ecbe78d280 Mon Sep 17 00:00:00 2001
> From: Zhenyu Wang
> Date: Thu, 19 Aug 2021 16:36:33 +0800
> Subject: [PATCH] TESTONLY:drm/i915/gvt: potential fix for refactor against
> current tip
>
> ---
> drivers/gpu/drm/i915/Makefile | 4 +++-
> drivers/gpu/drm/i915/gt/intel_context.c | 5 +
> drivers/gpu/drm/i915/gt/intel_context.h | 3 ++-
> drivers/gpu/drm/i915/i915_trace.h | 1 +
> 4 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index c4f953837f72..2248574428a1 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -296,7 +296,9 @@ i915-$(CONFIG_DRM_I915_SELFTEST) += \
>
> # virtual gpu code
> i915-y += i915_vgpu.o
> -i915-$(CONFIG_DRM_I915_GVT_KVMGT) += intel_gvt.o
> +ifneq ($(CONFIG_DRM_I915_GVT_KVMGT),)
> +i915-y += intel_gvt.o
> +endif
>
> kvmgt-y += gvt/kvmgt.o \
> gvt/gvt.o \
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c
> b/drivers/gpu/drm/i915/gt/intel_context.c
> index 745e84c72c90..20e7522fed84 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.c
> +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> @@ -328,6 +328,11 @@ void __intel_context_do_unpin(struct intel_context *ce,
> int sub)
> intel_context_put(ce);
> }
>
> +void intel_context_unpin(struct intel_context *ce)
> +{
> + _intel_context_unpin(ce);
> +}
> +
> static void __intel_context_retire(struct i915_active *active)
> {
> struct intel_context *ce = container_of(active, typeof(*ce), active);
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.h
> b/drivers/gpu/drm/i915/gt/intel_context.h
> index c41098950746..f942cbf6300a 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context.h
> @@ -131,7 +131,7 @@ static inline void
> intel_context_sched_disable_unpin(struct intel_context *ce)
> __intel_context_do_unpin(ce, 2);
> }
>
> -static inline void intel_context_unpin(struct intel_context *ce)
> +static inline void _intel_context_unpin(struct intel_context *ce)
> {
> if (!ce->ops->sched_disable) {
> __intel_context_do_unpin(ce, 1);
> @@ -150,6 +150,7 @@ static inline void intel_context_unpin(struct
> intel_context *ce)
> }
> }
> }
> +void intel_context_unpin(struct intel_context *ce);
>
> void intel_context_enter_engine(struct intel_context *ce);
> void intel_context_exit_engine(struct intel_context *ce);
> diff --git a/drivers/gpu/drm/i915/i915_trace.h
> b/drivers/gpu/drm/i915/i915_trace.h
> index 806ad688274
Re: refactor the i915 GVT support
On 2021.08.17 13:22:03 +0800, Zhenyu Wang wrote:
> > On 2021.08.16 19:34:58 +0200, Christoph Hellwig wrote:
> > > Any updates on this? I'd really hate to miss this merge window.
> >
> > I'm still waiting for our validation team's report on this. I'm afraid
> > it might be missing for next version as i915 merge window is mostly
> > till rc5...and for any change outside of gvt, it still needs to be
> > acked by i915 maintainers.
>
> Looks our validation team did have problem against recent i915 change.
> If you like to try, we have a gvt-staging branch on
> https://github.com/intel/gvt-linux which is generated against drm-tip
> with gvt changes for testing, currently it's broken.
>
> One issue is with i915 export that intel_context_unpin has been
> changed into static inline function. Another is that intel_gvt.c
> should be part of i915 for gvt interface instead of depending on KVMGT
> config.
I'm working on below patch to resolve this. But I met a weird issue in
case when building i915 as module and also kvmgt module, it caused
busy wait on request_module("kvmgt") when boot, it doesn't happen if
building i915 into kernel. I'm not sure what could be the reason?
> But the problem I see is that after moving gvt device model (gvt/*.c
> except kvmgt.c) into kvmgt module, we'll have issue with initial mmio
> state which current gvt relies on, that is in design supposed to get
> initial HW state before i915 driver has taken any operation. Before
> we can ensure that, I think we may only remove MPT part first but
> still keep gvt device model as part of i915 with config. I'll try to
> split that out.
Sorry I misread the code that as we always request kvmgt module when
gvt init, so it should still apply original method that this isn't a
problem. Our current validation result has shown no regression as well.
---8<---
From 58ff84572f1a0f9d79ca1d7ec0cff5ecbe78d280 Mon Sep 17 00:00:00 2001
From: Zhenyu Wang
Date: Thu, 19 Aug 2021 16:36:33 +0800
Subject: [PATCH] TESTONLY:drm/i915/gvt: potential fix for refactor against
current tip
---
drivers/gpu/drm/i915/Makefile | 4 +++-
drivers/gpu/drm/i915/gt/intel_context.c | 5 +
drivers/gpu/drm/i915/gt/intel_context.h | 3 ++-
drivers/gpu/drm/i915/i915_trace.h | 1 +
4 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index c4f953837f72..2248574428a1 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -296,7 +296,9 @@ i915-$(CONFIG_DRM_I915_SELFTEST) += \
# virtual gpu code
i915-y += i915_vgpu.o
-i915-$(CONFIG_DRM_I915_GVT_KVMGT) += intel_gvt.o
+ifneq ($(CONFIG_DRM_I915_GVT_KVMGT),)
+i915-y += intel_gvt.o
+endif
kvmgt-y += gvt/kvmgt.o \
gvt/gvt.o \
diff --git a/drivers/gpu/drm/i915/gt/intel_context.c
b/drivers/gpu/drm/i915/gt/intel_context.c
index 745e84c72c90..20e7522fed84 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -328,6 +328,11 @@ void __intel_context_do_unpin(struct intel_context *ce,
int sub)
intel_context_put(ce);
}
+void intel_context_unpin(struct intel_context *ce)
+{
+ _intel_context_unpin(ce);
+}
+
static void __intel_context_retire(struct i915_active *active)
{
struct intel_context *ce = container_of(active, typeof(*ce), active);
diff --git a/drivers/gpu/drm/i915/gt/intel_context.h
b/drivers/gpu/drm/i915/gt/intel_context.h
index c41098950746..f942cbf6300a 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.h
+++ b/drivers/gpu/drm/i915/gt/intel_context.h
@@ -131,7 +131,7 @@ static inline void intel_context_sched_disable_unpin(struct
intel_context *ce)
__intel_context_do_unpin(ce, 2);
}
-static inline void intel_context_unpin(struct intel_context *ce)
+static inline void _intel_context_unpin(struct intel_context *ce)
{
if (!ce->ops->sched_disable) {
__intel_context_do_unpin(ce, 1);
@@ -150,6 +150,7 @@ static inline void intel_context_unpin(struct intel_context
*ce)
}
}
}
+void intel_context_unpin(struct intel_context *ce);
void intel_context_enter_engine(struct intel_context *ce);
void intel_context_exit_engine(struct intel_context *ce);
diff --git a/drivers/gpu/drm/i915/i915_trace.h
b/drivers/gpu/drm/i915/i915_trace.h
index 806ad688274b..2c6a8bcef7c1 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -17,6 +17,7 @@
#undef TRACE_SYSTEM
#define TRACE_SYSTEM i915
+#undef TRACE_INCLUDE_FILE
#define TRACE_INCLUDE_FILE i915_trace
/* watermark/fifo updates */
--
2.32.0
---8<---
signature.asc
Description: PGP signature
Re: refactor the i915 GVT support
On 2021.08.17 09:08:55 +0800, Zhenyu Wang wrote: > On 2021.08.16 19:34:58 +0200, Christoph Hellwig wrote: > > On Wed, Aug 04, 2021 at 01:26:06PM +0800, Zhenyu Wang wrote: > > > On 2021.08.03 11:30:58 -0300, Jason Gunthorpe wrote: > > > > On Tue, Aug 03, 2021 at 05:43:15PM +0800, Zhenyu Wang wrote: > > > > > Acked-by: Zhenyu Wang > > > > > > > > > > Thanks a lot for this effort! > > > > > > > > Great, do we have a submission plan for this? how much does it clash > > > > with my open_device/etc patch? ie does the whole thing have to go > > > > through the vfio tree? > > > > > > > > > > I think Alex would determine when to merge open_device series, gvt part > > > can be through vfio tree without problem. For this refactor, I would first > > > merge for gvt staging to do more regression testing before sending through > > > i915 tree. > > > > Any updates on this? I'd really hate to miss this merge window. > > I'm still waiting for our validation team's report on this. I'm afraid > it might be missing for next version as i915 merge window is mostly > till rc5...and for any change outside of gvt, it still needs to be > acked by i915 maintainers. Looks our validation team did have problem against recent i915 change. If you like to try, we have a gvt-staging branch on https://github.com/intel/gvt-linux which is generated against drm-tip with gvt changes for testing, currently it's broken. One issue is with i915 export that intel_context_unpin has been changed into static inline function. Another is that intel_gvt.c should be part of i915 for gvt interface instead of depending on KVMGT config. But the problem I see is that after moving gvt device model (gvt/*.c except kvmgt.c) into kvmgt module, we'll have issue with initial mmio state which current gvt relies on, that is in design supposed to get initial HW state before i915 driver has taken any operation. Before we can ensure that, I think we may only remove MPT part first but still keep gvt device model as part of i915 with config. I'll try to split that out. signature.asc Description: PGP signature
Re: refactor the i915 GVT support
On 2021.08.16 19:34:58 +0200, Christoph Hellwig wrote: > On Wed, Aug 04, 2021 at 01:26:06PM +0800, Zhenyu Wang wrote: > > On 2021.08.03 11:30:58 -0300, Jason Gunthorpe wrote: > > > On Tue, Aug 03, 2021 at 05:43:15PM +0800, Zhenyu Wang wrote: > > > > Acked-by: Zhenyu Wang > > > > > > > > Thanks a lot for this effort! > > > > > > Great, do we have a submission plan for this? how much does it clash > > > with my open_device/etc patch? ie does the whole thing have to go > > > through the vfio tree? > > > > > > > I think Alex would determine when to merge open_device series, gvt part > > can be through vfio tree without problem. For this refactor, I would first > > merge for gvt staging to do more regression testing before sending through > > i915 tree. > > Any updates on this? I'd really hate to miss this merge window. I'm still waiting for our validation team's report on this. I'm afraid it might be missing for next version as i915 merge window is mostly till rc5...and for any change outside of gvt, it still needs to be acked by i915 maintainers. signature.asc Description: PGP signature
Re: refactor the i915 GVT support
On 2021.08.03 11:30:58 -0300, Jason Gunthorpe wrote: > On Tue, Aug 03, 2021 at 05:43:15PM +0800, Zhenyu Wang wrote: > > Acked-by: Zhenyu Wang > > > > Thanks a lot for this effort! > > Great, do we have a submission plan for this? how much does it clash > with my open_device/etc patch? ie does the whole thing have to go > through the vfio tree? > I think Alex would determine when to merge open_device series, gvt part can be through vfio tree without problem. For this refactor, I would first merge for gvt staging to do more regression testing before sending through i915 tree. Thanks signature.asc Description: PGP signature
Re: refactor the i915 GVT support
On Tue, Aug 03, 2021 at 05:43:15PM +0800, Zhenyu Wang wrote: > Acked-by: Zhenyu Wang > > Thanks a lot for this effort! Great, do we have a submission plan for this? how much does it clash with my open_device/etc patch? ie does the whole thing have to go through the vfio tree? Thanks, Jason
Re: refactor the i915 GVT support
On 2021.07.29 09:20:22 +0200, Christoph Hellwig wrote: > On Wed, Jul 28, 2021 at 02:59:25PM -0300, Jason Gunthorpe wrote: > > On Wed, Jul 28, 2021 at 01:38:58PM +, Wang, Zhi A wrote: > > > > > I guess those APIs you were talking about are KVM-only. For other > > > hypervisors, e.g. Xen, ARCN cannot use the APIs you mentioned. Not > > > sure if you have already noticed that VFIO is KVM-only right now. > > > > There is very little hard connection between VFIO and KVM, so no, I > > don't think that is completely true. > > The only connection is the SET_KVM notifier as far as I can tell. > Which is used by a total of two drivers, including i915/gvt. That > being said gvt does not only use vfio, but also does quite a few > direct cals to KVM. yeah, we mostly combined VFIO into hypervisor specific thing before, e.g interface to set vgpu edid, etc. along with kvm specific calls. > > > In an event, an in-tree version of other hypervisor support for GVT > > needs to go through enabling VFIO support so that the existing API > > multiplexers we have can be used properly, not adding a shim layer > > trying to recreate VFIO inside a GPU driver. > > Yes. And if we go back to actually looking at the series a lot of > it just removes entirely pointless indirect calls that go to generic > code and not even the kvm code, or questionable data structure designs. > If we were to support another upstream hypervisor we'd just need to > union a few fields in struct intel_gpu and maybe introduce a few > methods. Preferably in a way that avoids expensive indirect calls > in the fast path. ok, agree on that. Acked-by: Zhenyu Wang Thanks a lot for this effort! signature.asc Description: PGP signature
Re: refactor the i915 GVT support
On 7/28/2021 8:59 PM, Jason Gunthorpe wrote: > On Wed, Jul 28, 2021 at 01:38:58PM +, Wang, Zhi A wrote: > >> I guess those APIs you were talking about are KVM-only. For other >> hypervisors, e.g. Xen, ARCN cannot use the APIs you mentioned. Not >> sure if you have already noticed that VFIO is KVM-only right now. > There is very little hard connection between VFIO and KVM, so no, I > don't think that is completely true. > > In an event, an in-tree version of other hypervisor support for GVT > needs to go through enabling VFIO support so that the existing API > multiplexers we have can be used properly, not adding a shim layer > trying to recreate VFIO inside a GPU driver. We were delivering the presentation of GVT-g in Xen summit 2018 and we were thinking and talking about supporting VFIO in Xen during the presentation (the video can be found from Youtube). But we didn't see any motiviation from the Xen community to adopt it. If people take a look into the code in QEMU, in the PCI-passthrough part, Xen is actually not using VFIO even nowadays. We would be glad to see someone can influence on that part, especically making all the in-kernel hypervisor to use VFIO in PCI-passthrough and supporting mdev. That would be a huge benefit for all the users. >> GVT-g is designed for many hypervisors not only KVM. In the design, >> we implemented an abstraction layer for different hypervisors. You >> can check the link in the previous email which has an example of how >> the MPT module "xengt" supports GVT-g running under Xen. For >> example, injecting a msi in VFIO/KVM is via playing with >> eventfd. But in Xen, we need to issue a hypercall from Dom0. > This is obviously bad design, Xen should plug into the standardized > eventfd scheme as well and trigger its hypercall this way. Then it can > integrate with the existing VFIO interrupt abstraction infrastructure. > >> others, like querying mappings between GFN and HFN. > This should be done through VFIO containers, there is nothing KVM > specific there. > >> As you can see, to survive from this situation, we have to rely on >> an abstraction layer so that we can prevent introducing coding >> blocks like in the core logic: > No, you have to fix the abstractions we already have to support the > matrix of things you care about. If this can't be done then maybe we > can add new abstractions, but abstractions like this absoultely should > not be done inside drivers. > > Jason That's a good point and we were actually thinking about this before and I believe that's the correct direction. But just like the situation mentioned above, it would be nice if people can really put a great influence on all in-kernel hypervisors to use VFIO which can really benefit all the users. For now, we are just going to take christoph's patches. Zhi
Re: [Intel-gfx] refactor the i915 GVT support
On Thu, Jul 29, 2021 at 09:20:22AM +0200, Christoph Hellwig wrote: > On Wed, Jul 28, 2021 at 02:59:25PM -0300, Jason Gunthorpe wrote: > > On Wed, Jul 28, 2021 at 01:38:58PM +, Wang, Zhi A wrote: > > > > > I guess those APIs you were talking about are KVM-only. For other > > > hypervisors, e.g. Xen, ARCN cannot use the APIs you mentioned. Not > > > sure if you have already noticed that VFIO is KVM-only right now. > > > > There is very little hard connection between VFIO and KVM, so no, I > > don't think that is completely true. > > The only connection is the SET_KVM notifier as far as I can tell. > Which is used by a total of two drivers, including i915/gvt. That > being said gvt does not only use vfio, but also does quite a few > direct cals to KVM. > > > In an event, an in-tree version of other hypervisor support for GVT > > needs to go through enabling VFIO support so that the existing API > > multiplexers we have can be used properly, not adding a shim layer > > trying to recreate VFIO inside a GPU driver. > > Yes. And if we go back to actually looking at the series a lot of > it just removes entirely pointless indirect calls that go to generic > code and not even the kvm code, or questionable data structure designs. > If we were to support another upstream hypervisor we'd just need to > union a few fields in struct intel_gpu and maybe introduce a few > methods. Preferably in a way that avoids expensive indirect calls > in the fast path. fwiw I concur with the direction of this series. gvt landed 5 years ago, that should have been plenty of time to merge at least one of the other backends that float around. If it didn't happen in 5 years it aint suddenly happening in the next few, and the abstraction layer should be sunset. Also yes structuring it more as a helper layer with some unions/subclassing than full blown backend abstractor layer would be a good idea too I guess (it usually is the right thing to do). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: refactor the i915 GVT support
On Wed, Jul 28, 2021 at 01:38:58PM +, Wang, Zhi A wrote: > I guess those APIs you were talking about are KVM-only. For other > hypervisors, e.g. Xen, ARCN cannot use the APIs you mentioned. Not > sure if you have already noticed that VFIO is KVM-only right now. There is very little hard connection between VFIO and KVM, so no, I don't think that is completely true. In an event, an in-tree version of other hypervisor support for GVT needs to go through enabling VFIO support so that the existing API multiplexers we have can be used properly, not adding a shim layer trying to recreate VFIO inside a GPU driver. > GVT-g is designed for many hypervisors not only KVM. In the design, > we implemented an abstraction layer for different hypervisors. You > can check the link in the previous email which has an example of how > the MPT module "xengt" supports GVT-g running under Xen. For > example, injecting a msi in VFIO/KVM is via playing with > eventfd. But in Xen, we need to issue a hypercall from Dom0. This is obviously bad design, Xen should plug into the standardized eventfd scheme as well and trigger its hypercall this way. Then it can integrate with the existing VFIO interrupt abstraction infrastructure. > others, like querying mappings between GFN and HFN. This should be done through VFIO containers, there is nothing KVM specific there. > As you can see, to survive from this situation, we have to rely on > an abstraction layer so that we can prevent introducing coding > blocks like in the core logic: No, you have to fix the abstractions we already have to support the matrix of things you care about. If this can't be done then maybe we can add new abstractions, but abstractions like this absoultely should not be done inside drivers. Jason
Re: refactor the i915 GVT support
A: http://en.wikipedia.org/wiki/Top_post Q: Were do I find info about this thing called top-posting? A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? A: Top-posting. Q: What is the most annoying thing in e-mail? A: No. Q: Should I include quotations after my reply? http://daringfireball.net/2007/07/on_top On Wed, Jul 28, 2021 at 01:38:58PM +, Wang, Zhi A wrote: > Hi Jason: > > I guess those APIs you were talking about are KVM-only. For other > hypervisors, e.g. Xen, ARCN cannot use the APIs you mentioned. Not sure if > you have already noticed that VFIO is KVM-only right now. Please wrap your lines properly :( > GVT-g is designed for many hypervisors not only KVM. In the design, we > implemented an abstraction layer for different hypervisors. You can check the > link in the previous email which has an example of how the MPT module "xengt" > supports GVT-g running under Xen. > For example, injecting a msi in VFIO/KVM is via playing with eventfd. But in > Xen, we need to issue a hypercall from Dom0. So does others, like querying > mappings between GFN and HFN. Some GPU related emulation logic might be > implemented differently under different hypervisors because different > hypervisors might provide not exact the APIs we want. That's the reason why > they get a callback in the MPT (yet not perfect.) > > As you can see, to survive from this situation, we have to rely on an > abstraction layer so that we can prevent introducing coding blocks like in > the core logic: > > If (in_hypervisor_xen) > Issue hypercalls > else if (in_hypervisor_kvm) > Play with eventfds. > Else if (in_hypervisor_other) > That's horrid, and slow, please do this properly. > Thus some of the APIs have to be wrapped in the MPT module in GVT-g design. > > Sadly, not all customers are motivated or allowed to get their > hypervisor-specific modules into the kernel. We have a customer who runs > GVT-g with their private hypervisor. In this case, they don't want to get > their "xxxgt" MPT module into upstream since their hypervisor has been in the > kernel yet. Also, we have customers who ported the GVT-g to QNX which is > another widely used commercial hypervisor in the industry. They can't get the > "qnxgt" MPT module into upstream since it's not allowed. Why is it not allowed? > We do understand the situation and try to figure out a solution that can > fulfill expectations from different people in the community and also > customers. > > According to Greg KH's comments, we are collecting the requirements of MPT > modules from other open-source hypervisors in the kernel, e.g. ACRN, to see > if they can get another MPT module into the kernel, which will show an > example that how the MPT abstraction can benefit. Also, we are evaluating the > impact on our customers if we have to remove MPT abstraction in the kernel > because there is only one MPT module. Until that happens, can we please just remove the unneeded layer here as no one is using it? Then, when you have a real user for this type of middle-layer, you can add it back? We have no need for it now. thanks, greg k-h
RE: refactor the i915 GVT support
Hi Jason: I guess those APIs you were talking about are KVM-only. For other hypervisors, e.g. Xen, ARCN cannot use the APIs you mentioned. Not sure if you have already noticed that VFIO is KVM-only right now. GVT-g is designed for many hypervisors not only KVM. In the design, we implemented an abstraction layer for different hypervisors. You can check the link in the previous email which has an example of how the MPT module "xengt" supports GVT-g running under Xen. For example, injecting a msi in VFIO/KVM is via playing with eventfd. But in Xen, we need to issue a hypercall from Dom0. So does others, like querying mappings between GFN and HFN. Some GPU related emulation logic might be implemented differently under different hypervisors because different hypervisors might provide not exact the APIs we want. That's the reason why they get a callback in the MPT (yet not perfect.) As you can see, to survive from this situation, we have to rely on an abstraction layer so that we can prevent introducing coding blocks like in the core logic: If (in_hypervisor_xen) Issue hypercalls else if (in_hypervisor_kvm) Play with eventfds. Else if (in_hypervisor_other) Thus some of the APIs have to be wrapped in the MPT module in GVT-g design. Sadly, not all customers are motivated or allowed to get their hypervisor-specific modules into the kernel. We have a customer who runs GVT-g with their private hypervisor. In this case, they don't want to get their "xxxgt" MPT module into upstream since their hypervisor has been in the kernel yet. Also, we have customers who ported the GVT-g to QNX which is another widely used commercial hypervisor in the industry. They can't get the "qnxgt" MPT module into upstream since it's not allowed. We do understand the situation and try to figure out a solution that can fulfill expectations from different people in the community and also customers. According to Greg KH's comments, we are collecting the requirements of MPT modules from other open-source hypervisors in the kernel, e.g. ACRN, to see if they can get another MPT module into the kernel, which will show an example that how the MPT abstraction can benefit. Also, we are evaluating the impact on our customers if we have to remove MPT abstraction in the kernel because there is only one MPT module. Thanks so much for the comments. Thanks, Zhi. -Original Message- From: Jason Gunthorpe Sent: Tuesday, July 27, 2021 3:12 PM To: Gerd Hoffmann Cc: Wang, Zhi A ; Christoph Hellwig ; Jani Nikula ; Joonas Lahtinen ; Vivi, Rodrigo ; Zhenyu Wang ; intel-...@lists.freedesktop.org; intel-gvt-...@lists.freedesktop.org; linux-ker...@vger.kernel.org; dri-devel@lists.freedesktop.org Subject: Re: refactor the i915 GVT support On Thu, Jul 22, 2021 at 01:26:36PM +0200, Gerd Hoffmann wrote: > Hi, > > > https://github.com/intel/gvt-linux/blob/topic/gvt-xengt/drivers/gpu/ > > drm/i915/gvt/xengt.c > > > But it's hard for some customers to contribute their own "hypervisor" > > module to the upstream Linux kernel. I am thinking what would be a > > better solution here? The MPT layer in the kernel helps a lot for > > customers, but only one open-source "hypervisor" module is there in > > the kernel. That can confuse people which don't know the story. One > > thing I was thinking is to put a document about the background and > > more description in the MPT headers. So it won't confuse more people. > > Getting the xengt module linked above merged into mainline would also > nicely explain why there are hypervisor modules. It would also be nice to explain why a GPU driver needs a hypervisor specific shim like this in the first place. enum hypervisor_type type; int (*host_init)(struct device *dev, void *gvt, const void *ops); void (*host_exit)(struct device *dev, void *gvt); int (*attach_vgpu)(void *vgpu, unsigned long *handle); void (*detach_vgpu)(void *vgpu); Doesn't vfio provide all this generically with notifiers? int (*inject_msi)(unsigned long handle, u32 addr, u16 data); Isn't this one just an eventfd? unsigned long (*from_virt_to_mfn)(void *p); int (*read_gpa)(unsigned long handle, unsigned long gpa, void *buf, unsigned long len); int (*write_gpa)(unsigned long handle, unsigned long gpa, void *buf, unsigned long len); unsigned long (*gfn_to_mfn)(unsigned long handle, unsigned long gfn); int (*dma_map_guest_page)(unsigned long handle, unsigned long gfn, unsigned long size, dma_addr_t *dma_addr); void (*dma_unmap_guest_page)(unsigned long handle, dma_addr_t dma_addr); int (*dma_pin_guest_page)(unsigned
Re: refactor the i915 GVT support
On Thu, Jul 22, 2021 at 01:26:36PM +0200, Gerd Hoffmann wrote: > Hi, > > > https://github.com/intel/gvt-linux/blob/topic/gvt-xengt/drivers/gpu/drm/i915/gvt/xengt.c > > > But it's hard for some customers to contribute their own "hypervisor" > > module to the upstream Linux kernel. I am thinking what would be a > > better solution here? The MPT layer in the kernel helps a lot for > > customers, but only one open-source "hypervisor" module is there in > > the kernel. That can confuse people which don't know the story. One > > thing I was thinking is to put a document about the background and > > more description in the MPT headers. So it won't confuse more people. > > Getting the xengt module linked above merged into mainline > would also nicely explain why there are hypervisor modules. It would also be nice to explain why a GPU driver needs a hypervisor specific shim like this in the first place. enum hypervisor_type type; int (*host_init)(struct device *dev, void *gvt, const void *ops); void (*host_exit)(struct device *dev, void *gvt); int (*attach_vgpu)(void *vgpu, unsigned long *handle); void (*detach_vgpu)(void *vgpu); Doesn't vfio provide all this generically with notifiers? int (*inject_msi)(unsigned long handle, u32 addr, u16 data); Isn't this one just an eventfd? unsigned long (*from_virt_to_mfn)(void *p); int (*read_gpa)(unsigned long handle, unsigned long gpa, void *buf, unsigned long len); int (*write_gpa)(unsigned long handle, unsigned long gpa, void *buf, unsigned long len); unsigned long (*gfn_to_mfn)(unsigned long handle, unsigned long gfn); int (*dma_map_guest_page)(unsigned long handle, unsigned long gfn, unsigned long size, dma_addr_t *dma_addr); void (*dma_unmap_guest_page)(unsigned long handle, dma_addr_t dma_addr); int (*dma_pin_guest_page)(unsigned long handle, dma_addr_t dma_addr); int (*map_gfn_to_mfn)(unsigned long handle, unsigned long gfn, unsigned long mfn, unsigned int nr, bool map); bool (*is_valid_gfn)(unsigned long handle, unsigned long gfn); Shouldn't the vfio page SW IOMMU do all of this generically? int (*enable_page_track)(unsigned long handle, u64 gfn); int (*disable_page_track)(unsigned long handle, u64 gfn); int (*set_trap_area)(unsigned long handle, u64 start, u64 end, bool map); int (*set_opregion)(void *vgpu); int (*set_edid)(void *vgpu, int port_num); edid depends on hypervisor?? int (*get_vfio_device)(void *vgpu); void (*put_vfio_device)(void *vgpu); Jason
Re: refactor the i915 GVT support
On Thu, Jul 22, 2021 at 10:49:47AM +, Wang, Zhi A wrote: > But it's hard for some customers to contribute their own "hypervisor" > module to the upstream Linux kernel. What prevents them from doing this? We will take any code that meets our standards, what format is this external code in? > I am thinking what would be a > better solution here? The MPT layer in the kernel helps a lot for > customers, but only one open-source "hypervisor" module is there in > the kernel. That can confuse people which don't know the story. One > thing I was thinking is to put a document about the background and > more description in the MPT headers. So it won't confuse more people. If no one is using it in the kernel, it needs to be removed. No abstractions should be added that are not required by the in-tree code. So this series should be accepted, _or_ the external code needs to be submitted for inclusion. thanks, greg k-h
Re: refactor the i915 GVT support
Hi, > https://github.com/intel/gvt-linux/blob/topic/gvt-xengt/drivers/gpu/drm/i915/gvt/xengt.c > But it's hard for some customers to contribute their own "hypervisor" > module to the upstream Linux kernel. I am thinking what would be a > better solution here? The MPT layer in the kernel helps a lot for > customers, but only one open-source "hypervisor" module is there in > the kernel. That can confuse people which don't know the story. One > thing I was thinking is to put a document about the background and > more description in the MPT headers. So it won't confuse more people. Getting the xengt module linked above merged into mainline would also nicely explain why there are hypervisor modules. take care, Gerd
RE: refactor the i915 GVT support
Hi Christoph: Thanks so much for the patches and the testing. The abstraction between i915 and KVMGT module is for our customers who can easily port GVT-g into their own hypervisors. As you can see, all the hypervisor related functions were put in "hypervisor" module. The GVT-g module talks with the "hypervisor" module through the MPT layer. The customers just need to focus on their "hypervisor" module, implement and attach their own "hypervisor" modules to the MPT layer without touching the GVT-g core logic, which reduce great efforts during the porting as the core logic of GVT-g in i915 is pretty vendor-specific and customers aren't motivated to touch it unless they have to. The boundary between GVT-g core logic and "hypervisor" module also helps a lot on narrowing down the problems when supporting our customers. According to our experience during the support, the less a customer touches the core logic, the less problem will be introduced. We get many customers who are using commercial hypervisors like QNX or their private hypervisors in many areas in the industry. An reference implementation of "Xen hypervisor" module to demonstrate our customer how to port GVT-g to a type-1 hypervisor instead of KVM can be found here: https://github.com/intel/gvt-linux/blob/topic/gvt-xengt/drivers/gpu/drm/i915/gvt/xengt.c But it's hard for some customers to contribute their own "hypervisor" module to the upstream Linux kernel. I am thinking what would be a better solution here? The MPT layer in the kernel helps a lot for customers, but only one open-source "hypervisor" module is there in the kernel. That can confuse people which don't know the story. One thing I was thinking is to put a document about the background and more description in the MPT headers. So it won't confuse more people. Feel free to drop more comments. 😊 Thanks, Zhi. -Original Message- From: Christoph Hellwig Sent: Wednesday, July 21, 2021 6:54 PM To: Jani Nikula ; Joonas Lahtinen ; Vivi, Rodrigo ; Zhenyu Wang ; Wang, Zhi A Cc: intel-...@lists.freedesktop.org; intel-gvt-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-ker...@vger.kernel.org Subject: refactor the i915 GVT support Hi all, the GVT code in the i915 is a bit of a mess right now due to strange abstractions and lots of indirect calls. This series refactors various bits to clean that up. The main user visible change is that almost all of the GVT code moves out of the main i915 driver and into the kvmgt module. Tested on my Thinkpad with a Kaby Lake CPU and integrated graphics. Git tree: git://git.infradead.org/users/hch/misc.git i915-gvt Gitweb: http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/i915-gvt Diffstat: b/drivers/gpu/drm/i915/Kconfig | 31 b/drivers/gpu/drm/i915/Makefile| 30 b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c |4 b/drivers/gpu/drm/i915/gvt/cfg_space.c | 89 -- b/drivers/gpu/drm/i915/gvt/cmd_parser.c|4 b/drivers/gpu/drm/i915/gvt/dmabuf.c| 36 b/drivers/gpu/drm/i915/gvt/execlist.c | 12 b/drivers/gpu/drm/i915/gvt/gtt.c | 55 - b/drivers/gpu/drm/i915/gvt/gvt.c | 100 -- b/drivers/gpu/drm/i915/gvt/gvt.h | 132 ++- b/drivers/gpu/drm/i915/gvt/interrupt.c | 38 - b/drivers/gpu/drm/i915/gvt/kvmgt.c | 634 - b/drivers/gpu/drm/i915/gvt/mmio.c |4 b/drivers/gpu/drm/i915/gvt/opregion.c | 148 --- b/drivers/gpu/drm/i915/gvt/page_track.c|8 b/drivers/gpu/drm/i915/gvt/scheduler.c | 37 b/drivers/gpu/drm/i915/gvt/trace.h |2 b/drivers/gpu/drm/i915/gvt/vgpu.c | 22 b/drivers/gpu/drm/i915/i915_drv.h |7 b/drivers/gpu/drm/i915/i915_params.c |2 b/drivers/gpu/drm/i915/intel_gvt.c | 64 + b/drivers/gpu/drm/i915/intel_gvt.h |4 drivers/gpu/drm/i915/gvt/Makefile |9 drivers/gpu/drm/i915/gvt/hypercall.h | 82 -- drivers/gpu/drm/i915/gvt/mpt.h | 400 -- 25 files changed, 541 insertions(+), 1413 deletions(-)
Re: refactor the i915 GVT support
On 2021.07.21 17:53:34 +0200, Christoph Hellwig wrote: > Hi all, > > the GVT code in the i915 is a bit of a mess right now due to strange > abstractions and lots of indirect calls. This series refactors various > bits to clean that up. The main user visible change is that almost all > of the GVT code moves out of the main i915 driver and into the kvmgt > module. > The reason we isolated hypervisor specific code from core vgpu emulation is to make multiple hypervisor support possible. Yes, we do have Xen support but never got way into upstream...And we also have third party hypervisors which leverage gvt function through current hypervisor interface. Sorry I may not have more time to check in details for now, but some of them look fine to me. I'll review more after vacation or let Zhi check details. Thanks! > Tested on my Thinkpad with a Kaby Lake CPU and integrated graphics. > > Git tree: > > git://git.infradead.org/users/hch/misc.git i915-gvt > > Gitweb: > > http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/i915-gvt > > Diffstat: > b/drivers/gpu/drm/i915/Kconfig | 31 > b/drivers/gpu/drm/i915/Makefile| 30 > b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c |4 > b/drivers/gpu/drm/i915/gvt/cfg_space.c | 89 -- > b/drivers/gpu/drm/i915/gvt/cmd_parser.c|4 > b/drivers/gpu/drm/i915/gvt/dmabuf.c| 36 > b/drivers/gpu/drm/i915/gvt/execlist.c | 12 > b/drivers/gpu/drm/i915/gvt/gtt.c | 55 - > b/drivers/gpu/drm/i915/gvt/gvt.c | 100 -- > b/drivers/gpu/drm/i915/gvt/gvt.h | 132 ++- > b/drivers/gpu/drm/i915/gvt/interrupt.c | 38 - > b/drivers/gpu/drm/i915/gvt/kvmgt.c | 634 > - > b/drivers/gpu/drm/i915/gvt/mmio.c |4 > b/drivers/gpu/drm/i915/gvt/opregion.c | 148 --- > b/drivers/gpu/drm/i915/gvt/page_track.c|8 > b/drivers/gpu/drm/i915/gvt/scheduler.c | 37 > b/drivers/gpu/drm/i915/gvt/trace.h |2 > b/drivers/gpu/drm/i915/gvt/vgpu.c | 22 > b/drivers/gpu/drm/i915/i915_drv.h |7 > b/drivers/gpu/drm/i915/i915_params.c |2 > b/drivers/gpu/drm/i915/intel_gvt.c | 64 + > b/drivers/gpu/drm/i915/intel_gvt.h |4 > drivers/gpu/drm/i915/gvt/Makefile |9 > drivers/gpu/drm/i915/gvt/hypercall.h | 82 -- > drivers/gpu/drm/i915/gvt/mpt.h | 400 -- > 25 files changed, 541 insertions(+), 1413 deletions(-) signature.asc Description: PGP signature
