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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

>>          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; I think there's
nothing wrong with gflags having any of these set.

Jan


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