On 25/08/14 13:45, Gianluca Guida wrote:
On Fri, Aug 22, 2014 at 10:34 AM, Andrew Cooper <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.

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.

~Andrew