| Message ID | 20190807002311.9906-2-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/6] xen/arm: introduce p2m_is_mmio | expand |
Hi, On 07/08/2019 01:23, Stefano Stabellini 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_region. > Use map_mmio_region in vpci as it is x86-only today. This feels quite wrong. You have a "plural" function calling a "singular" function. This is usually the other way around. This is also quite difficult for a user to understand why the 's' is been dropped/added (depending how you view it) because in both case you only deal with a single region. The confusion is added because there are no unmap_mmio_region so the code looks strange to read: > + rc = map->map ? map_mmio_region(map->d, _gfn(s), size, _mfn(s)) > : unmap_mmio_regions(map->d, _gfn(s), size, _mfn(s)); Anyway, I realized that Jan suggested it and it is x86 code. So if he is happy with it so it be. [...] > diff --git a/xen/common/domctl.c b/xen/common/domctl.c > index b48e408583..2674caa005 100644 > --- a/xen/common/domctl.c > +++ b/xen/common/domctl.c > @@ -919,6 +919,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? */ > @@ -931,6 +932,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; I think it is a pretty bad idea to have arch specific code in common code. This is only to make more difficult to add new arch (such as RISCv). Instead we should provide helper in arch code. Cheers,
On 09.08.2019 12:23, Julien Grall wrote: > On 07/08/2019 01:23, Stefano Stabellini 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_region. >> Use map_mmio_region in vpci as it is x86-only today. > > This feels quite wrong. You have a "plural" function calling a "singular" function. This is usually the other way around. This is also quite difficult for a user to understand why the 's' is been dropped/added (depending how you view it) because in both case you only deal with a single region. "Happy" is the wrong term. I'd welcome any better suggestion. I simply couldn't think of a sensible alternative, but I do want to see the leading underscores gone that were there originally. Jan
Hi, On 09/08/2019 11:37, Jan Beulich wrote: > On 09.08.2019 12:23, Julien Grall wrote: >> On 07/08/2019 01:23, Stefano Stabellini 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_region. >>> Use map_mmio_region in vpci as it is x86-only today. >> >> This feels quite wrong. You have a "plural" function calling a "singular" >> function. This is usually the other way around. This is also quite difficult >> for a user to understand why the 's' is been dropped/added (depending how you >> view it) because in both case you only deal with a single region. > > "Happy" is the wrong term. I'd welcome any better suggestion. I > simply couldn't think of a sensible alternative, but I do want > to see the leading underscores gone that were there originally. We are already modifying all the callers in this series, so doing the renaming should not make the diff worst. A few of suggestion: 1) map_mmio_region() calling map_mmio_regions() 2) arch_map_mmio_region() calling map_mmio_region() 3) map_mmio_region() calling arch_map_mmio_region() Cheers,
On 09.08.2019 12:51, Julien Grall wrote: > Hi, > > On 09/08/2019 11:37, Jan Beulich wrote: >> On 09.08.2019 12:23, Julien Grall wrote: >>> On 07/08/2019 01:23, Stefano Stabellini 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_region. >>>> Use map_mmio_region in vpci as it is x86-only today. >>> >>> This feels quite wrong. You have a "plural" function calling a "singular" function. This is usually the other way around. This is also quite difficult for a user to understand why the 's' is been dropped/added (depending how you view it) because in both case you only deal with a single region. >> >> "Happy" is the wrong term. I'd welcome any better suggestion. I >> simply couldn't think of a sensible alternative, but I do want >> to see the leading underscores gone that were there originally. > > We are already modifying all the callers in this series, so doing the renaming should not make the diff worst. > > A few of suggestion: > 1) map_mmio_region() calling map_mmio_regions() > 2) arch_map_mmio_region() calling map_mmio_region() > 3) map_mmio_region() calling arch_map_mmio_region() Ideally the top level functions (i.e. also the unmap one) would lose their plurals. Seeing that both require a per-arch implementation, I guess the best choice from the above would be 2 (with the unmap one following suit). Jan
On 07.08.2019 02:23, Stefano Stabellini 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_region. > Use map_mmio_region 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> I guess apart from the naming question (see other sub-thread) I'm fine with this. However, ... > --- > 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 | 19 ++----------------- > 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 +- ... these two and ... > xen/include/asm-arm/p2m.h | 15 --------------- > xen/include/asm-x86/p2m.h | 8 ++++++++ > xen/include/xen/p2m-common.h | 11 +++++++---- ... this one require you to widen the Cc list. This would the also allow George to become more aware of the asm-x86/p2m.h change, which strictly by ./MAINTAINERS he may not need to ack, but which is part of "X86 MEMORY MANAGEMENT" really. Jan
diff --git a/xen/arch/arm/acpi/domain_build.c b/xen/arch/arm/acpi/domain_build.c index 1b1cfabb00..09f91cc8bf 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 4c8404155a..544b0040ce 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1168,7 +1168,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..4b26bca92a 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1310,31 +1310,16 @@ 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(p2m_is_mmio(p2mt)); 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 f062ae6f6a..7209405d80 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -1879,7 +1879,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..2d3940b0fb 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_region(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 fef97c82f6..e602cd229c 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -2277,10 +2277,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_region(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 b48e408583..2674caa005 100644 --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -919,6 +919,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? */ @@ -931,6 +932,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; @@ -948,7 +953,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 3c794f486d..76b33af58e 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_region(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 31902317da..f970c53764 100644 --- a/xen/include/asm-arm/p2m.h +++ b/xen/include/asm-arm/p2m.h @@ -258,21 +258,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 aff34e3adf..a7050ee21c 100644 --- a/xen/include/asm-x86/p2m.h +++ b/xen/include/asm-x86/p2m.h @@ -1001,6 +1001,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_region(d, start_gfn, nr, mfn) +int map_mmio_region(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_region. Use map_mmio_region 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 v4: - rename __map_mmio_regions to map_mmio_region - use p2m_is_mmio 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 | 19 ++----------------- 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, 49 insertions(+), 56 deletions(-)