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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
