Re: [Xen-devel] [PATCH 00/11] Alternate p2m: support multiple copies of host p2m

[Tim Deegan <tim@xxxxxxx>]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel