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 12.46, Jan Beulich ha escrit:
>>>> On 24.07.15 at 11:59, <roger.pau@citrix.com> wrote:
>> Just to recap and to make sure I got all the points:
>>
>>  - vCPU initialization from the toolstack (BSP initialization) is going 
>> to be done using XEN_DOMCTL_sethvmcontext.
>>  - XEN_DOMCTL_{get/set}vcpucontext is going to return EOPNOTSUPP for 
>> HVM guests.
>>  - A new vcpu_guest_contatext (vcpu_hvm_context?) is going to be 
>> introduced together with a a new VCPUOP_initialize hypercall 
>> (VCPUOP_hvm_initialize?).
> 
> Since the current one is called VCPUOP_initialise, why not just
> VCPUOP_initialize? Having "hvm" in the name when it's meant to
> also be used for PVH may end up being confusing.

Right.

>> The following is a layout proposal for the new vcpu_context:
>>
>> struct vcpu_hvm_context {
>> /* 32bit fields of the structure will be used. */
>> #define _VCPUHVM_MODE_32B              0
>> #define VCPUHVM_MODE_32B               (1<<_VCPUHVM_MODE_32B)
>> /* 64bit fields of the structure will be used. */
>> #define _VCPUHVM_MODE_64B              1
>> #define VCPUHVM_MODE_64B               (1<<_VCPUHVM_MODE_64B)
> 
> 16-bit mode?

I wasn't even considering that we would like to start the vCPUs in 16bit
mode, but given that's how APs are started on bare metal I guess it
makes sense to have it.

> 
>> #define _VCPUHVM_online                2
>> #define VCPUHVM_online                 (1<<_VCPUHVM_online )
> 
> What is this needed for?

It clears the VPF_down bit of the vcpu pause_flags, but we can also do
this using the VCPUOP_up hypercall so I'm just going to remove it.

>>     uint32_t flags;                         /* VCPUHVM_* flags.   */
>>     struct cpu_hvm_regs user_regs;          /* CPU registers.     */
>> };
>>
>> #if defined(__GNUC__) && !defined(__STRICT_ANSI__)
>> /* Anonymous union includes both 32- and 64-bit names (e.g., ebp/rbp). */
>> # define __DECL_REG(n64, n32) union {          \
>>         uint64_t n64;                          \
>>         uint32_t n32;                          \
>>     }
>> #else
>> /* Non-gcc sources must always use the proper 64-bit name (e.g., rbp). */
>> #define __DECL_REG(n64, n32) uint64_t n64
>> #endif
>>
>> #define __DECL_GP_REG(n) __DECL_REG(r##n, e##n)
>>
>> struct cpu_hvm_regs {
>>     /* General purpose registers. */
>>     __DECL_GP_REG(bx);
>>     __DECL_GP_REG(cx);
>>     __DECL_GP_REG(dx);
>>     __DECL_GP_REG(si);
>>     __DECL_GP_REG(di);
>>     __DECL_GP_REG(bp);
>>     __DECL_GP_REG(ax);
>>     __DECL_GP_REG(ip);
>>     __DECL_GP_REG(sp);
> 
> Please put GPRs in their natural order - no need to mimic the order
> in any existing structure.

Ack.

>>     __DECL_GP_REG(flags);
> 
> r8-r11, selector and descriptor registers, pseudo descriptor registers.
> Or else for all of them their default state would need to be spelled out.

r8-r15 right?

> 
>>     /* Control registers.         */
>>     uint64_t cr[8];
>>     /* Valid on amd64 only.       */
> 
> Fields valid/useful in one mode only should probably be put in
> union-ized sub-structures.

Do you mean something like:

    union {
        uint64_t efer;
        uint32_t __invalid32;
        uint16_t __invalid16;
    }

It seems kind of pointless IMHO, the reason to have the union is to be
able to access the registers using the native nomenclature, but if a
register doesn't exist in a specific bitness I don't see the point of
adding such "invalid" names.

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.

Roger.