Re: [PATCH v7 27/32] xen/x86: allow HVM guests to use hypercalls to bring up vCPUs

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

El 19/10/15 a les 17.48, Jan Beulich ha escrit:
>>>> On 02.10.15 at 17:48, <roger.pau@citrix.com> wrote:
>> @@ -1176,6 +1177,190 @@ int arch_set_info_guest(
>>  #undef c
>>  }
>>  
>> +/* Called by VCPUOP_initialise for HVM guests. */
>> +int arch_set_info_hvm_guest(struct vcpu *v, vcpu_hvm_context_t *ctx)
> 
> const ... *ctx

Sure.

>> +{
>> +    struct cpu_user_regs *uregs = &v->arch.user_regs;
>> +    struct segment_register cs, ds, ss, es, tr;
>> +
>> +    switch ( ctx->mode )
>> +    {
>> +    default:
>> +        return -EINVAL;
>> +
>> +    case VCPU_HVM_MODE_32B:
>> +    {
>> +        const struct vcpu_hvm_x86_32 *regs = &ctx->cpu_regs.x86_32;
>> +        uint32_t limit;
>> +
>> +#define SEG(s, r)                                                       \
>> +    (struct segment_register){ .sel = 0, .base = (r)->s ## _base,       \
>> +            .limit = (r)->s ## _limit, .attr.bytes = (r)->s ## _ar }
>> +        cs = SEG(cs, regs);
>> +        ds = SEG(ds, regs);
>> +        ss = SEG(ss, regs);
>> +        es = SEG(es, regs);
>> +        tr = SEG(tr, regs);
>> +#undef SEG
>> +
>> +        /* Basic sanity checks. */
>> +        if ( cs.attr.fields.pad != 0 || ds.attr.fields.pad != 0 ||
>> +             ss.attr.fields.pad != 0 || es.attr.fields.pad != 0 ||
>> +             tr.attr.fields.pad != 0 )
>> +        {
>> +            gprintk(XENLOG_ERR, "Attribute bits 12-15 of the segments are not null\n");
>> +            return -EINVAL;
>> +        }
>> +
>> +        limit = cs.limit * (cs.attr.fields.g ? PAGE_SIZE : 1);
>> +        if ( regs->eip > limit )
>> +        {
>> +            gprintk(XENLOG_ERR, "EIP address is outside of the CS limit\n");
>> +            return -EINVAL;
>> +        }
>> +
>> +        if ( ds.attr.fields.dpl > cs.attr.fields.dpl )
> 
> Checks like this imo need to take into account cases where the effect
> of a null selector loaded into the register is intended (in which case I
> would assume DPL to not matter). Speaking of which - with all these
> DPL checks done, what about non-code segments loaded into CS or
> other illegal things? Question is whether the
> hvm_set_segment_register() calls below could be made take care of
> these instead of having to enumerate everything here.

hvm_set_segment_register is just an inline wrapper around
hvm_funcs.set_segment_register. I could turn that into a proper function
with checks, but it's a shame because hvm_load_segment_selector also
performs some of this checks, but it requires a valid GDT to be loaded
in order to use it which we don't have.

I don't mind adding some more checks to the current ones:

 - Check that all segments that are not null selectors have the
'present' bit set.
 - Check that CS.type matches a code segment.
 - Check that all segments except CS don't have the 'code' type.
 - Don't perform the DPL check if the segment is a null selector.

I'm adding a small inline stub to do this checks.

>> --- a/xen/common/compat/domain.c
>> +++ b/xen/common/compat/domain.c
>> @@ -10,6 +10,9 @@
>>  #include <xen/guest_access.h>
>>  #include <xen/hypercall.h>
>>  #include <compat/vcpu.h>
>> +#ifdef CONFIG_X86
>> +#include <compat/hvm/hvm_vcpu.h>
>> +#endif
> 
> I'd avoid such #if-s in this file, since it's only x86 that uses compat
> code right now.

OK, knowing that the compat code is only used in x86 helps to simplify 
some of this code also.

>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -1207,11 +1207,35 @@ void unmap_vcpu_info(struct vcpu *v)
>>      put_page_and_type(mfn_to_page(mfn));
>>  }
>>  
>> +static int default_initialize_vcpu(struct vcpu *v,
>> +                                   XEN_GUEST_HANDLE_PARAM(void) arg)
>> +{
>> +    struct vcpu_guest_context *ctxt;
>> +    struct domain *d = v->domain;
>> +    int rc;
>> +
>> +    if ( (ctxt = alloc_vcpu_guest_context()) == NULL )
>> +            return -ENOMEM;
>> +
>> +    if ( copy_from_guest(ctxt, arg, 1) )
>> +    {
>> +        free_vcpu_guest_context(ctxt);
>> +        return -EFAULT;
>> +    }
>> +
>> +    domain_lock(d);
>> +    rc = v->is_initialised ? -EEXIST : arch_set_info_guest(v, ctxt);
>> +    domain_unlock(d);
>> +
>> +    free_vcpu_guest_context(ctxt);
>> +
>> +    return rc;
>> +}
>> +
>>  long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
>>  {
>>      struct domain *d = current->domain;
>>      struct vcpu *v;
>> -    struct vcpu_guest_context *ctxt;
>>      long rc = 0;
>>  
>>      if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
>> @@ -1223,20 +1247,28 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
>>          if ( v->vcpu_info == &dummy_vcpu_info )
>>              return -EINVAL;
>>  
>> -        if ( (ctxt = alloc_vcpu_guest_context()) == NULL )
>> -            return -ENOMEM;
>> -
>> -        if ( copy_from_guest(ctxt, arg, 1) )
>> +#if defined(CONFIG_X86)
> 
> Looks like you went from one extreme to the other: Now there's no
> per-arch function anymore, and hence you need this ugly #ifdef-ery.
> Why don't you add default_initialize_vcpu() as a non-static function,
> to be called by or (on ARM) used as arch_initialize_vcpu()?

OK, I think I've managed to fix some of this mess by re-adding the 
arch_initialize_vcpu function, at least we don't have all this 
ifdef-ery in the common domain.c any more.

>> --- a/xen/include/asm-x86/domain.h
>> +++ b/xen/include/asm-x86/domain.h
>> @@ -10,6 +10,7 @@
>>  #include <asm/mce.h>
>>  #include <public/vcpu.h>
>>  #include <public/hvm/hvm_info_table.h>
>> +#include <public/hvm/hvm_vcpu.h>
> 
> Please avoid this in headers so widely included:
> 
>> @@ -599,6 +600,8 @@ static inline void free_vcpu_guest_context(struct vcpu_guest_context *vgc)
>>      vfree(vgc);
>>  }
>>  
>> +int arch_set_info_hvm_guest(struct vcpu *v, vcpu_hvm_context_t *ctx);
> 
> If you replace the typedef name by the actual structure, and if you
> forward declare the structure, you don't have any build issue, but
> you will avoid almost every file including the new header.

Done.

> 
>> +struct vcpu_hvm_x86_32 {
>> +    uint32_t eax;
>> +    uint32_t ecx;
>> +    uint32_t edx;
>> +    uint32_t ebx;
>> +    uint32_t esp;
>> +    uint32_t ebp;
>> +    uint32_t esi;
>> +    uint32_t edi;
>> +    uint32_t eip;
>> +    uint32_t eflags;
>> +
>> +    uint32_t cr0;
>> +    uint32_t cr3;
>> +    uint32_t cr4;
>> +
>> +    uint32_t pad1;
>> +
>> +    /*
>> +     * EFER should only be used to set the NXE bit (if required)
>> +     * when starting a vCPU in 32bit mode with paging enabled or
>> +     * to set the LME/LMA bits in order to start the vCPU in
>> +     * compatibility mode.
>> +     */
>> +    uint64_t efer;
>> +
>> +    uint32_t cs_base;
>> +    uint32_t ds_base;
>> +    uint32_t ss_base;
>> +    uint32_t es_base;
>> +    uint32_t tr_base;
>> +    uint32_t cs_limit;
>> +    uint32_t ds_limit;
>> +    uint32_t ss_limit;
>> +    uint32_t es_limit;
>> +    uint32_t tr_limit;
>> +    uint16_t cs_ar;
>> +    uint16_t ds_ar;
>> +    uint16_t ss_ar;
>> +    uint16_t es_ar;
>> +    uint16_t tr_ar;
>> +
>> +    uint16_t pad2[2];
> 
> [3] perhaps?

Right, not sure why I've missed that one, probably changed some fields 
and I didn't update the explicit padding to match.

> 
> Also I don't think I've seen you check these padding fields to be zero,
> implying that we wouldn't be able to assign meaning to them later on.

Done.

> 
>> --- a/xen/include/xlat.lst
>> +++ b/xen/include/xlat.lst
>> @@ -56,6 +56,9 @@
>>  ?       grant_entry_header              grant_table.h
>>  ?	grant_entry_v2			grant_table.h
>>  ?	gnttab_swap_grant_ref		grant_table.h
>> +?	vcpu_hvm_context		hvm/hvm_vcpu.h
>> +?	vcpu_hvm_x86_32			hvm/hvm_vcpu.h
>> +?	vcpu_hvm_x86_64			hvm/hvm_vcpu.h
> 
> Do you really need all three added here, instead of just the
> top level one?

Unless I add all 3 here I get the following error:

/root/src/xen/xen/include/compat/xlat.h:540:5: error: data definition has no type or storage class [-Werror]
     CHECK_compat_vcpu_hvm_x86_32; \
     ^
domain.c:30:1: note: in expansion of macro ‘CHECK_vcpu_hvm_context’
 CHECK_vcpu_hvm_context;
 ^
/root/src/xen/xen/include/compat/xlat.h:540:5: error: type defaults to ‘int’ in declaration of ‘CHECK_compat_vcpu_hvm_x86_32’ [-Werror]
     CHECK_compat_vcpu_hvm_x86_32; \
     ^
domain.c:30:1: note: in expansion of macro ‘CHECK_vcpu_hvm_context’
 CHECK_vcpu_hvm_context;
 ^
/root/src/xen/xen/include/compat/xlat.h:541:5: error: data definition has no type or storage class [-Werror]
     CHECK_compat_vcpu_hvm_x86_64
     ^
domain.c:30:1: note: in expansion of macro ‘CHECK_vcpu_hvm_context’
 CHECK_vcpu_hvm_context;
 ^
/root/src/xen/xen/include/compat/xlat.h:541:5: error: type defaults to ‘int’ in declaration of ‘CHECK_compat_vcpu_hvm_x86_64’ [-Werror]
     CHECK_compat_vcpu_hvm_x86_64
     ^
domain.c:30:1: note: in expansion of macro ‘CHECK_vcpu_hvm_context’
 CHECK_vcpu_hvm_context;
 ^
cc1: all warnings being treated as errors

Thanks, Roger.