From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH 11/20] PVH xen: introduce pvh.c Date: Wed, 15 May 2013 11:42:34 +0100 Message-ID: <519382BA02000078000D6658@nat28.tlf.novell.com> References: <1368579168-30829-1-git-send-email-mukesh.rathor@oracle.com> <1368579168-30829-12-git-send-email-mukesh.rathor@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1368579168-30829-12-git-send-email-mukesh.rathor@oracle.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Mukesh Rathor Cc: xen-devel List-Id: xen-devel@lists.xenproject.org >>> On 15.05.13 at 02:52, Mukesh Rathor wrote: > --- /dev/null > +++ b/xen/arch/x86/hvm/pvh.c > @@ -0,0 +1,202 @@ > +/* > + * Copyright (C) 2013, Mukesh Rathor, Oracle Corp. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public > + * License v2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * General Public License for more details. > + * > + */ > +#include > +#include > +#include > +#include > +#include > +#include > + > + > +static int pvh_grant_table_op(unsigned int cmd, XEN_GUEST_HANDLE(void) uop, > + unsigned int count) > +{ > +#ifndef NDEBUG The whole function should be in such a conditional (if it's really needed, which I previously said I doubt). Without doing so, and with the way you have pvh_hypercall64_table[], build will fail for debug=n. > + switch ( cmd ) > + { > + /* the following grant ops have been tested for PVH guest. */ > + case GNTTABOP_map_grant_ref: > + case GNTTABOP_unmap_grant_ref: > + case GNTTABOP_setup_table: > + case GNTTABOP_copy: > + case GNTTABOP_query_size: > + case GNTTABOP_set_version: > + return do_grant_table_op(cmd, uop, count); > + } > + return -ENOSYS; > +#else > + return do_grant_table_op(cmd, uop, count); > +#endif > +} > + > +static long pvh_vcpu_op(int cmd, int vcpuid, XEN_GUEST_HANDLE(void) arg) > +{ > + long rc = -ENOSYS; > + > +#ifndef NDEBUG > + int valid = 0; > + > + switch ( cmd ) > + { > + case VCPUOP_register_runstate_memory_area: > + case VCPUOP_get_runstate_info: > + case VCPUOP_set_periodic_timer: > + case VCPUOP_stop_periodic_timer: > + case VCPUOP_set_singleshot_timer: > + case VCPUOP_stop_singleshot_timer: > + case VCPUOP_is_up: > + case VCPUOP_up: > + case VCPUOP_initialise: > + valid = 1; > + } > + if ( !valid ) > + return rc; > +#endif > + > + rc = do_vcpu_op(cmd, vcpuid, arg); > + > + /* pvh boot vcpu setting context for bringing up smp vcpu */ > + if ( cmd == VCPUOP_initialise ) > + vmx_vmcs_enter(current); This is wrong in three ways - for one, you can't call a vmx function from here, then the operation also doesn't appear to belong here, and with pvh_hypercall64_table[] not even existing in non-debug builds this won't happen there at all (which is making very clear that these function overrides are plain wrong, as I had tried to tell you from the beginning). > + return rc; > +} > + > +static long pvh_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg) > +{ > +#ifndef NDEBUG Same as for grant table op above. > + int valid = 0; > + switch ( cmd ) > + { > + case PHYSDEVOP_map_pirq: > + case PHYSDEVOP_unmap_pirq: > + case PHYSDEVOP_eoi: > + case PHYSDEVOP_irq_status_query: > + case PHYSDEVOP_get_free_pirq: > + valid = 1; > + } > + if ( !valid && !IS_PRIV(current->domain) ) > + return -ENOSYS; > +#endif > + return do_physdev_op(cmd, arg); > +} > + > +static long pvh_hvm_op(unsigned long op, XEN_GUEST_HANDLE(void) arg) > +{ > + long rc = -EINVAL; > + struct xen_hvm_param harg; > + struct domain *d; > + > + if ( copy_from_guest(&harg, arg, 1) ) > + return -EFAULT; > + > + rc = rcu_lock_target_domain_by_id(harg.domid, &d); > + if ( rc != 0 ) > + return rc; > + > + if ( is_hvm_domain(d) ) > + { > + /* pvh dom0 is building an hvm guest */ > + rcu_unlock_domain(d); > + return do_hvm_op(op, arg); > + } > + > + rc = -ENOSYS; > + if ( op == HVMOP_set_param ) > + { > + if ( harg.index == HVM_PARAM_CALLBACK_IRQ ) > + { > + struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq; > + uint64_t via = harg.value; > + uint8_t via_type = (uint8_t)(via >> 56) + 1; > + > + if ( via_type == HVMIRQ_callback_vector ) > + { > + hvm_irq->callback_via_type = HVMIRQ_callback_vector; > + hvm_irq->callback_via.vector = (uint8_t)via; > + rc = 0; > + } > + } > + } > + rcu_unlock_domain(d); > + if ( rc ) > + gdprintk(XENLOG_DEBUG, "op:%ld -ENOSYS\n", op); This should be dropped. > + > + return rc; > +} > + > +#ifndef NDEBUG > +/* PVH 32bitfixme */ > +static hvm_hypercall_t *pvh_hypercall64_table[NR_hypercalls] = { > + [__HYPERVISOR_platform_op] = (hvm_hypercall_t *)do_platform_op, > + [__HYPERVISOR_memory_op] = (hvm_hypercall_t *)do_memory_op, > + [__HYPERVISOR_xen_version] = (hvm_hypercall_t *)do_xen_version, > + [__HYPERVISOR_console_io] = (hvm_hypercall_t *)do_console_io, > + [__HYPERVISOR_grant_table_op] = (hvm_hypercall_t *)pvh_grant_table_op, > + [__HYPERVISOR_vcpu_op] = (hvm_hypercall_t *)pvh_vcpu_op, > + [__HYPERVISOR_mmuext_op] = (hvm_hypercall_t *)do_mmuext_op, > + [__HYPERVISOR_xsm_op] = (hvm_hypercall_t *)do_xsm_op, > + [__HYPERVISOR_sched_op] = (hvm_hypercall_t *)do_sched_op, > + [__HYPERVISOR_event_channel_op]= (hvm_hypercall_t *)do_event_channel_op, > + [__HYPERVISOR_physdev_op] = (hvm_hypercall_t *)pvh_physdev_op, > + [__HYPERVISOR_hvm_op] = (hvm_hypercall_t *)pvh_hvm_op, > + [__HYPERVISOR_sysctl] = (hvm_hypercall_t *)do_sysctl, > + [__HYPERVISOR_domctl] = (hvm_hypercall_t *)do_domctl > +}; > +#endif > + > +/* > + * Check if hypercall is valid > + * Returns: 0 if hcall is not valid with eax set to the errno to ret to guest > + */ > +static bool_t hcall_valid(struct cpu_user_regs *regs) > +{ > + struct segment_register sreg; > + > + hvm_get_segment_register(current, x86_seg_ss, &sreg); > + if ( unlikely(sreg.attr.fields.dpl != 0) ) > + { > + regs->eax = -EPERM; > + return 0; > + } > + > + return 1; > +} > + > +/* PVH 32bitfixme */ > +int pvh_do_hypercall(struct cpu_user_regs *regs) > +{ > + uint32_t hnum = regs->eax; > + > + if ( hnum >= NR_hypercalls || pvh_hypercall64_table[hnum] == NULL ) > + { > + gdprintk(XENLOG_WARNING, "PVH: Unimplemented HCALL:%d. Returning " > + "-ENOSYS. domid:%d IP:%lx SP:%lx\n", > + hnum, current->domain->domain_id, regs->rip, regs->rsp); > + regs->eax = -ENOSYS; > + vmx_update_guest_eip(); > + return HVM_HCALL_completed; > + } > + > + if ( !hcall_valid(regs) ) > + return HVM_HCALL_completed; > + > + current->arch.hvm_vcpu.hcall_preempted = 0; > + regs->rax = pvh_hypercall64_table[hnum](regs->rdi, regs->rsi, regs->rdx, > + regs->r10, regs->r8, regs->r9); Another build error with debug=n? > + > + if ( current->arch.hvm_vcpu.hcall_preempted ) > + return HVM_HCALL_preempted; > + > + return HVM_HCALL_completed; > +} Jan