Re: [Xen-devel] [PATCH 1/4] move XENMEM_add_to_physmap handling framework to common code

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.12.13 at 18:52, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> On Wed, 2013-12-18 at 14:34 +0000, Jan Beulich wrote:
>> There's really nothing really architecture specific here; the
>> architecture specific handling is limited to
>> xenmem_add_to_physmap_one().
>> 
>> Note that ARM's restriction of XENMAPSPACE_gmfn_foreign only being
>> supported through XENMEM_add_to_physmap_range is being dropped as being
>> pointless.
> 
> It's not pointless, dropping this restriction turns a comprehensible
> EINVAL result into an incorrect ESRCH result. Which is incorrect because
> no domain was specified so how can it not exist.

How can there have been no domain specified for a
XENMAPSPACE_gmfn_foreign? If the field was left uninitialized,
-ESRCH would be as valid as -EINVAL, and I don't think we
specify for any hypercall that between multiple applicable
error code a specific one would be preferred over others.

> I'd also rather avoid introducing unnecessary/legacy hypercalls to ARM
> if possible.

I can understand that, but please also take the perspective that
moving the architecture independent bits out should have been
done much earlier, and (see below) adding a conditional here
doesn't seem too attractive to me.

>> +static int xenmem_add_to_physmap(struct domain *d,
>> +                                 struct xen_add_to_physmap *xatp)
>> +{
>> +    struct xen_add_to_physmap start_xatp;
>> +    int rc = 0;
>> +
>> +    if ( xatp->space != XENMAPSPACE_gmfn_range )
>> +        return xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID,
>> +                                         xatp->idx, xatp->gpfn);
>> +
>> +#ifdef HAS_PASSTHROUGH
>> +    if ( need_iommu(d) )
>> +        this_cpu(iommu_dont_flush_iotlb) = 1;
>> +#endif
>> +
>> +    start_xatp = *xatp;
>> +    while ( xatp->size > 0 )
>> +    {
>> +        rc = xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID,
>> +                                       xatp->idx, xatp->gpfn);
> 
> ARM's xenmem_add_to_physmap_one does not support XENMAPSPACE_gmfn_range,
> so it will return ENOSYS and fail out. I suppose that is ok. But it
> makes the bulk of this function pointless. 
> 
> How about pulling the != XENMAPSPACE_gmfn_range check into do_memory_op
> either calling xenmem_add_to_phymap_one or a per arch gmfn_range
> function?

No, I think having just one arch specific function dealing with
everything, and having the common code really take care of
as much as is common is the right thing (and considering the
review by Tim and the ack from Keir, I think others agree).

Which would leave the option of adding a #ifdef CONFIG_ARM
(or #ifndef CONFIG_X86) in the common code - not really nice.

>> +        if ( rc < 0 )
>> +            break;
>> +
>> +        xatp->idx++;
>> +        xatp->gpfn++;
>> +        xatp->size--;
>> +
>> +        /* Check for continuation if it's not the last interation. */
> 
> That's the second or third time this "interation" typo has got
> copied ;-)

Fixed - I didn't look that closely at the comments I was moving
around. Albeit I had noticed and fixed the missing stops...

Jan


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