Re: [Xen-devel] [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 08/08/18 11:19, Paul Durrant wrote:
>
>>> +static inline unsigned int status_to_grant_frames(unsigned int
>> status_frames)
>>> +{
>>> +    return DIV_ROUND_UP(status_frames * GRANT_STATUS_PER_PAGE,
>> GRANT_PER_PAGE);
>>> +}
>>> +
>>>  /* Check if the page has been paged out, or needs unsharing.
>>>     If rc == GNTST_okay, *page contains the page struct with a ref taken.
>>>     Caller must do put_page(*page).
>>> @@ -3860,6 +3866,47 @@ int mem_sharing_gref_to_gfn(struct
>> grant_table *gt, grant_ref_t ref,
>>>  }
>>>  #endif
>>>
>>> +/* caller must hold write lock */
>>> +static int gnttab_get_status_frame_mfn(struct domain *d,
>>> +                                       unsigned long idx, mfn_t *mfn)
>>> +{
>>> +    struct grant_table *gt = d->grant_table;
>>> +
>>> +    ASSERT(gt->gt_version == 2);
>>> +
>>> +    if ( idx >= nr_status_frames(gt) )
>>> +    {
>>> +        unsigned long nr = status_to_grant_frames(idx + 1);
>> Why the +1 ? Won't that cause a failure if you attempt to map the
>> maximum valid index?
> That's kind of the idea...

To force a failure when mapping a legitimate index?  Whatever the old
code did, this smells like broken boundary case.

A caller is going to want to map frames 0 to N-1, based on some
knowledge of either how many frames the guest has, or what the total
number of expected frames is.  nr_status_frames() OTOH, is 1-based,
which is why this looks wrong.

How about a comment along the lines of

/* idx is 0-based, nr_* is 1-based. */

which might help reduce the confusion?

>>> +
>>> +    if ( idx >= nr_grant_frames(gt) )
>>> +        return -EINVAL;
>>> +
>>> +    *mfn = _mfn(virt_to_mfn(gt->shared_raw[idx]));
>>> +    return 0;
>>> +}
>>> +
>>>  int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
>>>                       mfn_t *mfn)
>>>  {
>>> diff --git a/xen/common/memory.c b/xen/common/memory.c
>>> index e29d596727..427f65a5e1 100644
>>> --- a/xen/common/memory.c
>>> +++ b/xen/common/memory.c
>>> @@ -23,6 +23,7 @@
>>>  #include <xen/numa.h>
>>>  #include <xen/mem_access.h>
>>>  #include <xen/trace.h>
>>> +#include <xen/grant_table.h>
>>>  #include <asm/current.h>
>>>  #include <asm/hardirq.h>
>>>  #include <asm/p2m.h>
>>> @@ -982,6 +983,43 @@ static long xatp_permission_check(struct domain
>> *d, unsigned int space)
>>>      return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
>>>  }
>>>
>>> +static int acquire_grant_table(struct domain *d, unsigned int id,
>>> +                               unsigned long frame,
>>> +                               unsigned int nr_frames,
>>> +                               xen_pfn_t mfn_list[])
>>> +{
>>> +    unsigned int i = nr_frames;
>>> +
>>> +    /* Iterate backwards in case table needs to grow */
>>> +    while ( i-- != 0 )
>>> +    {
>>> +        mfn_t mfn = INVALID_MFN;
>>> +        int rc;
>>> +
>>> +        switch ( id )
>>> +        {
>>> +        case XENMEM_resource_grant_table_id_shared:
>>> +            rc = gnttab_get_shared_frame(d, frame + i, &mfn);
>>> +            break;
>>> +
>>> +        case XENMEM_resource_grant_table_id_status:
>>> +            rc = gnttab_get_status_frame(d, frame + i, &mfn);
>>> +            break;
>>> +
>>> +        default:
>>> +            rc = -EINVAL;
>>> +            break;
>>> +        }
>>> +
>>> +        if ( rc )
>> Would it perhaps be prudent to have || mfn_eq(mfn, INVALID_MFN)
>> here?  I
>> don't think anything good will come of handing INVALID_MFN back to the
>> caller.
> Why? The value of mfn will be overwritten if rc == 0 and will be left as 
> INVALID_MFN if rc != 0. I can ASSERT that if you'd like.

Yeah - an ASSERT() would be nice.

>>> +            !is_hardware_domain(currd) )
>>> +            return -EOPNOTSUPP;
>> EPERM perhaps?
>>
> I debated that. I'm really not sure which one would be best.

Hmm, nor me.  Lets leave it like that for now.

~Andrew

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