Re: [PATCH v2 03/10] xen: extend XEN_DOMCTL_memory_mapping to handle memory policy

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

Hi Stefano,

On 4/30/19 10:02 PM, Stefano Stabellini wrote:
> Reuse the existing padding field to pass memory policy information.  On

NIT: I know that some developper like using two spaces after the final 
point. I don't mind if you use it but please be at least consistent 
within the commit message.

> Arm, the caller can specify whether the memory should be mapped as
> device nGRE, which is the default and the only possibility today, or

I am afraid this is not correct. The default on is Device-nGnRE (it is 
called Device Memory on Armv7).

> cacheable memory write-back. On x86, the only option is uncachable. The
> current behavior becomes the default (numerically '0').
> 
> On ARM, map device nGRE as p2m_mmio_direct_dev (as it is already done
> today) and WB cacheable memory as p2m_mmio_direct_c.

As I pointed out in v1, the wording is confusing. The resulting memory 
attribute will be a combination of stage-2 and stage-2 memory 
attributes. It will actually be whatever is the strongest between the 2 
stages attributes. You can see the stage-2 attributes as a way to give 
more or less freedom to the guest for configure the attributes.

The commit message and all documentation should actually reflect that to 
avoid misuse of the new option.

> 
> On x86, return error if the memory policy requested is not
> MEMORY_POLICY_X86_UC.
> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> CC: JBeulich@suse.com
> CC: andrew.cooper3@citrix.com
> 
> ---
> Changes in v2:
> - rebase
> - use p2m_mmio_direct_c
> - use EOPNOTSUPP
> - 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
> - add MEMORY_POLICY_X86_UC
> - add MEMORY_POLICY_DEFAULT and use it
> ---
>   xen/common/domctl.c         | 23 +++++++++++++++++++++--
>   xen/include/public/domctl.h | 14 +++++++++++++-
>   2 files changed, 34 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 140f979..9f62ead 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -928,6 +928,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>           unsigned long mfn_end = mfn + nr_mfns - 1;
>           int add = op->u.memory_mapping.add_mapping;
>           p2m_type_t p2mt;
> +        uint32_t memory_policy = op->u.memory_mapping.memory_policy;
>   
>           ret = -EINVAL;
>           if ( mfn_end < mfn || /* wrap? */
> @@ -958,9 +959,27 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>           if ( add )
>           {
>               printk(XENLOG_G_DEBUG
> -                   "memory_map:add: dom%d gfn=%lx mfn=%lx nr=%lx\n",
> -                   d->domain_id, gfn, mfn, nr_mfns);
> +                   "memory_map:add: dom%d gfn=%lx mfn=%lx nr=%lx cache=%u\n",
> +                   d->domain_id, gfn, mfn, nr_mfns, memory_policy);
>   
> +            switch ( memory_policy )
> +            {
> +#ifdef CONFIG_ARM
> +                case MEMORY_POLICY_ARM_MEM_WB:
> +                    p2mt = p2m_mmio_direct_c;
> +                    break;
> +                case MEMORY_POLICY_ARM_DEV_nGRE:
> +                    p2mt = p2m_mmio_direct_dev;
> +                    break;
> +#endif
> +#ifdef CONFIG_X86
> +                case MEMORY_POLICY_X86_UC:
> +                    p2mt = p2m_mmio_direct;
> +                    break;
> +#endif
> +                default:
> +                    return -EOPNOTSUPP;
> +            }
>               ret = map_regions(d, _gfn(gfn), nr_mfns, _mfn(mfn), p2mt);
>               if ( ret < 0 )
>                   printk(XENLOG_G_WARNING
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 19486d5..9330387 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -571,12 +571,24 @@ struct xen_domctl_bind_pt_irq {
>   */
>   #define DPCI_ADD_MAPPING         1
>   #define DPCI_REMOVE_MAPPING      0
> +/*
> + * Default memory policy. Corresponds to:
> + * Arm: MEMORY_POLICY_ARM_DEV_nGRE
> + * x86: MEMORY_POLICY_X86_UC
> + */
> +#define MEMORY_POLICY_DEFAULT    0
> +/* x86 only. Memory type UNCACHABLE */
> +#define MEMORY_POLICY_X86_UC     0
> +/* Arm only. Outer Shareable, Device-nGRE memory */

Device-nGRE is an Armv8 term. You might want to also specify the Armv7 
name in parenthesis to help the user.

> +#define MEMORY_POLICY_ARM_DEV_nGRE       0
> +/* Arm only. Outer Shareable, Outer/Inner Write-Back Cacheable memory */
> +#define MEMORY_POLICY_ARM_MEM_WB         1

I am wondering whether we should put Arm (resp. x86) defines under an 
ifdef arm (resp. x86). Do you see any use in the common toolstack code 
of those #ifdef?

>   struct xen_domctl_memory_mapping {
>       uint64_aligned_t first_gfn; /* first page (hvm guest phys page) in range */
>       uint64_aligned_t first_mfn; /* first page (machine page) in range */
>       uint64_aligned_t nr_mfns;   /* number of pages in range (>0) */
>       uint32_t add_mapping;       /* add or remove mapping */
> -    uint32_t padding;           /* padding for 64-bit aligned structure */
> +    uint32_t memory_policy;      /* cacheability of the memory mapping */

 From a quick look at libxc, it seems the padding field will not be 
initialized to 0 (aka MEMORY_DEFAULT_POLICY). As the libxc support is 
added in a follow-up patch, I think you want to ensure memory_policy is 
equal to MEMORY_DEFAULT_POLICY in libxc. So there are no unexpected 
behavior during bisection or this patch gets applied before the rest.

Cheers,

-- 
Julien Grall

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