From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tim Deegan Subject: Re: [PATCH 00/11] Alternate p2m: support multiple copies of host p2m Date: Fri, 16 Jan 2015 19:33:06 +0100 Message-ID: <20150116183306.GJ48064@deinos.phlegethon.org> References: <1420838801-11704-1-git-send-email-edmund.h.white@intel.com> <20150115161519.GA57240@deinos.phlegethon.org> <54B805B0.8090604@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <54B805B0.8090604@intel.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ed White Cc: keir@xen.org, ian.jackson@eu.citrix.com, ian.campbell@citrix.com, jbeulich@suse.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org Hi, At 10:23 -0800 on 15 Jan (1421313824), Ed White wrote: > On 01/15/2015 08:15 AM, Tim Deegan wrote: > > I see there's been some discussion of how this would be useful for an > > out-of-domain inspection tool, but could you talk some more about the > > usefulness of the in-VM callers? I'm not sure what advantage it > > brings over a kernel playing the same tricks in normal pagetables -- > > after all an attacker in the kernel can make hypercalls and so get > > around any p2m restrictions. > > > > Our original motivation for this work is that we have a requirement > internally to enable in-guest agent based memory partitioning for > Windows domains. > > There is commercially available software that uses a security > hypervisor to partition memory for Windows running on physical > hardware, and we want to make the same thing possible on > virtualized hardware. Righto, thanks. It sounds like you're not the only people interested in this feature, which is encouraging. > As I said in discussion with Andrew, my aim was to make it possible > for these same changes to be extensible to AMD processors if they > support multiple copies of whatever their EPT equivalent is, by > simply emulating VMFUNC and #VE. That's why there are some wrappers > in the implementation that appear redundant. Yep, understood, and thank you for that. But I think there was one function (to find a p2m by eptp) that's defined in vmx.c and only ever called from there -- that doesn't need an arch-specific wrapper, because an eventual AMD equivalent would also be entirely contained within svm.c. > > The second thing is how similar some of this is to nested p2m code, > > making me wonder whether it could share more code with that. It's not > > as much duplication as I had feared, but e.g. altp2m_write_p2m_entry() > > is _identical_ to nestedp2m_write_p2m_entry(), (making the > > copyright claim at the top of the file quite dubious, BTW). > > > > I did initially use nestedp2m_write_p2m_entry directly, but I knew > that wouldn't be acceptable! On this specific point, I would be more > inclined to refactor the normal write entry routine so you can call > it everywhere, since both the nested and alternate ones are simply > a copy of a part of the normal one. That sounds like an excellent idea. > > - Feature compatibilty/completeness. You pointed out yourself that > > it doesn't work with nested HVM or migration. I think I'd have to > > add mem_event/access/paging and PCI passthrough to the list of > > features that ought to still work. I'm resigned to the idea that > > many new features don't work with shadow pagetables. :) > > > > The intention is that mem_event/access should still work. I haven't > specifically looked at paging, but I don't see any fundamental reason > why it shouldn't. PCI passthrough I suspect won't. Does nested HVM > work with migration? Is it simply not acceptable to submit a feature > as experimental, with known compatibility issues? I had assumed that > it was, based on the nested HVM status as documented in the release > notes. Potentially, yes, if we have reasonable confidence that you (or someone else) will work towards fixing those things. If you can't make promises yourself, perhaps you can talk to someone who can. > > - Testing and sample code. If we're to carry this feature in the > > tree, we'll need at least some code to make use of it; ideally > > some sort of test we can run to find regressions later. > > Understood. As I said in another thread, I'm hoping the community > will be interested enough to help with that. If not, I'll try to > figure something out. Good, thanks. > > - Issues of coding style and patch hygiene. I don't think it's > > particularly useful to go into that in detail at this stage, but I > > did see some missing spaces in parentheses, e.g. for ( <-here-> ), > > and the patch series should be ordered so that the new feature is > > enabled in the last patch (in particular after 'fix log-dirty handling'!) > > The reason I put the fix log-dirty handling patch there is because I wasn't > convinced it would be acceptable, and it fixes a bug that already exists > and remains unfixed in the nested p2m code. IOW, alternate p2m would > work as well as nested p2m without that fix. I would have thought, from the tone of your earlier comments, that you were aiming for a bar somewhat higher than "as good as nestedp2m". :) I hope you'll also understand that given how well that has turned out, we shouldn't necessarily apply the same standard to new code as we did there. Cheers, Tim.