Re: [PATCH RFC v2 1/4] x86/mm: Shadow and p2m changes for PV mem_access

[Gianluca Guida <glguida@tlbflush.org>]

I just realised I replied to this email privately.

With apologies to Andrew, here's another reply.

On Mon, Aug 25, 2014 at 02:02:37PM +0100, Andrew Cooper wrote:
> On 25/08/14 13:45, Gianluca Guida wrote:
> > On Fri, Aug 22, 2014 at 10:34 AM, Andrew Cooper
> > <andrew.cooper3@citrix.com <mailto:andrew.cooper3@citrix.com>> wrote:
> >
> >     I am concerned with the addition of a the vcpu specifics to
> >     shadow_write_entries().  Most of the shadow code is already vcpu
> >     centric
> >     where it should be domain centric, and steps are being made to
> >     alleviate
> >     these problems.
> >
> >
> > The historical reason the code is set up this way, if you are
> > referring to the fact that every shadow operation is vcpu-specific
> > while the interface to it is domain specific, is due to the design
> > choice of leaving the door open to experiment with per-vcpu shadows.
> > That always looked like a nice feature, I am not sure anybody ever
> > implemented it. I would advocate -- for the sake of code consistency
> > -- to keep the current shadow internal interfaces per-vcpu in upcoming
> > patches, and change it when you propose your domain-centric patch,
> > effectively killing this probably never-exploited opportunity.
> >
> > Honestly, haven't been following shadow code in a while, so probably
> > consistency has already been lost, in which case you should feel free
> > to ignore this comment.
> >
> > Gianluca
> 
> I appreciate that it was done with vcpus to experiment with per-vcpu
> shadows, but it is fundamentally wrong (even in a per-vcpu shadows case)
> for certain paths to arbitrarily use d->vcpu[0] when calling into the
> shadow code.  At the very least, it adversely messes with the heuristics.

It is hackish, and ugly. Yes. [Not my code disclaimer here].
I don't follow it being _fundamentally_ wrong, or broken, but I am probably missing something. More specifically: are there known bugs in there due to the use of vcpu[0] from the callers of shadow code?

> There are fundamentally two separate entry paths into the shadow code. 
> First from the pagefault handler, which certainly is vcpu-centric, but
> also from toolstack operations, which is very much domain centric.  A
> per-vcpu shadow setup would need to use for_each_vcpu() under the hood,
> as it certainly couldn't trust that the caller has handed an appropriate
> vcpu.

This is a correct interpretation. But we should be able to trust the caller, if by that we mean mm.c.
But yes, adding an interface for the "toolstack operations" would makes things better, and contain in a proper way shadow internal architecture.

Gianluca