[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v8 17/21] xen/x86: allow HVM guests to use hypercalls to bring up vCPUs
El 27/11/15 a les 9.00, Jan Beulich ha escrit: >>>> On 26.11.15 at 17:57, <roger.pau@xxxxxxxxxx> wrote: >> El 12/11/15 a les 17.57, Jan Beulich ha escrit: >>>>>> On 06.11.15 at 17:05, <roger.pau@xxxxxxxxxx> wrote: >>>> + if ( reg.attr.fields.pad != 0 ) >>>> + { >>>> + gprintk(XENLOG_ERR, >>>> + "Attribute bits 12-15 of the segment are not zero\n"); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + if ( reg.sel == 0 && reg.base == 0 && reg.limit == 0 && >>> >>> What's the sel check good for when your only caller only ever calls >>> you with it being zero? >> >> I don't mind removing the sel == 0 check but I don't think it hurts either. > > Its presence having confused me means it may confuse other readers. > >>> Looking at base or limit here doesn't seem >>> right either. >> >> I'm sorry but I'm not following you here, why is this not right? Would >> you rather conclude that the user is trying to load a null segment by >> just looking at the attributes field (and checking it's 0)? > > Yes, exactly. Attributes being all zero makes a segment a null one > regardless of base or limit (if anything refusing non-zero base/limit > when attributes are zero as being inconsistent would be an option). Thanks for the feedback, I'm also wondering whether I should call hvm_cr4_guest_reserved_bits and hvm_efer_valid like it's done in hvm_load_cpu_ctxt. Currently we don't perform any of the EFER/CR4 checks in order to make sure that what the user enables is actually allowed. What do you think? >>>> +int arch_initialize_vcpu(struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg) >>>> +{ >>>> + struct domain *d = v->domain; >>>> + int rc; >>>> + >>>> + if ( is_hvm_vcpu(v) ) >>>> + { >>>> + struct vcpu_hvm_context ctxt; >>>> + >>>> + if ( copy_from_guest(&ctxt, arg, 1) ) >>>> + return -EFAULT; >>>> + >>>> + domain_lock(d); >>>> + rc = v->is_initialised ? -EEXIST : arch_set_info_hvm_guest(v, >>>> &ctxt); >>>> + domain_unlock(d); >>>> + } >>>> + else >>>> + { >>>> + struct vcpu_guest_context *ctxt; >>>> + >>>> + 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); >>>> + } >>> >>> This else branch looks suspiciously like the ARM variant, and iirc I >>> had asked already on an earlier version to have this handled in >>> common code (with ARM simply using the common function for its >>> arch_initialize_vcpu()). >> >> Done, I've created a default_initalize_vcpu that's shared between ARM >> and x86 PV guests. The arch_initialize_vcpu implementation on ARM is >> just a stub that calls default_initialize_vcpu. > > I'd actually have expected that to just be a #define, but okay. I wanted to do it as a define, like: #define arch_initialize_vcpu default_initalize_vcpu Inside of arm/domain.h, but that's included before the common domain.h, so the prototype of qrch_initialize_vcpu gets replaced to defaul_initialize_vcpu, and then the compiler complains about duplicate prototypes. I could shuffle them a bit in order to fix it, but I think the stub in arm/domain.c is clearer, and the compiler should optimize it anyway. Roger. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |