* [PATCH v2 0/8] Remaining patches for dynamic node programming using overlay dtbo
@ 2024-05-16 10:03 Henry Wang
2024-05-16 10:03 ` [PATCH v2 1/8] xen/common/dt-overlay: Fix lock issue when add/remove the device Henry Wang
` (7 more replies)
0 siblings, 8 replies; 27+ messages in thread
From: Henry Wang @ 2024-05-16 10:03 UTC (permalink / raw
To: xen-devel
Cc: Henry Wang, Stefano Stabellini, Julien Grall, Anthony PERARD,
Bertrand Marquis, Michal Orzel, Volodymyr Babchuk, George Dunlap,
Nick Rosbrook, Juergen Gross, Andrew Cooper, Jan Beulich
Hi all,
This is the remaining series for the full functional "dynamic node
programming using overlay dtbo" feature. The first part [1] has
already been merged.
Quoting from the original series, the first part has already made
Xen aware of new device tree node which means updating the dt_host
with overlay node information, and in this series, the goal is to
map IRQ and IOMMU during runtime, where we will do the actual IOMMU
and IRQ mapping and unmapping to a running domain. Also, documentation
of the "dynamic node programming using overlay dtbo" feature is added.
Patch 1 and 2 are fixes of the existing code which is noticed during
my local tests, details please see the commit message.
Gitlab CI for this series can be found in [1].
[1] https://gitlab.com/xen-project/people/henryw/xen/-/pipelines/1293126857
Henry Wang (6):
xen/common/dt-overlay: Fix lock issue when add/remove the device
tools/xl: Correct the help information and exit code of the dt-overlay
command
xen/arm, doc: Add a DT property to specify IOMMU for Dom0less domUs
tools/arm: Introduce the "nr_spis" xl config entry
xen/arm: Add XEN_DOMCTL_dt_overlay DOMCTL and related operations
tools: Introduce the "xl dt-overlay {attach,detach}" commands
Vikram Garhwal (2):
xen/arm/gic: Allow routing/removing interrupt to running VMs
docs: Add device tree overlay documentation
docs/man/xl.cfg.5.pod.in | 11 +
docs/misc/arm/device-tree/booting.txt | 13 +
docs/misc/arm/overlay.txt | 99 ++++++
tools/golang/xenlight/helpers.gen.go | 2 +
tools/golang/xenlight/types.gen.go | 1 +
tools/include/libxl.h | 15 +-
tools/include/xenctrl.h | 3 +
tools/libs/ctrl/xc_dt_overlay.c | 31 ++
tools/libs/light/libxl_arm.c | 4 +-
tools/libs/light/libxl_dt_overlay.c | 30 +-
tools/libs/light/libxl_types.idl | 1 +
tools/xl/xl_cmdtable.c | 4 +-
tools/xl/xl_parse.c | 3 +
tools/xl/xl_vmcontrol.c | 39 ++-
xen/arch/arm/dom0less-build.c | 7 +-
xen/arch/arm/domctl.c | 3 +
xen/arch/arm/gic-vgic.c | 8 +-
xen/arch/arm/gic.c | 15 -
xen/arch/arm/vgic/vgic.c | 5 +-
xen/common/dt-overlay.c | 418 +++++++++++++++++++++-----
xen/include/public/domctl.h | 15 +
xen/include/public/sysctl.h | 7 +-
xen/include/xen/dt-overlay.h | 7 +
23 files changed, 615 insertions(+), 126 deletions(-)
create mode 100644 docs/misc/arm/overlay.txt
--
2.34.1
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 1/8] xen/common/dt-overlay: Fix lock issue when add/remove the device
2024-05-16 10:03 [PATCH v2 0/8] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
@ 2024-05-16 10:03 ` Henry Wang
2024-05-19 10:04 ` Julien Grall
2024-05-16 10:03 ` [PATCH v2 2/8] tools/xl: Correct the help information and exit code of the dt-overlay command Henry Wang
` (6 subsequent siblings)
7 siblings, 1 reply; 27+ messages in thread
From: Henry Wang @ 2024-05-16 10:03 UTC (permalink / raw
To: xen-devel; +Cc: Henry Wang, Stefano Stabellini, Julien Grall
If CONFIG_DEBUG=y, below assertion will be triggered:
(XEN) Assertion 'rw_is_locked(&dt_host_lock)' failed at drivers/passthrough/device_tree.c:146
(XEN) ----[ Xen-4.19-unstable arm64 debug=y Not tainted ]----
[...]
(XEN) Xen call trace:
(XEN) [<00000a0000257418>] iommu_remove_dt_device+0x8c/0xd4 (PC)
(XEN) [<00000a00002573a0>] iommu_remove_dt_device+0x14/0xd4 (LR)
(XEN) [<00000a000020797c>] dt-overlay.c#remove_node_resources+0x8c/0x90
(XEN) [<00000a0000207f14>] dt-overlay.c#remove_nodes+0x524/0x648
(XEN) [<00000a0000208460>] dt_overlay_sysctl+0x428/0xc68
(XEN) [<00000a00002707f8>] arch_do_sysctl+0x1c/0x2c
(XEN) [<00000a0000230b40>] do_sysctl+0x96c/0x9ec
(XEN) [<00000a0000271e08>] traps.c#do_trap_hypercall+0x1e8/0x288
(XEN) [<00000a0000273490>] do_trap_guest_sync+0x448/0x63c
(XEN) [<00000a000025c480>] entry.o#guest_sync_slowpath+0xa8/0xd8
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) Assertion 'rw_is_locked(&dt_host_lock)' failed at drivers/passthrough/device_tree.c:146
(XEN) ****************************************
This is because iommu_remove_dt_device() is called without taking the
dt_host_lock. dt_host_lock is meant to ensure that the DT node will not
disappear behind back. So fix the issue by taking the lock as soon as
getting hold of overlay_node.
Similar issue will be observed in adding the dtbo:
(XEN) Assertion 'system_state < SYS_STATE_active || rw_is_locked(&dt_host_lock)'
failed at xen-source/xen/drivers/passthrough/device_tree.c:192
(XEN) ----[ Xen-4.19-unstable arm64 debug=y Not tainted ]----
[...]
(XEN) Xen call trace:
(XEN) [<00000a00002594f4>] iommu_add_dt_device+0x7c/0x17c (PC)
(XEN) [<00000a0000259494>] iommu_add_dt_device+0x1c/0x17c (LR)
(XEN) [<00000a0000267db4>] handle_device+0x68/0x1e8
(XEN) [<00000a0000208ba8>] dt_overlay_sysctl+0x9d4/0xb84
(XEN) [<00000a000027342c>] arch_do_sysctl+0x24/0x38
(XEN) [<00000a0000231ac8>] do_sysctl+0x9ac/0xa34
(XEN) [<00000a0000274b70>] traps.c#do_trap_hypercall+0x230/0x2dc
(XEN) [<00000a0000276330>] do_trap_guest_sync+0x478/0x688
(XEN) [<00000a000025e480>] entry.o#guest_sync_slowpath+0xa8/0xd8
This is because the lock is released too early. So fix the issue by
releasing the lock after handle_device().
Fixes: 7e5c4a8b86f1 ("xen/arm: Implement device tree node removal functionalities")
Signed-off-by: Henry Wang <xin.wang2@amd.com>
---
v2:
- Take the lock as soon as getting hold of overlay_node. Also
release the lock after handle_device() when adding dtbo.
v1.1:
- Move the unlock position before the check of rc.
---
xen/common/dt-overlay.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c
index 1b197381f6..9cece79067 100644
--- a/xen/common/dt-overlay.c
+++ b/xen/common/dt-overlay.c
@@ -429,18 +429,24 @@ static int remove_nodes(const struct overlay_track *tracker)
if ( overlay_node == NULL )
return -EINVAL;
+ write_lock(&dt_host_lock);
+
rc = remove_descendant_nodes_resources(overlay_node);
if ( rc )
+ {
+ write_unlock(&dt_host_lock);
return rc;
+ }
rc = remove_node_resources(overlay_node);
if ( rc )
+ {
+ write_unlock(&dt_host_lock);
return rc;
+ }
dt_dprintk("Removing node: %s\n", overlay_node->full_name);
- write_lock(&dt_host_lock);
-
rc = dt_overlay_remove_node(overlay_node);
if ( rc )
{
@@ -604,8 +610,6 @@ static long add_nodes(struct overlay_track *tr, char **nodes_full_path)
return rc;
}
- write_unlock(&dt_host_lock);
-
prev_node->allnext = next_node;
overlay_node = dt_find_node_by_path(overlay_node->full_name);
@@ -619,6 +623,7 @@ static long add_nodes(struct overlay_track *tr, char **nodes_full_path)
rc = handle_device(hardware_domain, overlay_node, p2m_mmio_direct_c,
tr->iomem_ranges,
tr->irq_ranges);
+ write_unlock(&dt_host_lock);
if ( rc )
{
printk(XENLOG_ERR "Adding IRQ and IOMMU failed\n");
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 2/8] tools/xl: Correct the help information and exit code of the dt-overlay command
2024-05-16 10:03 [PATCH v2 0/8] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
2024-05-16 10:03 ` [PATCH v2 1/8] xen/common/dt-overlay: Fix lock issue when add/remove the device Henry Wang
@ 2024-05-16 10:03 ` Henry Wang
2024-05-20 18:48 ` Jason Andryuk
2024-05-16 10:03 ` [PATCH v2 3/8] xen/arm, doc: Add a DT property to specify IOMMU for Dom0less domUs Henry Wang
` (5 subsequent siblings)
7 siblings, 1 reply; 27+ messages in thread
From: Henry Wang @ 2024-05-16 10:03 UTC (permalink / raw
To: xen-devel; +Cc: Henry Wang, Anthony PERARD, Anthony PERARD
Fix the name mismatch in the xl dt-overlay command, the
command name should be "dt-overlay" instead of "dt_overlay".
Add the missing "," in the cmdtable.
Fix the exit code of the dt-overlay command, use EXIT_FAILURE
instead of ERROR_FAIL.
Fixes: 61765a07e3d8 ("tools/xl: Add new xl command overlay for device tree overlay support")
Suggested-by: Anthony PERARD <anthony.perard@cloud.com>
Signed-off-by: Henry Wang <xin.wang2@amd.com>
---
v2:
- New patch
---
tools/xl/xl_cmdtable.c | 2 +-
tools/xl/xl_vmcontrol.c | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
index 62bdb2aeaa..1f3c6b5897 100644
--- a/tools/xl/xl_cmdtable.c
+++ b/tools/xl/xl_cmdtable.c
@@ -635,7 +635,7 @@ const struct cmd_spec cmd_table[] = {
{ "dt-overlay",
&main_dt_overlay, 0, 1,
"Add/Remove a device tree overlay",
- "add/remove <.dtbo>"
+ "add/remove <.dtbo>",
"-h print this help\n"
},
#endif
diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index 98f6bd2e76..02575d5d36 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -1278,7 +1278,7 @@ int main_dt_overlay(int argc, char **argv)
const int overlay_remove_op = 2;
if (argc < 2) {
- help("dt_overlay");
+ help("dt-overlay");
return EXIT_FAILURE;
}
@@ -1302,11 +1302,11 @@ int main_dt_overlay(int argc, char **argv)
fprintf(stderr, "failed to read the overlay device tree file %s\n",
overlay_config_file);
free(overlay_dtb);
- return ERROR_FAIL;
+ return EXIT_FAILURE;
}
} else {
fprintf(stderr, "overlay dtbo file not provided\n");
- return ERROR_FAIL;
+ return EXIT_FAILURE;
}
rc = libxl_dt_overlay(ctx, overlay_dtb, overlay_dtb_size, op);
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 3/8] xen/arm, doc: Add a DT property to specify IOMMU for Dom0less domUs
2024-05-16 10:03 [PATCH v2 0/8] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
2024-05-16 10:03 ` [PATCH v2 1/8] xen/common/dt-overlay: Fix lock issue when add/remove the device Henry Wang
2024-05-16 10:03 ` [PATCH v2 2/8] tools/xl: Correct the help information and exit code of the dt-overlay command Henry Wang
@ 2024-05-16 10:03 ` Henry Wang
2024-05-19 10:17 ` Julien Grall
2024-05-16 10:03 ` [PATCH v2 4/8] tools/arm: Introduce the "nr_spis" xl config entry Henry Wang
` (4 subsequent siblings)
7 siblings, 1 reply; 27+ messages in thread
From: Henry Wang @ 2024-05-16 10:03 UTC (permalink / raw
To: xen-devel
Cc: Henry Wang, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk
There are some use cases in which the dom0less domUs need to have
the XEN_DOMCTL_CDF_iommu set at the domain construction time. For
example, the dynamic dtbo feature allows the domain to be assigned
a device that is behind the IOMMU at runtime. For these use cases,
we need to have a way to specify the domain will need the IOMMU
mapping at domain construction time.
Introduce a "passthrough" DT property for Dom0less DomUs following
the same entry as the xl.cfg. Currently only provide two options,
i.e. "enable" and "disable". Set the XEN_DOMCTL_CDF_iommu at domain
construction time based on the property.
Signed-off-by: Henry Wang <xin.wang2@amd.com>
---
v2:
- New patch to replace the original patch in v1:
"[PATCH 03/15] xen/arm: Always enable IOMMU"
---
docs/misc/arm/device-tree/booting.txt | 13 +++++++++++++
xen/arch/arm/dom0less-build.c | 7 +++++--
2 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index bbd955e9c2..61f9082553 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -260,6 +260,19 @@ with the following properties:
value specified by Xen command line parameter gnttab_max_maptrack_frames
(or its default value if unspecified, i.e. 1024) is used.
+- passthrough
+
+ A string property specifying whether IOMMU mappings are enabled for the
+ domain and hence whether it will be enabled for passthrough hardware.
+ Possible property values are:
+
+ - "enabled"
+ IOMMU mappings are enabled for the domain.
+
+ - "disabled"
+ IOMMU mappings are disabled for the domain and so hardware may not be
+ passed through. This option is the default if this property is missing.
+
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/dom0less-build.c b/xen/arch/arm/dom0less-build.c
index 74f053c242..1396a102e1 100644
--- a/xen/arch/arm/dom0less-build.c
+++ b/xen/arch/arm/dom0less-build.c
@@ -848,6 +848,7 @@ static int __init construct_domU(struct domain *d,
void __init create_domUs(void)
{
struct dt_device_node *node;
+ const char *dom0less_iommu;
const struct dt_device_node *cpupool_node,
*chosen = dt_find_node_by_path("/chosen");
@@ -895,8 +896,10 @@ void __init create_domUs(void)
panic("Missing property 'cpus' for domain %s\n",
dt_node_name(node));
- if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") &&
- iommu_enabled )
+ if ( iommu_enabled &&
+ ((!dt_property_read_string(node, "passthrough", &dom0less_iommu) &&
+ !strcmp(dom0less_iommu, "enabled")) ||
+ dt_find_compatible_node(node, NULL, "multiboot,device-tree")) )
d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
if ( !dt_property_read_u32(node, "nr_spis", &d_cfg.arch.nr_spis) )
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 4/8] tools/arm: Introduce the "nr_spis" xl config entry
2024-05-16 10:03 [PATCH v2 0/8] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
` (2 preceding siblings ...)
2024-05-16 10:03 ` [PATCH v2 3/8] xen/arm, doc: Add a DT property to specify IOMMU for Dom0less domUs Henry Wang
@ 2024-05-16 10:03 ` Henry Wang
2024-05-20 19:13 ` Jason Andryuk
2024-05-16 10:03 ` [PATCH v2 5/8] xen/arm/gic: Allow routing/removing interrupt to running VMs Henry Wang
` (3 subsequent siblings)
7 siblings, 1 reply; 27+ messages in thread
From: Henry Wang @ 2024-05-16 10:03 UTC (permalink / raw
To: xen-devel
Cc: Henry Wang, Anthony PERARD, George Dunlap, Nick Rosbrook,
Juergen Gross
Currently, the number of SPIs allocated to the domain is only
configurable for Dom0less DomUs. Xen domains are supposed to be
platform agnostics and therefore the numbers of SPIs for libxl
guests should not be based on the hardware.
Introduce a new xl config entry for Arm to provide a method for
user to decide the number of SPIs. This would help to avoid
bumping the `config->arch.nr_spis` in libxl everytime there is a
new platform with increased SPI numbers.
Update the doc and the golang bindings accordingly.
Signed-off-by: Henry Wang <xin.wang2@amd.com>
---
v2:
- New patch to replace the original patch in v1:
"[PATCH 05/15] tools/libs/light: Increase nr_spi to 160"
---
docs/man/xl.cfg.5.pod.in | 11 +++++++++++
tools/golang/xenlight/helpers.gen.go | 2 ++
tools/golang/xenlight/types.gen.go | 1 +
tools/libs/light/libxl_arm.c | 4 ++--
tools/libs/light/libxl_types.idl | 1 +
tools/xl/xl_parse.c | 3 +++
6 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 8f2b375ce9..6a2d86065e 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -3072,6 +3072,17 @@ raised.
=back
+=over 4
+
+=item B<nr_spis="NR_SPIS">
+
+A 32-bit optional integer parameter specifying the number of SPIs (Shared
+Peripheral Interrupts) to allocate for the domain. If the `nr_spis` parameter
+is missing, the max number of SPIs calculated by the toolstack based on the
+devices allocated for the domain will be used.
+
+=back
+
=head3 x86
=over 4
diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
index 78bdb08b15..757ccaf035 100644
--- a/tools/golang/xenlight/helpers.gen.go
+++ b/tools/golang/xenlight/helpers.gen.go
@@ -1154,6 +1154,7 @@ return fmt.Errorf("invalid union key '%v'", x.Type)}
x.ArchArm.GicVersion = GicVersion(xc.arch_arm.gic_version)
x.ArchArm.Vuart = VuartType(xc.arch_arm.vuart)
x.ArchArm.SveVl = SveType(xc.arch_arm.sve_vl)
+x.ArchArm.NrSpis = uint32(xc.arch_arm.nr_spis)
if err := x.ArchX86.MsrRelaxed.fromC(&xc.arch_x86.msr_relaxed);err != nil {
return fmt.Errorf("converting field ArchX86.MsrRelaxed: %v", err)
}
@@ -1670,6 +1671,7 @@ return fmt.Errorf("invalid union key '%v'", x.Type)}
xc.arch_arm.gic_version = C.libxl_gic_version(x.ArchArm.GicVersion)
xc.arch_arm.vuart = C.libxl_vuart_type(x.ArchArm.Vuart)
xc.arch_arm.sve_vl = C.libxl_sve_type(x.ArchArm.SveVl)
+xc.arch_arm.nr_spis = C.uint32_t(x.ArchArm.NrSpis)
if err := x.ArchX86.MsrRelaxed.toC(&xc.arch_x86.msr_relaxed); err != nil {
return fmt.Errorf("converting field ArchX86.MsrRelaxed: %v", err)
}
diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go
index ccfe18019e..b7b4ba88af 100644
--- a/tools/golang/xenlight/types.gen.go
+++ b/tools/golang/xenlight/types.gen.go
@@ -597,6 +597,7 @@ ArchArm struct {
GicVersion GicVersion
Vuart VuartType
SveVl SveType
+NrSpis uint32
}
ArchX86 struct {
MsrRelaxed Defbool
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index 1cb89fa584..a4029e3ac8 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -181,8 +181,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
LOG(DEBUG, "Configure the domain");
- config->arch.nr_spis = nr_spis;
- LOG(DEBUG, " - Allocate %u SPIs", nr_spis);
+ config->arch.nr_spis = max(nr_spis, d_config->b_info.arch_arm.nr_spis);
+ LOG(DEBUG, " - Allocate %u SPIs", config->arch.nr_spis);
switch (d_config->b_info.arch_arm.gic_version) {
case LIBXL_GIC_VERSION_DEFAULT:
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 470122e768..3f143f405d 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -722,6 +722,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
("vuart", libxl_vuart_type),
("sve_vl", libxl_sve_type),
+ ("nr_spis", uint32),
])),
("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
])),
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index ab09d0288b..4aa99029b5 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -2933,6 +2933,9 @@ skip_usbdev:
}
}
+ if (!xlu_cfg_get_long (config, "nr_spis", &l, 0))
+ b_info->arch_arm.nr_spis = l;
+
parse_vkb_list(config, d_config);
d_config->virtios = NULL;
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 5/8] xen/arm/gic: Allow routing/removing interrupt to running VMs
2024-05-16 10:03 [PATCH v2 0/8] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
` (3 preceding siblings ...)
2024-05-16 10:03 ` [PATCH v2 4/8] tools/arm: Introduce the "nr_spis" xl config entry Henry Wang
@ 2024-05-16 10:03 ` Henry Wang
2024-05-17 6:03 ` Henry Wang
2024-05-16 10:03 ` [PATCH v2 6/8] xen/arm: Add XEN_DOMCTL_dt_overlay DOMCTL and related operations Henry Wang
` (2 subsequent siblings)
7 siblings, 1 reply; 27+ messages in thread
From: Henry Wang @ 2024-05-16 10:03 UTC (permalink / raw
To: xen-devel
Cc: Vikram Garhwal, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Volodymyr Babchuk, Henry Wang
From: Vikram Garhwal <fnu.vikram@xilinx.com>
Currently, routing/removing physical interrupts are only allowed at
the domain creation/destroy time. For use cases such as dynamic device
tree overlay adding/removing, the routing/removing of physical IRQ to
running domains should be allowed.
Removing the above-mentioned domain creation/dying check. Since this
will introduce interrupt state unsync issues for cases when the
interrupt is active or pending in the guest, therefore for these cases
we simply reject the operation. Do it for both new and old vGIC
implementations.
Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
Signed-off-by: Henry Wang <xin.wang2@amd.com>
---
v2:
- Reject the case where the IRQ is active or pending in guest.
---
xen/arch/arm/gic-vgic.c | 8 ++++++--
xen/arch/arm/gic.c | 15 ---------------
xen/arch/arm/vgic/vgic.c | 5 +++--
3 files changed, 9 insertions(+), 19 deletions(-)
diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
index 56490dbc43..d1608415f8 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -444,14 +444,18 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, unsigned int virq,
{
/* The VIRQ should not be already enabled by the guest */
if ( !p->desc &&
- !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
+ !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
+ !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) &&
+ !test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
p->desc = desc;
else
ret = -EBUSY;
}
else
{
- if ( desc && p->desc != desc )
+ if ( desc && p->desc != desc &&
+ (test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ||
+ test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status)) )
ret = -EINVAL;
else
p->desc = NULL;
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 44c40e86de..3ebd89940a 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -135,14 +135,6 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
ASSERT(virq < vgic_num_irqs(d));
ASSERT(!is_lpi(virq));
- /*
- * When routing an IRQ to guest, the virtual state is not synced
- * back to the physical IRQ. To prevent get unsync, restrict the
- * routing to when the Domain is been created.
- */
- if ( d->creation_finished )
- return -EBUSY;
-
ret = vgic_connect_hw_irq(d, NULL, virq, desc, true);
if ( ret )
return ret;
@@ -167,13 +159,6 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
ASSERT(test_bit(_IRQ_GUEST, &desc->status));
ASSERT(!is_lpi(virq));
- /*
- * Removing an interrupt while the domain is running may have
- * undesirable effect on the vGIC emulation.
- */
- if ( !d->is_dying )
- return -EBUSY;
-
desc->handler->shutdown(desc);
/* EOI the IRQ if it has not been done by the guest */
diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
index b9463a5f27..785ef2b192 100644
--- a/xen/arch/arm/vgic/vgic.c
+++ b/xen/arch/arm/vgic/vgic.c
@@ -877,7 +877,7 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *vcpu,
if ( connect ) /* assign a mapped IRQ */
{
/* The VIRQ should not be already enabled by the guest */
- if ( !irq->hw && !irq->enabled )
+ if ( !irq->hw && !irq->enabled && !irq->active && !irq->pending_latch )
{
irq->hw = true;
irq->hwintid = desc->irq;
@@ -887,7 +887,8 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *vcpu,
}
else /* remove a mapped IRQ */
{
- if ( desc && irq->hwintid != desc->irq )
+ if ( desc && irq->hwintid != desc->irq &&
+ (irq->active || irq->pending_latch) )
{
ret = -EINVAL;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 6/8] xen/arm: Add XEN_DOMCTL_dt_overlay DOMCTL and related operations
2024-05-16 10:03 [PATCH v2 0/8] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
` (4 preceding siblings ...)
2024-05-16 10:03 ` [PATCH v2 5/8] xen/arm/gic: Allow routing/removing interrupt to running VMs Henry Wang
@ 2024-05-16 10:03 ` Henry Wang
2024-05-16 12:31 ` Jan Beulich
2024-05-16 10:03 ` [PATCH v2 7/8] tools: Introduce the "xl dt-overlay {attach,detach}" commands Henry Wang
2024-05-16 10:03 ` [PATCH v2 8/8] docs: Add device tree overlay documentation Henry Wang
7 siblings, 1 reply; 27+ messages in thread
From: Henry Wang @ 2024-05-16 10:03 UTC (permalink / raw
To: xen-devel
Cc: Henry Wang, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk, Andrew Cooper, George Dunlap,
Jan Beulich
In order to support the dynamic dtbo device assignment to a running
VM, the add/remove of the DT overlay and the attach/detach of the
device from the DT overlay should happen separately. Therefore,
repurpose the existing XEN_SYSCTL_dt_overlay to only add the DT
overlay to Xen device tree, instead of assigning the device to the
hardware domain at the same time. Add the XEN_DOMCTL_dt_overlay with
operations XEN_DOMCTL_DT_OVERLAY_{ATTACH,DETACH} to do/undo the
device assignment to the domain.
The hypervisor firstly checks the DT overlay passed from the toolstack
is valid. Then the device nodes are retrieved from the overlay tracker
based on the DT overlay. The attach/detach of the device is implemented
by map/unmap the IRQ and IOMMU resources. Note that with these changes,
the device de-registration from the IOMMU driver should only happen at
the time when the DT overlay is removed from the Xen device tree.
Signed-off-by: Henry Wang <xin.wang2@amd.com>
Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
---
v2:
- New patch.
---
xen/arch/arm/domctl.c | 3 +
xen/common/dt-overlay.c | 415 ++++++++++++++++++++++++++++-------
xen/include/public/domctl.h | 15 ++
xen/include/public/sysctl.h | 7 +-
xen/include/xen/dt-overlay.h | 7 +
5 files changed, 366 insertions(+), 81 deletions(-)
diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
index ad56efb0f5..12a12ee781 100644
--- a/xen/arch/arm/domctl.c
+++ b/xen/arch/arm/domctl.c
@@ -5,6 +5,7 @@
* Copyright (c) 2012, Citrix Systems
*/
+#include <xen/dt-overlay.h>
#include <xen/errno.h>
#include <xen/guest_access.h>
#include <xen/hypercall.h>
@@ -176,6 +177,8 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
return rc;
}
+ case XEN_DOMCTL_dt_overlay:
+ return dt_overlay_domctl(d, &domctl->u.dt_overlay);
default:
return subarch_do_domctl(domctl, d, u_domctl);
}
diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c
index 9cece79067..593e985949 100644
--- a/xen/common/dt-overlay.c
+++ b/xen/common/dt-overlay.c
@@ -356,24 +356,100 @@ static int overlay_get_nodes_info(const void *fdto, char **nodes_full_path)
return 0;
}
+static int remove_irq(unsigned long s, unsigned long e, void *data)
+{
+ struct domain *d = data;
+ int rc = 0;
+
+ /*
+ * IRQ should always have access unless there are duplication of
+ * of irqs in device tree. There are few cases of xen device tree
+ * where there are duplicate interrupts for the same node.
+ */
+ if (!irq_access_permitted(d, s))
+ return 0;
+ /*
+ * TODO: We don't handle shared IRQs for now. So, it is assumed that
+ * the IRQs was not shared with another domain.
+ */
+ rc = irq_deny_access(d, s);
+ if ( rc )
+ {
+ printk(XENLOG_ERR "unable to revoke access for irq %ld\n", s);
+ return rc;
+ }
+
+ rc = release_guest_irq(d, s);
+ if ( rc )
+ {
+ printk(XENLOG_ERR "unable to release irq %ld\n", s);
+ return rc;
+ }
+
+ return rc;
+}
+
+static int remove_all_irqs(struct rangeset *irq_ranges, struct domain *d)
+{
+ return rangeset_report_ranges(irq_ranges, 0, ~0UL, remove_irq, d);
+}
+
+static int remove_iomem(unsigned long s, unsigned long e, void *data)
+{
+ struct domain *d = data;
+ int rc = 0;
+ p2m_type_t t;
+ mfn_t mfn;
+
+ mfn = p2m_lookup(d, _gfn(s), &t);
+ if ( mfn_x(mfn) == 0 || mfn_x(mfn) == ~0UL )
+ return -EINVAL;
+
+ rc = iomem_deny_access(d, s, e);
+ if ( rc )
+ {
+ printk(XENLOG_ERR "Unable to remove %pd access to %#lx - %#lx\n",
+ d, s, e);
+ return rc;
+ }
+
+ rc = unmap_mmio_regions(d, _gfn(s), e - s, _mfn(s));
+ if ( rc )
+ return rc;
+
+ return rc;
+}
+
+static int remove_all_iomems(struct rangeset *iomem_ranges, struct domain *d)
+{
+ return rangeset_report_ranges(iomem_ranges, 0, ~0UL, remove_iomem, d);
+}
+
/* Check if node itself can be removed and remove node from IOMMU. */
-static int remove_node_resources(struct dt_device_node *device_node)
+static int remove_node_resources(struct dt_device_node *device_node,
+ struct domain *d)
{
int rc = 0;
unsigned int len;
domid_t domid;
- domid = dt_device_used_by(device_node);
+ if ( !d )
+ {
+ domid = dt_device_used_by(device_node);
- dt_dprintk("Checking if node %s is used by any domain\n",
- device_node->full_name);
+ dt_dprintk("Checking if node %s is used by any domain\n",
+ device_node->full_name);
- /* Remove the node if only it's assigned to hardware domain or domain io. */
- if ( domid != hardware_domain->domain_id && domid != DOMID_IO )
- {
- printk(XENLOG_ERR "Device %s is being used by domain %u. Removing nodes failed\n",
- device_node->full_name, domid);
- return -EINVAL;
+ /*
+ * We also check if device is assigned to DOMID_IO as when a domain
+ * is destroyed device is assigned to DOMID_IO.
+ */
+ if ( domid != DOMID_IO )
+ {
+ printk(XENLOG_ERR "Device %s is being assigned to %u. Device is assigned to %d\n",
+ device_node->full_name, DOMID_IO, domid);
+ return -EINVAL;
+ }
}
/* Check if iommu property exists. */
@@ -381,9 +457,12 @@ static int remove_node_resources(struct dt_device_node *device_node)
{
if ( dt_device_is_protected(device_node) )
{
- rc = iommu_remove_dt_device(device_node);
- if ( rc < 0 )
- return rc;
+ if ( !list_empty(&device_node->domain_list) )
+ {
+ rc = iommu_deassign_dt_device(d, device_node);
+ if ( rc < 0 )
+ return rc;
+ }
}
}
@@ -392,7 +471,8 @@ static int remove_node_resources(struct dt_device_node *device_node)
/* Remove all descendants from IOMMU. */
static int
-remove_descendant_nodes_resources(const struct dt_device_node *device_node)
+remove_descendant_nodes_resources(const struct dt_device_node *device_node,
+ struct domain *d)
{
int rc = 0;
struct dt_device_node *child_node;
@@ -402,12 +482,12 @@ remove_descendant_nodes_resources(const struct dt_device_node *device_node)
{
if ( child_node->child )
{
- rc = remove_descendant_nodes_resources(child_node);
+ rc = remove_descendant_nodes_resources(child_node, d);
if ( rc )
return rc;
}
- rc = remove_node_resources(child_node);
+ rc = remove_node_resources(child_node, d);
if ( rc )
return rc;
}
@@ -420,8 +500,7 @@ static int remove_nodes(const struct overlay_track *tracker)
{
int rc = 0;
struct dt_device_node *overlay_node;
- unsigned int j;
- struct domain *d = hardware_domain;
+ unsigned int j, len;
for ( j = 0; j < tracker->num_nodes; j++ )
{
@@ -431,18 +510,15 @@ static int remove_nodes(const struct overlay_track *tracker)
write_lock(&dt_host_lock);
- rc = remove_descendant_nodes_resources(overlay_node);
- if ( rc )
- {
- write_unlock(&dt_host_lock);
- return rc;
- }
-
- rc = remove_node_resources(overlay_node);
- if ( rc )
+ /* Check if iommu property exists. */
+ if ( dt_get_property(overlay_node, "iommus", &len) )
{
- write_unlock(&dt_host_lock);
- return rc;
+ if ( dt_device_is_protected(overlay_node) )
+ {
+ rc = iommu_remove_dt_device(overlay_node);
+ if ( rc < 0 )
+ return rc;
+ }
}
dt_dprintk("Removing node: %s\n", overlay_node->full_name);
@@ -457,22 +533,6 @@ static int remove_nodes(const struct overlay_track *tracker)
write_unlock(&dt_host_lock);
}
- /* Remove IRQ access. */
- if ( tracker->irq_ranges )
- {
- rc = rangeset_consume_ranges(tracker->irq_ranges, irq_remove_cb, d);
- if ( rc )
- return rc;
- }
-
- /* Remove mmio access. */
- if ( tracker->iomem_ranges )
- {
- rc = rangeset_consume_ranges(tracker->iomem_ranges, iomem_remove_cb, d);
- if ( rc )
- return rc;
- }
-
return rc;
}
@@ -536,9 +596,6 @@ static long handle_remove_overlay_nodes(const void *overlay_fdt,
xfree(entry->nodes_address);
- rangeset_destroy(entry->irq_ranges);
- rangeset_destroy(entry->iomem_ranges);
-
xfree(entry);
out:
@@ -620,15 +677,7 @@ static long add_nodes(struct overlay_track *tr, char **nodes_full_path)
return -EFAULT;
}
- rc = handle_device(hardware_domain, overlay_node, p2m_mmio_direct_c,
- tr->iomem_ranges,
- tr->irq_ranges);
write_unlock(&dt_host_lock);
- if ( rc )
- {
- printk(XENLOG_ERR "Adding IRQ and IOMMU failed\n");
- return rc;
- }
/* Keep overlay_node address in tracker. */
tr->nodes_address[j] = (unsigned long)overlay_node;
@@ -638,9 +687,7 @@ static long add_nodes(struct overlay_track *tr, char **nodes_full_path)
}
/*
* Adds device tree nodes under target node.
- * We use tr->dt_host_new to unflatten the updated device_tree_flattened. This
- * is done to avoid the removal of device_tree generation, iomem regions mapping
- * to hardware domain done by handle_node().
+ * We use tr->dt_host_new to unflatten the updated device_tree_flattened.
*/
static long handle_add_overlay_nodes(void *overlay_fdt,
uint32_t overlay_fdt_size)
@@ -774,20 +821,6 @@ static long handle_add_overlay_nodes(void *overlay_fdt,
goto err;
}
- tr->irq_ranges = rangeset_new(hardware_domain, "Overlays: Interrupts", 0);
- if (tr->irq_ranges == NULL)
- {
- printk(XENLOG_ERR "Creating IRQ rangeset failed");
- goto err;
- }
-
- tr->iomem_ranges = rangeset_new(hardware_domain, "Overlay: I/O Memory", 0);
- if (tr->iomem_ranges == NULL)
- {
- printk(XENLOG_ERR "Creating IOMMU rangeset failed");
- goto err;
- }
-
rc = add_nodes(tr, nodes_full_path);
if ( rc )
{
@@ -843,14 +876,205 @@ static long handle_add_overlay_nodes(void *overlay_fdt,
xfree(tr->nodes_address);
xfree(tr->fdt);
- rangeset_destroy(tr->irq_ranges);
- rangeset_destroy(tr->iomem_ranges);
-
xfree(tr);
return rc;
}
+static long handle_detach_overlay_nodes(struct domain *d,
+ const void *overlay_fdt,
+ uint32_t overlay_fdt_size)
+{
+ int rc;
+ unsigned int j;
+ struct overlay_track *entry, *temp, *track;
+ bool found_entry = false;
+
+ rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size);
+ if ( rc )
+ return rc;
+
+ spin_lock(&overlay_lock);
+
+ /*
+ * First check if dtbo is correct i.e. it should one of the dtbo which was
+ * used when dynamically adding the node.
+ * Limitation: Cases with same node names but different property are not
+ * supported currently. We are relying on user to provide the same dtbo
+ * as it was used when adding the nodes.
+ */
+ list_for_each_entry_safe( entry, temp, &overlay_tracker, entry )
+ {
+ if ( memcmp(entry->overlay_fdt, overlay_fdt, overlay_fdt_size) == 0 )
+ {
+ track = entry;
+ found_entry = true;
+ break;
+ }
+ }
+
+ if ( !found_entry )
+ {
+ rc = -EINVAL;
+ printk(XENLOG_ERR "Cannot find any matching tracker with input dtbo."
+ " Detaching nodes is supported only for prior added dtbo.\n");
+ goto out;
+
+ }
+
+ for ( j = 0; j < entry->num_nodes; j++ )
+ {
+ struct dt_device_node *overlay_node;
+
+ overlay_node = (struct dt_device_node *)entry->nodes_address[j];
+ if ( overlay_node == NULL )
+ {
+ rc = -EINVAL;
+ goto out;
+ }
+
+ write_lock(&dt_host_lock);
+ rc = remove_descendant_nodes_resources(overlay_node, d);
+ if ( rc )
+ {
+ write_unlock(&dt_host_lock);
+ goto out;
+ }
+
+ rc = remove_node_resources(overlay_node, d);
+ if ( rc )
+ {
+ write_unlock(&dt_host_lock);
+ goto out;
+ }
+ write_unlock(&dt_host_lock);
+
+ rc = remove_all_irqs(entry->irq_ranges, d);
+ if ( rc )
+ goto out;
+
+ rc = remove_all_iomems(entry->iomem_ranges, d);
+ if ( rc )
+ goto out;
+ }
+
+ /* Remove IRQ access. */
+ if ( entry->irq_ranges )
+ {
+ rc = rangeset_consume_ranges(entry->irq_ranges, irq_remove_cb, d);
+ if ( rc )
+ goto out;
+ }
+
+ /* Remove mmio access. */
+ if ( entry->iomem_ranges )
+ {
+ rc = rangeset_consume_ranges(entry->iomem_ranges, iomem_remove_cb, d);
+ if ( rc )
+ goto out;
+ }
+
+ rangeset_destroy(entry->irq_ranges);
+ rangeset_destroy(entry->iomem_ranges);
+
+ out:
+ spin_unlock(&overlay_lock);
+
+ return rc;
+}
+
+static long handle_attach_overlay_nodes(struct domain *d,
+ const void *overlay_fdt,
+ uint32_t overlay_fdt_size)
+{
+ int rc;
+ unsigned int j;
+ struct overlay_track *entry, *temp, *track;
+ bool found_entry = false;
+
+ rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size);
+ if ( rc )
+ return rc;
+
+ spin_lock(&overlay_lock);
+
+ /*
+ * First check if dtbo is correct i.e. it should one of the dtbo which was
+ * used when dynamically adding the node.
+ * Limitation: Cases with same node names but different property are not
+ * supported currently. We are relying on user to provide the same dtbo
+ * as it was used when adding the nodes.
+ */
+ list_for_each_entry_safe( entry, temp, &overlay_tracker, entry )
+ {
+ if ( memcmp(entry->overlay_fdt, overlay_fdt, overlay_fdt_size) == 0 )
+ {
+ track = entry;
+ found_entry = true;
+ break;
+ }
+ }
+
+ if ( !found_entry )
+ {
+ rc = -EINVAL;
+ printk(XENLOG_ERR "Cannot find any matching tracker with input dtbo."
+ " Attaching nodes is supported only for prior added dtbo.\n");
+ goto out;
+
+ }
+
+ entry->irq_ranges = rangeset_new(d, "Overlays: Interrupts", 0);
+ if (entry->irq_ranges == NULL)
+ {
+ rc = -ENOMEM;
+ printk(XENLOG_ERR "Creating IRQ rangeset failed");
+ goto out;
+ }
+
+ entry->iomem_ranges = rangeset_new(d, "Overlay: I/O Memory", 0);
+ if (entry->iomem_ranges == NULL)
+ {
+ rc = -ENOMEM;
+ printk(XENLOG_ERR "Creating IOMMU rangeset failed");
+ goto out;
+ }
+
+ for ( j = 0; j < entry->num_nodes; j++ )
+ {
+ struct dt_device_node *overlay_node;
+
+ overlay_node = (struct dt_device_node *)entry->nodes_address[j];
+ if ( overlay_node == NULL )
+ {
+ rc = -EINVAL;
+ goto out;
+ }
+
+ write_lock(&dt_host_lock);
+ rc = handle_device(d, overlay_node, p2m_mmio_direct_c,
+ entry->iomem_ranges, entry->irq_ranges);
+ write_unlock(&dt_host_lock);
+ if ( rc )
+ {
+ printk(XENLOG_ERR "Adding IRQ and IOMMU failed\n");
+ goto out;
+ }
+ }
+
+ spin_unlock(&overlay_lock);
+
+ return 0;
+
+ out:
+ spin_unlock(&overlay_lock);
+
+ rangeset_destroy(entry->irq_ranges);
+ rangeset_destroy(entry->iomem_ranges);
+
+ return rc;
+}
+
long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op)
{
long ret;
@@ -890,6 +1114,45 @@ long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op)
return ret;
}
+long dt_overlay_domctl(struct domain *d, struct xen_domctl_dt_overlay *op)
+{
+ long ret;
+ void *overlay_fdt;
+
+ if ( op->overlay_op != XEN_DOMCTL_DT_OVERLAY_ATTACH &&
+ op->overlay_op != XEN_DOMCTL_DT_OVERLAY_DETACH )
+ return -EOPNOTSUPP;
+
+ if ( op->overlay_fdt_size == 0 || op->overlay_fdt_size > KB(500) )
+ return -EINVAL;
+
+ if ( op->pad[0] || op->pad[1] || op->pad[2] )
+ return -EINVAL;
+
+ overlay_fdt = xmalloc_bytes(op->overlay_fdt_size);
+
+ if ( overlay_fdt == NULL )
+ return -ENOMEM;
+
+ ret = copy_from_guest(overlay_fdt, op->overlay_fdt, op->overlay_fdt_size);
+ if ( ret )
+ {
+ gprintk(XENLOG_ERR, "copy from guest failed\n");
+ xfree(overlay_fdt);
+
+ return -EFAULT;
+ }
+
+ if ( op->overlay_op == XEN_DOMCTL_DT_OVERLAY_DETACH )
+ ret = handle_detach_overlay_nodes(d, overlay_fdt, op->overlay_fdt_size);
+ else
+ ret = handle_attach_overlay_nodes(d, overlay_fdt, op->overlay_fdt_size);
+
+ xfree(overlay_fdt);
+
+ return ret;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index a33f9ec32b..95bfb3c0e8 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1190,6 +1190,17 @@ struct xen_domctl_vmtrace_op {
typedef struct xen_domctl_vmtrace_op xen_domctl_vmtrace_op_t;
DEFINE_XEN_GUEST_HANDLE(xen_domctl_vmtrace_op_t);
+#if defined(__arm__) || defined (__aarch64__)
+struct xen_domctl_dt_overlay {
+ XEN_GUEST_HANDLE_64(const_void) overlay_fdt; /* IN: overlay fdt. */
+ uint32_t overlay_fdt_size; /* IN: Overlay dtb size. */
+#define XEN_DOMCTL_DT_OVERLAY_ATTACH 3
+#define XEN_DOMCTL_DT_OVERLAY_DETACH 4
+ uint8_t overlay_op; /* IN: Attach or detach. */
+ uint8_t pad[3]; /* IN: Must be zero. */
+};
+#endif
+
struct xen_domctl {
uint32_t cmd;
#define XEN_DOMCTL_createdomain 1
@@ -1277,6 +1288,7 @@ struct xen_domctl {
#define XEN_DOMCTL_vmtrace_op 84
#define XEN_DOMCTL_get_paging_mempool_size 85
#define XEN_DOMCTL_set_paging_mempool_size 86
+#define XEN_DOMCTL_dt_overlay 87
#define XEN_DOMCTL_gdbsx_guestmemio 1000
#define XEN_DOMCTL_gdbsx_pausevcpu 1001
#define XEN_DOMCTL_gdbsx_unpausevcpu 1002
@@ -1339,6 +1351,9 @@ struct xen_domctl {
struct xen_domctl_vuart_op vuart_op;
struct xen_domctl_vmtrace_op vmtrace_op;
struct xen_domctl_paging_mempool paging_mempool;
+#if defined(__arm__) || defined (__aarch64__)
+ struct xen_domctl_dt_overlay dt_overlay;
+#endif
uint8_t pad[128];
} u;
};
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index febaa4b16a..b613babdf9 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -1187,11 +1187,8 @@ DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpu_policy_t);
#if defined(__arm__) || defined (__aarch64__)
/*
* XEN_SYSCTL_dt_overlay
- * Performs addition/removal of device tree nodes under parent node using dtbo.
- * This does in three steps:
- * - Adds/Removes the nodes from dt_host.
- * - Adds/Removes IRQ permission for the nodes.
- * - Adds/Removes MMIO accesses.
+ * Performs addition/removal of device tree nodes under parent node using dtbo
+ * from dt_host.
*/
struct xen_sysctl_dt_overlay {
XEN_GUEST_HANDLE_64(const_void) overlay_fdt; /* IN: overlay fdt. */
diff --git a/xen/include/xen/dt-overlay.h b/xen/include/xen/dt-overlay.h
index c0567741ee..64c9e34a8a 100644
--- a/xen/include/xen/dt-overlay.h
+++ b/xen/include/xen/dt-overlay.h
@@ -14,6 +14,7 @@
#include <xen/device_tree.h>
#include <xen/list.h>
#include <xen/rangeset.h>
+#include <public/domctl.h>
/*
* overlay_track describes information about added nodes through dtbo.
@@ -42,12 +43,18 @@ struct xen_sysctl_dt_overlay;
#ifdef CONFIG_OVERLAY_DTB
long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op);
+long dt_overlay_domctl(struct domain *d, struct xen_domctl_dt_overlay *op);
#else
#include <xen/errno.h>
static inline long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op)
{
return -EOPNOTSUPP;
}
+static inline long dt_overlay_domctl(struct domain *d,
+ struct xen_domctl_dt_overlay *op)
+{
+ return -EOPNOTSUPP;
+}
#endif
#endif /* __XEN_DT_OVERLAY_H__ */
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 7/8] tools: Introduce the "xl dt-overlay {attach,detach}" commands
2024-05-16 10:03 [PATCH v2 0/8] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
` (5 preceding siblings ...)
2024-05-16 10:03 ` [PATCH v2 6/8] xen/arm: Add XEN_DOMCTL_dt_overlay DOMCTL and related operations Henry Wang
@ 2024-05-16 10:03 ` Henry Wang
2024-05-20 19:41 ` Jason Andryuk
2024-05-16 10:03 ` [PATCH v2 8/8] docs: Add device tree overlay documentation Henry Wang
7 siblings, 1 reply; 27+ messages in thread
From: Henry Wang @ 2024-05-16 10:03 UTC (permalink / raw
To: xen-devel; +Cc: Henry Wang, Anthony PERARD, Juergen Gross
With the XEN_DOMCTL_dt_overlay DOMCTL added, users should be able to
attach/detach devices from the provided DT overlay to domains.
Support this by introducing a new set of "xl dt-overlay" commands and
related documentation, i.e. "xl dt-overlay {attach,detach}". Slightly
rework the command option parsing logic.
Since the addition of these two commands modifies the existing libxl
API libxl_dt_overlay(), also provide the backward compatible for it.
Signed-off-by: Henry Wang <xin.wang2@amd.com>
---
v2:
- New patch.
---
tools/include/libxl.h | 15 ++++++++++++-
tools/include/xenctrl.h | 3 +++
tools/libs/ctrl/xc_dt_overlay.c | 31 +++++++++++++++++++++++++++
tools/libs/light/libxl_dt_overlay.c | 30 ++++++++++++++++++++------
tools/xl/xl_cmdtable.c | 4 ++--
tools/xl/xl_vmcontrol.c | 33 +++++++++++++++++++----------
6 files changed, 96 insertions(+), 20 deletions(-)
diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index 62cb07dea6..27aab4bcee 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -2549,8 +2549,21 @@ libxl_device_pci *libxl_device_pci_list(libxl_ctx *ctx, uint32_t domid,
void libxl_device_pci_list_free(libxl_device_pci* list, int num);
#if defined(__arm__) || defined(__aarch64__)
-int libxl_dt_overlay(libxl_ctx *ctx, void *overlay,
+#define LIBXL_DT_OVERLAY_ADD 1
+#define LIBXL_DT_OVERLAY_REMOVE 2
+#define LIBXL_DT_OVERLAY_ATTACH 3
+#define LIBXL_DT_OVERLAY_DETACH 4
+
+int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domain_id, void *overlay,
uint32_t overlay_size, uint8_t overlay_op);
+#if defined(LIBXL_API_VERSION) && LIBXL_API_VERSION < 0x041900
+int libxl_dt_overlay_0x041800(libxl_ctx *ctx, void *overlay,
+ uint32_t overlay_size, uint8_t overlay_op);
+{
+ return libxl_dt_overlay(ctx, 0, overlay, overlay_size, overlay_op);
+}
+#define libxl_dt_overlay libxl_dt_overlay_0x041800
+#endif
#endif
/*
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 4996855944..9ceca0cffc 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2657,6 +2657,9 @@ int xc_domain_cacheflush(xc_interface *xch, uint32_t domid,
#if defined(__arm__) || defined(__aarch64__)
int xc_dt_overlay(xc_interface *xch, void *overlay_fdt,
uint32_t overlay_fdt_size, uint8_t overlay_op);
+int xc_dt_overlay_domain(xc_interface *xch, void *overlay_fdt,
+ uint32_t overlay_fdt_size, uint8_t overlay_op,
+ uint32_t domain_id);
#endif
/* Compat shims */
diff --git a/tools/libs/ctrl/xc_dt_overlay.c b/tools/libs/ctrl/xc_dt_overlay.c
index c2224c4d15..ea1da522d1 100644
--- a/tools/libs/ctrl/xc_dt_overlay.c
+++ b/tools/libs/ctrl/xc_dt_overlay.c
@@ -48,3 +48,34 @@ err:
return err;
}
+
+int xc_dt_overlay_domain(xc_interface *xch, void *overlay_fdt,
+ uint32_t overlay_fdt_size, uint8_t overlay_op,
+ uint32_t domain_id)
+{
+ int err;
+ struct xen_domctl domctl = {
+ .cmd = XEN_DOMCTL_dt_overlay,
+ .domain = domain_id,
+ .u.dt_overlay = {
+ .overlay_op = overlay_op,
+ .overlay_fdt_size = overlay_fdt_size,
+ }
+ };
+
+ DECLARE_HYPERCALL_BOUNCE(overlay_fdt, overlay_fdt_size,
+ XC_HYPERCALL_BUFFER_BOUNCE_IN);
+
+ if ( (err = xc_hypercall_bounce_pre(xch, overlay_fdt)) )
+ goto err;
+
+ set_xen_guest_handle(domctl.u.dt_overlay.overlay_fdt, overlay_fdt);
+
+ if ( (err = do_domctl(xch, &domctl)) != 0 )
+ PERROR("%s failed", __func__);
+
+err:
+ xc_hypercall_bounce_post(xch, overlay_fdt);
+
+ return err;
+}
diff --git a/tools/libs/light/libxl_dt_overlay.c b/tools/libs/light/libxl_dt_overlay.c
index a6c709a6dc..9110b1efd2 100644
--- a/tools/libs/light/libxl_dt_overlay.c
+++ b/tools/libs/light/libxl_dt_overlay.c
@@ -41,8 +41,8 @@ static int check_overlay_fdt(libxl__gc *gc, void *fdt, size_t size)
return 0;
}
-int libxl_dt_overlay(libxl_ctx *ctx, void *overlay_dt, uint32_t overlay_dt_size,
- uint8_t overlay_op)
+int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domain_id, void *overlay_dt,
+ uint32_t overlay_dt_size, uint8_t overlay_op)
{
int rc;
int r;
@@ -57,11 +57,29 @@ int libxl_dt_overlay(libxl_ctx *ctx, void *overlay_dt, uint32_t overlay_dt_size,
rc = 0;
}
- r = xc_dt_overlay(ctx->xch, overlay_dt, overlay_dt_size, overlay_op);
-
- if (r) {
- LOG(ERROR, "%s: Adding/Removing overlay dtb failed.", __func__);
+ switch (overlay_op)
+ {
+ case LIBXL_DT_OVERLAY_ADD:
+ case LIBXL_DT_OVERLAY_REMOVE:
+ r = xc_dt_overlay(ctx->xch, overlay_dt, overlay_dt_size, overlay_op);
+ if (r) {
+ LOG(ERROR, "%s: Adding/Removing overlay dtb failed.", __func__);
+ rc = ERROR_FAIL;
+ }
+ break;
+ case LIBXL_DT_OVERLAY_ATTACH:
+ case LIBXL_DT_OVERLAY_DETACH:
+ r = xc_dt_overlay_domain(ctx->xch, overlay_dt, overlay_dt_size,
+ overlay_op, domain_id);
+ if (r) {
+ LOG(ERROR, "%s: Attaching/Detaching overlay dtb failed.", __func__);
+ rc = ERROR_FAIL;
+ }
+ break;
+ default:
+ LOG(ERROR, "%s: Invalid overlay dtb op.", __func__);
rc = ERROR_FAIL;
+ break;
}
out:
diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
index 1f3c6b5897..37770b20e3 100644
--- a/tools/xl/xl_cmdtable.c
+++ b/tools/xl/xl_cmdtable.c
@@ -634,8 +634,8 @@ const struct cmd_spec cmd_table[] = {
#ifdef LIBXL_HAVE_DT_OVERLAY
{ "dt-overlay",
&main_dt_overlay, 0, 1,
- "Add/Remove a device tree overlay",
- "add/remove <.dtbo>",
+ "Add/Remove a device tree overlay to Xen device tree, attach/detach the device to a domain",
+ "<operation=add|remove> <.dtbo> OR <operation=attach|detach> <.dtbo> <Domain>",
"-h print this help\n"
},
#endif
diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index 02575d5d36..53d1fa3655 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -1268,32 +1268,43 @@ int main_create(int argc, char **argv)
#ifdef LIBXL_HAVE_DT_OVERLAY
int main_dt_overlay(int argc, char **argv)
{
- const char *overlay_ops = NULL;
const char *overlay_config_file = NULL;
void *overlay_dtb = NULL;
int rc;
uint8_t op;
int overlay_dtb_size = 0;
- const int overlay_add_op = 1;
- const int overlay_remove_op = 2;
+ uint32_t domain_id = 0;
if (argc < 2) {
help("dt-overlay");
return EXIT_FAILURE;
}
- overlay_ops = argv[1];
- overlay_config_file = argv[2];
-
- if (strcmp(overlay_ops, "add") == 0)
- op = overlay_add_op;
- else if (strcmp(overlay_ops, "remove") == 0)
- op = overlay_remove_op;
+ if (strcmp(argv[optind], "add") == 0)
+ op = LIBXL_DT_OVERLAY_ADD;
+ else if (strcmp(argv[optind], "remove") == 0)
+ op = LIBXL_DT_OVERLAY_REMOVE;
+ else if (strcmp(argv[optind], "attach") == 0)
+ op = LIBXL_DT_OVERLAY_ATTACH;
+ else if (strcmp(argv[optind], "detach") == 0)
+ op = LIBXL_DT_OVERLAY_DETACH;
else {
fprintf(stderr, "Invalid dt overlay operation\n");
return EXIT_FAILURE;
}
+ overlay_config_file = argv[optind+1];
+
+ if (op == LIBXL_DT_OVERLAY_ATTACH || op == LIBXL_DT_OVERLAY_DETACH) {
+ if (argc <= optind + 2) {
+ fprintf(stderr, "Missing domain ID\n");
+ help("dt-overlay");
+ return EXIT_FAILURE;
+ } else {
+ domain_id = strtol(argv[optind+2], NULL, 10);
+ }
+ }
+
if (overlay_config_file) {
rc = libxl_read_file_contents(ctx, overlay_config_file,
&overlay_dtb, &overlay_dtb_size);
@@ -1309,7 +1320,7 @@ int main_dt_overlay(int argc, char **argv)
return EXIT_FAILURE;
}
- rc = libxl_dt_overlay(ctx, overlay_dtb, overlay_dtb_size, op);
+ rc = libxl_dt_overlay(ctx, domain_id, overlay_dtb, overlay_dtb_size, op);
free(overlay_dtb);
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 8/8] docs: Add device tree overlay documentation
2024-05-16 10:03 [PATCH v2 0/8] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
` (6 preceding siblings ...)
2024-05-16 10:03 ` [PATCH v2 7/8] tools: Introduce the "xl dt-overlay {attach,detach}" commands Henry Wang
@ 2024-05-16 10:03 ` Henry Wang
7 siblings, 0 replies; 27+ messages in thread
From: Henry Wang @ 2024-05-16 10:03 UTC (permalink / raw
To: xen-devel
Cc: Vikram Garhwal, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Volodymyr Babchuk, Henry Wang
From: Vikram Garhwal <fnu.vikram@xilinx.com>
Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
Signed-off-by: Henry Wang <xin.wang2@amd.com>
---
v2:
- Update the content based on the changes in this version.
---
docs/misc/arm/overlay.txt | 99 +++++++++++++++++++++++++++++++++++++++
1 file changed, 99 insertions(+)
create mode 100644 docs/misc/arm/overlay.txt
diff --git a/docs/misc/arm/overlay.txt b/docs/misc/arm/overlay.txt
new file mode 100644
index 0000000000..811a6de369
--- /dev/null
+++ b/docs/misc/arm/overlay.txt
@@ -0,0 +1,99 @@
+# Device Tree Overlays support in Xen
+
+Xen now supports dynamic device assignment to running domains,
+i.e. adding/removing nodes (using .dtbo) to/from Xen device tree, and
+attaching/detaching them to/from a running domain with given $domid.
+
+Dynamic node assignment works in two steps:
+
+## Add/Remove device tree overlay to/from Xen device tree
+
+1. Xen tools check the dtbo given and parse all other user provided arguments
+2. Xen tools pass the dtbo to Xen hypervisor via hypercall.
+3. Xen hypervisor applies/removes the dtbo to/from Xen device tree.
+
+## Attach/Detach device from the DT overlay to/from domain
+
+1. Xen tools check the dtbo given and parse all other user provided arguments
+2. Xen tools pass the dtbo to Xen hypervisor via hypercall.
+3. Xen hypervisor attach/detach the device to/from the user-provided $domid by
+ mapping/unmapping node resources in the DT overlay.
+
+# Examples
+
+Here are a few examples on how to use it.
+
+## Dom0 device add
+
+For assigning a device tree overlay to Dom0, user should firstly properly
+prepare the DT overlay. More information about device tree overlays can be
+found in [1]. Then, in Dom0, enter the following:
+
+ (dom0) xl dt-overlay add overlay.dtbo
+
+This will allocate the devices mentioned in overlay.dtbo to Xen device tree.
+
+To assign the newly added device from the dtbo to Dom0:
+
+ (dom0) xl dt-overlay attach overlay.dtbo 0
+
+Next, if the user wants to add the same device tree overlay to dom0
+Linux, execute the following:
+
+ (dom0) mkdir -p /sys/kernel/config/device-tree/overlays/new_overlay
+ (dom0) cat overlay.dtbo > /sys/kernel/config/device-tree/overlays/new_overlay/dtbo
+
+Finally if needed, the relevant Linux kernel drive can be loaded using:
+
+ (dom0) modprobe module_name.ko
+
+## Dom0 device remove
+
+For removing the device from Dom0, first detach the device from Dom0:
+
+ (dom0) xl dt-overlay detach overlay.dtbo 0
+
+NOTE: The user is expected to unload any Linux kernel modules which
+might be accessing the devices in overlay.dtbo before detach the device.
+Detaching devices without unloading the modules might result in a crash.
+
+Then remove the overlay from Xen device tree:
+
+ (dom0) xl dt-overlay remove overlay.dtbo
+
+## DomU device add/remove
+
+All the nodes in dtbo will be assigned to a domain; the user will need
+to prepare the dtb for the domU. For example, the `interrupt-parent` property
+of the DomU overlay should be changed to the Xen hardcoded value `0xfde8`.
+Below assumes the properly written DomU dtbo is `overlay_domu.dtbo`.
+
+User will need to create the DomU with below properties properly configured
+in the xl config file:
+- `iomem`
+- `passthrough` (if IOMMU is needed)
+
+User will also need to modprobe the relevant drivers.
+
+Example for domU device add:
+
+ (dom0) xl dt-overlay add overlay.dtbo # If not executed before
+ (dom0) xl dt-overlay attach overlay.dtbo $domid
+ (dom0) xl console $domid # To access $domid console
+
+Next, if the user needs to modify/prepare the overlay.dtbo suitable for
+the domU:
+
+ (domU) mkdir -p /sys/kernel/config/device-tree/overlays/new_overlay
+ (domU) cat overlay_domu.dtbo > /sys/kernel/config/device-tree/overlays/new_overlay/dtbo
+
+Finally, if needed, the relevant Linux kernel drive can be probed:
+
+ (domU) modprobe module_name.ko
+
+Example for domU overlay remove:
+
+ (dom0) xl dt-overlay detach overlay.dtbo $domid
+ (dom0) xl dt-overlay remove overlay.dtbo
+
+[1] https://www.kernel.org/doc/Documentation/devicetree/overlay-notes.txt
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2 6/8] xen/arm: Add XEN_DOMCTL_dt_overlay DOMCTL and related operations
2024-05-16 10:03 ` [PATCH v2 6/8] xen/arm: Add XEN_DOMCTL_dt_overlay DOMCTL and related operations Henry Wang
@ 2024-05-16 12:31 ` Jan Beulich
2024-05-17 1:36 ` Henry Wang
0 siblings, 1 reply; 27+ messages in thread
From: Jan Beulich @ 2024-05-16 12:31 UTC (permalink / raw
To: Henry Wang
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, xen-devel
On 16.05.2024 12:03, Henry Wang wrote:
> +static long handle_detach_overlay_nodes(struct domain *d,
> + const void *overlay_fdt,
> + uint32_t overlay_fdt_size)
> +{
> + int rc;
> + unsigned int j;
> + struct overlay_track *entry, *temp, *track;
> + bool found_entry = false;
> +
> + rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size);
> + if ( rc )
> + return rc;
> +
> + spin_lock(&overlay_lock);
> +
> + /*
> + * First check if dtbo is correct i.e. it should one of the dtbo which was
> + * used when dynamically adding the node.
> + * Limitation: Cases with same node names but different property are not
> + * supported currently. We are relying on user to provide the same dtbo
> + * as it was used when adding the nodes.
> + */
> + list_for_each_entry_safe( entry, temp, &overlay_tracker, entry )
> + {
> + if ( memcmp(entry->overlay_fdt, overlay_fdt, overlay_fdt_size) == 0 )
> + {
> + track = entry;
Random question (not doing a full review of the DT code): What use is
this (and the track variable itself)? It's never used further down afaics.
Same for attach.
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1190,6 +1190,17 @@ struct xen_domctl_vmtrace_op {
> typedef struct xen_domctl_vmtrace_op xen_domctl_vmtrace_op_t;
> DEFINE_XEN_GUEST_HANDLE(xen_domctl_vmtrace_op_t);
>
> +#if defined(__arm__) || defined (__aarch64__)
Nit: Consistent use of blanks please (also again below).
> +struct xen_domctl_dt_overlay {
> + XEN_GUEST_HANDLE_64(const_void) overlay_fdt; /* IN: overlay fdt. */
> + uint32_t overlay_fdt_size; /* IN: Overlay dtb size. */
> +#define XEN_DOMCTL_DT_OVERLAY_ATTACH 3
> +#define XEN_DOMCTL_DT_OVERLAY_DETACH 4
While the numbers don't really matter much, picking 3 and 4 rather than,
say, 1 and 2 still looks a little odd.
> --- a/xen/include/xen/dt-overlay.h
> +++ b/xen/include/xen/dt-overlay.h
> @@ -14,6 +14,7 @@
> #include <xen/device_tree.h>
> #include <xen/list.h>
> #include <xen/rangeset.h>
> +#include <public/domctl.h>
Why? All you need here ...
> @@ -42,12 +43,18 @@ struct xen_sysctl_dt_overlay;
>
> #ifdef CONFIG_OVERLAY_DTB
> long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op);
> +long dt_overlay_domctl(struct domain *d, struct xen_domctl_dt_overlay *op);
... is a forward declaration of struct xen_domctl_dt_overlay.
Jan
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 6/8] xen/arm: Add XEN_DOMCTL_dt_overlay DOMCTL and related operations
2024-05-16 12:31 ` Jan Beulich
@ 2024-05-17 1:36 ` Henry Wang
2024-05-17 7:11 ` Jan Beulich
0 siblings, 1 reply; 27+ messages in thread
From: Henry Wang @ 2024-05-17 1:36 UTC (permalink / raw
To: Jan Beulich
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, xen-devel
Hi Jan,
As usual, thanks for the review!
On 5/16/2024 8:31 PM, Jan Beulich wrote:
> On 16.05.2024 12:03, Henry Wang wrote:
>
> + /*
> + * First check if dtbo is correct i.e. it should one of the dtbo which was
> + * used when dynamically adding the node.
> + * Limitation: Cases with same node names but different property are not
> + * supported currently. We are relying on user to provide the same dtbo
> + * as it was used when adding the nodes.
> + */
> + list_for_each_entry_safe( entry, temp, &overlay_tracker, entry )
> + {
> + if ( memcmp(entry->overlay_fdt, overlay_fdt, overlay_fdt_size) == 0 )
> + {
> + track = entry;
> Random question (not doing a full review of the DT code): What use is
> this (and the track variable itself)? It's never used further down afaics.
> Same for attach.
I think you are correct, it is a copy paste of the existing code and the
track variable is indeed useless. So in v3, I will simply drop it and
mention this clean-up in commit message. Also I realized that the exact
logic of finding the entry is duplicated third times, so I will also
extract the logic to a function.
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -1190,6 +1190,17 @@ struct xen_domctl_vmtrace_op {
>> typedef struct xen_domctl_vmtrace_op xen_domctl_vmtrace_op_t;
>> DEFINE_XEN_GUEST_HANDLE(xen_domctl_vmtrace_op_t);
>>
>> +#if defined(__arm__) || defined (__aarch64__)
> Nit: Consistent use of blanks please (also again below).
Good catch. Will fix it.
>> +struct xen_domctl_dt_overlay {
>> + XEN_GUEST_HANDLE_64(const_void) overlay_fdt; /* IN: overlay fdt. */
>> + uint32_t overlay_fdt_size; /* IN: Overlay dtb size. */
>> +#define XEN_DOMCTL_DT_OVERLAY_ATTACH 3
>> +#define XEN_DOMCTL_DT_OVERLAY_DETACH 4
> While the numbers don't really matter much, picking 3 and 4 rather than,
> say, 1 and 2 still looks a little odd.
Well although I agree with you it is indeed a bit odd, the problem of
this is that, in current implementation I reused the libxl_dt_overlay()
(with proper backward compatible) to deliver the sysctl and domctl
depend on the op, and we have:
#define LIBXL_DT_OVERLAY_ADD 1
#define LIBXL_DT_OVERLAY_REMOVE 2
#define LIBXL_DT_OVERLAY_ATTACH 3
#define LIBXL_DT_OVERLAY_DETACH 4
Then the op-number is passed from the toolstack to Xen, and checked in
dt_overlay_domctl(). So with this implementation the attach/detach op
number should be 3 and 4 since 1 and 2 have different meanings.
But I realized that I can also implement a similar API, say
libxl_dt_overlay_domain() and that way we can reuse 1 and 2 and there is
not even need to provide backward compatible of libxl_dt_overlay(). So
would you mind sharing your preference on which approach would you like
more? Thanks!
>> --- a/xen/include/xen/dt-overlay.h
>> +++ b/xen/include/xen/dt-overlay.h
>> @@ -14,6 +14,7 @@
>> #include <xen/device_tree.h>
>> #include <xen/list.h>
>> #include <xen/rangeset.h>
>> +#include <public/domctl.h>
> Why? All you need here ...
>
>> @@ -42,12 +43,18 @@ struct xen_sysctl_dt_overlay;
>>
>> #ifdef CONFIG_OVERLAY_DTB
>> long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op);
>> +long dt_overlay_domctl(struct domain *d, struct xen_domctl_dt_overlay *op);
> ... is a forward declaration of struct xen_domctl_dt_overlay.
Oh indeed. Will fix this. Thanks!
Kind regards,
Henry
>
> Jan
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 5/8] xen/arm/gic: Allow routing/removing interrupt to running VMs
2024-05-16 10:03 ` [PATCH v2 5/8] xen/arm/gic: Allow routing/removing interrupt to running VMs Henry Wang
@ 2024-05-17 6:03 ` Henry Wang
2024-05-19 11:08 ` Julien Grall
0 siblings, 1 reply; 27+ messages in thread
From: Henry Wang @ 2024-05-17 6:03 UTC (permalink / raw
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
On 5/16/2024 6:03 PM, Henry Wang wrote:
> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>
> Currently, routing/removing physical interrupts are only allowed at
> the domain creation/destroy time. For use cases such as dynamic device
> tree overlay adding/removing, the routing/removing of physical IRQ to
> running domains should be allowed.
>
> Removing the above-mentioned domain creation/dying check. Since this
> will introduce interrupt state unsync issues for cases when the
> interrupt is active or pending in the guest, therefore for these cases
> we simply reject the operation. Do it for both new and old vGIC
> implementations.
>
> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> Signed-off-by: Henry Wang <xin.wang2@amd.com>
> ---
> v2:
> - Reject the case where the IRQ is active or pending in guest.
> ---
> xen/arch/arm/gic-vgic.c | 8 ++++++--
> xen/arch/arm/gic.c | 15 ---------------
> xen/arch/arm/vgic/vgic.c | 5 +++--
> 3 files changed, 9 insertions(+), 19 deletions(-)
>
> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> index 56490dbc43..d1608415f8 100644
> --- a/xen/arch/arm/gic-vgic.c
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -444,14 +444,18 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, unsigned int virq,
> {
> /* The VIRQ should not be already enabled by the guest */
> if ( !p->desc &&
> - !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
> + !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
> + !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) &&
> + !test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
> p->desc = desc;
> else
> ret = -EBUSY;
> }
> else
> {
> - if ( desc && p->desc != desc )
> + if ( desc && p->desc != desc &&
> + (test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ||
> + test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status)) )
This should be
+ if ( (desc && p->desc != desc) ||
+ test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ||
+ test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
> @@ -887,7 +887,8 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *vcpu,
> }
> else /* remove a mapped IRQ */
> {
> - if ( desc && irq->hwintid != desc->irq )
> + if ( desc && irq->hwintid != desc->irq &&
> + (irq->active || irq->pending_latch) )
Same here, this should be
+ if ( (desc && irq->hwintid != desc->irq) ||
+ irq->active || irq->pending_latch )
Kind regards,
Henry
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 6/8] xen/arm: Add XEN_DOMCTL_dt_overlay DOMCTL and related operations
2024-05-17 1:36 ` Henry Wang
@ 2024-05-17 7:11 ` Jan Beulich
0 siblings, 0 replies; 27+ messages in thread
From: Jan Beulich @ 2024-05-17 7:11 UTC (permalink / raw
To: Henry Wang
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, xen-devel
On 17.05.2024 03:36, Henry Wang wrote:
> On 5/16/2024 8:31 PM, Jan Beulich wrote:
>> On 16.05.2024 12:03, Henry Wang wrote:
>>> --- a/xen/include/public/domctl.h
>>> +++ b/xen/include/public/domctl.h
>>> @@ -1190,6 +1190,17 @@ struct xen_domctl_vmtrace_op {
>>> typedef struct xen_domctl_vmtrace_op xen_domctl_vmtrace_op_t;
>>> DEFINE_XEN_GUEST_HANDLE(xen_domctl_vmtrace_op_t);
>>>
>>> +#if defined(__arm__) || defined (__aarch64__)
>> Nit: Consistent use of blanks please (also again below).
>
> Good catch. Will fix it.
>
>>> +struct xen_domctl_dt_overlay {
>>> + XEN_GUEST_HANDLE_64(const_void) overlay_fdt; /* IN: overlay fdt. */
>>> + uint32_t overlay_fdt_size; /* IN: Overlay dtb size. */
>>> +#define XEN_DOMCTL_DT_OVERLAY_ATTACH 3
>>> +#define XEN_DOMCTL_DT_OVERLAY_DETACH 4
>> While the numbers don't really matter much, picking 3 and 4 rather than,
>> say, 1 and 2 still looks a little odd.
>
> Well although I agree with you it is indeed a bit odd, the problem of
> this is that, in current implementation I reused the libxl_dt_overlay()
> (with proper backward compatible) to deliver the sysctl and domctl
> depend on the op, and we have:
> #define LIBXL_DT_OVERLAY_ADD 1
> #define LIBXL_DT_OVERLAY_REMOVE 2
> #define LIBXL_DT_OVERLAY_ATTACH 3
> #define LIBXL_DT_OVERLAY_DETACH 4
>
> Then the op-number is passed from the toolstack to Xen, and checked in
> dt_overlay_domctl(). So with this implementation the attach/detach op
> number should be 3 and 4 since 1 and 2 have different meanings.
>
> But I realized that I can also implement a similar API, say
> libxl_dt_overlay_domain() and that way we can reuse 1 and 2 and there is
> not even need to provide backward compatible of libxl_dt_overlay(). So
> would you mind sharing your preference on which approach would you like
> more? Thanks!
While I think tying together libxl and domctl values isn't a very good idea,
if you really want to do so, you'll want to add suitable checking somewhere,
alongside comments. The comments ought to keep people from changing the
values then, while the checks would need to be there for people not paying
attention.
Jan
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/8] xen/common/dt-overlay: Fix lock issue when add/remove the device
2024-05-16 10:03 ` [PATCH v2 1/8] xen/common/dt-overlay: Fix lock issue when add/remove the device Henry Wang
@ 2024-05-19 10:04 ` Julien Grall
0 siblings, 0 replies; 27+ messages in thread
From: Julien Grall @ 2024-05-19 10:04 UTC (permalink / raw
To: Henry Wang, xen-devel; +Cc: Stefano Stabellini
Hi Henry,
On 16/05/2024 11:03, Henry Wang wrote:
> If CONFIG_DEBUG=y, below assertion will be triggered:
> (XEN) Assertion 'rw_is_locked(&dt_host_lock)' failed at drivers/passthrough/device_tree.c:146
> (XEN) ----[ Xen-4.19-unstable arm64 debug=y Not tainted ]----
> [...]
> (XEN) Xen call trace:
> (XEN) [<00000a0000257418>] iommu_remove_dt_device+0x8c/0xd4 (PC)
> (XEN) [<00000a00002573a0>] iommu_remove_dt_device+0x14/0xd4 (LR)
> (XEN) [<00000a000020797c>] dt-overlay.c#remove_node_resources+0x8c/0x90
> (XEN) [<00000a0000207f14>] dt-overlay.c#remove_nodes+0x524/0x648
> (XEN) [<00000a0000208460>] dt_overlay_sysctl+0x428/0xc68
> (XEN) [<00000a00002707f8>] arch_do_sysctl+0x1c/0x2c
> (XEN) [<00000a0000230b40>] do_sysctl+0x96c/0x9ec
> (XEN) [<00000a0000271e08>] traps.c#do_trap_hypercall+0x1e8/0x288
> (XEN) [<00000a0000273490>] do_trap_guest_sync+0x448/0x63c
> (XEN) [<00000a000025c480>] entry.o#guest_sync_slowpath+0xa8/0xd8
> (XEN)
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 0:
> (XEN) Assertion 'rw_is_locked(&dt_host_lock)' failed at drivers/passthrough/device_tree.c:146
> (XEN) ****************************************
>
> This is because iommu_remove_dt_device() is called without taking the
> dt_host_lock. dt_host_lock is meant to ensure that the DT node will not
> disappear behind back. So fix the issue by taking the lock as soon as
> getting hold of overlay_node.
>
> Similar issue will be observed in adding the dtbo:
> (XEN) Assertion 'system_state < SYS_STATE_active || rw_is_locked(&dt_host_lock)'
> failed at xen-source/xen/drivers/passthrough/device_tree.c:192
> (XEN) ----[ Xen-4.19-unstable arm64 debug=y Not tainted ]----
> [...]
> (XEN) Xen call trace:
> (XEN) [<00000a00002594f4>] iommu_add_dt_device+0x7c/0x17c (PC)
> (XEN) [<00000a0000259494>] iommu_add_dt_device+0x1c/0x17c (LR)
> (XEN) [<00000a0000267db4>] handle_device+0x68/0x1e8
> (XEN) [<00000a0000208ba8>] dt_overlay_sysctl+0x9d4/0xb84
> (XEN) [<00000a000027342c>] arch_do_sysctl+0x24/0x38
> (XEN) [<00000a0000231ac8>] do_sysctl+0x9ac/0xa34
> (XEN) [<00000a0000274b70>] traps.c#do_trap_hypercall+0x230/0x2dc
> (XEN) [<00000a0000276330>] do_trap_guest_sync+0x478/0x688
> (XEN) [<00000a000025e480>] entry.o#guest_sync_slowpath+0xa8/0xd8
>
> This is because the lock is released too early. So fix the issue by
> releasing the lock after handle_device().
>
> Fixes: 7e5c4a8b86f1 ("xen/arm: Implement device tree node removal functionalities")
> Signed-off-by: Henry Wang <xin.wang2@amd.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 3/8] xen/arm, doc: Add a DT property to specify IOMMU for Dom0less domUs
2024-05-16 10:03 ` [PATCH v2 3/8] xen/arm, doc: Add a DT property to specify IOMMU for Dom0less domUs Henry Wang
@ 2024-05-19 10:17 ` Julien Grall
2024-05-20 0:41 ` Henry Wang
0 siblings, 1 reply; 27+ messages in thread
From: Julien Grall @ 2024-05-19 10:17 UTC (permalink / raw
To: Henry Wang, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
Hi Henry,
On 16/05/2024 11:03, Henry Wang wrote:
> There are some use cases in which the dom0less domUs need to have
> the XEN_DOMCTL_CDF_iommu set at the domain construction time. For
> example, the dynamic dtbo feature allows the domain to be assigned
> a device that is behind the IOMMU at runtime. For these use cases,
> we need to have a way to specify the domain will need the IOMMU
> mapping at domain construction time.
>
> Introduce a "passthrough" DT property for Dom0less DomUs following
> the same entry as the xl.cfg. Currently only provide two options,
> i.e. "enable" and "disable". Set the XEN_DOMCTL_CDF_iommu at domain
> construction time based on the property.
>
> Signed-off-by: Henry Wang <xin.wang2@amd.com>
> ---
> v2:
> - New patch to replace the original patch in v1:
> "[PATCH 03/15] xen/arm: Always enable IOMMU"
> ---
> docs/misc/arm/device-tree/booting.txt | 13 +++++++++++++
> xen/arch/arm/dom0less-build.c | 7 +++++--
> 2 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index bbd955e9c2..61f9082553 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -260,6 +260,19 @@ with the following properties:
> value specified by Xen command line parameter gnttab_max_maptrack_frames
> (or its default value if unspecified, i.e. 1024) is used.
>
> +- passthrough
> +
> + A string property specifying whether IOMMU mappings are enabled for the
> + domain and hence whether it will be enabled for passthrough hardware.
> + Possible property values are:
> +
> + - "enabled"
> + IOMMU mappings are enabled for the domain.
> +
> + - "disabled"
> + IOMMU mappings are disabled for the domain and so hardware may not be
> + passed through. This option is the default if this property is missing.
Looking at the code below, it seems like the default will depend on
whether the partial device-tree is present. Did I misunderstand?
> +
> 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/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index 74f053c242..1396a102e1 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -848,6 +848,7 @@ static int __init construct_domU(struct domain *d,
> void __init create_domUs(void)
> {
> struct dt_device_node *node;
> + const char *dom0less_iommu;
> const struct dt_device_node *cpupool_node,
> *chosen = dt_find_node_by_path("/chosen");
>
> @@ -895,8 +896,10 @@ void __init create_domUs(void)
> panic("Missing property 'cpus' for domain %s\n",
> dt_node_name(node));
>
> - if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") &&
> - iommu_enabled )
> + if ( iommu_enabled &&
> + ((!dt_property_read_string(node, "passthrough", &dom0less_iommu) &&
> + !strcmp(dom0less_iommu, "enabled")) ||
> + dt_find_compatible_node(node, NULL, "multiboot,device-tree")) )
This condition is getting a little bit harder to read. Can we cache the
"passthrough" flag separately?
Also, shouldn't we throw a panic if passthrough = "enabled" but the
IOMMU is enabled?
> d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>
> if ( !dt_property_read_u32(node, "nr_spis", &d_cfg.arch.nr_spis) )
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 5/8] xen/arm/gic: Allow routing/removing interrupt to running VMs
2024-05-17 6:03 ` Henry Wang
@ 2024-05-19 11:08 ` Julien Grall
2024-05-20 1:01 ` Henry Wang
0 siblings, 1 reply; 27+ messages in thread
From: Julien Grall @ 2024-05-19 11:08 UTC (permalink / raw
To: Henry Wang, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
Hi,
On 17/05/2024 07:03, Henry Wang wrote:
>
>
> On 5/16/2024 6:03 PM, Henry Wang wrote:
>> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>>
>> Currently, routing/removing physical interrupts are only allowed at
>> the domain creation/destroy time. For use cases such as dynamic device
>> tree overlay adding/removing, the routing/removing of physical IRQ to
>> running domains should be allowed.
>>
>> Removing the above-mentioned domain creation/dying check. Since this
>> will introduce interrupt state unsync issues for cases when the
>> interrupt is active or pending in the guest, therefore for these cases
>> we simply reject the operation. Do it for both new and old vGIC
>> implementations.
>>
>> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
>> Signed-off-by: Henry Wang <xin.wang2@amd.com>
>> ---
>> v2:
>> - Reject the case where the IRQ is active or pending in guest.
>> ---
>> xen/arch/arm/gic-vgic.c | 8 ++++++--
>> xen/arch/arm/gic.c | 15 ---------------
>> xen/arch/arm/vgic/vgic.c | 5 +++--
>> 3 files changed, 9 insertions(+), 19 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
>> index 56490dbc43..d1608415f8 100644
>> --- a/xen/arch/arm/gic-vgic.c
>> +++ b/xen/arch/arm/gic-vgic.c
>> @@ -444,14 +444,18 @@ int vgic_connect_hw_irq(struct domain *d, struct
>> vcpu *v, unsigned int virq,
>> {
>> /* The VIRQ should not be already enabled by the guest */
This comment needs to be updated.
>> if ( !p->desc &&
>> - !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
>> + !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
>> + !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) &&
>> + !test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
>> p->desc = desc;
>> else
>> ret = -EBUSY;
>> }
>> else
>> {
>> - if ( desc && p->desc != desc )
>> + if ( desc && p->desc != desc &&
>> + (test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ||
>> + test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status)) )
>
> This should be
>
> + if ( (desc && p->desc != desc) ||
> + test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ||
> + test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
Looking at gic_set_lr(), we first check p->desc, before setting
IRQ_GUEST_VISIBLE.
I can't find a common lock, so what would guarantee that p->desc is not
going to be used or IRQ_GUEST_VISIBLE set afterwards?
>> @@ -887,7 +887,8 @@ int vgic_connect_hw_irq(struct domain *d, struct
>> vcpu *vcpu,
>> }
>> else /* remove a mapped IRQ */
>> {
>> - if ( desc && irq->hwintid != desc->irq )
>> + if ( desc && irq->hwintid != desc->irq &&
>> + (irq->active || irq->pending_latch) )
>
> Same here, this should be
>
> + if ( (desc && irq->hwintid != desc->irq) ||
> + irq->active || irq->pending_latch )
I think the new vGIC doesn't have the same problem because it looks like
the IRQ lock will be taken for any access to 'irq'.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 3/8] xen/arm, doc: Add a DT property to specify IOMMU for Dom0less domUs
2024-05-19 10:17 ` Julien Grall
@ 2024-05-20 0:41 ` Henry Wang
2024-05-20 1:38 ` Henry Wang
2024-05-20 1:40 ` Henry Wang
0 siblings, 2 replies; 27+ messages in thread
From: Henry Wang @ 2024-05-20 0:41 UTC (permalink / raw
To: Julien Grall, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
Hi Julien,
Thanks for spending time on the review!
On 5/19/2024 6:17 PM, Julien Grall wrote:
> Hi Henry,
>
> On 16/05/2024 11:03, Henry Wang wrote:
>> diff --git a/docs/misc/arm/device-tree/booting.txt
>> b/docs/misc/arm/device-tree/booting.txt
>> index bbd955e9c2..61f9082553 100644
>> --- a/docs/misc/arm/device-tree/booting.txt
>> +++ b/docs/misc/arm/device-tree/booting.txt
>> @@ -260,6 +260,19 @@ with the following properties:
>> value specified by Xen command line parameter
>> gnttab_max_maptrack_frames
>> (or its default value if unspecified, i.e. 1024) is used.
>> +- passthrough
>> +
>> + A string property specifying whether IOMMU mappings are enabled
>> for the
>> + domain and hence whether it will be enabled for passthrough
>> hardware.
>> + Possible property values are:
>> +
>> + - "enabled"
>> + IOMMU mappings are enabled for the domain.
>> +
>> + - "disabled"
>> + IOMMU mappings are disabled for the domain and so hardware may
>> not be
>> + passed through. This option is the default if this property is
>> missing.
>
> Looking at the code below, it seems like the default will depend on
> whether the partial device-tree is present. Did I misunderstand?
I am not sure if I understand the "partial device tree" in above comment
correctly. The "passthrough" property is supposed to be placed in the
dom0less domU domain node exactly the same way as the other dom0less
domU properties (such as "direct-map" etc.). This way we can control the
XEN_DOMCTL_CDF_iommu is set or not for each dom0less domU separately.
>> +
>> 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/dom0less-build.c
>> b/xen/arch/arm/dom0less-build.c
>> index 74f053c242..1396a102e1 100644
>> --- a/xen/arch/arm/dom0less-build.c
>> +++ b/xen/arch/arm/dom0less-build.c
>> @@ -848,6 +848,7 @@ static int __init construct_domU(struct domain *d,
>> void __init create_domUs(void)
>> {
>> struct dt_device_node *node;
>> + const char *dom0less_iommu;
>> const struct dt_device_node *cpupool_node,
>> *chosen =
>> dt_find_node_by_path("/chosen");
>> @@ -895,8 +896,10 @@ void __init create_domUs(void)
>> panic("Missing property 'cpus' for domain %s\n",
>> dt_node_name(node));
>> - if ( dt_find_compatible_node(node, NULL,
>> "multiboot,device-tree") &&
>> - iommu_enabled )
>> + if ( iommu_enabled &&
>> + ((!dt_property_read_string(node, "passthrough",
>> &dom0less_iommu) &&
>> + !strcmp(dom0less_iommu, "enabled")) ||
>> + dt_find_compatible_node(node, NULL,
>> "multiboot,device-tree")) )
>
> This condition is getting a little bit harder to read. Can we cache
> the "passthrough" flag separately?
Yes sure. Will do this in v3.
> Also, shouldn't we throw a panic if passthrough = "enabled" but the
> IOMMU is enabled?
I take the above "enabled" should be "disabled"? Actually we already
have several checks to do that: Firstly, the above if condition checks
the "iommu_enabled", so if IOMMU is disabled, the XEN_DOMCTL_CDF_iommu
is never set. Also, in later on domain config sanitising process, i.e.
domain_create() -> sanitise_domain_config(), there is also a check and
panic to check if XEN_DOMCTL_CDF_iommu is somehow set but IOMMU is
disabled. So I think these are sufficient for us. Did I understand your
comment correctly?
Kind regards,
Henry
>> d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>> if ( !dt_property_read_u32(node, "nr_spis",
>> &d_cfg.arch.nr_spis) )
>
> Cheers,
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 5/8] xen/arm/gic: Allow routing/removing interrupt to running VMs
2024-05-19 11:08 ` Julien Grall
@ 2024-05-20 1:01 ` Henry Wang
2024-05-20 9:50 ` Julien Grall
0 siblings, 1 reply; 27+ messages in thread
From: Henry Wang @ 2024-05-20 1:01 UTC (permalink / raw
To: Julien Grall, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
Hi Julien,
On 5/19/2024 7:08 PM, Julien Grall wrote:
> Hi,
>
> On 17/05/2024 07:03, Henry Wang wrote:
>>> @@ -444,14 +444,18 @@ int vgic_connect_hw_irq(struct domain *d,
>>> struct vcpu *v, unsigned int virq,
>>> {
>>> /* The VIRQ should not be already enabled by the guest */
>
> This comment needs to be updated.
Yes, sorry. I will update this and the one in the new vGIC in v3.
>>> if ( !p->desc &&
>>> - !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
>>> + !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
>>> + !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) &&
>>> + !test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
>>> p->desc = desc;
>>> else
>>> ret = -EBUSY;
>>> }
>>> else
>>> {
>>> - if ( desc && p->desc != desc )
>>> + if ( desc && p->desc != desc &&
>>> + (test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ||
>>> + test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status)) )
>>
>> This should be
>>
>> + if ( (desc && p->desc != desc) ||
>> + test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ||
>> + test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
> Looking at gic_set_lr(), we first check p->desc, before setting
> IRQ_GUEST_VISIBLE.
>
> I can't find a common lock, so what would guarantee that p->desc is
> not going to be used or IRQ_GUEST_VISIBLE set afterwards?
I think the gic_set_lr() is supposed to be called with v->arch.vgic.lock
taken, at least the current two callers (gic_raise_guest_irq() and
gic_restore_pending_irqs()) are doing it this way. Would this address
your concern? Thanks.
Kind regards,
Henry
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 3/8] xen/arm, doc: Add a DT property to specify IOMMU for Dom0less domUs
2024-05-20 0:41 ` Henry Wang
@ 2024-05-20 1:38 ` Henry Wang
2024-05-20 1:40 ` Henry Wang
1 sibling, 0 replies; 27+ messages in thread
From: Henry Wang @ 2024-05-20 1:38 UTC (permalink / raw
To: xen-devel
Hi Julien,
On 5/20/2024 8:41 AM, Henry Wang wrote:
> Hi Julien,
>
> Thanks for spending time on the review!
>
> On 5/19/2024 6:17 PM, Julien Grall wrote:
>> Hi Henry,
>>
>> On 16/05/2024 11:03, Henry Wang wrote:
>>> diff --git a/docs/misc/arm/device-tree/booting.txt
>>> b/docs/misc/arm/device-tree/booting.txt
>>> index bbd955e9c2..61f9082553 100644
>>> --- a/docs/misc/arm/device-tree/booting.txt
>>> +++ b/docs/misc/arm/device-tree/booting.txt
>>> @@ -260,6 +260,19 @@ with the following properties:
>>> value specified by Xen command line parameter
>>> gnttab_max_maptrack_frames
>>> (or its default value if unspecified, i.e. 1024) is used.
>>> +- passthrough
>>> +
>>> + A string property specifying whether IOMMU mappings are enabled
>>> for the
>>> + domain and hence whether it will be enabled for passthrough
>>> hardware.
>>> + Possible property values are:
>>> +
>>> + - "enabled"
>>> + IOMMU mappings are enabled for the domain.
>>> +
>>> + - "disabled"
>>> + IOMMU mappings are disabled for the domain and so hardware may
>>> not be
>>> + passed through. This option is the default if this property is
>>> missing.
>>
>> Looking at the code below, it seems like the default will depend on
>> whether the partial device-tree is present. Did I misunderstand?
>
> I am not sure if I understand the "partial device tree" in above
> comment correctly. The "passthrough" property is supposed to be placed
> in the dom0less domU domain node exactly the same way as the other
> dom0less domU properties (such as "direct-map" etc.). This way we can
> control the XEN_DOMCTL_CDF_iommu is set or not for each dom0less domU
> separately.
Oh I think I get your points, you meant the XEN_DOMCTL_CDF_iommu will
still be set if the passthrough dt property is "disabled", but user
provides a partial device tree. Yes you are correct. I will update the
doc to explain a bit more details as below. Thanks for pointing it out.
- "enabled"
IOMMU mappings are enabled for the domain. Note that this option is the
default if the user provides the device partial passthrough device tree
for the domain.
- "disabled"
IOMMU mappings are disabled for the domain and so hardware may not be
passed through. This option is the default if this property is missing
and the user does not provide the device partial device tree for
the domain.
Kind regards,
Henry
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 3/8] xen/arm, doc: Add a DT property to specify IOMMU for Dom0less domUs
2024-05-20 0:41 ` Henry Wang
2024-05-20 1:38 ` Henry Wang
@ 2024-05-20 1:40 ` Henry Wang
1 sibling, 0 replies; 27+ messages in thread
From: Henry Wang @ 2024-05-20 1:40 UTC (permalink / raw
To: Julien Grall, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
Hi Julien,
On 5/20/2024 8:41 AM, Henry Wang wrote:
> Hi Julien,
>
> Thanks for spending time on the review!
>
> On 5/19/2024 6:17 PM, Julien Grall wrote:
>> Hi Henry,
>>
>> On 16/05/2024 11:03, Henry Wang wrote:
>>> diff --git a/docs/misc/arm/device-tree/booting.txt
>>> b/docs/misc/arm/device-tree/booting.txt
>>> index bbd955e9c2..61f9082553 100644
>>> --- a/docs/misc/arm/device-tree/booting.txt
>>> +++ b/docs/misc/arm/device-tree/booting.txt
>>> @@ -260,6 +260,19 @@ with the following properties:
>>> value specified by Xen command line parameter
>>> gnttab_max_maptrack_frames
>>> (or its default value if unspecified, i.e. 1024) is used.
>>> +- passthrough
>>> +
>>> + A string property specifying whether IOMMU mappings are enabled
>>> for the
>>> + domain and hence whether it will be enabled for passthrough
>>> hardware.
>>> + Possible property values are:
>>> +
>>> + - "enabled"
>>> + IOMMU mappings are enabled for the domain.
>>> +
>>> + - "disabled"
>>> + IOMMU mappings are disabled for the domain and so hardware may
>>> not be
>>> + passed through. This option is the default if this property is
>>> missing.
>>
>> Looking at the code below, it seems like the default will depend on
>> whether the partial device-tree is present. Did I misunderstand?
>
> I am not sure if I understand the "partial device tree" in above
> comment correctly. The "passthrough" property is supposed to be placed
> in the dom0less domU domain node exactly the same way as the other
> dom0less domU properties (such as "direct-map" etc.). This way we can
> control the XEN_DOMCTL_CDF_iommu is set or not for each dom0less domU
> separately.
Oh I think I get your points, you meant the XEN_DOMCTL_CDF_iommu will
still be set if the passthrough dt property is "disabled", but user
provides a partial device tree. Yes you are correct. I will update the
doc to explain a bit more details as below. Thanks for pointing it out.
- "enabled"
IOMMU mappings are enabled for the domain. Note that this option is
the
default if the user provides the device partial passthrough device
tree
for the domain.
- "disabled"
IOMMU mappings are disabled for the domain and so hardware may not be
passed through. This option is the default if this property is missing
and the user does not provide the device partial device tree for
the domain.
Kind regards,
Henry
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 5/8] xen/arm/gic: Allow routing/removing interrupt to running VMs
2024-05-20 1:01 ` Henry Wang
@ 2024-05-20 9:50 ` Julien Grall
2024-05-21 2:29 ` Stefano Stabellini
0 siblings, 1 reply; 27+ messages in thread
From: Julien Grall @ 2024-05-20 9:50 UTC (permalink / raw
To: Henry Wang, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
Hi Henry,
On 20/05/2024 02:01, Henry Wang wrote:
> Hi Julien,
>
> On 5/19/2024 7:08 PM, Julien Grall wrote:
>> Hi,
>>
>> On 17/05/2024 07:03, Henry Wang wrote:
>>>> @@ -444,14 +444,18 @@ int vgic_connect_hw_irq(struct domain *d,
>>>> struct vcpu *v, unsigned int virq,
>>>> {
>>>> /* The VIRQ should not be already enabled by the guest */
>>
>> This comment needs to be updated.
>
> Yes, sorry. I will update this and the one in the new vGIC in v3.
>
>>>> if ( !p->desc &&
>>>> - !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
>>>> + !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
>>>> + !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) &&
>>>> + !test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
>>>> p->desc = desc;
>>>> else
>>>> ret = -EBUSY;
>>>> }
>>>> else
>>>> {
>>>> - if ( desc && p->desc != desc )
>>>> + if ( desc && p->desc != desc &&
>>>> + (test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ||
>>>> + test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status)) )
>>>
>>> This should be
>>>
>>> + if ( (desc && p->desc != desc) ||
>>> + test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ||
>>> + test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
>> Looking at gic_set_lr(), we first check p->desc, before setting
>> IRQ_GUEST_VISIBLE.
>>
>> I can't find a common lock, so what would guarantee that p->desc is
>> not going to be used or IRQ_GUEST_VISIBLE set afterwards?
>
> I think the gic_set_lr() is supposed to be called with v->arch.vgic.lock
> taken, at least the current two callers (gic_raise_guest_irq() and
> gic_restore_pending_irqs()) are doing it this way. Would this address
> your concern? Thanks.
I don't think it would address my concern. AFAICT, the lock is not taken
by vgic_connect_hw_irq().
I also haven't touched the vGIC for quite a while and didn't have much
time to dig into the code. Hence why I didn't propose a fix.
The vGIC code was mainly written by Stefano, so maybe he will have an
idea how this could be fixed.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/8] tools/xl: Correct the help information and exit code of the dt-overlay command
2024-05-16 10:03 ` [PATCH v2 2/8] tools/xl: Correct the help information and exit code of the dt-overlay command Henry Wang
@ 2024-05-20 18:48 ` Jason Andryuk
0 siblings, 0 replies; 27+ messages in thread
From: Jason Andryuk @ 2024-05-20 18:48 UTC (permalink / raw
To: Henry Wang, xen-devel; +Cc: Anthony PERARD, Anthony PERARD
On 2024-05-16 06:03, Henry Wang wrote:
> Fix the name mismatch in the xl dt-overlay command, the
> command name should be "dt-overlay" instead of "dt_overlay".
> Add the missing "," in the cmdtable.
>
> Fix the exit code of the dt-overlay command, use EXIT_FAILURE
> instead of ERROR_FAIL.
>
> Fixes: 61765a07e3d8 ("tools/xl: Add new xl command overlay for device tree overlay support")
> Suggested-by: Anthony PERARD <anthony.perard@cloud.com>
> Signed-off-by: Henry Wang <xin.wang2@amd.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
Thanks,
Jason
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 4/8] tools/arm: Introduce the "nr_spis" xl config entry
2024-05-16 10:03 ` [PATCH v2 4/8] tools/arm: Introduce the "nr_spis" xl config entry Henry Wang
@ 2024-05-20 19:13 ` Jason Andryuk
2024-05-20 23:19 ` Henry Wang
0 siblings, 1 reply; 27+ messages in thread
From: Jason Andryuk @ 2024-05-20 19:13 UTC (permalink / raw
To: Henry Wang, xen-devel
Cc: Anthony PERARD, George Dunlap, Nick Rosbrook, Juergen Gross
On 2024-05-16 06:03, Henry Wang wrote:
> Currently, the number of SPIs allocated to the domain is only
> configurable for Dom0less DomUs. Xen domains are supposed to be
> platform agnostics and therefore the numbers of SPIs for libxl
> guests should not be based on the hardware.
>
> Introduce a new xl config entry for Arm to provide a method for
> user to decide the number of SPIs. This would help to avoid
> bumping the `config->arch.nr_spis` in libxl everytime there is a
> new platform with increased SPI numbers.
>
> Update the doc and the golang bindings accordingly.
>
> Signed-off-by: Henry Wang <xin.wang2@amd.com>
> ---
> v2:
> - New patch to replace the original patch in v1:
> "[PATCH 05/15] tools/libs/light: Increase nr_spi to 160"
> ---
> docs/man/xl.cfg.5.pod.in | 11 +++++++++++
> tools/golang/xenlight/helpers.gen.go | 2 ++
> tools/golang/xenlight/types.gen.go | 1 +
> tools/libs/light/libxl_arm.c | 4 ++--
> tools/libs/light/libxl_types.idl | 1 +
> tools/xl/xl_parse.c | 3 +++
> 6 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index 8f2b375ce9..6a2d86065e 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -3072,6 +3072,17 @@ raised.
>
> =back
>
> +=over 4
> +
> +=item B<nr_spis="NR_SPIS">
> +
> +A 32-bit optional integer parameter specifying the number of SPIs (Shared
I'd phrase it "An optional 32-but integer"
> +Peripheral Interrupts) to allocate for the domain. If the `nr_spis` parameter
> +is missing, the max number of SPIs calculated by the toolstack based on the
> +devices allocated for the domain will be used.
This text says the maximum only applies if xl.cfg nr_spis is not setup.
> +
> +=back
> +
> =head3 x86
>
> =over 4
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 1cb89fa584..a4029e3ac8 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -181,8 +181,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>
> LOG(DEBUG, "Configure the domain");
>
> - config->arch.nr_spis = nr_spis;
> - LOG(DEBUG, " - Allocate %u SPIs", nr_spis);
> + config->arch.nr_spis = max(nr_spis, d_config->b_info.arch_arm.nr_spis);
> + LOG(DEBUG, " - Allocate %u SPIs", config->arch.nr_spis);
But this is always taking the max. Should it instead be:
config->arch.nr_spis = d_config->b_info.arch_arm.nr_spis ?: nr_spis;
However, I don't know if that makes sense for ARM. Does the hardware
nr_spis need to be a minimum for a domain?
Really, we just want the documentation to match the code.
Thanks,
Jason
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 7/8] tools: Introduce the "xl dt-overlay {attach,detach}" commands
2024-05-16 10:03 ` [PATCH v2 7/8] tools: Introduce the "xl dt-overlay {attach,detach}" commands Henry Wang
@ 2024-05-20 19:41 ` Jason Andryuk
2024-05-20 23:21 ` Henry Wang
0 siblings, 1 reply; 27+ messages in thread
From: Jason Andryuk @ 2024-05-20 19:41 UTC (permalink / raw
To: Henry Wang, xen-devel; +Cc: Anthony PERARD, Juergen Gross
On 2024-05-16 06:03, Henry Wang wrote:
> With the XEN_DOMCTL_dt_overlay DOMCTL added, users should be able to
> attach/detach devices from the provided DT overlay to domains.
> Support this by introducing a new set of "xl dt-overlay" commands and
> related documentation, i.e. "xl dt-overlay {attach,detach}". Slightly
> rework the command option parsing logic.
>
> Since the addition of these two commands modifies the existing libxl
> API libxl_dt_overlay(), also provide the backward compatible for it.
>
> Signed-off-by: Henry Wang <xin.wang2@amd.com>
> ---
> v2:
> - New patch.
Mostly looks good. One small thing below.
> diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
> index 02575d5d36..53d1fa3655 100644
> --- a/tools/xl/xl_vmcontrol.c
> +++ b/tools/xl/xl_vmcontrol.c
> @@ -1268,32 +1268,43 @@ int main_create(int argc, char **argv)
> #ifdef LIBXL_HAVE_DT_OVERLAY
> int main_dt_overlay(int argc, char **argv)
> {
> - const char *overlay_ops = NULL;
> const char *overlay_config_file = NULL;
> void *overlay_dtb = NULL;
> int rc;
> uint8_t op;
> int overlay_dtb_size = 0;
> - const int overlay_add_op = 1;
> - const int overlay_remove_op = 2;
> + uint32_t domain_id = 0;
>
> if (argc < 2) {
> help("dt-overlay");
> return EXIT_FAILURE;
> }
>
> - overlay_ops = argv[1];
> - overlay_config_file = argv[2];
> -
> - if (strcmp(overlay_ops, "add") == 0)
> - op = overlay_add_op;
> - else if (strcmp(overlay_ops, "remove") == 0)
> - op = overlay_remove_op;
> + if (strcmp(argv[optind], "add") == 0)
> + op = LIBXL_DT_OVERLAY_ADD;
> + else if (strcmp(argv[optind], "remove") == 0)
> + op = LIBXL_DT_OVERLAY_REMOVE;
> + else if (strcmp(argv[optind], "attach") == 0)
> + op = LIBXL_DT_OVERLAY_ATTACH;
> + else if (strcmp(argv[optind], "detach") == 0)
> + op = LIBXL_DT_OVERLAY_DETACH;
> else {
> fprintf(stderr, "Invalid dt overlay operation\n");
> return EXIT_FAILURE;
> }
>
> + overlay_config_file = argv[optind+1];
> +
> + if (op == LIBXL_DT_OVERLAY_ATTACH || op == LIBXL_DT_OVERLAY_DETACH) {
> + if (argc <= optind + 2) {
> + fprintf(stderr, "Missing domain ID\n");
> + help("dt-overlay");
> + return EXIT_FAILURE;
> + } else {
> + domain_id = strtol(argv[optind+2], NULL, 10);
domain_id = find_domain(argv[optind+2]);
And you'll get name resolution, too.
Thanks,
Jason
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 4/8] tools/arm: Introduce the "nr_spis" xl config entry
2024-05-20 19:13 ` Jason Andryuk
@ 2024-05-20 23:19 ` Henry Wang
0 siblings, 0 replies; 27+ messages in thread
From: Henry Wang @ 2024-05-20 23:19 UTC (permalink / raw
To: Jason Andryuk, xen-devel
Cc: Anthony PERARD, George Dunlap, Nick Rosbrook, Juergen Gross
Hi Jason,
On 5/21/2024 3:13 AM, Jason Andryuk wrote:
>> +
>> +=item B<nr_spis="NR_SPIS">
>> +
>> +A 32-bit optional integer parameter specifying the number of SPIs
>> (Shared
>
> I'd phrase it "An optional 32-but integer"
>
>> +Peripheral Interrupts) to allocate for the domain. If the `nr_spis`
>> parameter
>> +is missing, the max number of SPIs calculated by the toolstack based
>> on the
>> +devices allocated for the domain will be used.
>
> This text says the maximum only applies if xl.cfg nr_spis is not setup.
>
>> +
>> +=back
>> +
>> =head3 x86
>> =over 4
>
>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>> index 1cb89fa584..a4029e3ac8 100644
>> --- a/tools/libs/light/libxl_arm.c
>> +++ b/tools/libs/light/libxl_arm.c
>> @@ -181,8 +181,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>> LOG(DEBUG, "Configure the domain");
>> - config->arch.nr_spis = nr_spis;
>> - LOG(DEBUG, " - Allocate %u SPIs", nr_spis);
>> + config->arch.nr_spis = max(nr_spis,
>> d_config->b_info.arch_arm.nr_spis);
>> + LOG(DEBUG, " - Allocate %u SPIs", config->arch.nr_spis);
>
> But this is always taking the max. Should it instead be:
>
> config->arch.nr_spis = d_config->b_info.arch_arm.nr_spis ?: nr_spis;
>
> However, I don't know if that makes sense for ARM. Does the hardware
> nr_spis need to be a minimum for a domain?
>
> Really, we just want the documentation to match the code.
Before you pointed this out, I didn't realize the ambiguity in the doc
about the "max". The "max" in the doc have different meanings compared
to the "max()" in the code. I will drop the "max" in the doc and reword
the doc to "If the `nr_spis` parameter is missing, the number of SPIs
calculated by the toolstack based on the devices allocated for the
domain will be used.". Thanks for pointing it out.
Kind regards,
Henry
>
> Thanks,
> Jason
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 7/8] tools: Introduce the "xl dt-overlay {attach,detach}" commands
2024-05-20 19:41 ` Jason Andryuk
@ 2024-05-20 23:21 ` Henry Wang
0 siblings, 0 replies; 27+ messages in thread
From: Henry Wang @ 2024-05-20 23:21 UTC (permalink / raw
To: Jason Andryuk, xen-devel; +Cc: Anthony PERARD, Juergen Gross
Hi Jason,
On 5/21/2024 3:41 AM, Jason Andryuk wrote:
> On 2024-05-16 06:03, Henry Wang wrote:
>> + domain_id = strtol(argv[optind+2], NULL, 10);
>
> domain_id = find_domain(argv[optind+2]);
Good point, thanks. I will use the find_domain() in the next version.
Kind regards,
Henry
>
> And you'll get name resolution, too.
>
> Thanks,
> Jason
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 5/8] xen/arm/gic: Allow routing/removing interrupt to running VMs
2024-05-20 9:50 ` Julien Grall
@ 2024-05-21 2:29 ` Stefano Stabellini
0 siblings, 0 replies; 27+ messages in thread
From: Stefano Stabellini @ 2024-05-21 2:29 UTC (permalink / raw
To: Julien Grall
Cc: Henry Wang, xen-devel, Stefano Stabellini, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk
[-- Attachment #1: Type: text/plain, Size: 2736 bytes --]
On Mon, 20 May 2024, Julien Grall wrote:
> Hi Henry,
>
> On 20/05/2024 02:01, Henry Wang wrote:
> > Hi Julien,
> >
> > On 5/19/2024 7:08 PM, Julien Grall wrote:
> > > Hi,
> > >
> > > On 17/05/2024 07:03, Henry Wang wrote:
> > > > > @@ -444,14 +444,18 @@ int vgic_connect_hw_irq(struct domain *d, struct
> > > > > vcpu *v, unsigned int virq,
> > > > > {
> > > > > /* The VIRQ should not be already enabled by the guest */
> > >
> > > This comment needs to be updated.
> >
> > Yes, sorry. I will update this and the one in the new vGIC in v3.
> >
> > > > > if ( !p->desc &&
> > > > > - !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
> > > > > + !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
> > > > > + !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) &&
> > > > > + !test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
> > > > > p->desc = desc;
> > > > > else
> > > > > ret = -EBUSY;
> > > > > }
> > > > > else
> > > > > {
> > > > > - if ( desc && p->desc != desc )
> > > > > + if ( desc && p->desc != desc &&
> > > > > + (test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ||
> > > > > + test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status)) )
> > > >
> > > > This should be
> > > >
> > > > + if ( (desc && p->desc != desc) ||
> > > > + test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ||
> > > > + test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
> > > Looking at gic_set_lr(), we first check p->desc, before setting
> > > IRQ_GUEST_VISIBLE.
> > >
> > > I can't find a common lock, so what would guarantee that p->desc is not
> > > going to be used or IRQ_GUEST_VISIBLE set afterwards?
> >
> > I think the gic_set_lr() is supposed to be called with v->arch.vgic.lock
> > taken, at least the current two callers (gic_raise_guest_irq() and
> > gic_restore_pending_irqs()) are doing it this way. Would this address your
> > concern? Thanks.
>
> I don't think it would address my concern. AFAICT, the lock is not taken by
> vgic_connect_hw_irq().
>
> I also haven't touched the vGIC for quite a while and didn't have much time to
> dig into the code. Hence why I didn't propose a fix.
>
> The vGIC code was mainly written by Stefano, so maybe he will have an idea how
> this could be fixed.
I think we need to take the v->arch.vgic.lock just after the rank lock
in vgic_connect_hw_irq():
vgic_lock_rank(v_target, rank, flags);
spin_lock(&v_target->arch.vgic.lock);
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2024-05-21 2:29 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-16 10:03 [PATCH v2 0/8] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
2024-05-16 10:03 ` [PATCH v2 1/8] xen/common/dt-overlay: Fix lock issue when add/remove the device Henry Wang
2024-05-19 10:04 ` Julien Grall
2024-05-16 10:03 ` [PATCH v2 2/8] tools/xl: Correct the help information and exit code of the dt-overlay command Henry Wang
2024-05-20 18:48 ` Jason Andryuk
2024-05-16 10:03 ` [PATCH v2 3/8] xen/arm, doc: Add a DT property to specify IOMMU for Dom0less domUs Henry Wang
2024-05-19 10:17 ` Julien Grall
2024-05-20 0:41 ` Henry Wang
2024-05-20 1:38 ` Henry Wang
2024-05-20 1:40 ` Henry Wang
2024-05-16 10:03 ` [PATCH v2 4/8] tools/arm: Introduce the "nr_spis" xl config entry Henry Wang
2024-05-20 19:13 ` Jason Andryuk
2024-05-20 23:19 ` Henry Wang
2024-05-16 10:03 ` [PATCH v2 5/8] xen/arm/gic: Allow routing/removing interrupt to running VMs Henry Wang
2024-05-17 6:03 ` Henry Wang
2024-05-19 11:08 ` Julien Grall
2024-05-20 1:01 ` Henry Wang
2024-05-20 9:50 ` Julien Grall
2024-05-21 2:29 ` Stefano Stabellini
2024-05-16 10:03 ` [PATCH v2 6/8] xen/arm: Add XEN_DOMCTL_dt_overlay DOMCTL and related operations Henry Wang
2024-05-16 12:31 ` Jan Beulich
2024-05-17 1:36 ` Henry Wang
2024-05-17 7:11 ` Jan Beulich
2024-05-16 10:03 ` [PATCH v2 7/8] tools: Introduce the "xl dt-overlay {attach,detach}" commands Henry Wang
2024-05-20 19:41 ` Jason Andryuk
2024-05-20 23:21 ` Henry Wang
2024-05-16 10:03 ` [PATCH v2 8/8] docs: Add device tree overlay documentation Henry Wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).