[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [RFC PATCH 02/10] arm64: Add hook to handle guest GICv3 sysreg accesses

Hi Manish,

On 16/01/18 15:42, mjaggi@xxxxxxxxxxxxxxxxxx wrote:
From: Manish Jaggi <manish.jaggi@xxxxxxxxxx>

In order to start handling guest access to GICv3 system registers,
let's add a hook that will get called when we trap a system register
This handling code is kept independent of other traps.
Set CONFIG_VGIC_ERRATA to enable this code.

The commit message should explain the rationale behind calling do_fixup_vgic_errata from do_trap_guest_sync.

Signed-off-by: Manish Jaggi <manish.jaggi@xxxxxxxxxx>
  xen/arch/arm/arm64/vsysreg.c      | 17 +++++++++++++++++
  xen/arch/arm/traps.c              | 11 +++++++++++
  xen/include/asm-arm/arm64/traps.h |  2 ++
  3 files changed, 30 insertions(+)

diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
index c57ac12503..a5c17e460f 100644
--- a/xen/arch/arm/arm64/vsysreg.c
+++ b/xen/arch/arm/arm64/vsysreg.c
@@ -219,6 +219,23 @@ void do_sysreg(struct cpu_user_regs *regs,
      regs->pc += 4;
+int do_fixup_vgic_errata(struct cpu_user_regs *regs, const union hsr hsr)

Can we have a separate file to emulate vgic system registers? We might want to re-use some of the code for 32-bit guests in the future. Also as far as I can tell, this will only cover GICv3 system registers. To be more specific it will only be cpu interface register.

So I think you should name vgic_v3_handle_cpuif_access(...).

+    int ret = 0;

You are returning either -1 or 0. Can we please use bool in that case?

+    local_irq_disable();
+    switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
You will enter in do_trap_guest_sync for various reasons. So you should check that exception class correspond to a trapped of system instructions (e.g HSR_EC_SYSREG). Otherwise you will emulate sysreg on other encoding by mistake.

+    {
+    default:
+        ret = -1;
+        break;
+    }
+    local_irq_enable();
+    return ret;
   * Local variables:
   * mode: C
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f6f6de3691..d4f0581d33 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2103,6 +2103,17 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
      const union hsr hsr = { .bits = regs->hsr };
+    int ret;
+    ret  = do_fixup_vgic_errata(regs,hsr);

Most likely distros will have the errata built in Xen because they want the hypervisor to run on many platforms. And we don't want to affect the on platform that are not affected by the errata.

So we want to skip the call. Have a look at CHECK_WORKAROUND_HELPER.

+    if ( !ret )
+    {
+        advance_pc(regs, hsr);
+        return;
+    }
switch (hsr.ec) {
diff --git a/xen/include/asm-arm/arm64/traps.h 
index 2379b578cb..7378a1b022 100644
--- a/xen/include/asm-arm/arm64/traps.h
+++ b/xen/include/asm-arm/arm64/traps.h
@@ -5,6 +5,8 @@ void inject_undef64_exception(struct cpu_user_regs *regs, int 
void do_sysreg(struct cpu_user_regs *regs,
                 const union hsr hsr);
+int do_fixup_vgic_errata(struct cpu_user_regs *regs,
+               const union hsr hsr);
#endif /* __ASM_ARM64_TRAPS__ */


Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.