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

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	Wei Liu <wl@xen.org>, Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [Xen-devel] [PATCH 9/9] x86: PCID is unused when !PV
Date: Thu, 12 Sep 2019 17:31:47 +0200	[thread overview]
Message-ID: <20190912153147.nhxdrifqney752lc@Air-de-Roger> (raw)
In-Reply-To: <f44f12f3-6449-8014-43e4-1f08100be251@suse.com>

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.

> +
>  u32 tlbflush_clock = 1U;
>  DEFINE_PER_CPU(u32, tlbflush_time);
>  
> --- 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?

>  }
>  
>  static inline unsigned long read_cr4(void)
> @@ -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?

Thanks, Roger.

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