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