[PATCH] target/i386/cpu: disable PERFCORE for AMD when cpu.pmu is off

[PATCH] target/i386/cpu: disable PERFCORE for AMD when cpu.pmu is off

[Liang Yan]

With cpu.pmu=off, perfctr_core could still be seen in an AMD guest cpuid.
By further digging, I found cpu.perfctr_core did the trick. However,
considering the 'enable_pmu' in KVM could work on both Intel and AMD,
we may add AMD PMU control under 'enabe_pmu' in QEMU too.

This change will overide the property 'perfctr_ctr' and change the AMD PMU
to off by default.

Signed-off-by: Liang Yan <lyan@digtalocean.com>
---
 target/i386/cpu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 22b681ca37..edf5413c90 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5706,6 +5706,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                 *ecx |= 1 << 1;    /* CmpLegacy bit */
             }
         }
+
+        if (!cpu->enable_pmu) {
+            *ecx &= ~CPUID_EXT3_PERFCORE;
+        }
         break;
     case 0x80000002:
     case 0x80000003:
-- 
2.34.1

Re: [PATCH] target/i386/cpu: disable PERFCORE for AMD when cpu.pmu is off

[Vitaly Kuznetsov]

Liang Yan <lyan@digitalocean.com> writes:

> With cpu.pmu=off, perfctr_core could still be seen in an AMD guest cpuid.
> By further digging, I found cpu.perfctr_core did the trick. However,
> considering the 'enable_pmu' in KVM could work on both Intel and AMD,
> we may add AMD PMU control under 'enabe_pmu' in QEMU too.
>
> This change will overide the property 'perfctr_ctr' and change the AMD PMU
> to off by default.
>
> Signed-off-by: Liang Yan <lyan@digtalocean.com>
> ---
>  target/i386/cpu.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 22b681ca37..edf5413c90 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -5706,6 +5706,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>                  *ecx |= 1 << 1;    /* CmpLegacy bit */
>              }
>          }
> +
> +        if (!cpu->enable_pmu) {
> +            *ecx &= ~CPUID_EXT3_PERFCORE;
> +        }
>          break;
>      case 0x80000002:
>      case 0x80000003:

I may be missing something but my first impression is that this will
make CPUID_EXT3_PERFCORE bit disappear when a !enable_pmu VM is migrated
from an old QEMU (pre-patch) to a new one. If so, then additional
precautions should be taking against that (e.g. tying the change to
CPU/machine model versions, for example).

-- 
Vitaly

Re: [PATCH] target/i386/cpu: disable PERFCORE for AMD when cpu.pmu is off

[Liang Yan]

Hey Vitaly,

On 10/31/22 6:07 AM, Vitaly Kuznetsov wrote:
> Liang Yan <lyan@digitalocean.com> writes:
>
>> With cpu.pmu=off, perfctr_core could still be seen in an AMD guest cpuid.
>> By further digging, I found cpu.perfctr_core did the trick. However,
>> considering the 'enable_pmu' in KVM could work on both Intel and AMD,
>> we may add AMD PMU control under 'enabe_pmu' in QEMU too.
>>
>> This change will overide the property 'perfctr_ctr' and change the AMD PMU
>> to off by default.
>>
>> Signed-off-by: Liang Yan <lyan@digtalocean.com>
>> ---
>>   target/i386/cpu.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index 22b681ca37..edf5413c90 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -5706,6 +5706,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>>                   *ecx |= 1 << 1;    /* CmpLegacy bit */
>>               }
>>           }
>> +
>> +        if (!cpu->enable_pmu) {
>> +            *ecx &= ~CPUID_EXT3_PERFCORE;
>> +        }
>>           break;
>>       case 0x80000002:
>>       case 0x80000003:
> I may be missing something but my first impression is that this will
> make CPUID_EXT3_PERFCORE bit disappear when a !enable_pmu VM is migrated
> from an old QEMU (pre-patch) to a new one. If so, then additional
> precautions should be taking against that (e.g. tying the change to
> CPU/machine model versions, for example).
>
Thanks for the reply, it is a quite good point. I was struggled with it 
a little bit earlier because cpu.pmu has such operation for Intel CPU. 
After further talk with AMD people, I noticed that AMD PMU is more than 
perfctr_core, it has some legacy counters in use. I will dig a little 
further and send a v2 with extra cpu counters and live migration 
compatibility.


Regards,

Liang