Re: [PATCH v5 02/11] xen: introduce CDF_directmap

[Julien Grall <julien@xxxxxxx>]

Hi,

On 27/01/2022 07:49, Penny Zheng wrote:
> +- direct-map
> +
> +    Only available when statically allocated memory is used for the domain.
> +    An empty property to request the memory of the domain to be
> +    direct-map (guest physical address == physical address).
NIT: I would add "host" just after == so it is more explicit that we are 
talking about host physical address.
> +
>   Under the "xen,domain" compatible node, one or more sub-nodes are present
>   for the DomU kernel and ramdisk.
>   
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 92a6c509e5..8110c1df86 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -692,7 +692,8 @@ int arch_sanitise_domain_config(struct 
> xen_domctl_createdomain *config)
>   }
>   
>   int arch_domain_create(struct domain *d,
> -                       struct xen_domctl_createdomain *config)
> +                       struct xen_domctl_createdomain *config,
> +                       unsigned int flags)
>   {
>       int rc, count = 0;
>   
> @@ -708,6 +709,8 @@ int arch_domain_create(struct domain *d,
>       ioreq_domain_init(d);
>   #endif
>   
> +    d->arch.directmap = flags & CDF_directmap;
> +
>       /* p2m_init relies on some value initialized by the IOMMU subsystem */
>       if ( (rc = iommu_domain_init(d, config->iommu_opts)) != 0 )
>           goto fail;
[...]

> diff --git a/xen/arch/arm/include/asm/domain.h 
> b/xen/arch/arm/include/asm/domain.h
> index 9b3647587a..cb37ce89ec 100644
> --- a/xen/arch/arm/include/asm/domain.h
> +++ b/xen/arch/arm/include/asm/domain.h
> @@ -29,8 +29,7 @@ enum domain_type {
>   #define is_64bit_domain(d) (0)
>   #endif
>   
> -/* The hardware domain has always its memory direct mapped. */
> -#define is_domain_direct_mapped(d) is_hardware_domain(d)
> +#define is_domain_direct_mapped(d) (d->arch.directmap)
The outter parentheses are not necessary. However, you want to surround 
d with parentheses to avoid any surprise.
>   
>   struct vtimer {
>       struct vcpu *v;
> @@ -89,6 +88,8 @@ struct arch_domain
>   #ifdef CONFIG_TEE
>       void *tee;
>   #endif
> +
> +    bool directmap;
I am OK with creating a boolean for now. But long term, I think we 
should switch to storing the flags directly as this is more space 
efficient and make easier to add new flags (see d->options for instance).
>   }  __cacheline_aligned;
>   
>   struct arch_vcpu
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index ef1812dc14..9835f90ea0 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -722,7 +722,8 @@ static bool emulation_flags_ok(const struct domain *d, 
> uint32_t emflags)
>   }
>   
>   int arch_domain_create(struct domain *d,
> -                       struct xen_domctl_createdomain *config)
> +                       struct xen_domctl_createdomain *config,
> +                       unsigned int flags)
Shouldn't we return an error if the flag CDF_directmap is on x86? The 
other alternative is to...
>   {
>       bool paging_initialised = false;
>       uint32_t emflags;
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index a79103e04a..3742322d22 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -659,7 +659,7 @@ struct domain *domain_create(domid_t domid,
>           radix_tree_init(&d->pirq_tree);
>       }
>   
> -    if ( (err = arch_domain_create(d, config)) != 0 )
> +    if ( (err = arch_domain_create(d, config, flags)) != 0 )
>           goto fail;
>       init_status |= INIT_arch;
>   
> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> index cfb0b47f13..3a2371d715 100644
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -31,6 +31,8 @@ void arch_get_domain_info(const struct domain *d,
>   /* CDF_* constant. Internal flags for domain creation. */
>   /* Is this a privileged domain? */
>   #define CDF_privileged           (1U << 0)
> +/* Should domain memory be directly mapped? */
> +#define CDF_directmap            (1U << 1)
... protect this with #ifdef CONFIG_ARM.

Jan, what do you think?

>   
>   /*
>    * Arch-specifics.
> @@ -65,7 +67,8 @@ int map_vcpu_info(struct vcpu *v, unsigned long gfn, unsigned 
> offset);
>   void unmap_vcpu_info(struct vcpu *v);
>   
>   int arch_domain_create(struct domain *d,
> -                       struct xen_domctl_createdomain *config);
> +                       struct xen_domctl_createdomain *config,
> +                       unsigned int flags);
>   
>   void arch_domain_destroy(struct domain *d);
>   
Cheers,

--
Julien Grall