Re: [Xen-devel] [PATCH v3 4/5] libxl/xl: add memory policy option to iomem

[Julien Grall <julien.grall@arm.com>]

Hi Stefano,

On 6/19/19 12:20 AM, Stefano Stabellini wrote:
> Add a new memory policy option for the iomem parameter.
> Possible values are:
> - arm_dev_nGnRE, Device-nGnRE, the default on ARM

s/ARM/Arm/

> - arm_mem_WB, WB cachable memory
> - x86_UC_minus: uncachable memory, the default on x86
> 
> Store the parameter in a new field in libxl_iomem_range.
> 
> Pass the memory policy option to xc_domain_mem_map_policy.
> 
> Do the libxl to libxc value conversion in per-arch functions so that we
> can return error for x86 parameters on Arm architectures and vice versa.
> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> CC: ian.jackson@eu.citrix.com
> CC: wei.liu2@citrix.com
> ---
> 
> Andrew suggested to remove MEMORY_POLICY_X86_UC_MINUS and x86_UC_minus
> completely.  If that's the consensus I am happy to respin the series
> removing code.
> 
> 
> Changes in v3:
> - s/nGRE/nGnRE/g
> - s/LIBXL_MEMORY_POLICY_ARM_DEV_NGRE/LIBXL_MEMORY_POLICY_ARM_DEV_NGNRE/g
> - s/arm_devmem/arm_dev_nGnRE/g
> - s/arm_memory/arm_mem_WB/g
> - improve commit message
> - improve man page
> - s/MEMORY_POLICY_X86_UC/MEMORY_POLICY_X86_UC_MINUS/g
> - s/x86_uc/x86_UC_minus/g
> - move security support clarification to a separate patch
> 
> Changes in v2:
> - add #define LIBXL_HAVE_MEMORY_POLICY
> - ability to part the memory policy parameter even if gfn is not passed
> - rename cache_policy to memory policy
> - rename MEMORY_POLICY_DEVMEM to MEMORY_POLICY_ARM_DEV_nGRE
> - rename MEMORY_POLICY_MEMORY to MEMORY_POLICY_ARM_MEM_WB
> - rename memory to arm_memory and devmem to arm_devmem
> - expand the non-security support status to non device passthrough iomem
>    configurations
> - rename iomem options
> - add x86 specific iomem option
> ---
>   docs/man/xl.cfg.5.pod.in    | 10 +++++++++-
>   tools/libxl/libxl.h         |  5 +++++
>   tools/libxl/libxl_arch.h    |  3 +++
>   tools/libxl/libxl_arm.c     | 14 ++++++++++++++
>   tools/libxl/libxl_create.c  | 12 ++++++++++--
>   tools/libxl/libxl_types.idl |  9 +++++++++
>   tools/libxl/libxl_x86.c     | 12 ++++++++++++
>   tools/xl/xl_parse.c         | 22 +++++++++++++++++++++-
>   8 files changed, 83 insertions(+), 4 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index c99d40307e..fbb9e43e9e 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -1222,7 +1222,7 @@ is given in hexadecimal format and may either be a range, e.g. C<2f8-2ff>
>   It is recommended to only use this option for trusted VMs under
>   administrator's control.
>   
> -=item B<iomem=[ "IOMEM_START,NUM_PAGES[@GFN]", "IOMEM_START,NUM_PAGES[@GFN]", ...]>
> +=item B<iomem=[ "IOMEM_START,NUM_PAGES[@GFN],MEMORY_POLICY", "IOMEM_START,NUM_PAGES[@GFN][,MEMORY_POLICY]", ...]>
>   
>   Allow auto-translated domains to access specific hardware I/O memory pages.
>   
> @@ -1233,6 +1233,14 @@ B<GFN> is not specified, the mapping will be performed using B<IOMEM_START>
>   as a start in the guest's address space, therefore performing a 1:1 mapping
>   by default.
>   All of these values must be given in hexadecimal format.
> +B<MEMORY_POLICY> for ARM platforms:

Ditto.

> +  - "arm_dev_nGnRE" for Device-nGnRE (Device Memory on Armv7), the default on ARM

Ditto.

> +  - "arm_mem_WB" for Outer Shareable Write-Back Cacheable Memory
> +They select the stage-2 memory attributes, but note that the resulting
> +memory attributes will be a combination of stage-2 and stage-1 memory
> +attributes: it will be the strongest between the 2 stages attributes.
> +B<MEMORY_POLICY> can be for x86 platforms:
> +  - "x86_UC_minus" for Uncachable Memory, the default on x86
>   
>   Note that the IOMMU won't be updated with the mappings specified with this
>   option. This option therefore should not be used to pass through any
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 9bacfb97f0..cf12f1d3bd 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -378,6 +378,11 @@
>   #define LIBXL_HAVE_BUILDINFO_BOOTLOADER 1
>   #define LIBXL_HAVE_BUILDINFO_BOOTLOADER_ARGS 1
>   
> +/*
> + * Support specifying memory policy information for memory mappings.
> + */
> +#define LIBXL_HAVE_MEMORY_POLICY 1
> +
>   /*
>    * LIBXL_HAVE_EXTENDED_VKB indicates that libxl_device_vkb has extended fields:
>    *  - unique_id;
> diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
> index d624159e53..9c858b06ce 100644
> --- a/tools/libxl/libxl_arch.h
> +++ b/tools/libxl/libxl_arch.h
> @@ -77,6 +77,9 @@ int libxl__arch_extra_memory(libxl__gc *gc,
>                                const libxl_domain_build_info *info,
>                                uint64_t *out);
>   
> +_hidden
> +int libxl__memory_policy_to_xc(libxl_memory_policy c);

Functions name are prefixed with libxl__arch_ in this header.

[...]

> @@ -1895,11 +1896,30 @@ void parse_config_data(const char *config_source,
>                            &b_info->iomem[i].start,
>                            &b_info->iomem[i].number, &used,
>                            &b_info->iomem[i].gfn, &used);
> -            if (ret < 2 || buf[used] != '\0') {
> +            if (ret < 2) {
>                   fprintf(stderr,
>                           "xl: Invalid argument parsing iomem: %s\n", buf);
>                   exit(1);
>               }
> +            mempolicy = &buf[used];
> +            if (strlen(mempolicy) > 1) {
> +                mempolicy++;
> +                if (!strcmp(mempolicy, "arm_dev_nGnRE"))
> +                    b_info->iomem[i].memory_policy =
> +                        LIBXL_MEMORY_POLICY_ARM_DEV_NGNRE;
> +                else if (!strcmp(mempolicy, "x86_UC_minus"))
> +                    b_info->iomem[i].memory_policy =
> +                        LIBXL_MEMORY_POLICY_X86_UC_MINUS;
> +                else if (!strcmp(mempolicy, "arm_mem_WB"))
> +                    b_info->iomem[i].memory_policy =
> +                        LIBXL_MEMORY_POLICY_ARM_MEM_WB;

Any reason to not keep all arm policies together?

> +                else {
> +                    fprintf(stderr,
> +                            "xl: Invalid iomem memory policy parameter: %s\n",
> +                            mempolicy);
> +                    exit(1);
> +                }
> +            }
>           }
>       }
>   
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel