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

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

On Thu, 25 Apr 2013 09:36:56 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 25.04.13 at 02:57, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> wrote:
> > 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...
> 
> 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.

I really appreciate your time reviewing it. Given the size of the
feature and that I'm the only one working on it, the only way I know
is to do it in steps, and that sometimes requires temporary code.

I'll ifdef DEBUG the above code.

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

How would a poor soul who is trying to find all callers of do_xxx()
find it then? And is it really that often that hypercalls are added
that it is such a big deal?

thanks,
Mukesh