From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH RFC v2 1/4] x86/mm: Shadow and p2m changes for PV mem_access Date: Mon, 25 Aug 2014 14:02:37 +0100 Message-ID: <53FB33ED.7060107@citrix.com> References: <1404787805-4540-1-git-send-email-aravindp@cisco.com> <53D13457020000780002599B@mail.emea.novell.com> <97A500D504438F4ABC02EBA81613CC633188B795@xmb-aln-x02.cisco.com> <53D221270200007800025CEA@mail.emea.novell.com> <97A500D504438F4ABC02EBA81613CC633188BE3A@xmb-aln-x02.cisco.com> <53D60EA002000078000265FC@mail.emea.novell.com> <97A500D504438F4ABC02EBA81613CC633188EC46@xmb-aln-x02.cisco.com> <53D8B6AC0200007800027844@mail.emea.novell.com> <97A500D504438F4ABC02EBA81613CC633188EF44@xmb-aln-x02.cisco.com> <53DB52380200007800028448@mail.emea.novell.com> <97A500D504438F4ABC02EBA81613CC633188FBD7@xmb-aln-x02.cisco.com> <53DF4C6B0200007800028CA5@mail.emea.novell.com> <97A500D504438F4ABC02EBA81613CC63318A2DBD@xmb-rcd-x02.cisco.com> <53E096C902000078000294A2@mail.emea.novell.com> <97A500D 504438F4ABC02EBA81613CC63318E00F6@xmb-aln-x02.cisco.com> <53F70E99.7030501@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2322849279884248368==" Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1XLtvV-0005I1-Vv for xen-devel@lists.xenproject.org; Mon, 25 Aug 2014 13:03:14 +0000 In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Gianluca Guida Cc: Keir Fraser , Ian Campbell , Ian Jackson , Tim Deegan , Jan Beulich , "Aravindh Puthiyaparambil (aravindp)" , "xen-devel@lists.xenproject.org" List-Id: xen-devel@lists.xenproject.org --===============2322849279884248368== Content-Type: multipart/alternative; boundary="------------060606050204000605000507" --------------060606050204000605000507 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit On 25/08/14 13:45, Gianluca Guida wrote: > On Fri, Aug 22, 2014 at 10:34 AM, Andrew Cooper > > 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 --------------060606050204000605000507 Content-Type: text/html; charset="UTF-8" Content-Length: 3259 Content-Transfer-Encoding: quoted-printable
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().=C2=A0 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.=C2=A0 At the very least, it adversely messes with the heuristics.

There are fundamentally two separate entry paths into the shadow code.=C2=A0 First from the pagefault handler, which certainly is vcpu-centric, but also from toolstack operations, which is very much domain centric.=C2=A0 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 --------------060606050204000605000507-- --===============2322849279884248368== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============2322849279884248368==--