diff mbox series

[v5,02/11] xen: introduce CDF_directmap

Message ID 20220127074929.502885-3-penny.zheng@arm.com (mailing list archive)
State Superseded
Headers show
Series direct-map memory map | expand

Commit Message

Penny Zheng Jan. 27, 2022, 7:49 a.m. UTC 
From: Stefano Stabellini <sstabellini@kernel.org>

This commit introduces a new arm-specific flag CDF_directmap to specify
that a domain should have its memory direct-map(guest physical address
== physical address) at domain creation.

Also, add a directmap flag under struct arch_domain and use it to
reimplement is_domain_direct_mapped.

For now, direct-map is only available when statically allocated memory is
used for the domain, that is, "xen,static-mem" must be also defined in the
domain configuration.

Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Tested-by: Stefano Stabellini <sstabellini@kernel.org>
---
CC: andrew.cooper3@citrix.com
CC: jbeulich@suse.com
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Wei Liu <wl@xen.org>
CC: "Roger Pau Monné" <roger.pau@citrix.com>
---
v2 changes
- remove the introduce of internal flag
- remove flag direct_map since we already store this flag in d->options
- Refine is_domain_direct_mapped to check whether the flag
XEN_DOMCTL_CDF_directmap is set
- reword "1:1 direct-map" to just "direct-map"
---
v3 changes
- move flag back to xen/include/xen/domain.h, to let it be only available for
domain created by XEN.
- name it with extra "INTERNAL" and add comments to warn developers not
to accidently use its bitfield when introducing new XEN_DOMCTL_CDF_xxx flag.
- reject this flag in x86'es arch_sanitise_domain_config()
---
v4 changes
- introduce new internal flag CDF_directmap
- add a directmap flag under struct arch_domain and use it to
reimplement is_domain_direct_mapped.
- expand arch_domain_create to include internal-only parameter "const unsigned
int flags"
---
v5 changes
- remove const constraint
- strict "static allocation" check
---
 docs/misc/arm/device-tree/booting.txt |  6 ++++++
 xen/arch/arm/domain.c                 |  5 ++++-
 xen/arch/arm/domain_build.c           | 14 ++++++++++++--
 xen/arch/arm/include/asm/domain.h     |  5 +++--
 xen/arch/x86/domain.c                 |  3 ++-
 xen/common/domain.c                   |  2 +-
 xen/include/xen/domain.h              |  5 ++++-
 7 files changed, 32 insertions(+), 8 deletions(-)

Comments

Julien Grall Feb. 9, 2022, 12:42 p.m. UTC | #1 
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,
Jan Beulich Feb. 9, 2022, 1:21 p.m. UTC | #2 
On 09.02.2022 13:42, Julien Grall wrote:
> On 27/01/2022 07:49, Penny Zheng wrote:
>> --- 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...

Hmm, maybe rather assert that the flag is not set? But ...

>> --- 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.

... I don't mind an #ifdef here, apart from the general concern over
CONFIG_<arch> uses in common code.

Jan
diff mbox series

Patch

diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 71895663a4..a94125394e 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -182,6 +182,12 @@  with the following properties:
     Both #address-cells and #size-cells need to be specified because
     both sub-nodes (described shortly) have reg properties.
 
+- 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).
+
 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/domain_build.c b/xen/arch/arm/domain_build.c
index 0fab8604de..6467e8ee32 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -3029,10 +3029,20 @@  void __init create_domUs(void)
             .max_maptrack_frames = -1,
             .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version),
         };
+        unsigned int flags = 0U;
 
         if ( !dt_device_is_compatible(node, "xen,domain") )
             continue;
 
+        if ( dt_property_read_bool(node, "direct-map") )
+        {
+            if ( !IS_ENABLED(CONFIG_STATIC_MEMORY) || !dt_find_property(node, "xen,static-mem", NULL) )
+                panic("direct-map is not valid for domain %s without static allocation.\n",
+                      dt_node_name(node));
+
+            flags |= CDF_directmap;
+        }
+
         if ( !dt_property_read_u32(node, "cpus", &d_cfg.max_vcpus) )
             panic("Missing property 'cpus' for domain %s\n",
                   dt_node_name(node));
@@ -3058,7 +3068,7 @@  void __init create_domUs(void)
          * very important to use the pre-increment operator to call
          * domain_create() with a domid > 0. (domid == 0 is reserved for Dom0)
          */
-        d = domain_create(++max_init_domid, &d_cfg, 0);
+        d = domain_create(++max_init_domid, &d_cfg, flags);
         if ( IS_ERR(d) )
             panic("Error creating domain %s\n", dt_node_name(node));
 
@@ -3160,7 +3170,7 @@  void __init create_dom0(void)
     if ( iommu_enabled )
         dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
 
-    dom0 = domain_create(0, &dom0_cfg, CDF_privileged);
+    dom0 = domain_create(0, &dom0_cfg, CDF_privileged | CDF_directmap);
     if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
         panic("Error creating domain 0\n");
 
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)
 
 struct vtimer {
     struct vcpu *v;
@@ -89,6 +88,8 @@  struct arch_domain
 #ifdef CONFIG_TEE
     void *tee;
 #endif
+
+    bool directmap;
 }  __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)
 {
     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)
 
 /*
  * 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);