Re: [Xen-devel] [PATCH v2 3/5] x86: Fix APIC MSR constant names

2018-09-10 Thread Roger Pau Monné 
On Wed, Mar 07, 2018 at 06:58:34PM +, Andrew Cooper wrote:
> We currently have MSR_IA32_APICBASE and MSR_IA32_APICBASE_MSR which are
> synonymous from a naming point of view, but refer to very different things.
> 
> Rename the x2APIC MSRs to MSR_X2APIC_*, which are shorter constants and
> visually separate the register function from the generic APIC name.  For the
> case ranges, introduce MSR_X2APIC_LAST, rather than relying on the knowledge
> that there are 0x3ff MSRs architecturally reserved for x2APIC functionality.
> 
> For functionality relating to the APIC_BASE MSR, use MSR_APIC_BASE for the MSR
> itself, but drop the MSR prefix from the other constants to shorten the names.
> In all cases, the fact that we are dealing with the APIC_BASE MSR is obvious
> from the context.
> 
> No functional change (the combined binary is identical).
> 
> Signed-off-by: Andrew Cooper 

Reviewed-by: Roger Pau Monné 

> diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
> index 2b4014c..07f2209 100644
> --- a/xen/include/asm-x86/msr-index.h
> +++ b/xen/include/asm-x86/msr-index.h
> @@ -312,18 +312,21 @@
>  
>  #define MSR_IA32_TSC_ADJUST  0x003b
>  
> -#define MSR_IA32_APICBASE0x001b
> -#define MSR_IA32_APICBASE_BSP(1<<8)
> -#define MSR_IA32_APICBASE_EXTD   (1<<10)
> -#define MSR_IA32_APICBASE_ENABLE (1<<11)
> -#define MSR_IA32_APICBASE_BASE   0x000ff000ul
> -#define MSR_IA32_APICBASE_MSR   0x800
> -#define MSR_IA32_APICTPR_MSR0x808
> -#define MSR_IA32_APICPPR_MSR0x80a
> -#define MSR_IA32_APICEOI_MSR0x80b
> -#define MSR_IA32_APICTMICT_MSR  0x838
> -#define MSR_IA32_APICTMCCT_MSR  0x839
> -#define MSR_IA32_APICSELF_MSR   0x83f
> +#define MSR_APIC_BASE   0x001b
> +#define APIC_BASE_BSP   (1<<8)
> +#define APIC_BASE_EXTD  (1<<10)
> +#define APIC_BASE_ENABLE(1<<11)
> +#define APIC_BASE_BASE  0x000ff000ul

Maybe those could be indented like:

#define MSR_FOO
#define  FOO_BAR

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v2 3/5] x86: Fix APIC MSR constant names

2018-03-13 Thread Jan Beulich 
>>> On 07.03.18 at 19:58,  wrote:
> --- a/xen/include/asm-x86/msr-index.h
> +++ b/xen/include/asm-x86/msr-index.h
> @@ -312,18 +312,21 @@
>  
>  #define MSR_IA32_TSC_ADJUST  0x003b
>  
> -#define MSR_IA32_APICBASE0x001b
> -#define MSR_IA32_APICBASE_BSP(1<<8)
> -#define MSR_IA32_APICBASE_EXTD   (1<<10)
> -#define MSR_IA32_APICBASE_ENABLE (1<<11)
> -#define MSR_IA32_APICBASE_BASE   0x000ff000ul
> -#define MSR_IA32_APICBASE_MSR   0x800
> -#define MSR_IA32_APICTPR_MSR0x808
> -#define MSR_IA32_APICPPR_MSR0x80a
> -#define MSR_IA32_APICEOI_MSR0x80b
> -#define MSR_IA32_APICTMICT_MSR  0x838
> -#define MSR_IA32_APICTMCCT_MSR  0x839
> -#define MSR_IA32_APICSELF_MSR   0x83f
> +#define MSR_APIC_BASE   0x001b
> +#define APIC_BASE_BSP   (1<<8)
> +#define APIC_BASE_EXTD  (1<<10)
> +#define APIC_BASE_ENABLE(1<<11)
> +#define APIC_BASE_BASE  0x000ff000ul

This sounds a little clumsy; how about APIC_BASE_ADDR_MASK?

> +#define MSR_X2APIC_BASE 0x800
> +#define MSR_X2APIC_LAST 0xbff

With "LAST", perhaps also MSR_X2APIC_FIRST (even further
separating it from MSR_APIC_BASE)?

> +#define MSR_X2APIC_TPR  0x808
> +#define MSR_X2APIC_PPR  0x80a
> +#define MSR_X2APIC_EOI  0x80b
> +#define MSR_X2APIC_TMICT0x838
> +#define MSR_X2APIC_TMCCT0x839
> +#define MSR_X2APIC_SELF 0x83f

All surrounding MSR indexes have leading zeros spelled out; would
you mind doing so for the x2APIC ones as well?

I won't insist on any of these though, so with or without any or all
of them
Acked-by: Jan Beulich 

Jan


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

Re: [Xen-devel] [PATCH v2 3/5] x86: Fix APIC MSR constant names

2018-03-07 Thread Tian, Kevin 
> From: Andrew Cooper [mailto:andrew.coop...@citrix.com]
> Sent: Thursday, March 8, 2018 2:59 AM
> 
> We currently have MSR_IA32_APICBASE and MSR_IA32_APICBASE_MSR
> which are
> synonymous from a naming point of view, but refer to very different things.
> 
> Rename the x2APIC MSRs to MSR_X2APIC_*, which are shorter constants
> and
> visually separate the register function from the generic APIC name.  For the
> case ranges, introduce MSR_X2APIC_LAST, rather than relying on the
> knowledge
> that there are 0x3ff MSRs architecturally reserved for x2APIC functionality.
> 
> For functionality relating to the APIC_BASE MSR, use MSR_APIC_BASE for
> the MSR
> itself, but drop the MSR prefix from the other constants to shorten the
> names.
> In all cases, the fact that we are dealing with the APIC_BASE MSR is obvious
> from the context.
> 
> No functional change (the combined binary is identical).
> 
> Signed-off-by: Andrew Cooper 
> ---
> CC: Jan Beulich 
> CC: Roger Pau Monné 
> CC: Wei Liu 
> CC: Jun Nakajima 
> CC: Kevin Tian 
> 

Reviewed-by: Kevin Tian 
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 3/5] x86: Fix APIC MSR constant names

2018-03-07 Thread Konrad Rzeszutek Wilk 
On Wed, Mar 07, 2018 at 06:58:34PM +, Andrew Cooper wrote:
> We currently have MSR_IA32_APICBASE and MSR_IA32_APICBASE_MSR which are
> synonymous from a naming point of view, but refer to very different things.
> 
> Rename the x2APIC MSRs to MSR_X2APIC_*, which are shorter constants and
> visually separate the register function from the generic APIC name.  For the
> case ranges, introduce MSR_X2APIC_LAST, rather than relying on the knowledge
> that there are 0x3ff MSRs architecturally reserved for x2APIC functionality.
> 
> For functionality relating to the APIC_BASE MSR, use MSR_APIC_BASE for the MSR
> itself, but drop the MSR prefix from the other constants to shorten the names.
> In all cases, the fact that we are dealing with the APIC_BASE MSR is obvious
> from the context.
> 
> No functional change (the combined binary is identical).
> 
> Signed-off-by: Andrew Cooper 
Reviewed-by: Konrad Rzeszutek Wilk 

Thank you!

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

[Xen-devel] [PATCH v2 3/5] x86: Fix APIC MSR constant names

2018-03-07 Thread Andrew Cooper 
We currently have MSR_IA32_APICBASE and MSR_IA32_APICBASE_MSR which are
synonymous from a naming point of view, but refer to very different things.

Rename the x2APIC MSRs to MSR_X2APIC_*, which are shorter constants and
visually separate the register function from the generic APIC name.  For the
case ranges, introduce MSR_X2APIC_LAST, rather than relying on the knowledge
that there are 0x3ff MSRs architecturally reserved for x2APIC functionality.

For functionality relating to the APIC_BASE MSR, use MSR_APIC_BASE for the MSR
itself, but drop the MSR prefix from the other constants to shorten the names.
In all cases, the fact that we are dealing with the APIC_BASE MSR is obvious
from the context.

No functional change (the combined binary is identical).

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Roger Pau Monné 
CC: Wei Liu 
CC: Jun Nakajima 
CC: Kevin Tian 

v2:
 * New
---
 xen/arch/x86/apic.c  | 66 
 xen/arch/x86/genapic/x2apic.c|  4 +--
 xen/arch/x86/hvm/hvm.c   |  8 ++---
 xen/arch/x86/hvm/vlapic.c| 19 ++--
 xen/arch/x86/hvm/vmx/vmx.c   | 20 ++--
 xen/include/asm-x86/hvm/vlapic.h |  6 ++--
 xen/include/asm-x86/msr-index.h  | 27 
 7 files changed, 76 insertions(+), 74 deletions(-)

diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index ffa5a69..12e0c92 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -302,31 +302,31 @@ void disable_local_APIC(void)
 
 if (enabled_via_apicbase) {
 uint64_t msr_content;
-rdmsrl(MSR_IA32_APICBASE, msr_content);
-wrmsrl(MSR_IA32_APICBASE, msr_content &
-   ~(MSR_IA32_APICBASE_ENABLE|MSR_IA32_APICBASE_EXTD));
+rdmsrl(MSR_APIC_BASE, msr_content);
+wrmsrl(MSR_APIC_BASE, msr_content &
+   ~(APIC_BASE_ENABLE | APIC_BASE_EXTD));
 }
 
 if ( kexecing && (current_local_apic_mode() != apic_boot_mode) )
 {
 uint64_t msr_content;
-rdmsrl(MSR_IA32_APICBASE, msr_content);
-msr_content &= ~(MSR_IA32_APICBASE_ENABLE|MSR_IA32_APICBASE_EXTD);
-wrmsrl(MSR_IA32_APICBASE, msr_content);
+rdmsrl(MSR_APIC_BASE, msr_content);
+msr_content &= ~(APIC_BASE_ENABLE | APIC_BASE_EXTD);
+wrmsrl(MSR_APIC_BASE, msr_content);
 
 switch ( apic_boot_mode )
 {
 case APIC_MODE_DISABLED:
 break; /* Nothing to do - we did this above */
 case APIC_MODE_XAPIC:
-msr_content |= MSR_IA32_APICBASE_ENABLE;
-wrmsrl(MSR_IA32_APICBASE, msr_content);
+msr_content |= APIC_BASE_ENABLE;
+wrmsrl(MSR_APIC_BASE, msr_content);
 break;
 case APIC_MODE_X2APIC:
-msr_content |= MSR_IA32_APICBASE_ENABLE;
-wrmsrl(MSR_IA32_APICBASE, msr_content);
-msr_content |= MSR_IA32_APICBASE_EXTD;
-wrmsrl(MSR_IA32_APICBASE, msr_content);
+msr_content |= APIC_BASE_ENABLE;
+wrmsrl(MSR_APIC_BASE, msr_content);
+msr_content |= APIC_BASE_EXTD;
+wrmsrl(MSR_APIC_BASE, msr_content);
 break;
 default:
 printk("Default case when reverting #%d lapic to boot state\n",
@@ -478,12 +478,12 @@ static void __enable_x2apic(void)
 {
 uint64_t msr_content;
 
-rdmsrl(MSR_IA32_APICBASE, msr_content);
-if ( !(msr_content & MSR_IA32_APICBASE_EXTD) )
+rdmsrl(MSR_APIC_BASE, msr_content);
+if ( !(msr_content & APIC_BASE_EXTD) )
 {
-msr_content |= MSR_IA32_APICBASE_ENABLE | MSR_IA32_APICBASE_EXTD;
+msr_content |= APIC_BASE_ENABLE | APIC_BASE_EXTD;
 msr_content = (uint32_t)msr_content;
-wrmsrl(MSR_IA32_APICBASE, msr_content);
+wrmsrl(MSR_APIC_BASE, msr_content);
 }
 }
 
@@ -743,10 +743,10 @@ int lapic_resume(void)
  */
 if ( !x2apic_enabled )
 {
-rdmsrl(MSR_IA32_APICBASE, msr_content);
-msr_content &= ~MSR_IA32_APICBASE_BASE;
-wrmsrl(MSR_IA32_APICBASE,
-msr_content | MSR_IA32_APICBASE_ENABLE | mp_lapic_addr);
+rdmsrl(MSR_APIC_BASE, msr_content);
+msr_content &= ~APIC_BASE_BASE;
+wrmsrl(MSR_APIC_BASE,
+   msr_content | APIC_BASE_ENABLE | mp_lapic_addr);
 }
 else
 resume_x2apic();
@@ -817,7 +817,8 @@ static int __init detect_init_APIC (void)
 if (enable_local_apic < 0)
 return -1;
 
-if (rdmsr_safe(MSR_IA32_APICBASE, msr_content)) {
+if ( rdmsr_safe(MSR_APIC_BASE, msr_content) )
+{
 printk("No local APIC present\n");
 return -1;
 }
@@ -838,11 +839,12 @@ static int __init detect_init_APIC (void)
  * software for Intel P6 or later and AMD K7
  * (Model > 1) or later.
  */
-if (!(msr_content &