POWER platforms that supports the Protected Execution Facility (PEF)
introduce features that combine hardware facilities and firmware to
enable secure virtual machines. That includes a new processor mode
(ultravisor mode) and the ultravisor firmware.
In PEF enabled systems, the ultravisor firmware runs at a privilege
level above the hypervisor and also takes control over some system
resources. The hypervisor, though, can make system calls to access these
resources. Such system calls, a.k.a. ucalls, are handled by the
ultravisor firmware.
The processor allows part of the system memory to be configured as
secure memory, and introduces a a new mode, called secure mode, where
any software entity in that mode can access secure memory. The
hypervisor doesn't (and can't) run in secure mode, but a secure guest
and the ultravisor firmware do.
This patch set adds support for ultravisor calls and do some preparation
for running secure guests.
---
Changelog:
---
v3->v4:
- Patch "KVM: PPC: Ultravisor: Add PPC_UV config option":
- Moved to the patchset "kvmppc: HMM driver to manage pages of secure
guest" v5 that will be posted by Bharata Rao.
- Patch "powerpc: Introduce FW_FEATURE_ULTRAVISOR":
- Changed to depend only on CONFIG_PPC_POWERNV.
- Patch "KVM: PPC: Ultravisor: Add generic ultravisor call handler":
- Fixed whitespaces in ucall.S and in ultravisor-api.h.
- Changed to depend only on CONFIG_PPC_POWERNV.
- Changed the ucall wrapper to pass the ucall number in R3.
- Patch "KVM: PPC: Ultravisor: Use UV_WRITE_PATE ucall to register a
PATE:
- Changed to depend only on CONFIG_PPC_POWERNV.
- Patch "KVM: PPC: Ultravisor: Restrict LDBAR access":
- Fixed comment in opal-imc.c to be "Disable IMC devices, when
Ultravisor is enabled.
- Fixed signed-off-by.
- Patch "KVM: PPC: Ultravisor: Enter a secure guest":
- Changed the UV_RETURN assembly call to save the actual R3 in
R0 for the ultravisor and pass the UV_RETURN call number in R3.
- Patch "KVM: PPC: Ultravisor: Check for MSR_S during hv_reset_msr":
- Fixed commit message.
- Rebased to powerpc/next.
v2->v3:
- Squashed patches:
- "KVM: PPC: Ultravisor: Return to UV for hcalls from SVM"
- "KVM: PPC: Book3S HV: Fixed for running secure guests"
- Renamed patch from/to:
- "KVM: PPC: Ultravisor: Return to UV for hcalls from SVM"
- "KVM: PPC: Ultravisor: Enter a secure guest
- Rebased
- Addressed comments from Paul Mackerras
- Dropped ultravisor checks made in power8 code
- Updated the commit message for:
"KVM: PPC: Ultravisor: Enter a secure guest"
- Addressed comments from Maddy
- Dropped imc-pmu.c changes
- Changed opal-imc.c to fail the probe when the ultravisor is enabled
- Fixed "ucall defined but not used" issue when CONFIG_PPC_UV not set
v1->v2:
- Addressed comments from Paul Mackerras:
- Write the pate in HV's table before doing that in UV's
- Renamed and better documented the ultravisor header files. Also added
all possible return codes for each ucall
- Updated the commit message that introduces the MSR_S bit
- Moved ultravisor.c and ucall.S to arch/powerpc/kernel
- Changed ucall.S to not save CR
- Rebased
- Changed the patches order
- Updated several commit messages
- Added FW_FEATURE_ULTRAVISOR to enable use of firmware_has_feature()
- Renamed CONFIG_PPC_KVM_UV to CONFIG_PPC_UV and used it to ifdef the ucall
handler and the code that populates the powerpc_firmware_features for
ultravisor
- Exported the ucall symbol. KVM may be built as module.
- Restricted LDBAR access if the ultravisor firmware is available
- Dropped patches:
- "[PATCH 06/13] KVM: PPC: Ultravisor: UV_RESTRICTED_SPR_WRITE ucall"
- "[PATCH 07/13] KVM: PPC: Ultravisor: UV_RESTRICTED_SPR_READ ucall"
- "[PATCH 08/13] KVM: PPC: Ultravisor: fix mtspr and mfspr"
- Squashed patches:
- "[PATCH 09/13] KVM: PPC: Ultravisor: Return to UV for hcalls from SVM"
- "[PATCH 13/13] KVM: PPC: UV: Have fast_guest_return check secure_guest"
Claudio Carvalho (2):
powerpc: Introduce FW_FEATURE_ULTRAVISOR
KVM: PPC: Ultravisor: Restrict LDBAR access
Michael Anderson (2):
KVM: PPC: Ultravisor: Use UV_WRITE_PATE ucall to register a PATE
KVM: PPC: Ultravisor: Check for MSR_S during hv_reset_msr
Ram Pai (2):
KVM: PPC: Ultravisor: Add generic ultravisor call handler
KVM: PPC: Ultravisor: Restrict flush of the partition tlb cache
Sukadev Bhattiprolu (2):
KVM: PPC: Ultravisor: Introduce the MSR_S bit
KVM: PPC: Ultravisor: Enter a secure guest
arch/powerpc/include/asm/firmware.h | 5 +-
arch/powerpc/include/asm/kvm_host.h | 1 +
arch/powerpc/include/asm/reg.h | 3 ++
arch/powerpc/include/asm/ultravisor-api.h | 24 +++++++++
arch/powerpc/include/asm/ultravisor.h | 49 +++++++++++++++++
arch/powerpc/kernel/Makefile | 1 +
arch/powerpc/kernel/asm-offsets.c | 1 +
arch/powerpc/kernel/prom.c | 4 ++
arch/powerpc/kernel/ucall.S | 30 +++++++++++
arch/powerpc/kernel/ultravisor.c | 28 ++++++++++
arch/powerpc/kvm/book3s_64_mmu_hv.c | 1 +
arch/powerpc/kvm/book3s_hv_rmhandlers.S | 42 ++++++++++++---
arch/powerpc/mm/book3s64/hash_utils.c | 3 +-
arch/powerpc/mm/book3s64/pgtable.c | 65 +++++++++++++++++------
arch/powerpc/mm/book3s64/radix_pgtable.c | 9 ++--
arch/powerpc/platforms/powernv/idle.c | 6 ++-
arch/powerpc/platforms/powernv/opal-imc.c | 4 ++
17 files changed, 246 insertions(+), 30 deletions(-)
create mode 100644 arch/powerpc/include/asm/ultravisor-api.h
create mode 100644 arch/powerpc/include/asm/ultravisor.h
create mode 100644 arch/powerpc/kernel/ucall.S
create mode 100644 arch/powerpc/kernel/ultravisor.c
--
2.20.1
@@ -706,6 +707,9 @@ void __init early_init_devtree(void *params)#ifdef CONFIG_PPC_POWERNV/* Some machines might need OPAL info for debugging, grab it now. */of_scan_flat_dt(early_init_dt_scan_opal,NULL);++/* Scan tree for ultravisor feature */+of_scan_flat_dt(early_init_dt_scan_ultravisor,NULL);#endif#ifdef CONFIG_FA_DUMP
This feature tells if the ultravisor firmware is available to handle
ucalls.
Signed-off-by: Claudio Carvalho <redacted>
[ Device node name to "ibm,ultravisor" ]
Signed-off-by: Michael Anderson <redacted>
@@ -706,6 +707,9 @@ void __init early_init_devtree(void *params)#ifdef CONFIG_PPC_POWERNV/* Some machines might need OPAL info for debugging, grab it now. */of_scan_flat_dt(early_init_dt_scan_opal,NULL);++/* Scan tree for ultravisor feature */+of_scan_flat_dt(early_init_dt_scan_ultravisor,NULL);#endif#ifdef CONFIG_FA_DUMP
diff --git a/arch/powerpc/kernel/ultravisor.c
b/arch/powerpc/kernel/ultravisor.c
new file mode 100644
index 000000000000..dc6021f63c97
Is there a reason this (and other later files) aren't in platforms/powernv ?
quoted hunk
@@ -0,0 +1,24 @@+// SPDX-License-Identifier: GPL-2.0+/*+ * Ultravisor high level interfaces+ *+ * Copyright 2019, IBM Corporation.+ *+ */+#include <linux/init.h>+#include <linux/printk.h>+#include <linux/string.h>++#include <asm/ultravisor.h>+#include <asm/firmware.h>++int __init early_init_dt_scan_ultravisor(unsigned long node, const char *uname,+ int depth, void *data)+{+ if (depth != 1 || strcmp(uname, "ibm,ultravisor") != 0)+ return 0;
I know you're following the example of OPAL, but this is not the best
way to search for the ultravisor node.
It makes the location and name of the node part of the ABI, when there's
no need for it to be.
If instead you just scan the tree for a node that is *compatible* with
"ibm,ultravisor" (or whatever compatible string) then the node can be
placed any where in the tree and have any name, which gives us the most
flexibility in future to change the location of the device tree node.
cheers
Is there a reason this (and other later files) aren't in platforms/powernv ?
Yes, there is.
https://www.spinics.net/lists/kvm-ppc/msg14998.html
We also need to do ucalls from a secure guest and its kernel may not have CONFIG_PPC_POWERNV=y. I can make it clear in the commit message.
quoted
@@ -0,0 +1,24 @@+// SPDX-License-Identifier: GPL-2.0+/*+ * Ultravisor high level interfaces+ *+ * Copyright 2019, IBM Corporation.+ *+ */+#include <linux/init.h>+#include <linux/printk.h>+#include <linux/string.h>++#include <asm/ultravisor.h>+#include <asm/firmware.h>++int __init early_init_dt_scan_ultravisor(unsigned long node, const char *uname,+ int depth, void *data)+{+ if (depth != 1 || strcmp(uname, "ibm,ultravisor") != 0)+ return 0;
I know you're following the example of OPAL, but this is not the best
way to search for the ultravisor node.
It makes the location and name of the node part of the ABI, when there's
no need for it to be.
If instead you just scan the tree for a node that is *compatible* with
"ibm,ultravisor" (or whatever compatible string) then the node can be
placed any where in the tree and have any name, which gives us the most
flexibility in future to change the location of the device tree node.
I will do that in the next version.
Thanks,
Claudio
Is there a reason this (and other later files) aren't in platforms/powernv ?
Yes, there is.
https://www.spinics.net/lists/kvm-ppc/msg14998.html
We also need to do ucalls from a secure guest and its kernel may not
have CONFIG_PPC_POWERNV=y. I can make it clear in the commit message.
OK, sorry I missed Paul's comment. Yeah that is obviously a valid reason :)
quoted
quoted
@@ -0,0 +1,24 @@+// SPDX-License-Identifier: GPL-2.0+/*+ * Ultravisor high level interfaces+ *+ * Copyright 2019, IBM Corporation.+ *+ */+#include <linux/init.h>+#include <linux/printk.h>+#include <linux/string.h>++#include <asm/ultravisor.h>+#include <asm/firmware.h>++int __init early_init_dt_scan_ultravisor(unsigned long node, const char *uname,+ int depth, void *data)+{+ if (depth != 1 || strcmp(uname, "ibm,ultravisor") != 0)+ return 0;
I know you're following the example of OPAL, but this is not the best
way to search for the ultravisor node.
It makes the location and name of the node part of the ABI, when there's
no need for it to be.
If instead you just scan the tree for a node that is *compatible* with
"ibm,ultravisor" (or whatever compatible string) then the node can be
placed any where in the tree and have any name, which gives us the most
flexibility in future to change the location of the device tree node.
From: Sukadev Bhattiprolu <redacted>
The ultravisor processor mode is introduced in POWER platforms that
supports the Protected Execution Facility (PEF). Ultravisor is higher
privileged than hypervisor mode.
In PEF enabled platforms, the MSR_S bit is used to indicate if the
thread is in secure state. With the MSR_S bit, the privilege state of
the thread is now determined by MSR_S, MSR_HV and MSR_PR, as follows:
S HV PR
-----------------------
0 x 1 problem
1 0 1 problem
x x 0 privileged
x 1 0 hypervisor
1 1 0 ultravisor
1 1 1 reserved
The hypervisor doesn't (and can't) run with the MSR_S bit set, but a
secure guest and the ultravisor firmware do.
Signed-off-by: Sukadev Bhattiprolu <redacted>
Signed-off-by: Ram Pai <redacted>
[ Update the commit message ]
Signed-off-by: Claudio Carvalho <redacted>
---
arch/powerpc/include/asm/reg.h | 3 +++
1 file changed, 3 insertions(+)
From: Sukadev Bhattiprolu <redacted>
The ultravisor processor mode is introduced in POWER platforms that
supports the Protected Execution Facility (PEF). Ultravisor is higher
privileged than hypervisor mode.
In PEF enabled platforms, the MSR_S bit is used to indicate if the
thread is in secure state. With the MSR_S bit, the privilege state of
the thread is now determined by MSR_S, MSR_HV and MSR_PR, as follows:
S HV PR
-----------------------
0 x 1 problem
1 0 1 problem
x x 0 privileged
x 1 0 hypervisor
1 1 0 ultravisor
1 1 1 reserved
The hypervisor doesn't (and can't) run with the MSR_S bit set, but a
secure guest and the ultravisor firmware do.
Signed-off-by: Sukadev Bhattiprolu <redacted>
Signed-off-by: Ram Pai <redacted>
[ Update the commit message ]
Signed-off-by: Claudio Carvalho <redacted>
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2019-07-11 12:59:53
Claudio Carvalho [off-list ref] writes:
From: Sukadev Bhattiprolu <redacted>
The ultravisor processor mode is introduced in POWER platforms that
supports the Protected Execution Facility (PEF). Ultravisor is higher
privileged than hypervisor mode.
In PEF enabled platforms, the MSR_S bit is used to indicate if the
thread is in secure state. With the MSR_S bit, the privilege state of
the thread is now determined by MSR_S, MSR_HV and MSR_PR, as follows:
S HV PR
-----------------------
0 x 1 problem
1 0 1 problem
x x 0 privileged
x 1 0 hypervisor
1 1 0 ultravisor
1 1 1 reserved
What are you trying to express with the 'x' value?
I guess you mean it as "either" or "don't care" - but then you have
cases where it could only have one value, such as hypervisor. I think it
would be clearer if you spelled it out more explicitly.
The hypervisor doesn't (and can't) run with the MSR_S bit set, but a
secure guest and the ultravisor firmware do.
I know you're trying to be helpful, but this comment is really just
confusing to someone who doesn't have all the documentation.
I'd really like to see something in Documentation/powerpc describing at
least the outline of how the system works. I'm pretty sure most of that
is public, so even if it's mostly a list of references to other
documentations or presentations that would be fine. But I'm not really
happy with a whole new processor mode appearing with zero documentation
in the tree.
Signed-off-by: Sukadev Bhattiprolu <redacted>
Signed-off-by: Ram Pai <redacted>
[ Update the commit message ]
It's normal to prefix these comments with your handle to make it clear
who is saying it.
@@ -38,6 +38,7 @@#define MSR_TM_LG 32 /* Trans Mem Available */#define MSR_VEC_LG 25 /* Enable AltiVec */#define MSR_VSX_LG 23 /* Enable VSX */+#define MSR_S_LG 22 /* Secure VM bit */
I don't think that's the best description, because it's also the
Ultravisor bit when MSR[HV] = 1.
So "Secure state" as you have below would be better IMO.
cheers
quoted hunk
#define MSR_POW_LG 18 /* Enable Power Management */
#define MSR_WE_LG 18 /* Wait State Enable */
#define MSR_TGPR_LG 17 /* TLB Update registers in use */
@@ -71,11 +72,13 @@ #define MSR_SF __MASK(MSR_SF_LG) /* Enable 64 bit mode */ #define MSR_ISF __MASK(MSR_ISF_LG) /* Interrupt 64b mode valid on 630 */ #define MSR_HV __MASK(MSR_HV_LG) /* Hypervisor state */+#define MSR_S __MASK(MSR_S_LG) /* Secure state */ #else /* so tests for these bits fail on 32-bit */ #define MSR_SF 0 #define MSR_ISF 0 #define MSR_HV 0+#define MSR_S 0 #endif /*
From: Nicholas Piggin <npiggin@gmail.com> Date: 2019-07-12 01:05:17
Michael Ellerman's on July 11, 2019 10:57 pm:
Claudio Carvalho [off-list ref] writes:
quoted
From: Sukadev Bhattiprolu <redacted>
The ultravisor processor mode is introduced in POWER platforms that
supports the Protected Execution Facility (PEF). Ultravisor is higher
privileged than hypervisor mode.
In PEF enabled platforms, the MSR_S bit is used to indicate if the
thread is in secure state. With the MSR_S bit, the privilege state of
the thread is now determined by MSR_S, MSR_HV and MSR_PR, as follows:
S HV PR
-----------------------
0 x 1 problem
1 0 1 problem
x x 0 privileged
x 1 0 hypervisor
1 1 0 ultravisor
1 1 1 reserved
What are you trying to express with the 'x' value?
I guess you mean it as "either" or "don't care" - but then you have
cases where it could only have one value, such as hypervisor. I think it
would be clearer if you spelled it out more explicitly.
quoted
The hypervisor doesn't (and can't) run with the MSR_S bit set, but a
secure guest and the ultravisor firmware do.
I know you're trying to be helpful, but this comment is really just
confusing to someone who doesn't have all the documentation.
I'd really like to see something in Documentation/powerpc describing at
least the outline of how the system works. I'm pretty sure most of that
is public, so even if it's mostly a list of references to other
documentations or presentations that would be fine. But I'm not really
happy with a whole new processor mode appearing with zero documentation
in the tree.
quoted
Signed-off-by: Sukadev Bhattiprolu <redacted>
Signed-off-by: Ram Pai <redacted>
[ Update the commit message ]
It's normal to prefix these comments with your handle to make it clear
who is saying it.
@@ -38,6 +38,7 @@#define MSR_TM_LG 32 /* Trans Mem Available */#define MSR_VEC_LG 25 /* Enable AltiVec */#define MSR_VSX_LG 23 /* Enable VSX */+#define MSR_S_LG 22 /* Secure VM bit */
I don't think that's the best description, because it's also the
Ultravisor bit when MSR[HV] = 1.
So "Secure state" as you have below would be better IMO.
Ooops I see Michael covered everything I wrote, sorry for the noise
I missed the thread.
Thanks,
Nick
From: Nicholas Piggin <npiggin@gmail.com> Date: 2019-07-12 01:03:01
Claudio Carvalho's on June 29, 2019 6:08 am:
From: Sukadev Bhattiprolu <redacted>
The ultravisor processor mode is introduced in POWER platforms that
supports the Protected Execution Facility (PEF). Ultravisor is higher
privileged than hypervisor mode.
In PEF enabled platforms, the MSR_S bit is used to indicate if the
thread is in secure state. With the MSR_S bit, the privilege state of
the thread is now determined by MSR_S, MSR_HV and MSR_PR, as follows:
S HV PR
-----------------------
0 x 1 problem
1 0 1 problem
x x 0 privileged
x 1 0 hypervisor
1 1 0 ultravisor
1 1 1 reserved
What does this table mean? I thought 'x' meant either, but in that
case there are several states that can apply to the same
combination of bits.
Would it be clearer to rearrange the table so the columns are the HV
and PR bits we know and love, plus the effect of S=1 on each of them?
HV PR S=0 S=1
---------------------------------------------
0 0 privileged privileged (secure guest kernel)
0 1 problem problem (secure guest userspace)
1 0 hypervisor ultravisor
1 1 problem reserved
Is that accurate?
quoted hunk
The hypervisor doesn't (and can't) run with the MSR_S bit set, but a
secure guest and the ultravisor firmware do.
Signed-off-by: Sukadev Bhattiprolu <redacted>
Signed-off-by: Ram Pai <redacted>
[ Update the commit message ]
Signed-off-by: Claudio Carvalho <redacted>
---
arch/powerpc/include/asm/reg.h | 3 +++
1 file changed, 3 insertions(+)
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2019-07-12 06:31:53
Nicholas Piggin [off-list ref] writes:
Claudio Carvalho's on June 29, 2019 6:08 am:
quoted
From: Sukadev Bhattiprolu <redacted>
The ultravisor processor mode is introduced in POWER platforms that
supports the Protected Execution Facility (PEF). Ultravisor is higher
privileged than hypervisor mode.
In PEF enabled platforms, the MSR_S bit is used to indicate if the
thread is in secure state. With the MSR_S bit, the privilege state of
the thread is now determined by MSR_S, MSR_HV and MSR_PR, as follows:
S HV PR
-----------------------
0 x 1 problem
1 0 1 problem
x x 0 privileged
x 1 0 hypervisor
1 1 0 ultravisor
1 1 1 reserved
What does this table mean? I thought 'x' meant either, but in that
case there are several states that can apply to the same
combination of bits.
Would it be clearer to rearrange the table so the columns are the HV
and PR bits we know and love, plus the effect of S=1 on each of them?
HV PR S=0 S=1
---------------------------------------------
0 0 privileged privileged (secure guest kernel)
0 1 problem problem (secure guest userspace)
1 0 hypervisor ultravisor
1 1 problem reserved
Is that accurate?
From: Sukadev Bhattiprolu <redacted>
The ultravisor processor mode is introduced in POWER platforms that
supports the Protected Execution Facility (PEF). Ultravisor is higher
privileged than hypervisor mode.
In PEF enabled platforms, the MSR_S bit is used to indicate if the
thread is in secure state. With the MSR_S bit, the privilege state of
the thread is now determined by MSR_S, MSR_HV and MSR_PR, as follows:
S HV PR
-----------------------
0 x 1 problem
1 0 1 problem
x x 0 privileged
x 1 0 hypervisor
1 1 0 ultravisor
1 1 1 reserved
What does this table mean? I thought 'x' meant either
Yes, it means either. The table was arranged that way to say that:
- hypervisor state is also a privileged state,
- ultravisor state is also a hypervisor state.
, but in that
case there are several states that can apply to the same
combination of bits.
Would it be clearer to rearrange the table so the columns are the HV
and PR bits we know and love, plus the effect of S=1 on each of them?
HV PR S=0 S=1
---------------------------------------------
0 0 privileged privileged (secure guest kernel)
0 1 problem problem (secure guest userspace)
1 0 hypervisor ultravisor
1 1 problem reserved
Is that accurate?
Yes, it is. I also like this format. I will consider it.
quoted
The hypervisor doesn't (and can't) run with the MSR_S bit set, but a
secure guest and the ultravisor firmware do.
Signed-off-by: Sukadev Bhattiprolu <redacted>
Signed-off-by: Ram Pai <redacted>
[ Update the commit message ]
Signed-off-by: Claudio Carvalho <redacted>
---
arch/powerpc/include/asm/reg.h | 3 +++
1 file changed, 3 insertions(+)
From: Ram Pai <redacted>
Add the ucall() function, which can be used to make ultravisor calls
with varied number of in and out arguments. Ultravisor calls can be made
from the host or guests.
This copies the implementation of plpar_hcall().
Signed-off-by: Ram Pai <redacted>
[ Change ucall.S to not save CR, rename and move headers, build ucall.S
if CONFIG_PPC_POWERNV set, use R3 for the ucall number and add some
comments in the code ]
Signed-off-by: Claudio Carvalho <redacted>
---
arch/powerpc/include/asm/ultravisor-api.h | 20 +++++++++++++++
arch/powerpc/include/asm/ultravisor.h | 20 +++++++++++++++
arch/powerpc/kernel/Makefile | 2 +-
arch/powerpc/kernel/ucall.S | 30 +++++++++++++++++++++++
arch/powerpc/kernel/ultravisor.c | 4 +++
5 files changed, 75 insertions(+), 1 deletion(-)
create mode 100644 arch/powerpc/include/asm/ultravisor-api.h
create mode 100644 arch/powerpc/kernel/ucall.S
From: Ram Pai <redacted>
Add the ucall() function, which can be used to make ultravisor calls
with varied number of in and out arguments. Ultravisor calls can be
made
from the host or guests.
This copies the implementation of plpar_hcall().
Signed-off-by: Ram Pai <redacted>
[ Change ucall.S to not save CR, rename and move headers, build ucall.S
if CONFIG_PPC_POWERNV set, use R3 for the ucall number and add some
comments in the code ]
Signed-off-by: Claudio Carvalho <redacted>
char *uname,
int depth, void *data);
+/* API functions */
+#define UCALL_BUFSIZE 4
+/**
+ * ucall: Make a powerpc ultravisor call.
+ * @opcode: The ultravisor call to make.
+ * @retbuf: Buffer to store up to 4 return arguments in.
+ *
+ * This call supports up to 6 arguments and 4 return arguments. Use
+ * UCALL_BUFSIZE to size the return argument buffer.
+ */
+#if defined(CONFIG_PPC_POWERNV)
+long ucall(unsigned long opcode, unsigned long *retbuf, ...);
+#endif
+
+#endif /* !__ASSEMBLY__ */
+
#endif /* _ASM_POWERPC_ULTRAVISOR_H */
diff --git a/arch/powerpc/kernel/Makefile
b/arch/powerpc/kernel/Makefile
index f0caa302c8c0..f28baccc0a79 100644
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2019-07-11 13:02:51
Claudio Carvalho [off-list ref] writes:
From: Ram Pai <redacted>
Add the ucall() function, which can be used to make ultravisor calls
with varied number of in and out arguments. Ultravisor calls can be made
from the host or guests.
This copies the implementation of plpar_hcall().
.. with quite a few changes?
This is one of the things I'd like to see in a Documentation file, so
that people can review the implementation vs the specification.
Signed-off-by: Ram Pai <redacted>
[ Change ucall.S to not save CR, rename and move headers, build ucall.S
if CONFIG_PPC_POWERNV set, use R3 for the ucall number and add some
comments in the code ]
Why are we not saving CR? See previous comment about Documentation :)
extern int early_init_dt_scan_ultravisor(unsigned long node, const char *uname,
int depth, void *data);
+/* API functions */
+#define UCALL_BUFSIZE 4
Please don't copy this design from the hcall code, it has led to bugs in
the past.
See my (still unmerged) attempt to fix this for the hcall case:
https://patchwork.ozlabs.org/patch/683577/
Basically instead of asking callers nicely to define a certain sized
buffer, and them forgetting, define a proper type that has the right size.
+/**
+ * ucall: Make a powerpc ultravisor call.
+ * @opcode: The ultravisor call to make.
+ * @retbuf: Buffer to store up to 4 return arguments in.
+ *
+ * This call supports up to 6 arguments and 4 return arguments. Use
+ * UCALL_BUFSIZE to size the return argument buffer.
+ */
+#if defined(CONFIG_PPC_POWERNV)
#ifdef
quoted hunk
+long ucall(unsigned long opcode, unsigned long *retbuf, ...);
+#endif
+
+#endif /* !__ASSEMBLY__ */
+
#endif /* _ASM_POWERPC_ULTRAVISOR_H */
From: Ram Pai <redacted>
Add the ucall() function, which can be used to make ultravisor calls
with varied number of in and out arguments. Ultravisor calls can be made
from the host or guests.
This copies the implementation of plpar_hcall().
.. with quite a few changes?
This is one of the things I'd like to see in a Documentation file, so
that people can review the implementation vs the specification.
I will document this (and other things) in a file under Documentation/powerpc.
quoted
Signed-off-by: Ram Pai <redacted>
[ Change ucall.S to not save CR, rename and move headers, build ucall.S
if CONFIG_PPC_POWERNV set, use R3 for the ucall number and add some
comments in the code ]
Why are we not saving CR? See previous comment about Documentation :)
extern int early_init_dt_scan_ultravisor(unsigned long node, const char *uname,
int depth, void *data);
+/* API functions */
+#define UCALL_BUFSIZE 4
Please don't copy this design from the hcall code, it has led to bugs in
the past.
See my (still unmerged) attempt to fix this for the hcall case:
https://patchwork.ozlabs.org/patch/683577/
Basically instead of asking callers nicely to define a certain sized
buffer, and them forgetting, define a proper type that has the right size.
I will keep that in mind. For now I think we don't need that since the v5
will have ucall_norets() instead.
quoted
+/**
+ * ucall: Make a powerpc ultravisor call.
+ * @opcode: The ultravisor call to make.
+ * @retbuf: Buffer to store up to 4 return arguments in.
+ *
+ * This call supports up to 6 arguments and 4 return arguments. Use
+ * UCALL_BUFSIZE to size the return argument buffer.
+ */
+#if defined(CONFIG_PPC_POWERNV)
#ifdef
quoted
+long ucall(unsigned long opcode, unsigned long *retbuf, ...);
+#endif
+
+#endif /* !__ASSEMBLY__ */
+
#endif /* _ASM_POWERPC_ULTRAVISOR_H */
@@ -8,10 +8,14 @@#include<linux/init.h>#include<linux/printk.h>#include<linux/string.h>+#include<linux/export.h>#include<asm/ultravisor.h>#include<asm/firmware.h>+/* in ucall.S */+EXPORT_SYMBOL_GPL(ucall);
This should be in ucall.S
Here I'm following the same way that hypercall wrapper symbols are exported.
Last time I tried to export that in ucall.S the linker complained that the
ucall
symbol will not be versioned. Something like this:
https://patchwork.kernel.org/patch/9452759/
Thanks,
Claudio
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2019-07-15 04:52:38
Claudio Carvalho [off-list ref] writes:
On 7/11/19 9:57 AM, Michael Ellerman wrote:
quoted
Claudio Carvalho [off-list ref] writes:
quoted
From: Ram Pai <redacted>
Add the ucall() function, which can be used to make ultravisor calls
with varied number of in and out arguments. Ultravisor calls can be made
from the host or guests.
This copies the implementation of plpar_hcall().
.. with quite a few changes?
This is one of the things I'd like to see in a Documentation file, so
that people can review the implementation vs the specification.
I will document this (and other things) in a file under Documentation/powerpc.
Thanks.
quoted
quoted
+/* API functions */
+#define UCALL_BUFSIZE 4
Please don't copy this design from the hcall code, it has led to bugs in
the past.
See my (still unmerged) attempt to fix this for the hcall case:
https://patchwork.ozlabs.org/patch/683577/
Basically instead of asking callers nicely to define a certain sized
buffer, and them forgetting, define a proper type that has the right size.
I will keep that in mind. For now I think we don't need that since the v5
will have ucall_norets() instead.
@@ -8,10 +8,14 @@#include<linux/init.h>#include<linux/printk.h>#include<linux/string.h>+#include<linux/export.h>#include<asm/ultravisor.h>#include<asm/firmware.h>+/* in ucall.S */+EXPORT_SYMBOL_GPL(ucall);
This should be in ucall.S
Here I'm following the same way that hypercall wrapper symbols are exported.
Yeah we used to not be able to export from .S but that was fixed a few
years ago.
Last time I tried to export that in ucall.S the linker complained that the
ucall
symbol will not be versioned. Something like this:
https://patchwork.kernel.org/patch/9452759/
I think you just need to include <asm/export.h> in the .S ?
cheers
From: Nicholas Piggin <npiggin@gmail.com> Date: 2019-07-12 01:23:01
Claudio Carvalho's on June 29, 2019 6:08 am:
quoted hunk
From: Ram Pai <redacted>
Add the ucall() function, which can be used to make ultravisor calls
with varied number of in and out arguments. Ultravisor calls can be made
from the host or guests.
This copies the implementation of plpar_hcall().
Signed-off-by: Ram Pai <redacted>
[ Change ucall.S to not save CR, rename and move headers, build ucall.S
if CONFIG_PPC_POWERNV set, use R3 for the ucall number and add some
comments in the code ]
Signed-off-by: Claudio Carvalho <redacted>
---
arch/powerpc/include/asm/ultravisor-api.h | 20 +++++++++++++++
arch/powerpc/include/asm/ultravisor.h | 20 +++++++++++++++
arch/powerpc/kernel/Makefile | 2 +-
arch/powerpc/kernel/ucall.S | 30 +++++++++++++++++++++++
arch/powerpc/kernel/ultravisor.c | 4 +++
5 files changed, 75 insertions(+), 1 deletion(-)
create mode 100644 arch/powerpc/include/asm/ultravisor-api.h
create mode 100644 arch/powerpc/kernel/ucall.S
From: Michael Anderson <redacted>
When running under an ultravisor, the ultravisor controls the real
partition table and has it in secure memory where the hypervisor can't
access it, and therefore we (the HV) have to do a ucall whenever we want
to update an entry.
The HV still keeps a copy of its view of the partition table in normal
memory so that the nest MMU can access it.
Both partition tables will have PATE entries for HV and normal virtual
machines.
Suggested-by: Ryan Grimm <redacted>
Signed-off-by: Michael Anderson <redacted>
Signed-off-by: Madhavan Srinivasan <redacted>
Signed-off-by: Ram Pai <redacted>
[ Write the pate in HV's table before doing that in UV's ]
Signed-off-by: Claudio Carvalho <redacted>
---
arch/powerpc/include/asm/ultravisor-api.h | 5 +++-
arch/powerpc/include/asm/ultravisor.h | 14 ++++++++++
arch/powerpc/mm/book3s64/hash_utils.c | 3 +-
arch/powerpc/mm/book3s64/pgtable.c | 34 +++++++++++++++++++++--
arch/powerpc/mm/book3s64/radix_pgtable.c | 9 ++++--
5 files changed, 57 insertions(+), 8 deletions(-)
@@ -238,6 +253,19 @@ void mmu_partition_table_set_entry(unsigned int lpid, unsigned long dw0,/* do we need fixup here ?*/asmvolatile("eieio; tlbsync; ptesync":::"memory");}++voidmmu_partition_table_set_entry(unsignedintlpid,unsignedlongdw0,+unsignedlongdw1)+{+__mmu_partition_table_set_entry(lpid,dw0,dw1);++if(firmware_has_feature(FW_FEATURE_ULTRAVISOR)){+uv_register_pate(lpid,dw0,dw1);+pr_info("PATE registered by ultravisor: dw0 = 0x%lx, dw1 = 0x%lx\n",+dw0,dw1);+}+}+EXPORT_SYMBOL_GPL(mmu_partition_table_set_entry);staticpmd_t*get_pmd_from_cache(structmm_struct*mm)
From: Michael Anderson <redacted>
When running under an ultravisor, the ultravisor controls the real
partition table and has it in secure memory where the hypervisor can't
access it, and therefore we (the HV) have to do a ucall whenever we
want
to update an entry.
The HV still keeps a copy of its view of the partition table in normal
memory so that the nest MMU can access it.
Both partition tables will have PATE entries for HV and normal virtual
machines.
Suggested-by: Ryan Grimm <redacted>
Signed-off-by: Michael Anderson <redacted>
Signed-off-by: Madhavan Srinivasan <redacted>
Signed-off-by: Ram Pai <redacted>
[ Write the pate in HV's table before doing that in UV's ]
Signed-off-by: Claudio Carvalho <redacted>
dw0,
- unsigned long dw1)
+static void __mmu_partition_table_set_entry(unsigned int lpid,
+ unsigned long dw0,
+ unsigned long dw1)
{
unsigned long old = be64_to_cpu(partition_tb[lpid].patb0);
@@ -238,6 +253,19 @@ void mmu_partition_table_set_entry(unsigned int
lpid, unsigned long dw0,
/* do we need fixup here ?*/
asm volatile("eieio; tlbsync; ptesync" : : : "memory");
}
+
+void mmu_partition_table_set_entry(unsigned int lpid, unsigned long
dw0,
+ unsigned long dw1)
+{
+ __mmu_partition_table_set_entry(lpid, dw0, dw1);
+
+ if (firmware_has_feature(FW_FEATURE_ULTRAVISOR)) {
+ uv_register_pate(lpid, dw0, dw1);
+ pr_info("PATE registered by ultravisor: dw0 = 0x%lx, dw1 = 0x%lx\n",
+ dw0, dw1);
+ }
+}
+
EXPORT_SYMBOL_GPL(mmu_partition_table_set_entry);
static pmd_t *get_pmd_from_cache(struct mm_struct *mm)
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2019-07-11 13:08:17
Claudio Carvalho [off-list ref] writes:
From: Michael Anderson <redacted>
When running under an ultravisor, the ultravisor controls the real
partition table and has it in secure memory where the hypervisor can't
access it, and therefore we (the HV) have to do a ucall whenever we want
to update an entry.
The HV still keeps a copy of its view of the partition table in normal
memory so that the nest MMU can access it.
Both partition tables will have PATE entries for HV and normal virtual
Can you expand novel acronyms on their first usage please, ie. PATE.
machines.
Suggested-by: Ryan Grimm <redacted>
"Suggested" implies this is optional, but it's not :)
I'm not sure exactly what Ryan's input was, but feel free to make up a
better tag :)
quoted hunk
Signed-off-by: Michael Anderson <redacted>
Signed-off-by: Madhavan Srinivasan <redacted>
Signed-off-by: Ram Pai <redacted>
[ Write the pate in HV's table before doing that in UV's ]
Signed-off-by: Claudio Carvalho <redacted>
---
arch/powerpc/include/asm/ultravisor-api.h | 5 +++-
arch/powerpc/include/asm/ultravisor.h | 14 ++++++++++
arch/powerpc/mm/book3s64/hash_utils.c | 3 +-
arch/powerpc/mm/book3s64/pgtable.c | 34 +++++++++++++++++++++--
arch/powerpc/mm/book3s64/radix_pgtable.c | 9 ++++--
5 files changed, 57 insertions(+), 8 deletions(-)
But we just created the partition table?!
This comment and the one below would probably make more sense if they
were merged and appeared further up, where we allocate the partition
table.
+ if (!firmware_has_feature(FW_FEATURE_ULTRAVISOR))
+ mtspr(SPRN_PTCR, ptcr);
+
+ /*
+ * Since nestMMU cannot access secure memory. Create
+ * and manage our own partition table. This table
But we just said the UV was responsible for it :)
quoted hunk
+ * contains entries for nonsecure and hypervisor
+ * partition.
+ */
powernv_set_nmmu_ptcr(ptcr);
}
-void mmu_partition_table_set_entry(unsigned int lpid, unsigned long dw0,
- unsigned long dw1)
+static void __mmu_partition_table_set_entry(unsigned int lpid,
+ unsigned long dw0,
+ unsigned long dw1)
{
unsigned long old = be64_to_cpu(partition_tb[lpid].patb0);
@@ -238,6 +253,19 @@ void mmu_partition_table_set_entry(unsigned int lpid, unsigned long dw0, /* do we need fixup here ?*/ asm volatile("eieio; tlbsync; ptesync" : : : "memory"); }++void mmu_partition_table_set_entry(unsigned int lpid, unsigned long dw0,+ unsigned long dw1)+{+ __mmu_partition_table_set_entry(lpid, dw0, dw1);++ if (firmware_has_feature(FW_FEATURE_ULTRAVISOR)) {+ uv_register_pate(lpid, dw0, dw1);+ pr_info("PATE registered by ultravisor: dw0 = 0x%lx, dw1 = 0x%lx\n",+ dw0, dw1);+ }+}
I agree with Alexey that this patch and the next should be merged and
the result cleaned up a bit.
There's four of these case where we skip touching the PTCR, which is
right on the borderline of warranting an accessor. I guess we can do it
as a cleanup later.
cheers
From: Ryan Grimm <hidden> Date: 2019-07-17 15:02:30
On Thu, 2019-07-11 at 22:57 +1000, Michael Ellerman wrote:
Claudio Carvalho [off-list ref] writes:
quoted
From: Michael Anderson <redacted>
When running under an ultravisor, the ultravisor controls the real
partition table and has it in secure memory where the hypervisor
can't
access it, and therefore we (the HV) have to do a ucall whenever we
want
to update an entry.
The HV still keeps a copy of its view of the partition table in
normal
memory so that the nest MMU can access it.
Both partition tables will have PATE entries for HV and normal
virtual
Can you expand novel acronyms on their first usage please, ie. PATE.
Agreed. This confused me a while ago. It is "Partition Table Entry",
correct?
quoted
machines.
Suggested-by: Ryan Grimm <redacted>
Please remove this and add
Reviewed-by: Ryan Grimm <redacted>
"Suggested" implies this is optional, but it's not :)
I'm not sure exactly what Ryan's input was, but feel free to make up
a
better tag :)
quoted
Signed-off-by: Michael Anderson <redacted>
Signed-off-by: Madhavan Srinivasan <redacted>
Signed-off-by: Ram Pai <redacted>
[ Write the pate in HV's table before doing that in UV's ]
Signed-off-by: Claudio Carvalho <redacted>
---
arch/powerpc/include/asm/ultravisor-api.h | 5 +++-
arch/powerpc/include/asm/ultravisor.h | 14 ++++++++++
arch/powerpc/mm/book3s64/hash_utils.c | 3 +-
arch/powerpc/mm/book3s64/pgtable.c | 34
+++++++++++++++++++++--
arch/powerpc/mm/book3s64/radix_pgtable.c | 9 ++++--
5 files changed, 57 insertions(+), 8 deletions(-)
early_init_dt_scan_ultravisor(unsigned long node, const char
*uname,
*/
#if defined(CONFIG_PPC_POWERNV)
long ucall(unsigned long opcode, unsigned long *retbuf, ...);
+#else
+static long ucall(unsigned long opcode, unsigned long *retbuf,
...)
But we just created the partition table?!
This comment and the one below would probably make more sense if they
were merged and appeared further up, where we allocate the partition
table.
quoted
+ if (!firmware_has_feature(FW_FEATURE_ULTRAVISOR))
+ mtspr(SPRN_PTCR, ptcr);
+
+ /*
+ * Since nestMMU cannot access secure memory. Create
+ * and manage our own partition table. This table
But we just said the UV was responsible for it :)
quoted
+ * contains entries for nonsecure and hypervisor
+ * partition.
+ */
powernv_set_nmmu_ptcr(ptcr);
}
-void mmu_partition_table_set_entry(unsigned int lpid, unsigned
long dw0,
- unsigned long dw1)
+static void __mmu_partition_table_set_entry(unsigned int lpid,
+ unsigned long dw0,
+ unsigned long dw1)
{
unsigned long old = be64_to_cpu(partition_tb[lpid].patb0);
There's four of these case where we skip touching the PTCR, which is
right on the borderline of warranting an accessor. I guess we can do
it
as a cleanup later.
cheers
There's four of these case where we skip touching the PTCR, which is
right on the borderline of warranting an accessor. I guess we can do it
as a cleanup later.
I agree.
Since the kernel doesn't need to access a big number of ultravisor
privileged registers, maybe we can define mtspr_<reg> and mfspr_<reg>
inline functions that in ultravisor.h that skip touching the register if an
ultravisor is present and and the register is ultravisor privileged. Thus,
we don't need to replicate comments and that also would make it easier for
developers to know what are the ultravisor privileged registers.
Something like this:
* 64 K size.
*/
ptcr = __pa(partition_tb) | (PATB_SIZE_SHIFT - 12);
- mtspr(SPRN_PTCR, ptcr);
+ mtspr_ptcr(ptcr);
powernv_set_nmmu_ptcr(ptcr);
}
What do you think?
An alternative could be to change the mtspr() and mfspr() macros as we
proposed in the v1, but access to non-ultravisor privileged registers would
be performance impacted because we always would need to check if the
register is one of the few ultravisor registers that the kernel needs to
access.
Thanks,
Claudio
There's four of these case where we skip touching the PTCR, which is
right on the borderline of warranting an accessor. I guess we can do it
as a cleanup later.
I agree.
Since the kernel doesn't need to access a big number of ultravisor
privileged registers, maybe we can define mtspr_<reg> and mfspr_<reg>
inline functions that in ultravisor.h that skip touching the register if an
ultravisor is present and and the register is ultravisor privileged. Thus,
we don't need to replicate comments and that also would make it easier for
developers to know what are the ultravisor privileged registers.
Something like this:
#include <asm/ultravisor-api.h>
#include <asm/asm-prototypes.h>
+#include <asm/reg.h>
int early_init_dt_scan_ultravisor(unsigned long node, const char *uname,
int depth, void *data);
+static inline void mtspr_ptcr(unsigned long val)
+{
+ /*
+ * If the ultravisor firmware is present, it maintains the partition
+ * table. PTCR becomes ultravisor privileged only for writing.
+ */
+ if (!firmware_has_feature(FW_FEATURE_ULTRAVISOR))
+ mtspr(SPRN_PTCR, val);
+}
* 64 K size.
*/
ptcr = __pa(partition_tb) | (PATB_SIZE_SHIFT - 12);
- mtspr(SPRN_PTCR, ptcr);
+ mtspr_ptcr(ptcr);
powernv_set_nmmu_ptcr(ptcr);
}
What do you think?
I don't think that's actually clearer.
If the logic was always:
if (ultravisor)
do_ucall()
else
mtspr()
Then a wrapper called eg. set_ptcr() would make sense.
But because in some cases you do a ucall and some you don't, I don't
think it helps to hide that in an accessor like above.
That is confusing to a reader who sees all this code to setup a value
and then the write to PTCR does nothing.
And in fact you didn't explain why it's OK for those cases to not do the
write at all.
An alternative could be to change the mtspr() and mfspr() macros as we
proposed in the v1, but access to non-ultravisor privileged registers would
be performance impacted because we always would need to check if the
register is one of the few ultravisor registers that the kernel needs to
access.
Yeah that and it would be very confusing to a reader who sees:
ptcr = ...;
mtspr(SPRN_PTCR, ptcr);
...
And then they discover the mtspr does *nothing* when the Ultravisor is
enabled.
cheers
From: Ram Pai <redacted>
Ultravisor is responsible for flushing the tlb cache, since it manages
the PATE entries. Hence skip tlb flush, if the ultravisor firmware is
available.
Signed-off-by: Ram Pai <redacted>
Signed-off-by: Claudio Carvalho <redacted>
---
arch/powerpc/mm/book3s64/pgtable.c | 33 +++++++++++++++++-------------
1 file changed, 19 insertions(+), 14 deletions(-)
@@ -224,6 +224,23 @@ void __init mmu_partition_table_init(void)powernv_set_nmmu_ptcr(ptcr);}+staticvoidflush_partition(unsignedintlpid,unsignedlongdw0)+{+if(dw0&PATB_HR){+asmvolatile(PPC_TLBIE_5(%0,%1,2,0,1)::+"r"(TLBIEL_INVAL_SET_LPID),"r"(lpid));+asmvolatile(PPC_TLBIE_5(%0,%1,2,1,1)::+"r"(TLBIEL_INVAL_SET_LPID),"r"(lpid));+trace_tlbie(lpid,0,TLBIEL_INVAL_SET_LPID,lpid,2,0,1);+}else{+asmvolatile(PPC_TLBIE_5(%0,%1,2,0,0)::+"r"(TLBIEL_INVAL_SET_LPID),"r"(lpid));+trace_tlbie(lpid,0,TLBIEL_INVAL_SET_LPID,lpid,2,0,0);+}+/* do we need fixup here ?*/+asmvolatile("eieio; tlbsync; ptesync":::"memory");+}+staticvoid__mmu_partition_table_set_entry(unsignedintlpid,unsignedlongdw0,unsignedlongdw1)
@@ -238,20 +255,8 @@ static void __mmu_partition_table_set_entry(unsigned int lpid,*Thetypeofflush(hashorradix)dependsonwhattheprevious*useofthispartitionIDwas,notthenewuse.*/-asmvolatile("ptesync":::"memory");-if(old&PATB_HR){-asmvolatile(PPC_TLBIE_5(%0,%1,2,0,1)::-"r"(TLBIEL_INVAL_SET_LPID),"r"(lpid));-asmvolatile(PPC_TLBIE_5(%0,%1,2,1,1)::-"r"(TLBIEL_INVAL_SET_LPID),"r"(lpid));-trace_tlbie(lpid,0,TLBIEL_INVAL_SET_LPID,lpid,2,0,1);-}else{-asmvolatile(PPC_TLBIE_5(%0,%1,2,0,0)::-"r"(TLBIEL_INVAL_SET_LPID),"r"(lpid));-trace_tlbie(lpid,0,TLBIEL_INVAL_SET_LPID,lpid,2,0,0);-}-/* do we need fixup here ?*/-asmvolatile("eieio; tlbsync; ptesync":::"memory");+if(!firmware_has_feature(FW_FEATURE_ULTRAVISOR))+flush_partition(lpid,old);}voidmmu_partition_table_set_entry(unsignedintlpid,unsignedlongdw0,
From: Ram Pai <redacted>
Ultravisor is responsible for flushing the tlb cache, since it manages
the PATE entries. Hence skip tlb flush, if the ultravisor firmware is
available.
Signed-off-by: Ram Pai <redacted>
Signed-off-by: Claudio Carvalho <redacted>
---
arch/powerpc/mm/book3s64/pgtable.c | 33 +++++++++++++++++-------------
1 file changed, 19 insertions(+), 14 deletions(-)
@@ -224,6 +224,23 @@ void __init mmu_partition_table_init(void)powernv_set_nmmu_ptcr(ptcr);}+staticvoidflush_partition(unsignedintlpid,unsignedlongdw0)+{+if(dw0&PATB_HR){+asmvolatile(PPC_TLBIE_5(%0,%1,2,0,1)::+"r"(TLBIEL_INVAL_SET_LPID),"r"(lpid));+asmvolatile(PPC_TLBIE_5(%0,%1,2,1,1)::+"r"(TLBIEL_INVAL_SET_LPID),"r"(lpid));+trace_tlbie(lpid,0,TLBIEL_INVAL_SET_LPID,lpid,2,0,1);+}else{+asmvolatile(PPC_TLBIE_5(%0,%1,2,0,0)::+"r"(TLBIEL_INVAL_SET_LPID),"r"(lpid));+trace_tlbie(lpid,0,TLBIEL_INVAL_SET_LPID,lpid,2,0,0);+}+/* do we need fixup here ?*/+asmvolatile("eieio; tlbsync; ptesync":::"memory");+}+staticvoid__mmu_partition_table_set_entry(unsignedintlpid,unsignedlongdw0,unsignedlongdw1)
@@ -238,20 +255,8 @@ static void __mmu_partition_table_set_entry(unsigned int lpid,*Thetypeofflush(hashorradix)dependsonwhattheprevious*useofthispartitionIDwas,notthenewuse.*/-asmvolatile("ptesync":::"memory");-if(old&PATB_HR){-asmvolatile(PPC_TLBIE_5(%0,%1,2,0,1)::-"r"(TLBIEL_INVAL_SET_LPID),"r"(lpid));-asmvolatile(PPC_TLBIE_5(%0,%1,2,1,1)::-"r"(TLBIEL_INVAL_SET_LPID),"r"(lpid));-trace_tlbie(lpid,0,TLBIEL_INVAL_SET_LPID,lpid,2,0,1);-}else{-asmvolatile(PPC_TLBIE_5(%0,%1,2,0,0)::-"r"(TLBIEL_INVAL_SET_LPID),"r"(lpid));-trace_tlbie(lpid,0,TLBIEL_INVAL_SET_LPID,lpid,2,0,0);-}-/* do we need fixup here ?*/-asmvolatile("eieio; tlbsync; ptesync":::"memory");+if(!firmware_has_feature(FW_FEATURE_ULTRAVISOR))
__mmu_partition_table_set_entry() checks for UV and
mmu_partition_table_set_entry() (the caller) checks for UV and the whole
point of having separate flush_partition() and
__mmu_partition_table_set_entry() is not really clear.
4/8 and 5/8 make more sense as one patch imho.
+ flush_partition(lpid, old);
}
void mmu_partition_table_set_entry(unsigned int lpid, unsigned long dw0,
From: Ram Pai <redacted>
Ultravisor is responsible for flushing the tlb cache, since it manages
the PATE entries. Hence skip tlb flush, if the ultravisor firmware is
available.
Signed-off-by: Ram Pai <redacted>
Signed-off-by: Claudio Carvalho <redacted>
---
arch/powerpc/mm/book3s64/pgtable.c | 33 +++++++++++++++++-------------
1 file changed, 19 insertions(+), 14 deletions(-)
@@ -224,6 +224,23 @@ void __init mmu_partition_table_init(void)powernv_set_nmmu_ptcr(ptcr);}+staticvoidflush_partition(unsignedintlpid,unsignedlongdw0)+{+if(dw0&PATB_HR){+asmvolatile(PPC_TLBIE_5(%0,%1,2,0,1)::+"r"(TLBIEL_INVAL_SET_LPID),"r"(lpid));+asmvolatile(PPC_TLBIE_5(%0,%1,2,1,1)::+"r"(TLBIEL_INVAL_SET_LPID),"r"(lpid));+trace_tlbie(lpid,0,TLBIEL_INVAL_SET_LPID,lpid,2,0,1);+}else{+asmvolatile(PPC_TLBIE_5(%0,%1,2,0,0)::+"r"(TLBIEL_INVAL_SET_LPID),"r"(lpid));+trace_tlbie(lpid,0,TLBIEL_INVAL_SET_LPID,lpid,2,0,0);+}+/* do we need fixup here ?*/+asmvolatile("eieio; tlbsync; ptesync":::"memory");+}+staticvoid__mmu_partition_table_set_entry(unsignedintlpid,unsignedlongdw0,unsignedlongdw1)
@@ -238,20 +255,8 @@ static void __mmu_partition_table_set_entry(unsigned int lpid,*Thetypeofflush(hashorradix)dependsonwhattheprevious*useofthispartitionIDwas,notthenewuse.*/-asmvolatile("ptesync":::"memory");-if(old&PATB_HR){-asmvolatile(PPC_TLBIE_5(%0,%1,2,0,1)::-"r"(TLBIEL_INVAL_SET_LPID),"r"(lpid));-asmvolatile(PPC_TLBIE_5(%0,%1,2,1,1)::-"r"(TLBIEL_INVAL_SET_LPID),"r"(lpid));-trace_tlbie(lpid,0,TLBIEL_INVAL_SET_LPID,lpid,2,0,1);-}else{-asmvolatile(PPC_TLBIE_5(%0,%1,2,0,0)::-"r"(TLBIEL_INVAL_SET_LPID),"r"(lpid));-trace_tlbie(lpid,0,TLBIEL_INVAL_SET_LPID,lpid,2,0,0);-}-/* do we need fixup here ?*/-asmvolatile("eieio; tlbsync; ptesync":::"memory");+if(!firmware_has_feature(FW_FEATURE_ULTRAVISOR))
__mmu_partition_table_set_entry() checks for UV and
mmu_partition_table_set_entry() (the caller) checks for UV and the whole
point of having separate flush_partition() and
__mmu_partition_table_set_entry() is not really clear.
4/8 and 5/8 make more sense as one patch imho.
Makes sense. I merged them in the next version. Thanks.
Claudio
quoted
+ flush_partition(lpid, old);
}
void mmu_partition_table_set_entry(unsigned int lpid, unsigned long dw0,
From: Ram Pai <redacted>
Ultravisor is responsible for flushing the tlb cache, since it manages
the PATE entries. Hence skip tlb flush, if the ultravisor firmware is
available.
Signed-off-by: Ram Pai <redacted>
Signed-off-by: Claudio Carvalho <redacted>
---
arch/powerpc/mm/book3s64/pgtable.c | 33 +++++++++++++++++-------------
1 file changed, 19 insertions(+), 14 deletions(-)
diff --git a/arch/powerpc/mm/book3s64/pgtable.c
b/arch/powerpc/mm/book3s64/pgtable.c
index 224c5c7c2e3d..bc8eb2bf9810 100644
@@ -224,6 +224,23 @@ void __init mmu_partition_table_init(void)powernv_set_nmmu_ptcr(ptcr);}+staticvoidflush_partition(unsignedintlpid,unsignedlongdw0)+{+if(dw0&PATB_HR){+asmvolatile(PPC_TLBIE_5(%0,%1,2,0,1)::+"r"(TLBIEL_INVAL_SET_LPID),"r"(lpid));+asmvolatile(PPC_TLBIE_5(%0,%1,2,1,1)::+"r"(TLBIEL_INVAL_SET_LPID),"r"(lpid));+trace_tlbie(lpid,0,TLBIEL_INVAL_SET_LPID,lpid,2,0,1);+}else{+asmvolatile(PPC_TLBIE_5(%0,%1,2,0,0)::+"r"(TLBIEL_INVAL_SET_LPID),"r"(lpid));+trace_tlbie(lpid,0,TLBIEL_INVAL_SET_LPID,lpid,2,0,0);+}+/* do we need fixup here ?*/+asmvolatile("eieio; tlbsync; ptesync":::"memory");+}+staticvoid__mmu_partition_table_set_entry(unsignedintlpid,unsignedlongdw0,unsignedlongdw1)
@@ -238,20 +255,8 @@ static void
__mmu_partition_table_set_entry(unsigned int lpid,
* The type of flush (hash or radix) depends on what the previous
* use of this partition ID was, not the new use.
*/
- asm volatile("ptesync" : : : "memory");
Doesn't the line above that was deleted need to be added to the
beginning of flush_partition()
On Mon, Jul 08, 2019 at 02:54:52PM -0500, janani wrote:
On 2019-06-28 15:08, Claudio Carvalho wrote:
quoted
From: Ram Pai <redacted>
Ultravisor is responsible for flushing the tlb cache, since it manages
the PATE entries. Hence skip tlb flush, if the ultravisor firmware is
available.
Signed-off-by: Ram Pai <redacted>
Signed-off-by: Claudio Carvalho <redacted>
---
arch/powerpc/mm/book3s64/pgtable.c | 33 +++++++++++++++++-------------
1 file changed, 19 insertions(+), 14 deletions(-)
diff --git a/arch/powerpc/mm/book3s64/pgtable.c
b/arch/powerpc/mm/book3s64/pgtable.c
index 224c5c7c2e3d..bc8eb2bf9810 100644
__mmu_partition_table_set_entry(unsigned int lpid,
* The type of flush (hash or radix) depends on what the previous
* use of this partition ID was, not the new use.
*/
- asm volatile("ptesync" : : : "memory");
Doesn't the line above that was deleted need to be added to the
beginning of flush_partition()
It has to. It got dropped erroneously.
This is a good catch!
Thanks,
RP
When the ultravisor firmware is available, it takes control over the
LDBAR register. In this case, thread-imc updates and save/restore
operations on the LDBAR register are handled by ultravisor.
Signed-off-by: Claudio Carvalho <redacted>
Reviewed-by: Ram Pai <redacted>
Reviewed-by: Ryan Grimm <redacted>
Acked-by: Madhavan Srinivasan <redacted>
Acked-by: Paul Mackerras <redacted>
---
arch/powerpc/kvm/book3s_hv_rmhandlers.S | 2 ++
arch/powerpc/platforms/powernv/idle.c | 6 ++++--
arch/powerpc/platforms/powernv/opal-imc.c | 4 ++++
3 files changed, 10 insertions(+), 2 deletions(-)
@@ -679,7 +679,8 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)sprs.ptcr=mfspr(SPRN_PTCR);sprs.rpr=mfspr(SPRN_RPR);sprs.tscr=mfspr(SPRN_TSCR);-sprs.ldbar=mfspr(SPRN_LDBAR);+if(!firmware_has_feature(FW_FEATURE_ULTRAVISOR))+sprs.ldbar=mfspr(SPRN_LDBAR);sprs_saved=true;
@@ -762,7 +763,8 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)mtspr(SPRN_PTCR,sprs.ptcr);mtspr(SPRN_RPR,sprs.rpr);mtspr(SPRN_TSCR,sprs.tscr);-mtspr(SPRN_LDBAR,sprs.ldbar);+if(!firmware_has_feature(FW_FEATURE_ULTRAVISOR))+mtspr(SPRN_LDBAR,sprs.ldbar);if(pls>=pnv_first_tb_loss_level){/* TB loss */
When the ultravisor firmware is available, it takes control over the
LDBAR register. In this case, thread-imc updates and save/restore
operations on the LDBAR register are handled by ultravisor.
What does LDBAR do? "Power ISA™ Version 3.0 B" or "User’s Manual POWER9
Processor" do not tell.
@@ -679,7 +679,8 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)sprs.ptcr=mfspr(SPRN_PTCR);sprs.rpr=mfspr(SPRN_RPR);sprs.tscr=mfspr(SPRN_TSCR);-sprs.ldbar=mfspr(SPRN_LDBAR);+if(!firmware_has_feature(FW_FEATURE_ULTRAVISOR))+sprs.ldbar=mfspr(SPRN_LDBAR);sprs_saved=true;
@@ -762,7 +763,8 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)mtspr(SPRN_PTCR,sprs.ptcr);mtspr(SPRN_RPR,sprs.rpr);mtspr(SPRN_TSCR,sprs.tscr);-mtspr(SPRN_LDBAR,sprs.ldbar);+if(!firmware_has_feature(FW_FEATURE_ULTRAVISOR))+mtspr(SPRN_LDBAR,sprs.ldbar);if(pls>=pnv_first_tb_loss_level){/* TB loss */
When the ultravisor firmware is available, it takes control over the
LDBAR register. In this case, thread-imc updates and save/restore
operations on the LDBAR register are handled by ultravisor.
What does LDBAR do? "Power ISA™ Version 3.0 B" or "User’s Manual POWER9
Processor" do not tell.
LDBAR is a per-thread SPR used by thread-imc pmu to dump the counter
data into memory.
LDBAR contains memory address along with few other configuration bits
(it is populated
by the thread-imc pmu driver). It is populated and enabled only when any
of the thread
imc pmu events are monitored.
Maddy
@@ -679,7 +679,8 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)sprs.ptcr=mfspr(SPRN_PTCR);sprs.rpr=mfspr(SPRN_RPR);sprs.tscr=mfspr(SPRN_TSCR);-sprs.ldbar=mfspr(SPRN_LDBAR);+if(!firmware_has_feature(FW_FEATURE_ULTRAVISOR))+sprs.ldbar=mfspr(SPRN_LDBAR);sprs_saved=true;
@@ -762,7 +763,8 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)mtspr(SPRN_PTCR,sprs.ptcr);mtspr(SPRN_RPR,sprs.rpr);mtspr(SPRN_TSCR,sprs.tscr);-mtspr(SPRN_LDBAR,sprs.ldbar);+if(!firmware_has_feature(FW_FEATURE_ULTRAVISOR))+mtspr(SPRN_LDBAR,sprs.ldbar);if(pls>=pnv_first_tb_loss_level){/* TB loss */
When the ultravisor firmware is available, it takes control over the
LDBAR register. In this case, thread-imc updates and save/restore
operations on the LDBAR register are handled by ultravisor.
What does LDBAR do? "Power ISA™ Version 3.0 B" or "User’s Manual POWER9
Processor" do not tell.
LDBAR is a per-thread SPR used by thread-imc pmu to dump the counter
data into memory.
LDBAR contains memory address along with few other configuration bits
(it is populated
by the thread-imc pmu driver). It is populated and enabled only when any
of the thread
imc pmu events are monitored.
I was actually looking for a spec for this register, what is the
document name?
@@ -254,6 +254,10 @@ static int opal_imc_counters_probe(struct
platform_device *pdev)
bool core_imc_reg = false, thread_imc_reg = false;
u32 type;
+ /* Disable IMC devices, when Ultravisor is enabled. */
+ if (firmware_has_feature(FW_FEATURE_ULTRAVISOR))
+ return -EACCES;
+
/*
* Check whether this is kdump kernel. If yes, force the
engines to
* stop and return.
On Mon, Jul 01, 2019 at 04:30:55PM +1000, Alexey Kardashevskiy wrote:
On 01/07/2019 16:17, maddy wrote:
quoted
On 01/07/19 11:24 AM, Alexey Kardashevskiy wrote:
quoted
On 29/06/2019 06:08, Claudio Carvalho wrote:
quoted
When the ultravisor firmware is available, it takes control over the
LDBAR register. In this case, thread-imc updates and save/restore
operations on the LDBAR register are handled by ultravisor.
What does LDBAR do? "Power ISA™ Version 3.0 B" or "User’s Manual POWER9
Processor" do not tell.
LDBAR is a per-thread SPR used by thread-imc pmu to dump the counter
data into memory.
LDBAR contains memory address along with few other configuration bits
(it is populated
by the thread-imc pmu driver). It is populated and enabled only when any
of the thread
imc pmu events are monitored.
I was actually looking for a spec for this register, what is the
document name?
Its not a architected register. Its documented in the Power9
workbook.
RP
On Mon, Jul 01, 2019 at 04:30:55PM +1000, Alexey Kardashevskiy wrote:
quoted
On 01/07/2019 16:17, maddy wrote:
quoted
On 01/07/19 11:24 AM, Alexey Kardashevskiy wrote:
quoted
On 29/06/2019 06:08, Claudio Carvalho wrote:
quoted
When the ultravisor firmware is available, it takes control over the
LDBAR register. In this case, thread-imc updates and save/restore
operations on the LDBAR register are handled by ultravisor.
What does LDBAR do? "Power ISA™ Version 3.0 B" or "User’s Manual POWER9
Processor" do not tell.
LDBAR is a per-thread SPR used by thread-imc pmu to dump the counter
data into memory.
LDBAR contains memory address along with few other configuration bits
(it is populated
by the thread-imc pmu driver). It is populated and enabled only when any
of the thread
imc pmu events are monitored.
I was actually looking for a spec for this register, what is the
document name?
Its not a architected register. Its documented in the Power9
workbook.
I also found some information about the LDBAR in
arch/powerpc/perf/imc-pmu.c
Claudio
When the ultravisor firmware is available, it takes control over the
LDBAR register. In this case, thread-imc updates and save/restore
operations on the LDBAR register are handled by ultravisor.
Signed-off-by: Claudio Carvalho <redacted>
Reviewed-by: Ram Pai <redacted>
Reviewed-by: Ryan Grimm <redacted>
Acked-by: Madhavan Srinivasan <redacted>
Acked-by: Paul Mackerras <redacted>
@@ -254,6 +254,10 @@ static int opal_imc_counters_probe(struct
platform_device *pdev)
bool core_imc_reg = false, thread_imc_reg = false;
u32 type;
+ /* Disable IMC devices, when Ultravisor is enabled. */
+ if (firmware_has_feature(FW_FEATURE_ULTRAVISOR))
+ return -EACCES;
+
/*
* Check whether this is kdump kernel. If yes, force the engines to
* stop and return.
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2019-07-11 13:11:06
Claudio Carvalho [off-list ref] writes:
When the ultravisor firmware is available, it takes control over the
LDBAR register. In this case, thread-imc updates and save/restore
operations on the LDBAR register are handled by ultravisor.
Please roll up the replies to Alexey's question about LDBAR into the
change log.
When the ultravisor firmware is available, it takes control over the
LDBAR register. In this case, thread-imc updates and save/restore
operations on the LDBAR register are handled by ultravisor.
Please roll up the replies to Alexey's question about LDBAR into the
change log.
@@ -254,6 +254,10 @@ static int opal_imc_counters_probe(struct platform_device *pdev)boolcore_imc_reg=false,thread_imc_reg=false;u32type;+/* Disable IMC devices, when Ultravisor is enabled. */+if(firmware_has_feature(FW_FEATURE_ULTRAVISOR))+return-EACCES;
I don't mind taking this change. But at the same time should the IMC
stuff just be omitted from the device tree when we're in ultravisor mode?
Yes. Maddy said that he will patch skiboot to remove the IMC nodes if
ultravisor is present.
I added this check just to protect the kernel in case skiboot is not in the
right level for some
reason.
Thanks,
Claudio
From: Michael Anderson <redacted>
- Check for MSR_S so that kvmppc_set_msr will include it. Prior to this
change return to guest would not have the S bit set.
- Patch based on comment from Paul Mackerras [off-list ref]
Signed-off-by: Michael Anderson <redacted>
Signed-off-by: Claudio Carvalho <redacted>
Acked-by: Paul Mackerras <redacted>
---
arch/powerpc/kvm/book3s_64_mmu_hv.c | 1 +
1 file changed, 1 insertion(+)
From: Michael Anderson <redacted>
- Check for MSR_S so that kvmppc_set_msr will include it. Prior to
this
change return to guest would not have the S bit set.
- Patch based on comment from Paul Mackerras [off-list ref]
Signed-off-by: Michael Anderson <redacted>
Signed-off-by: Claudio Carvalho <redacted>
Acked-by: Paul Mackerras <redacted>
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2019-07-11 13:19:20
Claudio Carvalho [off-list ref] writes:
From: Michael Anderson <redacted>
- Check for MSR_S so that kvmppc_set_msr will include it. Prior to this
change return to guest would not have the S bit set.
That sounds like it would be bad?
Please spell out what the practical impact of the patch is, ie.
somewhere on the spectrum from "without this patch everything catches
fire", to "this is not a bug but makes things clearer because ..."
cheers
quoted hunk
- Patch based on comment from Paul Mackerras [off-list ref]
Signed-off-by: Michael Anderson <redacted>
Signed-off-by: Claudio Carvalho <redacted>
Acked-by: Paul Mackerras <redacted>
---
arch/powerpc/kvm/book3s_64_mmu_hv.c | 1 +
1 file changed, 1 insertion(+)
From: Sukadev Bhattiprolu <redacted>
To enter a secure guest, we have to go through the ultravisor, therefore
we do a ucall when we are entering a secure guest.
This change is needed for any sort of entry to the secure guest from the
hypervisor, whether it is a return from an hcall, a return from a
hypervisor interrupt, or the first time that a secure guest vCPU is run.
If we are returning from an hcall, the results are already in the
appropriate registers R3:12, except for R3, R6 and R7. R3 has the status
of the reflected hcall, therefore we move it to R0 for the ultravisor and
set R3 to the UV_RETURN ucall number. R6,7 were used as temporary
registers, hence we restore them.
Have fast_guest_return check the kvm_arch.secure_guest field so that a
new CPU enters UV when started (in response to a RTAS start-cpu call).
Thanks to input from Paul Mackerras, Ram Pai and Mike Anderson.
Signed-off-by: Sukadev Bhattiprolu <redacted>
[ Pass SRR1 in r11 for UV_RETURN, fix kvmppc_msr_interrupt to preserve
the MSR_S bit ]
Signed-off-by: Paul Mackerras <redacted>
[ Fix UV_RETURN ucall number and arch.secure_guest check ]
Signed-off-by: Ram Pai <redacted>
[ Save the actual R3 in R0 for the ultravisor and use R3 for the
UV_RETURN ucall number. Update commit message and ret_to_ultra comment ]
Signed-off-by: Claudio Carvalho <redacted>
---
arch/powerpc/include/asm/kvm_host.h | 1 +
arch/powerpc/include/asm/ultravisor-api.h | 1 +
arch/powerpc/kernel/asm-offsets.c | 1 +
arch/powerpc/kvm/book3s_hv_rmhandlers.S | 40 +++++++++++++++++++----
4 files changed, 37 insertions(+), 6 deletions(-)
From: Sukadev Bhattiprolu <redacted>
To enter a secure guest, we have to go through the ultravisor,
therefore
we do a ucall when we are entering a secure guest.
This change is needed for any sort of entry to the secure guest from
the
hypervisor, whether it is a return from an hcall, a return from a
hypervisor interrupt, or the first time that a secure guest vCPU is
run.
If we are returning from an hcall, the results are already in the
appropriate registers R3:12, except for R3, R6 and R7. R3 has the
status
of the reflected hcall, therefore we move it to R0 for the ultravisor
and
set R3 to the UV_RETURN ucall number. R6,7 were used as temporary
registers, hence we restore them.
Have fast_guest_return check the kvm_arch.secure_guest field so that a
new CPU enters UV when started (in response to a RTAS start-cpu call).
Thanks to input from Paul Mackerras, Ram Pai and Mike Anderson.
Signed-off-by: Sukadev Bhattiprolu <redacted>
[ Pass SRR1 in r11 for UV_RETURN, fix kvmppc_msr_interrupt to preserve
the MSR_S bit ]
Signed-off-by: Paul Mackerras <redacted>
[ Fix UV_RETURN ucall number and arch.secure_guest check ]
Signed-off-by: Ram Pai <redacted>
[ Save the actual R3 in R0 for the ultravisor and use R3 for the
UV_RETURN ucall number. Update commit message and ret_to_ultra
comment ]
Signed-off-by: Claudio Carvalho <redacted>
to do
+ * that. If we are returning from a hcall, the results are already in
the
+ * appropriate registers R3:12, except for R3, R6 and R7. R3 has the
status of
+ * the reflected hcall, therefore we move it to R0 for the ultravisor
and set
+ * R3 to the UV_RETURN ucall number. R6,7 were used as temporary
registers
+ * above, hence we restore them.
+ */
+ret_to_ultra:
+ lwz r6, VCPU_CR(r4)
+ mtcr r6
+ mfspr r11, SPRN_SRR1
+ mr r0, r3
+ LOAD_REG_IMMEDIATE(r3, UV_RETURN)
+ ld r7, VCPU_GPR(R7)(r4)
+ ld r6, VCPU_GPR(R6)(r4)
+ ld r4, VCPU_GPR(R4)(r4)
+ sc 2
/*
* Enter the guest on a P9 or later system where we have exactly
@@ -3318,13 +3343,16 @@ END_MMU_FTR_SECTION_IFSET(MMU_FTR_TYPE_RADIX) * r0 is used as a scratch register */ kvmppc_msr_interrupt:+ andis. r0, r11, MSR_S@h rldicl r0, r11, 64 - MSR_TS_S_LG, 62- cmpwi r0, 2 /* Check if we are in transactional state.. */+ cmpwi cr1, r0, 2 /* Check if we are in transactional state.. */ ld r11, VCPU_INTR_MSR(r9)- bne 1f+ bne cr1, 1f /* ... if transactional, change to suspended */ li r0, 1 1: rldimi r11, r0, MSR_TS_S_LG, 63 - MSR_TS_T_LG+ beqlr+ oris r11, r11, MSR_S@h /* preserve MSR_S bit setting */ blr /*
From: Sukadev Bhattiprolu <redacted>
To enter a secure guest, we have to go through the ultravisor, therefore
we do a ucall when we are entering a secure guest.
This change is needed for any sort of entry to the secure guest from the
hypervisor, whether it is a return from an hcall, a return from a
hypervisor interrupt, or the first time that a secure guest vCPU is run.
If we are returning from an hcall, the results are already in the
appropriate registers R3:12, except for R3, R6 and R7. R3 has the status
of the reflected hcall, therefore we move it to R0 for the ultravisor and
set R3 to the UV_RETURN ucall number. R6,7 were used as temporary
registers, hence we restore them.
Have fast_guest_return check the kvm_arch.secure_guest field so that a
new CPU enters UV when started (in response to a RTAS start-cpu call).
Thanks to input from Paul Mackerras, Ram Pai and Mike Anderson.
Signed-off-by: Sukadev Bhattiprolu <redacted>
[ Pass SRR1 in r11 for UV_RETURN, fix kvmppc_msr_interrupt to preserve
the MSR_S bit ]
Signed-off-by: Paul Mackerras <redacted>
[ Fix UV_RETURN ucall number and arch.secure_guest check ]
Signed-off-by: Ram Pai <redacted>
[ Save the actual R3 in R0 for the ultravisor and use R3 for the
UV_RETURN ucall number. Update commit message and ret_to_ultra comment ]
Signed-off-by: Claudio Carvalho <redacted>
mtspr SPRN_HDSISR, r0
END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
+ ld r6, VCPU_KVM(r4)
+ lbz r7, KVM_SECURE_GUEST(r6)
+ cmpdi r7, 0
+ bne ret_to_ultra
+
+ lwz r6, VCPU_CR(r4)
+ mtcr r6
+
+ ld r7, VCPU_GPR(R7)(r4)
+ ld r6, VCPU_GPR(R6)(r4)
ld r0, VCPU_GPR(R0)(r4)
ld r4, VCPU_GPR(R4)(r4)
HRFI_TO_GUEST
b .
+/*
+ * We are entering a secure guest, so we have to invoke the ultravisor
to do
+ * that. If we are returning from a hcall, the results are already in the
+ * appropriate registers R3:12, except for R3, R6 and R7. R3 has the
status of
+ * the reflected hcall, therefore we move it to R0 for the ultravisor
and set
+ * R3 to the UV_RETURN ucall number. R6,7 were used as temporary registers
+ * above, hence we restore them.
+ */
+ret_to_ultra:
+ lwz r6, VCPU_CR(r4)
+ mtcr r6
+ mfspr r11, SPRN_SRR1
+ mr r0, r3
+ LOAD_REG_IMMEDIATE(r3, UV_RETURN)
+ ld r7, VCPU_GPR(R7)(r4)
+ ld r6, VCPU_GPR(R6)(r4)
+ ld r4, VCPU_GPR(R4)(r4)
+ sc 2
/*
* Enter the guest on a P9 or later system where we have exactly
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2019-07-11 13:15:33
Claudio Carvalho [off-list ref] writes:
From: Sukadev Bhattiprolu <redacted>
To enter a secure guest, we have to go through the ultravisor, therefore
we do a ucall when we are entering a secure guest.
This change is needed for any sort of entry to the secure guest from the
hypervisor, whether it is a return from an hcall, a return from a
hypervisor interrupt, or the first time that a secure guest vCPU is run.
If we are returning from an hcall, the results are already in the
appropriate registers R3:12, except for R3, R6 and R7. R3 has the status
of the reflected hcall, therefore we move it to R0 for the ultravisor and
set R3 to the UV_RETURN ucall number. R6,7 were used as temporary
registers, hence we restore them.
This is another case where some documentation would help people to
review the code.
quoted hunk
Have fast_guest_return check the kvm_arch.secure_guest field so that a
new CPU enters UV when started (in response to a RTAS start-cpu call).
Thanks to input from Paul Mackerras, Ram Pai and Mike Anderson.
Signed-off-by: Sukadev Bhattiprolu <redacted>
[ Pass SRR1 in r11 for UV_RETURN, fix kvmppc_msr_interrupt to preserve
the MSR_S bit ]
Signed-off-by: Paul Mackerras <redacted>
[ Fix UV_RETURN ucall number and arch.secure_guest check ]
Signed-off-by: Ram Pai <redacted>
[ Save the actual R3 in R0 for the ultravisor and use R3 for the
UV_RETURN ucall number. Update commit message and ret_to_ultra comment ]
Signed-off-by: Claudio Carvalho <redacted>
---
arch/powerpc/include/asm/kvm_host.h | 1 +
arch/powerpc/include/asm/ultravisor-api.h | 1 +
arch/powerpc/kernel/asm-offsets.c | 1 +
arch/powerpc/kvm/book3s_hv_rmhandlers.S | 40 +++++++++++++++++++----
4 files changed, 37 insertions(+), 6 deletions(-)
+ bne ret_to_ultra
+
+ lwz r6, VCPU_CR(r4)
+ mtcr r6
+
+ ld r7, VCPU_GPR(R7)(r4)
+ ld r6, VCPU_GPR(R6)(r4)
ld r0, VCPU_GPR(R0)(r4)
ld r4, VCPU_GPR(R4)(r4)
HRFI_TO_GUEST
b .
+/*
+ * We are entering a secure guest, so we have to invoke the ultravisor to do
+ * that. If we are returning from a hcall, the results are already in the
+ * appropriate registers R3:12, except for R3, R6 and R7. R3 has the status of
+ * the reflected hcall, therefore we move it to R0 for the ultravisor and set
+ * R3 to the UV_RETURN ucall number. R6,7 were used as temporary registers
+ * above, hence we restore them.
+ */
+ret_to_ultra:
+ lwz r6, VCPU_CR(r4)
+ mtcr r6
+ mfspr r11, SPRN_SRR1
+ mr r0, r3
+ LOAD_REG_IMMEDIATE(r3, UV_RETURN)
Worth open coding to save three instructions?
quoted hunk
+ ld r7, VCPU_GPR(R7)(r4)
+ ld r6, VCPU_GPR(R6)(r4)
+ ld r4, VCPU_GPR(R4)(r4)
+ sc 2
/*
* Enter the guest on a P9 or later system where we have exactly
@@ -3318,13 +3343,16 @@ END_MMU_FTR_SECTION_IFSET(MMU_FTR_TYPE_RADIX) * r0 is used as a scratch register */ kvmppc_msr_interrupt:+ andis. r0, r11, MSR_S@h rldicl r0, r11, 64 - MSR_TS_S_LG, 62- cmpwi r0, 2 /* Check if we are in transactional state.. */+ cmpwi cr1, r0, 2 /* Check if we are in transactional state.. */ ld r11, VCPU_INTR_MSR(r9)- bne 1f+ bne cr1, 1f /* ... if transactional, change to suspended */ li r0, 1 1: rldimi r11, r0, MSR_TS_S_LG, 63 - MSR_TS_T_LG+ beqlr+ oris r11, r11, MSR_S@h /* preserve MSR_S bit setting */ blr
I don't see this part mentioned in the change log?
It's also pretty subtle, a comment might be helpful.
cheers
From: Sukadev Bhattiprolu <redacted>
To enter a secure guest, we have to go through the ultravisor, therefore
we do a ucall when we are entering a secure guest.
This change is needed for any sort of entry to the secure guest from the
hypervisor, whether it is a return from an hcall, a return from a
hypervisor interrupt, or the first time that a secure guest vCPU is run.
If we are returning from an hcall, the results are already in the
appropriate registers R3:12, except for R3, R6 and R7. R3 has the status
of the reflected hcall, therefore we move it to R0 for the ultravisor and
set R3 to the UV_RETURN ucall number. R6,7 were used as temporary
registers, hence we restore them.
This is another case where some documentation would help people to
review the code.
quoted
Have fast_guest_return check the kvm_arch.secure_guest field so that a
new CPU enters UV when started (in response to a RTAS start-cpu call).
Thanks to input from Paul Mackerras, Ram Pai and Mike Anderson.
Signed-off-by: Sukadev Bhattiprolu <redacted>
[ Pass SRR1 in r11 for UV_RETURN, fix kvmppc_msr_interrupt to preserve
the MSR_S bit ]
Signed-off-by: Paul Mackerras <redacted>
[ Fix UV_RETURN ucall number and arch.secure_guest check ]
Signed-off-by: Ram Pai <redacted>
[ Save the actual R3 in R0 for the ultravisor and use R3 for the
UV_RETURN ucall number. Update commit message and ret_to_ultra comment ]
Signed-off-by: Claudio Carvalho <redacted>
---
arch/powerpc/include/asm/kvm_host.h | 1 +
arch/powerpc/include/asm/ultravisor-api.h | 1 +
arch/powerpc/kernel/asm-offsets.c | 1 +
arch/powerpc/kvm/book3s_hv_rmhandlers.S | 40 +++++++++++++++++++----
4 files changed, 37 insertions(+), 6 deletions(-)
we could move 'ld r7' here. r6 is used to restore CR below so
it (r6) has to stay there?
quoted
+ bne ret_to_ultra
+
+ lwz r6, VCPU_CR(r4)
+ mtcr r6
+
+ ld r7, VCPU_GPR(R7)(r4)
+ ld r6, VCPU_GPR(R6)(r4)
ld r0, VCPU_GPR(R0)(r4)
ld r4, VCPU_GPR(R4)(r4)
HRFI_TO_GUEST
b .
+/*
+ * We are entering a secure guest, so we have to invoke the ultravisor to do
+ * that. If we are returning from a hcall, the results are already in the
+ * appropriate registers R3:12, except for R3, R6 and R7. R3 has the status of
+ * the reflected hcall, therefore we move it to R0 for the ultravisor and set
+ * R3 to the UV_RETURN ucall number. R6,7 were used as temporary registers
+ * above, hence we restore them.
+ */
+ret_to_ultra:
+ lwz r6, VCPU_CR(r4)
+ mtcr r6
+ mfspr r11, SPRN_SRR1
+ mr r0, r3
+ LOAD_REG_IMMEDIATE(r3, UV_RETURN)
Worth open coding to save three instructions?
Yes, good point:
- LOAD_REG_IMMEDIATE(r3, UV_RETURN)
+
+ li r3, 0
+ oris r3, r3, (UV_RETURN)@__AS_ATHIGH
+ ori r3, r3, (UV_RETURN)@l
+
quoted
+ ld r7, VCPU_GPR(R7)(r4)
+ ld r6, VCPU_GPR(R6)(r4)
+ ld r4, VCPU_GPR(R4)(r4)
+ sc 2
/*
* Enter the guest on a P9 or later system where we have exactly
@@ -3318,13 +3343,16 @@ END_MMU_FTR_SECTION_IFSET(MMU_FTR_TYPE_RADIX) * r0 is used as a scratch register */ kvmppc_msr_interrupt:+ andis. r0, r11, MSR_S@h rldicl r0, r11, 64 - MSR_TS_S_LG, 62- cmpwi r0, 2 /* Check if we are in transactional state.. */+ cmpwi cr1, r0, 2 /* Check if we are in transactional state.. */ ld r11, VCPU_INTR_MSR(r9)- bne 1f+ bne cr1, 1f /* ... if transactional, change to suspended */ li r0, 1 1: rldimi r11, r0, MSR_TS_S_LG, 63 - MSR_TS_T_LG+ beqlr+ oris r11, r11, MSR_S@h /* preserve MSR_S bit setting */ blr
I don't see this part mentioned in the change log?
It's also pretty subtle, a comment might be helpful.
cheers
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2019-07-22 11:07:23
Sukadev Bhattiprolu [off-list ref] writes:
Michael Ellerman [mpe@ellerman.id.au] wrote:
quoted
Claudio Carvalho [off-list ref] writes:
quoted
From: Sukadev Bhattiprolu <redacted>
To enter a secure guest, we have to go through the ultravisor, therefore
we do a ucall when we are entering a secure guest.
This change is needed for any sort of entry to the secure guest from the
hypervisor, whether it is a return from an hcall, a return from a
hypervisor interrupt, or the first time that a secure guest vCPU is run.
If we are returning from an hcall, the results are already in the
appropriate registers R3:12, except for R3, R6 and R7. R3 has the status
of the reflected hcall, therefore we move it to R0 for the ultravisor and
set R3 to the UV_RETURN ucall number. R6,7 were used as temporary
registers, hence we restore them.
This is another case where some documentation would help people to
review the code.
quoted
Have fast_guest_return check the kvm_arch.secure_guest field so that a
new CPU enters UV when started (in response to a RTAS start-cpu call).
Thanks to input from Paul Mackerras, Ram Pai and Mike Anderson.
Signed-off-by: Sukadev Bhattiprolu <redacted>
[ Pass SRR1 in r11 for UV_RETURN, fix kvmppc_msr_interrupt to preserve
the MSR_S bit ]
Signed-off-by: Paul Mackerras <redacted>
[ Fix UV_RETURN ucall number and arch.secure_guest check ]
Signed-off-by: Ram Pai <redacted>
[ Save the actual R3 in R0 for the ultravisor and use R3 for the
UV_RETURN ucall number. Update commit message and ret_to_ultra comment ]
Signed-off-by: Claudio Carvalho <redacted>
---
arch/powerpc/include/asm/kvm_host.h | 1 +
arch/powerpc/include/asm/ultravisor-api.h | 1 +
arch/powerpc/kernel/asm-offsets.c | 1 +
arch/powerpc/kvm/book3s_hv_rmhandlers.S | 40 +++++++++++++++++++----
4 files changed, 37 insertions(+), 6 deletions(-)
we could move 'ld r7' here. r6 is used to restore CR below so
it (r6) has to stay there?
It's used to restore CR in both paths, so both paths load VCPU_CR(r4)
into r6. So we could instead do that load once, before the branch?
quoted
quoted
+ bne ret_to_ultra
+
+ lwz r6, VCPU_CR(r4)
+ mtcr r6
+
+ ld r7, VCPU_GPR(R7)(r4)
+ ld r6, VCPU_GPR(R6)(r4)
ld r0, VCPU_GPR(R0)(r4)
ld r4, VCPU_GPR(R4)(r4)
HRFI_TO_GUEST
b .
+/*
+ * We are entering a secure guest, so we have to invoke the ultravisor to do
+ * that. If we are returning from a hcall, the results are already in the
+ * appropriate registers R3:12, except for R3, R6 and R7. R3 has the status of
+ * the reflected hcall, therefore we move it to R0 for the ultravisor and set
+ * R3 to the UV_RETURN ucall number. R6,7 were used as temporary registers
+ * above, hence we restore them.
+ */
+ret_to_ultra:
+ lwz r6, VCPU_CR(r4)
+ mtcr r6
+ mfspr r11, SPRN_SRR1
+ mr r0, r3
+ LOAD_REG_IMMEDIATE(r3, UV_RETURN)
Worth open coding to save three instructions?
Yes, good point:
- LOAD_REG_IMMEDIATE(r3, UV_RETURN)
+
+ li r3, 0
+ oris r3, r3, (UV_RETURN)@__AS_ATHIGH
+ ori r3, r3, (UV_RETURN)@l
This should do it no?
li r3, 0
oris r3, r3, UV_RETURN
cheers
From: Nicholas Piggin <npiggin@gmail.com> Date: 2019-07-12 02:08:34
Claudio Carvalho's on June 29, 2019 6:08 am:
quoted hunk
From: Sukadev Bhattiprolu <redacted>
To enter a secure guest, we have to go through the ultravisor, therefore
we do a ucall when we are entering a secure guest.
This change is needed for any sort of entry to the secure guest from the
hypervisor, whether it is a return from an hcall, a return from a
hypervisor interrupt, or the first time that a secure guest vCPU is run.
If we are returning from an hcall, the results are already in the
appropriate registers R3:12, except for R3, R6 and R7. R3 has the status
of the reflected hcall, therefore we move it to R0 for the ultravisor and
set R3 to the UV_RETURN ucall number. R6,7 were used as temporary
registers, hence we restore them.
Have fast_guest_return check the kvm_arch.secure_guest field so that a
new CPU enters UV when started (in response to a RTAS start-cpu call).
Thanks to input from Paul Mackerras, Ram Pai and Mike Anderson.
Signed-off-by: Sukadev Bhattiprolu <redacted>
[ Pass SRR1 in r11 for UV_RETURN, fix kvmppc_msr_interrupt to preserve
the MSR_S bit ]
Signed-off-by: Paul Mackerras <redacted>
[ Fix UV_RETURN ucall number and arch.secure_guest check ]
Signed-off-by: Ram Pai <redacted>
[ Save the actual R3 in R0 for the ultravisor and use R3 for the
UV_RETURN ucall number. Update commit message and ret_to_ultra comment ]
Signed-off-by: Claudio Carvalho <redacted>
---
arch/powerpc/include/asm/kvm_host.h | 1 +
arch/powerpc/include/asm/ultravisor-api.h | 1 +
arch/powerpc/kernel/asm-offsets.c | 1 +
arch/powerpc/kvm/book3s_hv_rmhandlers.S | 40 +++++++++++++++++++----
4 files changed, 37 insertions(+), 6 deletions(-)
Just to try to be less arbitrary about things, could you use regs
adjacent to r4? Generally good because then it has a chance to get
our loads paired up (which may not help some CPUs).
Thanks,
Nick