Re: [Xen-devel] [PATCH v4 3/6] xen: extend XEN_DOMCTL_memory_mapping to handle memory policy

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.08.2019 02:23, Stefano Stabellini wrote:
> @@ -950,9 +951,29 @@ 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 policy=%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_nGnRE:
> +                    p2mt = p2m_mmio_direct_dev;
> +                    break;
> +#endif
> +#ifdef CONFIG_X86
> +                case MEMORY_POLICY_DEFAULT:
> +                    p2mt = p2m_mmio_direct;
> +                    break;
> +#endif
> +                default:
> +                    domctl_lock_release();
> +                    ret = -EINVAL;
> +                    goto domctl_out_unlock_domonly;
> +            }
Indentation is wrong here: The case labels ought to align with their
switch(). Also please have blank lines between case blocks. For the
Arm part it would be nice if the case being MEMORY_POLICY_DEFAULT
would gain a respective comment, perhaps in the form of a commented
out case label.

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -571,12 +571,30 @@ 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_nGnRE
> + * x86: Memory type UNCACHABLE
> + */
I continue to be unhappy about the imprecise "uncachable" here
for x86: In shadow mode and on AMD/NPT we produce UC- mappings,
while the way EPT works it further depends on host and guest
MTRR settings.  Andrew - do you have any thoughts how to best
describe this here without growing the text too large, but also
without producing an overly imprecise description?

> +#define MEMORY_POLICY_DEFAULT         0
> +#if defined(__arm__) || defined (__aarch64__)
> +/* Arm only. Outer Shareable, Device-nGnRE memory (Device Memory on Armv7) */
> +# define MEMORY_POLICY_ARM_DEV_nGnRE      0
> +/* Arm only. Outer Shareable, Outer/Inner Write-Back Cacheable memory */
> +# define MEMORY_POLICY_ARM_MEM_WB         1
> +/*
> + * On Arm, MEMORY_POLICY selects 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.
> + */
> +#endif
>   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 */
The comment looks to start one column too far to the right.

Jan

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