Re: refactor the i915 GVT support

["Wang, Zhi A" <zhi.a.wang@intel.com>]

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 +0000, Wang, Zhi A wrote:
>>> On 9/28/21 3:05 PM, Jason Gunthorpe wrote:
>>>> On Tue, Sep 28, 2021 at 02:35:06PM +0000, 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
>
>