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

[Tim Deegan <tim@xen.org>]

Hello,

Thanks for sending this series - in particular, thank you for sending
it early in the release cycle!  I'll review some of the patches
individually but since I expect there will be some changes to come in
future versions I'm not going to go into too much detail.

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.


Looking at the code, the first thing that strikes me about it is that
you've tried to make a split between common code and VMX-specific
implementation details.  Thank you for that!  My only reservations
around that are that some of the naming of things in common code are
too vmx-specific.

I think it's probably OK to use 've', though I think that the term
'notify' might be better (which you use in the hypercall interface).
Using 'eptp' in common code is less good, though I think almost
everywhere in common code, you could just use 'p2m' instead.
Also, functions like 'find_by_eptp' that are only ever called
from vmx code don't need to be plumbed through common wrappers.
Also, I think you can probably s/altp2mhvm/altp2m/ throughout.


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).


In order to work towards getting this series merged, I think we have
four things to consider:

- General design.  I've made some comments above and some of the other
  maintainers have replied separately.  Assuming that the case can be
  made for needing this in the hypervisor at all, I think the overall
  direction is probably a good one.

- 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. :)

- 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.

- 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'!)

OK.  I have a few comments about the code itself; I'll reply to
individual patches with that.

Cheers,

Tim.