Re: [Xen-devel] [PATCH 11/20] PVH xen: introduce pvh.c

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.05.13 at 02:52, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> 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 <xen/hypercall.h>
> +#include <xen/guest_access.h>
> +#include <asm/p2m.h>
> +#include <asm/traps.h>
> +#include <asm/hvm/vmx/vmx.h>
> +#include <public/sched.h>
> +
> +
> +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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel