Re: [PATCH 10/17] PVH xen: introduce vmx_pvh.c and pvh.c

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Wed, 24 Apr 2013 09:47:55 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 23.04.13 at 23:25, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> wrote:
> > +static int pvh_grant_table_op(unsigned int cmd,
> > XEN_GUEST_HANDLE(void) uop,
> > +                              unsigned int count)
> > +{
> > +    switch ( cmd )
> > +    {
> > +        /*
> > +         * Only the following Grant Ops have been verified for PVH
> > guest, > hence
> > +         * we check for them here.
> > +         */
> > +        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;
> > +}
> 
> As said before - I object to this sort of white listing. A PVH guest
> ought to be permitted to issue any hypercall, with the sole
> exception of MMU and very few other ones. So if anything,
> specific hypercall functions should be black listed.

Well, like I said before, these are verified/tested with PVH currently, 
and during the early stages we need to do whatever to catch things as 
bugs come in. I can make it DEBUG only if that makes it easier for you?
I'd rather see a post here saying they got ENOSYS than saying they got
weird crash/hang/etc...

BTW, similar checks exist in hvm_physdev_op(),hvm_vcpu_op()......

> > +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
> > +};
> 
> Is this table complete? What about multicalls, timer_op, kexec_op,
> tmem_op, mca? I again think that copying hypercall_table and
> adjusting the entries you explicitly need to override might be
> better than creating yet another table that needs attention when
> a new hypercall gets added.

Nop.  Like I've said few times before, we are not fully done with PVH with 
this patch set. This patch set establishes min baseline to get linux to 
boot with PVH enabled. Next would be go thru timer_op, look at do_timer_op,
verify/test it out for PVH, and add it here. Then mca, tmem, ....
When we are fully satifisfied with PVH completeness, we can 
remove this table if you wish, or make it DEBUG so one knows right away
what PVH supports at that time.

I am not sure I understant what you mean by copying hypercall_table. You
mean copy all the calls in this table above from entry.S?

> > +/*
> > + * 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 == 3) )
> 
>     if ( unlikely(sreg.attr.fields.dpl != 0) )

Ok, thanks.

> > +    {
> > +        regs->eax = -EPERM;
> > +        return 0;
> > +    }
> > +
> > +    /* Following HCALLs have not been verified for PVH domUs */
> > +    if ( !IS_PRIV(current->domain) &&
> > +         (regs->eax == __HYPERVISOR_xsm_op ||
> > +          regs->eax == __HYPERVISOR_platform_op ||
> > +          regs->eax == __HYPERVISOR_domctl) )       /* for privcmd
> > mmap */
> > +    {
> > +        regs->eax = -ENOSYS;
> > +        return 0;
> > +    }
> 
> This looks bogus - it shouldn't be the job of the generic handler
> to verify permission to use certain hypercalls - the individual
> handlers should be doing this quite well. And I suppose you saw
> Daniel De Graaf's effort to eliminate IS_PRIV() from the tree?

Ok, will remove it. Yup, my next refresh will take care of IS_PRIV.

> > +    if ( regs->eax == __HYPERVISOR_sched_op && regs->rdi ==
> > SCHEDOP_shutdown )
> > +    {
> > +        domain_crash_synchronous();
> > +        return HVM_HCALL_completed;
> > +    }
> 
> ???

Right. my bad, don't need this anymore.

thanks,
Mukesh