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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.03.16 at 10:48, <zhaoshenglong@xxxxxxxxxx> 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).

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.

Jan


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