Re: [PATCH v3 07/32] xen/x86: fix arch_set_info_guest for HVM guests

["Jan Beulich" <JBeulich@suse.com>]

>>> On 24.07.15 at 17:26, <roger.pau@citrix.com> wrote:
> El 24/07/15 a les 14.44, Jan Beulich ha escrit:
>>>>> On 24.07.15 at 14:11, <roger.pau@citrix.com> wrote:
>>> Or your idea was to put all the bitness specific registers inside of
>>> another separate structure and then unionize them? AFAICT the 16 and
>>> 32bit structures are going to be empty.
>> 
>> How that? 64-bit mode e.g. doesn't need full descriptor data for
>> many of the segment registers.
> 
> Please bear with me, but AFAIK cs, ds, es and ss still need to point to 
> a GDT entry with a 0 base and a limit of 2^64.

No, in 64-bit mode bases are implicitly zero (with the exception of
fs and gs) and limits are implicitly 2^64-1 (which you can't even
express in a descriptor table entry, and with the exception of AMD's
Long Mode Segment Limit extension).

But looking at the code below I see that you're thinking of selector
values only, which would imply that you expect the hypervisor
to actually read descriptors from the descriptor tables. That's
doable, but cumbersome.

> What about the following layout:
> 
> #if defined(__GNUC__) && !defined(__STRICT_ANSI__)
> /* Anonymous union includes 16-, 32- and 64-bit names (e.g., bp/ebp/rbp). */
> # define __DECL_REG(n64, n32, n16) union {     \
>         uint64_t n64;                          \
>         uint32_t n32;                          \
>         uint16_t n16;                          \
>     }
> #else
> /* Non-gcc sources must always use the proper 64-bit name (e.g., rbp). */
> #define __DECL_REG(n64, n32, n16) uint64_t n64
> #endif

Since you keep repeating this, I finally have to comment on it: This
is a suitable model for where we use it currently. It's imo not
suitable at all here - not the least because even non-gcc users
should be allowed to use just a 32-bit field when they mean one.
But the problem is also that for 16- and 32-bit mode a 64-bit
register is meaningless: Equally well you could expose r8-r15 to
them.

> #define __DECL_GP_REG(n) __DECL_REG(r##n, e##n, n)
> 
> struct cpu_x86_16 {
>     /* Control registers. */
>     uint32_t cr[8];
>     /* Debug registers. */
>     uint32_t dr[8];
>     /* GDT descriptor address. */
>     uint16_t gdtr;

Even in 16-bit mode this is a 32-bit base address. And you're
omitting the limit here an below (which is a required part namely
when you expect the hypervisor to access this table).

> };
> 
> struct cpu_x86_32 {
>     /* Control registers. */
>     uint32_t cr[8];
>     /* Debug registers. */
>     uint32_t dr[8];
>     /* GDT descriptor address. */
>     uint32_t gdtr;
> };
> 
> struct cpu_x86_64 {
>     /* Additional amd64 general purpose registers. */
>     uint64_t r8, r9, r10, r11, r12, r13, r14, r15;
>     /* Control registers. */
>     uint64_t cr[9];

[16]

>     /* Debug registers. */
>     uint64_t dr[8];
>     /* Extended Feature Enable Register. */
>     uint64_t efer;
>     /* GDT descriptor address. */
>     uint64_t gdtr;
> };
> 
> struct cpu_hvm_regs {
>     /* General purpose registers. */
>     __DECL_GP_REG(ax);
>     __DECL_GP_REG(bx);
>     __DECL_GP_REG(cx);
>     __DECL_GP_REG(dx);
> 
>     /* Index registers. */
>     __DECL_GP_REG(di);
>     __DECL_GP_REG(si);
> 
>     /* Pointer registers. */
>     __DECL_GP_REG(bp);
>     __DECL_GP_REG(sp);

Now didn't you agree to use the _natural_ order (i.e. according to
the numbers used to encode registers e.g. in instructions)?

> struct vcpu_hvm_context {
> /* 16bit fields of the strucutre will be used. */
> #define _VCPUHVM_MODE_16B              0
> #define VCPUHVM_MODE_16B               (1<<_VCPUHVM_MODE_16B)
> /* 32bit fields of the structure will be used. */
> #define _VCPUHVM_MODE_32B              1
> #define VCPUHVM_MODE_32B               (1<<_VCPUHVM_MODE_32B)
> /* 64bit fields of the structure will be used. */
> #define _VCPUHVM_MODE_64B              2
> #define VCPUHVM_MODE_64B               (1<<_VCPUHVM_MODE_64B)

As soon as we have three values here, a boolean flag approach
doesn't make sense anymore.

Jan