diff mbox series

[v3,03/14] x86/cpu/vpmu: Add Hygon Dhyana and AMD Zen support for vPMU

Message ID babf13f907b9b85afe40cdffce4ee2cf17f67f11.1553520193.git.puwen@hygon.cn (mailing list archive)
State Superseded
Headers show
Series Add support for Hygon Dhyana Family 18h processor | expand

Commit Message

Wen Pu March 25, 2019, 1:30 p.m. UTC
As Hygon Dhyana CPU share similar PMU architecture with AMD family
17h one, so add Hygon Dhyana support in vpmu_arch_initialise() and
vpmu_init() by sharing AMD code path.

Split the common part in amd_vpmu_init() to a static function
_vpmu_init(), making AMD and Hygon to call the shared function to
initialize vPMU.

As current vPMU still not support Zen(family 17h), add 0x17 support
to amd_vpmu_init().

Also create a function hygon_vpmu_init() for Hygon vPMU initialization.

Both of AMD 17h and Hygon 18h have the same performance event select
and counter MSRs as AMD 15h has, so reuse the 15h definitions for them.

Signed-off-by: Pu Wen <puwen@hygon.cn>
---
 xen/arch/x86/cpu/vpmu.c     |  5 ++++
 xen/arch/x86/cpu/vpmu_amd.c | 57 ++++++++++++++++++++++++++++++++-------------
 xen/include/asm-x86/vpmu.h  |  1 +
 3 files changed, 47 insertions(+), 16 deletions(-)

Comments

Jan Beulich March 26, 2019, 4:10 p.m. UTC | #1
>>> On 25.03.19 at 14:30, <puwen@hygon.cn> wrote:
> --- a/xen/arch/x86/cpu/vpmu_amd.c
> +++ b/xen/arch/x86/cpu/vpmu_amd.c
> @@ -538,13 +538,37 @@ int svm_vpmu_initialise(struct vcpu *v)
>      return 0;
>  }
>  
> -int __init amd_vpmu_init(void)
> +static int _vpmu_init(void)

Despite it having been me (I think) to have suggested this as
a possible name, now that I see it in use I don't think it's a
good choice: We're in vPMU code anyway, so the vpmu_
prefix is pretty pointless. Simply init() would be too short and
generic for my taste, so how about common_init() or
shared_init()?

> @@ -565,24 +589,25 @@ int __init amd_vpmu_init(void)
>          return -EINVAL;
>      }
>  
> -    if ( sizeof(struct xen_pmu_data) +
> -         2 * sizeof(uint64_t) * num_counters > PAGE_SIZE )
> -    {
> -        printk(XENLOG_WARNING
> -               "VPMU: Register bank does not fit into VPMU shared page\n");
> -        counters = ctrls = NULL;
> -        num_counters = 0;
> -        return -ENOSPC;
> -    }
> +    return _vpmu_init();
> +}
>  
> -    for ( i = 0; i < num_counters; i++ )
> +int __init hygon_vpmu_init(void)
> +{
> +    switch ( current_cpu_data.x86 )
>      {
> -        rdmsrl(ctrls[i], ctrl_rsvd[i]);
> -        ctrl_rsvd[i] &= CTRL_RSVD_MASK;
> +    case 0x18:
> +        num_counters = F15H_NUM_COUNTERS;
> +        counters = AMD_F15H_COUNTERS;
> +        ctrls = AMD_F15H_CTRLS;
> +        k7_counters_mirrored = 1;
> +        break;
> +    default:
> +        printk(XENLOG_WARNING "VPMU: Unsupported CPU family %#x\n",
> +               current_cpu_data.x86);
> +        return -EINVAL;
>      }

While I'm not going to insist in cases where you add to existing
switch()-es which lack such blank lines, please add a blank line
between the case blocks here. Yet then again I wonder whether
the default case wouldn't better move into the shared function
as well, keying off of e.g. num_counters still being zero.

Jan
Wen Pu March 27, 2019, 8:16 a.m. UTC | #2
On 2019/3/27 0:10, Jan Beulich wrote:
> On 25.03.19 at 14:30, <puwen@hygon.cn> wrote:
>> --- a/xen/arch/x86/cpu/vpmu_amd.c
>> +++ b/xen/arch/x86/cpu/vpmu_amd.c
>> @@ -538,13 +538,37 @@ int svm_vpmu_initialise(struct vcpu *v)
>>       return 0;
>>   }
>>   
>> -int __init amd_vpmu_init(void)
>> +static int _vpmu_init(void)
> 
> Despite it having been me (I think) to have suggested this as
> a possible name, now that I see it in use I don't think it's a
> good choice: We're in vPMU code anyway, so the vpmu_
> prefix is pretty pointless. Simply init() would be too short and
> generic for my taste, so how about common_init() or
> shared_init()?

I prefer common_init() here.

>> -    for ( i = 0; i < num_counters; i++ )
>> +int __init hygon_vpmu_init(void)
>> +{
>> +    switch ( current_cpu_data.x86 )
>>       {
>> -        rdmsrl(ctrls[i], ctrl_rsvd[i]);
>> -        ctrl_rsvd[i] &= CTRL_RSVD_MASK;
>> +    case 0x18:
>> +        num_counters = F15H_NUM_COUNTERS;
>> +        counters = AMD_F15H_COUNTERS;
>> +        ctrls = AMD_F15H_CTRLS;
>> +        k7_counters_mirrored = 1;
>> +        break;
>> +    default:
>> +        printk(XENLOG_WARNING "VPMU: Unsupported CPU family %#x\n",
>> +               current_cpu_data.x86);
>> +        return -EINVAL;
>>       }
> 
> While I'm not going to insist in cases where you add to existing
> switch()-es which lack such blank lines, please add a blank line
> between the case blocks here. Yet then again I wonder whether
> the default case wouldn't better move into the shared function
> as well, keying off of e.g. num_counters still being zero.

I think it's a good idea to move the default case into the shared
function, which would like:
static int common_init(void)
{
     unsigned int i;

     if (!num_counters) {
         printk(XENLOG_WARNING "VPMU: Unsupported CPU family %#x\n",
                current_cpu_data.x86);
         return -EINVAL;
     }
...

Then as there is only one case in hygon_vpmu_init(), how about remove
switch()-es in this function?
Jan Beulich March 27, 2019, 8:37 a.m. UTC | #3
>>> On 27.03.19 at 09:16, <puwen@hygon.cn> wrote:
> On 2019/3/27 0:10, Jan Beulich wrote:
>> On 25.03.19 at 14:30, <puwen@hygon.cn> wrote:
>>> -    for ( i = 0; i < num_counters; i++ )
>>> +int __init hygon_vpmu_init(void)
>>> +{
>>> +    switch ( current_cpu_data.x86 )
>>>       {
>>> -        rdmsrl(ctrls[i], ctrl_rsvd[i]);
>>> -        ctrl_rsvd[i] &= CTRL_RSVD_MASK;
>>> +    case 0x18:
>>> +        num_counters = F15H_NUM_COUNTERS;
>>> +        counters = AMD_F15H_COUNTERS;
>>> +        ctrls = AMD_F15H_CTRLS;
>>> +        k7_counters_mirrored = 1;
>>> +        break;
>>> +    default:
>>> +        printk(XENLOG_WARNING "VPMU: Unsupported CPU family %#x\n",
>>> +               current_cpu_data.x86);
>>> +        return -EINVAL;
>>>       }
>> 
>> While I'm not going to insist in cases where you add to existing
>> switch()-es which lack such blank lines, please add a blank line
>> between the case blocks here. Yet then again I wonder whether
>> the default case wouldn't better move into the shared function
>> as well, keying off of e.g. num_counters still being zero.
> 
> I think it's a good idea to move the default case into the shared
> function, which would like:
> static int common_init(void)
> {
>      unsigned int i;
> 
>      if (!num_counters) {
>          printk(XENLOG_WARNING "VPMU: Unsupported CPU family %#x\n",
>                 current_cpu_data.x86);
>          return -EINVAL;
>      }
> ...
> 
> Then as there is only one case in hygon_vpmu_init(), how about remove
> switch()-es in this function?

Well, personally I'd prefer to keep the switch(), as that what's
going to be needed once you introduce a second model, but I
won't insist.

Jan
Wen Pu March 27, 2019, 10:08 a.m. UTC | #4
On 2019/3/27 16:38, Jan Beulich wrote:
> On 27.03.19 at 09:16, <puwen@hygon.cn> wrote:
>> On 2019/3/27 0:10, Jan Beulich wrote:
>>> On 25.03.19 at 14:30, <puwen@hygon.cn> wrote:
>>>> +    default:
>>>> +        printk(XENLOG_WARNING "VPMU: Unsupported CPU family %#x\n",
>>>> +               current_cpu_data.x86);
>>>> +        return -EINVAL;
>>>>        }
>>>
>>> While I'm not going to insist in cases where you add to existing
>>> switch()-es which lack such blank lines, please add a blank line
>>> between the case blocks here. Yet then again I wonder whether
>>> the default case wouldn't better move into the shared function
>>> as well, keying off of e.g. num_counters still being zero.
>>
>> Then as there is only one case in hygon_vpmu_init(), how about remove
>> switch()-es in this function?
> 
> Well, personally I'd prefer to keep the switch(), as that what's
> going to be needed once you introduce a second model, but I
> won't insist.

Keeping the switch() is also fine to me.
diff mbox series

Patch

diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c
index 8f6daf1..93a27d8 100644
--- a/xen/arch/x86/cpu/vpmu.c
+++ b/xen/arch/x86/cpu/vpmu.c
@@ -456,6 +456,7 @@  static int vpmu_arch_initialise(struct vcpu *v)
     switch ( vendor )
     {
     case X86_VENDOR_AMD:
+    case X86_VENDOR_HYGON:
         ret = svm_vpmu_initialise(v);
         break;
 
@@ -876,6 +877,10 @@  static int __init vpmu_init(void)
         if ( amd_vpmu_init() )
            vpmu_mode = XENPMU_MODE_OFF;
         break;
+    case X86_VENDOR_HYGON:
+        if ( hygon_vpmu_init() )
+           vpmu_mode = XENPMU_MODE_OFF;
+        break;
     case X86_VENDOR_INTEL:
         if ( core2_vpmu_init() )
            vpmu_mode = XENPMU_MODE_OFF;
diff --git a/xen/arch/x86/cpu/vpmu_amd.c b/xen/arch/x86/cpu/vpmu_amd.c
index 5efc39b..3fc955f 100644
--- a/xen/arch/x86/cpu/vpmu_amd.c
+++ b/xen/arch/x86/cpu/vpmu_amd.c
@@ -538,13 +538,37 @@  int svm_vpmu_initialise(struct vcpu *v)
     return 0;
 }
 
-int __init amd_vpmu_init(void)
+static int _vpmu_init(void)
 {
     unsigned int i;
 
+    if ( sizeof(struct xen_pmu_data) +
+         2 * sizeof(uint64_t) * num_counters > PAGE_SIZE )
+    {
+        printk(XENLOG_WARNING
+               "VPMU: Register bank does not fit into VPMU shared page\n");
+        counters = ctrls = NULL;
+        num_counters = 0;
+        return -ENOSPC;
+    }
+
+    for ( i = 0; i < num_counters; i++ )
+    {
+        rdmsrl(ctrls[i], ctrl_rsvd[i]);
+        ctrl_rsvd[i] &= CTRL_RSVD_MASK;
+    }
+
+    regs_sz = 2 * sizeof(uint64_t) * num_counters;
+
+    return 0;
+}
+
+int __init amd_vpmu_init(void)
+{
     switch ( current_cpu_data.x86 )
     {
     case 0x15:
+    case 0x17:
         num_counters = F15H_NUM_COUNTERS;
         counters = AMD_F15H_COUNTERS;
         ctrls = AMD_F15H_CTRLS;
@@ -565,24 +589,25 @@  int __init amd_vpmu_init(void)
         return -EINVAL;
     }
 
-    if ( sizeof(struct xen_pmu_data) +
-         2 * sizeof(uint64_t) * num_counters > PAGE_SIZE )
-    {
-        printk(XENLOG_WARNING
-               "VPMU: Register bank does not fit into VPMU shared page\n");
-        counters = ctrls = NULL;
-        num_counters = 0;
-        return -ENOSPC;
-    }
+    return _vpmu_init();
+}
 
-    for ( i = 0; i < num_counters; i++ )
+int __init hygon_vpmu_init(void)
+{
+    switch ( current_cpu_data.x86 )
     {
-        rdmsrl(ctrls[i], ctrl_rsvd[i]);
-        ctrl_rsvd[i] &= CTRL_RSVD_MASK;
+    case 0x18:
+        num_counters = F15H_NUM_COUNTERS;
+        counters = AMD_F15H_COUNTERS;
+        ctrls = AMD_F15H_CTRLS;
+        k7_counters_mirrored = 1;
+        break;
+    default:
+        printk(XENLOG_WARNING "VPMU: Unsupported CPU family %#x\n",
+               current_cpu_data.x86);
+        return -EINVAL;
     }
 
-    regs_sz = 2 * sizeof(uint64_t) * num_counters;
-
-    return 0;
+    return _vpmu_init();
 }
 
diff --git a/xen/include/asm-x86/vpmu.h b/xen/include/asm-x86/vpmu.h
index 1287b9f..55f85ba 100644
--- a/xen/include/asm-x86/vpmu.h
+++ b/xen/include/asm-x86/vpmu.h
@@ -52,6 +52,7 @@  struct arch_vpmu_ops {
 int core2_vpmu_init(void);
 int vmx_vpmu_initialise(struct vcpu *);
 int amd_vpmu_init(void);
+int hygon_vpmu_init(void);
 int svm_vpmu_initialise(struct vcpu *);
 
 struct vpmu_struct {