diff mbox series

[v6,02/11] xen: introduce CDF_directmap

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

Commit Message

Penny Zheng Feb. 14, 2022, 3:19 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
== host 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
---
v6 changes
- comment and coding style fix
- protect CDF_directmap with #ifdef CONFIG_ARM
---
 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              |  7 ++++++-
 7 files changed, 34 insertions(+), 8 deletions(-)

Comments

Julien Grall Feb. 15, 2022, 8:26 p.m. UTC | #1 
(+ Jan)

Hi Penny,

I am CCing Jan to give him a chance to...

On 14/02/2022 03:19, Penny Zheng wrote:
> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> index cfb0b47f13..24eb4cc7d3 100644
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -31,6 +31,10 @@ 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)
> +#ifdef CONFIG_ARM
> +/* Should domain memory be directly mapped? */
> +#define CDF_directmap            (1U << 1)
> +#endif

... comment on this approach. I would be happy to switch to an ASSERT() 
if that's preferred.

Please note that as you modify x86 code (even it is a couple of lines) 
you should technically CC the x86 maintainers. Similarly the changes in 
include/xen/domain.h should have the REST CCed.

We have a script that will find the proper correct CC for each patch 
(see scripts/add_maintainers.pl). The workflow is written down in the 
script itself.

Cheers,
Jan Beulich Feb. 16, 2022, 9:34 a.m. UTC | #2 
On 15.02.2022 21:26, Julien Grall wrote:
> (+ Jan)
> 
> Hi Penny,
> 
> I am CCing Jan to give him a chance to...

Thanks, but ...

> On 14/02/2022 03:19, Penny Zheng wrote:
>> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
>> index cfb0b47f13..24eb4cc7d3 100644
>> --- a/xen/include/xen/domain.h
>> +++ b/xen/include/xen/domain.h
>> @@ -31,6 +31,10 @@ 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)
>> +#ifdef CONFIG_ARM
>> +/* Should domain memory be directly mapped? */
>> +#define CDF_directmap            (1U << 1)
>> +#endif
> 
> ... comment on this approach. I would be happy to switch to an ASSERT() 
> if that's preferred.

... I think I did signal agreement with this approach beforehand.
It leaves the option to use the same bit for something x86-specific
down the road.

Jan
Julien Grall Feb. 17, 2022, 11:01 a.m. UTC | #3 
Hi Jan,

On 16/02/2022 09:34, Jan Beulich wrote:
> On 15.02.2022 21:26, Julien Grall wrote:
>> (+ Jan)
>>
>> Hi Penny,
>>
>> I am CCing Jan to give him a chance to...
> 
> Thanks, but ...
> 
>> On 14/02/2022 03:19, Penny Zheng wrote:
>>> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
>>> index cfb0b47f13..24eb4cc7d3 100644
>>> --- a/xen/include/xen/domain.h
>>> +++ b/xen/include/xen/domain.h
>>> @@ -31,6 +31,10 @@ 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)
>>> +#ifdef CONFIG_ARM
>>> +/* Should domain memory be directly mapped? */
>>> +#define CDF_directmap            (1U << 1)
>>> +#endif
>>
>> ... comment on this approach. I would be happy to switch to an ASSERT()
>> if that's preferred.
> 
> ... I think I did signal agreement with this approach beforehand.

You raised a concern and it wasn't 100% obvious whether you would still 
be happy if we merged it as-is.

So I preferred to ask rather than merging it and getting an angry 
e-mail/message afterwards :).

Anyway, this series is now fully acked. So I will commit the series in a 
bit.

Cheers,
Julien Grall Feb. 17, 2022, 7:47 p.m. UTC | #4 
Hi,

On 17/02/2022 11:01, Julien Grall wrote:
> On 16/02/2022 09:34, Jan Beulich wrote:
>> On 15.02.2022 21:26, Julien Grall wrote:
>>> (+ Jan)
>>>
>>> Hi Penny,
>>>
>>> I am CCing Jan to give him a chance to...
>>
>> Thanks, but ...
>>
>>> On 14/02/2022 03:19, Penny Zheng wrote:
>>>> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
>>>> index cfb0b47f13..24eb4cc7d3 100644
>>>> --- a/xen/include/xen/domain.h
>>>> +++ b/xen/include/xen/domain.h
>>>> @@ -31,6 +31,10 @@ 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)
>>>> +#ifdef CONFIG_ARM
>>>> +/* Should domain memory be directly mapped? */
>>>> +#define CDF_directmap            (1U << 1)
>>>> +#endif
>>>
>>> ... comment on this approach. I would be happy to switch to an ASSERT()
>>> if that's preferred.
>>
>> ... I think I did signal agreement with this approach beforehand.
> 
> You raised a concern and it wasn't 100% obvious whether you would still 
> be happy if we merged it as-is.
> 
> So I preferred to ask rather than merging it and getting an angry 
> e-mail/message afterwards :).
> 
> Anyway, this series is now fully acked. So I will commit the series in a 
> bit.

Well, I forgot to explicitely Acked this patch. So:

Acked-by: Julien Grall <jgrall@amazon.com>

This series is now fully committed. Thank you for your contribution!

Cheers,
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..aabe942cde 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..24eb4cc7d3 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -31,6 +31,10 @@  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)
+#ifdef CONFIG_ARM
+/* Should domain memory be directly mapped? */
+#define CDF_directmap            (1U << 1)
+#endif
 
 /*
  * Arch-specifics.
@@ -65,7 +69,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);