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

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

El 23/07/15 a les 13.41, Ian Campbell ha escrit:
> On Thu, 2015-07-23 at 05:29 -0600, Jan Beulich wrote:
>>>
>>>>> On 23.07.15 at 12:25, <roger.pau@citrix.com> wrote:
>>> El 13/07/15 a les 16.01, Jan Beulich ha escrit:
>>>>>>> On 03.07.15 at 13:34, <roger.pau@citrix.com> wrote:
>>>>> --- a/xen/arch/x86/domain.c
>>>>> +++ b/xen/arch/x86/domain.c
>>>>> @@ -795,6 +795,15 @@ int arch_set_info_guest(
>>>>>                c.nat->fs_base || c.nat->gs_base_user)) )
>>>>>              return -EINVAL;
>>>>>      }
>>>>> +    else if ( is_hvm_domain(d) )
>>>>> +    {
>>>>> +        if ( c(ctrlreg[0]) || c(ctrlreg[1]) || c(ctrlreg[2]) 
>>>>> ||
>>>>> +             c(ctrlreg[3]) || c(ctrlreg[4]) || c(ctrlreg[5]) 
>>>>> ||
>>>>> +             c(ctrlreg[6]) || c(ctrlreg[7]) || c(ldt_base) ||
>>>>> +             c(ldt_ents) || c(kernel_ss) || c(kernel_sp) ||
>>>>> +             c(gdt_ents) )
>>>>> +            return -EINVAL;
>>>>> +    }
>>>>
>>>> In addition to what Andrew said - is the use of c() here really 
>>>> correct
>>>> considering
>>>>
>>>>     compat = is_pv_32bit_domain(d);
>>>>
>>>> #define c(fld) (compat ? (c.cmp->fld) : (c.nat->fld))
>>>>
>>>> near the beginning of the function?
>>>
>>> I've been thinking about this. Since HVM vCPUs are always started 
>>> in
>>> 32bit mode, I would ideally like to use the 
>>> vcpu_guest_context_x86_32_t
>>> struct to set the vCPU context. This means that for HVM guests 
>>> "compat"
>>> should always be true.
>>>
>>> The problem is that inside of the guest there's no notion of
>>> vcpu_guest_context_x86_32_t or vcpu_guest_context_x86_64_t, there's 
>>> only
>>> vcpu_guest_context, which defaults to the native bitness of the 
>>> guest.
>>> So if your guest is running in 64bit mode vcpu_guest_context is 
>>> aliased
>>> to vcpu_guest_context_x86_64_t by default.
>>>
>>> TBH I'm not sure about the best way to solve this. Should
>>> vcpu_guest_context_x86_32_t and vcpu_guest_context_x86_64_t be 
>>> exposed
>>> to the guest like it's done for the tools?
>>
>> Why? We're in arch_set_info_guest(), which is unreachable for a
>> HVM guest on itself. Or is this in preparation of allowing HVM
>> guests to use VCPUOP_initialise? In that case, exposing might be
>> an option, but remember that the compat mode layout even today
>> is used only for PV guests. So I'd rather avoid exposing both
>> layouts to guests, and do translation instead inside the hypervisor.
> 
> On ARM we deliberately arranged that the vcpu_guest_context was the
> same for both arm32 and arm64. Moving to PVH seems like an ideal
> opportunity to do something similar for x86, if not by standardising on
> the x86_64 layout then by declaring a new struct which can encompass
> both and declaring the old ones PV legacy.

IMHO introducing a new structure that gets rid of all the PV-only 
fields seems like a good option:

struct vcpu_hvm_context {
#define _VGCF_online                   5
#define VGCF_online                    (1<<_VGCF_online)
    uint32_t flags;                         /* VGCF_* flags                 */
    struct cpu_hvm_user_regs user_regs;     /* User-level CPU registers     */
    /* NB. User pagetable on x86/64 is placed in ctrlreg[1]. */
    uint32_t ctrlreg[8];                    /* CR0-CR7 (control registers)  */
    uint32_t debugreg[8];                   /* DB0-DB7 (debug registers)    */
};

I'm also seriously considering getting rid of ctrlreg and debugreg.
Since HVM VCPUs will always be started in 32bit flat mode, it doesn't
make sense IMHO to have both the 32 and the 64 version of the 
registers, so cpu_hvm_user_regs is always going to be:

struct cpu_hvm_user_regs {
    uint32_t ebx;
    uint32_t ecx;
    uint32_t edx;
    uint32_t esi;
    uint32_t edi;
    uint32_t ebp;
    uint32_t eax;
    uint32_t eip;
    uint32_t esp;
    uint32_t eflags;
    uint16_t cs;
    uint16_t ss;
    uint16_t es;
    uint16_t ds;
    uint16_t fs;
    uint16_t gs;
};

We could however do something similar to what's done in ARM and have
a union of both the 32 and the 64bit registers in case we want to
start the vCPU in 64bit mode sometime in the future.

Roger.