Hi Manish,
On 12/03/18 12:42, [email protected] wrote:
From: Manish Jaggi <[email protected]>
Since this is a SoC errata and trapping of certain group1 registers
should not affect the normal flow. A new file vsysreg_errata.c is added.
Function vgic_v3_handle_cpuif_access is called from do_trap_guest_sync
if ARM64_WORKAROUND_CAVIUM_30115 capability is found.
A flag skip_hyp_tail is introduced in struct cpu_info. This flag specifies
that leave_hypervisor_tail not to be called when handling group1 traps
under this errata.
Please give some rationale in the commit message why
leave_hypervisor_tail is skipped on the errata.
Signed-off-by: Manish Jaggi <[email protected]>
---
xen/arch/arm/arm64/Makefile | 1 +
xen/arch/arm/arm64/vsysreg_errata.c | 28 ++++++++++++++++++++++++++++
The name of the file does not make sense, the errata is not about
sysreg. It is about vGIC. Please rename it to vgic-v3-sr.c.
xen/arch/arm/traps.c | 20 ++++++++++++++++++++
xen/include/asm-arm/arm64/traps.h | 3 ++-
xen/include/asm-arm/current.h | 1 +
5 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile
index 718fe44455..19440c3d8c 100644
--- a/xen/arch/arm/arm64/Makefile
+++ b/xen/arch/arm/arm64/Makefile
@@ -11,3 +11,4 @@ obj-y += smpboot.o
obj-y += traps.o
obj-y += vfp.o
obj-y += vsysreg.o
+obj-$(CONFIG_CAVIUM_ERRATUM_30115) += vsysreg_errata.o
diff --git a/xen/arch/arm/arm64/vsysreg_errata.c
b/xen/arch/arm/arm64/vsysreg_errata.c
new file mode 100644
index 0000000000..6af162bdf7
--- /dev/null
+++ b/xen/arch/arm/arm64/vsysreg_errata.c
@@ -0,0 +1,28 @@
Missing copyright header.
+#include <asm/current.h>
+#include <asm/regs.h>
+#include <asm/traps.h>
+#include <asm/system.h>
+
+bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs, const union hsr
hsr)
+{
+ bool ret = 0;
You should use false/true when using bool. No plain integer.
+
+ local_irq_disable();
Please add a comment explain why IRQs are disabled.
+ if ( hsr.ec != HSR_EC_SYSREG )
+ {
+ ret = 1;
+ goto end;
+ }
+
+ switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
+ {
+ default:
+ ret = 1;
+ break;
+ }
+end:
+ local_irq_enable();
+
+ return ret;
+}
+
Missing emacs magic.
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f6f6de3691..9d08cd6ad3 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -40,6 +40,7 @@
#include <asm/acpi.h>
#include <asm/cpuerrata.h>
#include <asm/cpufeature.h>
+#include <asm/cpuerrata.h>
#include <asm/debugger.h>
#include <asm/event.h>
#include <asm/flushtlb.h>
@@ -2103,6 +2104,21 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
{
const union hsr hsr = { .bits = regs->hsr };
+#ifdef CONFIG_CAVIUM_ERRATUM_30115
I am not a big fan of #ifdef in the code. I think it would be better to
stub the vgic_v3_handle_cpuif_access.
+ if ( cpus_have_cap(ARM64_WORKAROUND_CAVIUM_30115) )
cpus_have_cap is expensive to use in hot path. This is because the
function is going to check is the bits is set on every exit. You want to
use check_workaround_* for that purpose as this will be replaced by an
alternative.
+ {
+ int ret;
vgic_v3_handle_cpuif_access is returning a bool.
+ get_cpu_info()->skip_hyp_tail = 0;
Same remark as above about bool and integer. But I think this one is not
necessary. skip_hyp_tail is going to be false by default. So if you
reset to false in the return path when it is true, you avoid
+ ret = vgic_v3_handle_cpuif_access(regs, hsr);
+ if ( !ret )
+ {
+ advance_pc(regs, hsr);
+ get_cpu_info()->skip_hyp_tail = 1;
true.
+ return;
+ }
+ }
+#endif
+
enter_hypervisor_head(regs);
switch (hsr.ec) {
@@ -2295,6 +2311,10 @@ void do_trap_fiq(struct cpu_user_regs *regs)
void leave_hypervisor_tail(void)
{
+#ifdef CONFIG_CAVIUM_ERRATUM_30115
I don't think the #ifdef is necessary here. Supporting to skip the
hypervisor tail is a nice feature to have.
+ if ( get_cpu_info()->skip_hyp_tail )
You want this to be an unlikely(...).
+ return;
+#endif
while (1)
{
local_irq_disable();
diff --git a/xen/include/asm-arm/arm64/traps.h
b/xen/include/asm-arm/arm64/traps.h
index 2379b578cb..a5ae93ec11 100644
--- a/xen/include/asm-arm/arm64/traps.h
+++ b/xen/include/asm-arm/arm64/traps.h
@@ -2,7 +2,8 @@
#define __ASM_ARM64_TRAPS__
void inject_undef64_exception(struct cpu_user_regs *regs, int instr_len);
-
Why removing the newline?
+bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs,
+ const union hsr hsr);
void do_sysreg(struct cpu_user_regs *regs,
const union hsr hsr);
diff --git a/xen/include/asm-arm/current.h b/xen/include/asm-arm/current.h
index 7a0971fdea..dacf3adc85 100644
--- a/xen/include/asm-arm/current.h
+++ b/xen/include/asm-arm/current.h
@@ -22,6 +22,7 @@ struct cpu_info {
struct cpu_user_regs guest_cpu_user_regs;
unsigned long elr;
unsigned int pad;
+ bool skip_hyp_tail;
You should just reuse some bits of the padding (i.e 'pad' field) here.
bool skip_hyp_tail:1;
unsigned int pad:31;
Also, some documentation of the code would be highly appreciated.
};
static inline struct cpu_info *get_cpu_info(void)
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
