Re: [PATCH] perf/x86/intel/uncore: Enable uncore on vCPUs when using uncore discovery

Re: [PATCH] perf/x86/intel/uncore: Enable uncore on vCPUs when using uncore discovery

[Liang, Kan]

On 2024-09-23 10:47 p.m., 陈培鸿(乘鸿) wrote:
> With uncore discovery, kvm can choose to expose a subset of
> uncore related MSRs it wants to guest by emulate the uncore
> discovery device. 

I don't hear that the KVM has started to support uncore vPMU.
Can you please point me to patches?

The default of uncore_no_discover is 0. So it bypasses the HYPERVISOR
check unless the user specially sets the value. It could be a problem
for the earlier platforms which don't support discovery
table. How do you plan to emulate the devices on earlier platforms?

Thanks,
Kan
> So we can enable uncore on virtualized CPUs
> when uncore discovery is using.
> Signed-off-by: Cheng Hong <chenpeihong.cph@alibaba-inc.com>
> ---
>  arch/x86/events/intel/uncore.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
> index d98fac567684..33776df95aa4 100644
> --- a/arch/x86/events/intel/uncore.c
> +++ b/arch/x86/events/intel/uncore.c
> @@ -1920,7 +1920,7 @@ static int __init intel_uncore_init(void)
>  struct intel_uncore_init_fun *uncore_init;
>  int pret = 0, cret = 0, mret = 0, ret;
> - if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
> + if (uncore_no_discover && boot_cpu_has(X86_FEATURE_HYPERVISOR))
>  return -ENODEV;
>  __uncore_max_dies =

Re: 回复：[PATCH] perf/x86/intel/uncore: Enable uncore on vCPUs when using uncore discovery

[Liang, Kan]

On 2024-09-25 4:22 a.m., 陈培鸿(乘鸿) wrote:
>>> With uncore discovery, kvm can choose to expose a subset of
>>> uncore related MSRs it wants to guest by emulate the uncore
>>> discovery device. 
>>
>> I don't hear that the KVM has started to support uncore vPMU.
>> Can you please point me to patches?
> There are no such uncore vPMU related patches so far, which may
> be supported some day in future. I’m now working on this.

I think the patch should be part of the future KVM patch set.
Otherwise, It seems like a security hole because of the lack of
underlying support.

Thanks,
Kan

>> The default of uncore_no_discover is 0. So it bypasses the HYPERVISOR
>> check unless the user specially sets the value. It could be a problem
>> for the earlier platforms which don't support discovery
>> table. How do you plan to emulate the devices on earlier platforms?
>>
> U R right, I should make a more strict check here.
> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
> index 33776df95aa4..ca510c476895 100644
> --- a/arch/x86/events/intel/uncore.c
> +++ b/arch/x86/events/intel/uncore.c
> @@ -1919,8 +1919,9 @@ static int __init intel_uncore_init(void)
>  const struct x86_cpu_id *id;
>  struct intel_uncore_init_fun *uncore_init;
>  int pret = 0, cret = 0, mret = 0, ret;
> + bool in_guest = boot_cpu_has(X86_FEATURE_HYPERVISOR);
> - if (uncore_no_discover && boot_cpu_has(X86_FEATURE_HYPERVISOR))
> + if (uncore_no_discover && in_guest)
>  return -ENODEV;
>  __uncore_max_dies =
> @@ -1936,8 +1937,10 @@ static int __init intel_uncore_init(void)
>  uncore_init = (struct intel_uncore_init_fun *)id->driver_data;
>  if (uncore_no_discover && uncore_init->use_discovery)
>  return -ENODEV;
> - if (uncore_init->use_discovery &&
> - !intel_uncore_has_discovery_tables(uncore_init->uncore_units_ignore))
> + if (!uncore_init->use_discovery) {
> + if (in_guest)
> + return -ENODEV;
> + } else if (!intel_uncore_has_discovery_tables(uncore_init->uncore_units_ignore))
>  return -ENODEV;
>  }
> For the earlier platforms which don't support discovery table, just
> disable uncore for guests. Will there be any issues?
>> Thanks,
>> Kan
>>> So we can enable uncore on virtualized CPUs
>>> when uncore discovery is using.
>>> Signed-off-by: Cheng Hong <chenpeihong.cph@alibaba-inc.com>
>>> —
>>> arch/x86/events/intel/uncore.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
>>> index d98fac567684..33776df95aa4 100644
>>> --- a/arch/x86/events/intel/uncore.c
>>> +++ b/arch/x86/events/intel/uncore.c
>>> @@ -1920,7 +1920,7 @@ static int __init intel_uncore_init(void)
>>> struct intel_uncore_init_fun *uncore_init;
>>> int pret = 0, cret = 0, mret = 0, ret;
>>> - if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
>>> + if (uncore_no_discover && boot_cpu_has(X86_FEATURE_HYPERVISOR))
>>> return -ENODEV;
>>> __uncore_max_dies =
> Thanks,
> Chen

Re: 回复：回复：[PATCH] perf/x86/intel/uncore: Enable uncore on vCPUs when using uncore discovery

[Liang, Kan]

On 2024-09-25 10:07 p.m., 陈培鸿(乘鸿) wrote:
>>>>> With uncore discovery, kvm can choose to expose a subset of
>>>>> uncore related MSRs it wants to guest by emulate the uncore
>>>>> discovery device. 
>>>>
>>>> I don't hear that the KVM has started to support uncore vPMU.
>>>> Can you please point me to patches?
>>> There are no such uncore vPMU related patches so far, which may
>>> be supported some day in future. I’m now working on this.
>>
>> I think the patch should be part of the future KVM patch set.
>> Otherwise, It seems like a security hole because of the lack of
>> underlying support.
> I think this patch and the upcomming kvm patch set address two
> different issues. This patch enables uncore vPMU on hypervisors 
> with uncore discovery emulated, not limited just to QEMU/KVM.
> While it's true that the current QEMU/KVM setup lacks uncore vPMU
> support, it does not represent a security vulnerability. Instead,
> it simply allows guests on platforms utilizing uncore discovery,
> e.g., SPR/EMR/GNR, to access uncore capabilities via the emulated
> uncore discovery device. If there is no such device present, the
> uncore module remains inactive.

That's an ideal case. There is no mechanism to prevent a broken emulated
uncore discovery device is created, right?

You ask to expose some capabilities, but there is no real user for now.
I don't see a reason why we want to do it.

Thanks,
Kan
> Thanks,
> Chen
>>
>> Thanks,
>> Kan
>>
>>>> The default of uncore_no_discover is 0. So it bypasses the HYPERVISOR
>>>> check unless the user specially sets the value. It could be a problem
>>>> for the earlier platforms which don't support discovery
>>>> table. How do you plan to emulate the devices on earlier platforms?
>>>>
>>> U R right, I should make a more strict check here.
>>> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
>>> index 33776df95aa4..ca510c476895 100644
>>> --- a/arch/x86/events/intel/uncore.c
>>> +++ b/arch/x86/events/intel/uncore.c
>>> @@ -1919,8 +1919,9 @@ static int __init intel_uncore_init(void)
>>> const struct x86_cpu_id *id;
>>> struct intel_uncore_init_fun *uncore_init;
>>> int pret = 0, cret = 0, mret = 0, ret;
>>> + bool in_guest = boot_cpu_has(X86_FEATURE_HYPERVISOR);
>>> - if (uncore_no_discover && boot_cpu_has(X86_FEATURE_HYPERVISOR))
>>> + if (uncore_no_discover && in_guest)
>>> return -ENODEV;
>>> __uncore_max_dies =
>>> @@ -1936,8 +1937,10 @@ static int __init intel_uncore_init(void)
>>> uncore_init = (struct intel_uncore_init_fun *)id->driver_data;
>>> if (uncore_no_discover && uncore_init->use_discovery)
>>> return -ENODEV;
>>> - if (uncore_init->use_discovery &&
>>> - !intel_uncore_has_discovery_tables(uncore_init->uncore_units_ignore))
>>> + if (!uncore_init->use_discovery) {
>>> + if (in_guest)
>>> + return -ENODEV;
>>> + } else if (!intel_uncore_has_discovery_tables(uncore_init->uncore_units_ignore))
>>> return -ENODEV;
>>> }
>>> For the earlier platforms which don't support discovery table, just
>>> disable uncore for guests. Will there be any issues?
>>>> Thanks,
>>>> Kan
>>>>> So we can enable uncore on virtualized CPUs
>>>>> when uncore discovery is using.
>>>>> Signed-off-by: Cheng Hong <chenpeihong.cph@alibaba-inc.com>
>>>>> —
>>>>> arch/x86/events/intel/uncore.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
>>>>> index d98fac567684..33776df95aa4 100644
>>>>> --- a/arch/x86/events/intel/uncore.c
>>>>> +++ b/arch/x86/events/intel/uncore.c
>>>>> @@ -1920,7 +1920,7 @@ static int __init intel_uncore_init(void)
>>>>> struct intel_uncore_init_fun *uncore_init;
>>>>> int pret = 0, cret = 0, mret = 0, ret;
>>>>> - if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
>>>>> + if (uncore_no_discover && boot_cpu_has(X86_FEATURE_HYPERVISOR))
>>>>> return -ENODEV;
>>>>> __uncore_max_dies =
>>> Thanks,
>>> Chen