Re: [PATCH v2 4/6] x86/NPT: don't walk entire page tables when changing types on a range

[Tim Deegan <tim@xen.org>]

At 07:13 +0100 on 25 Apr (1398406414), Jan Beulich wrote:
> >>> On 24.04.14 at 18:25, <tim@xen.org> wrote:
> > At 13:30 +0100 on 22 Apr (1398169846), Jan Beulich wrote:
> >> This builds on the fact that in order for no NPF VM exit to occur,
> >> _PAGE_USER must always be set. I.e. by clearing the flag we can force a
> >> VM exit allowing us to do similar lazy type changes as on EPT.
> >> 
> >> That way, the generic entry-wise code can go away, and we could remove
> >> the range restriction in enforced on HVMOP_track_dirty_vram for XSA-27.
> >> 
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> 
> >> --- a/xen/arch/x86/hvm/svm/svm.c
> >> +++ b/xen/arch/x86/hvm/svm/svm.c
> >> @@ -2557,7 +2557,16 @@ void svm_vmexit_handler(struct cpu_user_
> >>          perfc_incra(svmexits, VMEXIT_NPF_PERFC);
> >>          if ( cpu_has_svm_decode )
> >>              v->arch.hvm_svm.cached_insn_len = vmcb->guest_ins_len & 0xf;
> >> -        svm_do_nested_pgfault(v, regs, vmcb->exitinfo1, vmcb->exitinfo2);
> >> +        rc = p2m_npt_fault(vmcb->exitinfo2);
> > 
> > Can we limit the classes of fault that we need to check for
> > recalc on?  e.g. if bit 0 of the error code is clear, then the page is
> > not mapped at all. 
> 
> That's a good suggestion (albeit I'm not sure if there really are
> frequent faults with P clear - with paging perhaps, but that's a slow
> path anyway). Ideally we would have an indication that the fault
> was because of the U bit clear, but sadly that doesn't exist.

Yeah, that's a shame.

> > I'm unsure about having two different fixup paths here anyway -- one
> > for log-dirty and one for everything else (PoD, sharing &c).  This
> > call should probably go _inside_ svm_do_nested_pgfault() at least.
> 
> I intentionally kept it separate, so it's not getting farther away than
> necessary from how EPT handling is structured.

Hmm.  Well, in that case I guess it's OK here.  The alternative would
be to put it right into the HVM nested fault handler and replumb the
ERPT path to go the same way, but I can see that would be inefficient...

> > Also, maybe it wants a more descriptive name than p2m_npt_fault().
> > p2m_pt_hnadle_misconfig(), to match the EPT equivalent?
> 
> I first considered naming it that way, but it's not really a mis-
> configuration. But if you think that's a better name despite not
> describing what it really does, I'm okay changing it...

I think I do prefer it, but maybe we can find a better name for both
of them: {p2m_pt,ept}_handle_deferred_changes()?

> > This looks like it could be merged with the EPT equivalent.  Or would
> > that be too unwieldy?
> 
> I considered unifying both this and/or the helpers of it, but decided
> that the result would be uglier (too many parameters just to tell one
> from the other) than the duplication that results from this approach.

OK, fair enough.

Tim.