Re: [Xen-devel] [PATCH V2] tools/libxc, xen/x86: Added xc_set_mem_access_multi()

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 09/02/2016 01:02 PM, Jan Beulich wrote:
>>>> On 02.09.16 at 10:51, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> Changes since V1 / RFC:
>>  - Renamed xc_set_mem_access_sparse() to xc_set_mem_access_multi(),
>>    and XENMEM_access_op_set_access_sparse to
>>    XENMEM_access_op_set_access_multi.
>>  - Renamed the 'nr' parameter to 'size'.
> 
> Why?

Tamas suggested it, size sounded more intuitive to him. I have no
problem with either nr or size.

>>  - Wrapped long line in the implementation of xc_set_mem_access_multi().
>>  - Factored out common code in _p2m_set_mem_access() (and modified
>>    p2m_set_altp2m_mem_access() in the process, to take an unsigned
>>    long argument instead of a gfn_t).
>>  - Factored out xenmem_access_t to p2m_access_t conversion code in
>>    p2m_xenmem_access_to_p2m_access().
>>  - Added hypercall continuation support.
>>  - Added compat translation support.
>>  - No longer allocating buffers (now using copy_from_guest_offset()).
>>  - Added support for setting an array of access restrictions, as
>>    suggested by Tamas Lengyel.
>>  - This patch incorporates Jan Beulich's "memory: fix compat handling
>>    of XENMEM_access_op".
> 
> I'm not sure that's okay; I'd have preferred you make the other
> patch a prereq (not the least because that one may want
> backporting, while this one certainly won't). In no event should
> such a bug fix go without mentioning in the commit message.

I agree, this is here simply because looking at the state of staging,
the patch hadn't yet made it in, and I was trying to be efficient and
have an intermediate review anyway. I'll manually apply your patch
before mine and remove that code from this patch.

>> @@ -1772,13 +1773,12 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned 
>> long gla,
>>  static inline
>>  int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>>                                struct p2m_domain *ap2m, p2m_access_t a,
>> -                              gfn_t gfn)
>> +                              unsigned long gfn_l)
>>  {
> 
> I'm not really happy about such a step backwards.

I'll keep p2m_set_altp2m_mem_access() as it is then. I just thought that
going from unsigned long to gfn_t just to have to go back to unsigned
long slightly clutters up the code, but it's certainly not a huge
difference.

>> @@ -1811,21 +1811,39 @@ int p2m_set_altp2m_mem_access(struct domain *d, 
>> struct p2m_domain *hp2m,
>>                           (current->domain != d));
>>  }
>>  
>> -/*
>> - * Set access type for a region of gfns.
>> - * If gfn == INVALID_GFN, sets the default access type.
>> - */
>> -long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
>> -                        uint32_t start, uint32_t mask, xenmem_access_t 
>> access,
>> -                        unsigned int altp2m_idx)
>> +static inline
>> +int _p2m_set_mem_access(struct domain *d, struct p2m_domain *p2m,
> 
> Considering the file we're in, can't this just be set_mem_access()?

Of course, I'll rename it.

> Also I'd leave the inlining decision completely to the compiler.

I took that cue from p2m_set_altp2m_mem_access() above. I'll remove the
inline.

>> +                        struct p2m_domain *ap2m, p2m_access_t a,
>> +                        unsigned long gfn_l)
>>  {
>> -    struct p2m_domain *p2m = p2m_get_hostp2m(d), *ap2m = NULL;
>> -    p2m_access_t a, _a;
>> -    p2m_type_t t;
>> -    mfn_t mfn;
>> -    unsigned long gfn_l;
>> -    long rc = 0;
>> +    int rc;
>>  
>> +    if ( ap2m )
>> +    {
>> +        rc = p2m_set_altp2m_mem_access(d, p2m, ap2m, a, gfn_l);
>> +        /* If the corresponding mfn is invalid we will want to just skip it 
>> */
>> +        if ( rc && rc != -ESRCH )
>> +            return rc;
> 
> If you just converted -ESRCH to 0 here, you could then ...
> 
>> +    }
>> +    else
>> +    {
>> +        mfn_t mfn;
>> +        p2m_access_t _a;
>> +        p2m_type_t t;
>> +
>> +        mfn = p2m->get_entry(p2m, gfn_l, &t, &_a, 0, NULL, NULL);
>> +        rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, -1);
>> +        return rc;
> 
> ... ditch this extra return path and ...
> 
>> +    }
>> +
>> +    return 0;
> 
> ... have both cases come here.

I'll do that.

>> @@ -1851,17 +1897,8 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, 
>> uint32_t nr,
>>          ap2m = d->arch.altp2m_p2m[altp2m_idx];
>>      }
>>  
>> -    switch ( access )
>> -    {
>> -    case 0 ... ARRAY_SIZE(memaccess) - 1:
>> -        a = memaccess[access];
>> -        break;
>> -    case XENMEM_access_default:
>> -        a = p2m->default_access;
>> -        break;
>> -    default:
>> +    if ( p2m_xenmem_access_to_p2m_access(p2m, access, &a) )
>>          return -EINVAL;
> 
> Either you make the helper function return bool, or you pass on
> whatever value it returned instead of assuming it's -EINVAL.

Fair enough, I'll return bool.

>> +long p2m_set_mem_access_multi(struct domain *d,
>> +                              XEN_GUEST_HANDLE(uint64_t) pfn_list,
>> +                              XEN_GUEST_HANDLE(uint8) access_list,
>> +                              uint32_t size, uint32_t start, uint32_t mask,
>> +                              unsigned int altp2m_idx)
>> +{
>> +    struct p2m_domain *p2m = p2m_get_hostp2m(d), *ap2m = NULL;
>> +    long rc = 0;
>> +
>> +    /* altp2m view 0 is treated as the hostp2m */
>> +    if ( altp2m_idx )
>> +    {
>> +        if ( altp2m_idx >= MAX_ALTP2M ||
>> +             d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
>> +            return -EINVAL;
>> +
>> +        ap2m = d->arch.altp2m_p2m[altp2m_idx];
>> +    }
>> +
>> +    p2m_lock(p2m);
>> +    if ( ap2m )
>> +        p2m_lock(ap2m);
>> +
>> +    while ( start < size )
>> +    {
>> +        p2m_access_t a;
>> +
>> +        uint8_t access;
>> +        uint64_t gfn_l;
> 
> Stray blank line between declarations.

Will remove it.

>> @@ -320,6 +321,23 @@ int compat_memory_op(unsigned int cmd, 
>> XEN_GUEST_HANDLE_PARAM(void) compat)
>>              break;
>>          }
>>  
>> +        case XENMEM_access_op:
>> +        {
>> +#define XLAT_mem_access_op_HNDL_pfn_list(_d_, _s_) \
>> +            guest_from_compat_handle((_d_)->pfn_list, (_s_)->pfn_list)
>> +#define XLAT_mem_access_op_HNDL_access_list(_d_, _s_) \
>> +            guest_from_compat_handle((_d_)->access_list, (_s_)->access_list)
>> +
>> +            XLAT_mem_access_op(nat.mao, &cmp.mao);
>> +
>> +#undef XLAT_mem_access_op_HNDL_pfn_list
>> +#undef XLAT_mem_access_op_HNDL_access_list
>> +
>> +            return mem_access_memop(cmd,
>> +                                    guest_handle_cast(compat,
>> +                                                      xen_mem_access_op_t));
>> +        }
> 
> You translate into native format, but then cast the compat handle
> to its native counterpart type. Such casting is okay only when
> accompanied by a respectively invoked CHECK_* macro. Please
> follow the model other code here uses.

I'll follow the model.

> And reviewing this I notice that my earlier bug fix also is still not
> really correct: It causes hypercall_xlat_continuation() to be
> bypassed.

Since you've addressed this comment in a subsequent reply, no need to
continue here.

>> @@ -452,6 +454,16 @@ struct xen_mem_access_op {
>>       * ~0ull is used to set and get the default access for pages
>>       */
>>      uint64_aligned_t pfn;
>> +    /*
>> +     * List of pfns to set access for
>> +     * Used only with XENMEM_access_op_set_access_multi
>> +     */
>> +    XEN_GUEST_HANDLE(uint64_t) pfn_list;
> 
> I'm not sure why we even have this kind of handle - please use
> XEN_GUEST_HANDLE(uint64) instead.

I'll change it.

>> +    /*
>> +     * Corresponding list of access settings for pfn_list
>> +     * Used only with XENMEM_access_op_set_access_multi
>> +     */
>> +    XEN_GUEST_HANDLE(uint8) access_list;
> 
> And for both of them - I don't think the arrays are meant to be
> used for output? In which case they should be handles of const
> types.

No, they're indeed not meant for output. I'll constify the code.


Thanks,
Razvan

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