|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v7 27/32] xen/x86: allow HVM guests to use hypercalls to bring up vCPUs
El 19/10/15 a les 17.48, Jan Beulich ha escrit:
>>>> On 02.10.15 at 17:48, <roger.pau@xxxxxxxxxx> 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.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |