Re: [Xen-devel] [PATCH 10/18 V2]: PVH xen: introduce vmx_pvh.c and pvh.c

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.03.13 at 01:41, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> +static int pvh_grant_table_op(
> +              unsigned int cmd, XEN_GUEST_HANDLE(void) uop, unsigned int 
> count)
> +{
> +    switch (cmd)
> +    {
> +        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);

While for HVM guests such white lists may be appropriate, for PVH
this doesn't seem to be the case: The code would require updating
whenever a new sub-hypercall gets added to any of the hypercalls
dealt with like this.

> +int hcall_a[NR_hypercalls];

What's this?

> +static pvh_hypercall_t *pvh_hypercall64_table[NR_hypercalls] = {
> +    [__HYPERVISOR_platform_op]     = (pvh_hypercall_t *)do_platform_op,
> +    [__HYPERVISOR_memory_op]       = (pvh_hypercall_t *)do_memory_op,
> +    /* [__HYPERVISOR_set_timer_op]     = (pvh_hypercall_t *)do_set_timer_op, 
> */

Why is this commented out?

> +    [__HYPERVISOR_xen_version]     = (pvh_hypercall_t *)do_xen_version,
> +    [__HYPERVISOR_console_io]      = (pvh_hypercall_t *)do_console_io,
> +    [__HYPERVISOR_grant_table_op]  = (pvh_hypercall_t *)pvh_grant_table_op,
> +    [__HYPERVISOR_vcpu_op]         = (pvh_hypercall_t *)pvh_vcpu_op,
> +    [__HYPERVISOR_mmuext_op]       = (pvh_hypercall_t *)do_mmuext_op,
> +    [__HYPERVISOR_xsm_op]          = (pvh_hypercall_t *)do_xsm_op,
> +    [__HYPERVISOR_sched_op]        = (pvh_hypercall_t *)do_sched_op,
> +    [__HYPERVISOR_event_channel_op]= (pvh_hypercall_t *)do_event_channel_op,
> +    [__HYPERVISOR_physdev_op]      = (pvh_hypercall_t *)pvh_physdev_op,
> +    [__HYPERVISOR_hvm_op]          = (pvh_hypercall_t *)do_pvh_hvm_op,
> +    [__HYPERVISOR_sysctl]          = (pvh_hypercall_t *)do_sysctl,
> +    [__HYPERVISOR_domctl]          = (pvh_hypercall_t *)do_domctl
> +};
> +
> +/* fixme: Do we need to worry about this and slow things down in this path? 
> */

???

> +static int pvh_long_mode_enabled(void)
> +{
> +    /* A 64bit linux guest should always run in this mode with CS.L selecting
> +     * either 64bit mode or 32bit compat mode */
> +    return 1;

How about compat mode guests? And then isn't this equivalent
to !is_pv_32bit_...(), i.e. determined via a global per-domain
flag once an for all?

The way it's now the function is pretty pointless.

> +}
> +
> +/* Check if hypercall is valid 
> + * Returns: 0 if hcall is not valid with eax set to the errno to ret to guest
> + */
> +static int hcall_valid(struct cpu_user_regs *regs)
> +{
> +    struct segment_register sreg;
> +
> +    if (!pvh_long_mode_enabled()) 
> +    {
> +        gdprintk(XENLOG_ERR, "PVH Error: Expected long mode set\n");
> +        return 1;
> +    }
> +    hvm_get_segment_register(current, x86_seg_ss, &sreg);
> +    if ( unlikely(sreg.attr.fields.dpl == 3) ) 
> +    {
> +        regs->eax = -EPERM;
> +        return 0;
> +    }
> +
> +    /* domU's are not allowed following hcalls */
> +    if ( !IS_PRIV(current->domain) &&
> +         (regs->eax == __HYPERVISOR_xsm_op ||
> +          regs->eax == __HYPERVISOR_platform_op ||
> +          regs->eax == __HYPERVISOR_domctl) ) {     /* for privcmd mmap */

Why do you need to do this here? The hypercall handlers should
be checking this as needed, no need to duplicate these checks.

> +
> +        regs->eax = -EPERM;
> +        return 0;
> +    }
> +    return 1;
> +}
> +
> +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 ( regs->eax == __HYPERVISOR_sched_op && regs->rdi == SCHEDOP_shutdown 
> ) 
> +    {
> +        regs->eax = -ENOSYS;
> +        vmx_update_guest_eip();
> +
> +        /* PVH fixme: show_guest_stack() from domain crash causes PF */
> +        domain_crash_synchronous();

What's this all about?

> --- /dev/null
> +++ b/xen/arch/x86/hvm/vmx/vmx_pvh.c

The whole file needs to be looked through for formatting issues.

> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> @@ -189,6 +189,7 @@ void vmx_update_secondary_exec_control(struct vcpu *v);
>   * Access Rights
>   */
>  #define X86_SEG_AR_SEG_TYPE     0xf        /* 3:0, segment type */
> +#define X86_SEG_AR_SEG_TYPE_CODE (1u << 3) /* code (vs data) segment */

Not used anywhere afaics, so what's the point of adding this in an
already big patch?

Jan

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