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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.04.13 at 02:57, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> On Wed, 24 Apr 2013 09:47:55 +0100 "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>> >>> On 23.04.13 at 23:25, 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 )
>> > +    {
>> > +        /*
>> > +         * 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...

Then this patch series really ought to continue to be RFC, and
I start questioning why I'm spending hours reviewing it. The
number of hacks you need clearly should be limited - to me it is
unacceptable to scatter half done code all over the tree. I had
the same problem when I did the 32-on-64 support, and iirc I
got things into largely hack free state before even posting the
first full, non-RFC series.

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

And of course the comment applies to all of them.

>> > +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.

Having that extra stuff only in debug builds would at least look
slightly more acceptable.

> 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?

Yes - memcpy() the whole table, then overwrite the (few) entries
you need to overwrite. After all, in the long run adding a new
hypercall ought to "just work" for PVH (and in most cases even for
HVM).

Jan

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