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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
