|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5 5/8] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
On Mon, 2014-04-07 at 01:31 +0200, Arianna Avanzini wrote:
> This commit introduces a first attempt of implementation of the
> XEN_DOMCTL_memory_mapping hypercall for ARM. As the implementation
> would have been almost identical to the one for x86, this code has
> been made common to both the architectures. The only difference
> between the two procedures lies in the arch-specific implementation
> of the map_mmio_regions() and unmap_mmio_regions() functions.
>
> Signed-off-by: Arianna Avanzini <avanzini.arianna@xxxxxxxxx>
> Cc: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
> Cc: Paolo Valente <paolo.valente@xxxxxxxxxx>
> Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxxxxx>
> Cc: Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>
> Cc: Jan Beulich <JBeulich@xxxxxxxx>
> Cc: Keir Fraser <keir@xxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> Cc: Jan Beulich <JBeulich@xxxxxxxx>
> Cc: Keir Fraser <keir@xxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> Cc: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
> Cc: Eric Trudeau <etrudeau@xxxxxxxxxxxx>
> Cc: Viktor Kleinik <viktor.kleinik@xxxxxxxxxxxxxxx>
>
> ---
>
> v5:
> - Let the unmap_mmio_regions() function for x86 return a proper
> error code upon failure.
> - Restore correct handling of errors in the remove path of the
> hypercall, assigning to the "ret" local variable the error
> code returned by the unmap_mmio_regions() function only if
> iomem_deny_access() didn't return with an error.
> - Compute gfn_end - 1 and mfn_end - 1 only once in the DOMCTL.
> - Use a local variable to keep the return value of the function
> unmap_mmio_regions() instead of re-using the "add" variable.
> - Add a comment to make hopefully clearer how error values of
> functions are handled in the remove path of the DOMCTL.
> - Rename new header to p2m-common.h.
>
> v4:
> - Use a define for paddr_bits instead of a new variable.
> - Define prototypes for common functions map_mmio_regions() and
> unmap_mmio_regions() only once in a common header.
> - Fix type and signedness of local variables used as indexes in
> map_mmio_regions() and unmap_mmio_regions() for x86.
> - Clear p2m entries in map_mmio_regions() for x86 only if
> set_mmio_p2m_entry() returned with an error.
> - Make ranges inclusive of the end address in map_mmio_regions()
> and unmap_mmio_regions() for x86.
> - Turn hard tabs into spaces.
>
> v3:
> - Move code to xen/common/domctl.c; abstract out differences between
> the x86 and ARM code:
> . add map_mmio_regions() and unmap_mmio_regions() functions for x86;
> . add a paddr_bits variable for ARM.
> - Use pfn as parameters to the unmap_mmio_regions() function.
> - Compute gfn + nr_mfns and mfn + nr_mfns only once.
>
> v2:
> - Move code to xen/arm/domctl.c.
> - Use the already-defined PADDR_BITS constant in the new DOMCTL.
> - Use paddr_t as arguments to the map_mmio_regions() function.
> - Page-align addresses given as arguments to map_mmio_regions() and
> unmap_mmio_regions().
>
> ---
> xen/arch/arm/p2m.c | 12 ++++++++
> xen/arch/x86/domctl.c | 70 -------------------------------------------
> xen/arch/x86/mm/p2m.c | 42 ++++++++++++++++++++++++++
> xen/common/domctl.c | 71
> ++++++++++++++++++++++++++++++++++++++++++++
> xen/include/asm-arm/p2m.h | 9 ++----
> xen/include/asm-x86/p2m.h | 1 +
> xen/include/xen/p2m-common.h | 16 ++++++++++
> 7 files changed, 145 insertions(+), 76 deletions(-)
> create mode 100644 xen/include/xen/p2m-common.h
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 110b63a..cc71a5d 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -529,6 +529,18 @@ int map_mmio_regions(struct domain *d,
> MATTR_DEV, p2m_mmio_direct);
> }
>
> +int unmap_mmio_regions(struct domain *d,
> + unsigned long start_gfn,
> + unsigned long end_gfn,
> + unsigned long mfn)
> +{
> + return apply_p2m_changes(d, REMOVE,
> + pfn_to_paddr(start_gfn),
> + pfn_to_paddr(end_gfn),
> + pfn_to_paddr(mfn),
> + MATTR_DEV, p2m_mmio_direct);
The final argument here is not used in the REMOVE case. Please pass
p2m_invalid here, unless/until that changes.
> +}
> +
> int guest_physmap_add_entry(struct domain *d,
> unsigned long gpfn,
> unsigned long mfn,
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 1e51289..69a7fbf 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -639,76 +639,6 @@ long arch_do_domctl(
> }
> break;
>
> - case XEN_DOMCTL_memory_mapping:
> - {
> - unsigned long gfn = domctl->u.memory_mapping.first_gfn;
> - unsigned long mfn = domctl->u.memory_mapping.first_mfn;
> - unsigned long nr_mfns = domctl->u.memory_mapping.nr_mfns;
> - int add = domctl->u.memory_mapping.add_mapping;
> - unsigned long i;
> -
> - ret = -EINVAL;
> - if ( (mfn + nr_mfns - 1) < mfn || /* wrap? */
> - ((mfn | (mfn + nr_mfns - 1)) >> (paddr_bits - PAGE_SHIFT)) ||
> - (gfn + nr_mfns - 1) < gfn ) /* wrap? */
> - break;
> -
> - ret = -EPERM;
> - if ( !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns -
> 1) )
> - break;
> -
> - ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, add);
> - if ( ret )
> - break;
> -
> - if ( add )
> - {
> - printk(XENLOG_G_INFO
> - "memory_map:add: dom%d gfn=%lx mfn=%lx nr=%lx\n",
> - d->domain_id, gfn, mfn, nr_mfns);
> -
> - ret = iomem_permit_access(d, mfn, mfn + nr_mfns - 1);
> - if ( !ret && paging_mode_translate(d) )
> - {
> - for ( i = 0; !ret && i < nr_mfns; i++ )
> - if ( !set_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i)) )
> - ret = -EIO;
> - if ( ret )
> - {
> - printk(XENLOG_G_WARNING
> - "memory_map:fail: dom%d gfn=%lx mfn=%lx\n",
> - d->domain_id, gfn + i, mfn + i);
> - while ( i-- )
> - clear_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i));
> - if ( iomem_deny_access(d, mfn, mfn + nr_mfns - 1) &&
> - is_hardware_domain(current->domain) )
> - printk(XENLOG_ERR
> - "memory_map: failed to deny dom%d access to
> [%lx,%lx]\n",
> - d->domain_id, mfn, mfn + nr_mfns - 1);
> - }
> - }
> - }
> - else
> - {
> - printk(XENLOG_G_INFO
> - "memory_map:remove: dom%d gfn=%lx mfn=%lx nr=%lx\n",
> - d->domain_id, gfn, mfn, nr_mfns);
> -
> - if ( paging_mode_translate(d) )
> - for ( i = 0; i < nr_mfns; i++ )
> - add |= !clear_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i));
> - ret = iomem_deny_access(d, mfn, mfn + nr_mfns - 1);
> - if ( !ret && add )
> - ret = -EIO;
> - if ( ret && is_hardware_domain(current->domain) )
> - printk(XENLOG_ERR
> - "memory_map: error %ld %s dom%d access to
> [%lx,%lx]\n",
> - ret, add ? "removing" : "denying", d->domain_id,
> - mfn, mfn + nr_mfns - 1);
> - }
> - }
> - break;
> -
> case XEN_DOMCTL_ioport_mapping:
> {
> #define MAX_IOPORTS 0x10000
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 2c894b8..2c6ef24 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1631,6 +1631,48 @@ unsigned long paging_gva_to_gfn(struct vcpu *v,
> return hostmode->gva_to_gfn(v, hostp2m, va, pfec);
> }
>
> +int map_mmio_regions(struct domain *d,
> + unsigned long start_gfn,
> + unsigned long end_gfn,
> + unsigned long mfn)
Personally I would have preferred to see x86 switch to this interface
separately from moving the implementation of the user from x86 to
generic code, since doing them at the same time makes it hard to see
what actually changes in XEN_DOMCTL_memory_mapping.
> + long unmap_ret;
Stray hard tab.
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index c403534..af53816 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -29,6 +29,7 @@
>
> #include <xen/config.h>
> #include <xen/paging.h>
> +#include <xen/p2m-common.h>
More normally xen/p2m.h (which you have as p2m-common.h) would include
the asm version. But I suppose that involves a lot more code changes.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |