Re: [Xen-devel] [PATCH 1/3] x86/emul: Optimise decode_register() somewhat

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 29/01/18 11:05, Jan Beulich wrote:
>>>> On 26.01.18 at 14:16, <andrew.cooper3@xxxxxxxxxx> wrote:
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -396,6 +396,51 @@ static const struct {
>>  /* Shift values between src and dst sizes of pmov{s,z}x{b,w,d}{w,d,q}. */
>>  static const uint8_t pmov_convert_delta[] = { 1, 2, 3, 1, 2, 1 };
>>  
>> +/*
>> + * Map GPRs by ModRM encoding to their offset within struct cpu_user_regs.
>> + * The AH,CH,DH,BH offsets are misaligned.
>> + */
>> +static const uint8_t cpu_user_regs_gpr_offsets[] = {
>> +    offsetof(struct cpu_user_regs, r(ax)),
>> +    offsetof(struct cpu_user_regs, r(cx)),
>> +    offsetof(struct cpu_user_regs, r(dx)),
>> +    offsetof(struct cpu_user_regs, r(bx)),
>> +    offsetof(struct cpu_user_regs, r(sp)),
>> +    offsetof(struct cpu_user_regs, r(bp)),
>> +    offsetof(struct cpu_user_regs, r(si)),
>> +    offsetof(struct cpu_user_regs, r(di)),
>> +#if defined(__x86_64__)
> #ifdef ?

Can do.

>
>> +    offsetof(struct cpu_user_regs, r8),
>> +    offsetof(struct cpu_user_regs, r9),
>> +    offsetof(struct cpu_user_regs, r10),
>> +    offsetof(struct cpu_user_regs, r11),
>> +    offsetof(struct cpu_user_regs, r12),
>> +    offsetof(struct cpu_user_regs, r13),
>> +    offsetof(struct cpu_user_regs, r14),
>> +    offsetof(struct cpu_user_regs, r15),
>> +#endif
>> +};
>> +static const uint8_t cpu_user_regs_high_gpr_offsets[] = {
>> +    offsetof(struct cpu_user_regs, r(ax)),
>> +    offsetof(struct cpu_user_regs, r(cx)),
>> +    offsetof(struct cpu_user_regs, r(dx)),
>> +    offsetof(struct cpu_user_regs, r(bx)),
> al, cl, dl, bl respectively?

Erm - one version of this patch definitely had these correct...

>
>> +    offsetof(struct cpu_user_regs, ah),
>> +    offsetof(struct cpu_user_regs, ch),
>> +    offsetof(struct cpu_user_regs, dh),
>> +    offsetof(struct cpu_user_regs, bh),
>> +#if defined(__x86_64__)
>> +    offsetof(struct cpu_user_regs, r8),
>> +    offsetof(struct cpu_user_regs, r9),
>> +    offsetof(struct cpu_user_regs, r10),
>> +    offsetof(struct cpu_user_regs, r11),
>> +    offsetof(struct cpu_user_regs, r12),
>> +    offsetof(struct cpu_user_regs, r13),
>> +    offsetof(struct cpu_user_regs, r14),
>> +    offsetof(struct cpu_user_regs, r15),
>> +#endif
> These 8 entries are pointless - there's no encoding which could
> result in them being used.
>
> Not also that the name of the array isn't reflecting its contents: Only
> four of the entries actually are "high byte" registers.
>
> I also think both arrays would better have function scope only.

You've noticed why cpu_user_regs_gpr_offsets can't be, but the high
encodings could be (in principle).

Having the arrays difference lengths would mean that we need to adjust
the boundary conditions on each side of the highbyte_reg branch.  Given
the way patch 3 has turned out, this is probably fine as most codepaths
use the simpler decode_gpr() anyway.  It might also be handy having an
assert catching attempts to use highbyte_reg with a rex addition.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel