Re: [V10 PATCH 18/23] PVH xen: add hypercall support for PVH

[George Dunlap <george.dunlap@eu.citrix.com>]

On 08/08/13 03:12, Mukesh Rathor wrote:
> On Wed, 7 Aug 2013 17:43:54 +0100
> George Dunlap <George.Dunlap@eu.citrix.com> wrote:
>
>> On Wed, Jul 24, 2013 at 2:59 AM, Mukesh Rathor
>> <mukesh.rathor@oracle.com> wrote:
>>> This patch expands HVM hcall support to include PVH.
>>>
>>> Changes in v8:
>>>    - Carve out PVH support of hvm_op to a small function.
>>>
>>> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
>>> ---
>>>   xen/arch/x86/hvm/hvm.c      |   80
>>> +++++++++++++++++++++++++++++++++++++------
>>> xen/arch/x86/x86_64/traps.c |    2 +- 2 files changed, 70
>>> insertions(+), 12 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>> index 383c5cd..6af020e 100644
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -3192,6 +3192,17 @@ static long hvm_vcpu_op(
>>>       case VCPUOP_register_vcpu_time_memory_area:
>>>           rc = do_vcpu_op(cmd, vcpuid, arg);
>>>           break;
>>> +
>>> +    case VCPUOP_is_up:
>>> +    case VCPUOP_up:
>>> +    case VCPUOP_initialise:
>>> +        /* PVH fixme: this white list should be removed eventually
>>> */
>> What do you mean by this?  That PVH won't need these in the future, or
>> that you'll have some other way?
> Just not have these checks here, but just support them all, whatever
> makese sense.

Sorry, I still don't understand -- do you mean you want to eventually 
just allow all VCPUOPs for PVH?

>
>>> +        if ( is_pvh_vcpu(current) )
>>> +            rc = do_vcpu_op(cmd, vcpuid, arg);
>>> +        else
>>> +            rc = -ENOSYS;
>>> +        break;
>>> +
>>>       default:
>>>           rc = -ENOSYS;
>>>           break;
>>> @@ -3312,6 +3323,24 @@ static hvm_hypercall_t *const
>>> hvm_hypercall32_table[NR_hypercalls] = { HYPERCALL(tmem_op)
>>>   };
>>>
>>> +/* PVH 32bitfixme. */
>>> +static hvm_hypercall_t *const pvh_hypercall64_table[NR_hypercalls]
>>> = {
>>> +    HYPERCALL(platform_op),
>>> +    HYPERCALL(memory_op),
>>> +    HYPERCALL(xen_version),
>>> +    HYPERCALL(console_io),
>>> +    [ __HYPERVISOR_grant_table_op ]  = (hvm_hypercall_t
>>> *)hvm_grant_table_op,
>>> +    [ __HYPERVISOR_vcpu_op ]         = (hvm_hypercall_t
>>> *)hvm_vcpu_op,
>>> +    HYPERCALL(mmuext_op),
>>> +    HYPERCALL(xsm_op),
>>> +    HYPERCALL(sched_op),
>>> +    HYPERCALL(event_channel_op),
>>> +    [ __HYPERVISOR_physdev_op ]      = (hvm_hypercall_t
>>> *)hvm_physdev_op,
>>> +    HYPERCALL(hvm_op),
>>> +    HYPERCALL(sysctl),
>>> +    HYPERCALL(domctl)
>>> +};
>> It would be nice if this list were in the same order as the other
>> lists, so that it is easy to figure out what calls are common and what
>> calls are different.
> These are ordered by the hcall number, and assists in the debug.

That makes sense.  What about adding a "prep" patch which re-organizes 
the other lists by hcall number?  I'm not particular about which order, 
I just think they should be the same.

  -George