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

["Roger Pau Monné" <roger.pau@citrix.com>]

El 24/07/15 a les 17.49, Jan Beulich ha escrit:
>>>> 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.

See the comment in the bottom of the email.

>> 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.

Sorry, I didn't get that feeling by reading your previous comments. So
you would prefer to pack everything inside of the cpu_x86_<bitness>
structures?

>> #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).

Well, I had in mind that this would be the memory address where the GDTR
base and limit is stored, just like the one you would use with the lgdt
instruction, but please read below.

>> };
>>
>> 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]

Why 16? Intel SDM section 2.5 only mentions cr8 as newly added in amd64.
TBH, I'm not sure it makes much sense to be able to set cr8 from this
interface, it can always be set by the guest OS and should not be
required for booting.

>>     /* 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)?

OK, sorry, I didn't understand it that way. I though you only wanted
them properly sorted (so a, b, c...) and labelled.

>> 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.

Yes, taking into account that only _one_ of them can be used at the same
time.

I have the feeling that we are over engineering this interface. IMHO we
should only allow the user to set the control registers, efer (on amd64)
and the GP registers. Segment selectors would be set by Xen to point to
a flat segment suitable for the mode the guest has selected. I think
that should be enough to get the guest OS into it's entry point, and
then it can do whatever it wants.

If that's not suitable, then my second option would be to allow the
guest to set the base, limit and AR bytes of the selectors, but not load
a GDT.

Roger.