Re: [Xen-devel] [PATCH 9/9] x86: PCID is unused when !PV

[Jan Beulich <jbeulich@suse.com>]

On 12.09.2019 17:31, Roger Pau Monné  wrote:
> On Wed, Sep 11, 2019 at 05:26:46PM +0200, Jan Beulich wrote:
>> This allows in particular some streamlining of the TLB flushing code
>> paths.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/x86/flushtlb.c
>> +++ b/xen/arch/x86/flushtlb.c
>> @@ -24,6 +24,11 @@
>>  #define WRAP_MASK (0x000003FFU)
>>  #endif
>>  
>> +#ifndef CONFIG_PV
>> +# undef X86_CR4_PCIDE
>> +# define X86_CR4_PCIDE 0
>> +#endif
> 
> I have to admit I find it quite ugly to have to mask PCID in such a
> way. Playing with the hardware architecture defines seems like asking
> for trouble. I would likely prefer to sprinkle IS_ENABLED(CONFIG_PV),
> which should achieve the same compile time short circuiting.

Well, yes, this isn't entirely without risk. But #ifdef-ary
isn't either. And the above fits the title of this patch
pretty well.

Andrew (in particular) - do you have any strong preference here?

>> --- a/xen/include/asm-x86/processor.h
>> +++ b/xen/include/asm-x86/processor.h
>> @@ -289,7 +289,11 @@ static inline unsigned long cr3_pa(unsig
>>  
>>  static inline unsigned int cr3_pcid(unsigned long cr3)
>>  {
>> +#ifdef CONFIG_PV
>>      return cr3 & X86_CR3_PCID_MASK;
>> +#else
>> +    return 0;
>> +#endif
> 
> Won't:
> 
> return IS_ENABLED(CONFIG_PV) ? cr3 & X86_CR3_PCID_MASK : 0;
> 
> Achieve the same?

Yes. I can certainly switch to that.

>> @@ -301,8 +305,12 @@ static inline void write_cr4(unsigned lo
>>  {
>>      struct cpu_info *info = get_cpu_info();
>>  
>> +#ifdef CONFIG_PV
>>      /* No global pages in case of PCIDs enabled! */
>>      ASSERT(!(val & X86_CR4_PGE) || !(val & X86_CR4_PCIDE));
>> +#else
>> +    ASSERT(!(val & X86_CR4_PCIDE));
> 
> That assert seems quite pointless, you have set X86_CR4_PCIDE to 0, so
> this is never going to trigger?

Good point, this is a leftover from when I started with the
#ifdef-ary approach, before it became too many of them for
my taste.

Jan

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