Re: [PATCH] nestedhvm: fix write access fault on ro mapping

[Tim Deegan <tim@xen.org>]

At 13:57 +0200 on 27 Jul (1343397454), Christoph Egger wrote:
> >> @@ -1291,6 +1291,8 @@ int hvm_hap_nested_page_fault(unsigned l
> >>              if ( !handle_mmio() )
> >>                  hvm_inject_hw_exception(TRAP_gp_fault, 0);
> >>              return 1;
> >> +        case NESTEDHVM_PAGEFAULT_READONLY:
> >> +            break;
> > 
> > Don't we have to translate the faulting PA into an L1 address before
> > letting the rest of this fault handler run?  It explicitly operates on
> > the hostp2m.  
> > 
> > If we do that, we should probably do it for NESTEDHVM_PAGEFAULT_ERROR,
> > rather than special-casing READONLY.  That way any other
> > automatically-fixed types (like the p2m_access magic) will be covered
> > too.
> 
> How do you differentiate if the error happened from walking l1 npt or
> host npt ?
> In the first case it isn't possible to provide l1 address.

It must be _possible_; after all we managed to detect the error. :)  In
any case it's definitely wrong to carry on with this handler with the
wrong address in hand.  So I wonder why this patch actually works for
you.  Does replacing the 'break' above with 'return 1' also fix the
problem?

In the short term, do you only care about pages that are read-only for
log-dirty tracking?  For the L1 walk, that should be handled by the PT
walker's own calls to paging_mark_dirty(), and the nested-p2m handler
could potentially take care of the other case by calling
paging_mark_dirty() (for writes!) before calling nestedhap_walk_L0_p2m().

Tim.