Re: [Xen-devel] Re: Possible regression in "x86-64: reduce range spanned by 1:1 mapping and frame table indexes"

[Simon Horman <horms@xxxxxxxxxxxx>]

On Fri, Dec 11, 2009 at 12:15:06PM +0000, Tim Deegan wrote:
> At 11:56 +0000 on 11 Dec (1260532578), Simon Horman wrote:
> > > Hah.  That explains why we're emulating.  The guest has CR0.WP clear,
> > > and this was a write fault that wouldn't have happened on real hardware,
> > > so we need to emulate it because we can't actually disable CR0.WP or the
> > > shadow pagetables stop working altogether.
> > > 
> > > In fact in this case we don't need to emulate it because retrying would
> > > be good enough, but in the general case we might.
> > 
> > I'm not sure that I understand why that is true.
> 
> Which part?  We need to emulate writes when the guest's CR0.WP is 0 because 
> - we can't just set CR0.WP=0 or our write-protection of shadowed
>   pagetables goes away. 
> - we can't just use writeable shadow PTEs for read-only guest PTEs 
>   because (a) other vcpus mihgt have CR0.WP==1 (this does happen 
>   with virus scanner software on Windows) and (b) you can't 
>   properly express "read-only in userspace, read-write in kernel".
> 
> All we need to do is retry because in this particular case although 
> WP=0 the PTE is in fact read/write; it was just missing from the
> shadows.
> 
> > Although I'm not really that familiar with the code in question,
> > I think that I have a preference for both guarding the goto emulate
> > and adding an ASSERT to sh_remove_shadows(). That is:
> > 
> > Index: xen-unstable.hg/xen/arch/x86/mm/shadow/common.c
> > ===================================================================
> > --- xen-unstable.hg.orig/xen/arch/x86/mm/shadow/common.c    2009-12-11 
> > 20:31:48.000000000 +0900
> > +++ xen-unstable.hg/xen/arch/x86/mm/shadow/common.c 2009-12-11 
> > 20:48:48.000000000 +0900
> > @@ -2752,6 +2752,7 @@ void sh_remove_shadows(struct vcpu *v, m
> >      };
> >  
> >      ASSERT(!(all && fast));
> > +    ASSERT(mfn_valid(gmfn));
> >  
> >      /* Although this is an externally visible function, we do not know
> >       * whether the shadow lock will be held when it is called (since it
> > Index: xen-unstable.hg/xen/arch/x86/mm/shadow/multi.c
> > ===================================================================
> > --- xen-unstable.hg.orig/xen/arch/x86/mm/shadow/multi.c     2009-12-11 
> > 20:47:17.000000000 +0900
> > +++ xen-unstable.hg/xen/arch/x86/mm/shadow/multi.c  2009-12-11 
> > 20:48:17.000000000 +0900
> > @@ -3305,7 +3305,8 @@ static int sh_page_fault(struct vcpu *v,
> >       * fault was a non-user write to a present page.  */
> >      if ( is_hvm_domain(d) 
> >           && unlikely(!hvm_wp_enabled(v)) 
> > -         && regs->error_code == (PFEC_write_access|PFEC_page_present) )
> > +         && regs->error_code == (PFEC_write_access|PFEC_page_present)
> > +         && mfn_valid(gmfn) )
> >      {
> >          perfc_incr(shadow_fault_emulate_wp);
> >          goto emulate;
> > 

Oops, forgot

Signed-off-by: Simon Horman <horms@xxxxxxxxxxxx>

> Yes, that seems good to me.
> 
> Signed-off-by: Tim Deegan <Tim.Deegan@xxxxxxxxxx>


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel