Re: [PATCH v5 21/22] xen/arm: Add a hypercall for device mmio mapping

[Shannon Zhao <zhaoshenglong@huawei.com>]

On 2016/3/16 18:04, Jan Beulich wrote:
>>>> On 16.03.16 at 10:48, <zhaoshenglong@huawei.com> wrote:
>> On 2016/3/4 18:29, Jan Beulich wrote:
>>>>> --- a/xen/arch/arm/mm.c
>>>>> +++ b/xen/arch/arm/mm.c
>>>>> @@ -1138,6 +1138,9 @@ int xenmem_add_to_physmap_one(
>>>>>          rcu_unlock_domain(od);
>>>>>          break;
>>>>>      }
>>>>> +    case XENMAPSPACE_dev_mmio:
>>>>> +        rc = map_dev_mmio_region(d, gpfn, 1, idx);
>>> This being the only caller, ...
>>>
>>>>> +int map_dev_mmio_region(struct domain *d,
>>>>> +                        unsigned long start_gfn,
>>>>> +                        unsigned long nr,
>>>>> +                        unsigned long mfn)
>>>>> +{
>>> ... what's the "nr" parameter good for? 
>> While currently the caller passes const value 1, but I think it's fine
>> to make this function support to map multiple pages for future possible use.
> 
> Well, it was just a remark. The maintainers will judge. Looking at the
> patch again I notice though that this
> 
> +    if(!iomem_access_permitted(d, start_gfn, start_gfn + nr))
> +        return 0;
> 
> is wrong (and not just malformed) - the range here is an inclusive
> one, i.e. you need to subtract one on the right side (and be sure
> nr is not zero).
> 
Ah, right! Will fix this.

> Also please note regarding
> 
> +        printk(XENLOG_ERR "Unable to map 0x%lx - 0x%lx in domain %d\n",
> 
> that at least in common and x86 code %#lx is preferred over
> 0x%lx as being a one byte shorter string literal with no meaningful
> difference to the produced output. I'd also recommend using
> mathematical range representation, to make it clear to the reader
> whether the range is inclusive or exclusive (i.e. use either
> [<low>,<high>) or [<low>,<high>]), and Dom%d or dom%d.
> 
Ok, thanks!

-- 
Shannon


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