[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v1 03/15] arm: Placeholder for handling Group0/1 traps for Cavium Erratum 30115
Hi Manish,
On 03/16/2018 11:58 AM, Manish Jaggi wrote:
Since this is a SoC errata and trapping of certain group1 registers
should not affect the normal flow. A new file vgic-v3-sr.c is added.
You trap both group1 and group0. But your first sentence is a bit
difficult to understand. How about:
"The errata will require to emulate the GIC virtual CPU interface in
Xen. Because the hypervisor will update its internal state of the vGIC,
we want to avoid messing up with it. So the errata is handled separately
from the rest of the hypervisor."
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
The wrapping of the commit message looks wrong.
that leave_hypervisor_tail not to be called when handling group1 traps
under this errata.
No the flag is much more generic than what you claim here. The flag is
used to skip the hypervisor_tail when enter_hypervisor_head. Your trap
handling is one of the user.
Technically this is 2 distinct features, one is using the other and
should be in separate patches. I am ok to keep in a single patch, but
you should get the commit message right.
enter_hypervisor_head is not invoked when workaround
30115 is in place. enter_hypervisor_head and leave_hypervisor_tail are
invoked in sync, if one is not called other one should be skipped,
otherwise guest vGIC state be out-of-date.
Signed-off-by: Manish Jaggi <manish.jaggi@xxxxxxxxxx>
diff --git a/xen/arch/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile
index 718fe44455..02cc115239 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) += vgic-v3-sr.o
diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
new file mode 100644
index 0000000000..56b02fd45b
--- /dev/null
+++ b/xen/arch/arm/arm64/vgic-v3-sr.c
@@ -0,0 +1,56 @@
+/*
+ * xen/arch/arm/arm64/vgic-v3-sr.c
+ *
+ * Code to handle Cavium Erratum 30115
+ *
+ * Manish Jaggi <manish.jaggi@xxxxxxxxxx>
+ * Copyright (c) 2018 Cavium.
+ *
+ * Ths program is free software; you can redistribute it and/or
+ * modify it under the terms and conditions of the GNU General Public
+ * License, version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#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 = true;
+
+ /* Disabling interrupts to prevent change in guest state */
+ local_irq_disable();
+ if ( hsr.ec != HSR_EC_SYSREG )
+ {
+ ret = false;
+ goto end;
+ }
+
+ switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
+ {
+ default:
+ ret = false;
+ break;
+ }
+end:
+ local_irq_enable();
+
+ return ret;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
+
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f6f6de3691..25778018fb 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2103,6 +2103,27 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
{
const union hsr hsr = { .bits = regs->hsr };
+ if ( check_workaround_30115() )
+ {
+ bool ret;
newline here. But ...
+ ret = vgic_v3_handle_cpuif_access(regs, hsr);
I don't think the separate variable is required.
You could just do if (vgic_v3_handle_cpuif_access(regs, hsr))...
+ if ( ret )
+ {
+ /* if true, g0/g1 vgic register trap is emulated for errata
+ * so rest of handling of do_trap_guest_sync is not required.
+ */
Please follow Xen coding style comments:
/*
* If ...
The if should also have the first letter uppercase and it would be
better to say "group" rather than "g". It is much clearer.
+ advance_pc(regs, hsr);
+ /*
+ * enter_hypervisor_head is not invoked when workaround 30115
+ * is in place. enter_hypervisor_head and leave_hypervisor_tail
+ * are invoked in sync, if one is not called other one should be
+ * skipped, otherwise guest vGIC state be out-of-date.
+ */
+ get_cpu_info()->skip_hyp_tail = true;
+ return;
+ }
+ }
+
enter_hypervisor_head(regs);
switch (hsr.ec) {
@@ -2295,6 +2316,16 @@ void do_trap_fiq(struct cpu_user_regs *regs)
void leave_hypervisor_tail(void)
{
+ /*
+ * if skip_hyp_tail is set simply retrun;
+ */
No can use the single line comment style here. And s/retrun/return/.
+ if ( unlikely(get_cpu_info()->skip_hyp_tail) )
+ {
+ /* clear it */
Well, that pretty obvious from the code. It would be much nicer to state
the less obvious, i.e why you clear skip_hyp_tail here.
+ get_cpu_info()->skip_hyp_tail = false;
+ return;
+ }
+
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..45fe582abd 100644
--- a/xen/include/asm-arm/arm64/traps.h
+++ b/xen/include/asm-arm/arm64/traps.h
@@ -3,6 +3,9 @@
void inject_undef64_exception(struct cpu_user_regs *regs, int instr_len);
+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..d7b3f4ddb4 100644
--- a/xen/include/asm-arm/current.h
+++ b/xen/include/asm-arm/current.h
@@ -21,7 +21,8 @@ DECLARE_PER_CPU(struct vcpu *, curr_vcpu);
struct cpu_info {
struct cpu_user_regs guest_cpu_user_regs;
unsigned long elr;
- unsigned int pad;
+ unsigned int pad:31;
+ bool skip_hyp_tail:1;
Firstly, please switch pad and skip_hyp_tail. We want to keep the
padding towards the end of the structure.
Furthermore, when I asked comments, it was not why you set skip_hyp_tail
= true on the erratum. It was on the field, so developer can understand
how this field can been used.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
