| Message ID | 20190618232019.26425-1-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/5] xen: add a p2mt parameter to map_mmio_regions | expand |
>>> On 19.06.19 at 01:20, <sstabellini@kernel.org> wrote: > Add a p2mt parameter to map_mmio_regions, pass p2m_mmio_direct_dev on > ARM and p2m_mmio_direct on x86 -- no changes in behavior. On x86, > introduce a macro to strip away the last parameter and rename the > existing implementation of map_mmio_regions to __map_mmio_regions. > Use __map_mmio_regions in vpci as it is x86-only today. > > On ARM, given the similarity between map_mmio_regions after the change > and map_regions_p2mt, remove un/map_regions_p2mt. Also add an ASSERT to > check that only p2m_mmio_* types are passed to it. > > Also fix the style of the comment on top of map_mmio_regions since we > are at it. > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > CC: JBeulich@suse.com > CC: andrew.cooper3@citrix.com > --- > Changes in v3: > - code style > - introduce __map_mmio_regions on x86 No. At the very least the name is badly chosen: There shouldn't be new name space violations. But ... > --- a/xen/include/asm-x86/p2m.h > +++ b/xen/include/asm-x86/p2m.h > @@ -1000,6 +1000,14 @@ static inline int p2m_entry_modify(struct p2m_domain *p2m, p2m_type_t nt, > return 0; > } > > +/* x86 doesn't use the p2mt parameter, just strip it away */ > +#define map_mmio_regions(d, start_gfn, nr, mfn, p2mt) \ > + __map_mmio_regions(d, start_gfn, nr, mfn) > +int __map_mmio_regions(struct domain *d, > + gfn_t start_gfn, > + unsigned long nr, > + mfn_t mfn); > + ... except for this perhaps not being everyone's taste, is there anything wrong with just /* x86 doesn't use the p2mt parameter, just strip it away */ #define map_mmio_regions(d, start_gfn, nr, mfn, p2mt) \ map_mmio_regions(d, start_gfn, nr, mfn) (placed ahead of the p2m-common.h inclusion point, such that the override would also affect the declaration)? The next best (imo) solution would be to utilize the fact that the function is mis-named right now anyway: There's no point for the plural in its name afaics. Hence the aliasing above could also go between map_mmio_regions() and map_mmio_region(), depending on whether you'd want to adjust the "common" name at the same time (but if you did so, then perhaps the unmap function should get renamed too). Jan
Hi Stefano, The Arm code looks good to me. One comment below. On 6/19/19 12:20 AM, Stefano Stabellini wrote: > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index e28ea1c85a..d88df11e09 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -1310,31 +1310,18 @@ static inline int p2m_remove_mapping(struct domain *d, > return rc; > } > > -int map_regions_p2mt(struct domain *d, > +int map_mmio_regions(struct domain *d, > gfn_t gfn, > unsigned long nr, > mfn_t mfn, > p2m_type_t p2mt) > { > + ASSERT( p2mt == p2m_mmio_direct_dev || > + p2mt == p2m_mmio_direct_nc || > + p2mt == p2m_mmio_direct_c ); Could you introduce p2m_is_mmio(...) in p2m.h? > return p2m_insert_mapping(d, gfn, nr, mfn, p2mt); > } Cheers,
On Wed, 19 Jun 2019, Jan Beulich wrote: > >>> On 19.06.19 at 01:20, <sstabellini@kernel.org> wrote: > > Add a p2mt parameter to map_mmio_regions, pass p2m_mmio_direct_dev on > > ARM and p2m_mmio_direct on x86 -- no changes in behavior. On x86, > > introduce a macro to strip away the last parameter and rename the > > existing implementation of map_mmio_regions to __map_mmio_regions. > > Use __map_mmio_regions in vpci as it is x86-only today. > > > > On ARM, given the similarity between map_mmio_regions after the change > > and map_regions_p2mt, remove un/map_regions_p2mt. Also add an ASSERT to > > check that only p2m_mmio_* types are passed to it. > > > > Also fix the style of the comment on top of map_mmio_regions since we > > are at it. > > > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > > CC: JBeulich@suse.com > > CC: andrew.cooper3@citrix.com > > --- > > Changes in v3: > > - code style > > - introduce __map_mmio_regions on x86 > > No. At the very least the name is badly chosen: There shouldn't be > new name space violations. But ... > > > --- a/xen/include/asm-x86/p2m.h > > +++ b/xen/include/asm-x86/p2m.h > > @@ -1000,6 +1000,14 @@ static inline int p2m_entry_modify(struct p2m_domain *p2m, p2m_type_t nt, > > return 0; > > } > > > > +/* x86 doesn't use the p2mt parameter, just strip it away */ > > +#define map_mmio_regions(d, start_gfn, nr, mfn, p2mt) \ > > + __map_mmio_regions(d, start_gfn, nr, mfn) > > +int __map_mmio_regions(struct domain *d, > > + gfn_t start_gfn, > > + unsigned long nr, > > + mfn_t mfn); > > + > > ... except for this perhaps not being everyone's taste, is there > anything wrong with just > > /* x86 doesn't use the p2mt parameter, just strip it away */ > #define map_mmio_regions(d, start_gfn, nr, mfn, p2mt) \ > map_mmio_regions(d, start_gfn, nr, mfn) > > (placed ahead of the p2m-common.h inclusion point, such that > the override would also affect the declaration)? I couldn't go with this suggestion because then usages of map_mmio_regions under arch/x86 would fail to compile unless adjusted by adding one useless argument, as the macro wouldn't apply correctly. > The next best (imo) solution would be to utilize the fact that the > function is mis-named right now anyway: There's no point for the > plural in its name afaics. Hence the aliasing above could also > go between map_mmio_regions() and map_mmio_region(), I went with this suggestion, basically I renamed __map_mmio_regions to map_mmio_region. > depending on whether you'd want to adjust the "common" name > at the same time (but if you did so, then perhaps the unmap > function should get renamed too).
On Wed, 10 Jul 2019, Julien Grall wrote: > Hi Stefano, > > The Arm code looks good to me. One comment below. Should I take it as a acked-by? > On 6/19/19 12:20 AM, Stefano Stabellini wrote: > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > > index e28ea1c85a..d88df11e09 100644 > > --- a/xen/arch/arm/p2m.c > > +++ b/xen/arch/arm/p2m.c > > @@ -1310,31 +1310,18 @@ static inline int p2m_remove_mapping(struct domain > > *d, > > return rc; > > } > > -int map_regions_p2mt(struct domain *d, > > +int map_mmio_regions(struct domain *d, > > gfn_t gfn, > > unsigned long nr, > > mfn_t mfn, > > p2m_type_t p2mt) > > { > > + ASSERT( p2mt == p2m_mmio_direct_dev || > > + p2mt == p2m_mmio_direct_nc || > > + p2mt == p2m_mmio_direct_c ); > > Could you introduce p2m_is_mmio(...) in p2m.h? Good idea, I'll do that in a prior patch and use it here. > > return p2m_insert_mapping(d, gfn, nr, mfn, p2mt); > > }
On 07/08/2019 00:38, Stefano Stabellini wrote: > On Wed, 10 Jul 2019, Julien Grall wrote: >> Hi Stefano, >> >> The Arm code looks good to me. One comment below. > > Should I take it as a acked-by? I will have a look at the next version. Cheers,
diff --git a/xen/arch/arm/acpi/domain_build.c b/xen/arch/arm/acpi/domain_build.c index 5aae32ac20..f4ac91cdac 100644 --- a/xen/arch/arm/acpi/domain_build.c +++ b/xen/arch/arm/acpi/domain_build.c @@ -193,7 +193,7 @@ static void __init acpi_map_other_tables(struct domain *d) { addr = acpi_gbl_root_table_list.tables[i].address; size = acpi_gbl_root_table_list.tables[i].length; - res = map_regions_p2mt(d, + res = map_mmio_regions(d, gaddr_to_gfn(addr), PFN_UP(size), maddr_to_mfn(addr), @@ -547,7 +547,7 @@ int __init prepare_acpi(struct domain *d, struct kernel_info *kinfo) acpi_create_efi_mmap_table(d, &kinfo->mem, tbl_add); /* Map the EFI and ACPI tables to Dom0 */ - rc = map_regions_p2mt(d, + rc = map_mmio_regions(d, gaddr_to_gfn(d->arch.efi_acpi_gpa), PFN_UP(d->arch.efi_acpi_len), virt_to_mfn(d->arch.efi_acpi_table), diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index d9836779d1..1f808b2ff1 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1171,7 +1171,7 @@ static int __init map_range_to_domain(const struct dt_device_node *dev, if ( need_mapping ) { - res = map_regions_p2mt(d, + res = map_mmio_regions(d, gaddr_to_gfn(addr), PFN_UP(len), maddr_to_mfn(addr), diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c index 256988c665..d2ef361fc7 100644 --- a/xen/arch/arm/gic-v2.c +++ b/xen/arch/arm/gic-v2.c @@ -701,7 +701,8 @@ static int gicv2_map_hwdown_extra_mappings(struct domain *d) ret = map_mmio_regions(d, gaddr_to_gfn(v2m_data->addr), PFN_UP(v2m_data->size), - maddr_to_mfn(v2m_data->addr)); + maddr_to_mfn(v2m_data->addr), + p2m_mmio_direct_dev); if ( ret ) { printk(XENLOG_ERR "GICv2: Map v2m frame to d%d failed.\n", diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index e28ea1c85a..d88df11e09 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1310,31 +1310,18 @@ static inline int p2m_remove_mapping(struct domain *d, return rc; } -int map_regions_p2mt(struct domain *d, +int map_mmio_regions(struct domain *d, gfn_t gfn, unsigned long nr, mfn_t mfn, p2m_type_t p2mt) { + ASSERT( p2mt == p2m_mmio_direct_dev || + p2mt == p2m_mmio_direct_nc || + p2mt == p2m_mmio_direct_c ); return p2m_insert_mapping(d, gfn, nr, mfn, p2mt); } -int unmap_regions_p2mt(struct domain *d, - gfn_t gfn, - unsigned long nr, - mfn_t mfn) -{ - return p2m_remove_mapping(d, gfn, nr, mfn); -} - -int map_mmio_regions(struct domain *d, - gfn_t start_gfn, - unsigned long nr, - mfn_t mfn) -{ - return p2m_insert_mapping(d, start_gfn, nr, mfn, p2m_mmio_direct_dev); -} - int unmap_mmio_regions(struct domain *d, gfn_t start_gfn, unsigned long nr, diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c index 6560507092..97cd080759 100644 --- a/xen/arch/arm/platforms/exynos5.c +++ b/xen/arch/arm/platforms/exynos5.c @@ -83,11 +83,13 @@ static int exynos5250_specific_mapping(struct domain *d) { /* Map the chip ID */ map_mmio_regions(d, gaddr_to_gfn(EXYNOS5_PA_CHIPID), 1, - maddr_to_mfn(EXYNOS5_PA_CHIPID)); + maddr_to_mfn(EXYNOS5_PA_CHIPID), + p2m_mmio_direct_dev); /* Map the PWM region */ map_mmio_regions(d, gaddr_to_gfn(EXYNOS5_PA_TIMER), 2, - maddr_to_mfn(EXYNOS5_PA_TIMER)); + maddr_to_mfn(EXYNOS5_PA_TIMER), + p2m_mmio_direct_dev); return 0; } diff --git a/xen/arch/arm/platforms/omap5.c b/xen/arch/arm/platforms/omap5.c index aee24e4d28..c5701dfd6c 100644 --- a/xen/arch/arm/platforms/omap5.c +++ b/xen/arch/arm/platforms/omap5.c @@ -99,19 +99,23 @@ static int omap5_specific_mapping(struct domain *d) { /* Map the PRM module */ map_mmio_regions(d, gaddr_to_gfn(OMAP5_PRM_BASE), 2, - maddr_to_mfn(OMAP5_PRM_BASE)); + maddr_to_mfn(OMAP5_PRM_BASE), + p2m_mmio_direct_dev); /* Map the PRM_MPU */ map_mmio_regions(d, gaddr_to_gfn(OMAP5_PRCM_MPU_BASE), 1, - maddr_to_mfn(OMAP5_PRCM_MPU_BASE)); + maddr_to_mfn(OMAP5_PRCM_MPU_BASE), + p2m_mmio_direct_dev); /* Map the Wakeup Gen */ map_mmio_regions(d, gaddr_to_gfn(OMAP5_WKUPGEN_BASE), 1, - maddr_to_mfn(OMAP5_WKUPGEN_BASE)); + maddr_to_mfn(OMAP5_WKUPGEN_BASE), + p2m_mmio_direct_dev); /* Map the on-chip SRAM */ map_mmio_regions(d, gaddr_to_gfn(OMAP5_SRAM_PA), 32, - maddr_to_mfn(OMAP5_SRAM_PA)); + maddr_to_mfn(OMAP5_SRAM_PA), + p2m_mmio_direct_dev); return 0; } diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index 3103620323..ec125cfd4f 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -1884,7 +1884,7 @@ static bool try_map_mmio(gfn_t gfn) if ( !iomem_access_permitted(d, mfn_x(mfn), mfn_x(mfn) + 1) ) return false; - return !map_regions_p2mt(d, gfn, 1, mfn, p2m_mmio_direct_c); + return !map_mmio_regions(d, gfn, 1, mfn, p2m_mmio_direct_c); } static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs, diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c index 64b141fea5..1543625ea4 100644 --- a/xen/arch/arm/vgic-v2.c +++ b/xen/arch/arm/vgic-v2.c @@ -691,7 +691,7 @@ static int vgic_v2_domain_init(struct domain *d) * region of the guest. */ ret = map_mmio_regions(d, gaddr_to_gfn(cbase), csize / PAGE_SIZE, - maddr_to_mfn(vbase)); + maddr_to_mfn(vbase), p2m_mmio_direct_dev); if ( ret ) return ret; diff --git a/xen/arch/arm/vgic/vgic-v2.c b/xen/arch/arm/vgic/vgic-v2.c index b5ba4ace87..04f34ddab5 100644 --- a/xen/arch/arm/vgic/vgic-v2.c +++ b/xen/arch/arm/vgic/vgic-v2.c @@ -309,7 +309,7 @@ int vgic_v2_map_resources(struct domain *d) * region of the guest. */ ret = map_mmio_regions(d, gaddr_to_gfn(cbase), csize / PAGE_SIZE, - maddr_to_mfn(vbase)); + maddr_to_mfn(vbase), p2m_mmio_direct_dev); if ( ret ) { gdprintk(XENLOG_ERR, "Unable to remap VGIC CPU to VCPU\n"); diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c index 8845399ae9..3a43234f69 100644 --- a/xen/arch/x86/hvm/dom0_build.c +++ b/xen/arch/x86/hvm/dom0_build.c @@ -79,7 +79,7 @@ static int __init modify_identity_mmio(struct domain *d, unsigned long pfn, for ( ; ; ) { - rc = map ? map_mmio_regions(d, _gfn(pfn), nr_pages, _mfn(pfn)) + rc = map ? __map_mmio_regions(d, _gfn(pfn), nr_pages, _mfn(pfn)) : unmap_mmio_regions(d, _gfn(pfn), nr_pages, _mfn(pfn)); if ( rc == 0 ) break; diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index 4c9954867c..e007eee42f 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -2276,10 +2276,10 @@ static unsigned int mmio_order(const struct domain *d, #define MAP_MMIO_MAX_ITER 64 /* pretty arbitrary */ -int map_mmio_regions(struct domain *d, - gfn_t start_gfn, - unsigned long nr, - mfn_t mfn) +int __map_mmio_regions(struct domain *d, + gfn_t start_gfn, + unsigned long nr, + mfn_t mfn) { int ret = 0; unsigned long i; diff --git a/xen/common/domctl.c b/xen/common/domctl.c index 72a44953d0..c6fd88d285 100644 --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -927,6 +927,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) unsigned long nr_mfns = op->u.memory_mapping.nr_mfns; unsigned long mfn_end = mfn + nr_mfns - 1; int add = op->u.memory_mapping.add_mapping; + p2m_type_t p2mt; ret = -EINVAL; if ( mfn_end < mfn || /* wrap? */ @@ -939,6 +940,10 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) /* Must break hypercall up as this could take a while. */ if ( nr_mfns > 64 ) break; + + p2mt = p2m_mmio_direct_dev; +#else + p2mt = p2m_mmio_direct; #endif ret = -EPERM; @@ -956,7 +961,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) "memory_map:add: dom%d gfn=%lx mfn=%lx nr=%lx\n", d->domain_id, gfn, mfn, nr_mfns); - ret = map_mmio_regions(d, _gfn(gfn), nr_mfns, _mfn(mfn)); + ret = map_mmio_regions(d, _gfn(gfn), nr_mfns, _mfn(mfn), p2mt); if ( ret < 0 ) printk(XENLOG_G_WARNING "memory_map:fail: dom%d gfn=%lx mfn=%lx nr=%lx ret:%ld\n", diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c index 258b91deed..ade6d19b45 100644 --- a/xen/drivers/vpci/header.c +++ b/xen/drivers/vpci/header.c @@ -52,7 +52,7 @@ static int map_range(unsigned long s, unsigned long e, void *data, * - {un}map_mmio_regions doesn't support preemption. */ - rc = map->map ? map_mmio_regions(map->d, _gfn(s), size, _mfn(s)) + rc = map->map ? __map_mmio_regions(map->d, _gfn(s), size, _mfn(s)) : unmap_mmio_regions(map->d, _gfn(s), size, _mfn(s)); if ( rc == 0 ) { diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h index 2f89bb00c3..76aadc215d 100644 --- a/xen/include/asm-arm/p2m.h +++ b/xen/include/asm-arm/p2m.h @@ -252,21 +252,6 @@ void p2m_toggle_cache(struct vcpu *v, bool was_enabled); void p2m_flush_vm(struct vcpu *v); -/* - * Map a region in the guest p2m with a specific p2m type. - * The memory attributes will be derived from the p2m type. - */ -int map_regions_p2mt(struct domain *d, - gfn_t gfn, - unsigned long nr, - mfn_t mfn, - p2m_type_t p2mt); - -int unmap_regions_p2mt(struct domain *d, - gfn_t gfn, - unsigned long nr, - mfn_t mfn); - int map_dev_mmio_region(struct domain *d, gfn_t gfn, unsigned long nr, diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h index 09ef7e02fd..8321dc1f2a 100644 --- a/xen/include/asm-x86/p2m.h +++ b/xen/include/asm-x86/p2m.h @@ -1000,6 +1000,14 @@ static inline int p2m_entry_modify(struct p2m_domain *p2m, p2m_type_t nt, return 0; } +/* x86 doesn't use the p2mt parameter, just strip it away */ +#define map_mmio_regions(d, start_gfn, nr, mfn, p2mt) \ + __map_mmio_regions(d, start_gfn, nr, mfn) +int __map_mmio_regions(struct domain *d, + gfn_t start_gfn, + unsigned long nr, + mfn_t mfn); + #endif /* _XEN_ASM_X86_P2M_H */ /* diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h index 58031a6ea8..e20b4974b0 100644 --- a/xen/include/xen/p2m-common.h +++ b/xen/include/xen/p2m-common.h @@ -8,13 +8,16 @@ int __must_check guest_physmap_remove_page(struct domain *d, gfn_t gfn, mfn_t mfn, unsigned int page_order); -/* Map MMIO regions in the p2m: start_gfn and nr describe the range in - * * the guest physical address space to map, starting from the machine - * * frame number mfn. */ +/* + * Map MMIO regions in the p2m: start_gfn and nr describe the range in + * the guest physical address space to map, starting from the machine + * frame number mfn. + */ int map_mmio_regions(struct domain *d, gfn_t start_gfn, unsigned long nr, - mfn_t mfn); + mfn_t mfn, + p2m_type_t p2mt); int unmap_mmio_regions(struct domain *d, gfn_t start_gfn, unsigned long nr,
Add a p2mt parameter to map_mmio_regions, pass p2m_mmio_direct_dev on ARM and p2m_mmio_direct on x86 -- no changes in behavior. On x86, introduce a macro to strip away the last parameter and rename the existing implementation of map_mmio_regions to __map_mmio_regions. Use __map_mmio_regions in vpci as it is x86-only today. On ARM, given the similarity between map_mmio_regions after the change and map_regions_p2mt, remove un/map_regions_p2mt. Also add an ASSERT to check that only p2m_mmio_* types are passed to it. Also fix the style of the comment on top of map_mmio_regions since we are at it. Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> CC: JBeulich@suse.com CC: andrew.cooper3@citrix.com --- Changes in v3: - code style - introduce __map_mmio_regions on x86 - fix comment style on top of map_mmio_regions - add an assert on allowed p2mt types in map_mmio_regions Changes in v2: - new patch --- xen/arch/arm/acpi/domain_build.c | 4 ++-- xen/arch/arm/domain_build.c | 2 +- xen/arch/arm/gic-v2.c | 3 ++- xen/arch/arm/p2m.c | 21 ++++----------------- xen/arch/arm/platforms/exynos5.c | 6 ++++-- xen/arch/arm/platforms/omap5.c | 12 ++++++++---- xen/arch/arm/traps.c | 2 +- xen/arch/arm/vgic-v2.c | 2 +- xen/arch/arm/vgic/vgic-v2.c | 2 +- xen/arch/x86/hvm/dom0_build.c | 2 +- xen/arch/x86/mm/p2m.c | 8 ++++---- xen/common/domctl.c | 7 ++++++- xen/drivers/vpci/header.c | 2 +- xen/include/asm-arm/p2m.h | 15 --------------- xen/include/asm-x86/p2m.h | 8 ++++++++ xen/include/xen/p2m-common.h | 11 +++++++---- 16 files changed, 51 insertions(+), 56 deletions(-)