Re: [XEN v1 4/4] xen/arm: traps.c: Enclose VMSA specific registers within CONFIG_MMU

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 14/09/2023 06:48, Penny Zheng wrote:
> Hi, Ayan
Hi Penny,
> On 2023/9/11 21:59, Ayan Kumar Halder wrote:
> > The VMSA specific registers (ie TCR, TTBR0, TTBR1, VTTBR, etc) are 
> > valid when
> > MMU is used, thus we can enclose them with CONFIG_MMU.
> >
> > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> > ---
> >   xen/arch/arm/traps.c | 17 +++++++++++++++--
> >   1 file changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index 46c9a4031b..83522fcc5a 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -698,8 +698,10 @@ static void __do_trap_serror(struct 
> > cpu_user_regs *regs, bool guest)
> >   struct reg_ctxt {
> >       /* Guest-side state */
> >       register_t sctlr_el1;
> > +#ifdef CONFIG_MMU
> >       register_t tcr_el1;
> >       uint64_t ttbr0_el1, ttbr1_el1;
> > +#endif
> >   #ifdef CONFIG_ARM_32
> >       uint32_t dfsr, ifsr;
> >       uint32_t dfar, ifar;
> > @@ -801,9 +803,11 @@ static void show_registers_32(const struct 
> > cpu_user_regs *regs,
> >       if ( guest_mode_on )
> >       {
> >           printk("     SCTLR: %"PRIregister"\n", ctxt->sctlr_el1);
> > +#ifdef CONFIG_MMU
> >           printk("       TCR: %"PRIregister"\n", ctxt->tcr_el1);
> >           printk("     TTBR0: %016"PRIx64"\n", ctxt->ttbr0_el1);
> >           printk("     TTBR1: %016"PRIx64"\n", ctxt->ttbr1_el1);
> > +#endif
> FWIS, on v8R-AArch64, with MPU on EL2, we could still support VMSA on 
> EL1, so registers like TTBR0_EL1/TCR_EL1 are valid.
> See ARM DDI 0600B.a Table G1-2 Alphabetical index of modified AArch64 
> System registers (continued) for detailed info.
Actually, you are correct. And, we should not be using CONFIG_MMU to 
enclose VMSA EL1 registers. I am ok to drop this patch.
However, refer G1.3.7  "ID_AA64MMFR0_EL1, AArch64 Memory Model Feature 
Register 0"
When MSA_frac, bits [55:52] == 0b0001, then there is no support for 
VMSAv8-64. IIUC, VMSA EL1 registers will not be present.
If this is correct, then we might need to have AARCH64_V8R specific 
config as ID_AA64MMFR0_EL1 can be interpreted for this configuration only.
Appreciate your comments.

- Ayan

> >           printk("      IFAR: %08"PRIx32", IFSR: %08"PRIx32"\n"
> >                  "      DFAR: %08"PRIx32", DFSR: %08"PRIx32"\n",
> >   #ifdef CONFIG_ARM_64
> > @@ -873,9 +877,11 @@ static void show_registers_64(const struct 
> > cpu_user_regs *regs,
> >           printk("   FAR_EL1: %016"PRIx64"\n", ctxt->far);
> >           printk("\n");
> >           printk(" SCTLR_EL1: %"PRIregister"\n", ctxt->sctlr_el1);
> > +#ifdef CONFIG_MMU
> >           printk("   TCR_EL1: %"PRIregister"\n", ctxt->tcr_el1);
> >           printk(" TTBR0_EL1: %016"PRIx64"\n", ctxt->ttbr0_el1);
> >           printk(" TTBR1_EL1: %016"PRIx64"\n", ctxt->ttbr1_el1);
> > +#endif
> >           printk("\n");
> >       }
> >   }
> > @@ -907,13 +913,15 @@ static void _show_registers(const struct 
> > cpu_user_regs *regs,
> >           show_registers_32(regs, ctxt, guest_mode_on, v);
> >   #endif
> >       }
> > +#ifdef CONFIG_MMU
> >       printk("  VTCR_EL2: %"PRIregister"\n", READ_SYSREG(VTCR_EL2));
> >       printk(" VTTBR_EL2: %016"PRIx64"\n", ctxt->vttbr_el2);
> > +    printk(" TTBR0_EL2: %016"PRIx64"\n", READ_SYSREG64(TTBR0_EL2));
> >       printk("\n");
> > +#endif
> >         printk(" SCTLR_EL2: %"PRIregister"\n", READ_SYSREG(SCTLR_EL2));
> >       printk("   HCR_EL2: %"PRIregister"\n", READ_SYSREG(HCR_EL2));
> > -    printk(" TTBR0_EL2: %016"PRIx64"\n", READ_SYSREG64(TTBR0_EL2));
> >       printk("\n");
> >       printk("   ESR_EL2: %"PRIregister"\n", regs->hsr);
> >       printk(" HPFAR_EL2: %"PRIregister"\n", READ_SYSREG(HPFAR_EL2));
> > @@ -931,9 +939,13 @@ void show_registers(const struct cpu_user_regs 
> > *regs)
> >   {
> >       struct reg_ctxt ctxt;
> >       ctxt.sctlr_el1 = READ_SYSREG(SCTLR_EL1);
> > +#ifdef CONFIG_MMU
> >       ctxt.tcr_el1 = READ_SYSREG(TCR_EL1);
> >       ctxt.ttbr0_el1 = READ_SYSREG64(TTBR0_EL1);
> >       ctxt.ttbr1_el1 = READ_SYSREG64(TTBR1_EL1);
> > +    ctxt.vttbr_el2 = READ_SYSREG64(VTTBR_EL2);
> > +#endif
> > +
> >   #ifdef CONFIG_ARM_32
> >       ctxt.dfar = READ_CP32(DFAR);
> >       ctxt.ifar = READ_CP32(IFAR);
> > @@ -945,7 +957,6 @@ void show_registers(const struct cpu_user_regs 
> > *regs)
> >       if ( guest_mode(regs) && is_32bit_domain(current->domain) )
> >           ctxt.ifsr32_el2 = READ_SYSREG(IFSR32_EL2);
> >   #endif
> > -    ctxt.vttbr_el2 = READ_SYSREG64(VTTBR_EL2);
> >         _show_registers(regs, &ctxt, guest_mode(regs), current);
> >   }
> > @@ -954,9 +965,11 @@ void vcpu_show_registers(const struct vcpu *v)
> >   {
> >       struct reg_ctxt ctxt;
> >       ctxt.sctlr_el1 = v->arch.sctlr;
> > +#ifdef CONFIG_MMU
> >       ctxt.tcr_el1 = v->arch.ttbcr;
> >       ctxt.ttbr0_el1 = v->arch.ttbr0;
> >       ctxt.ttbr1_el1 = v->arch.ttbr1;
> > +#endif
> >   #ifdef CONFIG_ARM_32
> >       ctxt.dfar = v->arch.dfar;
> >       ctxt.ifar = v->arch.ifar;