diff mbox series

[XEN,v2,01/12] xen/Arm: vGICv3: Sysreg emulation is applicable for Aarch64 only

Message ID 20221031151326.22634-2-ayankuma@amd.com (mailing list archive)
State New, archived
Headers show
Series Arm: Enable GICv3 for AArch32 (v8R) | expand

Commit Message

Ayan Kumar Halder Oct. 31, 2022, 3:13 p.m. UTC
Refer ARM DDI 0487G.b ID072021, EC==0b011000 is supported for Aarch64 state
only. Thus, vgic_v3_emulate_sysreg is enabled for CONFIG_ARM_64 only.

Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com>
---

Changes from -
v1 - 1. Updated the commit message.

 xen/arch/arm/vgic-v3.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Michal Orzel Oct. 31, 2022, 5:43 p.m. UTC | #1
Hi Ayan,

On 31/10/2022 16:13, Ayan Kumar Halder wrote:
> 
> 
> Refer ARM DDI 0487G.b ID072021, EC==0b011000 is supported for Aarch64 state

I think when adding new code we should be taking the latest spec (which is I.a) as a base +
you are lacking the information \wrt page number, table, whatever contains this information
within ARM ARM.

Apart from that, wouldn't it be easier for those reading the commit to just write e.g.:
"Sysreg emulation is 64-bit specific, so guard the calls to vgic_v3_emulate_sysreg
as well as the function itself with #ifdef CONFIG_ARM_64."

Placing EC code in such statement is not very helpful.

~Michal
Julien Grall Nov. 2, 2022, 8:51 a.m. UTC | #2
On 31/10/2022 17:43, Michal Orzel wrote:
> Hi Ayan,
> 
> On 31/10/2022 16:13, Ayan Kumar Halder wrote:
>>
>>
>> Refer ARM DDI 0487G.b ID072021, EC==0b011000 is supported for Aarch64 state
> 
> I think when adding new code we should be taking the latest spec (which is I.a) as a base +
> you are lacking the information \wrt page number, table, whatever contains this information
> within ARM ARM.

+1.

> 
> Apart from that, wouldn't it be easier for those reading the commit to just write e.g.:
> "Sysreg emulation is 64-bit specific, so guard the calls to vgic_v3_emulate_sysreg
> as well as the function itself with #ifdef CONFIG_ARM_64."

This read better. What matter is the emulation is 64-bit specific, not 
that the EC doesn't exist on 32-bit (BTW, in theory 32-bit could have 
re-used it for a different purpose...).

Cheers,
diff mbox series

Patch

diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 015446be17..3f4509dcd3 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1519,6 +1519,7 @@  static bool vgic_v3_emulate_sgi1r(struct cpu_user_regs *regs, uint64_t *r,
     }
 }
 
+#ifdef CONFIG_ARM_64
 static bool vgic_v3_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr)
 {
     struct hsr_sysreg sysreg = hsr.sysreg;
@@ -1539,6 +1540,7 @@  static bool vgic_v3_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr)
         return false;
     }
 }
+#endif
 
 static bool vgic_v3_emulate_cp64(struct cpu_user_regs *regs, union hsr hsr)
 {
@@ -1562,8 +1564,10 @@  static bool vgic_v3_emulate_reg(struct cpu_user_regs *regs, union hsr hsr)
 {
     switch (hsr.ec)
     {
+#ifdef CONFIG_ARM_64
     case HSR_EC_SYSREG:
         return vgic_v3_emulate_sysreg(regs, hsr);
+#endif
     case HSR_EC_CP15_64:
         return vgic_v3_emulate_cp64(regs, hsr);
     default: