Re: [Xen-devel] [PATCH RFC] x86/shadow: adjust cachability flags handling

[Tim Deegan <tim@xxxxxxx>]

At 11:10 +0000 on 06 Mar (1394100642), Jan Beulich wrote:
> >>> On 06.03.14 at 11:53, Tim Deegan <tim@xxxxxxx> wrote:
> > At 14:49 +0000 on 05 Mar (1394027364), Jan Beulich wrote:
> >> For one, including _PAGE_PAT in the pass-through flags is valid only
> >> for L1 entries (otherwise _PAGE_PSE_PAT would need looking at). Looking
> >> around I _think_ that for page directories we'd always get a valid MFN
> >> passed in here, and hence I _think_ the assertion is correct.
> >> 
> >> And second we need to avoid or-ing guest PAT/PCD/PWT with ones coming
> >> from pat_type_2_pte_flags()/get_pat_flags(). An alternative to the
> >> pass_thru_flags check might be to use mfn_valid(target_mfn).
> >> 
> >> --- a/xen/arch/x86/mm/shadow/multi.c
> >> +++ b/xen/arch/x86/mm/shadow/multi.c
> >> @@ -580,7 +580,10 @@ _sh_propagate(struct vcpu *v, 
> >>      if ( guest_supports_nx(v) )
> >>          pass_thru_flags |= _PAGE_NX_BIT;
> >>      if ( !shadow_mode_refcounts(d) && !mfn_valid(target_mfn) )
> >> +    {
> >> +        ASSERT(level == 1);
> > 
> > Yes, this assertion is correct -- enforced by this block just above:
> > 
> >     if ( !mfn_valid(target_mfn)
> >          && !(level == 1 && (!shadow_mode_refcounts(d) 
> >                              || p2mt == p2m_mmio_direct)) )
> >     {
> >         ASSERT((ft == ft_prefetch));
> >         *sp = shadow_l1e_empty();
> >         goto done;
> >     }
> 
> I see. Question then is whether, together with the below, another
> assertion here is really worthwhile.

I think the one below would be enough. 

> >>          pass_thru_flags |= _PAGE_PAT | _PAGE_PCD | _PAGE_PWT;
> >> +    }
> >>      sflags = gflags & pass_thru_flags;
> >>  
> >>      /*
> >> @@ -588,6 +591,7 @@ _sh_propagate(struct vcpu *v, 
> >>       * caching attributes in the shadows to match what was asked for.
> >>       */
> >>      if ( (level == 1) && is_hvm_domain(d) &&
> >> +         !(pass_thru_flags & (_PAGE_PAT | _PAGE_PCD | _PAGE_PWT)) &&
> > 
> > I don't think this is necessary -- is_hvm_domain() implies
> > shadow_mode_refcounts(), so we won't have set these flags in
> > pass_thru_flags above.
> > 
> > If you want to make a change for clarity, I'd be happier with 
> > ASSERT(!(gflags & (_PAGE_PAT | _PAGE_PCD | _PAGE_PWT))) here.
> 
> But I suppose you really meant to use sflags here

Oops, yes I did. 

Cheers,

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel