[v3,11/16] powerpc/pseries/svm: Export guest SVM status to user space via sysfs

From: Ryan Grimm <grimm@linux.vnet.ibm.com>

User space might want to know it's running in a secure VM.  It can't do
a mfmsr because mfmsr is a privileged instruction.

The solution here is to create a cpu attribute:

/sys/devices/system/cpu/svm

which will read 0 or 1 based on the S bit of the guest's CPU 0.

Signed-off-by: Ryan Grimm <grimm@linux.vnet.ibm.com>
Reviewed-by: Ram Pai <linuxram@us.ibm.com>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
---
 arch/powerpc/kernel/sysfs.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

Thiago Jung Bauermann <bauerman@linux.ibm.com> writes:
> From: Ryan Grimm <grimm@linux.vnet.ibm.com>
>
> User space might want to know it's running in a secure VM.  It can't do
> a mfmsr because mfmsr is a privileged instruction.
>
> The solution here is to create a cpu attribute:
>
> /sys/devices/system/cpu/svm
>
> which will read 0 or 1 based on the S bit of the guest's CPU 0.

Why CPU 0?

If we have different CPUs running with different MSR_S then something
has gone badly wrong, no?

So can't we just read the MSR on whatever CPU the sysfs code happens to
run on.

cheers

> diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
> index e2147d7c9e72..f7100ab77d29 100644
> --- a/arch/powerpc/kernel/sysfs.c
> +++ b/arch/powerpc/kernel/sysfs.c
> @@ -19,6 +19,7 @@
>  #include <asm/smp.h>
>  #include <asm/pmc.h>
>  #include <asm/firmware.h>
> +#include <asm/svm.h>
>  
>  #include "cacheinfo.h"
>  #include "setup.h"
> @@ -715,6 +716,32 @@ static struct device_attribute pa6t_attrs[] = {
>  #endif /* HAS_PPC_PMC_PA6T */
>  #endif /* HAS_PPC_PMC_CLASSIC */
>  
> +#ifdef CONFIG_PPC_SVM
> +static void get_svm(void *val)
> +{
> +	u32 *value = val;
> +
> +	*value = is_secure_guest();
> +}
> +
> +static ssize_t show_svm(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	u32 val;
> +	smp_call_function_single(0, get_svm, &val, 1);
> +	return sprintf(buf, "%u\n", val);
> +}
> +static DEVICE_ATTR(svm, 0444, show_svm, NULL);
> +
> +static void create_svm_file(void)
> +{
> +	device_create_file(cpu_subsys.dev_root, &dev_attr_svm);
> +}
> +#else
> +static void create_svm_file(void)
> +{
> +}
> +#endif /* CONFIG_PPC_SVM */
> +
>  static int register_cpu_online(unsigned int cpu)
>  {
>  	struct cpu *c = &per_cpu(cpu_devices, cpu);
> @@ -1058,6 +1085,8 @@ static int __init topology_init(void)
>  	sysfs_create_dscr_default();
>  #endif /* CONFIG_PPC64 */
>  
> +	create_svm_file();
> +
>  	return 0;
>  }
>  subsys_initcall(topology_init);

Michael Ellerman <mpe@ellerman.id.au> writes:

> Thiago Jung Bauermann <bauerman@linux.ibm.com> writes:
>> From: Ryan Grimm <grimm@linux.vnet.ibm.com>
>>
>> User space might want to know it's running in a secure VM.  It can't do
>> a mfmsr because mfmsr is a privileged instruction.
>>
>> The solution here is to create a cpu attribute:
>>
>> /sys/devices/system/cpu/svm
>>
>> which will read 0 or 1 based on the S bit of the guest's CPU 0.
>
> Why CPU 0?
>
> If we have different CPUs running with different MSR_S then something
> has gone badly wrong, no?

Yes, that would be very bad.

> So can't we just read the MSR on whatever CPU the sysfs code happens to
> run on.

Good point. I made the change in the patch below.

Thiago Jung Bauermann <bauerman@linux.ibm.com> writes:
> Michael Ellerman <mpe@ellerman.id.au> writes:
>> Thiago Jung Bauermann <bauerman@linux.ibm.com> writes:
>>> From: Ryan Grimm <grimm@linux.vnet.ibm.com>
>>> User space might want to know it's running in a secure VM.  It can't do
>>> a mfmsr because mfmsr is a privileged instruction.
>>>
>>> The solution here is to create a cpu attribute:
>>>
>>> /sys/devices/system/cpu/svm
>>>
>>> which will read 0 or 1 based on the S bit of the guest's CPU 0.
>>
>> Why CPU 0?
>>
>> If we have different CPUs running with different MSR_S then something
>> has gone badly wrong, no?
>
> Yes, that would be very bad.
>
>> So can't we just read the MSR on whatever CPU the sysfs code happens to
>> run on.
>
> Good point. I made the change in the patch below.

The patch looks good. Although, it raises the question of whether it
should be an attribute of the CPU at all.

I guess there's not obviously anywhere better for it.

Still you should document the attribute in Documentation/ABI/testing/sysfs-devices-system-cpu

cheers

> From 2d951305e118bf286f8e83cbf396448085186357 Mon Sep 17 00:00:00 2001
> From: Ryan Grimm <grimm@linux.vnet.ibm.com>
> Date: Tue, 15 Jan 2019 11:56:29 -0600
> Subject: [PATCH] powerpc/pseries/svm: Export guest SVM status to user space
>  via sysfs
>
> User space might want to know it's running in a secure VM.  It can't do
> a mfmsr because mfmsr is a privileged instruction.
>
> The solution here is to create a cpu attribute:
>
> /sys/devices/system/cpu/svm
>
> which will read 0 or 1 based on the S bit of the current CPU.
>
> Signed-off-by: Ryan Grimm <grimm@linux.vnet.ibm.com>
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> ---
>  arch/powerpc/kernel/sysfs.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
> index e2147d7c9e72..80a676da11cb 100644
> --- a/arch/powerpc/kernel/sysfs.c
> +++ b/arch/powerpc/kernel/sysfs.c
> @@ -19,6 +19,7 @@
>  #include <asm/smp.h>
>  #include <asm/pmc.h>
>  #include <asm/firmware.h>
> +#include <asm/svm.h>
>  
>  #include "cacheinfo.h"
>  #include "setup.h"
> @@ -715,6 +716,23 @@ static struct device_attribute pa6t_attrs[] = {
>  #endif /* HAS_PPC_PMC_PA6T */
>  #endif /* HAS_PPC_PMC_CLASSIC */
>  
> +#ifdef CONFIG_PPC_SVM
> +static ssize_t show_svm(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "%u\n", is_secure_guest());
> +}
> +static DEVICE_ATTR(svm, 0444, show_svm, NULL);
> +
> +static void create_svm_file(void)
> +{
> +	device_create_file(cpu_subsys.dev_root, &dev_attr_svm);
> +}
> +#else
> +static void create_svm_file(void)
> +{
> +}
> +#endif /* CONFIG_PPC_SVM */
> +
>  static int register_cpu_online(unsigned int cpu)
>  {
>  	struct cpu *c = &per_cpu(cpu_devices, cpu);
> @@ -1058,6 +1076,8 @@ static int __init topology_init(void)
>  	sysfs_create_dscr_default();
>  #endif /* CONFIG_PPC64 */
>  
> +	create_svm_file();
> +
>  	return 0;
>  }
>  subsys_initcall(topology_init);

Michael Ellerman <mpe@ellerman.id.au> writes:

> Thiago Jung Bauermann <bauerman@linux.ibm.com> writes:
>> Michael Ellerman <mpe@ellerman.id.au> writes:
>>> Thiago Jung Bauermann <bauerman@linux.ibm.com> writes:
>>>> From: Ryan Grimm <grimm@linux.vnet.ibm.com>
>>>> User space might want to know it's running in a secure VM.  It can't do
>>>> a mfmsr because mfmsr is a privileged instruction.
>>>>
>>>> The solution here is to create a cpu attribute:
>>>>
>>>> /sys/devices/system/cpu/svm
>>>>
>>>> which will read 0 or 1 based on the S bit of the guest's CPU 0.
>>>
>>> Why CPU 0?
>>>
>>> If we have different CPUs running with different MSR_S then something
>>> has gone badly wrong, no?
>>
>> Yes, that would be very bad.
>>
>>> So can't we just read the MSR on whatever CPU the sysfs code happens to
>>> run on.
>>
>> Good point. I made the change in the patch below.
>
> The patch looks good. Although, it raises the question of whether it
> should be an attribute of the CPU at all.
>
> I guess there's not obviously anywhere better for it.

Ok. TBH this patch is not as urgent as the others. It was added so that
tests have an easy way to tell if they're in an SVM. I can leave it out
for now to figure out if there's a better place for this information.

> Still you should document the attribute in Documentation/ABI/testing/sysfs-devices-system-cpu

Indedd, will do that.

--
Thiago Jung Bauermann
IBM Linux Technology Center