Re: [PATCH V2] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 03/09/2017 06:56 PM, Jan Beulich wrote:
>>>> On 09.03.17 at 10:38, <rcojocaru@bitdefender.com> wrote:
>> @@ -4535,6 +4536,30 @@ static int do_altp2m_op(
>>                                      a.u.set_mem_access.view);
>>          break;
>>  
>> +    case HVMOP_altp2m_set_mem_access_multi:
>> +        if ( a.u.set_mem_access_multi.pad ||
>> +             a.u.set_mem_access_multi.opaque >= a.u.set_mem_access_multi.nr )
>> +        {
>> +            rc = -EINVAL;
>> +            break;
>> +        }
>> +        rc = p2m_set_mem_access_multi(d, a.u.set_mem_access_multi.pfn_list,
>> +                                      a.u.set_mem_access_multi.access_list,
>> +                                      a.u.set_mem_access_multi.nr,
>> +                                      a.u.set_mem_access_multi.opaque,
>> +                                      MEMOP_CMD_MASK,
>> +                                      a.u.set_mem_access_multi.view);
>> +        if ( rc > 0 )
>> +        {
>> +            a.u.set_mem_access_multi.opaque = rc;
>> +            if ( __copy_to_guest(arg, &a, 1) )
>> +                rc = -EFAULT;
>> +            else
>> +                rc = hypercall_create_continuation(__HYPERVISOR_hvm_op, "lh",
>> +                                                   HVMOP_altp2m, arg);
>> +        }
>> +        break;
> 
> Okay, so this is a hvmop, in which case I'm fine with the continuation
> model used.
> 
> However - is this interface supposed to be usable by a guest on itself?
> Arguably the same question would apply to some of the other sub-ops
> too, but anyway.
> 
>> --- a/xen/include/public/hvm/hvm_op.h
>> +++ b/xen/include/public/hvm/hvm_op.h
>> @@ -231,6 +231,23 @@ struct xen_hvm_altp2m_set_mem_access {
>>  typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t;
>>  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
>>  
>> +struct xen_hvm_altp2m_set_mem_access_multi {
>> +    /* view */
>> +    uint16_t view;
>> +    uint16_t pad;
>> +    /* Number of pages */
>> +    uint32_t nr;
>> +    /* Used for continuation purposes */
>> +    uint64_t opaque;
>> +    /* List of pfns to set access for */
>> +    XEN_GUEST_HANDLE(const_uint64) pfn_list;
>> +    /* Corresponding list of access settings for pfn_list */
>> +    XEN_GUEST_HANDLE(const_uint8) access_list;
> 
> I'm afraid these handles aren't going to work for a 32-bit guest. Note
> how no other hvmop uses handles.

OK, so at this point, since Ravi has not expressed a preference, but
Andrew has said that these should be accessible from the guest as well,
I assume we should move forward with this as a HVMOP, addressing the
comment above regarding compatibility. (Just to help the discussion,
here is the original patch: https://patchwork.kernel.org/patch/9612799/).

I do prefer the DOMCTL version, but of course the operation will not be
available for the guest then and we may just as well make it a HVMOP
from the beginnig.

If there are any objections, please let me know.


Thanks,
Razvan

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