[PATCH v5 0/8] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM

* [PATCH v5 0/8] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM
@ 2014-04-06 23:31 Arianna Avanzini
  2014-04-06 23:31 ` [PATCH v5 1/8] arch, arm: domain build: let dom0 access I/O memory of mapped devices Arianna Avanzini
                   ` (7 more replies)
  0 siblings, 8 replies; 19+ messages in thread
From: Arianna Avanzini @ 2014-04-06 23:31 UTC (permalink / raw)
  To: xen-devel
  Cc: Ian.Campbell, paolo.valente, keir, stefano.stabellini,
	Ian.Jackson, dario.faggioli, tim, julien.grall, etrudeau,
	JBeulich, avanzini.arianna, viktor.kleinik

Hello,

here is my fifth attempt at proposing an implementation of the memory_mapping
DOMCTL for the ARM architecture. The following patchset adds both the proposed
implementation of the hypercall and some related changes made to existing code
after having the benefit of many comments to the previous proposal attempts
([1]). As of those previous discussions, I'm not summarizing them in the below
description to prevent this cover letter from becoming much longer than
appropriate, but more details should be available, if needed, in the cover
letter of the third and fourth patchsets ([2]).

I am aware of the fact that there is still on-going discussion about the
previous version of this patchset ([3]), but, as also suggested by Stefano
Stabellini, I thought I'd send this new version to try to get rid of the most
immediate issues that have been highlighted.

The first commit in this patchset implements the bookkeeping operations deemed
necessary to make a domain's iomem_caps coherent with the I/O-memory mapping
operations performed during domain build. As of now, the change is dom0-related,
even if the function affected by the change if generic.

The second commit adds to the apply_p2m_changes() function some consistency
checks useful for the REMOVE operation. More in detail, the REMOVE switch of
the function now checks:
. the validity of the mapping: if the mapping is found not to be valid, the
  page is skipped;
. the fact that the gfn whose mapping is to be removed is actually mapped to
  the machine address given as parameter.
With respect to v4, this commit uses the "maddr" parameter of the function to
keep the machine address used for the above-described comparison, incrementing
it where necessary (also when first and second level mappings are not valid), as
suggested by Julien Grall and Ian Campbell. It also gets the actual mfn mapped
to the gfn given as parameter to the function in the REMOVE case of the switch
construct, as in other cases it might be invalid and its value might be
uncorrectly used, as pointed out by Julien Grall. It also removes a harmful
ASSERT on the validity of the mapping, which was introduced by the v4 patchset
in the REMOVE case; the v5 commit turns it to an if(), as suggested by Julien
Grall.

The third commit changes the interface of the existing map_mmio_regions()
function for ARM. As of now, the function takes parameters of paddr_t type; this
interface needs caller functions to correctly page-align addresses. This commit
lets this function take page frame numbers as parameters instead, trying also to
modify callers to use the new interface. With respect to v4, the v5 commit adds
a macro for the paddr_to_pfn(PAGE_ALIGN(...)) pattern which was repeatedly used
throughout the patch, as pointed out by Julien Grall.
This commit still attempts to produce the minimum necessary changes; as a
consequence, it still does not modify also the interface of apply_p2m_changes(),
which instead was suggested by Stefano Stabellini.

The fourth commit adds to clear_mmio_p2m_entry() a check on the actual existence
of a mapping between the guest frame number and a machine frame number given as
parameter; the check is performed before removing such a mapping. This change
has been implemented after having been discussed with Jan Beulich, and should be
the equivalent of that added by commit 0002 of this patchset to the ARM-specific
code removing a p2m mapping.

The fifth commit moves the existing memory_mapping DOMCTL for x86 to
xen/common/domctl.c and attempts to adapt the code to be used for both the x86
and ARM architectures. More in detail, the arch-specific operations necessary to
map and unmap memory ranges are abstracted out in a pair of map_mmio_regions()
and unmap_mmio_regions() functions.
With respect to the v4 patchset, this commit attempts to address the following
issues.
. The unmap_mmio_regions() function for the x86 architecture now returns a
  proper error code, instead of relying on a wrongly-used true/false return
  value, as suggested by Jan Beulich.
. It restores the correct handling of return values from the iomem_deny_access()
  and unmap_mmio_regions() in the remove path of the hypercall, while the v4
  version uncorrectly let an error code returned by unmap_mmio_regions() be
  ignored, as pointed out by Jan Beulich and discussed with Stefano Stabellini.
. It adds to the new DOMCTL a comment which attempts to explain how error codes
  from the above functions are handled.
. It lets the DOMCTL compute gfn_end - 1 and mfn_end - 1 only once, as suggested
  by Jan Beulich.
. It uses a new local variable to keep the return value instead of re-using the
  "add" local variable, which was confusing as pointed out by Ian Campbell.
. It renames the newly-added header, common to the x86 and ARM architectures,
  from p2m.h to p2m-common.h, as suggested by Jan Beulich.  

The sixth commit adds to libxl the handling code for a new optional parameter to
the "iomem" configuration option. The new parameter, called "gfn", allows to
specify the start guest address used for the mapping of the given machine
address range; in case no start guest address is specified, the patch lets libxl
default its value to the start machine address.
With respect to the v4 corresponding one, this commit tries to address the
following issues, pointed out by Julien Grall and Ian Campbell.
. It uses -1 as a default value for the optional "gfn" parameter.
. It lets the default value of "gfn" be initialized with a proper call to the
  libxl_iomem_range_init() function.
. It defers to libxl the assignment of the "gfn" parameter to the start machine
  address, if "gfn" was initialized to its default value.
. It uses a local variable to keep the return value of the sscanf() function
  as to makes the code more readable.

The seventh commit adds two new architecture helpers to libxl: these helpers
should allow the caller to establish if a domain uses an auto-translated
physmap. This is instrumental to commit 0008, which needs to call the newly-
added DOMCTL only for auto-translated guests.

The eighth commit adds to libxl code to handle the iomem configuration option by
invoking the memory_mapping DOMCTL. The hypercall is invoked only if the domain
is auto-translated, by using the arch-specific helpers introduced with the
previous commit.

The code has been tested on a cubieboard2, with Linux v3.13 as a dom0 and ERIKA
Enterprise ([4]) as a domU.
As usual, any feedback about this new version is more than welcome.
Arianna

[1] http://markmail.org/message/d5vhhiin5tr2yee7
    http://markmail.org/message/dtzsvauzob332gnq
    http://markmail.org/message/fy7ioiv37gbp22an
    http://markmail.org/message/qca2nxrpe2dfwdks
[2] http://lists.xen.org/archives/html/xen-devel/2014-03/msg01724.html
    http://lists.xen.org/archives/html/xen-devel/2014-03/msg03138.html
[3] http://lists.xen.org/archives/html/xen-devel/2014-04/msg00118.html
    http://lists.xen.org/archives/html/xen-devel/2014-04/msg00123.html
[4] http://erika.tuxfamily.org/drupal/

Arianna Avanzini (8):
  arch, arm: domain build: let dom0 access I/O memory of mapped devices
  arch, arm: add consistency check to REMOVE p2m changes
  arch, arm: let map_mmio_regions() take pfn as parameters
  arch, x86: check if mapping exists before memory_mapping removes it
  xen, common: add the XEN_DOMCTL_memory_mapping hypercall
  tools, libxl: parse optional start gfn from the iomem config option
  tools, libxl: add helpers to establish if guest is auto-translated
  tools, libxl: handle the iomem parameter with the memory_mapping hcall

 docs/man/xl.cfg.pod.5                | 13 +++++--
 tools/libxl/libxl.h                  | 10 +++++
 tools/libxl/libxl_arch.h             |  2 +
 tools/libxl/libxl_arm.c              |  5 +++
 tools/libxl/libxl_create.c           | 19 ++++++++++
 tools/libxl/libxl_types.idl          |  7 +++-
 tools/libxl/libxl_x86.c              |  6 +++
 tools/libxl/xl_cmdimpl.c             | 19 +++++-----
 xen/arch/arm/domain_build.c          | 18 +++++++--
 xen/arch/arm/gic.c                   | 21 ++++++-----
 xen/arch/arm/p2m.c                   | 52 ++++++++++++++++++++++----
 xen/arch/arm/platforms/exynos5.c     | 12 +++---
 xen/arch/arm/platforms/omap5.c       | 21 ++++++-----
 xen/arch/arm/platforms/xgene-storm.c |  4 +-
 xen/arch/x86/domctl.c                | 70 -----------------------------------
 xen/arch/x86/mm/p2m.c                | 65 ++++++++++++++++++++++++++++++---
 xen/common/domctl.c                  | 71 ++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/mm.h             |  2 +
 xen/include/asm-arm/p2m.h            |  8 ++--
 xen/include/asm-x86/p2m.h            |  3 +-
 xen/include/xen/p2m-common.h         | 16 ++++++++
 21 files changed, 313 insertions(+), 131 deletions(-)
 create mode 100644 xen/include/xen/p2m-common.h

-- 
1.9.1

^ permalink raw reply	[flat|nested] 19+ messages in thread