| Message ID | 1457072152-16128-22-git-send-email-zhaoshenglong@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 04.03.16 at 07:15, <zhaoshenglong@huawei.com> wrote: > From: Shannon Zhao <shannon.zhao@linaro.org> > > It needs to map platform or amba device mmio to Dom0 on ARM. But when > booting with ACPI, it can't get the mmio region in Xen due to lack of > AML interpreter to parse DSDT table. Therefore, let Dom0 call a > hypercall to map mmio region when it adds the devices. > > Here we add a new map space like the XEN_DOMCTL_memory_mapping to map > mmio region for Dom0. > > Cc: Jan Beulich <jbeulich@suse.com> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> Same remark regarding the Cc list. > --- 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? Or alternatively - wouldn't you want to make it possible to have larger areas mapped by large pages? > + int res; > + > + if(!iomem_access_permitted(d, start_gfn, start_gfn + nr)) > + return 0; This would seem to belong into common code Also - coding style. > + res = map_mmio_regions(d, start_gfn, nr, mfn); > + if ( res < 0 ) > + { > + printk(XENLOG_ERR "Unable to map 0x%lx - 0x%lx in domain %d\n", %#lx > + start_gfn << PAGE_SHIFT, (start_gfn + nr) << PAGE_SHIFT, I see no reason for the shifts. > --- a/xen/common/memory.c > +++ b/xen/common/memory.c > @@ -980,6 +980,14 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > if ( d == NULL ) > return -ESRCH; > > + /* > + * XENMAPSPACE_dev_mmio mapping is only supported for hardware Domain > + * to map this kind of space to itself. > + */ > + if ( (xatp.space == XENMAPSPACE_dev_mmio) && > + (!is_hardware_domain(current->domain) || (d != current->domain)) ) Readability would benefit if you used "d" twice and "current->domain" just once, preferable after swapping the two sides of the ||. Overall I wonder whether this wouldn't help PVH on x86 too, where we currently do some hackery to (not even completely) map MMIO into Dom0's p2m. In such a case perhaps map_dev_mmio_regions() should become a general per-arch function right away (declared in a common header and stubbed out in x86 code for now). Boris, Roger? Jan
On Fri, 4 Mar 2016, Jan Beulich wrote: > >>> On 04.03.16 at 07:15, <zhaoshenglong@huawei.com> wrote: > Overall I wonder whether this wouldn't help PVH on x86 too, > where we currently do some hackery to (not even completely) > map MMIO into Dom0's p2m. In such a case perhaps > map_dev_mmio_regions() should become a general per-arch > function right away (declared in a common header and stubbed > out in x86 code for now). Boris, Roger? I though that we agreed that Xen will take care of doing all the MMIO mappings for HVMlite/PVHv2 guests: http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg01609.html My first idea was to use the MMIO device mapping hypercall (just like ARM) on the hardware domain, but I think we can get away without it on x86. Roger.
>>> On 04.03.16 at 12:00, <roger.pau@citrix.com> wrote: > On Fri, 4 Mar 2016, Jan Beulich wrote: >> >>> On 04.03.16 at 07:15, <zhaoshenglong@huawei.com> wrote: >> Overall I wonder whether this wouldn't help PVH on x86 too, >> where we currently do some hackery to (not even completely) >> map MMIO into Dom0's p2m. In such a case perhaps >> map_dev_mmio_regions() should become a general per-arch >> function right away (declared in a common header and stubbed >> out in x86 code for now). Boris, Roger? > > I though that we agreed that Xen will take care of doing all the MMIO > mappings for HVMlite/PVHv2 guests: > > http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg01609.html > > My first idea was to use the MMIO device mapping hypercall (just like > ARM) on the hardware domain, but I think we can get away without it on x86. Oh, you're right (albeit that doesn't cover MMIO regions not represented by BARs, which will need taking care of). Question back to the ARM folks then: Would such a model work for you too? (I'd really like to avoid having two different models, if we can avoid it.) Jan
On Fri, 4 Mar 2016, Jan Beulich wrote: > >>> On 04.03.16 at 12:00, <roger.pau@citrix.com> wrote: > > On Fri, 4 Mar 2016, Jan Beulich wrote: > >> >>> On 04.03.16 at 07:15, <zhaoshenglong@huawei.com> wrote: > >> Overall I wonder whether this wouldn't help PVH on x86 too, > >> where we currently do some hackery to (not even completely) > >> map MMIO into Dom0's p2m. In such a case perhaps > >> map_dev_mmio_regions() should become a general per-arch > >> function right away (declared in a common header and stubbed > >> out in x86 code for now). Boris, Roger? > > > > I though that we agreed that Xen will take care of doing all the MMIO > > mappings for HVMlite/PVHv2 guests: > > > > http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg01609.html > > > > My first idea was to use the MMIO device mapping hypercall (just like > > ARM) on the hardware domain, but I think we can get away without it on x86. > > Oh, you're right (albeit that doesn't cover MMIO regions not > represented by BARs, which will need taking care of). This. On ARM there can MMIO regions of non-PCI devices which need to be mapped and can only be discovered via DSDT. > Question > back to the ARM folks then: Would such a model work for you > too? (I'd really like to avoid having two different models, if we > can avoid it.) It doesn't cover non-PCI devices. But once you extend your model to deal with non-PCI devices and MMIO regions not represented by BARs, then potentually yes.
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. Thanks,
>>> 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). 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
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!
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index 81f9e2e..0aae6c5 100644 --- 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); + return rc; default: return -ENOSYS; diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index d206616..7264ed2 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -7,6 +7,7 @@ #include <xen/bitops.h> #include <xen/vm_event.h> #include <xen/mem_access.h> +#include <xen/iocap.h> #include <public/vm_event.h> #include <asm/flushtlb.h> #include <asm/gic.h> @@ -1270,6 +1271,28 @@ int unmap_mmio_regions(struct domain *d, d->arch.p2m.default_access); } +int map_dev_mmio_region(struct domain *d, + unsigned long start_gfn, + unsigned long nr, + unsigned long mfn) +{ + int res; + + if(!iomem_access_permitted(d, start_gfn, start_gfn + nr)) + return 0; + + res = map_mmio_regions(d, start_gfn, nr, mfn); + if ( res < 0 ) + { + printk(XENLOG_ERR "Unable to map 0x%lx - 0x%lx in domain %d\n", + start_gfn << PAGE_SHIFT, (start_gfn + nr) << PAGE_SHIFT, + d->domain_id); + return res; + } + + return 0; +} + int guest_physmap_add_entry(struct domain *d, unsigned long gpfn, unsigned long mfn, diff --git a/xen/common/memory.c b/xen/common/memory.c index ef57219..98db1cb 100644 --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -980,6 +980,14 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) if ( d == NULL ) return -ESRCH; + /* + * XENMAPSPACE_dev_mmio mapping is only supported for hardware Domain + * to map this kind of space to itself. + */ + if ( (xatp.space == XENMAPSPACE_dev_mmio) && + (!is_hardware_domain(current->domain) || (d != current->domain)) ) + return -EACCES; + rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); if ( rc ) { @@ -1024,6 +1032,14 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) if ( d == NULL ) return -ESRCH; + /* + * XENMAPSPACE_dev_mmio mapping is only supported for hardware Domain + * to map this kind of space to itself. + */ + if ( (xatpb.space == XENMAPSPACE_dev_mmio) && + (!is_hardware_domain(current->domain) || (d != current->domain)) ) + return -EACCES; + rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d); if ( rc ) { diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h index 17be6ad..5fc7ff3 100644 --- a/xen/include/asm-arm/p2m.h +++ b/xen/include/asm-arm/p2m.h @@ -154,6 +154,11 @@ int unmap_regions_rw(struct domain *d, unsigned long nr_mfns, unsigned long mfn); +int map_dev_mmio_region(struct domain *d, + unsigned long start_gfn, + unsigned long nr, + unsigned long mfn); + int guest_physmap_add_entry(struct domain *d, unsigned long gfn, unsigned long mfn, diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h index f69e92f..fe52ee1 100644 --- a/xen/include/public/memory.h +++ b/xen/include/public/memory.h @@ -220,6 +220,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_machphys_mapping_t); #define XENMAPSPACE_gmfn_range 3 /* GMFN range, XENMEM_add_to_physmap only. */ #define XENMAPSPACE_gmfn_foreign 4 /* GMFN from another dom, * XENMEM_add_to_physmap_batch only. */ +#define XENMAPSPACE_dev_mmio 5 /* device mmio region */ /* ` } */ /*