Re: [Xen-devel] [PATCH v6 9/9] xen/x86: use PCID feature

[Juergen Gross <jgross@xxxxxxxx>]

On 10/04/18 11:29, Jan Beulich wrote:
>>>> On 10.04.18 at 09:58, <jgross@xxxxxxxx> wrote:
>> @@ -102,14 +104,34 @@ void switch_cr3_cr4(unsigned long cr3, unsigned long 
>> cr4)
>>      old_cr4 = read_cr4();
>>      if ( old_cr4 & X86_CR4_PGE )
>>      {
>> +        /*
>> +         * X86_CR4_PGE set means PCID is inactive.
>> +         * We have to purge the TLB via flipping cr4.pge.
>> +         */
>>          old_cr4 = cr4 & ~X86_CR4_PGE;
>>          write_cr4(old_cr4);
>>      }
>> +    else if ( use_invpcid )
>> +        /*
>> +         * Flushing the TLB via INVPCID is necessary only in case PCIDs are
>> +         * in use, which is true only with INVPCID being available.
>> +         * Without PCID usage the following write_cr3() will purge the TLB
>> +         * (we are in the cr4.pge off path) from all entries.
> 
> s/from/of/ ?
> 
>> @@ -136,11 +158,32 @@ unsigned int flush_area_local(const void *va, unsigned 
>> int flags)
>>              /*
>>               * We don't INVLPG multi-page regions because the 2M/4M/1G
>>               * region may not have been mapped with a superpage. Also there
>> -             * are various errata surrounding INVLPG usage on superpages, 
>> and
>> -             * a full flush is in any case not *that* expensive.
>> +             * are various errata surrounding INVLPG usage on superpages,
>> +             * and a full flush is in any case not *that* expensive.
>>               */
> 
> Stray change?
> 
>> @@ -508,7 +513,8 @@ unsigned long pv_guest_cr4_to_real_cr4(const struct vcpu 
>> *v)
>>      cr4 = v->arch.pv_vcpu.ctrlreg[4] & ~X86_CR4_DE;
>>      cr4 |= mmu_cr4_features & (X86_CR4_PSE | X86_CR4_SMEP | X86_CR4_SMAP |
>>                                 X86_CR4_OSXSAVE | X86_CR4_FSGSBASE);
>> -    cr4 |= d->arch.pv_domain.xpti  ? 0 : X86_CR4_PGE;
>> +    cr4 |= (d->arch.pv_domain.xpti || d->arch.pv_domain.pcid) ? 0 : 
>> X86_CR4_PGE;
>> +    cr4 |= d->arch.pv_domain.pcid ? X86_CR4_PCIDE : 0;
> 
> I think this would be more clear to follow as
> 
>     if ( d->arch.pv_domain.pcid )
>         cr4 |= X86_CR4_PCIDE;
>     else if ( !d->arch.pv_domain.xpti )
>         cr4 |= X86_CR4_PGE;
> 
> Anyway, with or without these addressed (which could probably
> also be done while committing, as long as you agree)

I do agree.

> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel