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

[Tim Deegan <Tim.Deegan@xxxxxxxxxx>]

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

Yes, that seems good to me.

Signed-off-by: Tim Deegan <Tim.Deegan@xxxxxxxxxx>

Cheers,

Tim

-- 
Tim Deegan <Tim.Deegan@xxxxxxxxxx>
Principal Software Engineer, Citrix Systems (R&D) Ltd.
[Company #02300071, SL9 0DZ, UK.]

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