Re: [RFC 01/29] drm/i915/gvt: Introduce the basic architecture of GVT-g

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

Hi Chris:
     Thanks for your feedback. :) Currently there are some host i915 
changes we need to discussion, need your advice and feedback. For GVT 
LRC context, as you see, we need it to do shadow context. All the 
modifications of LRC are for it. Need inputs to think a better approach 
or code placement. :)

---

The GGTT page table is shadowed in GVT-g. When guest writes the GTT 
MMIOs, this write will be trapped by hypervisor, GVT-g will check if 
it's a MMIO access which needs to be emulated, or a GGTT page table 
entry which needs to be shadowed, then the shadowed GGTT page table 
entry will be written into pGGTT page table. So that the mapping looks like:

 From guest point of view:

guest PFN -> guest GGTT page table entry -> guest GGTT MMIO

The GVT-g shadow process looks like:

guest PFN -> guest GGTT page table entry -> GVT-g translated guest PFN 
to machine PFN -> GVT-g writes the translated entry into physical GGTT MMIO.

---

The hypervisor_{read, write}_va() abstraction layer is to cover the 
different approaches between hypervisors to let the host i915 to access 
guest memory.

For example, guest writes the vELSP to submit a workload, then 
hypervisor traps these writes and forward them to GVT-g core logic. 
Actually, if GVT-g wants to access the guest context, only LRCA (GGTT 
address) could be extracted from guest context descriptor.

First GVT-g will try to get the guest PFN from guest GGTT page table 
(You can see the GGTT/PPGTT page table entry extraction routines in gtt.c),

Second GVT-g will try to get the host VA with the guest PFN via 
hypervisor specific routines. (The function gvt_gma_to_va())

Then call hypervisor_{read,write}_va() to let hypervisor specific 
routines to read/write the data from/to guest via hypervisor specific 
routines.

This is a typical scenario of how GVT-g access a guest GGTT mapped 
memory. For PPGTT, it becomes much more complicated.(Also in 
gvt_gma_to_va())

On 01/30/16 00:48, Chris Wilson wrote:
> On Fri, Jan 29, 2016 at 03:57:09PM +0200, Joonas Lahtinen wrote:
>> Hi,
>>
>> TL;DR Overall, we have same problem as with the scheduler series, there
>> is too much placeholder stuff for easy review. Just squash enough code
>> into one commit so it actually does something logical that can be
>> reviewed and then extend it later. Then it can be reviewed and pushed.
>> Just splitting the code down to achieve smaller patches is not the
>> right thing to do.
>>
>> Comments on the overall code: You need to document all header file
>> functions (in the source files), and it is good to document the static
>> functions within a file too, to make future maintenance easier.
>>
>> It is not about splitting the code down to small chunks, but splitting
>> it down to small *logical* chunks. It doesn't make sense to commit
>> dozens of empty bodied functions for review, and then later add their
>> code.
>>
>> If you add functions, only add them at a patch that takes them into use
>> too, unless we're talking about general purpose shared code. And also
>> remember to add the function body and documentation header. If you
>> simply add a "return 0;" or similar function body, do add a comment to
>> why the function does not exist and when it will.
>>
>> Then, there is a trend of having a boolean return values in the code.
>> When possible, it should rather be int and the cause for failure should
>> be propagated from the last level all the way to up (-ENOMEN etc.).
>> This way debugging becomes easier and if new error conditions appear,
>> there is less of a maintenance burden to add the propagation later.
>>
>> Finally, make sure to look at the existing driver parts and
>> 	https://www.kernel.org/doc/Documentation/CodingStyle
>> for proper coding style. There are lots of whitespace fixes needed in
>> this series, like array initializations.
>>
>> I hope to see this first patch rerolled so that you squash some of the
>> later commits into it so that all functions have a body and you add
>> documentation for the functions so I can both see what it should do and
>> what it actually does. Only reroll the first patch, to keep the
>> iterative step smaller. Lets only then continue with the rest of the
>> series once we've reached a consensus on the formatting and style
>> basics.
>>
>> See more comments below.
>
> I'm glad you did the nitpicking. As far as the integration goes, on the
> whole I'm happy with the way it is structured and the reuse of existing
> code. I tried to attack various aspects of the GVT contexts and came to
> the conclusion I couldn't suggest a better approach (though maybe
> tomorrow!). A few bits and pieces I got lost trying to pull together
> (in particular like how we do is read back through the GTT entries
> performed, the hypervisor_read_va abstraction iirc) and would appreciate
> having a branch available to get the complete picture.
> -Chris
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx