Re: [Xen-devel] [PATCH V1 1/2] x86/altp2m: Add hypercall to set a range of sve bits

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On 05.11.2019 17:18, Tamas K Lengyel wrote:
> On Tue, Nov 5, 2019 at 5:43 AM Alexandru Stefan ISAILA
> <aisaila@xxxxxxxxxxxxxxx> wrote:
>>
>> By default the sve bits are not set.
>> This patch adds a new hypercall, xc_altp2m_set_supress_ve_multi(),
>> to set a range of sve bits.
>> The core function, p2m_set_suppress_ve_multi(), does not brake in case
>> of a error and it is doing a best effort for setting the bits in the
>> given range. A check for continuation is made in order to have
>> preemption on big ranges.
>>
>> Signed-off-by: Alexandru Isaila <aisaila@xxxxxxxxxxxxxxx>
>> ---
>>   tools/libxc/include/xenctrl.h   |  3 ++
>>   tools/libxc/xc_altp2m.c         | 25 ++++++++++++++
>>   xen/arch/x86/hvm/hvm.c          | 28 +++++++++++++--
>>   xen/arch/x86/mm/p2m.c           | 61 +++++++++++++++++++++++++++++++++
>>   xen/include/public/hvm/hvm_op.h |  4 ++-
>>   xen/include/xen/mem_access.h    |  3 ++
>>   6 files changed, 121 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>> index f4431687b3..21b644f459 100644
>> --- a/tools/libxc/include/xenctrl.h
>> +++ b/tools/libxc/include/xenctrl.h
>> @@ -1923,6 +1923,9 @@ int xc_altp2m_switch_to_view(xc_interface *handle, 
>> uint32_t domid,
>>                                uint16_t view_id);
>>   int xc_altp2m_set_suppress_ve(xc_interface *handle, uint32_t domid,
>>                                 uint16_t view_id, xen_pfn_t gfn, bool sve);
>> +int xc_altp2m_set_supress_ve_multi(xc_interface *handle, uint32_t domid,
>> +                                   uint16_t view_id, xen_pfn_t start_gfn,
>> +                                   uint32_t nr, bool sve);
>>   int xc_altp2m_get_suppress_ve(xc_interface *handle, uint32_t domid,
>>                                 uint16_t view_id, xen_pfn_t gfn, bool *sve);
>>   int xc_altp2m_set_mem_access(xc_interface *handle, uint32_t domid,
>> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
>> index 09dad0355e..6605d9abbe 100644
>> --- a/tools/libxc/xc_altp2m.c
>> +++ b/tools/libxc/xc_altp2m.c
>> @@ -234,6 +234,31 @@ int xc_altp2m_set_suppress_ve(xc_interface *handle, 
>> uint32_t domid,
>>       return rc;
>>   }
>>
>> +int xc_altp2m_set_supress_ve_multi(xc_interface *handle, uint32_t domid,
>> +                                   uint16_t view_id, xen_pfn_t start_gfn,
>> +                                   uint32_t nr, bool sve)
>> +{
>> +    int rc;
>> +    DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
>> +
>> +    arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
>> +    if ( arg == NULL )
>> +        return -1;
>> +
>> +    arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
>> +    arg->cmd = HVMOP_altp2m_set_suppress_ve_multi;
>> +    arg->domain = domid;
>> +    arg->u.suppress_ve.view = view_id;
>> +    arg->u.suppress_ve.gfn = start_gfn;
>> +    arg->u.suppress_ve.suppress_ve = sve;
>> +    arg->u.suppress_ve.nr = nr;
>> +
>> +    rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
>> +                  HYPERCALL_BUFFER_AS_ARG(arg));
>> +    xc_hypercall_buffer_free(handle, arg);
>> +    return rc;
>> +}
>> +
>>   int xc_altp2m_set_mem_access(xc_interface *handle, uint32_t domid,
>>                                uint16_t view_id, xen_pfn_t gfn,
>>                                xenmem_access_t access)
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index 06a7b40107..d3d9f8c30f 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -4535,6 +4535,7 @@ static int do_altp2m_op(
>>       case HVMOP_altp2m_destroy_p2m:
>>       case HVMOP_altp2m_switch_p2m:
>>       case HVMOP_altp2m_set_suppress_ve:
>> +    case HVMOP_altp2m_set_suppress_ve_multi:
>>       case HVMOP_altp2m_get_suppress_ve:
>>       case HVMOP_altp2m_set_mem_access:
>>       case HVMOP_altp2m_set_mem_access_multi:
>> @@ -4681,7 +4682,7 @@ static int do_altp2m_op(
>>           break;
>>
>>       case HVMOP_altp2m_set_suppress_ve:
>> -        if ( a.u.suppress_ve.pad1 || a.u.suppress_ve.pad2 )
>> +        if ( a.u.suppress_ve.pad1 )
>>               rc = -EINVAL;
>>           else
>>           {
>> @@ -4693,8 +4694,31 @@ static int do_altp2m_op(
>>           }
>>           break;
>>
>> +    case HVMOP_altp2m_set_suppress_ve_multi:
>> +        if ( a.u.suppress_ve.pad1 || !a.u.suppress_ve.nr )
>> +            rc = -EINVAL;
>> +        else
>> +        {
>> +            rc = p2m_set_suppress_ve_multi(d, a.u.suppress_ve.gfn,
>> +                                           a.u.suppress_ve.nr,
>> +                                           a.u.suppress_ve.suppress_ve,
>> +                                           a.u.suppress_ve.view);
> 
> I have to say I'm not a fan of stuffing the current gfn progress into
> rc, perhaps a separate pointer being passed in for storing that and
> returning -ERESTART would be cleaner.

This sounds cleaner, I will have it changed in v2.

> 
>> +            if ( rc > 0 )
>> +            {
>> +                a.u.suppress_ve.gfn = rc;
> 
> There had been discussion in the past whether its acceptable to
> overwrite fields that were passed in like this. This may not be the
> expected behavior. For the mem_sharing side at least we have
> introduced an "opaque" field in the structure to store that
> continuation value (see
> https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/include/public/memory.h;h=cfdda6e2a87ed30bed590034d8d717674abfbc79;hb=HEAD#l524).

I wanted to keep the size of the struct. It it is ok to have a new 
uint64_t opaque here and then pad the rest of the structures then I will 
change this in v2.

Thanks,
Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel