Re: [PATCH v8 00/14] Implement the XEN_DOMCTL_memory_mapping hypercall for ARM

[Julien Grall <julien.grall@linaro.org>]

Hi Arianna,

Do you have any plan to send a new version soon? I'm working on device
pass-through and this series is a dependency/blocker for my series.

IIRC, this version was already in good shape. There is only few comments.

Regards,

On 05/25/2014 11:51 AM, Arianna Avanzini wrote:
> Hello,
> 
> this is a new version - the eighth - of my implementation proposal for the
> memory_mapping DOMCTL for the ARM architecture. As usual, this cover letter just
> lists the most relevant changes performed on the patchset, while a more detailed
> description can be found in the description and changelog of each patch; also,
> some more information about the patch series' origin can be found in the last
> full cover letter ([1]).
> 
> First of all, the commit descriptions have been reworded to get rid of tentative
> phrases that were pointless in that context, as suggested by Dario Faggioli.
> 
> Patch 0002 has been modified to revert some uncorrect changes due to my
> misunderstanding of previous comments, as pointed out by Julien Grall. Also,
> according to his suggestions, an error message has been added to let a failure
> encountered during an unmap be less obscure. The error path has been improved
> to include unmapping previously-created page-table mappings, unlocking the
> p2m_lock and flushing the TLBs in case some mappings have been successfully
> removed.
> 
> Patch 0004 has been changed to address issues pointed out by Julien Grall and
> Ian Campbell. Now the correct DIV_ROUND_UP() macro is used instead of the
> ternary operator, and the map_one_mmio() function, defined for the xgene-storm
> platform, accepts pfns as arguments instead of paddr_t. Also, the function
> apply_p2m_changes() is now called from map_mmio_regions() with the correct
> count.
> 
> A new patch 0005 has been added to let the map_mmio_regions() function for ARM
> unmap partially-mapped I/O-memory regions if mapping a page fails, as suggested
> by Julien Grall. The approach which has been used includes having the function
> apply_p2m_changes() return, in the case of an error, the number of pages that
> have been successfully mapped, therefore allowing the caller to undo what has
> been done, if necessary. The definition of unmap_mmio_regions() for ARM has been
> moved to this patch and now uses the correct count as indicated by Julien Grall.
> 
> The title and description of patch 0006 have been changed to (hopefully) better
> fit the patch content, as requested by Jan Beulich.
> 
> The patch factoring out map and unmap operations from the x86 version of the
> memory_mapping DOMCTL has been split into two, as suggested by Jan Beulich, to
> make the review easier: patch 0007 now just cleans up existing code, by letting
> mfn + nr_mfns be computed only once and by changing the name of the tmp_rc local
> variable to rc; patch 0009 instead actually factors out the operations.
> 
> A new patch 0008 now adds an empty stub for the ARM architecture of the MTRR-
> related function memory_type_changed(), currently available only for x86, and
> makes its definition common to both x86 and ARM. This was requested by Jan
> Beulich and Ian Campbell.
> 
> The call to PERROR() in patch 0012 has been fixed, as it showed an error message
> whose meaning was the opposite of the correct one, as pointed out by Jan
> Beulich.
> 
> The patch which implemented a separation of the functions of the memory_mapping
> and iomem_permission DOMCTLs has now been split into two, as suggested by Jan
> Beulich: patch 0013 now contains only libxl-related preparatory changes, while
> patch 0014 actually implements the separation. As for patch 0014, changes
> performed on the iomem_permission DOMCTL handling code have been dropped, while
> the permission checks performed by memory_mapping have been changed to be more
> conservative and check both the calling domain and the grantee domain's
> permissions.
> As of now, patch 0014 performs the separation only for I/O memory and not for
> I/O ports (as was instead suggested by Jan Beulich) because, as far as I know,
> Julien Grall is working on the I/O ports patch series, and I'm afraid I might be
> conflicting with his work. I am however surely willing to add a patch for that
> if he is OK with it.
> 
> The code has again been tested on a cubieboard2, with Linux v3.15-rc3 as a dom0
> and ERIKA Enterprise ([2]) as a domU. The x86 bits have been tested on an x86_64
> machine, as I forgot to say in the last versions of the patchset and Jan Beulich
> pointed out; in particular, this last version, has been tested with Linux 
> 3.15.0-rc5 as both dom0 and domU.
> Any feedback about this new version of the patchset is more than welcome,
> Arianna
> 
> [1] http://lists.xen.org/archives/html/xen-devel/2014-03/msg01724.html
> [2] http://erika.tuxfamily.org/drupal/
> 
> Arianna Avanzini (14):
>   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/arm: let map_mmio_regions() use start and count
>   arch/arm: unmap partially-mapped I/O-memory regions
>   arch/x86: warn if to-be-removed mapping does not exist
>   arch/x86: cleanup memory_mapping DOMCTL
>   xen/common: move memory_type_changed() function to common code
>   xen/x86: factor out map and unmap from the memory_mapping DOMCTL
>   xen/common: move the memory_mapping DOMCTL hypercall to common code
>   tools/libxl: parse optional start gfn from the iomem config option
>   tools/libxl: handle the iomem parameter with the memory_mapping hcall
>   tools/libxl: explicitly grant access to needed I/O-memory ranges
>   xen/common: do not implicitly permit access to mapped I/O memory
> 
>  docs/man/xl.cfg.pod.5                | 18 +++++--
>  tools/libxc/xc_domain.c              | 12 +++++
>  tools/libxl/libxl.h                  | 10 ++++
>  tools/libxl/libxl_create.c           | 34 +++++++++++++
>  tools/libxl/libxl_internal.h         |  1 +
>  tools/libxl/libxl_pci.c              | 26 ++++------
>  tools/libxl/libxl_types.idl          |  4 ++
>  tools/libxl/xl_cmdimpl.c             | 17 ++++---
>  xen/arch/arm/Makefile                |  1 +
>  xen/arch/arm/domain_build.c          | 18 +++++--
>  xen/arch/arm/gic.c                   | 15 +++---
>  xen/arch/arm/mtrr.c                  |  6 +++
>  xen/arch/arm/p2m.c                   | 99 ++++++++++++++++++++++++++++--------
>  xen/arch/arm/platforms/exynos5.c     |  9 ++--
>  xen/arch/arm/platforms/omap5.c       | 17 +++----
>  xen/arch/arm/platforms/xgene-storm.c | 16 +++---
>  xen/arch/x86/domctl.c                | 76 ---------------------------
>  xen/arch/x86/mm/p2m.c                | 53 +++++++++++++++++--
>  xen/common/domctl.c                  | 55 +++++++++++++++++++-
>  xen/include/asm-arm/mm.h             |  2 +
>  xen/include/asm-arm/mtrr.h           |  6 +++
>  xen/include/asm-arm/p2m.h            | 11 ++--
>  xen/include/asm-x86/mtrr.h           |  2 +-
>  xen/include/asm-x86/p2m.h            |  3 +-
>  xen/include/xen/mtrr.h               |  8 +++
>  xen/include/xen/p2m-common.h         | 16 ++++++
>  26 files changed, 365 insertions(+), 170 deletions(-)
>  create mode 100644 xen/arch/arm/mtrr.c
>  create mode 100644 xen/include/asm-arm/mtrr.h
>  create mode 100644 xen/include/xen/mtrr.h
>  create mode 100644 xen/include/xen/p2m-common.h
> 


-- 
Julien Grall