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

[Wei Huang <w1.huang@samsung.com>]

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.
>
>> +    /* ======= 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.