Re: [Xen-devel] [RFC v3 4/6] xen/arm: Add save/restore support for guest core registers

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2014-05-09 at 11:35 -0500, Wei Huang wrote:
> On 05/08/2014 06:10 PM, Andrew Cooper wrote:
> > On 08/05/2014 22:18, Wei Huang wrote:
> >> +#ifdef CONFIG_ARM_32
> >> +    v->arch.ifar = ctxt.ifar;
> >> +    v->arch.dfar = ctxt.dfar;
> >> +    v->arch.dfsr = ctxt.dfsr;
> >> +#else
> >> +    v->arch.far = ctxt.far;
> >> +    v->arch.esr = ctxt.esr;
> >> +#endif
> >
> > Where you have code like this, please use a union in the structure to
> > reduce its size.
> I thought about it from your comments. We can only combine ifar and dfar 
> into the far register. But it became a bit confusing. Will try again.

We use an ifdef for these particular register everywhere internally
already, because they are quite annoyingly subtley different between
arm32 and arm64. There is no requirement for the save/restore code to do
any different/better.

For the public API half we can't have this sort of ifdef though, so a
union or just having all of the field is probably required. Don't forget
about the 32-bit guest on 64-bit hypervisor case (caveat: I've only
looked at the above quoted snippet, not the whole context)

> >> +    /* ======= Guest Core Registers =======
> >> +     *   - Each reg is multiplexed for AArch64 and AArch32 guests, if 
> >> possible
> >> +     *   - Each comments, /AArch64_reg, AArch32_reg/, describes its
> >> +     *     corresponding 64- and 32-bit register name. "NA" means
> >> +     *     "Not Applicable".
> >> +     *   - Check "struct vcpu_guest_core_regs" for details.
> >> +     */
> >> +    uint64_t x0;     /* x0, r0_usr */
> >> +    uint64_t x1;     /* x1, r1_usr */
> >> +    uint64_t x2;     /* x2, r2_usr */
> >> +    uint64_t x3;     /* x3, r3_usr */
> >> +    uint64_t x4;     /* x4, r4_usr */
> >> +    uint64_t x5;     /* x5, r5_usr */
> >> +    uint64_t x6;     /* x6, r6_usr */
> >> +    uint64_t x7;     /* x7, r7_usr */
> >> +    uint64_t x8;     /* x8, r8_usr */
> >> +    uint64_t x9;     /* x9, r9_usr */
> >> +    uint64_t x10;    /* x10, r10_usr */
> >> +    uint64_t x11;    /* x11, r11_usr */
> >> +    uint64_t x12;    /* x12, r12_usr */
> >> +    uint64_t x13;    /* x13, sp_usr */
> >> +    uint64_t x14;    /* x14, lr_usr; */
> >> +    uint64_t x15;    /* x15, __unused_sp_hyp */
> >> +    uint64_t x16;    /* x16, lr_irq */
> >> +    uint64_t x17;    /* x17, sp_irq */
> >> +    uint64_t x18;    /* x18, lr_svc */
> >> +    uint64_t x19;    /* x19, sp_svc */
> >> +    uint64_t x20;    /* x20, lr_abt */
> >> +    uint64_t x21;    /* x21, sp_abt */
> >> +    uint64_t x22;    /* x22, lr_und */
> >> +    uint64_t x23;    /* x23, sp_und */
> >> +    uint64_t x24;    /* x24, r8_fiq */
> >> +    uint64_t x25;    /* x25, r9_fiq */
> >> +    uint64_t x26;    /* x26, r10_fiq */
> >> +    uint64_t x27;    /* x27, r11_fiq */
> >> +    uint64_t x28;    /* x28, r12_fiq */
> >> +    uint64_t x29;    /* fp, sp_fiq */
> >> +    uint64_t x30;    /* lr, lr_fiq */
> >
> > Please use "uint64_t x[31];" and some loops.
> >
> Register multiplexing is very confusing when 32-bit VM is running on a 
> 64-bit Xen. Even though your request is reasonable, I still consider 
> expanding the register names here helpful. At least it gives me space to 
> add comments to show the register mapping layout. I believe others will 
> find it useful as well.

I am fine with what you've got, although matching the names used in the
internal struct cpu_user_regs for x29 and x30 would be nice perhaps.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel