[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC v12 10/21] pvh: PVH access to hypercalls
>>> On 13.09.13 at 18:25, George Dunlap <george.dunlap@xxxxxxxxxxxxx> wrote: > Hypercalls where we now have unrestricted access: > * memory_op > * console_io > * vcpu_op > * mmuext_op > > We also restrict PVH domain access to HVMOP_*_param to writing > HVM_PARAM_CALLBACK_IRQ. > > Most hvm_op functions require "is_hvm_domain()" and will default to > -EINVAL; exceptions are HVMOP_get_time and HVMOP_xentrace. > > Finally, we restrict setting IOPL permissions for a PVH domain. > > Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx> > Signed-off-by: Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx> again with a couple of comments: > @@ -3378,7 +3396,9 @@ int hvm_do_hypercall(struct cpu_user_regs *regs) > if ( (eax & 0x80000000) && is_viridian_domain(curr->domain) ) > return viridian_hypercall(regs); > > - if ( (eax >= NR_hypercalls) || !hvm_hypercall32_table[eax] ) > + if ( (eax >= NR_hypercalls) || > + (is_pvh_vcpu(curr) && !pvh_hypercall64_table[eax]) || > + (is_hvm_vcpu(curr) && !hvm_hypercall32_table[eax]) ) This sort of gives the impression that the is_pv_() case isn't handled. Realizing that PV can't make it here, doing this with the ?: operator would seem more natural (matching the if/else further down around doing the actual call). > @@ -3393,16 +3413,20 @@ int hvm_do_hypercall(struct cpu_user_regs *regs) > regs->r10, regs->r8, regs->r9); > > curr->arch.hvm_vcpu.hcall_64bit = 1; > - regs->rax = hvm_hypercall64_table[eax](regs->rdi, > - regs->rsi, > - regs->rdx, > - regs->r10, > - regs->r8, > - regs->r9); > + if ( is_pvh_vcpu(curr) ) > + regs->rax = pvh_hypercall64_table[eax](regs->rdi, regs->rsi, > + regs->rdx, regs->r10, > + regs->r8, regs->r9); > + else > + regs->rax = hvm_hypercall64_table[eax](regs->rdi, regs->rsi, > + regs->rdx, regs->r10, > + regs->r8, regs->r9); > curr->arch.hvm_vcpu.hcall_64bit = 0; > } > else > { > + ASSERT(!is_pvh_vcpu(curr)); /* PVH 32bitfixme. */ > + And this would then possibly better be ASSERT(is_hvm_vcpu(curr)); /* PVH 32bitfixme. */ > @@ -3827,7 +3851,12 @@ long do_hvm_op(unsigned long op, > XEN_GUEST_HANDLE_PARAM(void) arg) > return -ESRCH; > > rc = -EINVAL; > - if ( !is_hvm_domain(d) ) > + if ( is_pv_domain(d) ) This would be a case where I would see better fit for !has_hvm_container...(), if we already need to have that. > + goto param_fail; > + > + if ( is_pvh_domain(d) > + && ( a.index != HVM_PARAM_CALLBACK_IRQ > + || op != HVMOP_set_param ) ) Extra blanks inside the inner parentheses. > --- a/xen/arch/x86/physdev.c > +++ b/xen/arch/x86/physdev.c > @@ -519,6 +519,11 @@ ret_t do_physdev_op(int cmd, > XEN_GUEST_HANDLE_PARAM(void) arg) > > case PHYSDEVOP_set_iopl: { > struct physdev_set_iopl set_iopl; > + > + ret = -EINVAL; > + if ( is_pvh_vcpu(current) ) > + break; Now this is a case where -ENOSYS would actually make sense: The sub-hypercall is to be considered not implemented for PVH guests. > --- a/xen/common/kernel.c > +++ b/xen/common/kernel.c > @@ -293,6 +293,10 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) > arg) > fi.submap |= (1U << XENFEAT_mmu_pt_update_preserve_ad) | > (1U << XENFEAT_highmem_assist) | > (1U << XENFEAT_gnttab_map_avail_bits); > + else if ( is_pvh_vcpu(current) ) > + fi.submap |= (1U << XENFEAT_hvm_safe_pvclock) | > + (1U << XENFEAT_supervisor_mode_kernel) | > + (1U << XENFEAT_hvm_callback_vector); > else > fi.submap |= (1U << XENFEAT_hvm_safe_pvclock) | > (1U << XENFEAT_hvm_callback_vector) | This once again would better be a switch statement now that we're dealing with an enumerated type. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |