* [PATCH 01/11] xen: reserve flags for internal usage in xen_domctl_createdomain
2021-09-23 3:11 [PATCH 00/11] 1:1 direct-map memory map Penny Zheng
@ 2021-09-23 3:11 ` Penny Zheng
2021-09-23 9:54 ` Julien Grall
2021-09-23 3:11 ` [PATCH 02/11] xen/arm: introduce XEN_DOMCTL_INTERNAL_directmap Penny Zheng
` (9 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Penny Zheng @ 2021-09-23 3:11 UTC (permalink / raw)
To: xen-devel, sstabellini, julien; +Cc: Bertrand.Marquis, Penny.Zheng, Wei.Chen
From: Stefano Stabellini <stefano.stabellini@xilinx.com>
We are passing an extra special boolean flag at domain creation to
specify whether we want to the domain to be privileged (i.e. dom0) or
not. Another flag will be introduced later in this series.
Reserve bits 16-31 from the existing flags bitfield in struct
xen_domctl_createdomain for internal Xen usage.
Allocate bit 16 for XEN_DOMCTL_INTERNAL_ispriv: whether a domain is
privileged or not.
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
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>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Dario Faggioli <dfaggioli@suse.com>
---
xen/arch/arm/domain.c | 2 +-
xen/arch/arm/domain_build.c | 7 ++++---
xen/arch/x86/setup.c | 4 +++-
xen/common/domain.c | 19 +++++++++----------
xen/common/domctl.c | 3 ++-
xen/common/sched/core.c | 2 +-
xen/include/public/domctl.h | 3 +++
xen/include/xen/domain.h | 4 ++++
xen/include/xen/sched.h | 3 +--
9 files changed, 28 insertions(+), 19 deletions(-)
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 19c756ac3d..7922249d26 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -623,7 +623,7 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
unsigned int max_vcpus;
/* HVM and HAP must be set. IOMMU may or may not be */
- if ( (config->flags & ~XEN_DOMCTL_CDF_iommu) !=
+ if ( ((config->flags & XEN_DOMCTL_CDF_MASK) & ~XEN_DOMCTL_CDF_iommu) !=
(XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap) )
{
dprintk(XENLOG_INFO, "Unsupported configuration %#x\n",
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index d233d634c1..8cc4c800e9 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2679,7 +2679,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, false);
+ d = domain_create(++max_init_domid, &d_cfg);
if ( IS_ERR(d) )
panic("Error creating domain %s\n", dt_node_name(node));
@@ -2752,7 +2752,8 @@ void __init create_dom0(void)
{
struct domain *dom0;
struct xen_domctl_createdomain dom0_cfg = {
- .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
+ .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap |
+ XEN_DOMCTL_INTERNAL_ispriv,
.max_evtchn_port = -1,
.max_grant_frames = gnttab_dom0_frames(),
.max_maptrack_frames = -1,
@@ -2773,7 +2774,7 @@ void __init create_dom0(void)
if ( iommu_enabled )
dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
- dom0 = domain_create(0, &dom0_cfg, true);
+ dom0 = domain_create(0, &dom0_cfg);
if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
panic("Error creating domain 0\n");
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index b101565f14..6b7a1a3994 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -770,9 +770,11 @@ static struct domain *__init create_dom0(const module_t *image,
if ( iommu_enabled )
dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
+ if ( !pv_shim )
+ dom0_cfg.flags |= XEN_DOMCTL_INTERNAL_ispriv;
/* Create initial domain 0. */
- d = domain_create(get_initial_domain_id(), &dom0_cfg, !pv_shim);
+ d = domain_create(get_initial_domain_id(), &dom0_cfg);
if ( IS_ERR(d) || (alloc_dom0_vcpu0(d) == NULL) )
panic("Error creating domain 0\n");
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 6ee5d033b0..5fcca9b018 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -475,11 +475,11 @@ static void _domain_destroy(struct domain *d)
static int sanitise_domain_config(struct xen_domctl_createdomain *config)
{
- bool hvm = config->flags & XEN_DOMCTL_CDF_hvm;
- bool hap = config->flags & XEN_DOMCTL_CDF_hap;
- bool iommu = config->flags & XEN_DOMCTL_CDF_iommu;
+ bool hvm = (config->flags & XEN_DOMCTL_CDF_MASK) & XEN_DOMCTL_CDF_hvm;
+ bool hap = (config->flags & XEN_DOMCTL_CDF_MASK) & XEN_DOMCTL_CDF_hap;
+ bool iommu = (config->flags & XEN_DOMCTL_CDF_MASK) & XEN_DOMCTL_CDF_iommu;
- if ( config->flags &
+ if ( (config->flags & XEN_DOMCTL_CDF_MASK) &
~(XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap |
XEN_DOMCTL_CDF_s3_integrity | XEN_DOMCTL_CDF_oos_off |
XEN_DOMCTL_CDF_xs_domain | XEN_DOMCTL_CDF_iommu |
@@ -536,8 +536,7 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
}
struct domain *domain_create(domid_t domid,
- struct xen_domctl_createdomain *config,
- bool is_priv)
+ struct xen_domctl_createdomain *config)
{
struct domain *d, **pd, *old_hwdom = NULL;
enum { INIT_watchdog = 1u<<1,
@@ -563,7 +562,7 @@ struct domain *domain_create(domid_t domid,
}
/* Sort out our idea of is_control_domain(). */
- d->is_privileged = is_priv;
+ d->is_privileged = config ? config->flags & XEN_DOMCTL_INTERNAL_ispriv : 0;
/* Sort out our idea of is_hardware_domain(). */
if ( domid == 0 || domid == hardware_domid )
@@ -756,7 +755,7 @@ void __init setup_system_domains(void)
* Hidden PCI devices will also be associated with this domain
* (but be [partly] controlled by Dom0 nevertheless).
*/
- dom_xen = domain_create(DOMID_XEN, NULL, false);
+ dom_xen = domain_create(DOMID_XEN, NULL);
if ( IS_ERR(dom_xen) )
panic("Failed to create d[XEN]: %ld\n", PTR_ERR(dom_xen));
@@ -766,7 +765,7 @@ void __init setup_system_domains(void)
* array. Mappings occur at the priv of the caller.
* Quarantined PCI devices will be associated with this domain.
*/
- dom_io = domain_create(DOMID_IO, NULL, false);
+ dom_io = domain_create(DOMID_IO, NULL);
if ( IS_ERR(dom_io) )
panic("Failed to create d[IO]: %ld\n", PTR_ERR(dom_io));
@@ -775,7 +774,7 @@ void __init setup_system_domains(void)
* Initialise our COW domain.
* This domain owns sharable pages.
*/
- dom_cow = domain_create(DOMID_COW, NULL, false);
+ dom_cow = domain_create(DOMID_COW, NULL);
if ( IS_ERR(dom_cow) )
panic("Failed to create d[COW]: %ld\n", PTR_ERR(dom_cow));
#endif
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 12d6144d28..2ec6d454dd 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -431,7 +431,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
rover = dom;
}
- d = domain_create(dom, &op->u.createdomain, false);
+ op->u.createdomain.flags &= XEN_DOMCTL_CDF_MASK;
+ d = domain_create(dom, &op->u.createdomain);
if ( IS_ERR(d) )
{
ret = PTR_ERR(d);
diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index 8f4b1ca10d..27d5bc2259 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -3021,7 +3021,7 @@ void __init scheduler_init(void)
sched_ratelimit_us = SCHED_DEFAULT_RATELIMIT_US;
}
- idle_domain = domain_create(DOMID_IDLE, NULL, false);
+ idle_domain = domain_create(DOMID_IDLE, NULL);
BUG_ON(IS_ERR(idle_domain));
BUG_ON(nr_cpu_ids > ARRAY_SIZE(idle_vcpu));
idle_domain->vcpu = idle_vcpu;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 96696e3842..4d3fcd3bcb 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -74,6 +74,9 @@ struct xen_domctl_createdomain {
/* Max XEN_DOMCTL_CDF_* constant. Used for ABI checking. */
#define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_nested_virt
+/* Flags from (1U<<16) to (1U<<31) are reserved for internal usage */
+#define XEN_DOMCTL_CDF_MASK (0xffff)
+
uint32_t flags;
#define _XEN_DOMCTL_IOMMU_no_sharept 0
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 1708c36964..7ed0b62b78 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -28,6 +28,10 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info);
void arch_get_domain_info(const struct domain *d,
struct xen_domctl_getdomaininfo *info);
+/* Flags are reserved for internal usage */
+#define _XEN_DOMCTL_INTERNAL_ispriv 16
+#define XEN_DOMCTL_INTERNAL_ispriv (1U<<_XEN_DOMCTL_INTERNAL_ispriv)
+
/*
* Arch-specifics.
*/
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 28146ee404..a659b25dea 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -664,8 +664,7 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config);
* (domid < DOMID_FIRST_RESERVED).
*/
struct domain *domain_create(domid_t domid,
- struct xen_domctl_createdomain *config,
- bool is_priv);
+ struct xen_domctl_createdomain *config);
/*
* rcu_lock_domain_by_id() is more efficient than get_domain_by_id().
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 01/11] xen: reserve flags for internal usage in xen_domctl_createdomain
2021-09-23 3:11 ` [PATCH 01/11] xen: reserve flags for internal usage in xen_domctl_createdomain Penny Zheng
@ 2021-09-23 9:54 ` Julien Grall
2021-09-28 12:05 ` Jan Beulich
0 siblings, 1 reply; 37+ messages in thread
From: Julien Grall @ 2021-09-23 9:54 UTC (permalink / raw)
To: Penny Zheng, xen-devel, sstabellini; +Cc: Bertrand.Marquis, Wei.Chen
Hi,
On 23/09/2021 08:11, Penny Zheng wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
>
> We are passing an extra special boolean flag at domain creation to
> specify whether we want to the domain to be privileged (i.e. dom0) or
> not. Another flag will be introduced later in this series.
>
> Reserve bits 16-31 from the existing flags bitfield in struct
> xen_domctl_createdomain for internal Xen usage.
I am a bit split with this approach. This feels a bit of a hack to
reserve bits for internal purpose in external headers. But at the same
time I can see how this is easier to deal with it over repurposing the
last argument of domain_create().
I would suggest to add an extra sentence to explain the goal is to
consolidate all the information for a domain in a single structure.
> > Allocate bit 16 for XEN_DOMCTL_INTERNAL_ispriv: whether a domain is
> privileged or not.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> 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>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Dario Faggioli <dfaggioli@suse.com>
> ---
> xen/arch/arm/domain.c | 2 +-
> xen/arch/arm/domain_build.c | 7 ++++---
> xen/arch/x86/setup.c | 4 +++-
> xen/common/domain.c | 19 +++++++++----------
> xen/common/domctl.c | 3 ++-
> xen/common/sched/core.c | 2 +-
> xen/include/public/domctl.h | 3 +++
> xen/include/xen/domain.h | 4 ++++
> xen/include/xen/sched.h | 3 +--
> 9 files changed, 28 insertions(+), 19 deletions(-)
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 19c756ac3d..7922249d26 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -623,7 +623,7 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
> unsigned int max_vcpus;
>
> /* HVM and HAP must be set. IOMMU may or may not be */
> - if ( (config->flags & ~XEN_DOMCTL_CDF_iommu) !=
> + if ( ((config->flags & XEN_DOMCTL_CDF_MASK) & ~XEN_DOMCTL_CDF_iommu) !=
> (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap) )
> {
> dprintk(XENLOG_INFO, "Unsupported configuration %#x\n",
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index d233d634c1..8cc4c800e9 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2679,7 +2679,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, false);
> + d = domain_create(++max_init_domid, &d_cfg);
> if ( IS_ERR(d) )
> panic("Error creating domain %s\n", dt_node_name(node));
>
> @@ -2752,7 +2752,8 @@ void __init create_dom0(void)
> {
> struct domain *dom0;
> struct xen_domctl_createdomain dom0_cfg = {
> - .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
> + .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap |
> + XEN_DOMCTL_INTERNAL_ispriv,
> .max_evtchn_port = -1,
> .max_grant_frames = gnttab_dom0_frames(),
> .max_maptrack_frames = -1,
> @@ -2773,7 +2774,7 @@ void __init create_dom0(void)
> if ( iommu_enabled )
> dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>
> - dom0 = domain_create(0, &dom0_cfg, true);
> + dom0 = domain_create(0, &dom0_cfg);
> if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
> panic("Error creating domain 0\n");
>
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index b101565f14..6b7a1a3994 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -770,9 +770,11 @@ static struct domain *__init create_dom0(const module_t *image,
>
> if ( iommu_enabled )
> dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
> + if ( !pv_shim )
> + dom0_cfg.flags |= XEN_DOMCTL_INTERNAL_ispriv;
>
> /* Create initial domain 0. */
> - d = domain_create(get_initial_domain_id(), &dom0_cfg, !pv_shim);
> + d = domain_create(get_initial_domain_id(), &dom0_cfg);
> if ( IS_ERR(d) || (alloc_dom0_vcpu0(d) == NULL) )
> panic("Error creating domain 0\n");
>
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 6ee5d033b0..5fcca9b018 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -475,11 +475,11 @@ static void _domain_destroy(struct domain *d)
>
> static int sanitise_domain_config(struct xen_domctl_createdomain *config)
> {
> - bool hvm = config->flags & XEN_DOMCTL_CDF_hvm;
> - bool hap = config->flags & XEN_DOMCTL_CDF_hap;
> - bool iommu = config->flags & XEN_DOMCTL_CDF_iommu;
> + bool hvm = (config->flags & XEN_DOMCTL_CDF_MASK) & XEN_DOMCTL_CDF_hvm;
> + bool hap = (config->flags & XEN_DOMCTL_CDF_MASK) & XEN_DOMCTL_CDF_hap;
> + bool iommu = (config->flags & XEN_DOMCTL_CDF_MASK) & XEN_DOMCTL_CDF_iommu;
Adding "& XEN_DOMCTL_CDF_MASK" in the 3 lines above looks rather pointless.
>
> - if ( config->flags &
> + if ( (config->flags & XEN_DOMCTL_CDF_MASK) &
> ~(XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap |
> XEN_DOMCTL_CDF_s3_integrity | XEN_DOMCTL_CDF_oos_off |
> XEN_DOMCTL_CDF_xs_domain | XEN_DOMCTL_CDF_iommu |
> @@ -536,8 +536,7 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
> }
>
> struct domain *domain_create(domid_t domid,
> - struct xen_domctl_createdomain *config,
> - bool is_priv)
> + struct xen_domctl_createdomain *config)
> {
> struct domain *d, **pd, *old_hwdom = NULL;
> enum { INIT_watchdog = 1u<<1,
> @@ -563,7 +562,7 @@ struct domain *domain_create(domid_t domid,
> }
>
> /* Sort out our idea of is_control_domain(). */
> - d->is_privileged = is_priv;
> + d->is_privileged = config ? config->flags & XEN_DOMCTL_INTERNAL_ispriv : 0;
config->flags will be stored in d->options. Given there is a single
caller for d->is_privileged, I would drop the field and replace the use
with d->options & XEN_DOMCTL_INTERAL_ispriv?
>
> /* Sort out our idea of is_hardware_domain(). */
> if ( domid == 0 || domid == hardware_domid )
> @@ -756,7 +755,7 @@ void __init setup_system_domains(void)
> * Hidden PCI devices will also be associated with this domain
> * (but be [partly] controlled by Dom0 nevertheless).
> */
> - dom_xen = domain_create(DOMID_XEN, NULL, false);
> + dom_xen = domain_create(DOMID_XEN, NULL);
> if ( IS_ERR(dom_xen) )
> panic("Failed to create d[XEN]: %ld\n", PTR_ERR(dom_xen));
>
> @@ -766,7 +765,7 @@ void __init setup_system_domains(void)
> * array. Mappings occur at the priv of the caller.
> * Quarantined PCI devices will be associated with this domain.
> */
> - dom_io = domain_create(DOMID_IO, NULL, false);
> + dom_io = domain_create(DOMID_IO, NULL);
> if ( IS_ERR(dom_io) )
> panic("Failed to create d[IO]: %ld\n", PTR_ERR(dom_io));
>
> @@ -775,7 +774,7 @@ void __init setup_system_domains(void)
> * Initialise our COW domain.
> * This domain owns sharable pages.
> */
> - dom_cow = domain_create(DOMID_COW, NULL, false);
> + dom_cow = domain_create(DOMID_COW, NULL);
> if ( IS_ERR(dom_cow) )
> panic("Failed to create d[COW]: %ld\n", PTR_ERR(dom_cow));
> #endif
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 12d6144d28..2ec6d454dd 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -431,7 +431,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> rover = dom;
> }
>
> - d = domain_create(dom, &op->u.createdomain, false);
> + op->u.createdomain.flags &= XEN_DOMCTL_CDF_MASK;
I think it is a bad idea to silently ignore the reserved bits for
internal purpose. Instead, we should check and return and error if they
are set.
> + d = domain_create(dom, &op->u.createdomain);
> if ( IS_ERR(d) )
> {
> ret = PTR_ERR(d);
> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
> index 8f4b1ca10d..27d5bc2259 100644
> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -3021,7 +3021,7 @@ void __init scheduler_init(void)
> sched_ratelimit_us = SCHED_DEFAULT_RATELIMIT_US;
> }
>
> - idle_domain = domain_create(DOMID_IDLE, NULL, false);
> + idle_domain = domain_create(DOMID_IDLE, NULL);
> BUG_ON(IS_ERR(idle_domain));
> BUG_ON(nr_cpu_ids > ARRAY_SIZE(idle_vcpu));
> idle_domain->vcpu = idle_vcpu;
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 96696e3842..4d3fcd3bcb 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -74,6 +74,9 @@ struct xen_domctl_createdomain {
> /* Max XEN_DOMCTL_CDF_* constant. Used for ABI checking. */
> #define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_nested_virt
>
> +/* Flags from (1U<<16) to (1U<<31) are reserved for internal usage */
> +#define XEN_DOMCTL_CDF_MASK (0xffff)
I would suggest to add U at the end to make clear this is a unsigned value.
> +
> uint32_t flags;
>
> #define _XEN_DOMCTL_IOMMU_no_sharept 0
> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> index 1708c36964..7ed0b62b78 100644
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -28,6 +28,10 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info);
> void arch_get_domain_info(const struct domain *d,
> struct xen_domctl_getdomaininfo *info);
>
> +/* Flags are reserved for internal usage */
> +#define _XEN_DOMCTL_INTERNAL_ispriv 16
> +#define XEN_DOMCTL_INTERNAL_ispriv (1U<<_XEN_DOMCTL_INTERNAL_ispriv)
Coding style: space before and after <<.
> +
> /*
> * Arch-specifics.
> */
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 28146ee404..a659b25dea 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -664,8 +664,7 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config);
> * (domid < DOMID_FIRST_RESERVED).
> */
> struct domain *domain_create(domid_t domid,
> - struct xen_domctl_createdomain *config,
> - bool is_priv);
> + struct xen_domctl_createdomain *config);
>
> /*
> * rcu_lock_domain_by_id() is more efficient than get_domain_by_id().
>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 01/11] xen: reserve flags for internal usage in xen_domctl_createdomain
2021-09-23 9:54 ` Julien Grall
@ 2021-09-28 12:05 ` Jan Beulich
2021-10-11 10:45 ` Julien Grall
0 siblings, 1 reply; 37+ messages in thread
From: Jan Beulich @ 2021-09-28 12:05 UTC (permalink / raw)
To: Penny Zheng, xen-devel, sstabellini
Cc: Bertrand.Marquis, Wei.Chen, Julien Grall
On 23.09.2021 11:54, Julien Grall wrote:
> On 23/09/2021 08:11, Penny Zheng wrote:
>> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
>>
>> We are passing an extra special boolean flag at domain creation to
>> specify whether we want to the domain to be privileged (i.e. dom0) or
>> not. Another flag will be introduced later in this series.
>>
>> Reserve bits 16-31 from the existing flags bitfield in struct
>> xen_domctl_createdomain for internal Xen usage.
>
> I am a bit split with this approach. This feels a bit of a hack to
> reserve bits for internal purpose in external headers. But at the same
> time I can see how this is easier to deal with it over repurposing the
> last argument of domain_create().
I actually have trouble seeing why that's easier. It is a common thing
to widen a bool to "unsigned int flags" when more than one control is
needed. Plus this makes things needlessly harder once (in the future)
the low 16 bits are exhausted in the public interface.
Jan
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 01/11] xen: reserve flags for internal usage in xen_domctl_createdomain
2021-09-28 12:05 ` Jan Beulich
@ 2021-10-11 10:45 ` Julien Grall
2021-10-11 11:13 ` Jan Beulich
0 siblings, 1 reply; 37+ messages in thread
From: Julien Grall @ 2021-10-11 10:45 UTC (permalink / raw)
To: Jan Beulich, Penny Zheng, xen-devel, sstabellini
Cc: Bertrand.Marquis, Wei.Chen
Hi Jan,
On 28/09/2021 13:05, Jan Beulich wrote:
> On 23.09.2021 11:54, Julien Grall wrote:
>> On 23/09/2021 08:11, Penny Zheng wrote:
>>> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
>>>
>>> We are passing an extra special boolean flag at domain creation to
>>> specify whether we want to the domain to be privileged (i.e. dom0) or
>>> not. Another flag will be introduced later in this series.
>>>
>>> Reserve bits 16-31 from the existing flags bitfield in struct
>>> xen_domctl_createdomain for internal Xen usage.
>>
>> I am a bit split with this approach. This feels a bit of a hack to
>> reserve bits for internal purpose in external headers. But at the same
>> time I can see how this is easier to deal with it over repurposing the
>> last argument of domain_create().
>
> I actually have trouble seeing why that's easier. It is a common thing
> to widen a bool to "unsigned int flags" when more than one control is
> needed.
I was suggesting this is easier for the following two reasons:
1) All the option flags (internal and external) are in a single place.
2) Reduce the risk to make a mistake when widening the field. In
particular in the context of backporting. Although, this looks unlikely
here.
> Plus this makes things needlessly harder once (in the future)
> the low 16 bits are exhausted in the public interface.
That's why I suggested this sounds like a hack. At the same time the
split between external vs internal option is a bit more a pain for the
developper. So I didn't feel pushing for one vs the other. That said, I
will not argue against if you want to push for repurposing the last
argument.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 01/11] xen: reserve flags for internal usage in xen_domctl_createdomain
2021-10-11 10:45 ` Julien Grall
@ 2021-10-11 11:13 ` Jan Beulich
0 siblings, 0 replies; 37+ messages in thread
From: Jan Beulich @ 2021-10-11 11:13 UTC (permalink / raw)
To: Julien Grall, Penny Zheng
Cc: Bertrand.Marquis, Wei.Chen, xen-devel, sstabellini
On 11.10.2021 12:45, Julien Grall wrote:
> Hi Jan,
>
> On 28/09/2021 13:05, Jan Beulich wrote:
>> On 23.09.2021 11:54, Julien Grall wrote:
>>> On 23/09/2021 08:11, Penny Zheng wrote:
>>>> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
>>>>
>>>> We are passing an extra special boolean flag at domain creation to
>>>> specify whether we want to the domain to be privileged (i.e. dom0) or
>>>> not. Another flag will be introduced later in this series.
>>>>
>>>> Reserve bits 16-31 from the existing flags bitfield in struct
>>>> xen_domctl_createdomain for internal Xen usage.
>>>
>>> I am a bit split with this approach. This feels a bit of a hack to
>>> reserve bits for internal purpose in external headers. But at the same
>>> time I can see how this is easier to deal with it over repurposing the
>>> last argument of domain_create().
>>
>> I actually have trouble seeing why that's easier. It is a common thing
>> to widen a bool to "unsigned int flags" when more than one control is
>> needed.
>
> I was suggesting this is easier for the following two reasons:
> 1) All the option flags (internal and external) are in a single place.
> 2) Reduce the risk to make a mistake when widening the field. In
> particular in the context of backporting. Although, this looks unlikely
> here.
>
>> Plus this makes things needlessly harder once (in the future)
>> the low 16 bits are exhausted in the public interface.
>
> That's why I suggested this sounds like a hack. At the same time the
> split between external vs internal option is a bit more a pain for the
> developper. So I didn't feel pushing for one vs the other. That said, I
> will not argue against if you want to push for repurposing the last
> argument.
In which case - Penny, would you please change the patch accordingly?
Jan
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 02/11] xen/arm: introduce XEN_DOMCTL_INTERNAL_directmap
2021-09-23 3:11 [PATCH 00/11] 1:1 direct-map memory map Penny Zheng
2021-09-23 3:11 ` [PATCH 01/11] xen: reserve flags for internal usage in xen_domctl_createdomain Penny Zheng
@ 2021-09-23 3:11 ` Penny Zheng
2021-09-23 10:00 ` Julien Grall
2021-09-23 3:11 ` [PATCH 03/11] xen/arm: introduce 1:1 direct-map for domUs Penny Zheng
` (8 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Penny Zheng @ 2021-09-23 3:11 UTC (permalink / raw)
To: xen-devel, sstabellini, julien; +Cc: Bertrand.Marquis, Penny.Zheng, Wei.Chen
From: Stefano Stabellini <stefano.stabellini@xilinx.com>
This commit introduces a new ARM-specific flag to specify that the
domain should be 1:1 directly mapped (guest physical addresses ==
physical addresses).
Also, add a direct_map flag under struct arch_domain and use it to
implement is_domain_direct_mapped.
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
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>
---
xen/arch/arm/domain.c | 1 +
xen/arch/arm/domain_build.c | 2 +-
xen/include/asm-arm/domain.h | 9 +++++++--
xen/include/xen/domain.h | 4 ++++
4 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 7922249d26..0b3cff8a40 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -696,6 +696,7 @@ int arch_domain_create(struct domain *d,
return 0;
ASSERT(config != NULL);
+ d->arch.direct_map = config->flags & XEN_DOMCTL_INTERNAL_directmap;
#ifdef CONFIG_IOREQ_SERVER
ioreq_domain_init(d);
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 8cc4c800e9..21d8a559af 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2753,7 +2753,7 @@ void __init create_dom0(void)
struct domain *dom0;
struct xen_domctl_createdomain dom0_cfg = {
.flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap |
- XEN_DOMCTL_INTERNAL_ispriv,
+ XEN_DOMCTL_INTERNAL_ispriv | XEN_DOMCTL_INTERNAL_directmap,
.max_evtchn_port = -1,
.max_grant_frames = gnttab_dom0_frames(),
.max_maptrack_frames = -1,
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index c9277b5c6d..a74ee5720c 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -29,8 +29,11 @@ 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)
+/*
+ * The hardware domain has always its memory direct mapped. And DOM0 shall
+ * be always been set as 1:1 direct-map domain.
+ */
+#define is_domain_direct_mapped(d) (d)->arch.direct_map
struct vtimer {
struct vcpu *v;
@@ -89,6 +92,8 @@ struct arch_domain
#ifdef CONFIG_TEE
void *tee;
#endif
+
+ bool direct_map;
} __cacheline_aligned;
struct arch_vcpu
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 7ed0b62b78..6c2b07eb42 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -32,6 +32,10 @@ void arch_get_domain_info(const struct domain *d,
#define _XEN_DOMCTL_INTERNAL_ispriv 16
#define XEN_DOMCTL_INTERNAL_ispriv (1U<<_XEN_DOMCTL_INTERNAL_ispriv)
+/* This flag is ARM specific */
+#define _XEN_DOMCTL_INTERNAL_directmap 17
+#define XEN_DOMCTL_INTERNAL_directmap (1U<<_XEN_DOMCTL_INTERNAL_directmap)
+
/*
* Arch-specifics.
*/
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 02/11] xen/arm: introduce XEN_DOMCTL_INTERNAL_directmap
2021-09-23 3:11 ` [PATCH 02/11] xen/arm: introduce XEN_DOMCTL_INTERNAL_directmap Penny Zheng
@ 2021-09-23 10:00 ` Julien Grall
0 siblings, 0 replies; 37+ messages in thread
From: Julien Grall @ 2021-09-23 10:00 UTC (permalink / raw)
To: Penny Zheng, xen-devel, sstabellini; +Cc: Bertrand.Marquis, Wei.Chen
Hi,
On 23/09/2021 08:11, Penny Zheng wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
>
> This commit introduces a new ARM-specific flag to specify that the
> domain should be 1:1 directly mapped (guest physical addresses ==
"1:1" and "directly" means the exactly the same. Please remove either.
> physical addresses).
>
> Also, add a direct_map flag under struct arch_domain and use it to
> implement is_domain_direct_mapped.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> 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>
> ---
> xen/arch/arm/domain.c | 1 +
> xen/arch/arm/domain_build.c | 2 +-
> xen/include/asm-arm/domain.h | 9 +++++++--
> xen/include/xen/domain.h | 4 ++++
> 4 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 7922249d26..0b3cff8a40 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -696,6 +696,7 @@ int arch_domain_create(struct domain *d,
> return 0;
>
> ASSERT(config != NULL);
> + d->arch.direct_map = config->flags & XEN_DOMCTL_INTERNAL_directmap;
>
> #ifdef CONFIG_IOREQ_SERVER
> ioreq_domain_init(d);
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 8cc4c800e9..21d8a559af 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2753,7 +2753,7 @@ void __init create_dom0(void)
> struct domain *dom0;
> struct xen_domctl_createdomain dom0_cfg = {
> .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap |
> - XEN_DOMCTL_INTERNAL_ispriv,
> + XEN_DOMCTL_INTERNAL_ispriv | XEN_DOMCTL_INTERNAL_directmap,
> .max_evtchn_port = -1,
> .max_grant_frames = gnttab_dom0_frames(),
> .max_maptrack_frames = -1,
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index c9277b5c6d..a74ee5720c 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -29,8 +29,11 @@ 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)
> +/*
> + * The hardware domain has always its memory direct mapped. And DOM0 shall
> + * be always been set as 1:1 direct-map domain.
> + */
I think this comment should be moved on top of dom0_cfg.flags. This will
prevent in stall comment if in the future we decide to remove the direct
map (I know that cache coloring will want to drop it).
> +#define is_domain_direct_mapped(d) (d)->arch.direct_map
>
> struct vtimer {
> struct vcpu *v;
> @@ -89,6 +92,8 @@ struct arch_domain
> #ifdef CONFIG_TEE
> void *tee;
> #endif
> +
> + bool direct_map;
We already store the flag in d->options. So this is a bit redundant.
> } __cacheline_aligned;
>
> struct arch_vcpu
> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> index 7ed0b62b78..6c2b07eb42 100644
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -32,6 +32,10 @@ void arch_get_domain_info(const struct domain *d,
> #define _XEN_DOMCTL_INTERNAL_ispriv 16
> #define XEN_DOMCTL_INTERNAL_ispriv (1U<<_XEN_DOMCTL_INTERNAL_ispriv)
>
> +/* This flag is ARM specific */
> +#define _XEN_DOMCTL_INTERNAL_directmap 17
> +#define XEN_DOMCTL_INTERNAL_directmap (1U<<_XEN_DOMCTL_INTERNAL_directmap)
Coding style: space before and after <<.
> +
> /*
> * Arch-specifics.
> */
>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 03/11] xen/arm: introduce 1:1 direct-map for domUs
2021-09-23 3:11 [PATCH 00/11] 1:1 direct-map memory map Penny Zheng
2021-09-23 3:11 ` [PATCH 01/11] xen: reserve flags for internal usage in xen_domctl_createdomain Penny Zheng
2021-09-23 3:11 ` [PATCH 02/11] xen/arm: introduce XEN_DOMCTL_INTERNAL_directmap Penny Zheng
@ 2021-09-23 3:11 ` Penny Zheng
2021-09-23 10:36 ` Julien Grall
2021-09-23 3:11 ` [PATCH 04/11] xen/arm: introduce accessors for vgic dist, cpu, and rdist base addresses Penny Zheng
` (7 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Penny Zheng @ 2021-09-23 3:11 UTC (permalink / raw)
To: xen-devel, sstabellini, julien; +Cc: Bertrand.Marquis, Penny.Zheng, Wei.Chen
From: Stefano Stabellini <stefano.stabellini@xilinx.com>
Cases where domU needs 1:1 direct-map memory map:
* IOMMU not present in the system.
* IOMMU disabled if it doesn't cover a specific device and all the guests
are trusted. Thinking a mixed scenario, where a few devices with IOMMU and
a few without, then guest DMA security still could not be totally guaranteed.
So users may want to disable the IOMMU, to at least gain some performance
improvement from IOMMU disabled.
* IOMMU disabled as a workaround when it doesn't have enough bandwidth.
To be specific, in a few extreme situation, when multiple devices do DMA
concurrently, these requests may exceed IOMMU's transmission capacity.
* IOMMU disabled when it adds too much latency on DMA. For example,
TLB may be missing in some IOMMU hardware, which may bring latency in DMA
progress, so users may want to disable it in some realtime scenario.
*WARNING:
Users should be aware that it is not always secure to assign a device without
IOMMU protection.
When the device is not protected by the IOMMU, the administrator should make
sure that:
1. The device is assigned to a trusted guest.
2. Users have additional security mechanism on the platform.
Given that with direct-map, the IOMMU could be used but it is not required.
This commit avoids setting XEN_DOMCTL_CDF_iommu when the IOMMU is disabled and
direct_map is requested.
Since, for now, 1:1 direct-map is only supported when domain on Static
Allocation, that is, "xen.static-mem" is defined in the domain configuration.
This commit also re-implements allocate_static_memory to allocate static memory
as guest RAM for 1:1 direct-map domain.
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
docs/misc/arm/device-tree/booting.txt | 9 ++
xen/arch/arm/domain_build.c | 117 +++++++++++++++++---------
2 files changed, 85 insertions(+), 41 deletions(-)
diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 44cd9e1a9a..3d637c747e 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -164,6 +164,15 @@ 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
+
+ Optional for Domain on Static Allocation.
+ An empty property to request the memory of the domain to be 1:1
+ direct-map(guest physical address == physical address).
+ WARNING: Users must be aware of this risk, that guests having access
+ to hardware with DMA capacity must be trusted, or it could use the
+ DMA engine to access any other memory area.
+
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_build.c b/xen/arch/arm/domain_build.c
index 21d8a559af..213ad017dc 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -488,8 +488,14 @@ static bool __init append_static_memory_to_bank(struct domain *d,
{
int res;
unsigned int nr_pages = PFN_DOWN(size);
- /* Infer next GFN. */
- gfn_t sgfn = gaddr_to_gfn(bank->start + bank->size);
+ gfn_t sgfn;
+
+ if ( !is_domain_direct_mapped(d) )
+ /* Infer next GFN when GFN != MFN. */
+ sgfn = gaddr_to_gfn(bank->start + bank->size);
+ else
+ sgfn = gaddr_to_gfn(mfn_to_maddr(smfn));
+
res = guest_physmap_add_pages(d, sgfn, smfn, nr_pages);
if ( res )
@@ -537,14 +543,17 @@ static void __init allocate_static_memory(struct domain *d,
}
reg_cells = addr_cells + size_cells;
- /*
- * The static memory will be mapped in the guest at the usual guest memory
- * addresses (GUEST_RAM0_BASE, GUEST_RAM1_BASE) defined by
- * xen/include/public/arch-arm.h.
- */
- gbank = 0;
- gsize = ramsize[gbank];
- kinfo->mem.bank[gbank].start = rambase[gbank];
+ if ( !is_domain_direct_mapped(d) )
+ {
+ /*
+ * The static memory will be mapped in the guest at the usual guest
+ * memory addresses (GUEST_RAM0_BASE, GUEST_RAM1_BASE) defined by
+ * xen/include/public/arch-arm.h.
+ */
+ gbank = 0;
+ gsize = ramsize[gbank];
+ kinfo->mem.bank[gbank].start = rambase[gbank];
+ }
cell = (const __be32 *)prop->value;
nr_banks = (prop->length) / (reg_cells * sizeof (u32));
@@ -572,42 +581,58 @@ static void __init allocate_static_memory(struct domain *d,
printk(XENLOG_INFO "%pd: STATIC BANK[%u] %#"PRIpaddr"-%#"PRIpaddr"\n",
d, bank, pbase, pbase + psize);
- while ( 1 )
+ if ( !is_domain_direct_mapped(d) )
{
- /* Map as much as possible the static range to the guest bank */
- if ( !append_static_memory_to_bank(d, &kinfo->mem.bank[gbank], smfn,
- min(psize, gsize)) )
- goto fail;
-
- /*
- * The current physical bank is fully mapped.
- * Handle the next physical bank.
- */
- if ( gsize >= psize )
+ while ( 1 )
{
- gsize = gsize - psize;
- break;
+ /* Map as much as possible the static range to the guest bank */
+ if ( !append_static_memory_to_bank(d, &kinfo->mem.bank[gbank],
+ smfn, min(psize, gsize)) )
+ goto fail;
+
+ /*
+ * The current physical bank is fully mapped.
+ * Handle the next physical bank.
+ */
+ if ( gsize >= psize )
+ {
+ gsize = gsize - psize;
+ break;
+ }
+ /*
+ * When current guest bank is not enough to map, exhaust
+ * the current one and seek to the next.
+ * Before seeking to the next, check if we still have available
+ * guest bank.
+ */
+ else if ( (gbank + 1) >= GUEST_RAM_BANKS )
+ {
+ printk(XENLOG_ERR "Exhausted all possible guest banks.\n");
+ goto fail;
+ }
+ else
+ {
+ psize = psize - gsize;
+ smfn = mfn_add(smfn, gsize >> PAGE_SHIFT);
+ /* Update to the next guest bank. */
+ gbank++;
+ gsize = ramsize[gbank];
+ kinfo->mem.bank[gbank].start = rambase[gbank];
+ }
}
+ }
+ else /* 1:1 direct-map. */
+ {
/*
- * When current guest bank is not enough to map, exhaust
- * the current one and seek to the next.
- * Before seeking to the next, check if we still have available
- * guest bank.
+ * One guest memory bank is matched with one physical
+ * memory bank.
*/
- else if ( (gbank + 1) >= GUEST_RAM_BANKS )
- {
- printk(XENLOG_ERR "Exhausted all possible guest banks.\n");
+ gbank = bank;
+ kinfo->mem.bank[gbank].start = pbase;
+
+ if ( !append_static_memory_to_bank(d, &kinfo->mem.bank[gbank],
+ smfn, psize) )
goto fail;
- }
- else
- {
- psize = psize - gsize;
- smfn = mfn_add(smfn, gsize >> PAGE_SHIFT);
- /* Update to the next guest bank. */
- gbank++;
- gsize = ramsize[gbank];
- kinfo->mem.bank[gbank].start = rambase[gbank];
- }
}
tot_size += psize;
@@ -2638,6 +2663,7 @@ void __init create_domUs(void)
{
struct dt_device_node *node;
const struct dt_device_node *chosen = dt_find_node_by_path("/chosen");
+ bool direct_map = false;
BUG_ON(chosen == NULL);
dt_for_each_child_node(chosen, node)
@@ -2658,8 +2684,17 @@ void __init create_domUs(void)
panic("Missing property 'cpus' for domain %s\n",
dt_node_name(node));
+ direct_map = dt_property_read_bool(node, "direct-map");
+ d_cfg.flags |= direct_map ? XEN_DOMCTL_INTERNAL_directmap : 0;
if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") )
- d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
+ {
+ if ( iommu_enabled )
+ d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
+ else if ( !direct_map )
+ panic("Assign a device but IOMMU and direct-map are all disabled\n");
+ else
+ warning_add("Please be sure of having trusted guests, when doing device assignment without IOMMU protection\n");
+ }
if ( !dt_property_read_u32(node, "nr_spis", &d_cfg.arch.nr_spis) )
{
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 03/11] xen/arm: introduce 1:1 direct-map for domUs
2021-09-23 3:11 ` [PATCH 03/11] xen/arm: introduce 1:1 direct-map for domUs Penny Zheng
@ 2021-09-23 10:36 ` Julien Grall
2021-10-08 2:19 ` Penny Zheng
0 siblings, 1 reply; 37+ messages in thread
From: Julien Grall @ 2021-09-23 10:36 UTC (permalink / raw)
To: Penny Zheng, xen-devel, sstabellini; +Cc: Bertrand.Marquis, Wei.Chen
On 23/09/2021 08:11, Penny Zheng wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
>
> Cases where domU needs 1:1 direct-map memory map:
"1:1" and "direct-map" means pretty much the same. Given that the
property is name "direct-map", then I would drop "1:1".
> * IOMMU not present in the system.
> * IOMMU disabled if it doesn't cover a specific device and all the guests
> are trusted. Thinking a mixed scenario, where a few devices with IOMMU and
> a few without, then guest DMA security still could not be totally guaranteed.
> So users may want to disable the IOMMU, to at least gain some performance
> improvement from IOMMU disabled.
> * IOMMU disabled as a workaround when it doesn't have enough bandwidth.
> To be specific, in a few extreme situation, when multiple devices do DMA
> concurrently, these requests may exceed IOMMU's transmission capacity.
> * IOMMU disabled when it adds too much latency on DMA. For example,
> TLB may be missing in some IOMMU hardware, which may bring latency in DMA
> progress, so users may want to disable it in some realtime scenario.
>
> *WARNING:
> Users should be aware that it is not always secure to assign a device without
> IOMMU protection.
> When the device is not protected by the IOMMU, the administrator should make
> sure that:
> 1. The device is assigned to a trusted guest.
> 2. Users have additional security mechanism on the platform.
The two requirements are only necessary for device that are DMA-capable.
For device that can't do DMA, it will likely be fine to assign to
non-trusted guest.
>
> Given that with direct-map, the IOMMU could be used but it is not required.
I can't parse it.
> This commit avoids setting XEN_DOMCTL_CDF_iommu when the IOMMU is disabled and
> direct_map is requested.
>
> Since, for now, 1:1 direct-map is only supported when domain on Static
I think "Since" seems unnecessary.
> Allocation, that is, "xen.static-mem" is defined in the domain configuration.
>
> This commit also re-implements allocate_static_memory to allocate static memory
> as guest RAM for 1:1 direct-map domain.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> docs/misc/arm/device-tree/booting.txt | 9 ++
> xen/arch/arm/domain_build.c | 117 +++++++++++++++++---------
> 2 files changed, 85 insertions(+), 41 deletions(-)
>
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index 44cd9e1a9a..3d637c747e 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -164,6 +164,15 @@ 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
> +
> + Optional for Domain on Static Allocation.
> + An empty property to request the memory of the domain to be 1:1
> + direct-map(guest physical address == physical address).
> + WARNING: Users must be aware of this risk, that guests having access
> + to hardware with DMA capacity must be trusted, or it could use the
> + DMA engine to access any other memory area.
The WARNING is only applicable if the device is not protected by an
IOMMU. So this should be clarified because one may want still want to
use the direct-map (e.g. because the OS relies on the host layout) and
have IOMMU enabled.
> +
> 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_build.c b/xen/arch/arm/domain_build.c
> index 21d8a559af..213ad017dc 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -488,8 +488,14 @@ static bool __init append_static_memory_to_bank(struct domain *d,
> {
> int res;
> unsigned int nr_pages = PFN_DOWN(size);
> - /* Infer next GFN. */
> - gfn_t sgfn = gaddr_to_gfn(bank->start + bank->size);
> + gfn_t sgfn;
> +
> + if ( !is_domain_direct_mapped(d) )
> + /* Infer next GFN when GFN != MFN. */
> + sgfn = gaddr_to_gfn(bank->start + bank->size);
> + else
> + sgfn = gaddr_to_gfn(mfn_to_maddr(smfn));
> +
>
> res = guest_physmap_add_pages(d, sgfn, smfn, nr_pages);
> if ( res )
> @@ -537,14 +543,17 @@ static void __init allocate_static_memory(struct domain *d,
> }
This function was already pretty difficult to read. So I would rather
not add more complexity in it. Instead, I would look to split the common
code in a separate helper or possibly duplicate it.
> reg_cells = addr_cells + size_cells;
>
> - /*
> - * The static memory will be mapped in the guest at the usual guest memory
> - * addresses (GUEST_RAM0_BASE, GUEST_RAM1_BASE) defined by
> - * xen/include/public/arch-arm.h.
> - */
> - gbank = 0;
> - gsize = ramsize[gbank];
> - kinfo->mem.bank[gbank].start = rambase[gbank];
> + if ( !is_domain_direct_mapped(d) )
> + {
> + /*
> + * The static memory will be mapped in the guest at the usual guest
> + * memory addresses (GUEST_RAM0_BASE, GUEST_RAM1_BASE) defined by
> + * xen/include/public/arch-arm.h.
> + */
> + gbank = 0;
> + gsize = ramsize[gbank];
> + kinfo->mem.bank[gbank].start = rambase[gbank];
> + } >
> cell = (const __be32 *)prop->value;
> nr_banks = (prop->length) / (reg_cells * sizeof (u32));
> @@ -572,42 +581,58 @@ static void __init allocate_static_memory(struct domain *d,
> printk(XENLOG_INFO "%pd: STATIC BANK[%u] %#"PRIpaddr"-%#"PRIpaddr"\n",
> d, bank, pbase, pbase + psize);
>
> - while ( 1 )
> + if ( !is_domain_direct_mapped(d) )
> {
> - /* Map as much as possible the static range to the guest bank */
> - if ( !append_static_memory_to_bank(d, &kinfo->mem.bank[gbank], smfn,
> - min(psize, gsize)) )
> - goto fail;
> -
> - /*
> - * The current physical bank is fully mapped.
> - * Handle the next physical bank.
> - */
> - if ( gsize >= psize )
> + while ( 1 )
> {
> - gsize = gsize - psize;
> - break;
> + /* Map as much as possible the static range to the guest bank */
> + if ( !append_static_memory_to_bank(d, &kinfo->mem.bank[gbank],
> + smfn, min(psize, gsize)) )
> + goto fail;
> +
> + /*
> + * The current physical bank is fully mapped.
> + * Handle the next physical bank.
> + */
> + if ( gsize >= psize )
> + {
> + gsize = gsize - psize;
> + break;
> + }
> + /*
> + * When current guest bank is not enough to map, exhaust
> + * the current one and seek to the next.
> + * Before seeking to the next, check if we still have available
> + * guest bank.
> + */
> + else if ( (gbank + 1) >= GUEST_RAM_BANKS )
> + {
> + printk(XENLOG_ERR "Exhausted all possible guest banks.\n");
> + goto fail;
> + }
> + else
> + {
> + psize = psize - gsize;
> + smfn = mfn_add(smfn, gsize >> PAGE_SHIFT);
> + /* Update to the next guest bank. */
> + gbank++;
> + gsize = ramsize[gbank];
> + kinfo->mem.bank[gbank].start = rambase[gbank];
> + }
> }
> + }
> + else /* 1:1 direct-map. */
> + {
> /*
> - * When current guest bank is not enough to map, exhaust
> - * the current one and seek to the next.
> - * Before seeking to the next, check if we still have available
> - * guest bank.
> + * One guest memory bank is matched with one physical
> + * memory bank.
> */
> - else if ( (gbank + 1) >= GUEST_RAM_BANKS )
> - {
> - printk(XENLOG_ERR "Exhausted all possible guest banks.\n");
> + gbank = bank;
> + kinfo->mem.bank[gbank].start = pbase;
> +
> + if ( !append_static_memory_to_bank(d, &kinfo->mem.bank[gbank],
> + smfn, psize) )
> goto fail;
> - }
> - else
> - {
> - psize = psize - gsize;
> - smfn = mfn_add(smfn, gsize >> PAGE_SHIFT);
> - /* Update to the next guest bank. */
> - gbank++;
> - gsize = ramsize[gbank];
> - kinfo->mem.bank[gbank].start = rambase[gbank];
> - }
> }
>
> tot_size += psize;
> @@ -2638,6 +2663,7 @@ void __init create_domUs(void)
> {
> struct dt_device_node *node;
> const struct dt_device_node *chosen = dt_find_node_by_path("/chosen");
> + bool direct_map = false;
This is a bit redundant for just a couple of use. Instead, you could
directly use d_cfg.flags & XEN_DOMCTL_INTERNAL_directmap.
>
> BUG_ON(chosen == NULL);
> dt_for_each_child_node(chosen, node)
> @@ -2658,8 +2684,17 @@ void __init create_domUs(void)
> panic("Missing property 'cpus' for domain %s\n",
> dt_node_name(node));
>
> + direct_map = dt_property_read_bool(node, "direct-map");
> + d_cfg.flags |= direct_map ? XEN_DOMCTL_INTERNAL_directmap : 0;
> if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") )
> - d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
> + {
> + if ( iommu_enabled )
> + d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
> + else if ( !direct_map )
> + panic("Assign a device but IOMMU and direct-map are all disabled\n");
> + else
> + warning_add("Please be sure of having trusted guests, when doing device assignment without IOMMU protection\n");
> + }
>
> if ( !dt_property_read_u32(node, "nr_spis", &d_cfg.arch.nr_spis) )
> {
>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 37+ messages in thread* RE: [PATCH 03/11] xen/arm: introduce 1:1 direct-map for domUs
2021-09-23 10:36 ` Julien Grall
@ 2021-10-08 2:19 ` Penny Zheng
0 siblings, 0 replies; 37+ messages in thread
From: Penny Zheng @ 2021-10-08 2:19 UTC (permalink / raw)
To: Julien Grall, xen-devel, sstabellini; +Cc: Bertrand Marquis, Wei Chen
Hi Julien
So sorry for taking so long to respond. Just being back from the long National Day holidays. 😉
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Thursday, September 23, 2021 6:36 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>
> Subject: Re: [PATCH 03/11] xen/arm: introduce 1:1 direct-map for domUs
>
>
>
> On 23/09/2021 08:11, Penny Zheng wrote:
> > From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> >
> > Cases where domU needs 1:1 direct-map memory map:
>
> "1:1" and "direct-map" means pretty much the same. Given that the property
> is name "direct-map", then I would drop "1:1".
>
> > * IOMMU not present in the system.
> > * IOMMU disabled if it doesn't cover a specific device and all the
> > guests are trusted. Thinking a mixed scenario, where a few devices
> > with IOMMU and a few without, then guest DMA security still could not be
> totally guaranteed.
> > So users may want to disable the IOMMU, to at least gain some
> > performance improvement from IOMMU disabled.
> > * IOMMU disabled as a workaround when it doesn't have enough
> bandwidth.
> > To be specific, in a few extreme situation, when multiple devices do
> > DMA concurrently, these requests may exceed IOMMU's transmission
> capacity.
> > * IOMMU disabled when it adds too much latency on DMA. For example,
> > TLB may be missing in some IOMMU hardware, which may bring latency in
> > DMA progress, so users may want to disable it in some realtime scenario.
> >
> > *WARNING:
> > Users should be aware that it is not always secure to assign a device
> > without IOMMU protection.
> > When the device is not protected by the IOMMU, the administrator
> > should make sure that:
> > 1. The device is assigned to a trusted guest.
> > 2. Users have additional security mechanism on the platform.
>
> The two requirements are only necessary for device that are DMA-capable.
> For device that can't do DMA, it will likely be fine to assign to non-trusted
> guest.
>
> >
> > Given that with direct-map, the IOMMU could be used but it is not required.
>
> I can't parse it.
>
Now when doing device assignments, IOMMU is forced to be enabled. And since
we are introducing direct-map here, I think that maybe even if IOMMU is missing/disabled,
direct-map on trust guests could also make it work.
Maybe I should rephrase it to
"
When doing device assignments and IOMMU is missing or disabled, direct-map
shall be used on trust guests.
"
> > This commit avoids setting XEN_DOMCTL_CDF_iommu when the IOMMU is
> > disabled and direct_map is requested.
> >
> > Since, for now, 1:1 direct-map is only supported when domain on Static
>
> I think "Since" seems unnecessary.
>
> > Allocation, that is, "xen.static-mem" is defined in the domain configuration.
> >
> > This commit also re-implements allocate_static_memory to allocate
> > static memory as guest RAM for 1:1 direct-map domain.
> >
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> > docs/misc/arm/device-tree/booting.txt | 9 ++
> > xen/arch/arm/domain_build.c | 117 +++++++++++++++++---------
> > 2 files changed, 85 insertions(+), 41 deletions(-)
> >
> > diff --git a/docs/misc/arm/device-tree/booting.txt
> > b/docs/misc/arm/device-tree/booting.txt
> > index 44cd9e1a9a..3d637c747e 100644
> > --- a/docs/misc/arm/device-tree/booting.txt
> > +++ b/docs/misc/arm/device-tree/booting.txt
> > @@ -164,6 +164,15 @@ 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
> > +
> > + Optional for Domain on Static Allocation.
> > + An empty property to request the memory of the domain to be 1:1
> > + direct-map(guest physical address == physical address).
> > + WARNING: Users must be aware of this risk, that guests having access
> > + to hardware with DMA capacity must be trusted, or it could use the
> > + DMA engine to access any other memory area.
>
> The WARNING is only applicable if the device is not protected by an IOMMU.
> So this should be clarified because one may want still want to use the direct-
> map (e.g. because the OS relies on the host layout) and have IOMMU enabled.
>
> > +
> > 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_build.c b/xen/arch/arm/domain_build.c
> > index 21d8a559af..213ad017dc 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -488,8 +488,14 @@ static bool __init
> append_static_memory_to_bank(struct domain *d,
> > {
> > int res;
> > unsigned int nr_pages = PFN_DOWN(size);
> > - /* Infer next GFN. */
> > - gfn_t sgfn = gaddr_to_gfn(bank->start + bank->size);
> > + gfn_t sgfn;
> > +
> > + if ( !is_domain_direct_mapped(d) )
> > + /* Infer next GFN when GFN != MFN. */
> > + sgfn = gaddr_to_gfn(bank->start + bank->size);
> > + else
> > + sgfn = gaddr_to_gfn(mfn_to_maddr(smfn));
> > +
> >
> > res = guest_physmap_add_pages(d, sgfn, smfn, nr_pages);
> > if ( res )
> > @@ -537,14 +543,17 @@ static void __init allocate_static_memory(struct
> domain *d,
> > }
>
> This function was already pretty difficult to read. So I would rather not add
> more complexity in it. Instead, I would look to split the common code in a
> separate helper or possibly duplicate it.
>
> > reg_cells = addr_cells + size_cells;
> >
> > - /*
> > - * The static memory will be mapped in the guest at the usual guest
> memory
> > - * addresses (GUEST_RAM0_BASE, GUEST_RAM1_BASE) defined by
> > - * xen/include/public/arch-arm.h.
> > - */
> > - gbank = 0;
> > - gsize = ramsize[gbank];
> > - kinfo->mem.bank[gbank].start = rambase[gbank];
> > + if ( !is_domain_direct_mapped(d) )
> > + {
> > + /*
> > + * The static memory will be mapped in the guest at the usual guest
> > + * memory addresses (GUEST_RAM0_BASE, GUEST_RAM1_BASE)
> defined by
> > + * xen/include/public/arch-arm.h.
> > + */
> > + gbank = 0;
> > + gsize = ramsize[gbank];
> > + kinfo->mem.bank[gbank].start = rambase[gbank];
> > + } >
> > cell = (const __be32 *)prop->value;
> > nr_banks = (prop->length) / (reg_cells * sizeof (u32)); @@
> > -572,42 +581,58 @@ static void __init allocate_static_memory(struct
> domain *d,
> > printk(XENLOG_INFO "%pd: STATIC BANK[%u] %#"PRIpaddr"-
> %#"PRIpaddr"\n",
> > d, bank, pbase, pbase + psize);
> >
> > - while ( 1 )
> > + if ( !is_domain_direct_mapped(d) )
> > {
> > - /* Map as much as possible the static range to the guest bank */
> > - if ( !append_static_memory_to_bank(d, &kinfo->mem.bank[gbank],
> smfn,
> > - min(psize, gsize)) )
> > - goto fail;
> > -
> > - /*
> > - * The current physical bank is fully mapped.
> > - * Handle the next physical bank.
> > - */
> > - if ( gsize >= psize )
> > + while ( 1 )
> > {
> > - gsize = gsize - psize;
> > - break;
> > + /* Map as much as possible the static range to the guest bank */
> > + if ( !append_static_memory_to_bank(d, &kinfo->mem.bank[gbank],
> > + smfn, min(psize, gsize)) )
> > + goto fail;
> > +
> > + /*
> > + * The current physical bank is fully mapped.
> > + * Handle the next physical bank.
> > + */
> > + if ( gsize >= psize )
> > + {
> > + gsize = gsize - psize;
> > + break;
> > + }
> > + /*
> > + * When current guest bank is not enough to map, exhaust
> > + * the current one and seek to the next.
> > + * Before seeking to the next, check if we still have available
> > + * guest bank.
> > + */
> > + else if ( (gbank + 1) >= GUEST_RAM_BANKS )
> > + {
> > + printk(XENLOG_ERR "Exhausted all possible guest banks.\n");
> > + goto fail;
> > + }
> > + else
> > + {
> > + psize = psize - gsize;
> > + smfn = mfn_add(smfn, gsize >> PAGE_SHIFT);
> > + /* Update to the next guest bank. */
> > + gbank++;
> > + gsize = ramsize[gbank];
> > + kinfo->mem.bank[gbank].start = rambase[gbank];
> > + }
> > }
> > + }
> > + else /* 1:1 direct-map. */
> > + {
> > /*
> > - * When current guest bank is not enough to map, exhaust
> > - * the current one and seek to the next.
> > - * Before seeking to the next, check if we still have available
> > - * guest bank.
> > + * One guest memory bank is matched with one physical
> > + * memory bank.
> > */
> > - else if ( (gbank + 1) >= GUEST_RAM_BANKS )
> > - {
> > - printk(XENLOG_ERR "Exhausted all possible guest banks.\n");
> > + gbank = bank;
> > + kinfo->mem.bank[gbank].start = pbase;
> > +
> > + if ( !append_static_memory_to_bank(d, &kinfo->mem.bank[gbank],
> > + smfn, psize) )
> > goto fail;
> > - }
> > - else
> > - {
> > - psize = psize - gsize;
> > - smfn = mfn_add(smfn, gsize >> PAGE_SHIFT);
> > - /* Update to the next guest bank. */
> > - gbank++;
> > - gsize = ramsize[gbank];
> > - kinfo->mem.bank[gbank].start = rambase[gbank];
> > - }
> > }
> >
> > tot_size += psize;
> > @@ -2638,6 +2663,7 @@ void __init create_domUs(void)
> > {
> > struct dt_device_node *node;
> > const struct dt_device_node *chosen =
> > dt_find_node_by_path("/chosen");
> > + bool direct_map = false;
>
> This is a bit redundant for just a couple of use. Instead, you could directly use
> d_cfg.flags & XEN_DOMCTL_INTERNAL_directmap.
>
> >
> > BUG_ON(chosen == NULL);
> > dt_for_each_child_node(chosen, node) @@ -2658,8 +2684,17 @@ void
> > __init create_domUs(void)
> > panic("Missing property 'cpus' for domain %s\n",
> > dt_node_name(node));
> >
> > + direct_map = dt_property_read_bool(node, "direct-map");
> > + d_cfg.flags |= direct_map ? XEN_DOMCTL_INTERNAL_directmap :
> > + 0;
> > if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") )
> > - d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
> > + {
> > + if ( iommu_enabled )
> > + d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
> > + else if ( !direct_map )
> > + panic("Assign a device but IOMMU and direct-map are all
> disabled\n");
> > + else
> > + warning_add("Please be sure of having trusted guests, when doing
> device assignment without IOMMU protection\n");
> > + }
> >
> > if ( !dt_property_read_u32(node, "nr_spis", &d_cfg.arch.nr_spis) )
> > {
> >
>
> Cheers,
>
> --
> Julien Grall
Cheers,
--
Penny Zheng
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 04/11] xen/arm: introduce accessors for vgic dist, cpu, and rdist base addresses
2021-09-23 3:11 [PATCH 00/11] 1:1 direct-map memory map Penny Zheng
` (2 preceding siblings ...)
2021-09-23 3:11 ` [PATCH 03/11] xen/arm: introduce 1:1 direct-map for domUs Penny Zheng
@ 2021-09-23 3:11 ` Penny Zheng
2021-09-23 10:45 ` Julien Grall
2021-09-23 3:11 ` [PATCH 05/11] xen/arm: vgic: introduce vgic.cbase Penny Zheng
` (6 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Penny Zheng @ 2021-09-23 3:11 UTC (permalink / raw)
To: xen-devel, sstabellini, julien; +Cc: Bertrand.Marquis, Penny.Zheng, Wei.Chen
From: Stefano Stabellini <stefano.stabellini@xilinx.com>
Introduce accessors for vgic dist, cpu, and rdist base addresses, on
all gic types.
Use the accessors when making gic node for device tree of domU.
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
xen/arch/arm/domain_build.c | 21 ++++++++++++-----
xen/include/asm-arm/new_vgic.h | 24 ++++++++++++++++++++
xen/include/asm-arm/vgic.h | 41 ++++++++++++++++++++++++++++++++++
3 files changed, 80 insertions(+), 6 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 213ad017dc..d5f201f73e 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1802,8 +1802,12 @@ static int __init make_gicv2_domU_node(struct kernel_info *kinfo)
int res = 0;
__be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * 2];
__be32 *cells;
+ struct domain *d = kinfo->d;
+ char buf[38];
- res = fdt_begin_node(fdt, "interrupt-controller@"__stringify(GUEST_GICD_BASE));
+ snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64,
+ vgic_dist_base(&d->arch.vgic));
+ res = fdt_begin_node(fdt, buf);
if ( res )
return res;
@@ -1825,9 +1829,9 @@ static int __init make_gicv2_domU_node(struct kernel_info *kinfo)
cells = ®[0];
dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
- GUEST_GICD_BASE, GUEST_GICD_SIZE);
+ vgic_dist_base(&d->arch.vgic), GUEST_GICD_SIZE);
dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
- GUEST_GICC_BASE, GUEST_GICC_SIZE);
+ vgic_cpu_base(&d->arch.vgic), GUEST_GICC_SIZE);
res = fdt_property(fdt, "reg", reg, sizeof(reg));
if (res)
@@ -1852,8 +1856,12 @@ static int __init make_gicv3_domU_node(struct kernel_info *kinfo)
int res = 0;
__be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * 2];
__be32 *cells;
+ struct domain *d = kinfo->d;
+ char buf[38];
- res = fdt_begin_node(fdt, "interrupt-controller@"__stringify(GUEST_GICV3_GICD_BASE));
+ snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64,
+ vgic_dist_base(&d->arch.vgic));
+ res = fdt_begin_node(fdt, buf);
if ( res )
return res;
@@ -1875,9 +1883,10 @@ static int __init make_gicv3_domU_node(struct kernel_info *kinfo)
cells = ®[0];
dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
- GUEST_GICV3_GICD_BASE, GUEST_GICV3_GICD_SIZE);
+ vgic_dist_base(&d->arch.vgic), GUEST_GICV3_GICD_SIZE);
dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
- GUEST_GICV3_GICR0_BASE, GUEST_GICV3_GICR0_SIZE);
+ vgic_rdist_base(&d->arch.vgic, 0),
+ vgic_rdist_size(&d->arch.vgic, 0));
res = fdt_property(fdt, "reg", reg, sizeof(reg));
if (res)
diff --git a/xen/include/asm-arm/new_vgic.h b/xen/include/asm-arm/new_vgic.h
index 97d622bff6..9097522b27 100644
--- a/xen/include/asm-arm/new_vgic.h
+++ b/xen/include/asm-arm/new_vgic.h
@@ -186,6 +186,30 @@ struct vgic_cpu {
uint32_t num_id_bits;
};
+static inline paddr_t vgic_cpu_base(struct vgic_dist *vgic)
+{
+ return GUEST_GICC_BASE;
+}
+
+static inline paddr_t vgic_dist_base(struct vgic_dist *vgic)
+{
+ return GUEST_GICD_BASE;
+}
+
+static inline unsigned int vgic_rdist_nr(struct vgic_dist *vgic)
+{
+ return 0;
+}
+
+static inline paddr_t vgic_rdist_base(struct vgic_dist *vgic, unsigned int i)
+{
+ return INVALID_PADDR;
+}
+
+static inline paddr_t vgic_rdist_size(struct vgic_dist *vgic, unsigned int i)
+{
+ return 0;
+}
#endif /* __ASM_ARM_NEW_VGIC_H */
/*
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 62c2ae538d..e1bc5113da 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -277,6 +277,47 @@ enum gic_sgi_mode;
*/
#define REG_RANK_INDEX(b, n, s) ((((n) >> s) & ((b)-1)) % 32)
+static inline paddr_t vgic_cpu_base(struct vgic_dist *vgic)
+{
+ return GUEST_GICC_BASE;
+}
+
+static inline paddr_t vgic_dist_base(struct vgic_dist *vgic)
+{
+ return GUEST_GICD_BASE;
+}
+
+#ifdef CONFIG_GICV3
+static inline unsigned int vgic_rdist_nr(struct vgic_dist *vgic)
+{
+ return GUEST_GICV3_RDIST_REGIONS;
+}
+
+static inline paddr_t vgic_rdist_base(struct vgic_dist *vgic, unsigned int i)
+{
+ return GUEST_GICV3_GICR0_BASE;
+}
+
+static inline paddr_t vgic_rdist_size(struct vgic_dist *vgic, unsigned int i)
+{
+ return GUEST_GICV3_GICR0_SIZE;
+}
+#else
+static inline unsigned int vgic_rdist_nr(struct vgic_dist *vgic)
+{
+ return 0;
+}
+
+static inline paddr_t vgic_rdist_base(struct vgic_dist *vgic, unsigned int i)
+{
+ return INVALID_PADDR;
+}
+
+static inline paddr_t vgic_rdist_size(struct vgic_dist *vgic, unsigned int i)
+{
+ return INVALID_PADDR;
+}
+#endif
extern struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int virq);
extern void vgic_remove_irq_from_queues(struct vcpu *v, struct pending_irq *p);
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 04/11] xen/arm: introduce accessors for vgic dist, cpu, and rdist base addresses
2021-09-23 3:11 ` [PATCH 04/11] xen/arm: introduce accessors for vgic dist, cpu, and rdist base addresses Penny Zheng
@ 2021-09-23 10:45 ` Julien Grall
0 siblings, 0 replies; 37+ messages in thread
From: Julien Grall @ 2021-09-23 10:45 UTC (permalink / raw)
To: Penny Zheng, xen-devel, sstabellini; +Cc: Bertrand.Marquis, Wei.Chen
Hi,
On 23/09/2021 08:11, Penny Zheng wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
>
> Introduce accessors for vgic dist, cpu, and rdist base addresses, on
> all gic types.
>
> Use the accessors when making gic node for device tree of domU.
Please explain in the commit message why you need them.
however, I am not entirely convined of the usefulness of the helpers. It
will call to bits that are already directly accessible and you could
simply #ifdef make_gicv3_domU_node.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> xen/arch/arm/domain_build.c | 21 ++++++++++++-----
> xen/include/asm-arm/new_vgic.h | 24 ++++++++++++++++++++
> xen/include/asm-arm/vgic.h | 41 ++++++++++++++++++++++++++++++++++
> 3 files changed, 80 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 213ad017dc..d5f201f73e 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1802,8 +1802,12 @@ static int __init make_gicv2_domU_node(struct kernel_info *kinfo)
> int res = 0;
> __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * 2];
> __be32 *cells;
> + struct domain *d = kinfo->d;
> + char buf[38];
>
> - res = fdt_begin_node(fdt, "interrupt-controller@"__stringify(GUEST_GICD_BASE));
> + snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64,
> + vgic_dist_base(&d->arch.vgic));
> + res = fdt_begin_node(fdt, buf);
> if ( res )
> return res;
>
> @@ -1825,9 +1829,9 @@ static int __init make_gicv2_domU_node(struct kernel_info *kinfo)
>
> cells = ®[0];
> dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
> - GUEST_GICD_BASE, GUEST_GICD_SIZE);
> + vgic_dist_base(&d->arch.vgic), GUEST_GICD_SIZE);
> dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
> - GUEST_GICC_BASE, GUEST_GICC_SIZE);
> + vgic_cpu_base(&d->arch.vgic), GUEST_GICC_SIZE);
>
> res = fdt_property(fdt, "reg", reg, sizeof(reg));
> if (res)
> @@ -1852,8 +1856,12 @@ static int __init make_gicv3_domU_node(struct kernel_info *kinfo)
> int res = 0;
> __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * 2];
> __be32 *cells;
> + struct domain *d = kinfo->d;
> + char buf[38];
>
> - res = fdt_begin_node(fdt, "interrupt-controller@"__stringify(GUEST_GICV3_GICD_BASE));
> + snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64,
> + vgic_dist_base(&d->arch.vgic));
> + res = fdt_begin_node(fdt, buf);
> if ( res )
> return res;
>
> @@ -1875,9 +1883,10 @@ static int __init make_gicv3_domU_node(struct kernel_info *kinfo)
>
> cells = ®[0];
> dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
> - GUEST_GICV3_GICD_BASE, GUEST_GICV3_GICD_SIZE);
> + vgic_dist_base(&d->arch.vgic), GUEST_GICV3_GICD_SIZE);
> dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
> - GUEST_GICV3_GICR0_BASE, GUEST_GICV3_GICR0_SIZE);
> + vgic_rdist_base(&d->arch.vgic, 0),
> + vgic_rdist_size(&d->arch.vgic, 0));
>
> res = fdt_property(fdt, "reg", reg, sizeof(reg));
> if (res)
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 62c2ae538d..e1bc5113da 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -277,6 +277,47 @@ enum gic_sgi_mode;
> */
> #define REG_RANK_INDEX(b, n, s) ((((n) >> s) & ((b)-1)) % 32)
>
> +static inline paddr_t vgic_cpu_base(struct vgic_dist *vgic)
The names of the helpers suggest that if we pass any domain (including
dom0), then we would return the correct address. However, here this is
only going to return the address for the guests.
This looks like to be solved by the follow-up patches (in particular
patch #7, #8). So I think it would be best to first introduced all the
fields and then use it directly here.
> +{
> + return GUEST_GICC_BASE;
> +}
> +
> +static inline paddr_t vgic_dist_base(struct vgic_dist *vgic)
> +{
> + return GUEST_GICD_BASE;
> +}
> +
> +#ifdef CONFIG_GICV3
> +static inline unsigned int vgic_rdist_nr(struct vgic_dist *vgic)
> +{
> + return GUEST_GICV3_RDIST_REGIONS;
> +}
> +
> +static inline paddr_t vgic_rdist_base(struct vgic_dist *vgic, unsigned int i)
> +{
> + return GUEST_GICV3_GICR0_BASE;
> +}
> +
> +static inline paddr_t vgic_rdist_size(struct vgic_dist *vgic, unsigned int i)
> +{
> + return GUEST_GICV3_GICR0_SIZE;
> +}
> +#else
> +static inline unsigned int vgic_rdist_nr(struct vgic_dist *vgic)
> +{
> + return 0;
> +}
> +
> +static inline paddr_t vgic_rdist_base(struct vgic_dist *vgic, unsigned int i)
> +{
> + return INVALID_PADDR;
> +}
> +
> +static inline paddr_t vgic_rdist_size(struct vgic_dist *vgic, unsigned int i)
> +{
> + return INVALID_PADDR;
> +}
> +#endif
>
> extern struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int virq);
> extern void vgic_remove_irq_from_queues(struct vcpu *v, struct pending_irq *p);
>
--
Julien Grall
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 05/11] xen/arm: vgic: introduce vgic.cbase
2021-09-23 3:11 [PATCH 00/11] 1:1 direct-map memory map Penny Zheng
` (3 preceding siblings ...)
2021-09-23 3:11 ` [PATCH 04/11] xen/arm: introduce accessors for vgic dist, cpu, and rdist base addresses Penny Zheng
@ 2021-09-23 3:11 ` Penny Zheng
2021-09-23 10:47 ` Julien Grall
2021-09-23 3:11 ` [PATCH 06/11] xen/arm: new vgic: update vgic_cpu_base Penny Zheng
` (5 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Penny Zheng @ 2021-09-23 3:11 UTC (permalink / raw)
To: xen-devel, sstabellini, julien; +Cc: Bertrand.Marquis, Penny.Zheng, Wei.Chen
From: Stefano Stabellini <stefano.stabellini@xilinx.com>
Add a field in struct vgic_dist to store the cpu interface base address,
similar to the existing dbase field.
Use the field in vgic_v2_domain_init, instead of original local variable.
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
xen/arch/arm/vgic-v2.c | 10 +++++-----
xen/include/asm-arm/vgic.h | 1 +
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index b2da886adc..b34ec88106 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -652,7 +652,7 @@ static int vgic_v2_vcpu_init(struct vcpu *v)
static int vgic_v2_domain_init(struct domain *d)
{
int ret;
- paddr_t cbase, csize;
+ paddr_t csize;
paddr_t vbase;
/*
@@ -669,7 +669,7 @@ static int vgic_v2_domain_init(struct domain *d)
* Note that we assume the size of the CPU interface is always
* aligned to PAGE_SIZE.
*/
- cbase = vgic_v2_hw.cbase;
+ d->arch.vgic.cbase = vgic_v2_hw.cbase;
csize = vgic_v2_hw.csize;
vbase = vgic_v2_hw.vbase;
}
@@ -683,7 +683,7 @@ static int vgic_v2_domain_init(struct domain *d)
* region.
*/
BUILD_BUG_ON(GUEST_GICC_SIZE != SZ_8K);
- cbase = GUEST_GICC_BASE;
+ d->arch.vgic.cbase = GUEST_GICC_BASE;
csize = GUEST_GICC_SIZE;
vbase = vgic_v2_hw.vbase + vgic_v2_hw.aliased_offset;
}
@@ -692,8 +692,8 @@ static int vgic_v2_domain_init(struct domain *d)
* Map the gic virtual cpu interface in the gic cpu interface
* region of the guest.
*/
- ret = map_mmio_regions(d, gaddr_to_gfn(cbase), csize / PAGE_SIZE,
- maddr_to_mfn(vbase));
+ ret = map_mmio_regions(d, gaddr_to_gfn(d->arch.vgic.cbase),
+ csize / PAGE_SIZE, maddr_to_mfn(vbase));
if ( ret )
return ret;
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index e1bc5113da..d5ad1f387b 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -152,6 +152,7 @@ struct vgic_dist {
struct pending_irq *pending_irqs;
/* Base address for guest GIC */
paddr_t dbase; /* Distributor base address */
+ paddr_t cbase; /* CPU interface base address */
#ifdef CONFIG_GICV3
/* GIC V3 addressing */
/* List of contiguous occupied by the redistributors */
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 05/11] xen/arm: vgic: introduce vgic.cbase
2021-09-23 3:11 ` [PATCH 05/11] xen/arm: vgic: introduce vgic.cbase Penny Zheng
@ 2021-09-23 10:47 ` Julien Grall
0 siblings, 0 replies; 37+ messages in thread
From: Julien Grall @ 2021-09-23 10:47 UTC (permalink / raw)
To: Penny Zheng, xen-devel, sstabellini; +Cc: Bertrand.Marquis, Wei.Chen
Hi Penny,
On 23/09/2021 08:11, Penny Zheng wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
>
> Add a field in struct vgic_dist to store the cpu interface base address,
> similar to the existing dbase field.
>
> Use the field in vgic_v2_domain_init, instead of original local variable.
Please explain in the commit message why this is necessary.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> xen/arch/arm/vgic-v2.c | 10 +++++-----
> xen/include/asm-arm/vgic.h | 1 +
> 2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index b2da886adc..b34ec88106 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -652,7 +652,7 @@ static int vgic_v2_vcpu_init(struct vcpu *v)
> static int vgic_v2_domain_init(struct domain *d)
> {
> int ret;
> - paddr_t cbase, csize;
> + paddr_t csize;
> paddr_t vbase;
>
> /*
> @@ -669,7 +669,7 @@ static int vgic_v2_domain_init(struct domain *d)
> * Note that we assume the size of the CPU interface is always
> * aligned to PAGE_SIZE.
> */
> - cbase = vgic_v2_hw.cbase;
> + d->arch.vgic.cbase = vgic_v2_hw.cbase;
> csize = vgic_v2_hw.csize;
> vbase = vgic_v2_hw.vbase;
> }
> @@ -683,7 +683,7 @@ static int vgic_v2_domain_init(struct domain *d)
> * region.
> */
> BUILD_BUG_ON(GUEST_GICC_SIZE != SZ_8K);
> - cbase = GUEST_GICC_BASE;
> + d->arch.vgic.cbase = GUEST_GICC_BASE;
> csize = GUEST_GICC_SIZE;
Shouldn't we also stash d->arch.vgic.csize?
> vbase = vgic_v2_hw.vbase + vgic_v2_hw.aliased_offset;
> }
> @@ -692,8 +692,8 @@ static int vgic_v2_domain_init(struct domain *d)
> * Map the gic virtual cpu interface in the gic cpu interface
> * region of the guest.
> */
> - ret = map_mmio_regions(d, gaddr_to_gfn(cbase), csize / PAGE_SIZE,
> - maddr_to_mfn(vbase));
> + ret = map_mmio_regions(d, gaddr_to_gfn(d->arch.vgic.cbase),
> + csize / PAGE_SIZE, maddr_to_mfn(vbase));
> if ( ret )
> return ret;
>
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index e1bc5113da..d5ad1f387b 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -152,6 +152,7 @@ struct vgic_dist {
> struct pending_irq *pending_irqs;
> /* Base address for guest GIC */
> paddr_t dbase; /* Distributor base address */
> + paddr_t cbase; /* CPU interface base address */
> #ifdef CONFIG_GICV3
> /* GIC V3 addressing */
> /* List of contiguous occupied by the redistributors */
>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 06/11] xen/arm: new vgic: update vgic_cpu_base
2021-09-23 3:11 [PATCH 00/11] 1:1 direct-map memory map Penny Zheng
` (4 preceding siblings ...)
2021-09-23 3:11 ` [PATCH 05/11] xen/arm: vgic: introduce vgic.cbase Penny Zheng
@ 2021-09-23 3:11 ` Penny Zheng
2021-09-23 10:47 ` Julien Grall
2021-09-23 3:11 ` [PATCH 07/11] xen/arm: if 1:1 direct-map domain use native addresses for GICv2 Penny Zheng
` (4 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Penny Zheng @ 2021-09-23 3:11 UTC (permalink / raw)
To: xen-devel, sstabellini, julien; +Cc: Bertrand.Marquis, Penny.Zheng, Wei.Chen
From: Stefano Stabellini <stefano.stabellini@xilinx.com>
In vgic_v2_map_resources, use the new vgic_cpu_base field of struct
vgic_dist instead of original local variable.
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
xen/arch/arm/vgic/vgic-v2.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/xen/arch/arm/vgic/vgic-v2.c b/xen/arch/arm/vgic/vgic-v2.c
index b5ba4ace87..fd452fcb5a 100644
--- a/xen/arch/arm/vgic/vgic-v2.c
+++ b/xen/arch/arm/vgic/vgic-v2.c
@@ -258,7 +258,7 @@ void vgic_v2_enable(struct vcpu *vcpu)
int vgic_v2_map_resources(struct domain *d)
{
struct vgic_dist *dist = &d->arch.vgic;
- paddr_t cbase, csize;
+ paddr_t csize;
paddr_t vbase;
int ret;
@@ -276,7 +276,7 @@ int vgic_v2_map_resources(struct domain *d)
* Note that we assume the size of the CPU interface is always
* aligned to PAGE_SIZE.
*/
- cbase = gic_v2_hw_data.cbase;
+ d->arch.vgic.vgic_cpu_base = gic_v2_hw_data.cbase;
csize = gic_v2_hw_data.csize;
vbase = gic_v2_hw_data.vbase;
}
@@ -290,7 +290,7 @@ int vgic_v2_map_resources(struct domain *d)
* region.
*/
BUILD_BUG_ON(GUEST_GICC_SIZE != SZ_8K);
- cbase = GUEST_GICC_BASE;
+ d->arch.vgic.vgic_cpu_base = GUEST_GICC_BASE;
csize = GUEST_GICC_SIZE;
vbase = gic_v2_hw_data.vbase + gic_v2_hw_data.aliased_offset;
}
@@ -308,8 +308,8 @@ int vgic_v2_map_resources(struct domain *d)
* Map the gic virtual cpu interface in the gic cpu interface
* region of the guest.
*/
- ret = map_mmio_regions(d, gaddr_to_gfn(cbase), csize / PAGE_SIZE,
- maddr_to_mfn(vbase));
+ ret = map_mmio_regions(d, gaddr_to_gfn(d->arch.vgic.vgic_cpu_base),
+ csize / PAGE_SIZE, maddr_to_mfn(vbase));
if ( ret )
{
gdprintk(XENLOG_ERR, "Unable to remap VGIC CPU to VCPU\n");
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 06/11] xen/arm: new vgic: update vgic_cpu_base
2021-09-23 3:11 ` [PATCH 06/11] xen/arm: new vgic: update vgic_cpu_base Penny Zheng
@ 2021-09-23 10:47 ` Julien Grall
0 siblings, 0 replies; 37+ messages in thread
From: Julien Grall @ 2021-09-23 10:47 UTC (permalink / raw)
To: Penny Zheng, xen-devel, sstabellini; +Cc: Bertrand.Marquis, Wei.Chen
Hi,
On 23/09/2021 08:11, Penny Zheng wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
>
> In vgic_v2_map_resources, use the new vgic_cpu_base field of struct
> vgic_dist instead of original local variable.
I think this wants to be folded in patch #5.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> xen/arch/arm/vgic/vgic-v2.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/arm/vgic/vgic-v2.c b/xen/arch/arm/vgic/vgic-v2.c
> index b5ba4ace87..fd452fcb5a 100644
> --- a/xen/arch/arm/vgic/vgic-v2.c
> +++ b/xen/arch/arm/vgic/vgic-v2.c
> @@ -258,7 +258,7 @@ void vgic_v2_enable(struct vcpu *vcpu)
> int vgic_v2_map_resources(struct domain *d)
> {
> struct vgic_dist *dist = &d->arch.vgic;
> - paddr_t cbase, csize;
> + paddr_t csize;
> paddr_t vbase;
> int ret;
>
> @@ -276,7 +276,7 @@ int vgic_v2_map_resources(struct domain *d)
> * Note that we assume the size of the CPU interface is always
> * aligned to PAGE_SIZE.
> */
> - cbase = gic_v2_hw_data.cbase;
> + d->arch.vgic.vgic_cpu_base = gic_v2_hw_data.cbase;
> csize = gic_v2_hw_data.csize;
> vbase = gic_v2_hw_data.vbase;
> }
> @@ -290,7 +290,7 @@ int vgic_v2_map_resources(struct domain *d)
> * region.
> */
> BUILD_BUG_ON(GUEST_GICC_SIZE != SZ_8K);
> - cbase = GUEST_GICC_BASE;
> + d->arch.vgic.vgic_cpu_base = GUEST_GICC_BASE;
> csize = GUEST_GICC_SIZE;
> vbase = gic_v2_hw_data.vbase + gic_v2_hw_data.aliased_offset;
> }
> @@ -308,8 +308,8 @@ int vgic_v2_map_resources(struct domain *d)
> * Map the gic virtual cpu interface in the gic cpu interface
> * region of the guest.
> */
> - ret = map_mmio_regions(d, gaddr_to_gfn(cbase), csize / PAGE_SIZE,
> - maddr_to_mfn(vbase));
> + ret = map_mmio_regions(d, gaddr_to_gfn(d->arch.vgic.vgic_cpu_base),
> + csize / PAGE_SIZE, maddr_to_mfn(vbase));
> if ( ret )
> {
> gdprintk(XENLOG_ERR, "Unable to remap VGIC CPU to VCPU\n");
>
--
Julien Grall
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 07/11] xen/arm: if 1:1 direct-map domain use native addresses for GICv2
2021-09-23 3:11 [PATCH 00/11] 1:1 direct-map memory map Penny Zheng
` (5 preceding siblings ...)
2021-09-23 3:11 ` [PATCH 06/11] xen/arm: new vgic: update vgic_cpu_base Penny Zheng
@ 2021-09-23 3:11 ` Penny Zheng
2021-09-23 10:52 ` Julien Grall
2021-09-23 3:11 ` [PATCH 08/11] xen/arm: if 1:1 direct-map domain use native addresses for GICv3 Penny Zheng
` (3 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Penny Zheng @ 2021-09-23 3:11 UTC (permalink / raw)
To: xen-devel, sstabellini, julien; +Cc: Bertrand.Marquis, Penny.Zheng, Wei.Chen
From: Stefano Stabellini <stefano.stabellini@xilinx.com>
Today we use native addresses to map the GICv2 for Dom0 and fixed
addresses for DomUs.
This patch changes the behavior so that native addresses are used for
all domains that are 1:1 direct-map(including Dom0).
Update the accessors to use the struct vgic variables to provide the
updated addresses.
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
xen/arch/arm/vgic-v2.c | 16 ++++++++++++++++
xen/arch/arm/vgic/vgic-v2.c | 17 +++++++++++++++++
xen/include/asm-arm/new_vgic.h | 4 ++--
xen/include/asm-arm/vgic.h | 4 ++--
4 files changed, 37 insertions(+), 4 deletions(-)
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index b34ec88106..a8cf8173d0 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -673,6 +673,22 @@ static int vgic_v2_domain_init(struct domain *d)
csize = vgic_v2_hw.csize;
vbase = vgic_v2_hw.vbase;
}
+ else if ( is_domain_direct_mapped(d) )
+ {
+ /*
+ * For non-dom0 direct_mapped guests we only map a 8kB CPU
+ * interface but we make sure it is at a location occupied by
+ * the physical GIC in the host device tree.
+ *
+ * We need to add an offset to the virtual CPU interface base
+ * address when the GIC is aliased to get a 8kB contiguous
+ * region.
+ */
+ d->arch.vgic.dbase = vgic_v2_hw.dbase;
+ d->arch.vgic.cbase = vgic_v2_hw.cbase + vgic_v2_hw.aliased_offset;
+ csize = GUEST_GICC_SIZE;
+ vbase = vgic_v2_hw.vbase + vgic_v2_hw.aliased_offset;
+ }
else
{
d->arch.vgic.dbase = GUEST_GICD_BASE;
diff --git a/xen/arch/arm/vgic/vgic-v2.c b/xen/arch/arm/vgic/vgic-v2.c
index fd452fcb5a..ce1f6e4373 100644
--- a/xen/arch/arm/vgic/vgic-v2.c
+++ b/xen/arch/arm/vgic/vgic-v2.c
@@ -280,6 +280,23 @@ int vgic_v2_map_resources(struct domain *d)
csize = gic_v2_hw_data.csize;
vbase = gic_v2_hw_data.vbase;
}
+ else if ( is_domain_direct_mapped(d) )
+ {
+ d->arch.vgic.vgic_dist_base = gic_v2_hw_data.dbase;
+ /*
+ * For non-dom0 direct_mapped guests we only map a 8kB CPU
+ * interface but we make sure it is at a location occupied by
+ * the physical GIC in the host device tree.
+ *
+ * We need to add an offset to the virtual CPU interface base
+ * address when the GIC is aliased to get a 8kB contiguous
+ * region.
+ */
+ d->arch.vgic.vgic_cpu_base = gic_v2_hw_data.cbase +
+ gic_v2_hw_data.aliased_offset;
+ csize = GUEST_GICC_SIZE;
+ vbase = gic_v2_hw_data.vbase + gic_v2_hw_data.aliased_offset;
+ }
else
{
d->arch.vgic.vgic_dist_base = GUEST_GICD_BASE;
diff --git a/xen/include/asm-arm/new_vgic.h b/xen/include/asm-arm/new_vgic.h
index 9097522b27..0d16f878eb 100644
--- a/xen/include/asm-arm/new_vgic.h
+++ b/xen/include/asm-arm/new_vgic.h
@@ -188,12 +188,12 @@ struct vgic_cpu {
static inline paddr_t vgic_cpu_base(struct vgic_dist *vgic)
{
- return GUEST_GICC_BASE;
+ return vgic->vgic_cpu_base;
}
static inline paddr_t vgic_dist_base(struct vgic_dist *vgic)
{
- return GUEST_GICD_BASE;
+ return vgic->vgic_dist_base;
}
static inline unsigned int vgic_rdist_nr(struct vgic_dist *vgic)
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index d5ad1f387b..92f6a2d15d 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -280,12 +280,12 @@ enum gic_sgi_mode;
static inline paddr_t vgic_cpu_base(struct vgic_dist *vgic)
{
- return GUEST_GICC_BASE;
+ return vgic->cbase;
}
static inline paddr_t vgic_dist_base(struct vgic_dist *vgic)
{
- return GUEST_GICD_BASE;
+ return vgic->dbase;
}
#ifdef CONFIG_GICV3
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 07/11] xen/arm: if 1:1 direct-map domain use native addresses for GICv2
2021-09-23 3:11 ` [PATCH 07/11] xen/arm: if 1:1 direct-map domain use native addresses for GICv2 Penny Zheng
@ 2021-09-23 10:52 ` Julien Grall
0 siblings, 0 replies; 37+ messages in thread
From: Julien Grall @ 2021-09-23 10:52 UTC (permalink / raw)
To: Penny Zheng, xen-devel, sstabellini; +Cc: Bertrand.Marquis, Wei.Chen
Hi,
On 23/09/2021 08:11, Penny Zheng wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
>
> Today we use native addresses to map the GICv2 for Dom0 and fixed
> addresses for DomUs.
>
> This patch changes the behavior so that native addresses are used for
> all domains that are 1:1 direct-map(including Dom0).
>
> Update the accessors to use the struct vgic variables to provide the
> updated addresses.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> xen/arch/arm/vgic-v2.c | 16 ++++++++++++++++
> xen/arch/arm/vgic/vgic-v2.c | 17 +++++++++++++++++
> xen/include/asm-arm/new_vgic.h | 4 ++--
> xen/include/asm-arm/vgic.h | 4 ++--
> 4 files changed, 37 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index b34ec88106..a8cf8173d0 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -673,6 +673,22 @@ static int vgic_v2_domain_init(struct domain *d)
> csize = vgic_v2_hw.csize;
> vbase = vgic_v2_hw.vbase;
> }
> + else if ( is_domain_direct_mapped(d) )
> + {
> + /*
> + * For non-dom0 direct_mapped guests we only map a 8kB CPU
> + * interface but we make sure it is at a location occupied by
> + * the physical GIC in the host device tree.
> + *
> + * We need to add an offset to the virtual CPU interface base
> + * address when the GIC is aliased to get a 8kB contiguous
> + * region.
> + */
So I agree that we need to differentiate between dom0 and other direct
mapped domains. However, I think it would be good to explainm why given
that in other places you treat dom0 and direct mapped domain the same way.
> + d->arch.vgic.dbase = vgic_v2_hw.dbase;
> + d->arch.vgic.cbase = vgic_v2_hw.cbase + vgic_v2_hw.aliased_offset;
> + csize = GUEST_GICC_SIZE;
> + vbase = vgic_v2_hw.vbase + vgic_v2_hw.aliased_offset;
> + }
> else
> {
> d->arch.vgic.dbase = GUEST_GICD_BASE;
> diff --git a/xen/arch/arm/vgic/vgic-v2.c b/xen/arch/arm/vgic/vgic-v2.c
> index fd452fcb5a..ce1f6e4373 100644
> --- a/xen/arch/arm/vgic/vgic-v2.c
> +++ b/xen/arch/arm/vgic/vgic-v2.c
> @@ -280,6 +280,23 @@ int vgic_v2_map_resources(struct domain *d)
> csize = gic_v2_hw_data.csize;
> vbase = gic_v2_hw_data.vbase;
> }
> + else if ( is_domain_direct_mapped(d) )
> + {
> + d->arch.vgic.vgic_dist_base = gic_v2_hw_data.dbase;
> + /*
> + * For non-dom0 direct_mapped guests we only map a 8kB CPU
> + * interface but we make sure it is at a location occupied by
> + * the physical GIC in the host device tree.
> + *
> + * We need to add an offset to the virtual CPU interface base
> + * address when the GIC is aliased to get a 8kB contiguous
> + * region.
> + */
> + d->arch.vgic.vgic_cpu_base = gic_v2_hw_data.cbase +
> + gic_v2_hw_data.aliased_offset;
> + csize = GUEST_GICC_SIZE;
> + vbase = gic_v2_hw_data.vbase + gic_v2_hw_data.aliased_offset;
> + }
> else
> {
> d->arch.vgic.vgic_dist_base = GUEST_GICD_BASE;
> diff --git a/xen/include/asm-arm/new_vgic.h b/xen/include/asm-arm/new_vgic.h
> index 9097522b27..0d16f878eb 100644
> --- a/xen/include/asm-arm/new_vgic.h
> +++ b/xen/include/asm-arm/new_vgic.h
> @@ -188,12 +188,12 @@ struct vgic_cpu {
>
> static inline paddr_t vgic_cpu_base(struct vgic_dist *vgic)
> {
> - return GUEST_GICC_BASE;
> + return vgic->vgic_cpu_base;
> }
>
> static inline paddr_t vgic_dist_base(struct vgic_dist *vgic)
> {
> - return GUEST_GICD_BASE;
> + return vgic->vgic_dist_base;
> }
>
> static inline unsigned int vgic_rdist_nr(struct vgic_dist *vgic)
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index d5ad1f387b..92f6a2d15d 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -280,12 +280,12 @@ enum gic_sgi_mode;
>
> static inline paddr_t vgic_cpu_base(struct vgic_dist *vgic)
> {
> - return GUEST_GICC_BASE;
> + return vgic->cbase;
> }
>
> static inline paddr_t vgic_dist_base(struct vgic_dist *vgic)
> {
> - return GUEST_GICD_BASE;
> + return vgic->dbase;
> }
>
> #ifdef CONFIG_GICV3 >
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 08/11] xen/arm: if 1:1 direct-map domain use native addresses for GICv3
2021-09-23 3:11 [PATCH 00/11] 1:1 direct-map memory map Penny Zheng
` (6 preceding siblings ...)
2021-09-23 3:11 ` [PATCH 07/11] xen/arm: if 1:1 direct-map domain use native addresses for GICv2 Penny Zheng
@ 2021-09-23 3:11 ` Penny Zheng
2021-09-23 10:59 ` Julien Grall
2021-09-23 3:11 ` [PATCH 09/11] xen/arm: if 1:1 direct-map domain use native UART address and IRQ number for vPL011 Penny Zheng
` (2 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Penny Zheng @ 2021-09-23 3:11 UTC (permalink / raw)
To: xen-devel, sstabellini, julien; +Cc: Bertrand.Marquis, Penny.Zheng, Wei.Chen
From: Stefano Stabellini <stefano.stabellini@xilinx.com>
Today we use native addresses to map the GICv3 for Dom0 and fixed
addresses for DomUs.
This patch changes the behavior so that native addresses are used for
all domains that are 1:1 direct-map(including Dom0).
Update the rdist accessor to use the struct vgic variables to provide
the updated addresses.
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
xen/arch/arm/domain_build.c | 32 ++++++++++++++++++++++++--------
xen/arch/arm/vgic-v3.c | 10 +++++-----
xen/include/asm-arm/vgic.h | 6 +++---
3 files changed, 32 insertions(+), 16 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index d5f201f73e..120f1ae575 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1854,10 +1854,11 @@ static int __init make_gicv3_domU_node(struct kernel_info *kinfo)
{
void *fdt = kinfo->fdt;
int res = 0;
- __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * 2];
+ __be32 *reg;
__be32 *cells;
struct domain *d = kinfo->d;
char buf[38];
+ unsigned int i, len = 0;
snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64,
vgic_dist_base(&d->arch.vgic));
@@ -1881,27 +1882,42 @@ static int __init make_gicv3_domU_node(struct kernel_info *kinfo)
if ( res )
return res;
+ /* reg specifies all re-distributors and Distributor. */
+ len = (GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
+ (vgic_rdist_nr(&d->arch.vgic) + 1) * sizeof(__be32);
+ reg = xmalloc_bytes(len);
+ if ( reg == NULL )
+ return -ENOMEM;
+
cells = ®[0];
dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
vgic_dist_base(&d->arch.vgic), GUEST_GICV3_GICD_SIZE);
- dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
- vgic_rdist_base(&d->arch.vgic, 0),
- vgic_rdist_size(&d->arch.vgic, 0));
+ for ( i = 0;
+ i < vgic_rdist_nr(&d->arch.vgic);
+ i++, cells += (GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) )
+ {
+ dt_child_set_range(&cells,
+ GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
+ vgic_rdist_base(&d->arch.vgic, i),
+ vgic_rdist_size(&d->arch.vgic, i));
+ }
- res = fdt_property(fdt, "reg", reg, sizeof(reg));
+ res = fdt_property(fdt, "reg", reg, len);
if (res)
- return res;
+ goto out;
res = fdt_property_cell(fdt, "linux,phandle", kinfo->phandle_gic);
if (res)
- return res;
+ goto out;
res = fdt_property_cell(fdt, "phandle", kinfo->phandle_gic);
if (res)
- return res;
+ goto out;
res = fdt_end_node(fdt);
+ out:
+ xfree(reg);
return res;
}
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index cb5a70c42e..83f996b46c 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1647,8 +1647,8 @@ static inline unsigned int vgic_v3_max_rdist_count(struct domain *d)
* However DomU get a constructed memory map, so we can go with
* the architected single redistributor region.
*/
- return is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions :
- GUEST_GICV3_RDIST_REGIONS;
+ return is_domain_direct_mapped(d) ? vgic_v3_hw.nr_rdist_regions :
+ GUEST_GICV3_RDIST_REGIONS;
}
static int vgic_v3_domain_init(struct domain *d)
@@ -1670,10 +1670,10 @@ static int vgic_v3_domain_init(struct domain *d)
radix_tree_init(&d->arch.vgic.pend_lpi_tree);
/*
- * Domain 0 gets the hardware address.
- * Guests get the virtual platform layout.
+ * 1:1 direct-map domain (including Dom0) gets the hardware address.
+ * Other guests get the virtual platform layout.
*/
- if ( is_hardware_domain(d) )
+ if ( is_domain_direct_mapped(d) )
{
unsigned int first_cpu = 0;
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 92f6a2d15d..0f7cb32c58 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -291,17 +291,17 @@ static inline paddr_t vgic_dist_base(struct vgic_dist *vgic)
#ifdef CONFIG_GICV3
static inline unsigned int vgic_rdist_nr(struct vgic_dist *vgic)
{
- return GUEST_GICV3_RDIST_REGIONS;
+ return vgic->nr_regions;
}
static inline paddr_t vgic_rdist_base(struct vgic_dist *vgic, unsigned int i)
{
- return GUEST_GICV3_GICR0_BASE;
+ return vgic->rdist_regions[i].base;
}
static inline paddr_t vgic_rdist_size(struct vgic_dist *vgic, unsigned int i)
{
- return GUEST_GICV3_GICR0_SIZE;
+ return vgic->rdist_regions[i].size;
}
#else
static inline unsigned int vgic_rdist_nr(struct vgic_dist *vgic)
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 08/11] xen/arm: if 1:1 direct-map domain use native addresses for GICv3
2021-09-23 3:11 ` [PATCH 08/11] xen/arm: if 1:1 direct-map domain use native addresses for GICv3 Penny Zheng
@ 2021-09-23 10:59 ` Julien Grall
0 siblings, 0 replies; 37+ messages in thread
From: Julien Grall @ 2021-09-23 10:59 UTC (permalink / raw)
To: Penny Zheng, xen-devel, sstabellini; +Cc: Bertrand.Marquis, Wei.Chen
Hi,
On 23/09/2021 08:11, Penny Zheng wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
>
> Today we use native addresses to map the GICv3 for Dom0 and fixed
> addresses for DomUs.
>
> This patch changes the behavior so that native addresses are used for
> all domains that are 1:1 direct-map(including Dom0).
>
> Update the rdist accessor to use the struct vgic variables to provide
> the updated addresses.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> xen/arch/arm/domain_build.c | 32 ++++++++++++++++++++++++--------
> xen/arch/arm/vgic-v3.c | 10 +++++-----
> xen/include/asm-arm/vgic.h | 6 +++---
> 3 files changed, 32 insertions(+), 16 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index d5f201f73e..120f1ae575 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1854,10 +1854,11 @@ static int __init make_gicv3_domU_node(struct kernel_info *kinfo)
> {
> void *fdt = kinfo->fdt;
> int res = 0;
> - __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * 2];
> + __be32 *reg;
> __be32 *cells;
> struct domain *d = kinfo->d;
> char buf[38];
> + unsigned int i, len = 0;
>
> snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64,
> vgic_dist_base(&d->arch.vgic));
> @@ -1881,27 +1882,42 @@ static int __init make_gicv3_domU_node(struct kernel_info *kinfo)
> if ( res )
> return res;
>
> + /* reg specifies all re-distributors and Distributor. */
> + len = (GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
> + (vgic_rdist_nr(&d->arch.vgic) + 1) * sizeof(__be32);
> + reg = xmalloc_bytes(len);
> + if ( reg == NULL )
> + return -ENOMEM;
> +
> cells = ®[0];
> dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
> vgic_dist_base(&d->arch.vgic), GUEST_GICV3_GICD_SIZE);
> - dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
> - vgic_rdist_base(&d->arch.vgic, 0),
> - vgic_rdist_size(&d->arch.vgic, 0));
> + for ( i = 0;
> + i < vgic_rdist_nr(&d->arch.vgic);
> + i++, cells += (GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) )
> + {
> + dt_child_set_range(&cells,
> + GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
> + vgic_rdist_base(&d->arch.vgic, i),
> + vgic_rdist_size(&d->arch.vgic, i));
> + }
>
> - res = fdt_property(fdt, "reg", reg, sizeof(reg));
> + res = fdt_property(fdt, "reg", reg, len);
> if (res)
> - return res;
> + goto out;
>
> res = fdt_property_cell(fdt, "linux,phandle", kinfo->phandle_gic);
> if (res)
> - return res;
> + goto out;
>
> res = fdt_property_cell(fdt, "phandle", kinfo->phandle_gic);
> if (res)
> - return res;
> + goto out;
>
> res = fdt_end_node(fdt);
>
> + out:
> + xfree(reg);
> return res;
> }
>
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index cb5a70c42e..83f996b46c 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -1647,8 +1647,8 @@ static inline unsigned int vgic_v3_max_rdist_count(struct domain *d)
> * However DomU get a constructed memory map, so we can go with
> * the architected single redistributor region.
> */
The comment needs to be updated.
> - return is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions :
> - GUEST_GICV3_RDIST_REGIONS;
> + return is_domain_direct_mapped(d) ? vgic_v3_hw.nr_rdist_regions :
> + GUEST_GICV3_RDIST_REGIONS;
Let's avoid to make the assumption that dom0 is always direct mapped. So
this should be "is_domain_direct_mapped(d) || is_hardware_domain(d)".
I would actually suggest to introduce a new helper
"is_domain_use_host_layout()" (or similar) that wraps the two check.
This would make the code a bit more flexible.
> }
>
> static int vgic_v3_domain_init(struct domain *d)
> @@ -1670,10 +1670,10 @@ static int vgic_v3_domain_init(struct domain *d)
> radix_tree_init(&d->arch.vgic.pend_lpi_tree);
>
> /*
> - * Domain 0 gets the hardware address.
> - * Guests get the virtual platform layout.
> + * 1:1 direct-map domain (including Dom0) gets the hardware address.
> + * Other guests get the virtual platform layout.
> */
> - if ( is_hardware_domain(d) )
> + if ( is_domain_direct_mapped(d) )
Same here.
> {
> unsigned int first_cpu = 0;
>
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 92f6a2d15d..0f7cb32c58 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -291,17 +291,17 @@ static inline paddr_t vgic_dist_base(struct vgic_dist *vgic)
> #ifdef CONFIG_GICV3
> static inline unsigned int vgic_rdist_nr(struct vgic_dist *vgic)
> {
> - return GUEST_GICV3_RDIST_REGIONS;
> + return vgic->nr_regions;
> }
>
> static inline paddr_t vgic_rdist_base(struct vgic_dist *vgic, unsigned int i)
> {
> - return GUEST_GICV3_GICR0_BASE;
> + return vgic->rdist_regions[i].base;
> }
>
> static inline paddr_t vgic_rdist_size(struct vgic_dist *vgic, unsigned int i)
> {
> - return GUEST_GICV3_GICR0_SIZE;
> + return vgic->rdist_regions[i].size;
> }
I think this change in vgic.h should have belong to the patch
introducing the helpers (if we still plan to use them).
> #else
> static inline unsigned int vgic_rdist_nr(struct vgic_dist *vgic)
>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 09/11] xen/arm: if 1:1 direct-map domain use native UART address and IRQ number for vPL011
2021-09-23 3:11 [PATCH 00/11] 1:1 direct-map memory map Penny Zheng
` (7 preceding siblings ...)
2021-09-23 3:11 ` [PATCH 08/11] xen/arm: if 1:1 direct-map domain use native addresses for GICv3 Penny Zheng
@ 2021-09-23 3:11 ` Penny Zheng
2021-09-23 11:14 ` Julien Grall
2021-09-23 3:11 ` [PATCH 10/11] xen/arm: device assignment on 1:1 direct-map domain Penny Zheng
2021-09-23 3:11 ` [PATCH 11/11] xen/docs: add a document to explain how to do passthrough without IOMMU Penny Zheng
10 siblings, 1 reply; 37+ messages in thread
From: Penny Zheng @ 2021-09-23 3:11 UTC (permalink / raw)
To: xen-devel, sstabellini, julien; +Cc: Bertrand.Marquis, Penny.Zheng, Wei.Chen
From: Stefano Stabellini <stefano.stabellini@xilinx.com>
We always use a fix address to map the vPL011 to domains. The address
could be a problem for domains that are directly mapped.
So, for domains that are directly mapped, reuse the address of the
physical UART on the platform to avoid potential clashes.
Do the same for the virtual IRQ number: instead of always using
GUEST_VPL011_SPI, try to reuse the physical SPI number if possible.
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
xen/arch/arm/domain_build.c | 34 +++++++++++++++++++++++++++-------
xen/arch/arm/vpl011.c | 34 +++++++++++++++++++++++++++-------
xen/include/asm-arm/vpl011.h | 2 ++
3 files changed, 56 insertions(+), 14 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 120f1ae575..c92e510ae7 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -30,6 +30,7 @@
#include <xen/irq.h>
#include <xen/grant_table.h>
+#include <xen/serial.h>
static unsigned int __initdata opt_dom0_max_vcpus;
integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
@@ -1942,8 +1943,11 @@ static int __init make_vpl011_uart_node(struct kernel_info *kinfo)
gic_interrupt_t intr;
__be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS];
__be32 *cells;
+ struct domain *d = kinfo->d;
+ char buf[27];
- res = fdt_begin_node(fdt, "sbsa-uart@"__stringify(GUEST_PL011_BASE));
+ snprintf(buf, sizeof(buf), "sbsa-uart@%"PRIx64, d->arch.vpl011.base_addr);
+ res = fdt_begin_node(fdt, buf);
if ( res )
return res;
@@ -1953,14 +1957,14 @@ static int __init make_vpl011_uart_node(struct kernel_info *kinfo)
cells = ®[0];
dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS,
- GUEST_ROOT_SIZE_CELLS, GUEST_PL011_BASE,
+ GUEST_ROOT_SIZE_CELLS, d->arch.vpl011.base_addr,
GUEST_PL011_SIZE);
res = fdt_property(fdt, "reg", reg, sizeof(reg));
if ( res )
return res;
- set_interrupt(intr, GUEST_VPL011_SPI, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
+ set_interrupt(intr, d->arch.vpl011.virq, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
res = fdt_property(fdt, "interrupts", intr, sizeof (intr));
if ( res )
@@ -2670,6 +2674,13 @@ static int __init construct_domU(struct domain *d,
else
allocate_static_memory(d, &kinfo, node);
+ /*
+ * Initialization before creating its device
+ * tree node in prepare_dtb_domU.
+ */
+ if ( kinfo.vpl011 )
+ rc = domain_vpl011_init(d, NULL);
+
rc = prepare_dtb_domU(d, &kinfo);
if ( rc < 0 )
return rc;
@@ -2678,9 +2689,6 @@ static int __init construct_domU(struct domain *d,
if ( rc < 0 )
return rc;
- if ( kinfo.vpl011 )
- rc = domain_vpl011_init(d, NULL);
-
return rc;
}
@@ -2723,15 +2731,27 @@ void __init create_domUs(void)
if ( !dt_property_read_u32(node, "nr_spis", &d_cfg.arch.nr_spis) )
{
+ unsigned int vpl011_virq = GUEST_VPL011_SPI;
d_cfg.arch.nr_spis = gic_number_lines() - 32;
+ /*
+ * The VPL011 virq is GUEST_VPL011_SPI, unless direct-map in
+ * set, in which case we'll try to match the hardware.
+ *
+ * Typically, d->arch.vpl011.virq has the vpl011 irq number
+ * but at this point of the boot sequence it is not
+ * initialized yet.
+ */
+ if ( direct_map && serial_irq(SERHND_DTUART) > 0 )
+ vpl011_virq = serial_irq(SERHND_DTUART);
+
/*
* vpl011 uses one emulated SPI. If vpl011 is requested, make
* sure that we allocate enough SPIs for it.
*/
if ( dt_property_read_bool(node, "vpl011") )
d_cfg.arch.nr_spis = MAX(d_cfg.arch.nr_spis,
- GUEST_VPL011_SPI - 32 + 1);
+ vpl011_virq - 32 + 1);
}
/*
diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
index 895f436cc4..10df25f098 100644
--- a/xen/arch/arm/vpl011.c
+++ b/xen/arch/arm/vpl011.c
@@ -29,6 +29,7 @@
#include <xen/mm.h>
#include <xen/sched.h>
#include <xen/console.h>
+#include <xen/serial.h>
#include <public/domctl.h>
#include <public/io/console.h>
#include <asm/pl011-uart.h>
@@ -71,11 +72,11 @@ static void vpl011_update_interrupt_status(struct domain *d)
* status bit has been set since the last time.
*/
if ( uartmis & ~vpl011->shadow_uartmis )
- vgic_inject_irq(d, NULL, GUEST_VPL011_SPI, true);
+ vgic_inject_irq(d, NULL, vpl011->virq, true);
vpl011->shadow_uartmis = uartmis;
#else
- vgic_inject_irq(d, NULL, GUEST_VPL011_SPI, uartmis);
+ vgic_inject_irq(d, NULL, vpl011->virq, uartmis);
#endif
}
@@ -347,7 +348,8 @@ static int vpl011_mmio_read(struct vcpu *v,
void *priv)
{
struct hsr_dabt dabt = info->dabt;
- uint32_t vpl011_reg = (uint32_t)(info->gpa - GUEST_PL011_BASE);
+ uint32_t vpl011_reg = (uint32_t)(info->gpa -
+ v->domain->arch.vpl011.base_addr);
struct vpl011 *vpl011 = &v->domain->arch.vpl011;
struct domain *d = v->domain;
unsigned long flags;
@@ -430,7 +432,8 @@ static int vpl011_mmio_write(struct vcpu *v,
void *priv)
{
struct hsr_dabt dabt = info->dabt;
- uint32_t vpl011_reg = (uint32_t)(info->gpa - GUEST_PL011_BASE);
+ uint32_t vpl011_reg = (uint32_t)(info->gpa -
+ v->domain->arch.vpl011.base_addr);
struct vpl011 *vpl011 = &v->domain->arch.vpl011;
struct domain *d = v->domain;
unsigned long flags;
@@ -622,10 +625,27 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
{
int rc;
struct vpl011 *vpl011 = &d->arch.vpl011;
+ const struct vuart_info *uart = serial_vuart_info(SERHND_DTUART);
if ( vpl011->backend.dom.ring_buf )
return -EINVAL;
+ vpl011->base_addr = GUEST_PL011_BASE;
+ vpl011->virq = GUEST_VPL011_SPI;
+ if ( is_domain_direct_mapped(d) )
+ {
+ if ( uart != NULL && serial_irq(SERHND_DTUART) > 0 )
+ {
+ vpl011->base_addr = uart->base_addr;
+ vpl011->virq = serial_irq(SERHND_DTUART);
+ }
+ else
+ printk(XENLOG_ERR
+ "Unable to reuse physical UART address and irq for vPL011.\n"
+ "Defaulting to addr %#"PRIpaddr" and IRQ %u\n",
+ vpl011->base_addr, vpl011->virq);
+ }
+
/*
* info is NULL when the backend is in Xen.
* info is != NULL when the backend is in a domain.
@@ -661,7 +681,7 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
}
}
- rc = vgic_reserve_virq(d, GUEST_VPL011_SPI);
+ rc = vgic_reserve_virq(d, vpl011->virq);
if ( !rc )
{
rc = -EINVAL;
@@ -673,12 +693,12 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
spin_lock_init(&vpl011->lock);
register_mmio_handler(d, &vpl011_mmio_handler,
- GUEST_PL011_BASE, GUEST_PL011_SIZE, NULL);
+ vpl011->base_addr, GUEST_PL011_SIZE, NULL);
return 0;
out2:
- vgic_free_virq(d, GUEST_VPL011_SPI);
+ vgic_free_virq(d, vpl011->virq);
out1:
if ( vpl011->backend_in_domain )
diff --git a/xen/include/asm-arm/vpl011.h b/xen/include/asm-arm/vpl011.h
index e6c7ab7381..c09abcd7a9 100644
--- a/xen/include/asm-arm/vpl011.h
+++ b/xen/include/asm-arm/vpl011.h
@@ -53,6 +53,8 @@ struct vpl011 {
uint32_t uarticr; /* Interrupt clear register */
uint32_t uartris; /* Raw interrupt status register */
uint32_t shadow_uartmis; /* shadow masked interrupt register */
+ paddr_t base_addr;
+ unsigned int virq;
spinlock_t lock;
evtchn_port_t evtchn;
};
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 09/11] xen/arm: if 1:1 direct-map domain use native UART address and IRQ number for vPL011
2021-09-23 3:11 ` [PATCH 09/11] xen/arm: if 1:1 direct-map domain use native UART address and IRQ number for vPL011 Penny Zheng
@ 2021-09-23 11:14 ` Julien Grall
2021-10-09 8:47 ` Penny Zheng
0 siblings, 1 reply; 37+ messages in thread
From: Julien Grall @ 2021-09-23 11:14 UTC (permalink / raw)
To: Penny Zheng, xen-devel, sstabellini; +Cc: Bertrand.Marquis, Wei.Chen
On 23/09/2021 08:11, Penny Zheng wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
>
> We always use a fix address to map the vPL011 to domains. The address
> could be a problem for domains that are directly mapped.
>
> So, for domains that are directly mapped, reuse the address of the
> physical UART on the platform to avoid potential clashes.
>
> Do the same for the virtual IRQ number: instead of always using
> GUEST_VPL011_SPI, try to reuse the physical SPI number if possible.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> xen/arch/arm/domain_build.c | 34 +++++++++++++++++++++++++++-------
> xen/arch/arm/vpl011.c | 34 +++++++++++++++++++++++++++-------
> xen/include/asm-arm/vpl011.h | 2 ++
> 3 files changed, 56 insertions(+), 14 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 120f1ae575..c92e510ae7 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -30,6 +30,7 @@
>
> #include <xen/irq.h>
> #include <xen/grant_table.h>
> +#include <xen/serial.h>
>
> static unsigned int __initdata opt_dom0_max_vcpus;
> integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
> @@ -1942,8 +1943,11 @@ static int __init make_vpl011_uart_node(struct kernel_info *kinfo)
> gic_interrupt_t intr;
> __be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS];
> __be32 *cells;
> + struct domain *d = kinfo->d;
> + char buf[27];
>
> - res = fdt_begin_node(fdt, "sbsa-uart@"__stringify(GUEST_PL011_BASE));
> + snprintf(buf, sizeof(buf), "sbsa-uart@%"PRIx64, d->arch.vpl011.base_addr);
> + res = fdt_begin_node(fdt, buf);
> if ( res )
> return res;
>
> @@ -1953,14 +1957,14 @@ static int __init make_vpl011_uart_node(struct kernel_info *kinfo)
>
> cells = ®[0];
> dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS,
> - GUEST_ROOT_SIZE_CELLS, GUEST_PL011_BASE,
> + GUEST_ROOT_SIZE_CELLS, d->arch.vpl011.base_addr,
> GUEST_PL011_SIZE);
>
> res = fdt_property(fdt, "reg", reg, sizeof(reg));
> if ( res )
> return res;
>
> - set_interrupt(intr, GUEST_VPL011_SPI, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
> + set_interrupt(intr, d->arch.vpl011.virq, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
>
> res = fdt_property(fdt, "interrupts", intr, sizeof (intr));
> if ( res )
> @@ -2670,6 +2674,13 @@ static int __init construct_domU(struct domain *d,
> else
> allocate_static_memory(d, &kinfo, node);
>
> + /*
> + * Initialization before creating its device
> + * tree node in prepare_dtb_domU.
> + */
I think it would be better to explain *why* this needs to be done before.
> + if ( kinfo.vpl011 )
> + rc = domain_vpl011_init(d, NULL);
> +
> rc = prepare_dtb_domU(d, &kinfo);
> if ( rc < 0 )
> return rc;
> @@ -2678,9 +2689,6 @@ static int __init construct_domU(struct domain *d,
> if ( rc < 0 )
> return rc;
>
> - if ( kinfo.vpl011 )
> - rc = domain_vpl011_init(d, NULL);
> -
> return rc;
> }
>
> @@ -2723,15 +2731,27 @@ void __init create_domUs(void)
>
> if ( !dt_property_read_u32(node, "nr_spis", &d_cfg.arch.nr_spis) )
> {
> + unsigned int vpl011_virq = GUEST_VPL011_SPI;
Coding style: Add a newline here.
> d_cfg.arch.nr_spis = gic_number_lines() - 32;
>
> + /*
> + * The VPL011 virq is GUEST_VPL011_SPI, unless direct-map in
> + * set, in which case we'll try to match the hardware.
> + *
> + * Typically, d->arch.vpl011.virq has the vpl011 irq number
> + * but at this point of the boot sequence it is not
> + * initialized yet.
> + */
> + if ( direct_map && serial_irq(SERHND_DTUART) > 0 )
> + vpl011_virq = serial_irq(SERHND_DTUART);
I think we should not continue if the domain is direct-mapped *and* the
IRQ is not found. Otherwise, this will may just result to potential
breakage if GUEST_VPL011_SPI happens to be used for an HW device.
> +
> /*
> * vpl011 uses one emulated SPI. If vpl011 is requested, make
> * sure that we allocate enough SPIs for it.
> */
> if ( dt_property_read_bool(node, "vpl011") )
> d_cfg.arch.nr_spis = MAX(d_cfg.arch.nr_spis,
> - GUEST_VPL011_SPI - 32 + 1);
> + vpl011_virq - 32 + 1);
> }
>
> /*
> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> index 895f436cc4..10df25f098 100644
> --- a/xen/arch/arm/vpl011.c
> +++ b/xen/arch/arm/vpl011.c
> @@ -29,6 +29,7 @@
> #include <xen/mm.h>
> #include <xen/sched.h>
> #include <xen/console.h>
> +#include <xen/serial.h>
> #include <public/domctl.h>
> #include <public/io/console.h>
> #include <asm/pl011-uart.h>
> @@ -71,11 +72,11 @@ static void vpl011_update_interrupt_status(struct domain *d)
> * status bit has been set since the last time.
> */
> if ( uartmis & ~vpl011->shadow_uartmis )
> - vgic_inject_irq(d, NULL, GUEST_VPL011_SPI, true);
> + vgic_inject_irq(d, NULL, vpl011->virq, true);
>
> vpl011->shadow_uartmis = uartmis;
> #else
> - vgic_inject_irq(d, NULL, GUEST_VPL011_SPI, uartmis);
> + vgic_inject_irq(d, NULL, vpl011->virq, uartmis);
> #endif
> }
>
> @@ -347,7 +348,8 @@ static int vpl011_mmio_read(struct vcpu *v,
> void *priv)
> {
> struct hsr_dabt dabt = info->dabt;
> - uint32_t vpl011_reg = (uint32_t)(info->gpa - GUEST_PL011_BASE);
> + uint32_t vpl011_reg = (uint32_t)(info->gpa -
> + v->domain->arch.vpl011.base_addr);
> struct vpl011 *vpl011 = &v->domain->arch.vpl011;
> struct domain *d = v->domain;
> unsigned long flags;
> @@ -430,7 +432,8 @@ static int vpl011_mmio_write(struct vcpu *v,
> void *priv)
> {
> struct hsr_dabt dabt = info->dabt;
> - uint32_t vpl011_reg = (uint32_t)(info->gpa - GUEST_PL011_BASE);
> + uint32_t vpl011_reg = (uint32_t)(info->gpa -
> + v->domain->arch.vpl011.base_addr);
> struct vpl011 *vpl011 = &v->domain->arch.vpl011;
> struct domain *d = v->domain;
> unsigned long flags;
> @@ -622,10 +625,27 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
> {
> int rc;
> struct vpl011 *vpl011 = &d->arch.vpl011;
> + const struct vuart_info *uart = serial_vuart_info(SERHND_DTUART);
>
> if ( vpl011->backend.dom.ring_buf )
> return -EINVAL;
>
> + vpl011->base_addr = GUEST_PL011_BASE;
> + vpl011->virq = GUEST_VPL011_SPI;
> + if ( is_domain_direct_mapped(d) )
> + {
> + if ( uart != NULL && serial_irq(SERHND_DTUART) > 0 )
> + {
> + vpl011->base_addr = uart->base_addr;
> + vpl011->virq = serial_irq(SERHND_DTUART);
This seems a bit pointless to call serial_irq() twice. How about add a
field in vuart_info to return the interrupt number?
> + }
> + else
> + printk(XENLOG_ERR
> + "Unable to reuse physical UART address and irq for vPL011.\n"
> + "Defaulting to addr %#"PRIpaddr" and IRQ %u\n",
> + vpl011->base_addr, vpl011->virq);
> + }
> +
> /*
> * info is NULL when the backend is in Xen.
> * info is != NULL when the backend is in a domain.
> @@ -661,7 +681,7 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
> }
> }
>
> - rc = vgic_reserve_virq(d, GUEST_VPL011_SPI);
> + rc = vgic_reserve_virq(d, vpl011->virq);
> if ( !rc )
> {
> rc = -EINVAL;
> @@ -673,12 +693,12 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
> spin_lock_init(&vpl011->lock);
>
> register_mmio_handler(d, &vpl011_mmio_handler,
> - GUEST_PL011_BASE, GUEST_PL011_SIZE, NULL);
> + vpl011->base_addr, GUEST_PL011_SIZE, NULL);
So you are making the assumpption that the UART region will be equal to
(or bigger) than GUEST_PL011_SIZE. There are definitely UART out where
the MMIO region is smaller than 4K.
Although, I don't expect them to be passthrough today. So this is
probably fine to assume that the next 4K is free. Can you add some
justification in-code and in the commit message?
>
> return 0;
>
> out2:
> - vgic_free_virq(d, GUEST_VPL011_SPI);
> + vgic_free_virq(d, vpl011->virq);
>
> out1:
> if ( vpl011->backend_in_domain )
> diff --git a/xen/include/asm-arm/vpl011.h b/xen/include/asm-arm/vpl011.h
> index e6c7ab7381..c09abcd7a9 100644
> --- a/xen/include/asm-arm/vpl011.h
> +++ b/xen/include/asm-arm/vpl011.h
> @@ -53,6 +53,8 @@ struct vpl011 {
> uint32_t uarticr; /* Interrupt clear register */
> uint32_t uartris; /* Raw interrupt status register */
> uint32_t shadow_uartmis; /* shadow masked interrupt register */
> + paddr_t base_addr;
> + unsigned int virq;
> spinlock_t lock;
> evtchn_port_t evtchn;
> };
>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 37+ messages in thread* RE: [PATCH 09/11] xen/arm: if 1:1 direct-map domain use native UART address and IRQ number for vPL011
2021-09-23 11:14 ` Julien Grall
@ 2021-10-09 8:47 ` Penny Zheng
2021-10-11 10:49 ` Julien Grall
0 siblings, 1 reply; 37+ messages in thread
From: Penny Zheng @ 2021-10-09 8:47 UTC (permalink / raw)
To: Julien Grall, xen-devel, sstabellini; +Cc: Bertrand Marquis, Wei Chen
Hi Julien
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Thursday, September 23, 2021 7:14 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>
> Subject: Re: [PATCH 09/11] xen/arm: if 1:1 direct-map domain use native
> UART address and IRQ number for vPL011
>
>
>
> On 23/09/2021 08:11, Penny Zheng wrote:
> > From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> >
> > We always use a fix address to map the vPL011 to domains. The address
> > could be a problem for domains that are directly mapped.
> >
> > So, for domains that are directly mapped, reuse the address of the
> > physical UART on the platform to avoid potential clashes.
> >
> > Do the same for the virtual IRQ number: instead of always using
> > GUEST_VPL011_SPI, try to reuse the physical SPI number if possible.
> >
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> > xen/arch/arm/domain_build.c | 34 +++++++++++++++++++++++++++-------
> > xen/arch/arm/vpl011.c | 34 +++++++++++++++++++++++++++-------
> > xen/include/asm-arm/vpl011.h | 2 ++
> > 3 files changed, 56 insertions(+), 14 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 120f1ae575..c92e510ae7 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -30,6 +30,7 @@
> >
> > #include <xen/irq.h>
> > #include <xen/grant_table.h>
> > +#include <xen/serial.h>
> >
> > static unsigned int __initdata opt_dom0_max_vcpus;
> > integer_param("dom0_max_vcpus", opt_dom0_max_vcpus); @@ -1942,8
> > +1943,11 @@ static int __init make_vpl011_uart_node(struct kernel_info
> *kinfo)
> > gic_interrupt_t intr;
> > __be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS];
> > __be32 *cells;
> > + struct domain *d = kinfo->d;
> > + char buf[27];
> >
> > - res = fdt_begin_node(fdt, "sbsa-uart@"__stringify(GUEST_PL011_BASE));
> > + snprintf(buf, sizeof(buf), "sbsa-uart@%"PRIx64, d-
> >arch.vpl011.base_addr);
> > + res = fdt_begin_node(fdt, buf);
> > if ( res )
> > return res;
> >
> > @@ -1953,14 +1957,14 @@ static int __init make_vpl011_uart_node(struct
> > kernel_info *kinfo)
> >
> > cells = ®[0];
> > dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS,
> > - GUEST_ROOT_SIZE_CELLS, GUEST_PL011_BASE,
> > + GUEST_ROOT_SIZE_CELLS,
> > + d->arch.vpl011.base_addr,
> > GUEST_PL011_SIZE);
> >
> > res = fdt_property(fdt, "reg", reg, sizeof(reg));
> > if ( res )
> > return res;
> >
> > - set_interrupt(intr, GUEST_VPL011_SPI, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
> > + set_interrupt(intr, d->arch.vpl011.virq, 0xf,
> > + DT_IRQ_TYPE_LEVEL_HIGH);
> >
> > res = fdt_property(fdt, "interrupts", intr, sizeof (intr));
> > if ( res )
> > @@ -2670,6 +2674,13 @@ static int __init construct_domU(struct domain
> *d,
> > else
> > allocate_static_memory(d, &kinfo, node);
> >
> > + /*
> > + * Initialization before creating its device
> > + * tree node in prepare_dtb_domU.
> > + */
>
> I think it would be better to explain *why* this needs to be done before.
>
> > + if ( kinfo.vpl011 )
> > + rc = domain_vpl011_init(d, NULL);
> > +
> > rc = prepare_dtb_domU(d, &kinfo);
> > if ( rc < 0 )
> > return rc;
> > @@ -2678,9 +2689,6 @@ static int __init construct_domU(struct domain
> *d,
> > if ( rc < 0 )
> > return rc;
> >
> > - if ( kinfo.vpl011 )
> > - rc = domain_vpl011_init(d, NULL);
> > -
> > return rc;
> > }
> >
> > @@ -2723,15 +2731,27 @@ void __init create_domUs(void)
> >
> > if ( !dt_property_read_u32(node, "nr_spis", &d_cfg.arch.nr_spis) )
> > {
> > + unsigned int vpl011_virq = GUEST_VPL011_SPI;
>
> Coding style: Add a newline here.
>
> > d_cfg.arch.nr_spis = gic_number_lines() - 32;
> >
> > + /*
> > + * The VPL011 virq is GUEST_VPL011_SPI, unless direct-map in
> > + * set, in which case we'll try to match the hardware.
> > + *
> > + * Typically, d->arch.vpl011.virq has the vpl011 irq number
> > + * but at this point of the boot sequence it is not
> > + * initialized yet.
> > + */
> > + if ( direct_map && serial_irq(SERHND_DTUART) > 0 )
> > + vpl011_virq = serial_irq(SERHND_DTUART);
>
> I think we should not continue if the domain is direct-mapped *and* the IRQ
> is not found. Otherwise, this will may just result to potential breakage if
> GUEST_VPL011_SPI happens to be used for an HW device.
>
> > +
> > /*
> > * vpl011 uses one emulated SPI. If vpl011 is requested, make
> > * sure that we allocate enough SPIs for it.
> > */
> > if ( dt_property_read_bool(node, "vpl011") )
> > d_cfg.arch.nr_spis = MAX(d_cfg.arch.nr_spis,
> > - GUEST_VPL011_SPI - 32 + 1);
> > + vpl011_virq - 32 + 1);
> > }
> >
> > /*
> > diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c index
> > 895f436cc4..10df25f098 100644
> > --- a/xen/arch/arm/vpl011.c
> > +++ b/xen/arch/arm/vpl011.c
> > @@ -29,6 +29,7 @@
> > #include <xen/mm.h>
> > #include <xen/sched.h>
> > #include <xen/console.h>
> > +#include <xen/serial.h>
> > #include <public/domctl.h>
> > #include <public/io/console.h>
> > #include <asm/pl011-uart.h>
> > @@ -71,11 +72,11 @@ static void vpl011_update_interrupt_status(struct
> domain *d)
> > * status bit has been set since the last time.
> > */
> > if ( uartmis & ~vpl011->shadow_uartmis )
> > - vgic_inject_irq(d, NULL, GUEST_VPL011_SPI, true);
> > + vgic_inject_irq(d, NULL, vpl011->virq, true);
> >
> > vpl011->shadow_uartmis = uartmis;
> > #else
> > - vgic_inject_irq(d, NULL, GUEST_VPL011_SPI, uartmis);
> > + vgic_inject_irq(d, NULL, vpl011->virq, uartmis);
> > #endif
> > }
> >
> > @@ -347,7 +348,8 @@ static int vpl011_mmio_read(struct vcpu *v,
> > void *priv)
> > {
> > struct hsr_dabt dabt = info->dabt;
> > - uint32_t vpl011_reg = (uint32_t)(info->gpa - GUEST_PL011_BASE);
> > + uint32_t vpl011_reg = (uint32_t)(info->gpa -
> > +
> > + v->domain->arch.vpl011.base_addr);
> > struct vpl011 *vpl011 = &v->domain->arch.vpl011;
> > struct domain *d = v->domain;
> > unsigned long flags;
> > @@ -430,7 +432,8 @@ static int vpl011_mmio_write(struct vcpu *v,
> > void *priv)
> > {
> > struct hsr_dabt dabt = info->dabt;
> > - uint32_t vpl011_reg = (uint32_t)(info->gpa - GUEST_PL011_BASE);
> > + uint32_t vpl011_reg = (uint32_t)(info->gpa -
> > +
> > + v->domain->arch.vpl011.base_addr);
> > struct vpl011 *vpl011 = &v->domain->arch.vpl011;
> > struct domain *d = v->domain;
> > unsigned long flags;
> > @@ -622,10 +625,27 @@ int domain_vpl011_init(struct domain *d, struct
> vpl011_init_info *info)
> > {
> > int rc;
> > struct vpl011 *vpl011 = &d->arch.vpl011;
> > + const struct vuart_info *uart = serial_vuart_info(SERHND_DTUART);
> >
> > if ( vpl011->backend.dom.ring_buf )
> > return -EINVAL;
> >
> > + vpl011->base_addr = GUEST_PL011_BASE;
> > + vpl011->virq = GUEST_VPL011_SPI;
> > + if ( is_domain_direct_mapped(d) )
> > + {
> > + if ( uart != NULL && serial_irq(SERHND_DTUART) > 0 )
> > + {
> > + vpl011->base_addr = uart->base_addr;
> > + vpl011->virq = serial_irq(SERHND_DTUART);
>
> This seems a bit pointless to call serial_irq() twice. How about add a field in
> vuart_info to return the interrupt number?
>
> > + }
> > + else
> > + printk(XENLOG_ERR
> > + "Unable to reuse physical UART address and irq for vPL011.\n"
> > + "Defaulting to addr %#"PRIpaddr" and IRQ %u\n",
> > + vpl011->base_addr, vpl011->virq);
> > + }
> > +
> > /*
> > * info is NULL when the backend is in Xen.
> > * info is != NULL when the backend is in a domain.
> > @@ -661,7 +681,7 @@ int domain_vpl011_init(struct domain *d, struct
> vpl011_init_info *info)
> > }
> > }
> >
> > - rc = vgic_reserve_virq(d, GUEST_VPL011_SPI);
> > + rc = vgic_reserve_virq(d, vpl011->virq);
> > if ( !rc )
> > {
> > rc = -EINVAL;
> > @@ -673,12 +693,12 @@ int domain_vpl011_init(struct domain *d, struct
> vpl011_init_info *info)
> > spin_lock_init(&vpl011->lock);
> >
> > register_mmio_handler(d, &vpl011_mmio_handler,
> > - GUEST_PL011_BASE, GUEST_PL011_SIZE, NULL);
> > + vpl011->base_addr, GUEST_PL011_SIZE, NULL);
>
> So you are making the assumpption that the UART region will be equal to (or
> bigger) than GUEST_PL011_SIZE. There are definitely UART out where the
> MMIO region is smaller than 4K.
>
Sorry. I got a few confused here, since I am not very familiar with pl011/UART knowledge.
Problems will occur when UART region is bigger than GUEST_PL011_SIZE, since we
are only considering MMIO region of [vpl011->base_addr, vpl011->base_addr + GUEST_PL011_SIZE], right?
So I shall add the justification like
ASSERT(uart->size <= GUEST_PL011_SIZE);
> Although, I don't expect them to be passthrough today. So this is probably
> fine to assume that the next 4K is free. Can you add some justification in-
> code and in the commit message?
>
> >
> > return 0;
> >
> > out2:
> > - vgic_free_virq(d, GUEST_VPL011_SPI);
> > + vgic_free_virq(d, vpl011->virq);
> >
> > out1:
> > if ( vpl011->backend_in_domain ) diff --git
> > a/xen/include/asm-arm/vpl011.h b/xen/include/asm-arm/vpl011.h index
> > e6c7ab7381..c09abcd7a9 100644
> > --- a/xen/include/asm-arm/vpl011.h
> > +++ b/xen/include/asm-arm/vpl011.h
> > @@ -53,6 +53,8 @@ struct vpl011 {
> > uint32_t uarticr; /* Interrupt clear register */
> > uint32_t uartris; /* Raw interrupt status register */
> > uint32_t shadow_uartmis; /* shadow masked interrupt register */
> > + paddr_t base_addr;
> > + unsigned int virq;
> > spinlock_t lock;
> > evtchn_port_t evtchn;
> > };
> >
>
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 09/11] xen/arm: if 1:1 direct-map domain use native UART address and IRQ number for vPL011
2021-10-09 8:47 ` Penny Zheng
@ 2021-10-11 10:49 ` Julien Grall
2021-10-12 2:42 ` Penny Zheng
0 siblings, 1 reply; 37+ messages in thread
From: Julien Grall @ 2021-10-11 10:49 UTC (permalink / raw)
To: Penny Zheng, xen-devel, sstabellini; +Cc: Bertrand Marquis, Wei Chen
On 09/10/2021 09:47, Penny Zheng wrote:
> Hi Julien
Hi Penny,
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: Thursday, September 23, 2021 7:14 PM
>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
>> sstabellini@kernel.org
>> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
>> <Wei.Chen@arm.com>
>> Subject: Re: [PATCH 09/11] xen/arm: if 1:1 direct-map domain use native
>> UART address and IRQ number for vPL011
>>
>>
>>
>> On 23/09/2021 08:11, Penny Zheng wrote:
>>> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
>>>
>>> We always use a fix address to map the vPL011 to domains. The address
>>> could be a problem for domains that are directly mapped.
>>>
>>> So, for domains that are directly mapped, reuse the address of the
>>> physical UART on the platform to avoid potential clashes.
>>>
>>> Do the same for the virtual IRQ number: instead of always using
>>> GUEST_VPL011_SPI, try to reuse the physical SPI number if possible.
>>>
>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
>>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
>>> ---
>>> xen/arch/arm/domain_build.c | 34 +++++++++++++++++++++++++++-------
>>> xen/arch/arm/vpl011.c | 34 +++++++++++++++++++++++++++-------
>>> xen/include/asm-arm/vpl011.h | 2 ++
>>> 3 files changed, 56 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index 120f1ae575..c92e510ae7 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -30,6 +30,7 @@
>>>
>>> #include <xen/irq.h>
>>> #include <xen/grant_table.h>
>>> +#include <xen/serial.h>
>>>
>>> static unsigned int __initdata opt_dom0_max_vcpus;
>>> integer_param("dom0_max_vcpus", opt_dom0_max_vcpus); @@ -1942,8
>>> +1943,11 @@ static int __init make_vpl011_uart_node(struct kernel_info
>> *kinfo)
>>> gic_interrupt_t intr;
>>> __be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS];
>>> __be32 *cells;
>>> + struct domain *d = kinfo->d;
>>> + char buf[27];
>>>
>>> - res = fdt_begin_node(fdt, "sbsa-uart@"__stringify(GUEST_PL011_BASE));
>>> + snprintf(buf, sizeof(buf), "sbsa-uart@%"PRIx64, d-
>>> arch.vpl011.base_addr);
>>> + res = fdt_begin_node(fdt, buf);
>>> if ( res )
>>> return res;
>>>
>>> @@ -1953,14 +1957,14 @@ static int __init make_vpl011_uart_node(struct
>>> kernel_info *kinfo)
>>>
>>> cells = ®[0];
>>> dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS,
>>> - GUEST_ROOT_SIZE_CELLS, GUEST_PL011_BASE,
>>> + GUEST_ROOT_SIZE_CELLS,
>>> + d->arch.vpl011.base_addr,
>>> GUEST_PL011_SIZE);
>>>
>>> res = fdt_property(fdt, "reg", reg, sizeof(reg));
>>> if ( res )
>>> return res;
>>>
>>> - set_interrupt(intr, GUEST_VPL011_SPI, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
>>> + set_interrupt(intr, d->arch.vpl011.virq, 0xf,
>>> + DT_IRQ_TYPE_LEVEL_HIGH);
>>>
>>> res = fdt_property(fdt, "interrupts", intr, sizeof (intr));
>>> if ( res )
>>> @@ -2670,6 +2674,13 @@ static int __init construct_domU(struct domain
>> *d,
>>> else
>>> allocate_static_memory(d, &kinfo, node);
>>>
>>> + /*
>>> + * Initialization before creating its device
>>> + * tree node in prepare_dtb_domU.
>>> + */
>>
>> I think it would be better to explain *why* this needs to be done before.
>>
>>> + if ( kinfo.vpl011 )
>>> + rc = domain_vpl011_init(d, NULL);
>>> +
>>> rc = prepare_dtb_domU(d, &kinfo);
>>> if ( rc < 0 )
>>> return rc;
>>> @@ -2678,9 +2689,6 @@ static int __init construct_domU(struct domain
>> *d,
>>> if ( rc < 0 )
>>> return rc;
>>>
>>> - if ( kinfo.vpl011 )
>>> - rc = domain_vpl011_init(d, NULL);
>>> -
>>> return rc;
>>> }
>>>
>>> @@ -2723,15 +2731,27 @@ void __init create_domUs(void)
>>>
>>> if ( !dt_property_read_u32(node, "nr_spis", &d_cfg.arch.nr_spis) )
>>> {
>>> + unsigned int vpl011_virq = GUEST_VPL011_SPI;
>>
>> Coding style: Add a newline here.
>>
>>> d_cfg.arch.nr_spis = gic_number_lines() - 32;
>>>
>>> + /*
>>> + * The VPL011 virq is GUEST_VPL011_SPI, unless direct-map in
>>> + * set, in which case we'll try to match the hardware.
>>> + *
>>> + * Typically, d->arch.vpl011.virq has the vpl011 irq number
>>> + * but at this point of the boot sequence it is not
>>> + * initialized yet.
>>> + */
>>> + if ( direct_map && serial_irq(SERHND_DTUART) > 0 )
>>> + vpl011_virq = serial_irq(SERHND_DTUART);
>>
>> I think we should not continue if the domain is direct-mapped *and* the IRQ
>> is not found. Otherwise, this will may just result to potential breakage if
>> GUEST_VPL011_SPI happens to be used for an HW device.
>>
>>> +
>>> /*
>>> * vpl011 uses one emulated SPI. If vpl011 is requested, make
>>> * sure that we allocate enough SPIs for it.
>>> */
>>> if ( dt_property_read_bool(node, "vpl011") )
>>> d_cfg.arch.nr_spis = MAX(d_cfg.arch.nr_spis,
>>> - GUEST_VPL011_SPI - 32 + 1);
>>> + vpl011_virq - 32 + 1);
>>> }
>>>
>>> /*
>>> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c index
>>> 895f436cc4..10df25f098 100644
>>> --- a/xen/arch/arm/vpl011.c
>>> +++ b/xen/arch/arm/vpl011.c
>>> @@ -29,6 +29,7 @@
>>> #include <xen/mm.h>
>>> #include <xen/sched.h>
>>> #include <xen/console.h>
>>> +#include <xen/serial.h>
>>> #include <public/domctl.h>
>>> #include <public/io/console.h>
>>> #include <asm/pl011-uart.h>
>>> @@ -71,11 +72,11 @@ static void vpl011_update_interrupt_status(struct
>> domain *d)
>>> * status bit has been set since the last time.
>>> */
>>> if ( uartmis & ~vpl011->shadow_uartmis )
>>> - vgic_inject_irq(d, NULL, GUEST_VPL011_SPI, true);
>>> + vgic_inject_irq(d, NULL, vpl011->virq, true);
>>>
>>> vpl011->shadow_uartmis = uartmis;
>>> #else
>>> - vgic_inject_irq(d, NULL, GUEST_VPL011_SPI, uartmis);
>>> + vgic_inject_irq(d, NULL, vpl011->virq, uartmis);
>>> #endif
>>> }
>>>
>>> @@ -347,7 +348,8 @@ static int vpl011_mmio_read(struct vcpu *v,
>>> void *priv)
>>> {
>>> struct hsr_dabt dabt = info->dabt;
>>> - uint32_t vpl011_reg = (uint32_t)(info->gpa - GUEST_PL011_BASE);
>>> + uint32_t vpl011_reg = (uint32_t)(info->gpa -
>>> +
>>> + v->domain->arch.vpl011.base_addr);
>>> struct vpl011 *vpl011 = &v->domain->arch.vpl011;
>>> struct domain *d = v->domain;
>>> unsigned long flags;
>>> @@ -430,7 +432,8 @@ static int vpl011_mmio_write(struct vcpu *v,
>>> void *priv)
>>> {
>>> struct hsr_dabt dabt = info->dabt;
>>> - uint32_t vpl011_reg = (uint32_t)(info->gpa - GUEST_PL011_BASE);
>>> + uint32_t vpl011_reg = (uint32_t)(info->gpa -
>>> +
>>> + v->domain->arch.vpl011.base_addr);
>>> struct vpl011 *vpl011 = &v->domain->arch.vpl011;
>>> struct domain *d = v->domain;
>>> unsigned long flags;
>>> @@ -622,10 +625,27 @@ int domain_vpl011_init(struct domain *d, struct
>> vpl011_init_info *info)
>>> {
>>> int rc;
>>> struct vpl011 *vpl011 = &d->arch.vpl011;
>>> + const struct vuart_info *uart = serial_vuart_info(SERHND_DTUART);
>>>
>>> if ( vpl011->backend.dom.ring_buf )
>>> return -EINVAL;
>>>
>>> + vpl011->base_addr = GUEST_PL011_BASE;
>>> + vpl011->virq = GUEST_VPL011_SPI;
>>> + if ( is_domain_direct_mapped(d) )
>>> + {
>>> + if ( uart != NULL && serial_irq(SERHND_DTUART) > 0 )
>>> + {
>>> + vpl011->base_addr = uart->base_addr;
>>> + vpl011->virq = serial_irq(SERHND_DTUART);
>>
>> This seems a bit pointless to call serial_irq() twice. How about add a field in
>> vuart_info to return the interrupt number?
>>
>>> + }
>>> + else
>>> + printk(XENLOG_ERR
>>> + "Unable to reuse physical UART address and irq for vPL011.\n"
>>> + "Defaulting to addr %#"PRIpaddr" and IRQ %u\n",
>>> + vpl011->base_addr, vpl011->virq);
>>> + }
>>> +
>>> /*
>>> * info is NULL when the backend is in Xen.
>>> * info is != NULL when the backend is in a domain.
>>> @@ -661,7 +681,7 @@ int domain_vpl011_init(struct domain *d, struct
>> vpl011_init_info *info)
>>> }
>>> }
>>>
>>> - rc = vgic_reserve_virq(d, GUEST_VPL011_SPI);
>>> + rc = vgic_reserve_virq(d, vpl011->virq);
>>> if ( !rc )
>>> {
>>> rc = -EINVAL;
>>> @@ -673,12 +693,12 @@ int domain_vpl011_init(struct domain *d, struct
>> vpl011_init_info *info)
>>> spin_lock_init(&vpl011->lock);
>>>
>>> register_mmio_handler(d, &vpl011_mmio_handler,
>>> - GUEST_PL011_BASE, GUEST_PL011_SIZE, NULL);
>>> + vpl011->base_addr, GUEST_PL011_SIZE, NULL);
>>
>> So you are making the assumpption that the UART region will be equal to (or
>> bigger) than GUEST_PL011_SIZE. There are definitely UART out where the
>> MMIO region is smaller than 4K.
>>
>
> Sorry. I got a few confused here, since I am not very familiar with pl011/UART knowledge.
>
> Problems will occur when UART region is bigger than GUEST_PL011_SIZE, since we
> are only considering MMIO region of [vpl011->base_addr, vpl011->base_addr + GUEST_PL011_SIZE], right?
It is in fact the other way around. The problem will appear if the host
UART MMIO region is smaller than the one we will emulate for the guest
PL011.
>
> So I shall add the justification like
> ASSERT(uart->size <= GUEST_PL011_SIZE);
I think this would want to be a proper check so distro users would get
an error if they are trying to use this feature on such platform.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 37+ messages in thread* RE: [PATCH 09/11] xen/arm: if 1:1 direct-map domain use native UART address and IRQ number for vPL011
2021-10-11 10:49 ` Julien Grall
@ 2021-10-12 2:42 ` Penny Zheng
2021-10-13 18:00 ` Julien Grall
0 siblings, 1 reply; 37+ messages in thread
From: Penny Zheng @ 2021-10-12 2:42 UTC (permalink / raw)
To: Julien Grall, xen-devel, sstabellini; +Cc: Bertrand Marquis, Wei Chen
Hi Julien
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Monday, October 11, 2021 6:49 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>
> Subject: Re: [PATCH 09/11] xen/arm: if 1:1 direct-map domain use native UART
> address and IRQ number for vPL011
>
> On 09/10/2021 09:47, Penny Zheng wrote:
> > Hi Julien
>
> Hi Penny,
>
> >> -----Original Message-----
> >> From: Julien Grall <julien@xen.org>
> >> Sent: Thursday, September 23, 2021 7:14 PM
> >> To: Penny Zheng <Penny.Zheng@arm.com>;
> >> xen-devel@lists.xenproject.org; sstabellini@kernel.org
> >> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> >> <Wei.Chen@arm.com>
> >> Subject: Re: [PATCH 09/11] xen/arm: if 1:1 direct-map domain use
> >> native UART address and IRQ number for vPL011
> >>
> >>
> >>
> >> On 23/09/2021 08:11, Penny Zheng wrote:
> >>> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> >>>
> >>> We always use a fix address to map the vPL011 to domains. The
> >>> address could be a problem for domains that are directly mapped.
> >>>
> >>> So, for domains that are directly mapped, reuse the address of the
> >>> physical UART on the platform to avoid potential clashes.
> >>>
> >>> Do the same for the virtual IRQ number: instead of always using
> >>> GUEST_VPL011_SPI, try to reuse the physical SPI number if possible.
> >>>
> >>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> >>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> >>> ---
> >>> xen/arch/arm/domain_build.c | 34 +++++++++++++++++++++++++++-----
> --
> >>> xen/arch/arm/vpl011.c | 34 +++++++++++++++++++++++++++-------
> >>> xen/include/asm-arm/vpl011.h | 2 ++
> >>> 3 files changed, 56 insertions(+), 14 deletions(-)
> >>>
> >>> diff --git a/xen/arch/arm/domain_build.c
> >>> b/xen/arch/arm/domain_build.c index 120f1ae575..c92e510ae7 100644
> >>> --- a/xen/arch/arm/domain_build.c
> >>> +++ b/xen/arch/arm/domain_build.c
> >>> @@ -30,6 +30,7 @@
> >>>
> >>> #include <xen/irq.h>
> >>> #include <xen/grant_table.h>
> >>> +#include <xen/serial.h>
> >>>
> >>> static unsigned int __initdata opt_dom0_max_vcpus;
> >>> integer_param("dom0_max_vcpus", opt_dom0_max_vcpus); @@ -
> 1942,8
> >>> +1943,11 @@ static int __init make_vpl011_uart_node(struct
> >>> +kernel_info
> >> *kinfo)
> >>> gic_interrupt_t intr;
> >>> __be32 reg[GUEST_ROOT_ADDRESS_CELLS +
> GUEST_ROOT_SIZE_CELLS];
> >>> __be32 *cells;
> >>> + struct domain *d = kinfo->d;
> >>> + char buf[27];
> >>>
> >>> - res = fdt_begin_node(fdt, "sbsa-
> uart@"__stringify(GUEST_PL011_BASE));
> >>> + snprintf(buf, sizeof(buf), "sbsa-uart@%"PRIx64, d-
> >>> arch.vpl011.base_addr);
> >>> + res = fdt_begin_node(fdt, buf);
> >>> if ( res )
> >>> return res;
> >>>
> >>> @@ -1953,14 +1957,14 @@ static int __init
> >>> make_vpl011_uart_node(struct kernel_info *kinfo)
> >>>
> >>> cells = ®[0];
> >>> dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS,
> >>> - GUEST_ROOT_SIZE_CELLS, GUEST_PL011_BASE,
> >>> + GUEST_ROOT_SIZE_CELLS,
> >>> + d->arch.vpl011.base_addr,
> >>> GUEST_PL011_SIZE);
> >>>
> >>> res = fdt_property(fdt, "reg", reg, sizeof(reg));
> >>> if ( res )
> >>> return res;
> >>>
> >>> - set_interrupt(intr, GUEST_VPL011_SPI, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
> >>> + set_interrupt(intr, d->arch.vpl011.virq, 0xf,
> >>> + DT_IRQ_TYPE_LEVEL_HIGH);
> >>>
> >>> res = fdt_property(fdt, "interrupts", intr, sizeof (intr));
> >>> if ( res )
> >>> @@ -2670,6 +2674,13 @@ static int __init construct_domU(struct
> >>> domain
> >> *d,
> >>> else
> >>> allocate_static_memory(d, &kinfo, node);
> >>>
> >>> + /*
> >>> + * Initialization before creating its device
> >>> + * tree node in prepare_dtb_domU.
> >>> + */
> >>
> >> I think it would be better to explain *why* this needs to be done before.
> >>
> >>> + if ( kinfo.vpl011 )
> >>> + rc = domain_vpl011_init(d, NULL);
> >>> +
> >>> rc = prepare_dtb_domU(d, &kinfo);
> >>> if ( rc < 0 )
> >>> return rc;
> >>> @@ -2678,9 +2689,6 @@ static int __init construct_domU(struct domain
> >> *d,
> >>> if ( rc < 0 )
> >>> return rc;
> >>>
> >>> - if ( kinfo.vpl011 )
> >>> - rc = domain_vpl011_init(d, NULL);
> >>> -
> >>> return rc;
> >>> }
> >>>
> >>> @@ -2723,15 +2731,27 @@ void __init create_domUs(void)
> >>>
> >>> if ( !dt_property_read_u32(node, "nr_spis", &d_cfg.arch.nr_spis) )
> >>> {
> >>> + unsigned int vpl011_virq = GUEST_VPL011_SPI;
> >>
> >> Coding style: Add a newline here.
> >>
> >>> d_cfg.arch.nr_spis = gic_number_lines() - 32;
> >>>
> >>> + /*
> >>> + * The VPL011 virq is GUEST_VPL011_SPI, unless direct-map in
> >>> + * set, in which case we'll try to match the hardware.
> >>> + *
> >>> + * Typically, d->arch.vpl011.virq has the vpl011 irq number
> >>> + * but at this point of the boot sequence it is not
> >>> + * initialized yet.
> >>> + */
> >>> + if ( direct_map && serial_irq(SERHND_DTUART) > 0 )
> >>> + vpl011_virq = serial_irq(SERHND_DTUART);
> >>
> >> I think we should not continue if the domain is direct-mapped *and*
> >> the IRQ is not found. Otherwise, this will may just result to
> >> potential breakage if GUEST_VPL011_SPI happens to be used for an HW
> device.
> >>
> >>> +
> >>> /*
> >>> * vpl011 uses one emulated SPI. If vpl011 is requested, make
> >>> * sure that we allocate enough SPIs for it.
> >>> */
> >>> if ( dt_property_read_bool(node, "vpl011") )
> >>> d_cfg.arch.nr_spis = MAX(d_cfg.arch.nr_spis,
> >>> - GUEST_VPL011_SPI - 32 + 1);
> >>> + vpl011_virq - 32 + 1);
> >>> }
> >>>
> >>> /*
> >>> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c index
> >>> 895f436cc4..10df25f098 100644
> >>> --- a/xen/arch/arm/vpl011.c
> >>> +++ b/xen/arch/arm/vpl011.c
> >>> @@ -29,6 +29,7 @@
> >>> #include <xen/mm.h>
> >>> #include <xen/sched.h>
> >>> #include <xen/console.h>
> >>> +#include <xen/serial.h>
> >>> #include <public/domctl.h>
> >>> #include <public/io/console.h>
> >>> #include <asm/pl011-uart.h>
> >>> @@ -71,11 +72,11 @@ static void
> >>> vpl011_update_interrupt_status(struct
> >> domain *d)
> >>> * status bit has been set since the last time.
> >>> */
> >>> if ( uartmis & ~vpl011->shadow_uartmis )
> >>> - vgic_inject_irq(d, NULL, GUEST_VPL011_SPI, true);
> >>> + vgic_inject_irq(d, NULL, vpl011->virq, true);
> >>>
> >>> vpl011->shadow_uartmis = uartmis;
> >>> #else
> >>> - vgic_inject_irq(d, NULL, GUEST_VPL011_SPI, uartmis);
> >>> + vgic_inject_irq(d, NULL, vpl011->virq, uartmis);
> >>> #endif
> >>> }
> >>>
> >>> @@ -347,7 +348,8 @@ static int vpl011_mmio_read(struct vcpu *v,
> >>> void *priv)
> >>> {
> >>> struct hsr_dabt dabt = info->dabt;
> >>> - uint32_t vpl011_reg = (uint32_t)(info->gpa - GUEST_PL011_BASE);
> >>> + uint32_t vpl011_reg = (uint32_t)(info->gpa -
> >>> +
> >>> + v->domain->arch.vpl011.base_addr);
> >>> struct vpl011 *vpl011 = &v->domain->arch.vpl011;
> >>> struct domain *d = v->domain;
> >>> unsigned long flags;
> >>> @@ -430,7 +432,8 @@ static int vpl011_mmio_write(struct vcpu *v,
> >>> void *priv)
> >>> {
> >>> struct hsr_dabt dabt = info->dabt;
> >>> - uint32_t vpl011_reg = (uint32_t)(info->gpa - GUEST_PL011_BASE);
> >>> + uint32_t vpl011_reg = (uint32_t)(info->gpa -
> >>> +
> >>> + v->domain->arch.vpl011.base_addr);
> >>> struct vpl011 *vpl011 = &v->domain->arch.vpl011;
> >>> struct domain *d = v->domain;
> >>> unsigned long flags;
> >>> @@ -622,10 +625,27 @@ int domain_vpl011_init(struct domain *d,
> >>> struct
> >> vpl011_init_info *info)
> >>> {
> >>> int rc;
> >>> struct vpl011 *vpl011 = &d->arch.vpl011;
> >>> + const struct vuart_info *uart =
> >>> + serial_vuart_info(SERHND_DTUART);
> >>>
> >>> if ( vpl011->backend.dom.ring_buf )
> >>> return -EINVAL;
> >>>
> >>> + vpl011->base_addr = GUEST_PL011_BASE;
> >>> + vpl011->virq = GUEST_VPL011_SPI;
> >>> + if ( is_domain_direct_mapped(d) )
> >>> + {
> >>> + if ( uart != NULL && serial_irq(SERHND_DTUART) > 0 )
> >>> + {
> >>> + vpl011->base_addr = uart->base_addr;
> >>> + vpl011->virq = serial_irq(SERHND_DTUART);
> >>
> >> This seems a bit pointless to call serial_irq() twice. How about add
> >> a field in vuart_info to return the interrupt number?
> >>
> >>> + }
> >>> + else
> >>> + printk(XENLOG_ERR
> >>> + "Unable to reuse physical UART address and irq for vPL011.\n"
> >>> + "Defaulting to addr %#"PRIpaddr" and IRQ %u\n",
> >>> + vpl011->base_addr, vpl011->virq);
> >>> + }
> >>> +
> >>> /*
> >>> * info is NULL when the backend is in Xen.
> >>> * info is != NULL when the backend is in a domain.
> >>> @@ -661,7 +681,7 @@ int domain_vpl011_init(struct domain *d, struct
> >> vpl011_init_info *info)
> >>> }
> >>> }
> >>>
> >>> - rc = vgic_reserve_virq(d, GUEST_VPL011_SPI);
> >>> + rc = vgic_reserve_virq(d, vpl011->virq);
> >>> if ( !rc )
> >>> {
> >>> rc = -EINVAL;
> >>> @@ -673,12 +693,12 @@ int domain_vpl011_init(struct domain *d,
> >>> struct
> >> vpl011_init_info *info)
> >>> spin_lock_init(&vpl011->lock);
> >>>
> >>> register_mmio_handler(d, &vpl011_mmio_handler,
> >>> - GUEST_PL011_BASE, GUEST_PL011_SIZE, NULL);
> >>> + vpl011->base_addr, GUEST_PL011_SIZE,
> >>> + NULL);
> >>
> >> So you are making the assumpption that the UART region will be equal
> >> to (or
> >> bigger) than GUEST_PL011_SIZE. There are definitely UART out where
> >> the MMIO region is smaller than 4K.
> >>
> >
> > Sorry. I got a few confused here, since I am not very familiar with pl011/UART
> knowledge.
> >
> > Problems will occur when UART region is bigger than GUEST_PL011_SIZE,
> > since we are only considering MMIO region of [vpl011->base_addr, vpl011-
> >base_addr + GUEST_PL011_SIZE], right?
>
> It is in fact the other way around. The problem will appear if the host UART
> MMIO region is smaller than the one we will emulate for the guest PL011.
>
Sorry to keep bothering.
Is it that because when the UART MMIO region is smaller than the one we emulated,
register(DR, RSR, FR, ...) will not be at the place where we emulated?
> >
> > So I shall add the justification like
> > ASSERT(uart->size <= GUEST_PL011_SIZE);
>
> I think this would want to be a proper check so distro users would get an error
> if they are trying to use this feature on such platform.
>
Sure, I’ll add the length check.
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 09/11] xen/arm: if 1:1 direct-map domain use native UART address and IRQ number for vPL011
2021-10-12 2:42 ` Penny Zheng
@ 2021-10-13 18:00 ` Julien Grall
2021-10-14 2:31 ` Penny Zheng
0 siblings, 1 reply; 37+ messages in thread
From: Julien Grall @ 2021-10-13 18:00 UTC (permalink / raw)
To: Penny Zheng, xen-devel, sstabellini; +Cc: Bertrand Marquis, Wei Chen
On 12/10/2021 03:42, Penny Zheng wrote:
> Hi Julien
Hi Penny,
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: Monday, October 11, 2021 6:49 PM
>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
>> sstabellini@kernel.org
>> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
>> <Wei.Chen@arm.com>
>> Subject: Re: [PATCH 09/11] xen/arm: if 1:1 direct-map domain use native UART
>> address and IRQ number for vPL011
>>
>> On 09/10/2021 09:47, Penny Zheng wrote:
>>> Hi Julien
>>
>> Hi Penny,
>>
>>>> -----Original Message-----
>>>> From: Julien Grall <julien@xen.org>
>>>> Sent: Thursday, September 23, 2021 7:14 PM
>>>> To: Penny Zheng <Penny.Zheng@arm.com>;
>>>> xen-devel@lists.xenproject.org; sstabellini@kernel.org
>>>> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
>>>> <Wei.Chen@arm.com>
>>>> Subject: Re: [PATCH 09/11] xen/arm: if 1:1 direct-map domain use
>>>> native UART address and IRQ number for vPL011
>>>>
>>>>
>>>>
>>>> On 23/09/2021 08:11, Penny Zheng wrote:
>>>>> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
>>>>>
>>>>> We always use a fix address to map the vPL011 to domains. The
>>>>> address could be a problem for domains that are directly mapped.
>>>>>
>>>>> So, for domains that are directly mapped, reuse the address of the
>>>>> physical UART on the platform to avoid potential clashes.
>>>>>
>>>>> Do the same for the virtual IRQ number: instead of always using
>>>>> GUEST_VPL011_SPI, try to reuse the physical SPI number if possible.
>>>>>
>>>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
>>>>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
>>>>> ---
>>>>> xen/arch/arm/domain_build.c | 34 +++++++++++++++++++++++++++-----
>> --
>>>>> xen/arch/arm/vpl011.c | 34 +++++++++++++++++++++++++++-------
>>>>> xen/include/asm-arm/vpl011.h | 2 ++
>>>>> 3 files changed, 56 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/arm/domain_build.c
>>>>> b/xen/arch/arm/domain_build.c index 120f1ae575..c92e510ae7 100644
>>>>> --- a/xen/arch/arm/domain_build.c
>>>>> +++ b/xen/arch/arm/domain_build.c
>>>>> @@ -30,6 +30,7 @@
>>>>>
>>>>> #include <xen/irq.h>
>>>>> #include <xen/grant_table.h>
>>>>> +#include <xen/serial.h>
>>>>>
>>>>> static unsigned int __initdata opt_dom0_max_vcpus;
>>>>> integer_param("dom0_max_vcpus", opt_dom0_max_vcpus); @@ -
>> 1942,8
>>>>> +1943,11 @@ static int __init make_vpl011_uart_node(struct
>>>>> +kernel_info
>>>> *kinfo)
>>>>> gic_interrupt_t intr;
>>>>> __be32 reg[GUEST_ROOT_ADDRESS_CELLS +
>> GUEST_ROOT_SIZE_CELLS];
>>>>> __be32 *cells;
>>>>> + struct domain *d = kinfo->d;
>>>>> + char buf[27];
>>>>>
>>>>> - res = fdt_begin_node(fdt, "sbsa-
>> uart@"__stringify(GUEST_PL011_BASE));
>>>>> + snprintf(buf, sizeof(buf), "sbsa-uart@%"PRIx64, d-
>>>>> arch.vpl011.base_addr);
>>>>> + res = fdt_begin_node(fdt, buf);
>>>>> if ( res )
>>>>> return res;
>>>>>
>>>>> @@ -1953,14 +1957,14 @@ static int __init
>>>>> make_vpl011_uart_node(struct kernel_info *kinfo)
>>>>>
>>>>> cells = ®[0];
>>>>> dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS,
>>>>> - GUEST_ROOT_SIZE_CELLS, GUEST_PL011_BASE,
>>>>> + GUEST_ROOT_SIZE_CELLS,
>>>>> + d->arch.vpl011.base_addr,
>>>>> GUEST_PL011_SIZE);
>>>>>
>>>>> res = fdt_property(fdt, "reg", reg, sizeof(reg));
>>>>> if ( res )
>>>>> return res;
>>>>>
>>>>> - set_interrupt(intr, GUEST_VPL011_SPI, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
>>>>> + set_interrupt(intr, d->arch.vpl011.virq, 0xf,
>>>>> + DT_IRQ_TYPE_LEVEL_HIGH);
>>>>>
>>>>> res = fdt_property(fdt, "interrupts", intr, sizeof (intr));
>>>>> if ( res )
>>>>> @@ -2670,6 +2674,13 @@ static int __init construct_domU(struct
>>>>> domain
>>>> *d,
>>>>> else
>>>>> allocate_static_memory(d, &kinfo, node);
>>>>>
>>>>> + /*
>>>>> + * Initialization before creating its device
>>>>> + * tree node in prepare_dtb_domU.
>>>>> + */
>>>>
>>>> I think it would be better to explain *why* this needs to be done before.
>>>>
>>>>> + if ( kinfo.vpl011 )
>>>>> + rc = domain_vpl011_init(d, NULL);
>>>>> +
>>>>> rc = prepare_dtb_domU(d, &kinfo);
>>>>> if ( rc < 0 )
>>>>> return rc;
>>>>> @@ -2678,9 +2689,6 @@ static int __init construct_domU(struct domain
>>>> *d,
>>>>> if ( rc < 0 )
>>>>> return rc;
>>>>>
>>>>> - if ( kinfo.vpl011 )
>>>>> - rc = domain_vpl011_init(d, NULL);
>>>>> -
>>>>> return rc;
>>>>> }
>>>>>
>>>>> @@ -2723,15 +2731,27 @@ void __init create_domUs(void)
>>>>>
>>>>> if ( !dt_property_read_u32(node, "nr_spis", &d_cfg.arch.nr_spis) )
>>>>> {
>>>>> + unsigned int vpl011_virq = GUEST_VPL011_SPI;
>>>>
>>>> Coding style: Add a newline here.
>>>>
>>>>> d_cfg.arch.nr_spis = gic_number_lines() - 32;
>>>>>
>>>>> + /*
>>>>> + * The VPL011 virq is GUEST_VPL011_SPI, unless direct-map in
>>>>> + * set, in which case we'll try to match the hardware.
>>>>> + *
>>>>> + * Typically, d->arch.vpl011.virq has the vpl011 irq number
>>>>> + * but at this point of the boot sequence it is not
>>>>> + * initialized yet.
>>>>> + */
>>>>> + if ( direct_map && serial_irq(SERHND_DTUART) > 0 )
>>>>> + vpl011_virq = serial_irq(SERHND_DTUART);
>>>>
>>>> I think we should not continue if the domain is direct-mapped *and*
>>>> the IRQ is not found. Otherwise, this will may just result to
>>>> potential breakage if GUEST_VPL011_SPI happens to be used for an HW
>> device.
>>>>
>>>>> +
>>>>> /*
>>>>> * vpl011 uses one emulated SPI. If vpl011 is requested, make
>>>>> * sure that we allocate enough SPIs for it.
>>>>> */
>>>>> if ( dt_property_read_bool(node, "vpl011") )
>>>>> d_cfg.arch.nr_spis = MAX(d_cfg.arch.nr_spis,
>>>>> - GUEST_VPL011_SPI - 32 + 1);
>>>>> + vpl011_virq - 32 + 1);
>>>>> }
>>>>>
>>>>> /*
>>>>> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c index
>>>>> 895f436cc4..10df25f098 100644
>>>>> --- a/xen/arch/arm/vpl011.c
>>>>> +++ b/xen/arch/arm/vpl011.c
>>>>> @@ -29,6 +29,7 @@
>>>>> #include <xen/mm.h>
>>>>> #include <xen/sched.h>
>>>>> #include <xen/console.h>
>>>>> +#include <xen/serial.h>
>>>>> #include <public/domctl.h>
>>>>> #include <public/io/console.h>
>>>>> #include <asm/pl011-uart.h>
>>>>> @@ -71,11 +72,11 @@ static void
>>>>> vpl011_update_interrupt_status(struct
>>>> domain *d)
>>>>> * status bit has been set since the last time.
>>>>> */
>>>>> if ( uartmis & ~vpl011->shadow_uartmis )
>>>>> - vgic_inject_irq(d, NULL, GUEST_VPL011_SPI, true);
>>>>> + vgic_inject_irq(d, NULL, vpl011->virq, true);
>>>>>
>>>>> vpl011->shadow_uartmis = uartmis;
>>>>> #else
>>>>> - vgic_inject_irq(d, NULL, GUEST_VPL011_SPI, uartmis);
>>>>> + vgic_inject_irq(d, NULL, vpl011->virq, uartmis);
>>>>> #endif
>>>>> }
>>>>>
>>>>> @@ -347,7 +348,8 @@ static int vpl011_mmio_read(struct vcpu *v,
>>>>> void *priv)
>>>>> {
>>>>> struct hsr_dabt dabt = info->dabt;
>>>>> - uint32_t vpl011_reg = (uint32_t)(info->gpa - GUEST_PL011_BASE);
>>>>> + uint32_t vpl011_reg = (uint32_t)(info->gpa -
>>>>> +
>>>>> + v->domain->arch.vpl011.base_addr);
>>>>> struct vpl011 *vpl011 = &v->domain->arch.vpl011;
>>>>> struct domain *d = v->domain;
>>>>> unsigned long flags;
>>>>> @@ -430,7 +432,8 @@ static int vpl011_mmio_write(struct vcpu *v,
>>>>> void *priv)
>>>>> {
>>>>> struct hsr_dabt dabt = info->dabt;
>>>>> - uint32_t vpl011_reg = (uint32_t)(info->gpa - GUEST_PL011_BASE);
>>>>> + uint32_t vpl011_reg = (uint32_t)(info->gpa -
>>>>> +
>>>>> + v->domain->arch.vpl011.base_addr);
>>>>> struct vpl011 *vpl011 = &v->domain->arch.vpl011;
>>>>> struct domain *d = v->domain;
>>>>> unsigned long flags;
>>>>> @@ -622,10 +625,27 @@ int domain_vpl011_init(struct domain *d,
>>>>> struct
>>>> vpl011_init_info *info)
>>>>> {
>>>>> int rc;
>>>>> struct vpl011 *vpl011 = &d->arch.vpl011;
>>>>> + const struct vuart_info *uart =
>>>>> + serial_vuart_info(SERHND_DTUART);
>>>>>
>>>>> if ( vpl011->backend.dom.ring_buf )
>>>>> return -EINVAL;
>>>>>
>>>>> + vpl011->base_addr = GUEST_PL011_BASE;
>>>>> + vpl011->virq = GUEST_VPL011_SPI;
>>>>> + if ( is_domain_direct_mapped(d) )
>>>>> + {
>>>>> + if ( uart != NULL && serial_irq(SERHND_DTUART) > 0 )
>>>>> + {
>>>>> + vpl011->base_addr = uart->base_addr;
>>>>> + vpl011->virq = serial_irq(SERHND_DTUART);
>>>>
>>>> This seems a bit pointless to call serial_irq() twice. How about add
>>>> a field in vuart_info to return the interrupt number?
>>>>
>>>>> + }
>>>>> + else
>>>>> + printk(XENLOG_ERR
>>>>> + "Unable to reuse physical UART address and irq for vPL011.\n"
>>>>> + "Defaulting to addr %#"PRIpaddr" and IRQ %u\n",
>>>>> + vpl011->base_addr, vpl011->virq);
>>>>> + }
>>>>> +
>>>>> /*
>>>>> * info is NULL when the backend is in Xen.
>>>>> * info is != NULL when the backend is in a domain.
>>>>> @@ -661,7 +681,7 @@ int domain_vpl011_init(struct domain *d, struct
>>>> vpl011_init_info *info)
>>>>> }
>>>>> }
>>>>>
>>>>> - rc = vgic_reserve_virq(d, GUEST_VPL011_SPI);
>>>>> + rc = vgic_reserve_virq(d, vpl011->virq);
>>>>> if ( !rc )
>>>>> {
>>>>> rc = -EINVAL;
>>>>> @@ -673,12 +693,12 @@ int domain_vpl011_init(struct domain *d,
>>>>> struct
>>>> vpl011_init_info *info)
>>>>> spin_lock_init(&vpl011->lock);
>>>>>
>>>>> register_mmio_handler(d, &vpl011_mmio_handler,
>>>>> - GUEST_PL011_BASE, GUEST_PL011_SIZE, NULL);
>>>>> + vpl011->base_addr, GUEST_PL011_SIZE,
>>>>> + NULL);
>>>>
>>>> So you are making the assumpption that the UART region will be equal
>>>> to (or
>>>> bigger) than GUEST_PL011_SIZE. There are definitely UART out where
>>>> the MMIO region is smaller than 4K.
>>>>
>>>
>>> Sorry. I got a few confused here, since I am not very familiar with pl011/UART
>> knowledge.
>>>
>>> Problems will occur when UART region is bigger than GUEST_PL011_SIZE,
>>> since we are only considering MMIO region of [vpl011->base_addr, vpl011-
>>> base_addr + GUEST_PL011_SIZE], right?
>>
>> It is in fact the other way around. The problem will appear if the host UART
>> MMIO region is smaller than the one we will emulate for the guest PL011.
>>
>
> Sorry to keep bothering.
> Is it that because when the UART MMIO region is smaller than the one we emulated,
> register(DR, RSR, FR, ...) will not be at the place where we emulated?
Let me give an example to clarify my point. On some Hardware (IIRC
pine64), the UART MMIO region is less than 4KB. In fact, there are
multiple device within the same 4KB region.
At the moment, we are removing those devices because we can't assign to
a domain a region that is not page aligned (4KB today). But I can see
some benefits to be able to assign such devices to different domain/xen.
To support them, we would need to trap the region and then forward only
access to address the domain can access.
The PL011 we emulate for the guest require a 4KB region. So this would
overlap with other device in the same region we may want to trap.
For is not an issue for the reasons I mentionned above. However, I think
it is a good idea to harden the code and add a check/comment when we
know potential pitfalls.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 37+ messages in thread* RE: [PATCH 09/11] xen/arm: if 1:1 direct-map domain use native UART address and IRQ number for vPL011
2021-10-13 18:00 ` Julien Grall
@ 2021-10-14 2:31 ` Penny Zheng
0 siblings, 0 replies; 37+ messages in thread
From: Penny Zheng @ 2021-10-14 2:31 UTC (permalink / raw)
To: Julien Grall, xen-devel, sstabellini; +Cc: Bertrand Marquis, Wei Chen
Hi Julien
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Thursday, October 14, 2021 2:01 AM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>
> Subject: Re: [PATCH 09/11] xen/arm: if 1:1 direct-map domain use native UART
> address and IRQ number for vPL011
>
> On 12/10/2021 03:42, Penny Zheng wrote:
> > Hi Julien
>
> Hi Penny,
>
> >> -----Original Message-----
> >> From: Julien Grall <julien@xen.org>
> >> Sent: Monday, October 11, 2021 6:49 PM
> >> To: Penny Zheng <Penny.Zheng@arm.com>;
> >> xen-devel@lists.xenproject.org; sstabellini@kernel.org
> >> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> >> <Wei.Chen@arm.com>
> >> Subject: Re: [PATCH 09/11] xen/arm: if 1:1 direct-map domain use
> >> native UART address and IRQ number for vPL011
> >>
> >> On 09/10/2021 09:47, Penny Zheng wrote:
> >>> Hi Julien
> >>
> >> Hi Penny,
> >>
> >>>> -----Original Message-----
> >>>> From: Julien Grall <julien@xen.org>
> >>>> Sent: Thursday, September 23, 2021 7:14 PM
> >>>> To: Penny Zheng <Penny.Zheng@arm.com>;
> >>>> xen-devel@lists.xenproject.org; sstabellini@kernel.org
> >>>> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> >>>> <Wei.Chen@arm.com>
> >>>> Subject: Re: [PATCH 09/11] xen/arm: if 1:1 direct-map domain use
> >>>> native UART address and IRQ number for vPL011
> >>>>
> >>>>
> >>>>
> >>>> On 23/09/2021 08:11, Penny Zheng wrote:
> >>>>> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> >>>>>
> >>>>> We always use a fix address to map the vPL011 to domains. The
> >>>>> address could be a problem for domains that are directly mapped.
> >>>>>
> >>>>> So, for domains that are directly mapped, reuse the address of the
> >>>>> physical UART on the platform to avoid potential clashes.
> >>>>>
> >>>>> Do the same for the virtual IRQ number: instead of always using
> >>>>> GUEST_VPL011_SPI, try to reuse the physical SPI number if possible.
> >>>>>
> >>>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> >>>>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> >>>>> ---
> >>>>> xen/arch/arm/domain_build.c | 34
> >>>>> +++++++++++++++++++++++++++-----
> >> --
> >>>>> xen/arch/arm/vpl011.c | 34 +++++++++++++++++++++++++++-------
> >>>>> xen/include/asm-arm/vpl011.h | 2 ++
> >>>>> 3 files changed, 56 insertions(+), 14 deletions(-)
> >>>>>
> >>>>> diff --git a/xen/arch/arm/domain_build.c
> >>>>> b/xen/arch/arm/domain_build.c index 120f1ae575..c92e510ae7 100644
> >>>>> --- a/xen/arch/arm/domain_build.c
> >>>>> +++ b/xen/arch/arm/domain_build.c
> >>>>> @@ -30,6 +30,7 @@
> >>>>>
> >>>>> #include <xen/irq.h>
> >>>>> #include <xen/grant_table.h>
> >>>>> +#include <xen/serial.h>
> >>>>>
> >>>>> static unsigned int __initdata opt_dom0_max_vcpus;
> >>>>> integer_param("dom0_max_vcpus", opt_dom0_max_vcpus); @@ -
> >> 1942,8
> >>>>> +1943,11 @@ static int __init make_vpl011_uart_node(struct
> >>>>> +kernel_info
> >>>> *kinfo)
> >>>>> gic_interrupt_t intr;
> >>>>> __be32 reg[GUEST_ROOT_ADDRESS_CELLS +
> >> GUEST_ROOT_SIZE_CELLS];
> >>>>> __be32 *cells;
> >>>>> + struct domain *d = kinfo->d;
> >>>>> + char buf[27];
> >>>>>
> >>>>> - res = fdt_begin_node(fdt, "sbsa-
> >> uart@"__stringify(GUEST_PL011_BASE));
> >>>>> + snprintf(buf, sizeof(buf), "sbsa-uart@%"PRIx64, d-
> >>>>> arch.vpl011.base_addr);
> >>>>> + res = fdt_begin_node(fdt, buf);
> >>>>> if ( res )
> >>>>> return res;
> >>>>>
> >>>>> @@ -1953,14 +1957,14 @@ static int __init
> >>>>> make_vpl011_uart_node(struct kernel_info *kinfo)
> >>>>>
> >>>>> cells = ®[0];
> >>>>> dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS,
> >>>>> - GUEST_ROOT_SIZE_CELLS, GUEST_PL011_BASE,
> >>>>> + GUEST_ROOT_SIZE_CELLS,
> >>>>> + d->arch.vpl011.base_addr,
> >>>>> GUEST_PL011_SIZE);
> >>>>>
> >>>>> res = fdt_property(fdt, "reg", reg, sizeof(reg));
> >>>>> if ( res )
> >>>>> return res;
> >>>>>
> >>>>> - set_interrupt(intr, GUEST_VPL011_SPI, 0xf,
> DT_IRQ_TYPE_LEVEL_HIGH);
> >>>>> + set_interrupt(intr, d->arch.vpl011.virq, 0xf,
> >>>>> + DT_IRQ_TYPE_LEVEL_HIGH);
> >>>>>
> >>>>> res = fdt_property(fdt, "interrupts", intr, sizeof (intr));
> >>>>> if ( res )
> >>>>> @@ -2670,6 +2674,13 @@ static int __init construct_domU(struct
> >>>>> domain
> >>>> *d,
> >>>>> else
> >>>>> allocate_static_memory(d, &kinfo, node);
> >>>>>
> >>>>> + /*
> >>>>> + * Initialization before creating its device
> >>>>> + * tree node in prepare_dtb_domU.
> >>>>> + */
> >>>>
> >>>> I think it would be better to explain *why* this needs to be done before.
> >>>>
> >>>>> + if ( kinfo.vpl011 )
> >>>>> + rc = domain_vpl011_init(d, NULL);
> >>>>> +
> >>>>> rc = prepare_dtb_domU(d, &kinfo);
> >>>>> if ( rc < 0 )
> >>>>> return rc;
> >>>>> @@ -2678,9 +2689,6 @@ static int __init construct_domU(struct
> >>>>> domain
> >>>> *d,
> >>>>> if ( rc < 0 )
> >>>>> return rc;
> >>>>>
> >>>>> - if ( kinfo.vpl011 )
> >>>>> - rc = domain_vpl011_init(d, NULL);
> >>>>> -
> >>>>> return rc;
> >>>>> }
> >>>>>
> >>>>> @@ -2723,15 +2731,27 @@ void __init create_domUs(void)
> >>>>>
> >>>>> if ( !dt_property_read_u32(node, "nr_spis", &d_cfg.arch.nr_spis) )
> >>>>> {
> >>>>> + unsigned int vpl011_virq = GUEST_VPL011_SPI;
> >>>>
> >>>> Coding style: Add a newline here.
> >>>>
> >>>>> d_cfg.arch.nr_spis = gic_number_lines() - 32;
> >>>>>
> >>>>> + /*
> >>>>> + * The VPL011 virq is GUEST_VPL011_SPI, unless direct-map in
> >>>>> + * set, in which case we'll try to match the hardware.
> >>>>> + *
> >>>>> + * Typically, d->arch.vpl011.virq has the vpl011 irq number
> >>>>> + * but at this point of the boot sequence it is not
> >>>>> + * initialized yet.
> >>>>> + */
> >>>>> + if ( direct_map && serial_irq(SERHND_DTUART) > 0 )
> >>>>> + vpl011_virq = serial_irq(SERHND_DTUART);
> >>>>
> >>>> I think we should not continue if the domain is direct-mapped *and*
> >>>> the IRQ is not found. Otherwise, this will may just result to
> >>>> potential breakage if GUEST_VPL011_SPI happens to be used for an HW
> >> device.
> >>>>
> >>>>> +
> >>>>> /*
> >>>>> * vpl011 uses one emulated SPI. If vpl011 is requested, make
> >>>>> * sure that we allocate enough SPIs for it.
> >>>>> */
> >>>>> if ( dt_property_read_bool(node, "vpl011") )
> >>>>> d_cfg.arch.nr_spis = MAX(d_cfg.arch.nr_spis,
> >>>>> - GUEST_VPL011_SPI - 32 + 1);
> >>>>> + vpl011_virq - 32 + 1);
> >>>>> }
> >>>>>
> >>>>> /*
> >>>>> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c index
> >>>>> 895f436cc4..10df25f098 100644
> >>>>> --- a/xen/arch/arm/vpl011.c
> >>>>> +++ b/xen/arch/arm/vpl011.c
> >>>>> @@ -29,6 +29,7 @@
> >>>>> #include <xen/mm.h>
> >>>>> #include <xen/sched.h>
> >>>>> #include <xen/console.h>
> >>>>> +#include <xen/serial.h>
> >>>>> #include <public/domctl.h>
> >>>>> #include <public/io/console.h>
> >>>>> #include <asm/pl011-uart.h>
> >>>>> @@ -71,11 +72,11 @@ static void
> >>>>> vpl011_update_interrupt_status(struct
> >>>> domain *d)
> >>>>> * status bit has been set since the last time.
> >>>>> */
> >>>>> if ( uartmis & ~vpl011->shadow_uartmis )
> >>>>> - vgic_inject_irq(d, NULL, GUEST_VPL011_SPI, true);
> >>>>> + vgic_inject_irq(d, NULL, vpl011->virq, true);
> >>>>>
> >>>>> vpl011->shadow_uartmis = uartmis;
> >>>>> #else
> >>>>> - vgic_inject_irq(d, NULL, GUEST_VPL011_SPI, uartmis);
> >>>>> + vgic_inject_irq(d, NULL, vpl011->virq, uartmis);
> >>>>> #endif
> >>>>> }
> >>>>>
> >>>>> @@ -347,7 +348,8 @@ static int vpl011_mmio_read(struct vcpu *v,
> >>>>> void *priv)
> >>>>> {
> >>>>> struct hsr_dabt dabt = info->dabt;
> >>>>> - uint32_t vpl011_reg = (uint32_t)(info->gpa - GUEST_PL011_BASE);
> >>>>> + uint32_t vpl011_reg = (uint32_t)(info->gpa -
> >>>>> +
> >>>>> + v->domain->arch.vpl011.base_addr);
> >>>>> struct vpl011 *vpl011 = &v->domain->arch.vpl011;
> >>>>> struct domain *d = v->domain;
> >>>>> unsigned long flags;
> >>>>> @@ -430,7 +432,8 @@ static int vpl011_mmio_write(struct vcpu *v,
> >>>>> void *priv)
> >>>>> {
> >>>>> struct hsr_dabt dabt = info->dabt;
> >>>>> - uint32_t vpl011_reg = (uint32_t)(info->gpa - GUEST_PL011_BASE);
> >>>>> + uint32_t vpl011_reg = (uint32_t)(info->gpa -
> >>>>> +
> >>>>> + v->domain->arch.vpl011.base_addr);
> >>>>> struct vpl011 *vpl011 = &v->domain->arch.vpl011;
> >>>>> struct domain *d = v->domain;
> >>>>> unsigned long flags;
> >>>>> @@ -622,10 +625,27 @@ int domain_vpl011_init(struct domain *d,
> >>>>> struct
> >>>> vpl011_init_info *info)
> >>>>> {
> >>>>> int rc;
> >>>>> struct vpl011 *vpl011 = &d->arch.vpl011;
> >>>>> + const struct vuart_info *uart =
> >>>>> + serial_vuart_info(SERHND_DTUART);
> >>>>>
> >>>>> if ( vpl011->backend.dom.ring_buf )
> >>>>> return -EINVAL;
> >>>>>
> >>>>> + vpl011->base_addr = GUEST_PL011_BASE;
> >>>>> + vpl011->virq = GUEST_VPL011_SPI;
> >>>>> + if ( is_domain_direct_mapped(d) )
> >>>>> + {
> >>>>> + if ( uart != NULL && serial_irq(SERHND_DTUART) > 0 )
> >>>>> + {
> >>>>> + vpl011->base_addr = uart->base_addr;
> >>>>> + vpl011->virq = serial_irq(SERHND_DTUART);
> >>>>
> >>>> This seems a bit pointless to call serial_irq() twice. How about
> >>>> add a field in vuart_info to return the interrupt number?
> >>>>
> >>>>> + }
> >>>>> + else
> >>>>> + printk(XENLOG_ERR
> >>>>> + "Unable to reuse physical UART address and irq for
> vPL011.\n"
> >>>>> + "Defaulting to addr %#"PRIpaddr" and IRQ %u\n",
> >>>>> + vpl011->base_addr, vpl011->virq);
> >>>>> + }
> >>>>> +
> >>>>> /*
> >>>>> * info is NULL when the backend is in Xen.
> >>>>> * info is != NULL when the backend is in a domain.
> >>>>> @@ -661,7 +681,7 @@ int domain_vpl011_init(struct domain *d,
> >>>>> struct
> >>>> vpl011_init_info *info)
> >>>>> }
> >>>>> }
> >>>>>
> >>>>> - rc = vgic_reserve_virq(d, GUEST_VPL011_SPI);
> >>>>> + rc = vgic_reserve_virq(d, vpl011->virq);
> >>>>> if ( !rc )
> >>>>> {
> >>>>> rc = -EINVAL;
> >>>>> @@ -673,12 +693,12 @@ int domain_vpl011_init(struct domain *d,
> >>>>> struct
> >>>> vpl011_init_info *info)
> >>>>> spin_lock_init(&vpl011->lock);
> >>>>>
> >>>>> register_mmio_handler(d, &vpl011_mmio_handler,
> >>>>> - GUEST_PL011_BASE, GUEST_PL011_SIZE, NULL);
> >>>>> + vpl011->base_addr, GUEST_PL011_SIZE,
> >>>>> + NULL);
> >>>>
> >>>> So you are making the assumpption that the UART region will be
> >>>> equal to (or
> >>>> bigger) than GUEST_PL011_SIZE. There are definitely UART out where
> >>>> the MMIO region is smaller than 4K.
> >>>>
> >>>
> >>> Sorry. I got a few confused here, since I am not very familiar with
> >>> pl011/UART
> >> knowledge.
> >>>
> >>> Problems will occur when UART region is bigger than
> >>> GUEST_PL011_SIZE, since we are only considering MMIO region of
> >>> [vpl011->base_addr, vpl011- base_addr + GUEST_PL011_SIZE], right?
> >>
> >> It is in fact the other way around. The problem will appear if the
> >> host UART MMIO region is smaller than the one we will emulate for the
> guest PL011.
> >>
> >
> > Sorry to keep bothering.
> > Is it that because when the UART MMIO region is smaller than the one
> > we emulated, register(DR, RSR, FR, ...) will not be at the place where we
> emulated?
>
> Let me give an example to clarify my point. On some Hardware (IIRC pine64),
> the UART MMIO region is less than 4KB. In fact, there are multiple device
> within the same 4KB region.
>
> At the moment, we are removing those devices because we can't assign to a
> domain a region that is not page aligned (4KB today). But I can see some
> benefits to be able to assign such devices to different domain/xen.
> To support them, we would need to trap the region and then forward only
> access to address the domain can access.
>
> The PL011 we emulate for the guest require a 4KB region. So this would
> overlap with other device in the same region we may want to trap.
>
> For is not an issue for the reasons I mentionned above. However, I think it is a
> good idea to harden the code and add a check/comment when we know
> potential pitfalls.
>
Understood, a thousand thanks for the detailed explanation! ;)
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 10/11] xen/arm: device assignment on 1:1 direct-map domain
2021-09-23 3:11 [PATCH 00/11] 1:1 direct-map memory map Penny Zheng
` (8 preceding siblings ...)
2021-09-23 3:11 ` [PATCH 09/11] xen/arm: if 1:1 direct-map domain use native UART address and IRQ number for vPL011 Penny Zheng
@ 2021-09-23 3:11 ` Penny Zheng
2021-09-23 11:26 ` Julien Grall
2021-09-23 3:11 ` [PATCH 11/11] xen/docs: add a document to explain how to do passthrough without IOMMU Penny Zheng
10 siblings, 1 reply; 37+ messages in thread
From: Penny Zheng @ 2021-09-23 3:11 UTC (permalink / raw)
To: xen-devel, sstabellini, julien; +Cc: Bertrand.Marquis, Penny.Zheng, Wei.Chen
User could do device passthrough, with "xen,force-assign-without-iommu" in
the device tree snippet, on trusted guest through 1:1 direct-map,
if IOMMU absent or disabled on hardware.
In order to achieve that, this patch adds 1:1 direct-map check and disables
iommu-related action.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
xen/arch/arm/domain_build.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index c92e510ae7..9a9d2522b7 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2070,14 +2070,18 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo,
if ( res < 0 )
return res;
+ /*
+ * If xen_force, we allow assignment of devices without IOMMU protection.
+ * And if IOMMU is disabled or absent, 1:1 direct-map is necessary
+ */
+ if ( xen_force && is_domain_direct_mapped(kinfo->d) &&
+ !dt_device_is_protected(node) )
+ return 0;
+
res = iommu_add_dt_device(node);
if ( res < 0 )
return res;
- /* If xen_force, we allow assignment of devices without IOMMU protection. */
- if ( xen_force && !dt_device_is_protected(node) )
- return 0;
-
return iommu_assign_dt_device(kinfo->d, node);
}
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 10/11] xen/arm: device assignment on 1:1 direct-map domain
2021-09-23 3:11 ` [PATCH 10/11] xen/arm: device assignment on 1:1 direct-map domain Penny Zheng
@ 2021-09-23 11:26 ` Julien Grall
2021-10-09 9:40 ` Penny Zheng
0 siblings, 1 reply; 37+ messages in thread
From: Julien Grall @ 2021-09-23 11:26 UTC (permalink / raw)
To: Penny Zheng, xen-devel, sstabellini; +Cc: Bertrand.Marquis, Wei.Chen
Hi,
On 23/09/2021 08:11, Penny Zheng wrote:
> User could do device passthrough, with "xen,force-assign-without-iommu" in
> the device tree snippet, on trusted guest through 1:1 direct-map,
> if IOMMU absent or disabled on hardware.
At the moment, it would be possible to passthrough a non-DMA capable
device with direct-mapping. After this patch, this is going to be forbidden.
>
> In order to achieve that, this patch adds 1:1 direct-map check and disables
> iommu-related action.
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> xen/arch/arm/domain_build.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index c92e510ae7..9a9d2522b7 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2070,14 +2070,18 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo,
> if ( res < 0 )
> return res;
>
> + /*
> + * If xen_force, we allow assignment of devices without IOMMU protection.
> + * And if IOMMU is disabled or absent, 1:1 direct-map is necessary > + */
> + if ( xen_force && is_domain_direct_mapped(kinfo->d) &&
> + !dt_device_is_protected(node) )
dt_device_is_protected() will be always false unless the device is
protected behing an SMMU using the legacy binding. So I don't think this
is correct to move this check ahead. In fact..
> + return 0;
> +
> res = iommu_add_dt_device(node);
... the call should already be a NOP when the IOMMU is disabled or the
device is not behind an IOMMU. So can you explain what you are trying to
prevent here?
> if ( res < 0 )
> return res;
>
> - /* If xen_force, we allow assignment of devices without IOMMU protection. */
> - if ( xen_force && !dt_device_is_protected(node) )
> - return 0;
> -
> return iommu_assign_dt_device(kinfo->d, node);
> }
>
>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 37+ messages in thread
* RE: [PATCH 10/11] xen/arm: device assignment on 1:1 direct-map domain
2021-09-23 11:26 ` Julien Grall
@ 2021-10-09 9:40 ` Penny Zheng
2021-10-11 11:14 ` Julien Grall
0 siblings, 1 reply; 37+ messages in thread
From: Penny Zheng @ 2021-10-09 9:40 UTC (permalink / raw)
To: Julien Grall, xen-devel, sstabellini; +Cc: Bertrand Marquis, Wei Chen
Hi Julien
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Thursday, September 23, 2021 7:27 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>
> Subject: Re: [PATCH 10/11] xen/arm: device assignment on 1:1 direct-map
> domain
>
> Hi,
>
> On 23/09/2021 08:11, Penny Zheng wrote:
> > User could do device passthrough, with
> > "xen,force-assign-without-iommu" in the device tree snippet, on
> > trusted guest through 1:1 direct-map, if IOMMU absent or disabled on
> hardware.
>
> At the moment, it would be possible to passthrough a non-DMA capable
> device with direct-mapping. After this patch, this is going to be forbidden.
>
> >
> > In order to achieve that, this patch adds 1:1 direct-map check and
> > disables iommu-related action.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> > xen/arch/arm/domain_build.c | 12 ++++++++----
> > 1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index c92e510ae7..9a9d2522b7 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -2070,14 +2070,18 @@ static int __init
> handle_passthrough_prop(struct kernel_info *kinfo,
> > if ( res < 0 )
> > return res;
> >
> > + /*
> > + * If xen_force, we allow assignment of devices without IOMMU
> protection.
> > + * And if IOMMU is disabled or absent, 1:1 direct-map is necessary > +
> */
> > + if ( xen_force && is_domain_direct_mapped(kinfo->d) &&
> > + !dt_device_is_protected(node) )
>
> dt_device_is_protected() will be always false unless the device is protected
> behing an SMMU using the legacy binding. So I don't think this is correct to
> move this check ahead. In fact..
>
> > + return 0;
> > +
> > res = iommu_add_dt_device(node);
>
> ... the call should already be a NOP when the IOMMU is disabled or the
> device is not behind an IOMMU. So can you explain what you are trying to
> prevent here?
>
If the IOMMU is disabled, iommu_add_dt_device will return 1 as errno.
So we could not make it to the xen_force check...
So I tried to move all IOMMU action behind xen_force check.
Now, device assignment without IOMMU protection is only
applicable on direct-map domains, so this commit also adds
is_domain_direct_mapped check together with xen_force check.
> > if ( res < 0 )
> > return res;
> >
> > - /* If xen_force, we allow assignment of devices without IOMMU
> protection. */
> > - if ( xen_force && !dt_device_is_protected(node) )
> > - return 0;
> > -
> > return iommu_assign_dt_device(kinfo->d, node);
> > }
> >
> >
>
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 10/11] xen/arm: device assignment on 1:1 direct-map domain
2021-10-09 9:40 ` Penny Zheng
@ 2021-10-11 11:14 ` Julien Grall
2021-10-12 2:29 ` Penny Zheng
2021-10-13 7:44 ` Penny Zheng
0 siblings, 2 replies; 37+ messages in thread
From: Julien Grall @ 2021-10-11 11:14 UTC (permalink / raw)
To: Penny Zheng, xen-devel, sstabellini; +Cc: Bertrand Marquis, Wei Chen
On 09/10/2021 10:40, Penny Zheng wrote:
> Hi Julien
Hi Penny,
>
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: Thursday, September 23, 2021 7:27 PM
>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
>> sstabellini@kernel.org
>> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
>> <Wei.Chen@arm.com>
>> Subject: Re: [PATCH 10/11] xen/arm: device assignment on 1:1 direct-map
>> domain
>>
>> Hi,
>>
>> On 23/09/2021 08:11, Penny Zheng wrote:
>>> User could do device passthrough, with
>>> "xen,force-assign-without-iommu" in the device tree snippet, on
>>> trusted guest through 1:1 direct-map, if IOMMU absent or disabled on
>> hardware.
>>
>> At the moment, it would be possible to passthrough a non-DMA capable
>> device with direct-mapping. After this patch, this is going to be forbidden.
>>
>>>
>>> In order to achieve that, this patch adds 1:1 direct-map check and
>>> disables iommu-related action.
>>>
>>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
>>> ---
>>> xen/arch/arm/domain_build.c | 12 ++++++++----
>>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index c92e510ae7..9a9d2522b7 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -2070,14 +2070,18 @@ static int __init
>> handle_passthrough_prop(struct kernel_info *kinfo,
>>> if ( res < 0 )
>>> return res;
>>>
>>> + /*
>>> + * If xen_force, we allow assignment of devices without IOMMU
>> protection.
>>> + * And if IOMMU is disabled or absent, 1:1 direct-map is necessary > +
>> */
>>> + if ( xen_force && is_domain_direct_mapped(kinfo->d) &&
>>> + !dt_device_is_protected(node) )
>>
>> dt_device_is_protected() will be always false unless the device is protected
>> behing an SMMU using the legacy binding. So I don't think this is correct to
>> move this check ahead. In fact..
>>
>>> + return 0;
>>> +
>>> res = iommu_add_dt_device(node);
>>
>> ... the call should already be a NOP when the IOMMU is disabled or the
>> device is not behind an IOMMU. So can you explain what you are trying to
>> prevent here?
>>
>
> If the IOMMU is disabled, iommu_add_dt_device will return 1 as errno.
> So we could not make it to the xen_force check...
I disagree. The check is:
if ( res < 0 )
return res;
Given that res is 1, we wouldn't return and move to check whether the
assignment can be done.
>
> So I tried to move all IOMMU action behind xen_force check.
>
> Now, device assignment without IOMMU protection is only
> applicable on direct-map domains,
It is fine to assign a non-DMA capable device without direct-mapping. So
why do you want to add this restriction?
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 37+ messages in thread
* RE: [PATCH 10/11] xen/arm: device assignment on 1:1 direct-map domain
2021-10-11 11:14 ` Julien Grall
@ 2021-10-12 2:29 ` Penny Zheng
2021-10-13 7:44 ` Penny Zheng
1 sibling, 0 replies; 37+ messages in thread
From: Penny Zheng @ 2021-10-12 2:29 UTC (permalink / raw)
To: Julien Grall, xen-devel, sstabellini; +Cc: Bertrand Marquis, Wei Chen
Hi julien
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Monday, October 11, 2021 7:14 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>
> Subject: Re: [PATCH 10/11] xen/arm: device assignment on 1:1 direct-map
> domain
>
>
>
> On 09/10/2021 10:40, Penny Zheng wrote:
> > Hi Julien
>
> Hi Penny,
>
> >
> >> -----Original Message-----
> >> From: Julien Grall <julien@xen.org>
> >> Sent: Thursday, September 23, 2021 7:27 PM
> >> To: Penny Zheng <Penny.Zheng@arm.com>;
> >> xen-devel@lists.xenproject.org; sstabellini@kernel.org
> >> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> >> <Wei.Chen@arm.com>
> >> Subject: Re: [PATCH 10/11] xen/arm: device assignment on 1:1
> >> direct-map domain
> >>
> >> Hi,
> >>
> >> On 23/09/2021 08:11, Penny Zheng wrote:
> >>> User could do device passthrough, with
> >>> "xen,force-assign-without-iommu" in the device tree snippet, on
> >>> trusted guest through 1:1 direct-map, if IOMMU absent or disabled on
> >> hardware.
> >>
> >> At the moment, it would be possible to passthrough a non-DMA capable
> >> device with direct-mapping. After this patch, this is going to be forbidden.
> >>
> >>>
> >>> In order to achieve that, this patch adds 1:1 direct-map check and
> >>> disables iommu-related action.
> >>>
> >>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> >>> ---
> >>> xen/arch/arm/domain_build.c | 12 ++++++++----
> >>> 1 file changed, 8 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/xen/arch/arm/domain_build.c
> >>> b/xen/arch/arm/domain_build.c index c92e510ae7..9a9d2522b7 100644
> >>> --- a/xen/arch/arm/domain_build.c
> >>> +++ b/xen/arch/arm/domain_build.c
> >>> @@ -2070,14 +2070,18 @@ static int __init
> >> handle_passthrough_prop(struct kernel_info *kinfo,
> >>> if ( res < 0 )
> >>> return res;
> >>>
> >>> + /*
> >>> + * If xen_force, we allow assignment of devices without IOMMU
> >> protection.
> >>> + * And if IOMMU is disabled or absent, 1:1 direct-map is
> >>> + necessary > +
> >> */
> >>> + if ( xen_force && is_domain_direct_mapped(kinfo->d) &&
> >>> + !dt_device_is_protected(node) )
> >>
> >> dt_device_is_protected() will be always false unless the device is
> >> protected behing an SMMU using the legacy binding. So I don't think
> >> this is correct to move this check ahead. In fact..
> >>
> >>> + return 0;
> >>> +
> >>> res = iommu_add_dt_device(node);
> >>
> >> ... the call should already be a NOP when the IOMMU is disabled or
> >> the device is not behind an IOMMU. So can you explain what you are
> >> trying to prevent here?
> >>
> >
> > If the IOMMU is disabled, iommu_add_dt_device will return 1 as errno.
> > So we could not make it to the xen_force check...
>
> I disagree. The check is:
>
> if ( res < 0 )
> return res;
>
> Given that res is 1, we wouldn't return and move to check whether the
> assignment can be done.
>
> >
> > So I tried to move all IOMMU action behind xen_force check.
> >
> > Now, device assignment without IOMMU protection is only applicable on
> > direct-map domains,
>
> It is fine to assign a non-DMA capable device without direct-mapping. So why
> do you want to add this restriction?
I understand.
If it is fine to assign a non-DMA capable device without direct-mapping, the former
commit containing the following changes needs fix.
if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") )
{
if ( iommu_enabled )
d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
else if ( d_cfg.flags & XEN_DOMCTL_INTERNAL_directmap )
warning_add("Please be sure of having trusted guests, when doing device assignment without IOMMU protection\n");
else
panic("Assign a device but IOMMU and direct-map are all disabled\n");
}
Definitely no panic when IOMMU and direct-map are all disabled.
>
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 37+ messages in thread* RE: [PATCH 10/11] xen/arm: device assignment on 1:1 direct-map domain
2021-10-11 11:14 ` Julien Grall
2021-10-12 2:29 ` Penny Zheng
@ 2021-10-13 7:44 ` Penny Zheng
2021-10-13 7:51 ` Penny Zheng
1 sibling, 1 reply; 37+ messages in thread
From: Penny Zheng @ 2021-10-13 7:44 UTC (permalink / raw)
To: Julien Grall, xen-devel, sstabellini; +Cc: Bertrand Marquis, Wei Chen
Hi Julien
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Monday, October 11, 2021 7:14 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>
> Subject: Re: [PATCH 10/11] xen/arm: device assignment on 1:1 direct-map
> domain
>
>
>
> On 09/10/2021 10:40, Penny Zheng wrote:
> > Hi Julien
>
> Hi Penny,
>
> >
> >> -----Original Message-----
> >> From: Julien Grall <julien@xen.org>
> >> Sent: Thursday, September 23, 2021 7:27 PM
> >> To: Penny Zheng <Penny.Zheng@arm.com>;
> >> xen-devel@lists.xenproject.org; sstabellini@kernel.org
> >> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> >> <Wei.Chen@arm.com>
> >> Subject: Re: [PATCH 10/11] xen/arm: device assignment on 1:1
> >> direct-map domain
> >>
> >> Hi,
> >>
> >> On 23/09/2021 08:11, Penny Zheng wrote:
> >>> User could do device passthrough, with
> >>> "xen,force-assign-without-iommu" in the device tree snippet, on
> >>> trusted guest through 1:1 direct-map, if IOMMU absent or disabled on
> >> hardware.
> >>
> >> At the moment, it would be possible to passthrough a non-DMA capable
> >> device with direct-mapping. After this patch, this is going to be forbidden.
> >>
> >>>
> >>> In order to achieve that, this patch adds 1:1 direct-map check and
> >>> disables iommu-related action.
> >>>
> >>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> >>> ---
> >>> xen/arch/arm/domain_build.c | 12 ++++++++----
> >>> 1 file changed, 8 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/xen/arch/arm/domain_build.c
> >>> b/xen/arch/arm/domain_build.c index c92e510ae7..9a9d2522b7 100644
> >>> --- a/xen/arch/arm/domain_build.c
> >>> +++ b/xen/arch/arm/domain_build.c
> >>> @@ -2070,14 +2070,18 @@ static int __init
> >> handle_passthrough_prop(struct kernel_info *kinfo,
> >>> if ( res < 0 )
> >>> return res;
> >>>
> >>> + /*
> >>> + * If xen_force, we allow assignment of devices without IOMMU
> >> protection.
> >>> + * And if IOMMU is disabled or absent, 1:1 direct-map is
> >>> + necessary > +
> >> */
> >>> + if ( xen_force && is_domain_direct_mapped(kinfo->d) &&
> >>> + !dt_device_is_protected(node) )
> >>
> >> dt_device_is_protected() will be always false unless the device is
> >> protected behing an SMMU using the legacy binding. So I don't think
> >> this is correct to move this check ahead. In fact..
> >>
> >>> + return 0;
> >>> +
> >>> res = iommu_add_dt_device(node);
> >>
> >> ... the call should already be a NOP when the IOMMU is disabled or
> >> the device is not behind an IOMMU. So can you explain what you are
> >> trying to prevent here?
> >>
> >
> > If the IOMMU is disabled, iommu_add_dt_device will return 1 as errno.
> > So we could not make it to the xen_force check...
>
> I disagree. The check is:
>
> if ( res < 0 )
> return res;
>
> Given that res is 1, we wouldn't return and move to check whether the
> assignment can be done.
>
> >
> > So I tried to move all IOMMU action behind xen_force check.
> >
> > Now, device assignment without IOMMU protection is only applicable on
> > direct-map domains,
>
> It is fine to assign a non-DMA capable device without direct-mapping. So why
> do you want to add this restriction?
>
When constructing direct-map-v2, found that, in xen/arch/arm/domain_build.c
if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") )
d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
And this flag XEN_DOMCTL_CDF_iommu determines whether iommu is enabled.
In ./xen/include/xen/sched.h
static always_inline bool is_iommu_enabled(const struct domain *d)
{
return evaluate_nospec(d->options & XEN_DOMCTL_CDF_iommu);
}
That is, even if we assign a non-DMA capable device, we request the platform to be
iommu enabled.
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 37+ messages in thread* RE: [PATCH 10/11] xen/arm: device assignment on 1:1 direct-map domain
2021-10-13 7:44 ` Penny Zheng
@ 2021-10-13 7:51 ` Penny Zheng
2021-10-13 16:34 ` Julien Grall
0 siblings, 1 reply; 37+ messages in thread
From: Penny Zheng @ 2021-10-13 7:51 UTC (permalink / raw)
To: Julien Grall, xen-devel, sstabellini; +Cc: Bertrand Marquis, Wei Chen
> -----Original Message-----
> From: Penny Zheng
> Sent: Wednesday, October 13, 2021 3:44 PM
> To: Julien Grall <julien@xen.org>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>
> Subject: RE: [PATCH 10/11] xen/arm: device assignment on 1:1 direct-map
> domain
>
> Hi Julien
>
> > -----Original Message-----
> > From: Julien Grall <julien@xen.org>
> > Sent: Monday, October 11, 2021 7:14 PM
> > To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> > sstabellini@kernel.org
> > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> > <Wei.Chen@arm.com>
> > Subject: Re: [PATCH 10/11] xen/arm: device assignment on 1:1
> > direct-map domain
> >
> >
> >
> > On 09/10/2021 10:40, Penny Zheng wrote:
> > > Hi Julien
> >
> > Hi Penny,
> >
> > >
> > >> -----Original Message-----
> > >> From: Julien Grall <julien@xen.org>
> > >> Sent: Thursday, September 23, 2021 7:27 PM
> > >> To: Penny Zheng <Penny.Zheng@arm.com>;
> > >> xen-devel@lists.xenproject.org; sstabellini@kernel.org
> > >> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> > >> <Wei.Chen@arm.com>
> > >> Subject: Re: [PATCH 10/11] xen/arm: device assignment on 1:1
> > >> direct-map domain
> > >>
> > >> Hi,
> > >>
> > >> On 23/09/2021 08:11, Penny Zheng wrote:
> > >>> User could do device passthrough, with
> > >>> "xen,force-assign-without-iommu" in the device tree snippet, on
> > >>> trusted guest through 1:1 direct-map, if IOMMU absent or disabled
> > >>> on
> > >> hardware.
> > >>
> > >> At the moment, it would be possible to passthrough a non-DMA
> > >> capable device with direct-mapping. After this patch, this is going to be
> forbidden.
> > >>
> > >>>
> > >>> In order to achieve that, this patch adds 1:1 direct-map check and
> > >>> disables iommu-related action.
> > >>>
> > >>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > >>> ---
> > >>> xen/arch/arm/domain_build.c | 12 ++++++++----
> > >>> 1 file changed, 8 insertions(+), 4 deletions(-)
> > >>>
> > >>> diff --git a/xen/arch/arm/domain_build.c
> > >>> b/xen/arch/arm/domain_build.c index c92e510ae7..9a9d2522b7 100644
> > >>> --- a/xen/arch/arm/domain_build.c
> > >>> +++ b/xen/arch/arm/domain_build.c
> > >>> @@ -2070,14 +2070,18 @@ static int __init
> > >> handle_passthrough_prop(struct kernel_info *kinfo,
> > >>> if ( res < 0 )
> > >>> return res;
> > >>>
> > >>> + /*
> > >>> + * If xen_force, we allow assignment of devices without IOMMU
> > >> protection.
> > >>> + * And if IOMMU is disabled or absent, 1:1 direct-map is
> > >>> + necessary > +
> > >> */
> > >>> + if ( xen_force && is_domain_direct_mapped(kinfo->d) &&
> > >>> + !dt_device_is_protected(node) )
> > >>
> > >> dt_device_is_protected() will be always false unless the device is
> > >> protected behing an SMMU using the legacy binding. So I don't think
> > >> this is correct to move this check ahead. In fact..
> > >>
> > >>> + return 0;
> > >>> +
> > >>> res = iommu_add_dt_device(node);
> > >>
> > >> ... the call should already be a NOP when the IOMMU is disabled or
> > >> the device is not behind an IOMMU. So can you explain what you are
> > >> trying to prevent here?
> > >>
> > >
> > > If the IOMMU is disabled, iommu_add_dt_device will return 1 as errno.
> > > So we could not make it to the xen_force check...
> >
> > I disagree. The check is:
> >
> > if ( res < 0 )
> > return res;
> >
> > Given that res is 1, we wouldn't return and move to check whether the
> > assignment can be done.
> >
> > >
> > > So I tried to move all IOMMU action behind xen_force check.
> > >
> > > Now, device assignment without IOMMU protection is only applicable
> > > on direct-map domains,
> >
> > It is fine to assign a non-DMA capable device without direct-mapping.
> > So why do you want to add this restriction?
> >
>
> When constructing direct-map-v2, found that, in
> xen/arch/arm/domain_build.c
>
> if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") )
> d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>
> And this flag XEN_DOMCTL_CDF_iommu determines whether iommu is
> enabled.
>
> In ./xen/include/xen/sched.h
>
> static always_inline bool is_iommu_enabled(const struct domain *d) {
> return evaluate_nospec(d->options & XEN_DOMCTL_CDF_iommu); }
>
> That is, even if we assign a non-DMA capable device, we request the platform
> to be iommu enabled.
>
I intend to change it to
if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") )
{
if ( iommu_enabled )
d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
else if ( d_cfg.flags & XEN_DOMCTL_CDF_directmap )
warning_add("Please be sure of having trusted guests, when doing device assignment without IOMMU protection\n");
}
> > Cheers,
> >
> > --
> > Julien Grall
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 10/11] xen/arm: device assignment on 1:1 direct-map domain
2021-10-13 7:51 ` Penny Zheng
@ 2021-10-13 16:34 ` Julien Grall
0 siblings, 0 replies; 37+ messages in thread
From: Julien Grall @ 2021-10-13 16:34 UTC (permalink / raw)
To: Penny Zheng, xen-devel, sstabellini; +Cc: Bertrand Marquis, Wei Chen
Hi Penny,
On 13/10/2021 08:51, Penny Zheng wrote:
>
>> -----Original Message-----
>> From: Penny Zheng
>> Sent: Wednesday, October 13, 2021 3:44 PM
>> To: Julien Grall <julien@xen.org>; xen-devel@lists.xenproject.org;
>> sstabellini@kernel.org
>> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
>> <Wei.Chen@arm.com>
>> Subject: RE: [PATCH 10/11] xen/arm: device assignment on 1:1 direct-map
>> domain
>>
>> Hi Julien
>>
>>> -----Original Message-----
>>> From: Julien Grall <julien@xen.org>
>>> Sent: Monday, October 11, 2021 7:14 PM
>>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
>>> sstabellini@kernel.org
>>> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
>>> <Wei.Chen@arm.com>
>>> Subject: Re: [PATCH 10/11] xen/arm: device assignment on 1:1
>>> direct-map domain
>>>
>>>
>>>
>>> On 09/10/2021 10:40, Penny Zheng wrote:
>>>> Hi Julien
>>>
>>> Hi Penny,
>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Julien Grall <julien@xen.org>
>>>>> Sent: Thursday, September 23, 2021 7:27 PM
>>>>> To: Penny Zheng <Penny.Zheng@arm.com>;
>>>>> xen-devel@lists.xenproject.org; sstabellini@kernel.org
>>>>> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
>>>>> <Wei.Chen@arm.com>
>>>>> Subject: Re: [PATCH 10/11] xen/arm: device assignment on 1:1
>>>>> direct-map domain
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 23/09/2021 08:11, Penny Zheng wrote:
>>>>>> User could do device passthrough, with
>>>>>> "xen,force-assign-without-iommu" in the device tree snippet, on
>>>>>> trusted guest through 1:1 direct-map, if IOMMU absent or disabled
>>>>>> on
>>>>> hardware.
>>>>>
>>>>> At the moment, it would be possible to passthrough a non-DMA
>>>>> capable device with direct-mapping. After this patch, this is going to be
>> forbidden.
>>>>>
>>>>>>
>>>>>> In order to achieve that, this patch adds 1:1 direct-map check and
>>>>>> disables iommu-related action.
>>>>>>
>>>>>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
>>>>>> ---
>>>>>> xen/arch/arm/domain_build.c | 12 ++++++++----
>>>>>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/xen/arch/arm/domain_build.c
>>>>>> b/xen/arch/arm/domain_build.c index c92e510ae7..9a9d2522b7 100644
>>>>>> --- a/xen/arch/arm/domain_build.c
>>>>>> +++ b/xen/arch/arm/domain_build.c
>>>>>> @@ -2070,14 +2070,18 @@ static int __init
>>>>> handle_passthrough_prop(struct kernel_info *kinfo,
>>>>>> if ( res < 0 )
>>>>>> return res;
>>>>>>
>>>>>> + /*
>>>>>> + * If xen_force, we allow assignment of devices without IOMMU
>>>>> protection.
>>>>>> + * And if IOMMU is disabled or absent, 1:1 direct-map is
>>>>>> + necessary > +
>>>>> */
>>>>>> + if ( xen_force && is_domain_direct_mapped(kinfo->d) &&
>>>>>> + !dt_device_is_protected(node) )
>>>>>
>>>>> dt_device_is_protected() will be always false unless the device is
>>>>> protected behing an SMMU using the legacy binding. So I don't think
>>>>> this is correct to move this check ahead. In fact..
>>>>>
>>>>>> + return 0;
>>>>>> +
>>>>>> res = iommu_add_dt_device(node);
>>>>>
>>>>> ... the call should already be a NOP when the IOMMU is disabled or
>>>>> the device is not behind an IOMMU. So can you explain what you are
>>>>> trying to prevent here?
>>>>>
>>>>
>>>> If the IOMMU is disabled, iommu_add_dt_device will return 1 as errno.
>>>> So we could not make it to the xen_force check...
>>>
>>> I disagree. The check is:
>>>
>>> if ( res < 0 )
>>> return res;
>>>
>>> Given that res is 1, we wouldn't return and move to check whether the
>>> assignment can be done.
>>>
>>>>
>>>> So I tried to move all IOMMU action behind xen_force check.
>>>>
>>>> Now, device assignment without IOMMU protection is only applicable
>>>> on direct-map domains,
>>>
>>> It is fine to assign a non-DMA capable device without direct-mapping.
>>> So why do you want to add this restriction?
>>>
>>
>> When constructing direct-map-v2, found that, in
>> xen/arch/arm/domain_build.c
>>
>> if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") )
>> d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>>
>> And this flag XEN_DOMCTL_CDF_iommu determines whether iommu is
>> enabled.
>>
>> In ./xen/include/xen/sched.h
>>
>> static always_inline bool is_iommu_enabled(const struct domain *d) {
>> return evaluate_nospec(d->options & XEN_DOMCTL_CDF_iommu); }
>>
>> That is, even if we assign a non-DMA capable device, we request the platform
>> to be iommu enabled.
>>
>
> I intend to change it to
>
> if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") )
> {
> if ( iommu_enabled )
> d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
> else if ( d_cfg.flags & XEN_DOMCTL_CDF_directmap )
> warning_add("Please be sure of having trusted guests, when doing device assignment without IOMMU protection\n");
> }
I think the warning is misleading. You don't need to trust a guest in
order to assign non-DMA capable device.
But, I think the message is not necessary because from my understanding,
an admin would need to add the property xen,force-assign-without-iommu
in order to passthrough. So they should be fully aware of the
consequence to do the passthrough.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 11/11] xen/docs: add a document to explain how to do passthrough without IOMMU
2021-09-23 3:11 [PATCH 00/11] 1:1 direct-map memory map Penny Zheng
` (9 preceding siblings ...)
2021-09-23 3:11 ` [PATCH 10/11] xen/arm: device assignment on 1:1 direct-map domain Penny Zheng
@ 2021-09-23 3:11 ` Penny Zheng
10 siblings, 0 replies; 37+ messages in thread
From: Penny Zheng @ 2021-09-23 3:11 UTC (permalink / raw)
To: xen-devel, sstabellini, julien; +Cc: Bertrand.Marquis, Penny.Zheng, Wei.Chen
From: Stefano Stabellini <stefano.stabellini@xilinx.com>
Make sure to start with a WARNING about security.
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
docs/misc/arm/passthrough-noiommu.txt | 54 +++++++++++++++++++++++++++
1 file changed, 54 insertions(+)
create mode 100644 docs/misc/arm/passthrough-noiommu.txt
diff --git a/docs/misc/arm/passthrough-noiommu.txt b/docs/misc/arm/passthrough-noiommu.txt
new file mode 100644
index 0000000000..61aeb8a5cd
--- /dev/null
+++ b/docs/misc/arm/passthrough-noiommu.txt
@@ -0,0 +1,54 @@
+Request Device Assignment without IOMMU support
+===============================================
+
+*WARNING:
+Users should be aware that it is not always secure to assign a device without
+IOMMU protection.
+When the device is not protected by the IOMMU, the administrator should make
+sure that:
+ 1. The device is assigned to a trusted guest.
+ 2. Users have additional security mechanism on the platform.
+
+
+This document assumes that the IOMMU is absent from the system or it is
+disabled (status = "disabled" in device tree).
+
+
+Add xen,force-assign-without-iommu; to the device tree snippet:
+
+ethernet: ethernet@ff0e0000 {
+ compatible = "cdns,zynqmp-gem";
+ xen,path = "/amba/ethernet@ff0e0000";
+ xen,reg = <0x0 0xff0e0000 0x1000 0x0 0xff0e0000>;
+ xen,force-assign-without-iommu;
+};
+
+Request 1:1 memory mapping for the domain on static allocation
+==============================================================
+
+Add a direct-map property under the appropriate /chosen/domU node which
+is also statically allocated with physical memory ranges through
+xen,static-mem property as its guest RAM.
+
+Below is an example on how to specify the 1:1 memory mapping for the domain
+on static allocation in the device-tree:
+
+/ {
+ chosen {
+ domU1 {
+ compatible = "xen,domain";
+ #address-cells = <0x2>;
+ #size-cells = <0x2>;
+ cpus = <2>;
+ memory = <0x0 0x80000>;
+ #xen,static-mem-address-cells = <0x1>;
+ #xen,static-mem-size-cells = <0x1>;
+ xen,static-mem = <0x30000000 0x20000000>;
+ direct-map;
+ ...
+ };
+ };
+};
+
+Besides reserving a 512MB region starting at the host physical address
+0x30000000 to DomU1, it also requests 1:1 memory mapping.
--
2.25.1
^ permalink raw reply related [flat|nested] 37+ messages in thread