| Message ID | 20190713060023.8479-4-bauerman@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Secure Virtual Machine Enablement | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch next (f5c20693d8edcd665f1159dc941b9e7f87c17647) |
| snowpatch_ozlabs/apply_patch | fail | Failed to apply to any branch |
On 13/07/2019 16:00, Thiago Jung Bauermann wrote: > From: Ram Pai <linuxram@us.ibm.com> > > Make the Enter-Secure-Mode (ESM) ultravisor call to switch the VM to secure > mode. Add "svm=" command line option to turn on switching to secure mode. > > Signed-off-by: Ram Pai <linuxram@us.ibm.com> > [ andmike: Generate an RTAS os-term hcall when the ESM ucall fails. ] > Signed-off-by: Michael Anderson <andmike@linux.ibm.com> > [ bauerman: Cleaned up the code a bit. ] > Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> > --- > .../admin-guide/kernel-parameters.txt | 5 + > arch/powerpc/include/asm/ultravisor-api.h | 1 + > arch/powerpc/kernel/prom_init.c | 99 +++++++++++++++++++ > 3 files changed, 105 insertions(+) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index 7b15abf7db21..c611891b5992 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -4585,6 +4585,11 @@ > /sys/power/pm_test). Only available when CONFIG_PM_DEBUG > is set. Default value is 5. > > + svm= [PPC] > + Format: { on | off | y | n | 1 | 0 } > + This parameter controls use of the Protected > + Execution Facility on pSeries. > + > swapaccount=[0|1] > [KNL] Enable accounting of swap in memory resource > controller if no parameter or 1 is given or disable > diff --git a/arch/powerpc/include/asm/ultravisor-api.h b/arch/powerpc/include/asm/ultravisor-api.h > index c8180427fa01..fe9a0d8d7673 100644 > --- a/arch/powerpc/include/asm/ultravisor-api.h > +++ b/arch/powerpc/include/asm/ultravisor-api.h > @@ -19,6 +19,7 @@ > > /* opcodes */ > #define UV_WRITE_PATE 0xF104 > +#define UV_ESM 0xF110 > #define UV_RETURN 0xF11C > #define UV_REGISTER_MEM_SLOT 0xF120 > #define UV_UNREGISTER_MEM_SLOT 0xF124 > diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c > index a3fb90bb5a39..6389a992451b 100644 > --- a/arch/powerpc/kernel/prom_init.c > +++ b/arch/powerpc/kernel/prom_init.c > @@ -44,6 +44,7 @@ > #include <asm/sections.h> > #include <asm/machdep.h> > #include <asm/asm-prototypes.h> > +#include <asm/ultravisor-api.h> > > #include <linux/linux_logo.h> > > @@ -175,6 +176,10 @@ static bool __prombss prom_radix_disable; > static bool __prombss prom_xive_disable; > #endif > > +#ifdef CONFIG_PPC_SVM > +static bool __prombss prom_svm_enable; > +#endif > + > struct platform_support { > bool hash_mmu; > bool radix_mmu; > @@ -816,6 +821,17 @@ static void __init early_cmdline_parse(void) > prom_debug("XIVE disabled from cmdline\n"); > } > #endif /* CONFIG_PPC_PSERIES */ > + > +#ifdef CONFIG_PPC_SVM > + opt = prom_strstr(prom_cmd_line, "svm="); > + if (opt) { > + bool val; > + > + opt += sizeof("svm=") - 1; > + if (!prom_strtobool(opt, &val)) > + prom_svm_enable = val; > + } > +#endif /* CONFIG_PPC_SVM */ > } > > #ifdef CONFIG_PPC_PSERIES > @@ -1716,6 +1732,43 @@ static void __init prom_close_stdin(void) > } > } > > +#ifdef CONFIG_PPC_SVM > +static int prom_rtas_hcall(uint64_t args) > +{ > + register uint64_t arg1 asm("r3") = H_RTAS; > + register uint64_t arg2 asm("r4") = args; > + > + asm volatile("sc 1\n" : "=r" (arg1) : > + "r" (arg1), > + "r" (arg2) :); > + return arg1; > +} > + > +static struct rtas_args __prombss os_term_args; > + > +static void __init prom_rtas_os_term(char *str) > +{ > + phandle rtas_node; > + __be32 val; > + u32 token; > + > + prom_debug("%s: start...\n", __func__); > + rtas_node = call_prom("finddevice", 1, 1, ADDR("/rtas")); > + prom_debug("rtas_node: %x\n", rtas_node); > + if (!PHANDLE_VALID(rtas_node)) > + return; > + > + val = 0; > + prom_getprop(rtas_node, "ibm,os-term", &val, sizeof(val)); > + token = be32_to_cpu(val); > + prom_debug("ibm,os-term: %x\n", token); > + if (token == 0) > + prom_panic("Could not get token for ibm,os-term\n"); > + os_term_args.token = cpu_to_be32(token); > + prom_rtas_hcall((uint64_t)&os_term_args); > +} > +#endif /* CONFIG_PPC_SVM */ > + > /* > * Allocate room for and instantiate RTAS > */ > @@ -3172,6 +3225,49 @@ static void unreloc_toc(void) > #endif > #endif > > +#ifdef CONFIG_PPC_SVM > +/* > + * Perform the Enter Secure Mode ultracall. > + */ > +static int enter_secure_mode(unsigned long kbase, unsigned long fdt) > +{ > + register uint64_t func asm("r3") = UV_ESM; > + register uint64_t arg1 asm("r4") = (uint64_t)kbase; > + register uint64_t arg2 asm("r5") = (uint64_t)fdt; What does UV do with kbase and fdt precisely? Few words in the commit log will do. > + > + asm volatile("sc 2\n" > + : "=r"(func) > + : "0"(func), "r"(arg1), "r"(arg2) > + :); > + > + return (int)func; And why "func"? Is it "function"? Weird name. Thanks, > +} > + > +/* > + * Call the Ultravisor to transfer us to secure memory if we have an ESM blob. > + */ > +static void setup_secure_guest(unsigned long kbase, unsigned long fdt) > +{ > + int ret; > + > + if (!prom_svm_enable) > + return; > + > + /* Switch to secure mode. */ > + prom_printf("Switching to secure mode.\n"); > + > + ret = enter_secure_mode(kbase, fdt); > + if (ret != U_SUCCESS) { > + prom_printf("Returned %d from switching to secure mode.\n", ret); > + prom_rtas_os_term("Switch to secure mode failed.\n"); > + } > +} > +#else > +static void setup_secure_guest(unsigned long kbase, unsigned long fdt) > +{ > +} > +#endif /* CONFIG_PPC_SVM */ > + > /* > * We enter here early on, when the Open Firmware prom is still > * handling exceptions and the MMU hash table for us. > @@ -3370,6 +3466,9 @@ unsigned long __init prom_init(unsigned long r3, unsigned long r4, > unreloc_toc(); > #endif > > + /* Move to secure memory if we're supposed to be secure guests. */ > + setup_secure_guest(kbase, hdr); > + > __start(hdr, kbase, 0, 0, 0, 0, 0); > > return 0; >
(Sorry to hijack your reply). On Thu, Jul 18, 2019 at 06:11:48PM +1000, Alexey Kardashevskiy wrote: > On 13/07/2019 16:00, Thiago Jung Bauermann wrote: > >From: Ram Pai <linuxram@us.ibm.com> > >+static int enter_secure_mode(unsigned long kbase, unsigned long fdt) > >+{ > >+ register uint64_t func asm("r3") = UV_ESM; > >+ register uint64_t arg1 asm("r4") = (uint64_t)kbase; > >+ register uint64_t arg2 asm("r5") = (uint64_t)fdt; > > What does UV do with kbase and fdt precisely? Few words in the commit > log will do. > > >+ > >+ asm volatile("sc 2\n" > >+ : "=r"(func) > >+ : "0"(func), "r"(arg1), "r"(arg2) > >+ :); > >+ > >+ return (int)func; > > And why "func"? Is it "function"? Weird name. Thanks, Maybe the three vars should just be called "r3", "r4", and "r5" -- r3 is used as return value as well, so "func" isn't a great name for it. Some other comments about this inline asm: The "\n" makes the generated asm look funny and has no other function. Instead of using backreferences you can use a "+" constraint, "inout". Empty clobber list is strange. Casts to the return type, like most other casts, are an invitation to bugs and not actually useful. So this can be written static int enter_secure_mode(unsigned long kbase, unsigned long fdt) { register uint64_t r3 asm("r3") = UV_ESM; register uint64_t r4 asm("r4") = kbase; register uint64_t r4 asm("r5") = fdt; asm volatile("sc 2" : "+r"(r3) : "r"(r4), "r"(r5)); return r3; } (and it probably should use u64 instead of both uint64_t and unsigned long?) Segher
Hello Segher, Thanks for your review and suggestions! Segher Boessenkool <segher@kernel.crashing.org> writes: > (Sorry to hijack your reply). > > On Thu, Jul 18, 2019 at 06:11:48PM +1000, Alexey Kardashevskiy wrote: >> On 13/07/2019 16:00, Thiago Jung Bauermann wrote: >> >From: Ram Pai <linuxram@us.ibm.com> >> >+static int enter_secure_mode(unsigned long kbase, unsigned long fdt) >> >+{ >> >+ register uint64_t func asm("r3") = UV_ESM; >> >+ register uint64_t arg1 asm("r4") = (uint64_t)kbase; >> >+ register uint64_t arg2 asm("r5") = (uint64_t)fdt; >> >> What does UV do with kbase and fdt precisely? Few words in the commit >> log will do. >> >> >+ >> >+ asm volatile("sc 2\n" >> >+ : "=r"(func) >> >+ : "0"(func), "r"(arg1), "r"(arg2) >> >+ :); >> >+ >> >+ return (int)func; >> >> And why "func"? Is it "function"? Weird name. Thanks, Yes, I believe func is for function. Perhaps ucall would be clearer if the variable wasn't reused for the return value as Segher points out. > Maybe the three vars should just be called "r3", "r4", and "r5" -- > r3 is used as return value as well, so "func" isn't a great name for it. Yes, that does seem simpler. > Some other comments about this inline asm: > > The "\n" makes the generated asm look funny and has no other function. > Instead of using backreferences you can use a "+" constraint, "inout". > Empty clobber list is strange. > Casts to the return type, like most other casts, are an invitation to > bugs and not actually useful. > > So this can be written > > static int enter_secure_mode(unsigned long kbase, unsigned long fdt) > { > register uint64_t r3 asm("r3") = UV_ESM; > register uint64_t r4 asm("r4") = kbase; > register uint64_t r4 asm("r5") = fdt; > > asm volatile("sc 2" : "+r"(r3) : "r"(r4), "r"(r5)); > > return r3; > } I'll adopt your version, it is cleaner inded. Thanks for providing it! > (and it probably should use u64 instead of both uint64_t and unsigned long?) Almost all of prom_init.c uses unsigned long, with u64 in just a few places. uint64_t isn't used anywhere else in the file. I'll switch to unsigned long everywhere, since this feature is only for 64 bit.
On 19/07/2019 07:28, Thiago Jung Bauermann wrote: > > Hello Segher, > > Thanks for your review and suggestions! > > Segher Boessenkool <segher@kernel.crashing.org> writes: > >> (Sorry to hijack your reply). >> >> On Thu, Jul 18, 2019 at 06:11:48PM +1000, Alexey Kardashevskiy wrote: >>> On 13/07/2019 16:00, Thiago Jung Bauermann wrote: >>>> From: Ram Pai <linuxram@us.ibm.com> >>>> +static int enter_secure_mode(unsigned long kbase, unsigned long fdt) >>>> +{ >>>> + register uint64_t func asm("r3") = UV_ESM; >>>> + register uint64_t arg1 asm("r4") = (uint64_t)kbase; >>>> + register uint64_t arg2 asm("r5") = (uint64_t)fdt; >>> >>> What does UV do with kbase and fdt precisely? Few words in the commit >>> log will do. What about this one? :) >>> >>>> + >>>> + asm volatile("sc 2\n" >>>> + : "=r"(func) >>>> + : "0"(func), "r"(arg1), "r"(arg2) >>>> + :); >>>> + >>>> + return (int)func; >>> >>> And why "func"? Is it "function"? Weird name. Thanks, > > Yes, I believe func is for function. Perhaps ucall would be clearer > if the variable wasn't reused for the return value as Segher points out. > >> Maybe the three vars should just be called "r3", "r4", and "r5" -- >> r3 is used as return value as well, so "func" isn't a great name for it. > > Yes, that does seem simpler. > >> Some other comments about this inline asm: >> >> The "\n" makes the generated asm look funny and has no other function. >> Instead of using backreferences you can use a "+" constraint, "inout". >> Empty clobber list is strange. >> Casts to the return type, like most other casts, are an invitation to >> bugs and not actually useful. >> >> So this can be written >> >> static int enter_secure_mode(unsigned long kbase, unsigned long fdt) >> { >> register uint64_t r3 asm("r3") = UV_ESM; >> register uint64_t r4 asm("r4") = kbase; >> register uint64_t r4 asm("r5") = fdt; >> >> asm volatile("sc 2" : "+r"(r3) : "r"(r4), "r"(r5)); >> >> return r3; >> } > > I'll adopt your version, it is cleaner inded. Thanks for providing it! > >> (and it probably should use u64 instead of both uint64_t and unsigned long?) > > Almost all of prom_init.c uses unsigned long, with u64 in just a few > places. uint64_t isn't used anywhere else in the file. I'll switch to > unsigned long everywhere, since this feature is only for 64 bit. >
Alexey Kardashevskiy <aik@ozlabs.ru> writes: > On 19/07/2019 07:28, Thiago Jung Bauermann wrote: >> >> Hello Segher, >> >> Thanks for your review and suggestions! >> >> Segher Boessenkool <segher@kernel.crashing.org> writes: >> >>> (Sorry to hijack your reply). >>> >>> On Thu, Jul 18, 2019 at 06:11:48PM +1000, Alexey Kardashevskiy wrote: >>>> On 13/07/2019 16:00, Thiago Jung Bauermann wrote: >>>>> From: Ram Pai <linuxram@us.ibm.com> >>>>> +static int enter_secure_mode(unsigned long kbase, unsigned long fdt) >>>>> +{ >>>>> + register uint64_t func asm("r3") = UV_ESM; >>>>> + register uint64_t arg1 asm("r4") = (uint64_t)kbase; >>>>> + register uint64_t arg2 asm("r5") = (uint64_t)fdt; >>>> >>>> What does UV do with kbase and fdt precisely? Few words in the commit >>>> log will do. > > > What about this one? :) Sorry, I don't have an elaborate answer yet. The non-elaborate answer is that the ultravisor uses the kbase and fdt as part of integrity checking of the secure guest. -- Thiago Jung Bauermann IBM Linux Technology Center
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index 7b15abf7db21..c611891b5992 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -4585,6 +4585,11 @@ /sys/power/pm_test). Only available when CONFIG_PM_DEBUG is set. Default value is 5. + svm= [PPC] + Format: { on | off | y | n | 1 | 0 } + This parameter controls use of the Protected + Execution Facility on pSeries. + swapaccount=[0|1] [KNL] Enable accounting of swap in memory resource controller if no parameter or 1 is given or disable diff --git a/arch/powerpc/include/asm/ultravisor-api.h b/arch/powerpc/include/asm/ultravisor-api.h index c8180427fa01..fe9a0d8d7673 100644 --- a/arch/powerpc/include/asm/ultravisor-api.h +++ b/arch/powerpc/include/asm/ultravisor-api.h @@ -19,6 +19,7 @@ /* opcodes */ #define UV_WRITE_PATE 0xF104 +#define UV_ESM 0xF110 #define UV_RETURN 0xF11C #define UV_REGISTER_MEM_SLOT 0xF120 #define UV_UNREGISTER_MEM_SLOT 0xF124 diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c index a3fb90bb5a39..6389a992451b 100644 --- a/arch/powerpc/kernel/prom_init.c +++ b/arch/powerpc/kernel/prom_init.c @@ -44,6 +44,7 @@ #include <asm/sections.h> #include <asm/machdep.h> #include <asm/asm-prototypes.h> +#include <asm/ultravisor-api.h> #include <linux/linux_logo.h> @@ -175,6 +176,10 @@ static bool __prombss prom_radix_disable; static bool __prombss prom_xive_disable; #endif +#ifdef CONFIG_PPC_SVM +static bool __prombss prom_svm_enable; +#endif + struct platform_support { bool hash_mmu; bool radix_mmu; @@ -816,6 +821,17 @@ static void __init early_cmdline_parse(void) prom_debug("XIVE disabled from cmdline\n"); } #endif /* CONFIG_PPC_PSERIES */ + +#ifdef CONFIG_PPC_SVM + opt = prom_strstr(prom_cmd_line, "svm="); + if (opt) { + bool val; + + opt += sizeof("svm=") - 1; + if (!prom_strtobool(opt, &val)) + prom_svm_enable = val; + } +#endif /* CONFIG_PPC_SVM */ } #ifdef CONFIG_PPC_PSERIES @@ -1716,6 +1732,43 @@ static void __init prom_close_stdin(void) } } +#ifdef CONFIG_PPC_SVM +static int prom_rtas_hcall(uint64_t args) +{ + register uint64_t arg1 asm("r3") = H_RTAS; + register uint64_t arg2 asm("r4") = args; + + asm volatile("sc 1\n" : "=r" (arg1) : + "r" (arg1), + "r" (arg2) :); + return arg1; +} + +static struct rtas_args __prombss os_term_args; + +static void __init prom_rtas_os_term(char *str) +{ + phandle rtas_node; + __be32 val; + u32 token; + + prom_debug("%s: start...\n", __func__); + rtas_node = call_prom("finddevice", 1, 1, ADDR("/rtas")); + prom_debug("rtas_node: %x\n", rtas_node); + if (!PHANDLE_VALID(rtas_node)) + return; + + val = 0; + prom_getprop(rtas_node, "ibm,os-term", &val, sizeof(val)); + token = be32_to_cpu(val); + prom_debug("ibm,os-term: %x\n", token); + if (token == 0) + prom_panic("Could not get token for ibm,os-term\n"); + os_term_args.token = cpu_to_be32(token); + prom_rtas_hcall((uint64_t)&os_term_args); +} +#endif /* CONFIG_PPC_SVM */ + /* * Allocate room for and instantiate RTAS */ @@ -3172,6 +3225,49 @@ static void unreloc_toc(void) #endif #endif +#ifdef CONFIG_PPC_SVM +/* + * Perform the Enter Secure Mode ultracall. + */ +static int enter_secure_mode(unsigned long kbase, unsigned long fdt) +{ + register uint64_t func asm("r3") = UV_ESM; + register uint64_t arg1 asm("r4") = (uint64_t)kbase; + register uint64_t arg2 asm("r5") = (uint64_t)fdt; + + asm volatile("sc 2\n" + : "=r"(func) + : "0"(func), "r"(arg1), "r"(arg2) + :); + + return (int)func; +} + +/* + * Call the Ultravisor to transfer us to secure memory if we have an ESM blob. + */ +static void setup_secure_guest(unsigned long kbase, unsigned long fdt) +{ + int ret; + + if (!prom_svm_enable) + return; + + /* Switch to secure mode. */ + prom_printf("Switching to secure mode.\n"); + + ret = enter_secure_mode(kbase, fdt); + if (ret != U_SUCCESS) { + prom_printf("Returned %d from switching to secure mode.\n", ret); + prom_rtas_os_term("Switch to secure mode failed.\n"); + } +} +#else +static void setup_secure_guest(unsigned long kbase, unsigned long fdt) +{ +} +#endif /* CONFIG_PPC_SVM */ + /* * We enter here early on, when the Open Firmware prom is still * handling exceptions and the MMU hash table for us. @@ -3370,6 +3466,9 @@ unsigned long __init prom_init(unsigned long r3, unsigned long r4, unreloc_toc(); #endif + /* Move to secure memory if we're supposed to be secure guests. */ + setup_secure_guest(kbase, hdr); + __start(hdr, kbase, 0, 0, 0, 0, 0); return 0;