arm64: Mediate access to GICv3 sysregs at EL2
cover arm64: Mediate access to GICv3 sysregs at EL2
This patchset is a Xen port of Marc's patchset.
arm64: KVM: Mediate access to GICv3 sysregs at EL2 [1]
The current RFC patchset is a subset of [1], as it handleing only Group1 traps as a PoC. Most of the trap code is added in vsysreg.c. Trap handler function is kept independent of the usual guest trap handling code.
Looking for feedback on this approach.
The errata has been validated on Cavium ThunderX platform.
Steps to reporduce the errata
- Boot Xen with 2 cores.
- Disable group1 interrupts in domU kernel
- start domU, the kill and start again.
One of the Xen core would hang.
Code in this patchset fixes this issue.
[1] https://lists.cs.columbia.edu/pipermail/kvmarm/2017-June/026029.html
Changes from v0
- Added Group0 traps.
- Some cleanups and documentation
Changes from RFC
- Added commit information on ported patches from linux
- Added skip_hyp_tail to control calling leave_hypervisor_tail
- Added CAVIUM_CONFIG_ERRATUM_30115 which will auto enable workaround
Manish Jaggi (15):
arm64: cputype: Add MIDR values for Cavium ThunderX1 CPU family
arm64: Add config for Cavium Thunder erratum 30115
arm: Placeholder for handling Group0/1 traps for Cavium Erratum 30115
arm64: vgic-v3: Add ICV_BPR1_EL1 handler
arm64: vgic-v3: Add ICV_IGRPEN1_EL1 handler
arm64: Add accessors for the ICH_APxRn_EL2 registers Expose ich_read/write_lr in vgic-v3-sr.c
arm64: Add ICV_IAR1_EL1 handler
arm64: vgic-v3: Add ICV_EOIR1_EL1 handler
arm64: vgic-v3: Add ICV_HPPIR1_EL1 handler
arm64: vgic-v3: Add ICV_BPR0_EL1 handler
arm64: vgic-v3: Add ICV_IGNREN0_EL1 handler
arm64: vgic-v3: Add misc Group-0 handlers
arm64: vgic-v3: Add ICV_AP(0/1)Rn_EL1 handler
Enable Group0/1 Traps by default for Cavium ThunderX1
docs/misc/arm/silicon-errata.txt | 1 + xen/arch/arm/Kconfig | 11 + xen/arch/arm/arm64/Makefile | 1 + xen/arch/arm/arm64/vgic-v3-sr.c | 841 ++++++++++++++++++++++++++++++++++++ xen/arch/arm/cpuerrata.c | 21 + xen/arch/arm/gic-v3.c | 12 +- xen/arch/arm/traps.c | 31 ++ xen/include/asm-arm/arm64/sysregs.h | 21 + xen/include/asm-arm/arm64/traps.h | 3 + xen/include/asm-arm/cpuerrata.h | 1 + xen/include/asm-arm/cpufeature.h | 3 +- xen/include/asm-arm/current.h | 3 +- xen/include/asm-arm/gic.h | 2 + xen/include/asm-arm/gic_v3_defs.h | 33 +- xen/include/asm-arm/processor.h | 9 + 15 files changed, 987 insertions(+), 6 deletions(-) create mode 100644 xen/arch/arm/arm64/vgic-v3-sr.c
Signature · 7 lines
2.14.1 _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
PATCH 1/15 arm64: cputype: Add MIDR values for Cavium ThunderX1 CPU family
Add MIDR values for Cavium ThunderX1 SoC family.
ThunderX1, 81XX, 83XX.
xen/include/asm-arm/processor.h +9 -0
@@ -43,15 +43,24 @@ })#define ARM_CPU_IMP_ARM 0x41 +#define ARM_CPU_IMP_CAVIUM 0x43#define ARM_CPU_PART_CORTEX_A15 0xC0F#define ARM_CPU_PART_CORTEX_A53 0xD03#define ARM_CPU_PART_CORTEX_A57 0xD07 +#define CAVIUM_CPU_PART_THUNDERX 0x0A1 +#define CAVIUM_CPU_PART_THUNDERX_81XX 0x0A2 +#define CAVIUM_CPU_PART_THUNDERX_83XX 0x0A3 +#define MIDR_CORTEX_A15 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A15)#define MIDR_CORTEX_A53 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A53)#define MIDR_CORTEX_A57 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A57) +#define MIDR_THUNDERX MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX) +#define MIDR_THUNDERX_81XX MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX_81XX) +#define MIDR_THUNDERX_83XX MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX_83XX) +/* MPIDR Multiprocessor Affinity Register */ #define _MPIDR_UP (30)#define MPIDR_UP (_AC(1,U) << _MPIDR_UP)
Signature · 7 lines
2.14.1 _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [PATCH v1 01/15] arm64: cputype: Add MIDR values for Cavium ThunderX1 CPU family
Hi Manish,
Quoted 1 line · Manish Jaggi
Add MIDR values for Cavium ThunderX1 SoC family.
Did you intend to use a : instead of .?
Quoted 34 lines
ThunderX1, 81XX, 83XX.
Signed-off-by Manish Jaggi <[email protected]>xen/include/asm-arm/processor.h +9
-0@@ -43,15 +43,24 @@ })#define ARM_CPU_IMP_ARM 0x41 +#define ARM_CPU_IMP_CAVIUM 0x43#define ARM_CPU_PART_CORTEX_A15 0xC0F#define ARM_CPU_PART_CORTEX_A53 0xD03#define ARM_CPU_PART_CORTEX_A57 0xD07 +#define CAVIUM_CPU_PART_THUNDERX 0x0A1 +#define CAVIUM_CPU_PART_THUNDERX_81XX 0x0A2 +#define CAVIUM_CPU_PART_THUNDERX_83XX 0x0A3 +#define MIDR_CORTEX_A15 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A15)#define MIDR_CORTEX_A53 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A53)#define MIDR_CORTEX_A57 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A57) +#define MIDR_THUNDERX MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX) +#define MIDR_THUNDERX_81XX MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX_81XX) +#define MIDR_THUNDERX_83XX MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX_83XX) +/* MPIDR Multiprocessor Affinity Register */ #define _MPIDR_UP (30)#define MPIDR_UP (_AC(1,U) << _MPIDR_UP)
Signature · 6 lines
Julien Grall _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
PATCH 2/15 arm64: Add config for Cavium Thunder erratum 30115
In reply to Manish JaggiSome Cavium Thunder CPUs suffer a problem where a Xen guest may inadvertently cause the host kernel to quit receiving interrupts. This patch adds CONFIG_CAVIUM_ERRATUM_30115. Subsequent patches will provide workaround.
docs/misc/arm/silicon-errata.txt +1 -0
@@ -48,3 +48,4 @@ stable hypervisors. | ARM | Cortex-A57 | #852523 | N/A | | ARM | Cortex-A57 | #832075 | ARM64_ERRATUM_832075 | | ARM | Cortex-A57 | #834220 | ARM64_ERRATUM_834220 | +| CAVIUM | ThunderX1 | #30115 | CAVIUM_ERRATUM_30115 |
xen/arch/arm/Kconfig +11 -0
@@ -169,6 +169,17 @@ config ARM64_ERRATUM_834220 If unsure, say Y. +config CAVIUM_ERRATUM_30115 + bool "Cavium Erratum 30115" + depends on HAS_GICV3 + help + On ThunderX T88 pass 1.x through 2.2, T81 pass 1.0 through + 1.2, and T83 Pass 1.0, guest execution may disable + interrupts in host. Trapping both GICv3 group-0 and group-1 + accesses sidesteps the issue. + + If unsure, say Y. + endmenu source "common/Kconfig"
xen/arch/arm/cpuerrata.c +21 -0
@@ -56,6 +56,27 @@ static const struct arm_cpu_capabilities arm_errata[] = { MIDR_RANGE(MIDR_CORTEX_A57, 0x00, (1 << MIDR_VARIANT_SHIFT) | 2), }, +#endif +#ifdef CONFIG_CAVIUM_ERRATUM_30115 + { + /* Cavium ThunderX, T88 pass 1.x - 2.2 */ + .desc = "Cavium erratum 30115", + .capability = ARM64_WORKAROUND_CAVIUM_30115, + MIDR_RANGE(MIDR_THUNDERX, 0x00, + (1 << MIDR_VARIANT_SHIFT) | 2), + }, + { + /* Cavium ThunderX, T81 pass 1.0 - 1.2 */ + .desc = "Cavium erratum 30115", + .capability = ARM64_WORKAROUND_CAVIUM_30115, + MIDR_RANGE(MIDR_THUNDERX_81XX, 0x00, 0x02), + }, + { + /* Cavium ThunderX, T83 pass 1.0 */ + .desc = "Cavium erratum 30115", + .capability = ARM64_WORKAROUND_CAVIUM_30115, + MIDR_RANGE(MIDR_THUNDERX_83XX, 0x00, 0x00), + }, #endif {}, };
xen/include/asm-arm/cpuerrata.h +1 -0
@@ -41,6 +41,7 @@ static inline bool check_workaround_##erratum(void) \ CHECK_WORKAROUND_HELPER(766422, ARM32_WORKAROUND_766422, CONFIG_ARM_32) CHECK_WORKAROUND_HELPER(834220, ARM64_WORKAROUND_834220, CONFIG_ARM_64) +CHECK_WORKAROUND_HELPER(30115, ARM64_WORKAROUND_CAVIUM_30115, CONFIG_ARM_64)#undef CHECK_WORKAROUND_HELPER
xen/include/asm-arm/cpufeature.h +2 -1
@@ -42,8 +42,9 @@#define LIVEPATCH_FEATURE 4#define SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT 5#define SKIP_CTXT_SWITCH_SERROR_SYNC 6 +#define ARM64_WORKAROUND_CAVIUM_30115 7 -#define ARM_NCAPS 7 +#define ARM_NCAPS 8#ifndef __ASSEMBLY__
Signature · 7 lines
2.14.1 _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [PATCH v1 02/15] arm64: Add config for Cavium Thunder erratum 30115
Hi,
Quoted 43 lines · Manish Jaggi
Some Cavium Thunder CPUs suffer a problem where a Xen guest may inadvertently cause the host kernel to quit receiving interrupts. This patch adds
CONFIG_CAVIUM_ERRATUM_30115. Subsequent patches will provide workaround.Signed-off-by Manish Jaggi <[email protected]>docs/misc/arm/silicon-errata.txt +1
-0@@ -48,3 +48,4 @@ stable hypervisors. | ARM | Cortex-A57 | #852523 | N/A | | ARM | Cortex-A57 | #832075 | ARM64_ERRATUM_832075 | | ARM | Cortex-A57 | #834220 | ARM64_ERRATUM_834220 | +| CAVIUM | ThunderX1 | #30115 | CAVIUM_ERRATUM_30115 |xen/arch/arm/Kconfig +11
-0@@ -169,6 +169,17 @@ config ARM64_ERRATUM_834220 If unsure, say Y. +config CAVIUM_ERRATUM_30115 + bool "Cavium Erratum 30115" + depends on HAS_GICV3 + help + On ThunderX T88 pass 1.x through 2.2, T81 pass 1.0 through + 1.2, and T83 Pass 1.0, guest execution may disable + interrupts in host. Trapping both GICv3 group-0 and group-1 + accesses sidesteps the issue. + + If unsure, say Y. + endmenu source "common/Kconfig"xen/arch/arm/cpuerrata.c +0
-0
xen/arch/arm/cpuerrata.c static const struct arm_cpu_capabilities arm_errata[] = { · line 62
Quoted 7 lines
xen/arch/arm/cpuerrata.c +4
-0@@ -56,6 +56,27 @@ static const struct arm_cpu_capabilities arm_errata[] = { MIDR_RANGE(MIDR_CORTEX_A57, 0x00, (1 << MIDR_VARIANT_SHIFT) | 2), }, +#endif +#ifdef CONFIG_CAVIUM_ERRATUM_30115 + { +/* Cavium ThunderX, T88 pass 1.x - 2.2 */
This is quite odd. You specify a number in the commit message here, but in the previous one you just say "thunderx1". Can you please try to agree on the name? Like the right naming is MIDR_THUNDERX_88.
Quoted 25 lines
xen/arch/arm/cpuerrata.c +17
-0+ .desc = "Cavium erratum 30115", + .capability =ARM64_WORKAROUND_CAVIUM_30115 , +MIDR_RANGE (MIDR_THUNDERX ,0x00 , + (1 << MIDR_VARIANT_SHIFT) | 2), + }, + { +/* Cavium ThunderX, T81 pass 1.0 - 1.2 */ + .desc ="Cavium erratum 30115" , + .capability = ARM64_WORKAROUND_CAVIUM_30115, + MIDR_RANGE(MIDR_THUNDERX_81XX, 0x00, 0x02), + }, + { +/* Cavium ThunderX, T83 pass 1.0 */ + .desc ="Cavium erratum 30115" , + .capability = ARM64_WORKAROUND_CAVIUM_30115, + MIDR_RANGE(MIDR_THUNDERX_83XX, 0x00, 0x00), + }, #endif {}, };xen/include/asm-arm/cpuerrata.h +0
-0
xen/include/asm-arm/cpuerrata.h static inline bool check_workaround_##erratum(void) \ · line 44
Quoted 4 lines
xen/include/asm-arm/cpuerrata.h +1
-0@@ -41,6 +41,7 @@ static inline bool check_workaround_##erratum(void) \ CHECK_WORKAROUND_HELPER(766422, ARM32_WORKAROUND_766422, CONFIG_ARM_32) CHECK_WORKAROUND_HELPER(834220, ARM64_WORKAROUND_834220, CONFIG_ARM_64) +CHECK_WORKAROUND_HELPER (30115 ,ARM64_WORKAROUND_CAVIUM_30115 ,CONFIG_ARM_64 )
Please add cavium_ in the erratum name. So it is easy to know where the erratum is from.
Quoted 9 lines
xen/include/asm-arm/cpuerrata.h +0
-0#undef CHECK_WORKAROUND_HELPERxen/include/asm-arm/cpufeature.h +0
-0
xen/include/asm-arm/cpufeature.h line 50
Quoted 10 lines
xen/include/asm-arm/cpufeature.h +2
-1@@ -42,8 +42,9 @@#define LIVEPATCH_FEATURE 4#define SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT 5#define SKIP_CTXT_SWITCH_SERROR_SYNC 6 +#define ARM64_WORKAROUND_CAVIUM_30115 7 -#define ARM_NCAPS 7 +#define ARM_NCAPS 8#ifndef __ASSEMBLY__
Cheers,
Signature · 6 lines
Julien Grall _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [PATCH v1 02/15] arm64: Add config for Cavium Thunder erratum 30115
Quoted 46 lines · Julien Grall
Hi,
Quoted 43 lines · Manish Jaggi
Some Cavium Thunder CPUs suffer a problem where a Xen guest may inadvertently cause the host kernel to quit receiving interrupts. This patch adds
CONFIG_CAVIUM_ERRATUM_30115. Subsequent patches will provide workaround.Signed-off-by Manish Jaggi <[email protected]>docs/misc/arm/silicon-errata.txt +0
-0@@ -48,3 +48,4 @@ stable hypervisors.| ARM | Cortex-A57 | #852523 | N/A | | ARM | Cortex-A57 | #832075 | ARM64_ERRATUM_832075 | | ARM | Cortex-A57 | #834220 | ARM64_ERRATUM_834220 | +| CAVIUM | ThunderX1 | #30115 | CAVIUM_ERRATUM_30115 |xen/arch/arm/Kconfig +0
-0@@ -169,6 +169,17 @@ config ARM64_ERRATUM_834220If unsure, say Y.
+configCAVIUM_ERRATUM_30115
+ bool "Cavium Erratum 30115"
+ depends onHAS_GICV3
+ help
+ On ThunderX T88 pass 1.x through 2.2, T81 pass 1.0 through + 1.2, and T83 Pass 1.0, guest execution may disable + interrupts in host. Trapping both GICv3 group-0 and group-1 + accesses sidesteps the issue.xen/arch/arm/Kconfig +3
-0+ + If unsure, say Y. +endmenu
source "common/Kconfig"xen/arch/arm/cpuerrata.c +0
-0@@ -56,6 +56,27 @@ static const struct arm_cpu_capabilities arm_errata[] = { MIDR_RANGE(MIDR_CORTEX_A57, 0x00,
Quoted 8 lines
xen/arch/arm/cpuerrata.c static const struct arm_cpu_capabilities arm_errata[] = { · line 62
Quoted 6 lines
xen/arch/arm/cpuerrata.c +4
-0(1 << MIDR_VARIANT_SHIFT) | 2), }, +#endif +#ifdef CONFIG_CAVIUM_ERRATUM_30115 + { + /* Cavium ThunderX, T88 pass 1.x - 2.2 */This is quite odd. You specify a number in the commit message here, but in the previous one you just say "thunderx1". Can you please try to agree on the name? Like the right naming is
MIDR_THUNDERX_88.
ThunderX1 is 88xx. Also this patch is using the same patch from linux. Please see https://lists.cs.columbia.edu/pipermail/kvmarm/2017-June/025815.html
Quoted 25 lines
Quoted 25 lines
xen/arch/arm/cpuerrata.c +17
-0+ .desc = "Cavium erratum 30115", + .capability = ARM64_WORKAROUND_CAVIUM_30115, + MIDR_RANGE(MIDR_THUNDERX, 0x00, + (1 << MIDR_VARIANT_SHIFT) | 2), + }, + { + /* Cavium ThunderX, T81 pass 1.0 - 1.2 */ + .desc = "Cavium erratum 30115", + .capability = ARM64_WORKAROUND_CAVIUM_30115, + MIDR_RANGE(MIDR_THUNDERX_81XX, 0x00, 0x02), + }, + { + /* Cavium ThunderX, T83 pass 1.0 */ + .desc = "Cavium erratum 30115", + .capability = ARM64_WORKAROUND_CAVIUM_30115, + MIDR_RANGE(MIDR_THUNDERX_83XX, 0x00, 0x00), + }, #endif {},};
xen/include/asm-arm/cpuerrata.h +0
-0
Quoted 8 lines
xen/include/asm-arm/cpuerrata.h static inline bool · line 45
Quoted 6 lines
xen/include/asm-arm/cpuerrata.h +1
-0@@ -41,6 +41,7 @@ static inline bool check_workaround_##erratum(void) \ CHECK_WORKAROUND_HELPER(766422, ARM32_WORKAROUND_766422, CONFIG_ARM_32) CHECK_WORKAROUND_HELPER(834220, ARM64_WORKAROUND_834220, CONFIG_ARM_64) +CHECK_WORKAROUND_HELPER (30115 ,ARM64_WORKAROUND_CAVIUM_30115 ,CONFIG_ARM_64 )Please add
cavium_in the erratum name. So it is easy to know where the erratum is from.
ok
Quoted 18 lines
Quoted 11 lines
xen/include/asm-arm/cpuerrata.h +0
-0#undef CHECK_WORKAROUND_HELPERdiff --git
a/xen/include/asm-arm/cpufeature.hb/xen/include/asm-arm/cpufeature.hindexf00b6dbd39..d409636bf0100644xen/include/asm-arm/cpufeature.h +1
-0@@ -42,8 +42,9 @@ #define LIVEPATCH_FEATURE 4 #define SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT 5 #define SKIP_CTXT_SWITCH_SERROR_SYNC 6 +#define ARM64_WORKAROUND_CAVIUM_30115 7Quoted 4 lines
-#define ARM_NCAPS 7
+#define ARM_NCAPS 8
#ifndef__ASSEMBLY__Cheers,
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [PATCH v1 02/15] arm64: Add config for Cavium Thunder erratum 30115
Quoted 17 lines · Manish Jaggi
Quoted 16 lines · Julien Grall
xen/arch/arm/cpuerrata.c static const struct arm_cpu_capabilities · line 63
Quoted 13 lines · Manish Jaggi
xen/arch/arm/cpuerrata.c +4
-0@@ -56,6 +56,27 @@ static const struct arm_cpu_capabilities arm_errata[] = { MIDR_RANGE(MIDR_CORTEX_A57, 0x00, (1 << MIDR_VARIANT_SHIFT) | 2), }, +#endif +#ifdef CONFIG_CAVIUM_ERRATUM_30115 + { + /* Cavium ThunderX, T88 pass 1.x - 2.2 */This is quite odd. You specify a number in the commit message here, but in the previous one you just say "thunderx1". Can you please try to agree on the name? Like the right naming is
MIDR_THUNDERX_88.
Quoted 3 lines
ThunderX1 is 88xx. Also this patch is using the same patch from linux. Please see
https://lists.cs.columbia.edu/pipermail/kvmarm/2017-June/025815.html
If you re-use a patch from Linux then write it in the commit message... And in patch #1, you should clarify you are re-using Linux name.
Cheers,
Signature · 6 lines
Julien Grall _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
PATCH 3/15 arm: Placeholder for handling Group0/1 traps for Cavium Erratum 30115
In reply to Manish JaggiSince 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.
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. 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.
xen/arch/arm/arm64/Makefile +1 -0
@@ -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
xen/arch/arm/arm64/vgic-v3-sr.c new file +56 -0
56 lines not on this page · raw message
xen/arch/arm/traps.c +31 -0
@@ -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; + ret =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. +*/ +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; +*/ +if (unlikely (get_cpu_info ()->skip_hyp_tail) ) + { +/* clear it */ +get_cpu_info ()->skip_hyp_tail = false; +return ; + } +while (1 ) {local_irq_disable ();
xen/include/asm-arm/arm64/traps.h +3 -0
@@ -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);
xen/include/asm-arm/current.h +2 -1
@@ -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 ; };static inline struct cpu_info *get_cpu_info (void )
Signature · 7 lines
2.14.1 _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [PATCH v1 03/15] arm: Placeholder for handling Group0/1 traps for Cavium Erratum 30115
Hi Manish,
Quoted 2 lines · Manish Jaggi
Since this is a SoC errata and trapping of certain group1 registers should not affect the normal flow. A new file
vgic-v3-sr.cis 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."
Quoted 6 lines
Function
vgic_v3_handle_cpuif_accessis called fromdo_trap_guest_syncifARM64_WORKAROUND_CAVIUM_30115capability is found.A flag
skip_hyp_tailis introduced in structcpu_info. This flag specifies
The wrapping of the commit message looks wrong.
Quoted 2 lines
that
leave_hypervisor_tailnot 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.
Quoted 83 lines
enter_hypervisor_headis not invoked when workaround
30115 is in place.enter_hypervisor_headandleave_hypervisor_tailare 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 <[email protected]>xen/arch/arm/arm64/Makefile +1
-0@@ -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.oxen/arch/arm/arm64/vgic-v3-sr.c new file +56
-0@@ -0,0 +1,56 @@ +/* +* xen/arch/arm/arm64/vgic-v3-sr.c +* +* Code to handle Cavium Erratum 30115 +* +* Manish Jaggi <[email protected]> +* 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: +*/ +xen/arch/arm/traps.c +0
-0
xen/arch/arm/traps.c void do_trap_guest_sync(struct cpu_user_regs *regs) · line 2108
Quoted 6 lines
xen/arch/arm/traps.c +3
-0@@ -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 ...
xen/arch/arm/traps.c void do_trap_guest_sync(struct cpu_user_regs *regs) · line 2109
Quoted 1 line
xen/arch/arm/traps.c +1
-0+ 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))...
xen/arch/arm/traps.c void do_trap_guest_sync(struct cpu_user_regs *regs) · line 2114
Quoted 5 lines
xen/arch/arm/traps.c +5
-0+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.
Quoted 16 lines
xen/arch/arm/traps.c +12
-0+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) {
xen/arch/arm/traps.c void do_trap_fiq(struct cpu_user_regs *regs) · line 2321
Quoted 6 lines
xen/arch/arm/traps.c +3
-0@@ -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/.
xen/arch/arm/traps.c void do_trap_fiq(struct cpu_user_regs *regs) · line 2324
Quoted 3 lines
xen/arch/arm/traps.c +3
-0+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.
Quoted 26 lines
xen/arch/arm/traps.c +4
-0+get_cpu_info ()->skip_hyp_tail = false; +return ; + } +while (1 ) {local_irq_disable ();xen/include/asm-arm/arm64/traps.h +3
-0@@ -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);xen/include/asm-arm/current.h +0
-0
xen/include/asm-arm/current.h DECLARE_PER_CPU(struct vcpu *, curr_vcpu); · line 25
Quoted 6 lines
xen/include/asm-arm/current.h +2
-1@@ -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,
Signature · 6 lines
Julien Grall _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
PATCH 4/15 arm64: vgic-v3: Add ICV_BPR1_EL1 handler
In reply to Manish JaggiThis patch is ported to xen from linux commitd70c7b31a60f2458f35c226131f2a01a7a98b6cf
Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1 register, which is located in the ICH_VMCR_EL2.BPR1 field.
xen/arch/arm/arm64/vgic-v3-sr.c +70 -0
@@ -20,10 +20,76 @@#include <asm/regs.h> #include <asm/traps.h> #include <asm/system.h> +#include <asm/gic_v3_defs.h> + +#define vtr_to_nr_pre_bits (v) ((((uint32_t)(v) >> 26) & 7) + 1) + +static int __vgic_v3_bpr_min (void ) +{ +/* See Pseudocode for VPriorityGroup */ +return 8 -vtr_to_nr_pre_bits (READ_SYSREG32 (ICH_VTR_EL2 )); +} + +static unsigned int __vgic_v3_get_bpr0 (uint32_t vmcr) +{ +return (vmcr &ICH_VMCR_BPR0_MASK ) >>ICH_VMCR_BPR0_SHIFT ; +} + +static unsigned int __vgic_v3_get_bpr1 (uint32_t vmcr) +{ +unsigned int bpr; + +if ( vmcr &ICH_VMCR_CBPR_MASK ) + { + bpr =__vgic_v3_get_bpr0 (vmcr); +if ( bpr <7 ) + bpr++; + } +else + bpr = (vmcr &ICH_VMCR_BPR1_MASK ) >>ICH_VMCR_BPR1_SHIFT ; + +return bpr; +} + +static void __vgic_v3_read_bpr1 (struct cpu_user_regs *regs,int regidx) +{ +uint32_t vmcr =READ_SYSREG32 (ICH_VMCR_EL2 ); +set_user_reg (regs, regidx,__vgic_v3_get_bpr1 (vmcr)); +} + +static void __vgic_v3_write_bpr1 (struct cpu_user_regs *regs,int regidx) +{ +register_t val =get_user_reg (regs, regidx); +uint8_t bpr_min =__vgic_v3_bpr_min (); +uint32_t vmcr =READ_SYSREG32 (ICH_VMCR_EL2 ); + +if ( vmcr &ICH_VMCR_CBPR_MASK ) +return ; + +/* Enforce BPR limiting */ +if ( val < bpr_min ) + val = bpr_min; + + val <<=ICH_VMCR_BPR1_SHIFT ; + val &=ICH_VMCR_BPR1_MASK ; + vmcr &= ~ICH_VMCR_BPR1_MASK ; + vmcr |= val; + +WRITE_SYSREG32 (vmcr,ICH_VMCR_EL2 ); +} + +void handle_bpr1 (struct cpu_user_regs *regs,int regidx,const union hsr hsr) +{ +if ( hsr.sysreg.read ) +__vgic_v3_read_bpr1 (regs, regidx); +else +__vgic_v3_write_bpr1 (regs, regidx); +}bool vgic_v3_handle_cpuif_access (struct cpu_user_regs *regs,const union hsr hsr) {bool ret = true; +int regidx = hsr.sysreg.reg;/* Disabling interrupts to prevent change in guest state */ local_irq_disable ();
@@ -35,6 +101,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs, const union hsr hsrswitch ( hsr.bits &HSR_SYSREG_REGS_MASK ) { +case HSR_SYSREG_ICC_BPR1_EL1 : +handle_bpr1 (regs, regidx, hsr); +break ; +default : ret = false;break ;
xen/include/asm-arm/arm64/sysregs.h +1 -0
@@ -89,6 +89,7 @@#define HSR_SYSREG_ICC_ASGI1R_EL1 HSR_SYSREG(3,1,c12,c11,6)#define HSR_SYSREG_ICC_SGI0R_EL1 HSR_SYSREG(3,2,c12,c11,7)#define HSR_SYSREG_ICC_SRE_EL1 HSR_SYSREG(3,0,c12,c12,5) +#define HSR_SYSREG_ICC_BPR1_EL1 HSR_SYSREG(3,0,c12,c12,3)#define HSR_SYSREG_CONTEXTIDR_EL1 HSR_SYSREG(3,0,c13,c0,1)#define HSR_SYSREG_PMCR_EL0 HSR_SYSREG(3,3,c9,c12,0)
xen/include/asm-arm/gic_v3_defs.h +6 -0
@@ -157,6 +157,12 @@#define GICH_VMCR_EOI (1 << 9)#define GICH_VMCR_VENG1 (1 << 1) +#define ICH_VMCR_CBPR_SHIFT 4 +#define ICH_VMCR_CBPR_MASK (1 << ICH_VMCR_CBPR_SHIFT) +#define ICH_VMCR_BPR0_SHIFT 21 +#define ICH_VMCR_BPR0_MASK (7 << ICH_VMCR_BPR0_SHIFT) +#define ICH_VMCR_BPR1_SHIFT 18 +#define ICH_VMCR_BPR1_MASK (7 << ICH_VMCR_BPR1_SHIFT)#define GICH_LR_VIRTUAL_MASK 0xffff#define GICH_LR_VIRTUAL_SHIFT 0
Signature · 7 lines
2.14.1 _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [PATCH v1 04/15] arm64: vgic-v3: Add ICV_BPR1_EL1 handler
Hi Manish,
Quoted 7 lines · Manish Jaggi
This patch is ported to xen from linux commit
d70c7b31a60f2458f35c226131f2a01a7a98b6cfAdd a handler for reading/writing the guest's view of the
ICC_BPR1_EL1register, which is located in the ICH_VMCR_EL2.BPR1 field.Signed-off-by Manish Jaggi <[email protected]>
As you port commit by commit, please at least mention Marc as he is the original author of that patch.
Quoted 6 lines
xen/arch/arm/arm64/vgic-v3-sr.c +0
-0
xen/arch/arm/arm64/vgic-v3-sr.c line 27
Quoted 8 lines
xen/arch/arm/arm64/vgic-v3-sr.c +5
-0@@ -20,10 +20,76 @@#include <asm/regs.h> #include <asm/traps.h> #include <asm/system.h> +#include <asm/gic_v3_defs.h> + +#define vtr_to_nr_pre_bits (v) ((((uint32_t)(v) >> 26) & 7) + 1) + +static int __vgic_v3_bpr_min (void )
Please remove one of the space.
As you convert to Xen coding style, please remove __ from all function names.
Quoted 44 lines
xen/arch/arm/arm64/vgic-v3-sr.c +44
-0+{ +/* See Pseudocode for VPriorityGroup */ +return 8 -vtr_to_nr_pre_bits (READ_SYSREG32 (ICH_VTR_EL2 )); +} + +static unsigned int __vgic_v3_get_bpr0 (uint32_t vmcr) +{ +return (vmcr &ICH_VMCR_BPR0_MASK ) >>ICH_VMCR_BPR0_SHIFT ; +} + +static unsigned int __vgic_v3_get_bpr1 (uint32_t vmcr) +{ +unsigned int bpr; + +if ( vmcr &ICH_VMCR_CBPR_MASK ) + { + bpr =__vgic_v3_get_bpr0 (vmcr); +if ( bpr <7 ) + bpr++; + } +else + bpr = (vmcr &ICH_VMCR_BPR1_MASK ) >>ICH_VMCR_BPR1_SHIFT ; + +return bpr; +} + +static void __vgic_v3_read_bpr1 (struct cpu_user_regs *regs,int regidx) +{ +uint32_t vmcr =READ_SYSREG32 (ICH_VMCR_EL2 ); +set_user_reg (regs, regidx,__vgic_v3_get_bpr1 (vmcr)); +} + +static void __vgic_v3_write_bpr1 (struct cpu_user_regs *regs,int regidx) +{ +register_t val =get_user_reg (regs, regidx); +uint8_t bpr_min =__vgic_v3_bpr_min (); +uint32_t vmcr =READ_SYSREG32 (ICH_VMCR_EL2 ); + +if ( vmcr &ICH_VMCR_CBPR_MASK ) +return ; + +/* Enforce BPR limiting */ +if ( val < bpr_min ) + val = bpr_min;
xen/arch/arm/arm64/vgic-v3-sr.c line 81
Quoted 10 lines
xen/arch/arm/arm64/vgic-v3-sr.c +10
-0+ + val <<=ICH_VMCR_BPR1_SHIFT ; + val &=ICH_VMCR_BPR1_MASK ; + vmcr &= ~ICH_VMCR_BPR1_MASK ; + vmcr |= val; + +WRITE_SYSREG32 (vmcr,ICH_VMCR_EL2 ); +} + +void handle_bpr1 (struct cpu_user_regs *regs,int regidx,const union hsr hsr)
I am pretty sure I said it in the past. If the function is not exported it, it *must* be static. I am not going to repeat it and expect all the sites to be fixed in the next version.
The naming is also a bit odd. All of the file is using "vgic_v3_". So please try to be consistent.
Quoted 15 lines
xen/arch/arm/arm64/vgic-v3-sr.c +7
-0+{ +if ( hsr.sysreg.read ) +__vgic_v3_read_bpr1 (regs, regidx); +else +__vgic_v3_write_bpr1 (regs, regidx); +}bool vgic_v3_handle_cpuif_access (struct cpu_user_regs *regs,const union hsr hsr) {bool ret = true; +int regidx = hsr.sysreg.reg;/* Disabling interrupts to prevent change in guest state */ local_irq_disable ();
xen/arch/arm/arm64/vgic-v3-sr.c bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs, const union hsr hsr · line 105
Quoted 5 lines
xen/arch/arm/arm64/vgic-v3-sr.c +2
-0@@ -35,6 +101,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs, const union hsr hsrswitch ( hsr.bits &HSR_SYSREG_REGS_MASK ) { +case HSR_SYSREG_ICC_BPR1_EL1 : +handle_bpr1 (regs, regidx, hsr);
Please use vreg_emulate_* helpers.
Quoted 25 lines
xen/arch/arm/arm64/vgic-v3-sr.c +2
-0+break ; +default : ret = false;break ;xen/include/asm-arm/arm64/sysregs.h +1
-0@@ -89,6 +89,7 @@#define HSR_SYSREG_ICC_ASGI1R_EL1 HSR_SYSREG(3,1,c12,c11,6)#define HSR_SYSREG_ICC_SGI0R_EL1 HSR_SYSREG(3,2,c12,c11,7)#define HSR_SYSREG_ICC_SRE_EL1 HSR_SYSREG(3,0,c12,c12,5) +#define HSR_SYSREG_ICC_BPR1_EL1 HSR_SYSREG(3,0,c12,c12,3)#define HSR_SYSREG_CONTEXTIDR_EL1 HSR_SYSREG(3,0,c13,c0,1)#define HSR_SYSREG_PMCR_EL0 HSR_SYSREG(3,3,c9,c12,0)xen/include/asm-arm/gic_v3_defs.h +0
-0@@ -157,6 +157,12 @@#define GICH_VMCR_EOI (1 << 9)
xen/include/asm-arm/gic_v3_defs.h line 168
Quoted 10 lines
xen/include/asm-arm/gic_v3_defs.h +6
-0#define GICH_VMCR_VENG1 (1 << 1) +#define ICH_VMCR_CBPR_SHIFT 4 +#define ICH_VMCR_CBPR_MASK (1 << ICH_VMCR_CBPR_SHIFT) +#define ICH_VMCR_BPR0_SHIFT 21 +#define ICH_VMCR_BPR0_MASK (7 << ICH_VMCR_BPR0_SHIFT) +#define ICH_VMCR_BPR1_SHIFT 18 +#define ICH_VMCR_BPR1_MASK (7 << ICH_VMCR_BPR1_SHIFT)#define GICH_LR_VIRTUAL_MASK 0xffff#define GICH_LR_VIRTUAL_SHIFT 0
Cheers,
Signature · 6 lines
Julien Grall _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [PATCH v1 04/15] arm64: vgic-v3: Add ICV_BPR1_EL1 handler
Quoted 6 lines · Julien Grall
Hi Manish,
Quoted 3 lines · Manish Jaggi
This patch is ported to xen from linux commit
d70c7b31a60f2458f35c226131f2a01a7a98b6cf
Quoted 6 lines
Quoted 4 lines
Add a handler for reading/writing the guest's view of the
ICC_BPR1_EL1register, which is located in the ICH_VMCR_EL2.BPR1 field.Signed-off-by Manish Jaggi <[email protected]>As you port commit by commit, please at least mention Marc as he is the original author of that patch.
I have a little confusion, should a signoff be added once it is reviewed or even if it is ported? I thought once it is acked, we add signoff.
Please suggest.
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
PATCH 5/15 arm64: vgic-v3: Add ICV_IGRPEN1_EL1 handler
In reply to Manish JaggiThis patch is ported to xen from linux commit:f8b630bc542e0368886ae193d3519c832b270359
Add a handler for reading/writing the guest's view of the ICC_IGRPEN1_EL1
register, which is located in the ICH_VMCR_EL2.VENG1 field.
xen/arch/arm/arm64/vgic-v3-sr.c +32 -0
@@ -86,6 +86,34 @@ void handle_bpr1(struct cpu_user_regs *regs, int regidx, const union hsr hsr)__vgic_v3_write_bpr1 (regs, regidx); } +static void __vgic_v3_read_igrpen1 (struct cpu_user_regs *regs,int regidx) +{ +uint32_t vmcr =READ_SYSREG32 (ICH_VMCR_EL2 ); +set_user_reg (regs, regidx, !!(vmcr &ICH_VMCR_ENG1_MASK )); +} + +static void __vgic_v3_write_igrpen1 (struct cpu_user_regs *regs,int regidx) +{ +register_t val =get_user_reg (regs, regidx); +uint32_t vmcr =READ_SYSREG32 (ICH_VMCR_EL2 ); + +if ( val &1 ) + vmcr |=ICH_VMCR_ENG1_MASK ; +else + vmcr &= ~ICH_VMCR_ENG1_MASK ; + +WRITE_SYSREG32 (vmcr,ICH_VMCR_EL2 ); +} + +void handle_igrpen1 (struct cpu_user_regs *regs,int regidx, +const union hsr hsr) +{ +if ( hsr.sysreg.read ) +__vgic_v3_read_igrpen1 (regs, regidx); +else +__vgic_v3_write_igrpen1 (regs, regidx); +} +bool vgic_v3_handle_cpuif_access (struct cpu_user_regs *regs,const union hsr hsr) {bool ret = true;
@@ -105,6 +133,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs, const union hsr hsrhandle_bpr1 (regs, regidx, hsr);break ; +case HSR_SYSREG_ICC_IGRPEN1_EL1 : +handle_igrpen1 (regs, regidx, hsr); +break ; +default : ret = false;break ;
xen/include/asm-arm/arm64/sysregs.h +1 -0
@@ -90,6 +90,7 @@#define HSR_SYSREG_ICC_SGI0R_EL1 HSR_SYSREG(3,2,c12,c11,7)#define HSR_SYSREG_ICC_SRE_EL1 HSR_SYSREG(3,0,c12,c12,5)#define HSR_SYSREG_ICC_BPR1_EL1 HSR_SYSREG(3,0,c12,c12,3) +#define HSR_SYSREG_ICC_IGRPEN1_EL1 HSR_SYSREG(3,0,c12,c12,7)#define HSR_SYSREG_CONTEXTIDR_EL1 HSR_SYSREG(3,0,c13,c0,1)#define HSR_SYSREG_PMCR_EL0 HSR_SYSREG(3,3,c9,c12,0)
xen/include/asm-arm/gic_v3_defs.h +2 -0
@@ -163,6 +163,8 @@#define ICH_VMCR_BPR0_MASK (7 << ICH_VMCR_BPR0_SHIFT)#define ICH_VMCR_BPR1_SHIFT 18#define ICH_VMCR_BPR1_MASK (7 << ICH_VMCR_BPR1_SHIFT) +#define ICH_VMCR_ENG1_SHIFT 1 +#define ICH_VMCR_ENG1_MASK (1 << ICH_VMCR_ENG1_SHIFT)#define GICH_LR_VIRTUAL_MASK 0xffff#define GICH_LR_VIRTUAL_SHIFT 0
Signature · 7 lines
2.14.1 _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [PATCH v1 05/15] arm64: vgic-v3: Add ICV_IGRPEN1_EL1 handler
Quoted 5 lines · Manish Jaggi
This patch is ported to xen from linux commit:
f8b630bc542e0368886ae193d3519c832b270359Add a handler for reading/writing the guest's view of the
ICC_IGRPEN1_EL1
The wrapping looks wrong.
xen/arch/arm/arm64/vgic-v3-sr.c void handle_bpr1(struct cpu_user_regs *regs, int regidx, const union hsr hsr) · line 89
Quoted 13 lines
register, which is located in the ICH_VMCR_EL2.VENG1 field.
Signed-off-by Manish Jaggi <[email protected]>xen/arch/arm/arm64/vgic-v3-sr.c +1
-0@@ -86,6 +86,34 @@ void handle_bpr1(struct cpu_user_regs *regs, int regidx, const union hsr hsr)__vgic_v3_write_bpr1 (regs, regidx); } +static void __vgic_v3_read_igrpen1 (struct cpu_user_regs *regs,int regidx)
Please remove the __ for all the functions.
xen/arch/arm/arm64/vgic-v3-sr.c void handle_bpr1(struct cpu_user_regs *regs, int regidx, const union hsr hsr) · line 91
Quoted 2 lines
xen/arch/arm/arm64/vgic-v3-sr.c +2
-0+{ +uint32_t vmcr =READ_SYSREG32 (ICH_VMCR_EL2 );
Newline.
Quoted 4 lines
xen/arch/arm/arm64/vgic-v3-sr.c +4
-0+set_user_reg (regs, regidx, !!(vmcr &ICH_VMCR_ENG1_MASK )); +} + +static void __vgic_v3_write_igrpen1 (struct cpu_user_regs *regs,int regidx)
xen/arch/arm/arm64/vgic-v3-sr.c void handle_bpr1(struct cpu_user_regs *regs, int regidx, const union hsr hsr) · line 105
Quoted 10 lines
xen/arch/arm/arm64/vgic-v3-sr.c +10
-0+{ +register_t val =get_user_reg (regs, regidx); +uint32_t vmcr =READ_SYSREG32 (ICH_VMCR_EL2 ); + +if ( val &1 ) + vmcr |=ICH_VMCR_ENG1_MASK ; +else + vmcr &= ~ICH_VMCR_ENG1_MASK ; + +WRITE_SYSREG32 (vmcr,ICH_VMCR_EL2 );
So basically, you ported commit without even looking if there were change on top. For instance the latest code has a specific has an helper to update vmcr. Can you please make sure that all the code is ported?
Quoted 43 lines
xen/arch/arm/arm64/vgic-v3-sr.c +15
-0+} + +void handle_igrpen1 (struct cpu_user_regs *regs,int regidx, +const union hsr hsr) +{ +if ( hsr.sysreg.read ) +__vgic_v3_read_igrpen1 (regs, regidx); +else +__vgic_v3_write_igrpen1 (regs, regidx); +} +bool vgic_v3_handle_cpuif_access (struct cpu_user_regs *regs,const union hsr hsr) {bool ret = true;@@ -105,6 +133,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs, const union hsr hsrhandle_bpr1 (regs, regidx, hsr);break ; +case HSR_SYSREG_ICC_IGRPEN1_EL1 : +handle_igrpen1 (regs, regidx, hsr); +break ; +default : ret = false;break ;xen/include/asm-arm/arm64/sysregs.h +1
-0@@ -90,6 +90,7 @@#define HSR_SYSREG_ICC_SGI0R_EL1 HSR_SYSREG(3,2,c12,c11,7)#define HSR_SYSREG_ICC_SRE_EL1 HSR_SYSREG(3,0,c12,c12,5)#define HSR_SYSREG_ICC_BPR1_EL1 HSR_SYSREG(3,0,c12,c12,3) +#define HSR_SYSREG_ICC_IGRPEN1_EL1 HSR_SYSREG(3,0,c12,c12,7)#define HSR_SYSREG_CONTEXTIDR_EL1 HSR_SYSREG(3,0,c13,c0,1)#define HSR_SYSREG_PMCR_EL0 HSR_SYSREG(3,3,c9,c12,0)xen/include/asm-arm/gic_v3_defs.h +0
-0
xen/include/asm-arm/gic_v3_defs.h line 170
Quoted 8 lines
xen/include/asm-arm/gic_v3_defs.h +2
-0@@ -163,6 +163,8 @@#define ICH_VMCR_BPR0_MASK (7 << ICH_VMCR_BPR0_SHIFT)#define ICH_VMCR_BPR1_SHIFT 18#define ICH_VMCR_BPR1_MASK (7 << ICH_VMCR_BPR1_SHIFT) +#define ICH_VMCR_ENG1_SHIFT 1 +#define ICH_VMCR_ENG1_MASK (1 << ICH_VMCR_ENG1_SHIFT)#define GICH_LR_VIRTUAL_MASK 0xffff#define GICH_LR_VIRTUAL_SHIFT 0
Cheers,
Signature · 6 lines
Julien Grall _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [PATCH v1 05/15] arm64: vgic-v3: Add ICV_IGRPEN1_EL1 handler
Hi Julien,
Quoted 15 lines · Julien Grall
Quoted 5 lines · Manish Jaggi
This patch is ported to xen from linux commit:
f8b630bc542e0368886ae193d3519c832b270359Add a handler for reading/writing the guest's view of the
ICC_IGRPEN1_EL1The wrapping looks wrong.
Quoted 4 lines
register, which is located in the ICH_VMCR_EL2.VENG1 field.
Signed-off-by Manish Jaggi <[email protected]>
Quoted 11 lines
xen/arch/arm/arm64/vgic-v3-sr.c void handle_bpr1(struct cpu_user_regs *regs, int · line 89
Quoted 9 lines
xen/arch/arm/arm64/vgic-v3-sr.c +0
-0@@ -86,6 +86,34 @@ void handle_bpr1(struct cpu_user_regs *regs, int regidx, const union hsr hsr) __vgic_v3_write_bpr1(regs, regidx); } +static void __vgic_v3_read_igrpen1(struct cpu_user_regs *regs, int regidx)Please remove the __ for all the functions.
ok
Quoted 5 lines
xen/arch/arm/arm64/vgic-v3-sr.c void handle_bpr1(struct cpu_user_regs *regs, int · line 91
Quoted 3 lines
xen/arch/arm/arm64/vgic-v3-sr.c +2
-0+{ + uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);Newline.
For 2 line function should there be a newline?
Will add it.
Quoted 10 lines
Quoted 10 lines
xen/arch/arm/arm64/vgic-v3-sr.c +8
-0+ set_user_reg(regs, regidx, !!(vmcr & ICH_VMCR_ENG1_MASK)); +} + +static void __vgic_v3_write_igrpen1(struct cpu_user_regs *regs, int regidx) +{ + register_t val = get_user_reg(regs, regidx); + uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2); +
Quoted 9 lines
xen/arch/arm/arm64/vgic-v3-sr.c void handle_bpr1(struct cpu_user_regs *regs, int regidx, const union hsr hsr) · line 105
Quoted 6 lines
xen/arch/arm/arm64/vgic-v3-sr.c +6
-0+ if ( val & 1 ) + vmcr |= ICH_VMCR_ENG1_MASK; + else + vmcr &= ~ICH_VMCR_ENG1_MASK; + + WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);So basically, you ported commit without even looking if there were change on top. For instance the latest code has a specific has an helper to update vmcr. Can you please make sure that all the code is ported?
Well julien, I did that and it was in original code as well. __vgic_v3_write_vmcr was called which was calling write_gicreg(vmcr, ICH_VMCR_EL2). Xen aleady has a macro so it is better to use xen macro.
I can write that in commit log if that helps in easier review
Quoted 43 lines
Quoted 43 lines
xen/arch/arm/arm64/vgic-v3-sr.c +11
-0+} + +void handle_igrpen1 (struct cpu_user_regs *regs,int regidx, + const union hsr hsr) +{ + if ( hsr.sysreg.read ) + __vgic_v3_read_igrpen1(regs, regidx); + else + __vgic_v3_write_igrpen1(regs, regidx); +} + bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs, const union hsr hsr){
bool ret = true;xen/arch/arm/arm64/vgic-v3-sr.c +0
-0@@ -105,6 +133,10 @@ bool vgic_v3_handle_cpuif_access(struct
cpu_user_regs*regs, const union hsr hsr
handle_bpr1(regs, regidx, hsr);
break;
+ caseHSR_SYSREG_ICC_IGRPEN1_EL1:
+handle_igrpen1(regs, regidx, hsr);
+ break;
+
default:
ret = false;
break;xen/include/asm-arm/arm64/sysregs.h +1
-0@@ -90,6 +90,7 @@ #define HSR_SYSREG_ICC_SGI0R_EL1 HSR_SYSREG(3,2,c12,c11,7) #define HSR_SYSREG_ICC_SRE_EL1 HSR_SYSREG(3,0,c12,c12,5) #define HSR_SYSREG_ICC_BPR1_EL1 HSR_SYSREG(3,0,c12,c12,3) +#define HSR_SYSREG_ICC_IGRPEN1_EL1 HSR_SYSREG(3,0,c12,c12,7) #define HSR_SYSREG_CONTEXTIDR_EL1 HSR_SYSREG(3,0,c13,c0,1) #define HSR_SYSREG_PMCR_EL0 HSR_SYSREG(3,3,c9,c12,0)xen/include/asm-arm/gic_v3_defs.h +0
-0@@ -163,6 +163,8 @@ #define ICH_VMCR_BPR0_MASK (7 << ICH_VMCR_BPR0_SHIFT) #define ICH_VMCR_BPR1_SHIFT 18
Quoted 8 lines
xen/include/asm-arm/gic_v3_defs.h
Quoted 6 lines
xen/include/asm-arm/gic_v3_defs.h +2
-0#define ICH_VMCR_BPR1_MASK (7 << ICH_VMCR_BPR1_SHIFT) +#define ICH_VMCR_ENG1_SHIFT 1 +#define ICH_VMCR_ENG1_MASK (1 << ICH_VMCR_ENG1_SHIFT) #define GICH_LR_VIRTUAL_MASK 0xffff #define GICH_LR_VIRTUAL_SHIFT 0Cheers,
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
PATCH 6/15 arm64: Add accessors for the ICH_APxRn_EL2 registers
In reply to Manish JaggiThis patch is ported to xen from linux commit63000dd8006dc987db31ba670edc23142ea91e01
As we're about to access the Active Priority registers a lot more, let's define accessors that take the register number as a parameter.
This patch only has accessors, another patch will have register trap handlers
xen/arch/arm/arm64/vgic-v3-sr.c +92 -0
@@ -114,6 +114,98 @@ void handle_igrpen1(struct cpu_user_regs *regs, int regidx,__vgic_v3_write_igrpen1 (regs, regidx); } +void __vgic_v3_write_ap0rn (uint32_t val,int n) +{ +switch (n) + { +case 0 : +WRITE_SYSREG32 (val,ICH_AP0R0_EL2 ); +break ; +case 1 : +WRITE_SYSREG32 (val,ICH_AP0R1_EL2 ); +break ; +case 2 : +WRITE_SYSREG32 (val,ICH_AP0R2_EL2 ); +break ; +case 3 : +WRITE_SYSREG32 (val,ICH_AP0R3_EL2 ); +break ; +default : +unreachable (); + } +} + +void __vgic_v3_write_ap1rn (uint32_t val,int n) +{ +switch (n) + { +case 0 : +WRITE_SYSREG32 (val,ICH_AP1R0_EL2 ); +break ; +case 1 : +WRITE_SYSREG32 (val,ICH_AP1R1_EL2 ); +break ; +case 2 : +WRITE_SYSREG32 (val,ICH_AP1R2_EL2 ); +break ; +case 3 : +WRITE_SYSREG32 (val,ICH_AP1R3_EL2 ); +break ; +default : +unreachable (); + } +} + +uint32_t __vgic_v3_read_ap0rn (int n) +{ +uint32_t val; + +switch (n) + { +case 0 : + val =READ_SYSREG32 (ICH_AP0R0_EL2 ); +break ; +case 1 : + val =READ_SYSREG32 (ICH_AP0R1_EL2 ); +break ; +case 2 : + val =READ_SYSREG32 (ICH_AP0R2_EL2 ); +break ; +case 3 : + val =READ_SYSREG32 (ICH_AP0R3_EL2 ); +break ; +default : +unreachable (); + } + +return val; +} + +uint32_t __vgic_v3_read_ap1rn (int n) +{ +uint32_t val; + +switch (n) + { +case 0 : + val =READ_SYSREG32 (ICH_AP1R0_EL2 ); +break ; +case 1 : + val =READ_SYSREG32 (ICH_AP1R1_EL2 ); +break ; +case 2 : + val =READ_SYSREG32 (ICH_AP1R2_EL2 ); +break ; +case 3 : + val =READ_SYSREG32 (ICH_AP1R3_EL2 ); +break ; +default : +unreachable (); + } + +return val; +} +bool vgic_v3_handle_cpuif_access (struct cpu_user_regs *regs,const union hsr hsr) {bool ret = true;
Signature · 7 lines
2.14.1 _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [PATCH v1 06/15] arm64: Add accessors for the ICH_APxRn_EL2 registers
Hi Marc,
I have a query on this patch. The original patch was using these functions so it was ok to make them static. But this patch is not touching the xen vgic code similar to what your patch did.
Will it be ok to merge this patch with https://www.spinics.net/lists/arm-kernel/msg587089.html
-manish
Quoted 103 lines · Manish Jaggi
This patch is ported to xen from linux commit
63000dd8006dc987db31ba670edc23142ea91e01As we're about to access the Active Priority registers a lot more, let's define accessors that take the register number as a parameter.
This patch only has accessors, another patch will have register trap handlers
Signed-off-by Manish Jaggi <[email protected]>xen/arch/arm/arm64/vgic-v3-sr.c +85
-0@@ -114,6 +114,98 @@ void handle_igrpen1(struct cpu_user_regs *regs, int regidx,__vgic_v3_write_igrpen1 (regs, regidx); } +void __vgic_v3_write_ap0rn (uint32_t val,int n) +{ +switch (n) + { +case 0 : +WRITE_SYSREG32 (val,ICH_AP0R0_EL2 ); +break ; +case 1 : +WRITE_SYSREG32 (val,ICH_AP0R1_EL2 ); +break ; +case 2 : +WRITE_SYSREG32 (val,ICH_AP0R2_EL2 ); +break ; +case 3 : +WRITE_SYSREG32 (val,ICH_AP0R3_EL2 ); +break ; +default : +unreachable (); + } +} + +void __vgic_v3_write_ap1rn (uint32_t val,int n) +{ +switch (n) + { +case 0 : +WRITE_SYSREG32 (val,ICH_AP1R0_EL2 ); +break ; +case 1 : +WRITE_SYSREG32 (val,ICH_AP1R1_EL2 ); +break ; +case 2 : +WRITE_SYSREG32 (val,ICH_AP1R2_EL2 ); +break ; +case 3 : +WRITE_SYSREG32 (val,ICH_AP1R3_EL2 ); +break ; +default : +unreachable (); + } +} + +uint32_t __vgic_v3_read_ap0rn (int n) +{ +uint32_t val; + +switch (n) + { +case 0 : + val =READ_SYSREG32 (ICH_AP0R0_EL2 ); +break ; +case 1 : + val =READ_SYSREG32 (ICH_AP0R1_EL2 ); +break ; +case 2 : + val =READ_SYSREG32 (ICH_AP0R2_EL2 ); +break ; +case 3 : + val =READ_SYSREG32 (ICH_AP0R3_EL2 ); +break ; +default : +unreachable (); + } + +return val; +} + +uint32_t __vgic_v3_read_ap1rn (int n) +{ +uint32_t val; + +switch (n) + { +case 0 : + val =READ_SYSREG32 (ICH_AP1R0_EL2 ); +break ; +case 1 : + val =READ_SYSREG32 (ICH_AP1R1_EL2 ); +break ; +case 2 : + val =READ_SYSREG32 (ICH_AP1R2_EL2 ); +break ; +case 3 : + val =READ_SYSREG32 (ICH_AP1R3_EL2 ); +break ;
xen/arch/arm/arm64/vgic-v3-sr.c void handle_igrpen1(struct cpu_user_regs *regs, int regidx, · line 211
Quoted 10 lines
xen/arch/arm/arm64/vgic-v3-sr.c +7
-0+default : +unreachable (); + } + +return val; +} +bool vgic_v3_handle_cpuif_access (struct cpu_user_regs *regs,const union hsr hsr) {bool ret = true;
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [PATCH v1 06/15] arm64: Add accessors for the ICH_APxRn_EL2 registers
Quoted 9 lines · Manish Jaggi
Hi Marc,
I have a query on this patch. The original patch was using these functions so it was ok to make them static.
But this patch is not touching the xen vgic code similar to what your patch did.Will it be ok to merge this patch with
https://www.spinics.net/lists/arm-kernel/msg587089.html
I can only repeat the argument I try to convey earlier. By changing the structure of the series, you're making it harder to review it, as it is not possible to look at two patches side by side and work out what changed.
In the end, that's your call. If you want to change the shape of the series, go for it. But also appreciate the consequences of doing so.
M.
Signature · 6 lines
Jazz is not dead. It just smells funny... _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
PATCH 7/15 Expose ich_read/write_lr in vgic-v3-sr.c
In reply to Manish Jaggigicv3_ich_read/write_lr functions are duplicated in vgic-v3-sr.c This is done to make the file independent of the xen vgic code for handling the errata.
xen/arch/arm/arm64/vgic-v3-sr.c +83 -0
@@ -206,6 +206,89 @@ uint32_t __vgic_v3_read_ap1rn(int n)return val; } +uint64_t gicv3_ich_read_lr (int lr) +{ +switch ( lr ) + { +case 0 :return READ_SYSREG (ICH_LR0_EL2 ); +case 1 :return READ_SYSREG (ICH_LR1_EL2 ); +case 2 :return READ_SYSREG (ICH_LR2_EL2 ); +case 3 :return READ_SYSREG (ICH_LR3_EL2 ); +case 4 :return READ_SYSREG (ICH_LR4_EL2 ); +case 5 :return READ_SYSREG (ICH_LR5_EL2 ); +case 6 :return READ_SYSREG (ICH_LR6_EL2 ); +case 7 :return READ_SYSREG (ICH_LR7_EL2 ); +case 8 :return READ_SYSREG (ICH_LR8_EL2 ); +case 9 :return READ_SYSREG (ICH_LR9_EL2 ); +case 10 :return READ_SYSREG (ICH_LR10_EL2 ); +case 11 :return READ_SYSREG (ICH_LR11_EL2 ); +case 12 :return READ_SYSREG (ICH_LR12_EL2 ); +case 13 :return READ_SYSREG (ICH_LR13_EL2 ); +case 14 :return READ_SYSREG (ICH_LR14_EL2 ); +case 15 :return READ_SYSREG (ICH_LR15_EL2 ); +default : +BUG (); + } +} + +void gicv3_ich_write_lr (int lr,uint64_t val) +{ +switch ( lr ) + { +case 0 : +WRITE_SYSREG (val,ICH_LR0_EL2 ); +break ; +case 1 : +WRITE_SYSREG (val,ICH_LR1_EL2 ); +break ; +case 2 : +WRITE_SYSREG (val,ICH_LR2_EL2 ); +break ; +case 3 : +WRITE_SYSREG (val,ICH_LR3_EL2 ); +break ; +case 4 : +WRITE_SYSREG (val,ICH_LR4_EL2 ); +break ; +case 5 : +WRITE_SYSREG (val,ICH_LR5_EL2 ); +break ; +case 6 : +WRITE_SYSREG (val,ICH_LR6_EL2 ); +break ; +case 7 : +WRITE_SYSREG (val,ICH_LR7_EL2 ); +break ; +case 8 : +WRITE_SYSREG (val,ICH_LR8_EL2 ); +break ; +case 9 : +WRITE_SYSREG (val,ICH_LR9_EL2 ); +break ; +case 10 : +WRITE_SYSREG (val,ICH_LR10_EL2 ); +break ; +case 11 : +WRITE_SYSREG (val,ICH_LR11_EL2 ); +break ; +case 12 : +WRITE_SYSREG (val,ICH_LR12_EL2 ); +break ; +case 13 : +WRITE_SYSREG (val,ICH_LR13_EL2 ); +break ; +case 14 : +WRITE_SYSREG (val,ICH_LR14_EL2 ); +break ; +case 15 : +WRITE_SYSREG (val,ICH_LR15_EL2 ); +break ; +default : +return ; + } +isb (); +} +bool vgic_v3_handle_cpuif_access (struct cpu_user_regs *regs,const union hsr hsr) {bool ret = true;
Signature · 7 lines
2.14.1 _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
PATCH 8/15 arm64: Add ICV_IAR1_EL1 handler
In reply to Manish JaggiThis patch is ported to xen from linux commit132a324ab62fe4fb8d6dcc2ab4eddb0e93b69afe.
Add a handler for reading the guest's view of the ICC_IAR1_EL1 register. This involves finding the highest priority Group-1 interrupt, checking against both PMR and the active group priority, activating the interrupt and setting the group priority as active.
xen/arch/arm/arm64/vgic-v3-sr.c +194 -0
@@ -21,8 +21,18 @@#include <asm/traps.h> #include <asm/system.h> #include <asm/gic_v3_defs.h> +#include <xen/sched.h> +#include <asm/vtimer.h> +#define vtr_to_nr_pre_bits (v) ((((uint32_t)(v) >> 26) & 7) + 1) +#define vtr_to_nr_apr_regs (v) (1 << (vtr_to_nr_pre_bits(v) - 5)) + +#define ESR_ELx_SYS64_ISS_CRM_SHIFT 1 +#define ESR_ELx_SYS64_ISS_CRM_MASK (0xf << ESR_ELx_SYS64_ISS_CRM_SHIFT) + +#define ICC_IAR1_EL1_SPURIOUS 0x3ff +#define VGIC_MAX_SPI 1019static int __vgic_v3_bpr_min (void ) {
@@ -289,6 +299,186 @@ void gicv3_ich_write_lr(int lr, uint64_t val)isb (); } +static int __vgic_v3_get_group (const union hsr hsr) +{ +uint8_t crm = (hsr.bits & ESR_ELx_SYS64_ISS_CRM_MASK) >> + ESR_ELx_SYS64_ISS_CRM_SHIFT; + +return crm !=8 ; +} + +unsigned int gic_get_num_lrs (void ) +{ +uint32_t vtr; + + vtr =READ_SYSREG32 (ICH_VTR_EL2 ); +return (vtr &GICH_VTR_NRLRGS ) +1 ; +} + +static int __vgic_v3_highest_priority_lr (struct cpu_user_regs *regs, +uint32_t vmcr,uint64_t *lr_val) +{ +int i, lr =-1 ; +unsigned int used_lrs =gic_get_num_lrs (); +uint8_t priority =GICV3_IDLE_PRIORITY ; + +for ( i =0 ; i < used_lrs; i++ ) + { +uint64_t val =gicv3_ich_read_lr (i); +uint8_t lr_prio = (val &ICH_LR_PRIORITY_MASK ) >>ICH_LR_PRIORITY_SHIFT ; + +/* Not pending in the state? */ +if ( (val &ICH_LR_STATE ) !=ICH_LR_PENDING_BIT ) +continue ; + +/* Group-0 interrupt, but Group-0 disabled? */ +if ( !(val &ICH_LR_GROUP ) && !(vmcr &ICH_VMCR_ENG0_MASK ) ) +continue ; + +/* Group-1 interrupt, but Group-1 disabled? */ +if ( (val &ICH_LR_GROUP ) && !(vmcr &ICH_VMCR_ENG1_MASK ) ) +continue ; + +/* Not the highest priority? */ +if ( lr_prio >= priority ) +continue ; + +/* This is a candidate */ + priority = lr_prio; + *lr_val = val; + lr = i; + } + +if ( lr ==-1 ) + *lr_val =ICC_IAR1_EL1_SPURIOUS ; + +return lr; +} + +static int __vgic_v3_get_highest_active_priority (void ) +{ +int i; +uint32_t hap =0 ; +uint8_t nr_apr_regs =vtr_to_nr_apr_regs (READ_SYSREG32 (ICH_VTR_EL2 )); + +for ( i =0 ; i < nr_apr_regs; i++ ) + { +uint32_t val; + +/* +* The ICH_AP0Rn_EL2 and ICH_AP1Rn_EL2 registers +* contain the active priority levels for this VCPU +* for the maximum number of supported priority +* levels, and we return the full priority level only +* if the BPR is programmed to its minimum, otherwise +* we return a combination of the priority level and +* subpriority, as determined by the setting of the +* BPR, but without the full subpriority. +*/ + val =__vgic_v3_read_ap0rn (i); + val |=__vgic_v3_read_ap1rn (i); +if ( !val ) + { + hap +=32 ; +continue ; + } + +return (hap +__ffs (val)) <<__vgic_v3_bpr_min (); + } + +return GICV3_IDLE_PRIORITY ; +} + +/* +* Convert a priority to a preemption level, taking the relevant BPR +* into account by zeroing the sub-priority bits. +*/ +static uint8_t __vgic_v3_pri_to_pre (uint8_t pri,uint32_t vmcr,int grp) +{ +unsigned int bpr; + +if ( !grp ) + bpr =__vgic_v3_get_bpr0 (vmcr) +1 ; +else + bpr =__vgic_v3_get_bpr1 (vmcr); + +return pri & (GENMASK (7 ,0 ) << bpr); +} + +/* +* The priority value is independent of any of the BPR values, so we +* normalize it using the minumal BPR value. This guarantees that no +* matter what the guest does with its BPR, we can always set/get the +* same value of a priority. +*/ +static void __vgic_v3_set_active_priority (uint8_t pri,uint32_t vmcr,int grp) +{ +uint8_t pre, ap; +uint32_t val; +int apr; + + pre =__vgic_v3_pri_to_pre (pri, vmcr, grp); + ap = pre >>__vgic_v3_bpr_min (); + apr = ap /32 ; + +if ( !grp ) + { + val =__vgic_v3_read_ap0rn (apr); +__vgic_v3_write_ap0rn (val |BIT (ap %32 ), apr); + } +else + { + val =__vgic_v3_read_ap1rn (apr); +__vgic_v3_write_ap1rn (val |BIT (ap %32 ), apr); + } +} + +static void __vgic_v3_read_iar (struct cpu_user_regs *regs,int regidx, +const union hsr hsr) +{ +uint64_t lr_val; +uint8_t lr_prio, pmr; +int lr, grp; + +uint32_t vmcr =READ_SYSREG32 (ICH_VMCR_EL2 ); + grp =__vgic_v3_get_group (hsr); + + lr =__vgic_v3_highest_priority_lr (regs, vmcr, &lr_val); +if ( lr <0 ) + goto spurious; + +if ( grp != !!(lr_val &ICH_LR_GROUP ) ) + goto spurious; + + pmr = (vmcr &ICH_VMCR_PMR_MASK ) >>ICH_VMCR_PMR_SHIFT ; + lr_prio = (lr_val &ICH_LR_PRIORITY_MASK ) >>ICH_LR_PRIORITY_SHIFT ; +if ( pmr <= lr_prio ) + goto spurious; + +if (__vgic_v3_get_highest_active_priority () <= +__vgic_v3_pri_to_pre (lr_prio, vmcr, grp) ) + goto spurious; + + lr_val &= ~ICH_LR_STATE ; +/* No active state for LPIs */ +if ( (lr_val &ICH_LR_VIRTUAL_ID_MASK ) <=VGIC_MAX_SPI ) + lr_val |=ICH_LR_ACTIVE_BIT ; + +gicv3_ich_write_lr (lr, lr_val); +__vgic_v3_set_active_priority (lr_prio, vmcr, grp); +set_user_reg (regs, regidx, lr_val &ICH_LR_VIRTUAL_ID_MASK ); + +return ; + +spurious: +set_user_reg (regs, regidx,ICC_IAR1_EL1_SPURIOUS ); +} + +void handle_iar (struct cpu_user_regs *regs,int regidx,const union hsr hsr) +{ +__vgic_v3_read_iar (regs, regidx, hsr); +} +bool vgic_v3_handle_cpuif_access (struct cpu_user_regs *regs,const union hsr hsr) {bool ret = true;
@@ -312,6 +502,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs, const union hsr hsrhandle_igrpen1 (regs, regidx, hsr);break ; +case HSR_SYSREG_ICC_IAR1_EL1 : +handle_iar (regs, regidx, hsr); +break ; +default : ret = false;break ;
xen/include/asm-arm/arm64/sysregs.h +1 -0
@@ -91,6 +91,7 @@#define HSR_SYSREG_ICC_SRE_EL1 HSR_SYSREG(3,0,c12,c12,5)#define HSR_SYSREG_ICC_BPR1_EL1 HSR_SYSREG(3,0,c12,c12,3)#define HSR_SYSREG_ICC_IGRPEN1_EL1 HSR_SYSREG(3,0,c12,c12,7) +#define HSR_SYSREG_ICC_IAR1_EL1 HSR_SYSREG(3,0,c12,c12,0)#define HSR_SYSREG_CONTEXTIDR_EL1 HSR_SYSREG(3,0,c13,c0,1)#define HSR_SYSREG_PMCR_EL0 HSR_SYSREG(3,3,c9,c12,0)
xen/include/asm-arm/gic_v3_defs.h +17 -0
@@ -67,6 +67,7 @@*/ #define GICV3_GICD_IIDR_VAL 0x34c#define GICV3_GICR_IIDR_VAL GICV3_GICD_IIDR_VAL +#define GICV3_IDLE_PRIORITY 0xff#define GICR_CTLR (0x0000)#define GICR_IIDR (0x0004)
@@ -165,6 +166,10 @@#define ICH_VMCR_BPR1_MASK (7 << ICH_VMCR_BPR1_SHIFT)#define ICH_VMCR_ENG1_SHIFT 1#define ICH_VMCR_ENG1_MASK (1 << ICH_VMCR_ENG1_SHIFT) +#define ICH_VMCR_ENG0_SHIFT 0 +#define ICH_VMCR_ENG0_MASK (1 << ICH_VMCR_ENG0_SHIFT) +#define ICH_VMCR_PMR_SHIFT 24 +#define ICH_VMCR_PMR_MASK (0xffUL << ICH_VMCR_PMR_SHIFT)#define GICH_LR_VIRTUAL_MASK 0xffff#define GICH_LR_VIRTUAL_SHIFT 0
@@ -182,6 +187,18 @@#define GICH_LR_GRP1 (1UL<<60)#define GICH_LR_HW (1UL<<61) +#define ICH_LR_PRIORITY_SHIFT 48 +#define ICH_LR_PRIORITY_MASK (0xffULL << ICH_LR_PRIORITY_SHIFT) +#define ICH_LR_EOI (1ULL << 41) +#define ICH_LR_GROUP (1ULL << 60) +#define ICH_LR_HW (1ULL << 61) +#define ICH_LR_STATE (3ULL << 62) +#define ICH_LR_PENDING_BIT (1ULL << 62) +#define ICH_LR_ACTIVE_BIT (1ULL << 63) +#define ICH_LR_PHYS_ID_SHIFT 32 +#define ICH_LR_PHYS_ID_MASK (0x3ffULL << ICH_LR_PHYS_ID_SHIFT) +#define ICH_LR_VIRTUAL_ID_MASK ((1ULL << 32) - 1) +#define GICH_VTR_NRLRGS 0x3f#define GICH_VTR_PRIBITS_MASK 0x7#define GICH_VTR_PRIBITS_SHIFT 29
Signature · 7 lines
2.14.1 _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
PATCH 9/15 arm64: vgic-v3: Add ICV_EOIR1_EL1 handler
In reply to Manish JaggiThis patch is ported to xen from linux commitb6f49035b4bf6e2709f2a5fed3107f5438c1fd02
Add a handler for writing the guest's view of the ICC_EOIR1_EL1 register. This involves dropping the priority of the interrupt, and deactivating it if required (EOImode == 0).
xen/arch/arm/arm64/vgic-v3-sr.c +136 -0
@@ -33,6 +33,7 @@#define ICC_IAR1_EL1_SPURIOUS 0x3ff#define VGIC_MAX_SPI 1019 +#define VGIC_MIN_LPI 8192static int __vgic_v3_bpr_min (void ) {
@@ -479,6 +480,137 @@ void handle_iar(struct cpu_user_regs *regs, int regidx, const union hsr hsr)__vgic_v3_read_iar (regs, regidx, hsr); } +static int __vgic_v3_find_active_lr (int intid,uint64_t *lr_val) +{ +int i; +unsigned int used_lrs =gic_get_num_lrs (); + +for ( i =0 ; i < used_lrs; i++ ) + { +uint64_t val =gicv3_ich_read_lr (i); + +if ( (val &ICH_LR_VIRTUAL_ID_MASK ) == intid && + (val &ICH_LR_ACTIVE_BIT ) ) + { + *lr_val = val; +return i; + } + } + + *lr_val =ICC_IAR1_EL1_SPURIOUS ; +return -1 ; +} + +static int __vgic_v3_clear_highest_active_priority (void ) +{ +int i; +uint32_t hap =0 ; +uint8_t nr_apr_regs =vtr_to_nr_apr_regs (READ_SYSREG32 (ICH_VTR_EL2 )); + +for ( i =0 ; i < nr_apr_regs; i++ ) + { +uint32_t ap0, ap1; +int c0, c1; + + ap0 =__vgic_v3_read_ap0rn (i); + ap1 =__vgic_v3_read_ap1rn (i); +if ( !ap0 && !ap1 ) + { + hap +=32 ; +continue ; + } + + c0 = ap0 ?__ffs (ap0) :32 ; + c1 = ap1 ?__ffs (ap1) :32 ; + +/* Always clear the LSB, which is the highest priority */ +if ( c0 < c1 ) + { + ap0 &= ~BIT (c0); +__vgic_v3_write_ap0rn (ap0, i); + hap += c0; + } +else + { + ap1 &= ~BIT (c1); +__vgic_v3_write_ap1rn (ap1, i); + hap += c1; + } + +/* Rescale to 8 bits of priority */ +return hap <<__vgic_v3_bpr_min (); + } + +return GICV3_IDLE_PRIORITY ; +} + +static void __vgic_v3_clear_active_lr (int lr,uint64_t lr_val) +{ + lr_val &= ~ICH_LR_ACTIVE_BIT ; +if ( lr_val &ICH_LR_HW ) + { +uint32_t pid; + + pid = (lr_val &ICH_LR_PHYS_ID_MASK ) >>ICH_LR_PHYS_ID_SHIFT ; +WRITE_SYSREG32 (pid,ICC_DIR_EL1 ); + } +gicv3_ich_write_lr (lr, lr_val); +} + +static void __vgic_v3_bump_eoicount (void ) +{ +uint32_t hcr; + + hcr =READ_SYSREG32 (ICH_HCR_EL2 ); + hcr +=1 << ICH_HCR_EOIcount_SHIFT; +WRITE_SYSREG32 (hcr,ICH_HCR_EL2 ); +} + +static void __vgic_v3_write_eoir (struct cpu_user_regs *regs,int regidx, +const union hsr hsr) +{ +uint32_t vmcr =READ_SYSREG32 (ICH_VMCR_EL2 ); +register_t vid =get_user_reg (regs, regidx); +uint64_t lr_val; +uint8_t lr_prio, act_prio; +int lr, grp; + + grp =__vgic_v3_get_group (hsr); + +/* Drop priority in any case */ + act_prio =__vgic_v3_clear_highest_active_priority (); + +/* If EOIing an LPI, no deactivate to be performed */ +if ( vid >=VGIC_MIN_LPI ) +return ; + +/* EOImode == 1, nothing to be done here */ +if ( vmcr &ICH_VMCR_EOIM_MASK ) +return ; + + lr =__vgic_v3_find_active_lr (vid, &lr_val); +if ( lr ==-1 ) + { +__vgic_v3_bump_eoicount (); +return ; + } + + lr_prio = (lr_val &ICH_LR_PRIORITY_MASK ) >>ICH_LR_PRIORITY_SHIFT ; + +/* If priorities or group do not match, the guest has fscked-up. */ +if ( grp != !!(lr_val &ICH_LR_GROUP ) || +__vgic_v3_pri_to_pre (lr_prio, vmcr, grp) != act_prio ) +return ; + +/* Let's now perform the deactivation */ +__vgic_v3_clear_active_lr (lr, lr_val); +} + +void handle_eoi (struct cpu_user_regs *regs,int regidx,const union hsr hsr) +{ +__vgic_v3_write_eoir (regs, regidx, hsr); +} +bool vgic_v3_handle_cpuif_access (struct cpu_user_regs *regs,const union hsr hsr) {bool ret = true;
@@ -506,6 +638,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs, const union hsr hsrhandle_iar (regs, regidx, hsr);break ; +case HSR_SYSREG_ICC_EOIR1_EL1 : +handle_eoi (regs, regidx, hsr); +break ; +default : ret = false;break ;
xen/include/asm-arm/arm64/sysregs.h +1 -0
@@ -92,6 +92,7 @@#define HSR_SYSREG_ICC_BPR1_EL1 HSR_SYSREG(3,0,c12,c12,3)#define HSR_SYSREG_ICC_IGRPEN1_EL1 HSR_SYSREG(3,0,c12,c12,7)#define HSR_SYSREG_ICC_IAR1_EL1 HSR_SYSREG(3,0,c12,c12,0) +#define HSR_SYSREG_ICC_EOIR1_EL1 HSR_SYSREG(3,0,c12,c12,1)#define HSR_SYSREG_CONTEXTIDR_EL1 HSR_SYSREG(3,0,c13,c0,1)#define HSR_SYSREG_PMCR_EL0 HSR_SYSREG(3,3,c9,c12,0)
xen/include/asm-arm/gic_v3_defs.h +6 -2
@@ -140,9 +140,9 @@#define GICR_PENDBASER_INNER_CACHEABILITY_SHIFT 7#define GICR_PENDBASER_OUTER_CACHEABILITY_SHIFT 56#define GICR_PENDBASER_SHAREABILITY_MASK \ - (3UL << GICR_PENDBASER_SHAREABILITY_SHIFT) +(3UL << GICR_PENDBASER_SHAREABILITY_SHIFT)#define GICR_PENDBASER_INNER_CACHEABILITY_MASK \ - (7UL << GICR_PENDBASER_INNER_CACHEABILITY_SHIFT) +(7UL << GICR_PENDBASER_INNER_CACHEABILITY_SHIFT)#define GICR_PENDBASER_OUTER_CACHEABILITY_MASK \ (7UL << GICR_PENDBASER_OUTER_CACHEABILITY_SHIFT)#define GICR_PENDBASER_PTZ BIT(62)
@@ -170,6 +170,10 @@#define ICH_VMCR_ENG0_MASK (1 << ICH_VMCR_ENG0_SHIFT)#define ICH_VMCR_PMR_SHIFT 24#define ICH_VMCR_PMR_MASK (0xffUL << ICH_VMCR_PMR_SHIFT) +#define ICH_VMCR_EOIM_SHIFT 9 +#define ICH_VMCR_EOIM_MASK (1 << ICH_VMCR_EOIM_SHIFT) +#define ICH_HCR_EOIcount_SHIFT 27 +#define ICH_HCR_EOIcount_MASK (0x1f << ICH_HCR_EOIcount_SHIFT)#define GICH_LR_VIRTUAL_MASK 0xffff#define GICH_LR_VIRTUAL_SHIFT 0
Signature · 7 lines
2.14.1 _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
PATCH 10/15 arm64: vgic-v3: Add ICV_HPPIR1_EL1 handler
In reply to Manish JaggiThis patch is ported from linux to xen
commit: 2724c11a1df4b22ee966c04809ea0e808f66b04e
(KVM: arm64: vgic-v3: Add ICV_HPPIR1_EL1 handler)
Add a handler for reading the guest's view of the ICV_HPPIR1_EL1 register. This is a simple parsing of the available LRs, extracting the highest available interrupt.
xen/arch/arm/arm64/vgic-v3-sr.c +24 -0
@@ -611,6 +611,26 @@ void handle_eoi(struct cpu_user_regs *regs, int regidx, const union hsr hsr)__vgic_v3_write_eoir (regs, regidx, hsr); } +void handle_hppir1 (struct cpu_user_regs *regs,int regidx,const union hsr hsr) +{ +uint64_t lr_val; +int lr, lr_grp, grp; +uint32_t vmcr =READ_SYSREG32 (ICH_VMCR_EL2 ); + + grp =__vgic_v3_get_group (hsr); + lr =__vgic_v3_highest_priority_lr (regs, vmcr, &lr_val); + +if ( lr ==-1 ) + goto spurious; + + lr_grp = !!(lr_val &ICH_LR_GROUP ); +if ( lr_grp != grp ) + lr_val =ICC_IAR1_EL1_SPURIOUS ; + +spurious: +set_user_reg (regs, regidx, lr_val &ICH_LR_VIRTUAL_ID_MASK ); +} +bool vgic_v3_handle_cpuif_access (struct cpu_user_regs *regs,const union hsr hsr) {bool ret = true;
@@ -642,6 +662,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs, const union hsr hsrhandle_eoi (regs, regidx, hsr);break ; +case HSR_SYSREG_ICC_HPPIR1_EL1 : +handle_hppir1 (regs, regidx, hsr); +break ; +default : ret = false;break ;
xen/include/asm-arm/arm64/sysregs.h +1 -0
@@ -93,6 +93,7 @@#define HSR_SYSREG_ICC_IGRPEN1_EL1 HSR_SYSREG(3,0,c12,c12,7)#define HSR_SYSREG_ICC_IAR1_EL1 HSR_SYSREG(3,0,c12,c12,0)#define HSR_SYSREG_ICC_EOIR1_EL1 HSR_SYSREG(3,0,c12,c12,1) +#define HSR_SYSREG_ICC_HPPIR1_EL1 HSR_SYSREG(3,0,c12,c12,2)#define HSR_SYSREG_CONTEXTIDR_EL1 HSR_SYSREG(3,0,c13,c0,1)#define HSR_SYSREG_PMCR_EL0 HSR_SYSREG(3,3,c9,c12,0)
Signature · 7 lines
2.14.1 _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
PATCH 11/15 arm64: vgic-v3: Add ICV_BPR0_EL1 handler
In reply to Manish JaggiThis patch is ported to xen from linux commit:423de85a98c2b50715a0784a74f6124fbc0b1548
Add a handler for reading/writing the guest's view of the ICC_BPR0_EL1 register, which is located in the ICH_VMCR_EL2.BPR0 field.
xen/arch/arm/arm64/vgic-v3-sr.c +39 -0
@@ -97,6 +97,41 @@ void handle_bpr1(struct cpu_user_regs *regs, int regidx, const union hsr hsr)__vgic_v3_write_bpr1 (regs, regidx); } +static void __vgic_v3_read_bpr0 (struct cpu_user_regs *regs,int regidx) +{ +uint32_t vmcr =READ_SYSREG32 (ICH_VMCR_EL2 ); +set_user_reg (regs, regidx,__vgic_v3_get_bpr0 (vmcr)); +} + +static void __vgic_v3_write_bpr0 (struct cpu_user_regs *regs,int regidx) +{ +register_t val =get_user_reg (regs, regidx); +uint8_t bpr_min =__vgic_v3_bpr_min (); +uint32_t vmcr =READ_SYSREG32 (ICH_VMCR_EL2 ); + +if ( vmcr &ICH_VMCR_CBPR_MASK ) +return ; + +/* Enforce BPR limiting */ +if ( val < bpr_min ) + val = bpr_min; + + val <<=ICH_VMCR_BPR0_SHIFT ; + val &=ICH_VMCR_BPR0_MASK ; + vmcr &= ~ICH_VMCR_BPR0_MASK ; + vmcr |= val; + +WRITE_SYSREG32 (vmcr,ICH_VMCR_EL2 ); +} + +void handle_bpr0 (struct cpu_user_regs *regs,int regidx,const union hsr hsr) +{ +if ( hsr.sysreg.read ) +__vgic_v3_read_bpr0 (regs, regidx); +else +__vgic_v3_write_bpr0 (regs, regidx); +} +static void __vgic_v3_read_igrpen1 (struct cpu_user_regs *regs,int regidx) {uint32_t vmcr =READ_SYSREG32 (ICH_VMCR_EL2 );
@@ -646,6 +681,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs, const union hsr hsrswitch ( hsr.bits &HSR_SYSREG_REGS_MASK ) { +case HSR_SYSREG_ICC_BPR0_EL1 : +handle_bpr0 (regs, regidx, hsr); +break ; +case HSR_SYSREG_ICC_BPR1_EL1 :handle_bpr1 (regs, regidx, hsr);break ;
xen/include/asm-arm/arm64/sysregs.h +1 -0
@@ -94,6 +94,7 @@#define HSR_SYSREG_ICC_IAR1_EL1 HSR_SYSREG(3,0,c12,c12,0)#define HSR_SYSREG_ICC_EOIR1_EL1 HSR_SYSREG(3,0,c12,c12,1)#define HSR_SYSREG_ICC_HPPIR1_EL1 HSR_SYSREG(3,0,c12,c12,2) +#define HSR_SYSREG_ICC_BPR0_EL1 HSR_SYSREG(3,0,c12,c8,3)#define HSR_SYSREG_CONTEXTIDR_EL1 HSR_SYSREG(3,0,c13,c0,1)#define HSR_SYSREG_PMCR_EL0 HSR_SYSREG(3,3,c9,c12,0)
Signature · 7 lines
2.14.1 _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
PATCH 12/15 arm64: vgic-v3: Add ICV_IGNREN0_EL1 handler
In reply to Manish JaggiThis patch is ported to xen from linux commit:fbc48a0011deb3d51cb657ca9c0f9083f41c0665
Add a handler for reading/writing the guest's view of the ICC_IGRPEN0_EL1 register, which is located in the ICH_VMCR_EL2.VENG0 field.
xen/arch/arm/arm64/vgic-v3-sr.c +32 -0
@@ -160,6 +160,34 @@ void handle_igrpen1(struct cpu_user_regs *regs, int regidx,__vgic_v3_write_igrpen1 (regs, regidx); } +static void __vgic_v3_read_igrpen0 (struct cpu_user_regs *regs,int regidx) +{ +uint32_t vmcr =READ_SYSREG32 (ICH_VMCR_EL2 ); +set_user_reg (regs, regidx, !!(vmcr &ICH_VMCR_ENG0_MASK )); +} + +static void __vgic_v3_write_igrpen0 (struct cpu_user_regs *regs,int regidx) +{ +register_t val =get_user_reg (regs, regidx); +uint32_t vmcr =READ_SYSREG32 (ICH_VMCR_EL2 ); + +if ( val &1 ) + vmcr |=ICH_VMCR_ENG0_MASK ; +else + vmcr &= ~ICH_VMCR_ENG0_MASK ; + +WRITE_SYSREG32 (vmcr,ICH_VMCR_EL2 ); +} + +void handle_igrpen0 (struct cpu_user_regs *regs,int regidx, +const union hsr hsr) +{ +if ( hsr.sysreg.read ) +__vgic_v3_read_igrpen0 (regs, regidx); +else +__vgic_v3_write_igrpen0 (regs, regidx); +} +void __vgic_v3_write_ap0rn (uint32_t val,int n) { switch (n)
@@ -689,6 +717,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs, const union hsr hsrhandle_bpr1 (regs, regidx, hsr);break ; +case HSR_SYSREG_ICC_IGRPEN0_EL1 : +handle_igrpen0 (regs, regidx, hsr); +break ; +case HSR_SYSREG_ICC_IGRPEN1_EL1 :handle_igrpen1 (regs, regidx, hsr);break ;
xen/include/asm-arm/arm64/sysregs.h +1 -0
@@ -91,6 +91,7 @@#define HSR_SYSREG_ICC_SRE_EL1 HSR_SYSREG(3,0,c12,c12,5)#define HSR_SYSREG_ICC_BPR1_EL1 HSR_SYSREG(3,0,c12,c12,3)#define HSR_SYSREG_ICC_IGRPEN1_EL1 HSR_SYSREG(3,0,c12,c12,7) +#define HSR_SYSREG_ICC_IGRPEN0_EL1 HSR_SYSREG(3,0,c12,c12,6)#define HSR_SYSREG_ICC_IAR1_EL1 HSR_SYSREG(3,0,c12,c12,0)#define HSR_SYSREG_ICC_EOIR1_EL1 HSR_SYSREG(3,0,c12,c12,1)#define HSR_SYSREG_ICC_HPPIR1_EL1 HSR_SYSREG(3,0,c12,c12,2)
Signature · 7 lines
2.14.1 _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
PATCH 13/15 arm64: vgic-v3: Add misc Group-0 handlers
In reply to Manish JaggiThis patch is ported to xen from linux commit:eab0b2dc4f6f34147e3d10da49ab8032e15dbea0
A number of Group-0 registers can be handled by the same accessors as that of Group-1, so let's add the required system register encodings and catch them in the dispatching function.
xen/arch/arm/arm64/vgic-v3-sr.c +3 -0
@@ -725,14 +725,17 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs, const union hsr hsrhandle_igrpen1 (regs, regidx, hsr);break ; +case HSR_SYSREG_ICC_IAR0_EL1 :case HSR_SYSREG_ICC_IAR1_EL1 :handle_iar (regs, regidx, hsr);break ; +case HSR_SYSREG_ICC_EOIR0_EL1 :case HSR_SYSREG_ICC_EOIR1_EL1 :handle_eoi (regs, regidx, hsr);break ; +case HSR_SYSREG_ICC_HPPIR0_EL1 :case HSR_SYSREG_ICC_HPPIR1_EL1 :handle_hppir1 (regs, regidx, hsr);break ;
xen/include/asm-arm/arm64/sysregs.h +3 -0
@@ -95,6 +95,9 @@#define HSR_SYSREG_ICC_IAR1_EL1 HSR_SYSREG(3,0,c12,c12,0)#define HSR_SYSREG_ICC_EOIR1_EL1 HSR_SYSREG(3,0,c12,c12,1)#define HSR_SYSREG_ICC_HPPIR1_EL1 HSR_SYSREG(3,0,c12,c12,2) +#define HSR_SYSREG_ICC_IAR0_EL1 HSR_SYSREG(3,0,c12,c8,0) +#define HSR_SYSREG_ICC_EOIR0_EL1 HSR_SYSREG(3,0,c12,c8,1) +#define HSR_SYSREG_ICC_HPPIR0_EL1 HSR_SYSREG(3,0,c12,c8,2)#define HSR_SYSREG_ICC_BPR0_EL1 HSR_SYSREG(3,0,c12,c8,3)#define HSR_SYSREG_CONTEXTIDR_EL1 HSR_SYSREG(3,0,c13,c0,1)
Signature · 7 lines
2.14.1 _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
PATCH 14/15 arm64: vgic-v3: Add ICV_AP(0/1)Rn_EL1 handler
In reply to Manish JaggiThis patch is a xen port of linux commit f9e7449c780f688bf61a13dfa8c344afeb4ad6e0.
Add a handler for reading/writing the guest's view of the ICV_AP1Rn_EL1 registers. We just map them to the corresponding ICH_AP(0/1)Rn_EL2 registers.
xen/arch/arm/arm64/vgic-v3-sr.c +80 -0
@@ -694,6 +694,66 @@ spurious:set_user_reg (regs, regidx, lr_val &ICH_LR_VIRTUAL_ID_MASK ); } +static void __vgic_v3_read_apxrn (struct cpu_user_regs *regs,int regidx, +const union hsr hsr,int n) +{ +uint32_t val; + +if ( !__vgic_v3_get_group (hsr) ) + val =__vgic_v3_read_ap0rn (n); +else + val =__vgic_v3_read_ap1rn (n); + +set_user_reg (regs, regidx, val); +} + +static void __vgic_v3_write_apxrn (struct cpu_user_regs *regs,int regidx, +const union hsr hsr,int n) +{ +uint32_t val =get_user_reg (regs, regidx); + +if (!__vgic_v3_get_group (hsr)) +__vgic_v3_write_ap0rn (val, n); +else +__vgic_v3_write_ap1rn (val, n); +} + +void handle_apxr0 (struct cpu_user_regs *regs,int regidx, +const union hsr hsr) +{ +if (hsr.sysreg.read) +__vgic_v3_read_apxrn (regs, regidx, hsr,0 ); +else +__vgic_v3_write_apxrn (regs, regidx, hsr,0 ); +} + +void handle_apxr1 (struct cpu_user_regs *regs,int regidx, +const union hsr hsr) +{ +if (hsr.sysreg.read) +__vgic_v3_read_apxrn (regs, regidx, hsr,1 ); +else +__vgic_v3_write_apxrn (regs, regidx, hsr,1 ); +} + +void handle_apxr2 (struct cpu_user_regs *regs,int regidx, +const union hsr hsr) +{ +if (hsr.sysreg.read) +__vgic_v3_read_apxrn (regs, regidx, hsr,2 ); +else +__vgic_v3_write_apxrn (regs, regidx, hsr,2 ); +} + +void handle_apxr3 (struct cpu_user_regs *regs,int regidx, +const union hsr hsr) +{ +if (hsr.sysreg.read) +__vgic_v3_read_apxrn (regs, regidx, hsr,3 ); +else +__vgic_v3_write_apxrn (regs, regidx, hsr,3 ); +} +bool vgic_v3_handle_cpuif_access (struct cpu_user_regs *regs,const union hsr hsr) {bool ret = true;
@@ -740,6 +800,26 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs, const union hsr hsrhandle_hppir1 (regs, regidx, hsr);break ; +case HSR_SYSREG_ICC_AP0Rn_EL1 (0 ): +case HSR_SYSREG_ICC_AP1Rn_EL1 (0 ): +handle_apxr0 (regs, regidx, hsr); +break ; + +case HSR_SYSREG_ICC_AP0Rn_EL1 (1 ): +case HSR_SYSREG_ICC_AP1Rn_EL1 (1 ): +handle_apxr1 (regs, regidx, hsr); +break ; + +case HSR_SYSREG_ICC_AP0Rn_EL1 (2 ): +case HSR_SYSREG_ICC_AP1Rn_EL1 (2 ): +handle_apxr2 (regs, regidx, hsr); +break ; + +case HSR_SYSREG_ICC_AP0Rn_EL1 (3 ): +case HSR_SYSREG_ICC_AP1Rn_EL1 (3 ): +handle_apxr3 (regs, regidx, hsr); +break ; +default : ret = false;break ;
xen/include/asm-arm/arm64/sysregs.h +11 -0
@@ -85,6 +85,17 @@#define HSR_SYSREG_PMINTENCLR_EL1 HSR_SYSREG(3,0,c9,c14,2)#define HSR_SYSREG_MAIR_EL1 HSR_SYSREG(3,0,c10,c2,0)#define HSR_SYSREG_AMAIR_EL1 HSR_SYSREG(3,0,c10,c3,0) +#define HSR_SYSREG_ICC_AP0Rn_EL1 (n) HSR_SYSREG(3,0,c12,c8,4|n) +#define HSR_SYSREG_ICC_AP0R0_EL1 HSR_SYSREG_ICC_AP0Rn_EL1(0) +#define HSR_SYSREG_ICC_AP0R1_EL1 HSR_SYSREG_ICC_AP0Rn_EL1(1) +#define HSR_SYSREG_ICC_AP0R2_EL1 HSR_SYSREG_ICC_AP0Rn_EL1(2) +#define HSR_SYSREG_ICC_AP0R3_EL1 HSR_SYSREG_ICC_AP0Rn_EL1(3) +#define HSR_SYSREG_ICC_AP1Rn_EL1 (n) HSR_SYSREG(3,0,c12,c9, n) +#define HSR_SYSREG_ICC_AP1R0_EL1 HSR_SYSREG_ICC_AP1Rn_EL1(0) +#define HSR_SYSREG_ICC_AP1R1_EL1 HSR_SYSREG_ICC_AP1Rn_EL1(1) +#define HSR_SYSREG_ICC_AP1R2_EL1 HSR_SYSREG_ICC_AP1Rn_EL1(2) +#define HSR_SYSREG_ICC_AP1R3_EL1 HSR_SYSREG_ICC_AP1Rn_EL1(3) +#define HSR_SYSREG_ICC_SGI1R_EL1 HSR_SYSREG(3,0,c12,c11,5)#define HSR_SYSREG_ICC_ASGI1R_EL1 HSR_SYSREG(3,1,c12,c11,6)#define HSR_SYSREG_ICC_SGI0R_EL1 HSR_SYSREG(3,2,c12,c11,7)
Signature · 7 lines
2.14.1 _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
PATCH 15/15 Enable Group0/1 Traps by default for Cavium ThunderX1
In reply to Manish JaggiEnable trapping for Group0/1 register access whenCONFIG_CAVIUM_ERRATUM_30115 is enabled.
xen/arch/arm/gic-v3.c +10 -2
@@ -43,6 +43,7 @@#include <asm/gic.h> #include <asm/gic_v3_defs.h> #include <asm/gic_v3_its.h> +#include <asm/cpuerrata.h> #include <asm/cpufeature.h> #include <asm/acpi.h>
@@ -825,7 +826,7 @@ static void gicv3_cpu_disable(void)static void gicv3_hyp_init (void ) { -uint32_t vtr; +uint32_t vtr, hcr =GICH_HCR_EN ; vtr =READ_SYSREG32 (ICH_VTR_EL2 ); gicv3_info.nr_lrs = (vtr &GICH_VTR_NRLRGS ) +1 ;
@@ -836,7 +837,14 @@ static void gicv3_hyp_init(void)panic ("GICv3: Invalid number of priority bits\n" );WRITE_SYSREG32 (GICH_VMCR_EOI |GICH_VMCR_VENG1 ,ICH_VMCR_EL2 ); -WRITE_SYSREG32 (GICH_HCR_EN ,ICH_HCR_EL2 ); + +if (cpus_have_cap (ARM64_WORKAROUND_CAVIUM_30115 ) ) + { + hcr |=GICH_HCR_TALL1 ; + hcr |=GICH_HCR_TALL0 ; + } + +WRITE_SYSREG32 (hcr,ICH_HCR_EL2 ); }/* Set up the per-CPU parts of the GIC for a secondary CPU */
xen/include/asm-arm/gic.h +2 -0
@@ -117,6 +117,8 @@#define GICH_HCR_VGRP0DIE (1 << 5)#define GICH_HCR_VGRP1EIE (1 << 6)#define GICH_HCR_VGRP1DIE (1 << 7) +#define GICH_HCR_TALL1 (1 << 12) +#define GICH_HCR_TALL0 (1 << 11)#define GICH_MISR_EOI (1 << 0)#define GICH_MISR_U (1 << 1)
Signature · 7 lines
2.14.1 _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 7 lines · Manish Jaggi
This patchset is a Xen port of Marc's patchset.
arm64: KVM: Mediate access to GICv3 sysregs at EL2 [1]The current RFC patchset is a subset of [1], as it handleing only Group1 traps as a PoC. Most of the trap code is added in
vsysreg.c. Trap handler function is kept independent of the usual guest trap handling code.
Looking for feedback on this approach.
This cover letter does not seem to match the series. Please update it on every time you send a series.
Cheers,
Signature · 6 lines
Julien Grall _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
Hi Julien,
Quoted 6 lines · Julien Grall
Quoted 3 lines · Manish Jaggi
This patchset is a Xen port of Marc's patchset.
arm64: KVM: Mediate access to GICv3 sysregs at EL2 [1]
Quoted 6 lines
Quoted 4 lines
The current RFC patchset is a subset of [1], as it handleing only Group1 traps as a PoC. Most of the trap code is added in
vsysreg.c. Trap handler function is kept independent of the usual guest trap handling code.
Looking for feedback on this approach.This cover letter does not seem to match the series. Please update it on every time you send a series.
%s/vsysreg.c/vgic-v3-sr..
Could you please review the other patches in the series, so that I can send v2.
Quoted 3 lines
Cheers,
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 10 lines · Manish Jaggi
Hi Julien,
Quoted 6 lines · Julien Grall
Quoted 3 lines · Manish Jaggi
This patchset is a Xen port of Marc's patchset.
arm64: KVM: Mediate access to GICv3 sysregs at EL2 [1]
Quoted 7 lines
Quoted 6 lines
Quoted 4 lines
The current RFC patchset is a subset of [1], as it handleing only Group1 traps as a PoC. Most of the trap code is added in
vsysreg.c. Trap handler function is kept independent of the usual guest trap handling code.
Looking for feedback on this approach.This cover letter does not seem to match the series. Please update it on every time you send a series.
%s/vsysreg.c/vgic-v3-sr..
"The current RFC patchset is a subset of [1], as it handling only Group 1 traps as a PoC". This is clearly not a PoC anymore nor only handling Group 1.
Quoted 2 lines
Could you please review the other patches in the series, so that I can send v2.
It is in my queue of patch to review.
Cheers,
Signature · 6 lines
Julien Grall _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 15 lines · Manish Jaggi
Hi Julien,
Quoted 11 lines · Julien Grall
Quoted 7 lines · Manish Jaggi
This patchset is a Xen port of Marc's patchset.
arm64: KVM: Mediate access to GICv3 sysregs at EL2 [1]The current RFC patchset is a subset of [1], as it handleing only Group1 traps as a PoC. Most of the trap code is added in
vsysreg.c. Trap handler function is kept independent of the usual guest trap handling code.
Looking for feedback on this approach.
Quoted 4 lines
Quoted 1 line
This cover letter does not seem to match the series. Please update it on every time you send a series.
%s/vsysreg.c/vgic-v3-sr..
Could you please review the other patches in the series, so that I can send v2.
Here the major comments for the series (included patch not reviewed):
1) You seem to miss some patches from Linux. I would like to understand
why they are not there.
2) Strangely some commits does not match the Linux one either in order
and content (I am not speaking about the changes required by Xen). For instance this is the case of patch #14 "arm64: vgic-v3: Add ICV_AP(0/1)Rn_EL1 handler". If you port commit from Linux, then you should follow the same. This help a lot for review.
3)
Code organization:
1) Please drop __ from all functions
2) All functions not exported *should* be static. At the same time you
need to make sure that the series are bisectable. So you probably hook the file in the build system at the end rather than in #3.
Cheers,
Signature · 6 lines
Julien Grall _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 18 lines · Julien Grall
Quoted 15 lines · Manish Jaggi
Hi Julien,
Quoted 11 lines · Julien Grall
Quoted 7 lines · Manish Jaggi
This patchset is a Xen port of Marc's patchset.
arm64: KVM: Mediate access to GICv3 sysregs at EL2 [1]The current RFC patchset is a subset of [1], as it handleing only Group1 traps as a PoC. Most of the trap code is added in
vsysreg.c. Trap handler function is kept independent of the usual guest trap handling code.
Looking for feedback on this approach.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
This cover letter does not seem to match the series. Please update it on every time you send a series.
%s/vsysreg.c/vgic-v3-sr..
Could you please review the other patches in the series, so that I can send v2.
Here the major comments for the series (included patch not reviewed):
1) You seem to miss some patches from Linux. I would like to understand why they are not there.
if code is ported to xen, it is perfectly fine to take only relevant patches.
For instance we are not providing any command line option to individually enable group1 grou0 traps.
Quoted 1 line
2) Strangely some commits does not match the Linux one either in order and content (I am not speaking about the changes required by Xen). For instance this is the case of patch #14 "arm64: vgic-v3: Add ICV_AP(0/1)Rn_EL1 handler". If you port commit from Linux, then you should follow the same. This help a lot for review.
Since we are not doing individually enable of group0/1, it doesnt make sense to have two set of patches for ICV_AP0 / ICV_AP1. So I merged it.
Quoted 4 lines
3)
Code organization:
1) Please drop __ from all functions
ok I will change.
Quoted 2 lines
2) All functions not exported *should* be static. At the same time you need to make sure that the series are bisectable. So you probably hook the file in the build system at the end rather than in #3.
ok.
Quoted 3 lines
Cheers,
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Hi Manish,
Quoted 21 lines · Manish Jaggi
Quoted 18 lines · Julien Grall
Quoted 15 lines · Manish Jaggi
Hi Julien,
Quoted 11 lines · Julien Grall
Quoted 7 lines · Manish Jaggi
This patchset is a Xen port of Marc's patchset.
arm64: KVM: Mediate access to GICv3 sysregs at EL2 [1]The current RFC patchset is a subset of [1], as it handleing only Group1 traps as a PoC. Most of the trap code is added in
vsysreg.c. Trap handler function is kept independent of the usual guest trap handling code.
Looking for feedback on this approach.
Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
This cover letter does not seem to match the series. Please update it on every time you send a series.
%s/vsysreg.c/vgic-v3-sr..
Could you please review the other patches in the series, so that I can send v2.
Here the major comments for the series (included patch not reviewed):
1) You seem to miss some patches from Linux. I would like to understand why they are not there.if code is ported to xen, it is perfectly fine to take only relevant patches.
It is usually expected from the contributor to have some sort of explanation in the cover letter. In particular when you are based on a series from Linux.
Where I am more worried is there are patch on top in Linux, that you didn't backport. So it would be really nice to understand why those patches are not in Xen.
A non-exhaustive list:
- KVM: arm64: Log an error if trapping a write-to-read-only GICv3 access
- KVM: arm64: Log an error if trapping a read-from-write-only
GICv3 access
Quoted 1 line
For instance we are not providing any command line option to individually enable group1 grou0 traps.
I think the command line option could be useful for testing. Developer don't necessarily have a Thunder-X in hand.
Quoted 2 lines
Quoted 1 line
2) Strangely some commits does not match the Linux one either in order and content (I am not speaking about the changes required by Xen). For instance this is the case of patch #14 "arm64: vgic-v3: Add ICV_AP(0/1)Rn_EL1 handler". If you port commit from Linux, then you should follow the same. This help a lot for review.
Since we are not doing individually enable of group0/1, it doesnt make sense to have two set of patches for
ICV_AP0/ICV_AP1. So I merged it.
Sorry, but it does not make sense. Looking at the series you pointed. I don't see a patch just for ICV_AP0. Instead it is part of " KVM: arm64: vgic-v3: Enable trapping of Group-0 system registers". You ported that patch in Xen.
Cheers,
Signature · 6 lines
Julien Grall _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 42 lines · Julien Grall
Hi Manish,
Quoted 21 lines · Manish Jaggi
Quoted 18 lines · Julien Grall
Quoted 15 lines · Manish Jaggi
Hi Julien,
Quoted 11 lines · Julien Grall
Quoted 7 lines · Manish Jaggi
This patchset is a Xen port of Marc's patchset.
arm64: KVM: Mediate access to GICv3 sysregs at EL2 [1]The current RFC patchset is a subset of [1], as it handleing only Group1 traps as a PoC. Most of the trap code is added in
vsysreg.c. Trap handler function is kept independent of the usual guest trap handling code.
Looking for feedback on this approach.Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
This cover letter does not seem to match the series. Please update it on every time you send a series.
%s/vsysreg.c/vgic-v3-sr..
Could you please review the other patches in the series, so that I can send v2.
Here the major comments for the series (included patch not reviewed):
1) You seem to miss some patches from Linux. I would like to understand why they are not there.if code is ported to xen, it is perfectly fine to take only relevant patches.
It is usually expected from the contributor to have some sort of explanation in the cover letter. In particular when you are based on a series from Linux.
Where I am more worried is there are patch on top in Linux, that you didn't backport. So it would be really nice to understand why those patches are not in Xen.
A non-exhaustive list:
- KVM: arm64: Log an error if trapping a write-to-read-only GICv3 access
- KVM: arm64: Log an error if trapping a read-from-write-only GICv3 access
Quoted 9 lines
Quoted 1 line
For instance we are not providing any command line option to individually enable group1 grou0 traps.
I think the command line option could be useful for testing. Developer don't necessarily have a Thunder-X in hand.
Quoted 2 lines
Quoted 1 line
2) Strangely some commits does not match the Linux one either in order and content (I am not speaking about the changes required by Xen). For instance this is the case of patch #14 "arm64: vgic-v3: Add ICV_AP(0/1)Rn_EL1 handler". If you port commit from Linux, then you should follow the same. This help a lot for review.
Since we are not doing individually enable of group0/1, it doesnt make sense to have two set of patches for
ICV_AP0/ICV_AP1. So I merged it.Sorry, but it does not make sense. Looking at the series you pointed. I don't see a patch just for
ICV_AP0. Instead it is part of " KVM: arm64: vgic-v3: Enable trapping of Group-0 system registers". You ported that patch in Xen.
If you see this patch, you will find this one specifically for ICV_AP1
https://lists.cs.columbia.edu/pipermail/kvmarm/2017-June/026040.html
Quoted 2 lines
Cheers,
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
(Sorry for the formatting)
On 23 Mar 2018 14:46, "Manish Jaggi" <[email protected]> wrote:
Quoted 81 lines · Julien Grall
Hi Manish,
Quoted 37 lines · Manish Jaggi
Quoted 33 lines · Julien Grall
Quoted 28 lines · Manish Jaggi
Hi Julien,
Quoted 15 lines · Julien Grall
Quoted 10 lines · Manish Jaggi
This patchset is a Xen port of Marc's patchset.
arm64: KVM: Mediate access to GICv3 sysregs at EL2 [1]The current RFC patchset is a subset of [1], as it handleing only Group1 traps
as a PoC. Most of the trap code is added invsysreg.c. Trap handler function is kept
independent of the usual guest trap handling code. Looking for feedback on this approach.Quoted 3 lines
This cover letter does not seem to match the series. Please update it on every time you send a series.
%s/vsysreg.c/vgic-v3-sr..
Could you please review the other patches in the series, so that I can send v2.
Quoted 7 lines
Quoted 4 lines
Here the major comments for the series (included patch not reviewed):
1) You seem to miss some patches from Linux. I would like to
understand why they are not there.if code is ported to xen, it is perfectly fine to take only relevant patches.
It is usually expected from the contributor to have some sort of explanation in the cover letter. In particular when you are based on a series from Linux.
Where I am more worried is there are patch on top in Linux, that you didn't backport. So it would be really nice to understand why those patches are not in Xen.
A non-exhaustive list:
- KVM: arm64: Log an error if trapping a write-to-read-only GICv3
access
- KVM: arm64: Log an error if trapping a read-from-write-only
GICv3 accessFor instance we are not providing any command line option to individually
Quoted 2 lines
enable group1 grou0 traps.
I think the command line option could be useful for testing. Developer don't necessarily have a Thunder-X in hand.
2) Strangely some commits does not match the Linux one either in order
Quoted 8 lines
Quoted 5 lines
and content (I am not speaking about the changes required by Xen). For instance this is the case of patch #14 "arm64: vgic-v3: Add ICV_AP(0/1)Rn_EL1 handler". If you port commit from Linux, then you should follow the same. This help a lot for review.
Since we are not doing individually enable of group0/1, it doesnt make sense to have two set of patches for
ICV_AP0/ICV_AP1. So I merged it.
Quoted 6 lines
Sorry, but it does not make sense. Looking at the series you pointed. I don't see a patch just for
ICV_AP0. Instead it is part of " KVM: arm64:
vgic-v3: Enable trapping of Group-0 system registers". You ported that patch in Xen.If you see this patch, you will find this one specifically for
ICV_AP1
https://lists.cs.columbia.edu/pipermail/kvmarm/2017-June/026040.html
You didn't get my point... You still don't explain why you move the ICV_AP0 from "Enable trapping of Group-0 system registers" to that patch. If you take commit from Linux then don't move code between commit around unless there is a good reason.
Please try to make the review a bit easier...
Cheers,
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 102 lines · Julien Grall
(Sorry for the formatting)
On 23 Mar 2018 14:46, "Manish Jaggi" <[email protected] <mailto:[email protected]>> wrote:
On 03/21/2018 03:26 PM, Julien Grall wrote:
Hi Manish,
On 03/21/2018 09:38 AM, Manish Jaggi wrote:
On 03/21/2018 02:15 PM, Julien Grall wrote:
On 03/21/2018 04:58 AM, Manish Jaggi wrote:
Hi Julien,
On 03/20/2018 01:16 PM, Julien Grall wrote:
On 03/16/2018 11:58 AM, Manish Jaggi wrote:
This patchset is a Xen port of Marc's patchset.
arm64: KVM: Mediate access to GICv3 sysregs at EL2 [1]The current RFC patchset is a subset of [1], as it handleing only Group1 traps as a PoC. Most of the trap code is added in
vsysreg.c. Trap handler function is kept independent of the usual guest trap handling code.
Looking for feedback on this approach.This cover letter does not seem to match the series. Please update it on every time you send a series.
%s/vsysreg.c/vgic-v3-sr..
Could you please review the other patches in the series, so that I can send v2.
Here the major comments for the series (included patch not reviewed):
1) You seem to miss some patches from Linux. I would like to understand why they are not there.if code is ported to xen, it is perfectly fine to take only relevant patches.
It is usually expected from the contributor to have some sort of explanation in the cover letter. In particular when you are based on a series from Linux.
Where I am more worried is there are patch on top in Linux, that you didn't backport. So it would be really nice to understand why those patches are not in Xen.
A non-exhaustive list:
- KVM: arm64: Log an error if trapping a write-to-read-only GICv3 access
- KVM: arm64: Log an error if trapping a read-from-write-only GICv3 accessFor instance we are not providing any command line option to individually enable group1 grou0 traps.
I think the command line option could be useful for testing. Developer don't necessarily have a Thunder-X in hand.
2) Strangely some commits does not match the Linux one either in order and content (I am not speaking about the changes required by Xen). For instance this is the case of patch #14 "arm64: vgic-v3: Add ICV_AP(0/1)Rn_EL1 handler". If you port commit from Linux, then you should follow the same. This help a lot for review.
Since we are not doing individually enable of group0/1, it doesnt make sense to have two set of patches for
ICV_AP0/ICV_AP1. So I merged it.Sorry, but it does not make sense. Looking at the series you pointed. I don't see a patch just for
ICV_AP0. Instead it is part of " KVM: arm64: vgic-v3: Enable trapping of Group-0 system registers". You ported that patch in Xen.
Quoted 6 lines
If you see this patch, you will find this one specifically for
ICV_AP1
https://lists.cs.columbia.edu/pipermail/kvmarm/2017-June/026040.html
<https://lists.cs.columbia.edu/pipermail/kvmarm/2017-June/026040.html>You didn't get my point... You still don't explain why you move the
ICV_AP0from "Enable trapping of Group-0 system registers" to that patch. If you take commit from Linux then don't move code between commit around unless there is a good reason.
I gave a good reason, that it make sense to club two patches as we are not individually enabling g0/g1 for 30115 errata. There is no documented rule that code taken from linux has to be 1:1 mapping with linux patch.
When complete smmu.c is taken as one file the rule is not applied?
Quoted 5 lines
Please try to make the review a bit easier...
Cheers,
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 111 lines · Manish Jaggi
Quoted 108 lines · Julien Grall
(Sorry for the formatting)
On 23 Mar 2018 14:46, "Manish Jaggi" <[email protected] <mailto:[email protected]>> wrote:
On 03/21/2018 03:26 PM, Julien Grall wrote:
Hi Manish,
On 03/21/2018 09:38 AM, Manish Jaggi wrote:
On 03/21/2018 02:15 PM, Julien Grall wrote:
On 03/21/2018 04:58 AM, Manish Jaggi wrote:
Hi Julien,
On 03/20/2018 01:16 PM, Julien Grall wrote:
On 03/16/2018 11:58 AM, Manish Jaggi wrote:
This patchset is a Xen port of Marc's patchset.
arm64: KVM: Mediate access to GICv3 sysregs at EL2 [1]The current RFC patchset is a subset of [1], as it handleing only Group1 traps as a PoC. Most of the trap code is added in
vsysreg.c. Trap handler function is kept independent of the usual guest trap handling code.
Looking for feedback on this approach.This cover letter does not seem to match the series. Please update it on every time you send a series.
%s/vsysreg.c/vgic-v3-sr..
Could you please review the other patches in the series, so that I can send v2.
Here the major comments for the series (included patch not reviewed):
1) You seem to miss some patches from Linux. I would like to understand why they are not there.if code is ported to xen, it is perfectly fine to take only relevant patches.
It is usually expected from the contributor to have some sort of explanation in the cover letter. In particular when you are based on a series from Linux.
Where I am more worried is there are patch on top in Linux, that you didn't backport. So it would be really nice to understand why those patches are not in Xen.
A non-exhaustive list:
- KVM: arm64: Log an error if trapping a write-to-read-only GICv3 access
- KVM: arm64: Log an error if trapping a read-from-write-only GICv3 accessFor instance we are not providing any command line option to individually enable group1 grou0 traps.
I think the command line option could be useful for testing. Developer don't necessarily have a Thunder-X in hand.
2) Strangely some commits does not match the Linux one either in order and content (I am not speaking about the changes required by Xen). For instance this is the case of patch #14 "arm64: vgic-v3: Add ICV_AP(0/1)Rn_EL1 handler". If you port commit from Linux, then you should follow the same. This help a lot for review.
Since we are not doing individually enable of group0/1, it doesnt make sense to have two set of patches for
ICV_AP0/ICV_AP1. So I merged it.Sorry, but it does not make sense. Looking at the series you pointed. I don't see a patch just for
ICV_AP0. Instead it is part of " KVM: arm64: vgic-v3: Enable trapping of Group-0 system registers". You ported that patch in Xen.If you see this patch, you will find this one specifically for
ICV_AP1
https://lists.cs.columbia.edu/pipermail/kvmarm/2017-June/026040.html
<https://lists.cs.columbia.edu/pipermail/kvmarm/2017-June/026040.html>
Quoted 8 lines
Quoted 4 lines
You didn't get my point... You still don't explain why you move the
ICV_AP0from "Enable trapping of Group-0 system registers" to that patch. If you take commit from Linux then don't move code between commit around unless there is a good reason.I gave a good reason, that it make sense to club two patches as we are not individually enabling g0/g1 for 30115 errata.
There is no documented rule that code taken from linux has to be 1:1 mapping with linux patch.
As the original author of these patches, I want to be able to easily refer to the original design to see what has changed, and understand why. I suspect I'm not the only one in this case. Given that there are at most 4 people in this community who understand enough about the GIC architecture to follow exactly what these patches are doing, you might as well try to make it easier for them by offering a 1:1 mapping between the original code and your port.
Now, if you're happy about these patches not being reviewed, and thus not merged, that's absolutely your call. In which case, how about not posting them at all?
Quoted 1 line
When complete
smmu.cis taken as one file the rule is not applied?
A bad decision one day doesn't mean you should always make the same mistake. People usually learn.
Thanks,
M.
Signature · 6 lines
Jazz is not dead. It just smells funny... _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel