* [PATCH 00/15] Remaining patches for dynamic node programming using overlay dtbo
@ 2024-04-24 3:34 Henry Wang
2024-04-24 3:34 ` [PATCH 01/15] xen/commom/dt-overlay: Fix missing lock when remove the device Henry Wang
` (15 more replies)
0 siblings, 16 replies; 58+ messages in thread
From: Henry Wang @ 2024-04-24 3:34 UTC (permalink / raw)
To: xen-devel
Cc: Henry Wang, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk, Anthony PERARD, Juergen Gross,
Andrew Cooper, George Dunlap, 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 to a running domain and will call unmap_mmio_regions()
to remove the mapping.
Also, documentation of the "dynamic node programming using overlay dtbo"
feature is added.
Patch 1 is a fix of [1] which is noticed during my local test, details
please see the commit message.
Gitlab CI for this series can be found in [2].
[1] https://lore.kernel.org/xen-devel/20230906011631.30310-1-vikram.garhwal@amd.com/
[2] https://gitlab.com/xen-project/people/henryw/xen/-/pipelines/1265297506
Henry Wang (1):
xen/commom/dt-overlay: Fix missing lock when remove the device
Vikram Garhwal (14):
xen/arm/gic: Enable interrupt assignment to running VM
xen/arm: Always enable IOMMU
tools/libs/light: Always enable IOMMU
tools/libs/light: Increase nr_spi to 160
rangeset: Move struct range and struct rangeset to headerfile
xen/overlay: Enable device tree overlay assignment to running domains
tools: Add domain_id and expert mode for overlay operations
tools/libs/light: Modify dtbo to domU linux dtbo format
tools/xl: Share overlay with domU
tools/helpers: Add get_overlay
get_overlay: remove domU overlay
xl/overlay: add remove operation to xenstore
add a domU script to fetch overlays and applying them to linux
docs: add device tree overlay documentation
docs/misc/arm/overlay.txt | 172 +++++++++
tools/helpers/Makefile | 8 +
tools/helpers/get_overlay.c | 507 ++++++++++++++++++++++++++
tools/helpers/get_overlay.sh | 81 +++++
tools/include/libxl.h | 8 +-
tools/include/xenctrl.h | 5 +-
tools/libs/ctrl/xc_dt_overlay.c | 7 +-
tools/libs/light/libxl_arm.c | 9 +-
tools/libs/light/libxl_dt_overlay.c | 90 ++++-
tools/xl/Makefile | 2 +-
tools/xl/xl_cmdtable.c | 2 +-
tools/xl/xl_vmcontrol.c | 539 +++++++++++++++++++++++++++-
xen/arch/arm/device.c | 8 +-
xen/arch/arm/dom0less-build.c | 3 +-
xen/arch/arm/domain_build.c | 2 +-
xen/arch/arm/gic.c | 4 +
xen/arch/arm/include/asm/setup.h | 3 +-
xen/common/dt-overlay.c | 223 ++++++++++--
xen/common/rangeset.c | 31 +-
xen/include/public/sysctl.h | 4 +-
xen/include/xen/rangeset.h | 32 +-
21 files changed, 1654 insertions(+), 86 deletions(-)
create mode 100644 docs/misc/arm/overlay.txt
create mode 100644 tools/helpers/get_overlay.c
create mode 100755 tools/helpers/get_overlay.sh
--
2.34.1
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH 01/15] xen/commom/dt-overlay: Fix missing lock when remove the device
2024-04-24 3:34 [PATCH 00/15] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
@ 2024-04-24 3:34 ` Henry Wang
2024-04-24 5:58 ` Jan Beulich
2024-04-24 3:34 ` [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM Henry Wang
` (14 subsequent siblings)
15 siblings, 1 reply; 58+ messages in thread
From: Henry Wang @ 2024-04-24 3:34 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) CPU: 0
(XEN) PC: 00000a0000257418 iommu_remove_dt_device+0x8c/0xd4
(XEN) LR: 00000a00002573a0
(XEN) SP: 00008000fff7fb30
(XEN) CPSR: 0000000000000249 MODE:64-bit EL2h (Hypervisor, handler)
[...]
(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. Fix the issue by taking and releasing the lock properly.
Fixes: 7e5c4a8b86f1 ("xen/arm: Implement device tree node removal functionalities")
Signed-off-by: Henry Wang <xin.wang2@amd.com>
---
xen/common/dt-overlay.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c
index 1b197381f6..39e4ba59dd 100644
--- a/xen/common/dt-overlay.c
+++ b/xen/common/dt-overlay.c
@@ -381,9 +381,14 @@ static int remove_node_resources(struct dt_device_node *device_node)
{
if ( dt_device_is_protected(device_node) )
{
+ write_lock(&dt_host_lock);
rc = iommu_remove_dt_device(device_node);
if ( rc < 0 )
+ {
+ write_unlock(&dt_host_lock);
return rc;
+ }
+ write_unlock(&dt_host_lock);
}
}
--
2.34.1
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM
2024-04-24 3:34 [PATCH 00/15] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
2024-04-24 3:34 ` [PATCH 01/15] xen/commom/dt-overlay: Fix missing lock when remove the device Henry Wang
@ 2024-04-24 3:34 ` Henry Wang
2024-04-24 12:58 ` Julien Grall
2024-04-24 3:34 ` [PATCH 03/15] xen/arm: Always enable IOMMU Henry Wang
` (13 subsequent siblings)
15 siblings, 1 reply; 58+ messages in thread
From: Henry Wang @ 2024-04-24 3:34 UTC (permalink / raw)
To: xen-devel
Cc: Vikram Garhwal, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Volodymyr Babchuk,
Stefano Stabellini, Henry Wang
From: Vikram Garhwal <fnu.vikram@xilinx.com>
Enable interrupt assign/remove for running VMs in CONFIG_OVERLAY_DTB.
Currently, irq_route and mapping is only allowed at the domain creation. Adding
exception for CONFIG_OVERLAY_DTB.
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>
---
xen/arch/arm/gic.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 44c40e86de..a775f886ed 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -140,8 +140,10 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
* back to the physical IRQ. To prevent get unsync, restrict the
* routing to when the Domain is been created.
*/
+#ifndef CONFIG_OVERLAY_DTB
if ( d->creation_finished )
return -EBUSY;
+#endif
ret = vgic_connect_hw_irq(d, NULL, virq, desc, true);
if ( ret )
@@ -171,8 +173,10 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
* Removing an interrupt while the domain is running may have
* undesirable effect on the vGIC emulation.
*/
+#ifndef CONFIG_OVERLAY_DTB
if ( !d->is_dying )
return -EBUSY;
+#endif
desc->handler->shutdown(desc);
--
2.34.1
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 03/15] xen/arm: Always enable IOMMU
2024-04-24 3:34 [PATCH 00/15] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
2024-04-24 3:34 ` [PATCH 01/15] xen/commom/dt-overlay: Fix missing lock when remove the device Henry Wang
2024-04-24 3:34 ` [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM Henry Wang
@ 2024-04-24 3:34 ` Henry Wang
2024-04-24 13:03 ` Julien Grall
2024-04-24 3:34 ` [PATCH 04/15] tools/libs/light: " Henry Wang
` (12 subsequent siblings)
15 siblings, 1 reply; 58+ messages in thread
From: Henry Wang @ 2024-04-24 3:34 UTC (permalink / raw)
To: xen-devel
Cc: Vikram Garhwal, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Volodymyr Babchuk,
Stefano Stabellini, Henry Wang
From: Vikram Garhwal <fnu.vikram@xilinx.com>
For overlay with iommu functionality to work with running VMs, we need to enable
IOMMU by default for the domains.
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>
---
xen/arch/arm/dom0less-build.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
index fb63ec6fd1..2d1fd1e214 100644
--- a/xen/arch/arm/dom0less-build.c
+++ b/xen/arch/arm/dom0less-build.c
@@ -894,7 +894,8 @@ 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") &&
+ if ( (IS_ENABLED(CONFIG_OVERLAY_DTB) ||
+ dt_find_compatible_node(node, NULL, "multiboot,device-tree")) &&
iommu_enabled )
d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
--
2.34.1
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 04/15] tools/libs/light: Always enable IOMMU
2024-04-24 3:34 [PATCH 00/15] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
` (2 preceding siblings ...)
2024-04-24 3:34 ` [PATCH 03/15] xen/arm: Always enable IOMMU Henry Wang
@ 2024-04-24 3:34 ` Henry Wang
2024-05-01 13:47 ` Anthony PERARD
2024-04-24 3:34 ` [PATCH 05/15] tools/libs/light: Increase nr_spi to 160 Henry Wang
` (11 subsequent siblings)
15 siblings, 1 reply; 58+ messages in thread
From: Henry Wang @ 2024-04-24 3:34 UTC (permalink / raw)
To: xen-devel; +Cc: Vikram Garhwal, Anthony PERARD, Juergen Gross, Henry Wang
From: Vikram Garhwal <fnu.vikram@xilinx.com>
For overlay with iommu functionality to work with running VMs, we need
to enable IOMMU when iomem presents for the domains.
Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
Signed-off-by: Henry Wang <xin.wang2@amd.com>
---
tools/libs/light/libxl_arm.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index 1cb89fa584..dd5c9f4917 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -222,6 +222,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
config->arch.sve_vl = d_config->b_info.arch_arm.sve_vl / 128U;
}
+#ifdef LIBXL_HAVE_DT_OVERLAY
+ if (d_config->b_info.num_iomem) {
+ config->flags |= XEN_DOMCTL_CDF_iommu;
+ }
+#endif
+
return 0;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 05/15] tools/libs/light: Increase nr_spi to 160
2024-04-24 3:34 [PATCH 00/15] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
` (3 preceding siblings ...)
2024-04-24 3:34 ` [PATCH 04/15] tools/libs/light: " Henry Wang
@ 2024-04-24 3:34 ` Henry Wang
2024-05-01 13:58 ` Anthony PERARD
2024-04-24 3:34 ` [PATCH 06/15] rangeset: Move struct range and struct rangeset to headerfile Henry Wang
` (10 subsequent siblings)
15 siblings, 1 reply; 58+ messages in thread
From: Henry Wang @ 2024-04-24 3:34 UTC (permalink / raw)
To: xen-devel
Cc: Vikram Garhwal, Anthony PERARD, Juergen Gross,
Stefano Stabellini, Henry Wang
From: Vikram Garhwal <fnu.vikram@xilinx.com>
Increase number of spi to 160 i.e. gic_number_lines() for Xilinx ZynqMP - 32.
This was done to allocate and assign IRQs to a running domain.
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>
---
tools/libs/light/libxl_arm.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index dd5c9f4917..50dbd0f2a9 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -181,7 +181,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
LOG(DEBUG, "Configure the domain");
- config->arch.nr_spis = nr_spis;
+ /* gic_number_lines() is 192 for Xilinx ZynqMP. min nr_spis = 192 - 32. */
+ config->arch.nr_spis = MAX(nr_spis, 160);
LOG(DEBUG, " - Allocate %u SPIs", nr_spis);
switch (d_config->b_info.arch_arm.gic_version) {
--
2.34.1
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 06/15] rangeset: Move struct range and struct rangeset to headerfile
2024-04-24 3:34 [PATCH 00/15] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
` (4 preceding siblings ...)
2024-04-24 3:34 ` [PATCH 05/15] tools/libs/light: Increase nr_spi to 160 Henry Wang
@ 2024-04-24 3:34 ` Henry Wang
2024-04-24 6:22 ` Jan Beulich
2024-04-24 3:34 ` [PATCH 07/15] xen/overlay: Enable device tree overlay assignment to running domains Henry Wang
` (9 subsequent siblings)
15 siblings, 1 reply; 58+ messages in thread
From: Henry Wang @ 2024-04-24 3:34 UTC (permalink / raw)
To: xen-devel
Cc: Vikram Garhwal, Andrew Cooper, George Dunlap, Jan Beulich,
Julien Grall, Stefano Stabellini, Henry Wang
From: Vikram Garhwal <vikram.garhwal@amd.com>
Move struct range, rangeset and removed static from first_range and next_range().
IRQs and IOMEMs for nodes are stored as rangeset in the dynamic node addition
part. While removing the nodes we need to access every IRQ and IOMEM ranges to
unmap IRQ and IOMEM from the domain.
Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
Signed-off-by: Henry Wang <xin.wang2@amd.com>
---
xen/common/rangeset.c | 31 ++-----------------------------
xen/include/xen/rangeset.h | 32 +++++++++++++++++++++++++++++++-
2 files changed, 33 insertions(+), 30 deletions(-)
diff --git a/xen/common/rangeset.c b/xen/common/rangeset.c
index b75590f907..d3f4297e41 100644
--- a/xen/common/rangeset.c
+++ b/xen/common/rangeset.c
@@ -12,31 +12,6 @@
#include <xen/rangeset.h>
#include <xsm/xsm.h>
-/* An inclusive range [s,e] and pointer to next range in ascending order. */
-struct range {
- struct list_head list;
- unsigned long s, e;
-};
-
-struct rangeset {
- /* Owning domain and threaded list of rangesets. */
- struct list_head rangeset_list;
- struct domain *domain;
-
- /* Ordered list of ranges contained in this set, and protecting lock. */
- struct list_head range_list;
-
- /* Number of ranges that can be allocated */
- long nr_ranges;
- rwlock_t lock;
-
- /* Pretty-printing name. */
- char name[32];
-
- /* RANGESETF flags. */
- unsigned int flags;
-};
-
/*****************************
* Private range functions hide the underlying linked-list implemnetation.
*/
@@ -57,8 +32,7 @@ static struct range *find_range(
return x;
}
-/* Return the lowest range in the set r, or NULL if r is empty. */
-static struct range *first_range(
+struct range *first_range(
struct rangeset *r)
{
if ( list_empty(&r->range_list) )
@@ -66,8 +40,7 @@ static struct range *first_range(
return list_entry(r->range_list.next, struct range, list);
}
-/* Return range following x in ascending order, or NULL if x is the highest. */
-static struct range *next_range(
+struct range *next_range(
struct rangeset *r, struct range *x)
{
if ( x->list.next == &r->range_list )
diff --git a/xen/include/xen/rangeset.h b/xen/include/xen/rangeset.h
index 96c9180825..cd80fd9179 100644
--- a/xen/include/xen/rangeset.h
+++ b/xen/include/xen/rangeset.h
@@ -13,7 +13,37 @@
#include <xen/types.h>
struct domain;
-struct rangeset;
+
+struct rangeset {
+ /* Owning domain and threaded list of rangesets. */
+ struct list_head rangeset_list;
+ struct domain *domain;
+
+ /* Ordered list of ranges contained in this set, and protecting lock. */
+ struct list_head range_list;
+
+ /* Number of ranges that can be allocated */
+ long nr_ranges;
+ rwlock_t lock;
+
+ /* Pretty-printing name. */
+ char name[32];
+
+ /* RANGESETF flags. */
+ unsigned int flags;
+};
+
+/* An inclusive range [s,e] and pointer to next range in ascending order. */
+struct range {
+ struct list_head list;
+ unsigned long s, e;
+};
+
+/* Return the lowest range in the set r, or NULL if r is empty. */
+struct range *first_range(struct rangeset *r);
+
+/* Return range following x in ascending order, or NULL if x is the highest. */
+struct range *next_range(struct rangeset *r, struct range *x);
/*
* Initialise/destroy per-domain rangeset information.
--
2.34.1
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 07/15] xen/overlay: Enable device tree overlay assignment to running domains
2024-04-24 3:34 [PATCH 00/15] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
` (5 preceding siblings ...)
2024-04-24 3:34 ` [PATCH 06/15] rangeset: Move struct range and struct rangeset to headerfile Henry Wang
@ 2024-04-24 3:34 ` Henry Wang
2024-04-24 6:05 ` Jan Beulich
2024-04-24 3:34 ` [PATCH 08/15] tools: Add domain_id and expert mode for overlay operations Henry Wang
` (8 subsequent siblings)
15 siblings, 1 reply; 58+ messages in thread
From: Henry Wang @ 2024-04-24 3:34 UTC (permalink / raw)
To: xen-devel
Cc: Vikram Garhwal, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, Jan Beulich, Vikram Garhwal, Henry Wang
From: Vikram Garhwal <fnu.vikram@xilinx.com>
Following changes are done to enable dtbo assignment to a running vm with given
domain_id:
1. Support for irq map and unmap for running domain. We store the IRQ nums for
each overlay and while removing those IRQ nums are removed.
2. Support iommu assign/reassign to running domains.
3. Added "map_nodes" options to support two modes:
a. Add the nodes in Xen device tree and map the nodes to given VM
b. Just add them in xen device tree without mapping.
Change device.c: handle_device() function with input domain_mapping flag.
Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
Signed-off-by: Michal Orzel <michal.orzel@amd.com>
Signed-off-by: Henry Wang <xin.wang2@amd.com>
---
xen/arch/arm/device.c | 8 +-
xen/arch/arm/domain_build.c | 2 +-
xen/arch/arm/include/asm/setup.h | 3 +-
xen/common/dt-overlay.c | 218 ++++++++++++++++++++++++++-----
xen/include/public/sysctl.h | 4 +-
5 files changed, 199 insertions(+), 36 deletions(-)
diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
index 3e02cff008..df5035f9a8 100644
--- a/xen/arch/arm/device.c
+++ b/xen/arch/arm/device.c
@@ -317,7 +317,8 @@ static int map_device_children(const struct dt_device_node *dev,
* - Map the IRQs and iomem regions to DOM0
*/
int handle_device(struct domain *d, struct dt_device_node *dev, p2m_type_t p2mt,
- struct rangeset *iomem_ranges, struct rangeset *irq_ranges)
+ struct rangeset *iomem_ranges, struct rangeset *irq_ranges,
+ bool device_mapping)
{
unsigned int naddr;
unsigned int i;
@@ -334,9 +335,10 @@ int handle_device(struct domain *d, struct dt_device_node *dev, p2m_type_t p2mt,
struct map_range_data mr_data = {
.d = d,
.p2mt = p2mt,
- .skip_mapping = !own_device ||
+ .skip_mapping = (!own_device ||
(is_pci_passthrough_enabled() &&
- (device_get_class(dev) == DEVICE_PCI_HOSTBRIDGE)),
+ (device_get_class(dev) == DEVICE_PCI_HOSTBRIDGE))) &&
+ !device_mapping,
.iomem_ranges = iomem_ranges,
.irq_ranges = irq_ranges
};
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 54232ed4cb..a525aed175 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1699,7 +1699,7 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
"WARNING: Path %s is reserved, skip the node as we may re-use the path.\n",
path);
- res = handle_device(d, node, p2mt, NULL, NULL);
+ res = handle_device(d, node, p2mt, NULL, NULL, false);
if ( res)
return res;
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index d15a88d2e0..d6d8c28d02 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -172,7 +172,8 @@ u32 device_tree_get_u32(const void *fdt, int node,
const char *prop_name, u32 dflt);
int handle_device(struct domain *d, struct dt_device_node *dev, p2m_type_t p2mt,
- struct rangeset *iomem_ranges, struct rangeset *irq_ranges);
+ struct rangeset *iomem_ranges, struct rangeset *irq_ranges,
+ bool device_mapping);
int map_device_irqs_to_domain(struct domain *d, struct dt_device_node *dev,
bool need_mapping, struct rangeset *irq_ranges);
diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c
index 39e4ba59dd..8840ea756b 100644
--- a/xen/common/dt-overlay.c
+++ b/xen/common/dt-overlay.c
@@ -356,24 +356,133 @@ static int overlay_get_nodes_info(const void *fdto, char **nodes_full_path)
return 0;
}
+static int remove_all_irqs(struct rangeset *irq_ranges,
+ struct domain *d, bool domain_mapping)
+{
+ struct range *x;
+ int rc = 0;
+
+ read_lock(&irq_ranges->lock);
+
+ for ( x = first_range(irq_ranges); x != NULL;
+ x = next_range(irq_ranges, x) )
+ {
+ /*
+ * Handle invalid use cases 1:
+ * Where user assigned the nodes to dom0 along with their irq
+ * mappings but now just wants to remove the nodes entry from Xen device
+ * device tree without unmapping the irq.
+ */
+ if ( !domain_mapping && vgic_get_hw_irq_desc(d, NULL, x->s) )
+ {
+ printk(XENLOG_ERR "Removing node from device tree without releasing it's IRQ/IOMMU is not allowed\n");
+ rc = -EINVAL;
+ goto out;
+ }
+
+ /*
+ * 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, x->s))
+ continue;
+ /*
+ * 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, x->s);
+ if ( rc )
+ {
+ printk(XENLOG_ERR "unable to revoke access for irq %ld\n", x->s);
+ goto out;
+ }
+
+ if ( domain_mapping )
+ {
+ rc = release_guest_irq(d, x->s);
+ if ( rc )
+ {
+ printk(XENLOG_ERR "unable to release irq %ld\n", x->s);
+ goto out;
+ }
+ }
+ }
+
+out:
+ read_unlock(&irq_ranges->lock);
+ return rc;
+}
+
+static int remove_all_iomems(struct rangeset *iomem_ranges,
+ struct domain *d,
+ bool domain_mapping)
+{
+ struct range *x;
+ int rc = 0;
+ p2m_type_t t;
+ mfn_t mfn;
+
+ read_lock(&iomem_ranges->lock);
+
+ for ( x = first_range(iomem_ranges); x != NULL;
+ x = next_range(iomem_ranges, x) )
+ {
+ mfn = p2m_lookup(d, _gfn(x->s), &t);
+ if ( mfn_x(mfn) == 0 || mfn_x(mfn) == ~0UL )
+ {
+ rc = -EINVAL;
+ goto out;
+ }
+
+ rc = iomem_deny_access(d, x->s, x->e);
+ if ( rc )
+ {
+ printk(XENLOG_ERR "Unable to remove dom%d access to %#lx - %#lx\n",
+ d->domain_id,
+ x->s, x->e);
+ goto out;
+ }
+
+ rc = unmap_mmio_regions(d, _gfn(x->s), x->e - x->s,
+ _mfn(x->s));
+ if ( rc )
+ goto out;
+ }
+
+out:
+ read_unlock(&iomem_ranges->lock);
+ return rc;
+}
+
/* 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, bool domain_mapping)
{
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);
+ /*
+ * We also check if device is assigned to DOMID_IO as when a domain
+ * is destroyed device is assigned to DOMID_IO or for the case when
+ * device was never mapped to a running domain.
+ */
+ if ( domid != hardware_domain->domain_id && 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;
+ }
- /* 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;
+ /*
+ * Device is assigned to hardware domain.
+ */
+ d = hardware_domain;
}
/* Check if iommu property exists. */
@@ -382,6 +491,16 @@ static int remove_node_resources(struct dt_device_node *device_node)
if ( dt_device_is_protected(device_node) )
{
write_lock(&dt_host_lock);
+ if ( domain_mapping && !list_empty(&device_node->domain_list) )
+ {
+ rc = iommu_deassign_dt_device(d, device_node);
+ if ( rc < 0 )
+ {
+ write_unlock(&dt_host_lock);
+ return rc;
+ }
+ }
+
rc = iommu_remove_dt_device(device_node);
if ( rc < 0 )
{
@@ -397,7 +516,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, bool domain_mapping)
{
int rc = 0;
struct dt_device_node *child_node;
@@ -407,12 +527,13 @@ 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,
+ domain_mapping);
if ( rc )
return rc;
}
- rc = remove_node_resources(child_node);
+ rc = remove_node_resources(child_node, d, domain_mapping);
if ( rc )
return rc;
}
@@ -421,12 +542,13 @@ remove_descendant_nodes_resources(const struct dt_device_node *device_node)
}
/* Remove nodes from dt_host. */
-static int remove_nodes(const struct overlay_track *tracker)
+static int remove_nodes(const struct overlay_track *tracker,
+ struct domain *d)
{
int rc = 0;
struct dt_device_node *overlay_node;
unsigned int j;
- struct domain *d = hardware_domain;
+ bool domain_mapping = (d != NULL);
for ( j = 0; j < tracker->num_nodes; j++ )
{
@@ -434,11 +556,19 @@ static int remove_nodes(const struct overlay_track *tracker)
if ( overlay_node == NULL )
return -EINVAL;
- rc = remove_descendant_nodes_resources(overlay_node);
+ rc = remove_descendant_nodes_resources(overlay_node, d, domain_mapping);
+ if ( rc )
+ return rc;
+
+ rc = remove_node_resources(overlay_node, d, domain_mapping);
+ if ( rc )
+ return rc;
+
+ rc = remove_all_irqs(tracker->irq_ranges, d, domain_mapping);
if ( rc )
return rc;
- rc = remove_node_resources(overlay_node);
+ rc = remove_all_iomems(tracker->iomem_ranges, d, domain_mapping);
if ( rc )
return rc;
@@ -481,7 +611,8 @@ static int remove_nodes(const struct overlay_track *tracker)
* while destroying the domain.
*/
static long handle_remove_overlay_nodes(const void *overlay_fdt,
- uint32_t overlay_fdt_size)
+ uint32_t overlay_fdt_size,
+ struct domain *d)
{
int rc;
struct overlay_track *entry, *temp, *track;
@@ -520,7 +651,7 @@ static long handle_remove_overlay_nodes(const void *overlay_fdt,
}
- rc = remove_nodes(entry);
+ rc = remove_nodes(entry, d);
if ( rc )
{
printk(XENLOG_ERR "Removing node failed\n");
@@ -560,12 +691,21 @@ static void free_nodes_full_path(unsigned int num_nodes, char **nodes_full_path)
xfree(nodes_full_path);
}
-static long add_nodes(struct overlay_track *tr, char **nodes_full_path)
+static long add_nodes(struct overlay_track *tr, char **nodes_full_path,
+ struct domain *d)
{
int rc;
unsigned int j;
struct dt_device_node *overlay_node;
+ bool domain_mapping = (d != NULL);
+
+ /*
+ * If domain is NULL, then we add the devices into hardware domain and skip
+ * IRQs/IOMMUs mapping.
+ */
+ if ( d == NULL )
+ d = hardware_domain;
for ( j = 0; j < tr->num_nodes; j++ )
{
@@ -609,8 +749,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);
@@ -618,18 +756,23 @@ static long add_nodes(struct overlay_track *tr, char **nodes_full_path)
{
/* Sanity check. But code will never come here. */
ASSERT_UNREACHABLE();
+ write_unlock(&dt_host_lock);
return -EFAULT;
}
- rc = handle_device(hardware_domain, overlay_node, p2m_mmio_direct_c,
+ rc = handle_device(d, overlay_node, p2m_mmio_direct_c,
tr->iomem_ranges,
- tr->irq_ranges);
+ tr->irq_ranges,
+ domain_mapping);
if ( rc )
{
printk(XENLOG_ERR "Adding IRQ and IOMMU failed\n");
+ write_unlock(&dt_host_lock);
return rc;
}
+ write_unlock(&dt_host_lock);
+
/* Keep overlay_node address in tracker. */
tr->nodes_address[j] = (unsigned long)overlay_node;
}
@@ -643,7 +786,8 @@ static long add_nodes(struct overlay_track *tr, char **nodes_full_path)
* to hardware domain done by handle_node().
*/
static long handle_add_overlay_nodes(void *overlay_fdt,
- uint32_t overlay_fdt_size)
+ uint32_t overlay_fdt_size,
+ struct domain *d)
{
int rc;
unsigned int j;
@@ -788,7 +932,7 @@ static long handle_add_overlay_nodes(void *overlay_fdt,
goto err;
}
- rc = add_nodes(tr, nodes_full_path);
+ rc = add_nodes(tr, nodes_full_path, d);
if ( rc )
{
printk(XENLOG_ERR "Adding nodes failed. Removing the partially added nodes.\n");
@@ -810,7 +954,7 @@ static long handle_add_overlay_nodes(void *overlay_fdt,
*/
remove_node:
tr->num_nodes = j;
- rc = remove_nodes(tr);
+ rc = remove_nodes(tr, d);
if ( rc )
{
@@ -855,6 +999,7 @@ long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op)
{
long ret;
void *overlay_fdt;
+ struct domain *d = NULL;
if ( op->overlay_op != XEN_SYSCTL_DT_OVERLAY_ADD &&
op->overlay_op != XEN_SYSCTL_DT_OVERLAY_REMOVE )
@@ -863,7 +1008,7 @@ long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op)
if ( op->overlay_fdt_size == 0 || op->overlay_fdt_size > KB(500) )
return -EINVAL;
- if ( op->pad[0] || op->pad[1] || op->pad[2] )
+ if ( op->pad[0] || op->pad[1] )
return -EINVAL;
overlay_fdt = xmalloc_bytes(op->overlay_fdt_size);
@@ -880,13 +1025,26 @@ long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op)
return -EFAULT;
}
+ /*
+ * If domain_mapping == false, domain_id can be ignored as we don't need to
+ * map resource to any domain.
+ *
+ * If domain_mapping == true, domain_id, get the target domain for the
+ * mapping.
+ */
+ if ( op->domain_mapping )
+ d = rcu_lock_domain_by_id(op->domain_id);
+
if ( op->overlay_op == XEN_SYSCTL_DT_OVERLAY_REMOVE )
- ret = handle_remove_overlay_nodes(overlay_fdt, op->overlay_fdt_size);
+ ret = handle_remove_overlay_nodes(overlay_fdt, op->overlay_fdt_size, d);
else
- ret = handle_add_overlay_nodes(overlay_fdt, op->overlay_fdt_size);
+ ret = handle_add_overlay_nodes(overlay_fdt, op->overlay_fdt_size, d);
xfree(overlay_fdt);
+ if ( op->domain_mapping )
+ rcu_unlock_domain(d);
+
return ret;
}
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 9b19679cae..f5435f8890 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -1197,7 +1197,9 @@ struct xen_sysctl_dt_overlay {
#define XEN_SYSCTL_DT_OVERLAY_ADD 1
#define XEN_SYSCTL_DT_OVERLAY_REMOVE 2
uint8_t overlay_op; /* IN: Add or remove. */
- uint8_t pad[3]; /* IN: Must be zero. */
+ bool domain_mapping; /* IN: True of False. */
+ uint8_t pad[2]; /* IN: Must be zero. */
+ uint32_t domain_id;
};
#endif
--
2.34.1
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 08/15] tools: Add domain_id and expert mode for overlay operations
2024-04-24 3:34 [PATCH 00/15] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
` (6 preceding siblings ...)
2024-04-24 3:34 ` [PATCH 07/15] xen/overlay: Enable device tree overlay assignment to running domains Henry Wang
@ 2024-04-24 3:34 ` Henry Wang
2024-05-01 14:46 ` Anthony PERARD
2024-04-24 3:34 ` [PATCH 09/15] tools/libs/light: Modify dtbo to domU linux dtbo format Henry Wang
` (7 subsequent siblings)
15 siblings, 1 reply; 58+ messages in thread
From: Henry Wang @ 2024-04-24 3:34 UTC (permalink / raw)
To: xen-devel
Cc: Vikram Garhwal, Anthony PERARD, Juergen Gross,
Stefano Stabellini, Henry Wang
From: Vikram Garhwal <fnu.vikram@xilinx.com>
Add domain_id and expert mode for overlay assignment. This enables dynamic
programming of nodes during runtime.
Take the opportunity to fix the name mismatch in the xl command, the
command name should be "dt-overlay" instead of "dt_overlay".
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>
---
tools/include/libxl.h | 8 +++++--
tools/include/xenctrl.h | 5 +++--
tools/libs/ctrl/xc_dt_overlay.c | 7 ++++--
tools/libs/light/libxl_dt_overlay.c | 17 +++++++++++----
tools/xl/xl_vmcontrol.c | 34 ++++++++++++++++++++++++++---
5 files changed, 58 insertions(+), 13 deletions(-)
diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index 62cb07dea6..59a3e1b37c 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -2549,8 +2549,12 @@ 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,
- uint32_t overlay_size, uint8_t overlay_op);
+#define LIBXL_DT_OVERLAY_ADD 1
+#define LIBXL_DT_OVERLAY_REMOVE 2
+
+int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domain_id, void *overlay,
+ uint32_t overlay_size, uint8_t overlay_op, bool auto_mode,
+ bool domain_mapping);
#endif
/*
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 2ef8b4e054..3861d0a23f 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2654,8 +2654,9 @@ int xc_domain_cacheflush(xc_interface *xch, uint32_t domid,
xen_pfn_t start_pfn, xen_pfn_t nr_pfns);
#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(xc_interface *xch, uint32_t domain_id, void *overlay_fdt,
+ uint32_t overlay_fdt_size, uint8_t overlay_op,
+ bool domain_mapping);
#endif
/* Compat shims */
diff --git a/tools/libs/ctrl/xc_dt_overlay.c b/tools/libs/ctrl/xc_dt_overlay.c
index c2224c4d15..a1dc549915 100644
--- a/tools/libs/ctrl/xc_dt_overlay.c
+++ b/tools/libs/ctrl/xc_dt_overlay.c
@@ -20,15 +20,18 @@
#include "xc_private.h"
-int xc_dt_overlay(xc_interface *xch, void *overlay_fdt,
- uint32_t overlay_fdt_size, uint8_t overlay_op)
+int xc_dt_overlay(xc_interface *xch, uint32_t domid, void *overlay_fdt,
+ uint32_t overlay_fdt_size, uint8_t overlay_op,
+ bool domain_mapping)
{
int err;
struct xen_sysctl sysctl = {
.cmd = XEN_SYSCTL_dt_overlay,
.u.dt_overlay = {
+ .domain_id = domid,
.overlay_op = overlay_op,
.overlay_fdt_size = overlay_fdt_size,
+ .domain_mapping = domain_mapping,
}
};
diff --git a/tools/libs/light/libxl_dt_overlay.c b/tools/libs/light/libxl_dt_overlay.c
index a6c709a6dc..cdb62b28cf 100644
--- a/tools/libs/light/libxl_dt_overlay.c
+++ b/tools/libs/light/libxl_dt_overlay.c
@@ -41,8 +41,9 @@ 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 domid, void *overlay_dt,
+ uint32_t overlay_dt_size, uint8_t overlay_op,
+ bool auto_mode, bool domain_mapping)
{
int rc;
int r;
@@ -57,10 +58,18 @@ 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);
+ /* Check if user entered a valid domain id. */
+ rc = libxl_domain_info(CTX, NULL, domid);
+ if (rc == ERROR_DOMAIN_NOTFOUND) {
+ LOGD(ERROR, domid, "Non-existant domain.");
+ return ERROR_FAIL;
+ }
+
+ r = xc_dt_overlay(ctx->xch, domid, overlay_dt, overlay_dt_size, overlay_op,
+ domain_mapping);
if (r) {
- LOG(ERROR, "%s: Adding/Removing overlay dtb failed.", __func__);
+ LOG(ERROR, "domain%d: Adding/Removing overlay dtb failed.", domid);
rc = ERROR_FAIL;
}
diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index 98f6bd2e76..9674383ec3 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -1270,21 +1270,48 @@ int main_dt_overlay(int argc, char **argv)
{
const char *overlay_ops = NULL;
const char *overlay_config_file = NULL;
+ uint32_t domain_id = 0;
void *overlay_dtb = NULL;
int rc;
+ bool auto_mode = true;
+ bool domain_mapping = false;
uint8_t op;
int overlay_dtb_size = 0;
const int overlay_add_op = 1;
const int overlay_remove_op = 2;
- if (argc < 2) {
- help("dt_overlay");
+ if (argc < 3) {
+ help("dt-overlay");
return EXIT_FAILURE;
}
+ if (argc > 5) {
+ fprintf(stderr, "Too many arguments\n");
+ return ERROR_FAIL;
+ }
+
overlay_ops = argv[1];
overlay_config_file = argv[2];
+ if (!strcmp(argv[argc - 1], "-e"))
+ auto_mode = false;
+
+ if (argc == 4 || !auto_mode) {
+ domain_id = find_domain(argv[argc-1]);
+ domain_mapping = true;
+ }
+
+ if (argc == 5 || !auto_mode) {
+ domain_id = find_domain(argv[argc-2]);
+ domain_mapping = true;
+ }
+
+ /* User didn't prove any overlay operation. */
+ if (overlay_ops == NULL) {
+ fprintf(stderr, "No overlay operation mode provided\n");
+ return ERROR_FAIL;
+ }
+
if (strcmp(overlay_ops, "add") == 0)
op = overlay_add_op;
else if (strcmp(overlay_ops, "remove") == 0)
@@ -1309,7 +1336,8 @@ int main_dt_overlay(int argc, char **argv)
return ERROR_FAIL;
}
- rc = libxl_dt_overlay(ctx, overlay_dtb, overlay_dtb_size, op);
+ rc = libxl_dt_overlay(ctx, domain_id, overlay_dtb, overlay_dtb_size, op,
+ auto_mode, domain_mapping);
free(overlay_dtb);
--
2.34.1
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 09/15] tools/libs/light: Modify dtbo to domU linux dtbo format
2024-04-24 3:34 [PATCH 00/15] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
` (7 preceding siblings ...)
2024-04-24 3:34 ` [PATCH 08/15] tools: Add domain_id and expert mode for overlay operations Henry Wang
@ 2024-04-24 3:34 ` Henry Wang
2024-05-01 15:09 ` Anthony PERARD
2024-04-24 3:34 ` [PATCH 10/15] tools/xl: Share overlay with domU Henry Wang
` (6 subsequent siblings)
15 siblings, 1 reply; 58+ messages in thread
From: Henry Wang @ 2024-04-24 3:34 UTC (permalink / raw)
To: xen-devel
Cc: Vikram Garhwal, Anthony PERARD, Juergen Gross,
Stefano Stabellini, Henry Wang
From: Vikram Garhwal <fnu.vikram@xilinx.com>
Add support for modifying dtbo to make it suitable for DomU Linux. This is
done for '-e expert' mode.
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>
---
tools/libs/light/libxl_dt_overlay.c | 73 +++++++++++++++++++++++++++++
1 file changed, 73 insertions(+)
diff --git a/tools/libs/light/libxl_dt_overlay.c b/tools/libs/light/libxl_dt_overlay.c
index cdb62b28cf..eaf11a0f9c 100644
--- a/tools/libs/light/libxl_dt_overlay.c
+++ b/tools/libs/light/libxl_dt_overlay.c
@@ -17,6 +17,7 @@
#include "libxl_internal.h"
#include <libfdt.h>
#include <xenctrl.h>
+#include <xen/device_tree_defs.h>
static int check_overlay_fdt(libxl__gc *gc, void *fdt, size_t size)
{
@@ -41,6 +42,69 @@ static int check_overlay_fdt(libxl__gc *gc, void *fdt, size_t size)
return 0;
}
+static int modify_overlay_for_domU(libxl__gc *gc, void *overlay_dt_domU,
+ size_t size)
+{
+ int rc = 0;
+ int virtual_interrupt_parent = GUEST_PHANDLE_GIC;
+ const struct fdt_property *fdt_prop_node = NULL;
+ int overlay;
+ int prop_len = 0;
+ int subnode = 0;
+ int fragment;
+ const char *prop_name;
+ const char *target_path = "/";
+
+ fdt_for_each_subnode(fragment, overlay_dt_domU, 0) {
+ prop_name = fdt_getprop(overlay_dt_domU, fragment, "target-path",
+ &prop_len);
+ if (prop_name == NULL) {
+ LOG(ERROR, "target-path property not found\n");
+ rc = ERROR_FAIL;
+ goto err;
+ }
+
+ /* Change target path for domU dtb. */
+ rc = fdt_setprop_string(overlay_dt_domU, fragment, "target-path",
+ target_path);
+ if (rc) {
+ LOG(ERROR, "Setting interrupt parent property failed for %s\n",
+ prop_name);
+ goto err;
+ }
+
+ overlay = fdt_subnode_offset(overlay_dt_domU, fragment, "__overlay__");
+
+ fdt_for_each_subnode(subnode, overlay_dt_domU, overlay)
+ {
+ const char *node_name = fdt_get_name(overlay_dt_domU, subnode,
+ NULL);
+
+ fdt_prop_node = fdt_getprop(overlay_dt_domU, subnode,
+ "interrupt-parent", &prop_len);
+ if (fdt_prop_node == NULL) {
+ LOG(DETAIL, "%s property not found for %s. Skip to next node\n",
+ "interrupt-parent", node_name);
+ continue;
+ }
+
+ rc = fdt_setprop_inplace_u32(overlay_dt_domU, subnode,
+ "interrupt-parent",
+ virtual_interrupt_parent);
+ if (rc) {
+ LOG(ERROR, "Setting interrupt parent property failed for %s\n",
+ "interrupt-parent");
+ goto err;
+ }
+ }
+ }
+
+return 0;
+
+err:
+ return rc;
+}
+
int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domid, void *overlay_dt,
uint32_t overlay_dt_size, uint8_t overlay_op,
bool auto_mode, bool domain_mapping)
@@ -73,6 +137,15 @@ int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domid, void *overlay_dt,
rc = ERROR_FAIL;
}
+ /*
+ * auto_mode doesn't apply to dom0 as dom0 can get the physical
+ * description of the hardware.
+ */
+ if (domid && auto_mode) {
+ if (overlay_op == LIBXL_DT_OVERLAY_ADD)
+ rc = modify_overlay_for_domU(gc, overlay_dt, overlay_dt_size);
+ }
+
out:
GC_FREE;
return rc;
--
2.34.1
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 10/15] tools/xl: Share overlay with domU
2024-04-24 3:34 [PATCH 00/15] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
` (8 preceding siblings ...)
2024-04-24 3:34 ` [PATCH 09/15] tools/libs/light: Modify dtbo to domU linux dtbo format Henry Wang
@ 2024-04-24 3:34 ` Henry Wang
2024-04-24 3:34 ` [PATCH 11/15] tools/helpers: Add get_overlay Henry Wang
` (5 subsequent siblings)
15 siblings, 0 replies; 58+ messages in thread
From: Henry Wang @ 2024-04-24 3:34 UTC (permalink / raw)
To: xen-devel
Cc: Vikram Garhwal, Anthony PERARD, Stewart Hildebrand,
Stefano Stabellini, Henry Wang
From: Vikram Garhwal <fnu.vikram@xilinx.com>
Add domid to enable assigning the node to a running VMs.
Add expert mode option to share the overlay dtbo with domU. In this mode dom0
communicates with domid domain to share the overlay dtbo. This is done via
xenstore.
Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
Signed-off-by: Henry Wang <xin.wang2@amd.com>
---
tools/xl/Makefile | 2 +-
tools/xl/xl_cmdtable.c | 2 +-
tools/xl/xl_vmcontrol.c | 343 +++++++++++++++++++++++++++++++++++++++-
3 files changed, 342 insertions(+), 5 deletions(-)
diff --git a/tools/xl/Makefile b/tools/xl/Makefile
index d742e96a5b..122dfb9346 100644
--- a/tools/xl/Makefile
+++ b/tools/xl/Makefile
@@ -32,7 +32,7 @@ $(XL_OBJS): CFLAGS += -include $(XEN_ROOT)/tools/config.h # libxl_json.h needs i
all: xl
xl: $(XL_OBJS)
- $(CC) $(LDFLAGS) -o $@ $(XL_OBJS) $(LDLIBS_libxenutil) $(LDLIBS_libxenlight) $(LDLIBS_libxentoollog) -lyajl $(APPEND_LDFLAGS)
+ $(CC) $(LDFLAGS) -o $@ $(XL_OBJS) $(LDLIBS_libxenutil) $(LDLIBS_libxenlight) $(LDLIBS_libxentoollog) $(LDLIBS_libxenstore) $(LDLIBS_libxengnttab) -lyajl $(APPEND_LDFLAGS)
.PHONY: install
install: all
diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
index 62bdb2aeaa..2531e8517b 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> domain_id -e[expert_mode]",
"-h print this help\n"
},
#endif
diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index 9674383ec3..2bf76dd389 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -16,6 +16,7 @@
#include <inttypes.h>
#include <limits.h>
#include <stdlib.h>
+#include <sys/mman.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <sys/utsname.h>
@@ -29,6 +30,8 @@
#include "xl.h"
#include "xl_utils.h"
#include "xl_parse.h"
+#include <xenstore.h>
+#include <xengnttab.h>
static int fd_lock = -1;
@@ -1266,6 +1269,335 @@ int main_create(int argc, char **argv)
}
#ifdef LIBXL_HAVE_DT_OVERLAY
+static int copy_overlay_to_domU(uint32_t domain_id, void *overlay_dt_domU,
+ uint32_t overlay_dt_size, uint32_t *grant_ref,
+ uint32_t num_pages)
+{
+ void *buffer = NULL;
+ xengnttab_handle *gnttab = xengnttab_open(NULL, 0);
+
+ if (!gnttab) {
+ fprintf(stderr,"opening gnttab failed for domain %d\n", domain_id);
+ return -1;
+ }
+
+ buffer = xengnttab_map_domain_grant_refs(gnttab, num_pages, domain_id,
+ grant_ref, PROT_READ|PROT_WRITE);
+
+ if (buffer == NULL) {
+ fprintf(stderr, "Getting the buffer failed for grant_refs\n");
+
+ return -1;
+ }
+
+ /* buffer is contiguous allocated. */
+ memcpy(buffer, overlay_dt_domU, overlay_dt_size);
+
+ xengnttab_unmap(gnttab, buffer, num_pages);
+
+ return 0;
+}
+
+static bool wait_for_status(struct xs_handle *xs, int fd, char *status_path,
+ const char *status)
+{
+ unsigned int num_strings;
+ char *buf = NULL;
+ char **vec = NULL;
+ bool ret = false;
+ unsigned int len;
+ int rc = 0;
+ fd_set set;
+
+ while (1)
+ {
+ FD_ZERO(&set);
+ FD_SET(fd, &set);
+
+ rc = select(fd + 1, &set, NULL, NULL, NULL);
+ /* Poll for data: Blocking. */
+ if (rc <= 0)
+ break;
+
+ if (FD_ISSET(fd, &set)) {
+ /*
+ * num_strings will be set to the number of elements in vec
+ * (2 - the watched path and the overlay_watch)
+ */
+ vec = xs_read_watch(xs, &num_strings);
+ if (!vec) {
+ break;
+ }
+
+ /* do a read. */
+ buf = xs_read(xs, XBT_NULL, status_path, &len);
+ if (buf) {
+ if (!strcmp(buf, status)) {
+ ret = true;
+ break;
+ }
+ }
+ }
+ }
+
+ free(vec);
+ free(buf);
+
+ return ret;
+}
+
+static bool write_overlay_size(struct xs_handle *xs, uint32_t overlay_size,
+ char *path)
+{
+ xs_transaction_t xs_trans = XBT_NULL;
+ char buf[128];
+ char ref[16];
+
+retry_transaction:
+ xs_trans = xs_transaction_start(xs);
+ if (!xs_trans)
+ return false;
+
+ snprintf(ref, sizeof(ref), "%u", overlay_size);
+ snprintf(buf, sizeof(buf), "%s/overlay-size", path);
+
+ if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+ return false;
+
+ if (!xs_transaction_end(xs, xs_trans, 0)) {
+ if (errno == EAGAIN)
+ goto retry_transaction;
+ else
+ return false;
+ }
+
+ return true;
+}
+
+static bool write_status(struct xs_handle *xs, char *status, char *status_path)
+{
+ xs_transaction_t xs_trans = XBT_NULL;
+
+retry_transaction:
+ xs_trans = xs_transaction_start(xs);
+ if (!xs_trans)
+ return false;
+
+ if (!xs_write(xs, xs_trans, status_path, status, strlen(status)))
+ return false;
+
+ if (!xs_transaction_end(xs, xs_trans, 0)) {
+ if (errno == EAGAIN)
+ goto retry_transaction;
+ else
+ return false;
+ }
+
+ return true;
+}
+
+static uint32_t *get_page_ref(struct xs_handle *xs, const char *xs_path,
+ uint32_t num_pages)
+{
+ char buf[128];
+ uint32_t *ref = NULL;
+ char *data;
+ char temp[16] = { };
+ unsigned int len;
+ int i = 0;
+ int j = 0;
+ int start_ind = 0;
+
+ snprintf(buf, sizeof(buf), "%s/page-ref", xs_path);
+
+ /* do a read. */
+ data = xs_read(xs, XBT_NULL, buf, &len);
+
+ if (data) {
+ /* Caller will free this. */
+ ref = (uint32_t *)calloc(num_pages, sizeof(uint32_t));
+
+ for (i = 0; data[i] != '\0'; i++) {
+ if (data[i] == ',') {
+ /* Next page_ref data. */
+ memset(temp, '\0', sizeof(temp));
+
+ /* Copy the data between previous ',' to current. */
+ memcpy(temp, &data[start_ind], i - start_ind);
+
+ /* Convert to int. */
+ ref[j] = atoi(temp);
+ start_ind = i + 1;
+ j++;
+ }
+
+ if (j == num_pages)
+ break;
+ }
+ }
+
+ if (j != num_pages) {
+ fprintf(stderr, "Number of page_refs are not equal to num_pages\n");
+ free(ref);
+ ref = NULL;
+ }
+
+ free(data);
+ return ref;
+}
+
+static uint32_t get_num_pages(struct xs_handle *xs, const char *xs_path)
+{
+ char buf[128];
+ char *ref;
+ unsigned int len;
+ uint32_t num_pages = 0;
+
+ snprintf(buf, sizeof(buf), "%s/num-pages", xs_path);
+
+ /* do a read. */
+ ref = xs_read(xs, XBT_NULL, buf, &len);
+
+ if (ref)
+ num_pages = atoi(ref);
+
+ free(ref);
+
+ return num_pages;
+}
+
+static int share_overlay_with_domu(void *overlay_dt_domU, int overlay_dt_size,
+ int domain_id)
+{
+ struct xs_handle *xs = NULL;
+ char *path = NULL;
+ char receiver_status_path[64] = { };
+ char sender_status_path[64] = { };
+ int fd = 0;
+ int err;
+ uint32_t num_pages= 1;
+ uint32_t *page_ref = NULL;
+
+ /* XS_watch part. */
+ /* Open a connection to the xenstore. */
+ xs = xs_open(0);
+ if (xs == NULL) {
+ fprintf(stderr, "Deamon open for domain%d failed\n", domain_id);
+ err = ERROR_FAIL;
+ goto out;
+ }
+
+ /* Get the local domain path */
+ path = xs_get_domain_path(xs, domain_id);
+ if (path == NULL) {
+ fprintf(stderr, "Getting domain%d path failed\n", domain_id);
+ err = ERROR_FAIL;
+ goto out;
+ }
+
+ /* Make space for our node on the path */
+ path = realloc(path, strlen(path) + strlen("/data/overlay") + 1);
+ if (path == NULL) {
+ fprintf(stderr, "Path allocation failed\n");
+ err = ERROR_NOMEM;
+ goto out;
+ }
+
+ strcat(path, "/data/overlay");
+ strcpy(receiver_status_path, path);
+ strcat(receiver_status_path, "/receiver-status");
+
+ /*
+ * Watch a node for changes (poll on fd to detect).
+ * When the node (or any child) changes, fd will become readable.
+ */
+ err = xs_watch(xs, receiver_status_path, "overlay_watch");
+ if (!err) {
+ fprintf(stderr, "Creating watch failed\n");
+ err = ERROR_FAIL;
+ goto out;
+ }
+
+ /*
+ * We are notified of read availability on the watch via the
+ * file descriptor.
+ */
+ fd = xs_fileno(xs);
+
+ /* Wait for "waiting" status from other domain. */
+ if (!wait_for_status(xs, fd, receiver_status_path, "waiting")) {
+ err = ERROR_NOT_READY;
+ goto out;
+ }
+
+ /* Share the dtb size with domain. */
+ if (!write_overlay_size(xs, overlay_dt_size, path)) {
+ err = ERROR_FAIL;
+ fprintf(stderr,"Writing page ref failed\n");
+ goto out;
+ }
+
+ strcpy(sender_status_path, path);
+ strcat(sender_status_path, "/sender-status");
+
+ /* Write the status "ready". */
+ if (!write_status(xs, "ready", sender_status_path)) {
+ err = ERROR_FAIL;
+ fprintf(stderr,"Writing status ready failed\n");
+ goto out;
+ }
+
+ /* Wait for "page_ref" status from other domain. */
+ if (!wait_for_status(xs, fd, receiver_status_path, "page_ref")) {
+ err = ERROR_NOT_READY;
+ goto out;
+ }
+
+ num_pages = get_num_pages(xs, path);
+ if (num_pages == 0) {
+ fprintf(stderr, "no pages allocated\n");
+ err = ERROR_FAIL;
+ goto out;
+ }
+
+ page_ref = get_page_ref(xs, path, num_pages);
+ if (page_ref == NULL) {
+ fprintf(stderr,"page ref is null.\n");
+ err = ERROR_FAIL;
+ goto out;
+ }
+
+ if (copy_overlay_to_domU(domain_id, overlay_dt_domU, overlay_dt_size,
+ page_ref, num_pages)) {
+ fprintf(stderr,"Copy overlay failed\n");
+ err = ERROR_FAIL;
+ goto out;
+ }
+
+ /* Write the status "done". */
+ if (!write_status(xs, "done", sender_status_path)) {
+ fprintf(stderr,"Writing status DONE failed\n");
+ err = ERROR_FAIL;
+ goto out;
+ }
+
+/* Cleanup */
+out:
+ if (xs) {
+ close(fd);
+ xs_unwatch(xs, path, "overlay_watch");
+ xs_close(xs);
+ }
+
+ if (path)
+ free(path);
+
+ if (page_ref)
+ free(page_ref);
+
+ return err;
+}
+
int main_dt_overlay(int argc, char **argv)
{
const char *overlay_ops = NULL;
@@ -1339,11 +1671,16 @@ int main_dt_overlay(int argc, char **argv)
rc = libxl_dt_overlay(ctx, domain_id, overlay_dtb, overlay_dtb_size, op,
auto_mode, domain_mapping);
- free(overlay_dtb);
-
- if (rc)
+ if (rc) {
+ free(overlay_dtb);
return EXIT_FAILURE;
+ }
+
+ if (domain_id && auto_mode && (op == LIBXL_DT_OVERLAY_ADD))
+ rc = share_overlay_with_domu(overlay_dtb, overlay_dtb_size,
+ domain_id);
+ free(overlay_dtb);
return rc;
}
#endif
--
2.34.1
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 11/15] tools/helpers: Add get_overlay
2024-04-24 3:34 [PATCH 00/15] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
` (9 preceding siblings ...)
2024-04-24 3:34 ` [PATCH 10/15] tools/xl: Share overlay with domU Henry Wang
@ 2024-04-24 3:34 ` Henry Wang
2024-04-24 6:08 ` Jan Beulich
2024-04-24 3:34 ` [PATCH 12/15] get_overlay: remove domU overlay Henry Wang
` (4 subsequent siblings)
15 siblings, 1 reply; 58+ messages in thread
From: Henry Wang @ 2024-04-24 3:34 UTC (permalink / raw)
To: xen-devel; +Cc: Vikram Garhwal, Anthony PERARD, Stefano Stabellini, Henry Wang
From: Vikram Garhwal <fnu.vikram@xilinx.com>
This user level application copies the overlay dtbo shared by dom0 while doing
overlay node assignment operation. It uses xenstore to communicate with dom0.
More information on the protocol is writtien in docs/misc/overlay.txt file.
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>
---
tools/helpers/Makefile | 8 +
tools/helpers/get_overlay.c | 393 ++++++++++++++++++++++++++++++++++++
2 files changed, 401 insertions(+)
create mode 100644 tools/helpers/get_overlay.c
diff --git a/tools/helpers/Makefile b/tools/helpers/Makefile
index 09590eb5b6..dfe17ef269 100644
--- a/tools/helpers/Makefile
+++ b/tools/helpers/Makefile
@@ -12,6 +12,7 @@ TARGETS += init-xenstore-domain
endif
ifeq ($(CONFIG_ARM),y)
TARGETS += init-dom0less
+TARGETS += get_overlay
endif
endif
@@ -39,6 +40,9 @@ $(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxenctrl)
$(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxenevtchn)
init-dom0less: LDLIBS += $(call xenlibs-ldlibs,ctrl evtchn toollog store light guest foreignmemory)
+SHARE_OVERLAY_OBJS = get_overlay.o
+$(SHARE_OVERLAY_OBJS): CFLAGS += $(CFLAGS_libxengnttab) $(CFLAGS_libxenstore) $(CFLAGS_libxenctrl)
+
.PHONY: all
all: $(TARGETS)
@@ -51,6 +55,10 @@ init-xenstore-domain: $(INIT_XENSTORE_DOMAIN_OBJS)
init-dom0less: $(INIT_DOM0LESS_OBJS)
$(CC) $(LDFLAGS) -o $@ $(INIT_DOM0LESS_OBJS) $(LDLIBS) $(APPEND_LDFLAGS)
+get_overlay: $(SHARE_OVERLAY_OBJS)
+ $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenvchan) $(LDLIBS_libxenstore) $(LDLIBS_libxenctrl) $(LDLIBS_libxengnttab) $(APPEND_LDFLAGS)
+
+
.PHONY: install
install: all
$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
diff --git a/tools/helpers/get_overlay.c b/tools/helpers/get_overlay.c
new file mode 100644
index 0000000000..ca3007570e
--- /dev/null
+++ b/tools/helpers/get_overlay.c
@@ -0,0 +1,393 @@
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <unistd.h>
+#include <fcntl.h>
+#include <errno.h>
+#include <limits.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <xenstore.h>
+#include <libxenvchan.h>
+#include <xengnttab.h>
+
+#define PAGE_SIZE 4096
+
+static int xs_create_overlay_node(int domain, const char *xs_base,
+ struct xs_handle *xs)
+{
+ int ret = -1;
+ struct xs_permissions perms[2];
+ char buf[64];
+ char ref[16];
+ char *domid_str = NULL;
+ int overlay_size = 0;
+
+ xs_transaction_t xs_trans = XBT_NULL;
+
+ domid_str = xs_read(xs, XBT_NULL, "domid", NULL);
+
+ if (!domid_str)
+ return ret;
+
+ /* owner domain is us */
+ perms[0].id = atoi(domid_str);
+ /* permissions for domains not listed = none. */
+ perms[0].perms = XS_PERM_NONE;
+ /* other domains i.e. domain provided by user gets r/w permissions. */
+ perms[1].id = domain;
+ perms[1].perms = XS_PERM_READ | XS_PERM_WRITE;
+
+retry_transaction:
+
+ xs_trans = xs_transaction_start(xs);
+ if (!xs_trans)
+ goto fail_xs_transaction;
+
+ /* Create overlay-size node. */
+ snprintf(ref, sizeof(ref), "%d", overlay_size);
+ snprintf(buf, sizeof(buf), "%s/overlay-size", xs_base);
+
+ if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+ goto fail_xs_transaction;
+ if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
+ goto fail_xs_transaction;
+
+ /* Create domU status node. */
+ snprintf(ref, sizeof(ref), "%s", "waiting");
+ snprintf(buf, sizeof(buf), "%s/receiver-status", xs_base);
+
+ if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+ goto fail_xs_transaction;
+ if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
+ goto fail_xs_transaction;
+
+ /* Create dom0 status node. */
+ snprintf(ref, sizeof(ref), "%s", "not_ready");
+ snprintf(buf, sizeof(buf), "%s/sender-status", xs_base);
+
+ if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+ goto fail_xs_transaction;
+ if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
+ goto fail_xs_transaction;
+
+ if (!xs_transaction_end(xs, xs_trans, 0)) {
+ if (errno == EAGAIN)
+ goto retry_transaction;
+ else
+ goto fail_xs_transaction;
+ } else
+ ret = 0;
+
+fail_xs_transaction:
+ free(domid_str);
+
+ return ret;
+}
+
+static int get_overlay_size(struct xs_handle *xs, int domain,
+ const char *xs_path)
+{
+ char buf[128];
+ char *ref;
+ unsigned int len;
+ int dt_size = 0;
+
+ snprintf(buf, sizeof(buf), "%s/overlay-size", xs_path);
+
+ ref = xs_read(xs, XBT_NULL, buf, &len);
+
+ if (!ref)
+ return dt_size;
+
+ dt_size = atoi(ref);
+
+ free(ref);
+
+ return dt_size;
+}
+
+static uint32_t get_num_pages(int dtbo_size)
+{
+ int num_pages = 1;
+
+ while (dtbo_size > PAGE_SIZE) {
+ dtbo_size = dtbo_size - PAGE_SIZE;
+ num_pages++;
+ }
+
+ return num_pages;
+}
+
+static void *create_shared_buffer(int domain, uint32_t *refs, uint32_t pages,
+ xengntshr_handle *gntshr)
+{
+ return xengntshr_share_pages(gntshr, domain, pages, refs, 1);
+}
+
+static bool wait_for_status(struct xs_handle *xs, int fd, char *status_path,
+ const char *status)
+{
+ unsigned int num_strings;
+ char *buf = NULL;
+ char **vec = NULL;
+ bool ret = false;
+ unsigned int len;
+ int rc = 0;
+ fd_set set;
+
+ while (1)
+ {
+ FD_ZERO(&set);
+ FD_SET(fd, &set);
+
+ rc = select(fd + 1, &set, NULL, NULL, NULL);
+ /* Poll for data: Blocking. */
+ if (rc <= 0)
+ break;
+
+ if (FD_ISSET(fd, &set)) {
+ /*
+ * num_strings will be set to the number of elements in vec
+ * (2 - the watched path and the overlay_watch)
+ */
+ vec = xs_read_watch(xs, &num_strings);
+ if (!vec) {
+ break;
+ }
+
+ /* do a read. */
+ buf = xs_read(xs, XBT_NULL, status_path, &len);
+ if (buf) {
+ if (!strcmp(buf, status)) {
+ ret = true;
+ break;
+ }
+ }
+ }
+ }
+
+ free(vec);
+ free(buf);
+
+ return ret;
+}
+
+static bool write_page_ref(struct xs_handle *xs, uint32_t *page_ref,
+ uint32_t num_pages, char *path)
+{
+ xs_transaction_t xs_trans = XBT_NULL;
+ char buf[128];
+ char *ref = NULL;
+ char tmp[16];
+ int i = 0;
+ bool rc = false;
+
+ /* Caller will free this. */
+ ref = (char *)calloc(num_pages * 16, sizeof(char)); /* For each number. */
+ if (ref == NULL) {
+ fprintf(stderr, "Memory calloc for ref failed\n");
+ return rc;
+ }
+
+retry_transaction:
+ xs_trans = xs_transaction_start(xs);
+ if (!xs_trans)
+ goto out;
+
+ for (i = 0; i < num_pages; i++) {
+ snprintf(tmp, sizeof(tmp), "%d,", page_ref[i]);
+ strcat(ref, tmp);
+ }
+
+ snprintf(buf, sizeof(buf), "%s/page-ref", path);
+
+ if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+ goto out;
+
+ snprintf(buf, sizeof(buf), "%s/num-pages", path);
+ snprintf(tmp, sizeof(tmp), "%u", num_pages);
+ if (!xs_write(xs, xs_trans, buf, tmp, strlen(tmp)))
+ goto out;
+
+ if (!xs_transaction_end(xs, xs_trans, 0)) {
+ if (errno == EAGAIN)
+ goto retry_transaction;
+ else
+ goto out;
+ }
+
+ rc = true;
+
+out:
+ if (ref)
+ free(ref);
+
+ return rc;
+}
+
+static bool write_status(struct xs_handle *xs, const char *status,
+ const char *status_path)
+{
+ xs_transaction_t xs_trans = XBT_NULL;
+
+retry_transaction:
+ xs_trans = xs_transaction_start(xs);
+ if (!xs_trans)
+ return false;
+
+ if (!xs_write(xs, xs_trans, status_path, status, strlen(status)))
+ return false;
+
+ if (!xs_transaction_end(xs, xs_trans, 0)) {
+ if (errno == EAGAIN)
+ goto retry_transaction;
+ else
+ return false;
+ }
+
+ return true;
+}
+
+int main(int argc, char **argv)
+{
+ void *buffer = NULL;
+ int domain ;
+ uint32_t *page_refs = NULL;
+ FILE *fptr;
+ int dtbo_size = 0;
+ const char *path = "data/overlay";
+ char receiver_status_path[64] = { };
+ char sender_status_path[64] = { };
+ struct xs_handle *xs = NULL;
+ int rc = 0;
+ int fd = 0;
+ uint32_t num_pages = 0;
+ xengntshr_handle *gntshr;
+
+ if (argc < 2) {
+ fprintf(stderr,"Please enter domain_id.\n");
+ return 0;
+ }
+
+ domain = atoi(argv[1]);
+
+ xs = xs_open(0);
+ if (xs == NULL) {
+ fprintf(stderr, "Xenstore open for domain%d failed\n", domain);
+ goto out;
+ }
+
+ rc = xs_create_overlay_node(domain, path, xs);
+ if (rc) {
+ fprintf(stderr,"Creating overlay nodes failed\n");
+ goto out;
+ }
+
+ strcpy(receiver_status_path, path);
+ strcat(receiver_status_path, "/receiver-status");
+
+ strcpy(sender_status_path, path);
+ strcat(sender_status_path, "/sender-status");
+
+ /*
+ * Watch a node for changes (poll on fd to detect).
+ * When the node changes, fd will become readable.
+ */
+ rc = xs_watch(xs, sender_status_path, "overlay_watch");
+ if (rc == 0) {
+ fprintf(stderr, "Creating watch failed\n");
+ goto out;
+ }
+
+ /* We are notified of read availability on the watch via the
+ * file descriptor.
+ */
+ fd = xs_fileno(xs);
+
+ /* Wait for ready. */
+ if (!wait_for_status(xs, fd, sender_status_path, "ready")) {
+ fprintf(stderr, "dom0 not ready.\n");
+ goto out;
+ }
+
+ dtbo_size = get_overlay_size(xs, domain, path);
+ if (dtbo_size == 0) {
+ fprintf(stderr,"Overlay data size is zero. Exiting the application\n");
+ goto out;
+ }
+
+ gntshr = xengntshr_open(NULL, 0);
+ if (!gntshr) {
+ fprintf(stderr,"Error in opening gntshr\n");
+ goto out;
+ }
+
+ num_pages = get_num_pages(dtbo_size);
+
+ page_refs =(uint32_t *)malloc(num_pages * sizeof(int));
+ if (page_refs == NULL) {
+ fprintf(stderr, "Allocating page_ref array failed\n");
+ goto out;
+ }
+
+ /* Allocate memory for data size and share with domain. */
+ buffer = create_shared_buffer(domain, page_refs, num_pages,
+ gntshr);
+ if (buffer == NULL) {
+ fprintf(stderr,"Buffer allocation failed\n");
+ goto out;
+ }
+
+ /* Created the buffer and got page_ref. Share the page_ref with domain. */
+ if (!write_page_ref(xs, page_refs, num_pages, path)) {
+ fprintf(stderr,"Writing page ref failed\n");
+ goto out;
+ }
+
+ /* Write the status "page_ref". */
+ if (!write_status(xs, "page_ref", receiver_status_path)) {
+ fprintf(stderr,"Writing status DONE failed\n");
+ goto out;
+ }
+
+ /* Wait for done. This means other domain done copying the dtb to buffer. */
+ if (!wait_for_status(xs, fd, sender_status_path, "done")) {
+ fprintf(stderr, "dom0 status not done\n");
+ goto out;
+ }
+
+ if ((fptr = fopen("overlay.dtbo","wb")) == NULL) {
+ fprintf(stderr,"Error! opening file");
+ goto out;
+ }
+
+ printf("Writing to file overlay.dtbo.\n");
+
+ fwrite(buffer, dtbo_size, 1, fptr);
+
+ printf("Done writing to file overlay.dtbo \n");
+
+out:
+ if (fptr)
+ fclose(fptr);
+
+ if (page_refs)
+ free(page_refs);
+
+ if (xs) {
+ close(fd);
+
+ xs_unwatch(xs, path, "overlay_watch");
+
+ xs_close(xs);
+ }
+
+ if (buffer)
+ xengntshr_unshare(gntshr, buffer, num_pages);
+
+ if (gntshr)
+ xengntshr_close(gntshr);
+
+ return 0;
+}
--
2.34.1
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 12/15] get_overlay: remove domU overlay
2024-04-24 3:34 [PATCH 00/15] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
` (10 preceding siblings ...)
2024-04-24 3:34 ` [PATCH 11/15] tools/helpers: Add get_overlay Henry Wang
@ 2024-04-24 3:34 ` Henry Wang
2024-04-24 3:34 ` [PATCH 13/15] xl/overlay: add remove operation to xenstore Henry Wang
` (3 subsequent siblings)
15 siblings, 0 replies; 58+ messages in thread
From: Henry Wang @ 2024-04-24 3:34 UTC (permalink / raw)
To: xen-devel
Cc: Vikram Garhwal, Anthony PERARD, Stewart Hildebrand,
Stefano Stabellini, Henry Wang
From: Vikram Garhwal <fnu.vikram@xilinx.com>
Retrieve 4 new parameters from xenstore: overlay name, type, whether it
is a partial overlay and operation. Operation can be "add" or "remove".
Add correspond to existing mode of operation. Remove introduces support
for removing an overlay from a domU.
Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
Signed-off-by: Henry Wang <xin.wang2@amd.com>
---
tools/helpers/get_overlay.c | 132 +++++++++++++++++++++++++++++++++---
1 file changed, 123 insertions(+), 9 deletions(-)
diff --git a/tools/helpers/get_overlay.c b/tools/helpers/get_overlay.c
index ca3007570e..daa697ca04 100644
--- a/tools/helpers/get_overlay.c
+++ b/tools/helpers/get_overlay.c
@@ -66,6 +66,33 @@ retry_transaction:
snprintf(ref, sizeof(ref), "%s", "not_ready");
snprintf(buf, sizeof(buf), "%s/sender-status", xs_base);
+ if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+ goto fail_xs_transaction;
+ if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
+ goto fail_xs_transaction;
+
+ /* Create overlay-name node. */
+ snprintf(ref, sizeof(ref), "%s", "overlay_node");
+ snprintf(buf, sizeof(buf), "%s/overlay-name", xs_base);
+
+ if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+ goto fail_xs_transaction;
+ if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
+ goto fail_xs_transaction;
+
+ /* Create overlay-type node. */
+ snprintf(ref, sizeof(ref), "%s", "type");
+ snprintf(buf, sizeof(buf), "%s/overlay-type", xs_base);
+
+ if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+ goto fail_xs_transaction;
+ if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
+ goto fail_xs_transaction;
+
+ /* Create overlay-partial node. */
+ snprintf(ref, sizeof(ref), "%d", 0);
+ snprintf(buf, sizeof(buf), "%s/overlay-partial", xs_base);
+
if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
goto fail_xs_transaction;
if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
@@ -174,7 +201,7 @@ static bool wait_for_status(struct xs_handle *xs, int fd, char *status_path,
}
static bool write_page_ref(struct xs_handle *xs, uint32_t *page_ref,
- uint32_t num_pages, char *path)
+ uint32_t num_pages, const char *path)
{
xs_transaction_t xs_trans = XBT_NULL;
char buf[128];
@@ -249,12 +276,69 @@ retry_transaction:
return true;
}
+static char *get_overlay_ops(struct xs_handle *xs, const char *xs_path)
+{
+ char buf[128];
+ char *ref = NULL;
+ unsigned int len;
+
+ snprintf(buf, sizeof(buf), "%s/overlay-operation", xs_path);
+
+ ref = xs_read(xs, XBT_NULL, buf, &len);
+
+ return ref;
+}
+static char *get_overlay_name(struct xs_handle *xs, const char *xs_path)
+{
+ char buf[128];
+ char *ref = NULL;
+ unsigned int len;
+
+ snprintf(buf, sizeof(buf), "%s/overlay-name", xs_path);
+
+ ref = xs_read(xs, XBT_NULL, buf, &len);
+
+ return ref;
+}
+
+static char *get_overlay_type(struct xs_handle *xs, const char *xs_path)
+{
+ char buf[128];
+ char *ref = NULL;
+ unsigned int len;
+
+ snprintf(buf, sizeof(buf), "%s/overlay-type", xs_path);
+
+ ref = xs_read(xs, XBT_NULL, buf, &len);
+
+ return ref;
+}
+
+static bool get_overlay_partial(struct xs_handle *xs, const char *xs_path)
+{
+ char buf[128];
+ char *ref = NULL;
+ unsigned int len;
+
+ snprintf(buf, sizeof(buf), "%s/overlay-partial", xs_path);
+
+ ref = xs_read(xs, XBT_NULL, buf, &len);
+
+ if (ref) {
+ bool is_partial = atoi(ref);
+ free(ref);
+ return is_partial;
+ }
+
+ return false;
+}
+
int main(int argc, char **argv)
{
void *buffer = NULL;
int domain ;
uint32_t *page_refs = NULL;
- FILE *fptr;
+ FILE *fptr = NULL;
int dtbo_size = 0;
const char *path = "data/overlay";
char receiver_status_path[64] = { };
@@ -263,7 +347,11 @@ int main(int argc, char **argv)
int rc = 0;
int fd = 0;
uint32_t num_pages = 0;
- xengntshr_handle *gntshr;
+ xengntshr_handle *gntshr = NULL;
+ char *overlay_ops = NULL;
+ char *name = NULL;
+ char *type = NULL;
+ bool is_partial = false;
if (argc < 2) {
fprintf(stderr,"Please enter domain_id.\n");
@@ -357,16 +445,33 @@ int main(int argc, char **argv)
goto out;
}
- if ((fptr = fopen("overlay.dtbo","wb")) == NULL) {
- fprintf(stderr,"Error! opening file");
+ overlay_ops = get_overlay_ops(xs, path);
+ name = get_overlay_name(xs, path);
+ type = get_overlay_type(xs, path);
+ is_partial = get_overlay_partial(xs, path);
+
+ if (overlay_ops == NULL || name == NULL || type == NULL)
goto out;
- }
- printf("Writing to file overlay.dtbo.\n");
+ printf("%s %s %s", overlay_ops, name, type);
+ if (is_partial)
+ printf(" %d", is_partial);
+
+ printf("\n");
- fwrite(buffer, dtbo_size, 1, fptr);
+ if (!strcmp(overlay_ops, "add")) {
- printf("Done writing to file overlay.dtbo \n");
+ if ((fptr = fopen("overlay.dtbo","wb")) == NULL) {
+ fprintf(stderr,"Error! opening file");
+ goto out;
+ }
+
+ printf("Writing to file overlay.dtbo.\n");
+
+ fwrite(buffer, dtbo_size, 1, fptr);
+
+ printf("Done writing to file overlay.dtbo \n");
+ }
out:
if (fptr)
@@ -375,6 +480,15 @@ out:
if (page_refs)
free(page_refs);
+ if (overlay_ops)
+ free(overlay_ops);
+
+ if (name)
+ free(name);
+
+ if (type)
+ free(type);
+
if (xs) {
close(fd);
--
2.34.1
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 13/15] xl/overlay: add remove operation to xenstore
2024-04-24 3:34 [PATCH 00/15] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
` (11 preceding siblings ...)
2024-04-24 3:34 ` [PATCH 12/15] get_overlay: remove domU overlay Henry Wang
@ 2024-04-24 3:34 ` Henry Wang
2024-04-24 3:34 ` [PATCH 14/15] add a domU script to fetch overlays and applying them to linux Henry Wang
` (2 subsequent siblings)
15 siblings, 0 replies; 58+ messages in thread
From: Henry Wang @ 2024-04-24 3:34 UTC (permalink / raw)
To: xen-devel; +Cc: Vikram Garhwal, Anthony PERARD, Stefano Stabellini, Henry Wang
From: Vikram Garhwal <fnu.vikram@xilinx.com>
Add 3 new command line parameters to the xl overlay command: overlay
name, type and partial. Pass these paramters to the domU via xenstore.
Also introduce support for "operation" in xenstore: it can be "add" or
"remove". In case of "remove", the overlay is to be removed from the
domU device tree.
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>
---
tools/xl/xl_vmcontrol.c | 184 +++++++++++++++++++++++++++++++++++++---
1 file changed, 173 insertions(+), 11 deletions(-)
diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index 2bf76dd389..ddd6e9e370 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -1466,8 +1466,123 @@ static uint32_t get_num_pages(struct xs_handle *xs, const char *xs_path)
return num_pages;
}
+static bool write_overlay_operation(struct xs_handle *xs, char *operation,
+ char *path)
+{
+ xs_transaction_t xs_trans = XBT_NULL;
+ char buf[128];
+ char ref[64];
+
+retry_transaction:
+ xs_trans = xs_transaction_start(xs);
+ if (!xs_trans)
+ return false;
+
+ snprintf(ref, sizeof(ref), "%s", operation);
+ snprintf(buf, sizeof(buf), "%s/overlay-operation", path);
+
+ if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+ return false;
+
+ if (!xs_transaction_end(xs, xs_trans, 0)) {
+ if (errno == EAGAIN)
+ goto retry_transaction;
+ else
+ return false;
+ }
+
+ return true;
+}
+
+static bool write_overlay_name(struct xs_handle *xs, char *name,
+ char *path)
+{
+ xs_transaction_t xs_trans = XBT_NULL;
+ char buf[128];
+ char ref[64];
+
+retry_transaction:
+ xs_trans = xs_transaction_start(xs);
+ if (!xs_trans)
+ return false;
+
+ snprintf(ref, sizeof(ref), "%s", name);
+ snprintf(buf, sizeof(buf), "%s/overlay-name", path);
+
+ if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+ return false;
+
+ if (!xs_transaction_end(xs, xs_trans, 0)) {
+ if (errno == EAGAIN)
+ goto retry_transaction;
+ else
+ return false;
+ }
+
+ return true;
+}
+
+static bool write_overlay_type(struct xs_handle *xs, char *type,
+ char *path)
+{
+ xs_transaction_t xs_trans = XBT_NULL;
+ char buf[128];
+ char ref[64];
+
+retry_transaction:
+ xs_trans = xs_transaction_start(xs);
+ if (!xs_trans)
+ return false;
+
+ snprintf(ref, sizeof(ref), "%s", type);
+ snprintf(buf, sizeof(buf), "%s/overlay-type", path);
+
+ if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+ return false;
+
+ if (!xs_transaction_end(xs, xs_trans, 0)) {
+ if (errno == EAGAIN)
+ goto retry_transaction;
+ else
+ return false;
+ }
+
+ return true;
+}
+
+static bool write_overlay_partial(struct xs_handle *xs, bool is_partial,
+ char *path)
+{
+ xs_transaction_t xs_trans = XBT_NULL;
+ char buf[128];
+ char ref[4];
+
+retry_transaction:
+ xs_trans = xs_transaction_start(xs);
+ if (!xs_trans)
+ return false;
+
+ snprintf(ref, sizeof(ref), "%d", is_partial);
+ snprintf(buf, sizeof(buf), "%s/overlay-partial", path);
+
+ if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+ return false;
+
+ if (!xs_transaction_end(xs, xs_trans, 0)) {
+ if (errno == EAGAIN)
+ goto retry_transaction;
+ else
+ return false;
+ }
+
+ return true;
+}
+
+
static int share_overlay_with_domu(void *overlay_dt_domU, int overlay_dt_size,
- int domain_id)
+ int domain_id, char *overlay_ops,
+ char *overlay_name,
+ char *overlay_type, bool is_overlay_partial)
{
struct xs_handle *xs = NULL;
char *path = NULL;
@@ -1574,6 +1689,34 @@ static int share_overlay_with_domu(void *overlay_dt_domU, int overlay_dt_size,
goto out;
}
+ /* write overlay ops */
+ if (!write_overlay_operation(xs, overlay_ops, path)) {
+ err = ERROR_FAIL;
+ fprintf(stderr,"Writing overlay_ops ready failed\n");
+ goto out;
+ }
+
+ /* Write the overlay-name. */
+ if (!write_overlay_name(xs, overlay_name, path)) {
+ err = ERROR_FAIL;
+ fprintf(stderr,"Writing overlay_name ready failed\n");
+ goto out;
+ }
+
+ /* Write the overlay-type. */
+ if (!write_overlay_type(xs, overlay_type, path)) {
+ err = ERROR_FAIL;
+ fprintf(stderr,"Writing overlay_type ready failed\n");
+ goto out;
+ }
+
+ /* Write the overlay-partial. */
+ if (!write_overlay_partial(xs, is_overlay_partial, path)) {
+ err = ERROR_FAIL;
+ fprintf(stderr,"Writing overlay_partial ready failed\n");
+ goto out;
+ }
+
/* Write the status "done". */
if (!write_status(xs, "done", sender_status_path)) {
fprintf(stderr,"Writing status DONE failed\n");
@@ -1611,13 +1754,16 @@ int main_dt_overlay(int argc, char **argv)
int overlay_dtb_size = 0;
const int overlay_add_op = 1;
const int overlay_remove_op = 2;
+ char *overlay_name = "overlay";
+ char *overlay_type = "normal";
+ bool is_overlay_partial = false;
if (argc < 3) {
help("dt-overlay");
return EXIT_FAILURE;
}
- if (argc > 5) {
+ if (argc > 7) {
fprintf(stderr, "Too many arguments\n");
return ERROR_FAIL;
}
@@ -1625,17 +1771,22 @@ int main_dt_overlay(int argc, char **argv)
overlay_ops = argv[1];
overlay_config_file = argv[2];
- if (!strcmp(argv[argc - 1], "-e"))
- auto_mode = false;
-
- if (argc == 4 || !auto_mode) {
+ if (argc == 4 ) {
domain_id = find_domain(argv[argc-1]);
domain_mapping = true;
- }
-
- if (argc == 5 || !auto_mode) {
- domain_id = find_domain(argv[argc-2]);
+ } else if (argc == 5 && !strcmp(argv[4], "-e")) {
+ domain_id = find_domain(argv[3]);
+ auto_mode = false;
domain_mapping = true;
+ } else if (argc == 7) {
+ domain_id = find_domain(argv[3]);
+ domain_mapping = true;
+ overlay_name = argv[4];
+ overlay_type = argv[5];
+ is_overlay_partial = atoi(argv[6]);
+ } else {
+ fprintf(stderr, "Invalid arguments\n");
+ return ERROR_FAIL;
}
/* User didn't prove any overlay operation. */
@@ -1678,7 +1829,18 @@ int main_dt_overlay(int argc, char **argv)
if (domain_id && auto_mode && (op == LIBXL_DT_OVERLAY_ADD))
rc = share_overlay_with_domu(overlay_dtb, overlay_dtb_size,
- domain_id);
+ domain_id, "add", overlay_name,
+ overlay_type, is_overlay_partial);
+
+ if (rc) {
+ free(overlay_dtb);
+ return rc;
+ }
+
+ if (domain_id && auto_mode && (op == LIBXL_DT_OVERLAY_REMOVE))
+ rc = share_overlay_with_domu(overlay_dtb, overlay_dtb_size,
+ domain_id, "remove", overlay_name,
+ overlay_type, is_overlay_partial);
free(overlay_dtb);
return rc;
--
2.34.1
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 14/15] add a domU script to fetch overlays and applying them to linux
2024-04-24 3:34 [PATCH 00/15] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
` (12 preceding siblings ...)
2024-04-24 3:34 ` [PATCH 13/15] xl/overlay: add remove operation to xenstore Henry Wang
@ 2024-04-24 3:34 ` Henry Wang
2024-04-24 6:16 ` Jan Beulich
2024-04-24 3:34 ` [PATCH 15/15] docs: add device tree overlay documentation Henry Wang
2024-04-24 6:29 ` [PATCH 00/15] Remaining patches for dynamic node programming using overlay dtbo Jan Beulich
15 siblings, 1 reply; 58+ messages in thread
From: Henry Wang @ 2024-04-24 3:34 UTC (permalink / raw)
To: xen-devel; +Cc: Vikram Garhwal, Anthony PERARD, Stefano Stabellini, Henry Wang
From: Vikram Garhwal <fnu.vikram@xilinx.com>
Introduce a shell script that runs in the background and calls
get_overlay to retrive overlays and add them (or remove them) to Linux
device tree (running as a domU).
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>
---
tools/helpers/Makefile | 2 +-
tools/helpers/get_overlay.sh | 81 ++++++++++++++++++++++++++++++++++++
2 files changed, 82 insertions(+), 1 deletion(-)
create mode 100755 tools/helpers/get_overlay.sh
diff --git a/tools/helpers/Makefile b/tools/helpers/Makefile
index dfe17ef269..2d0558aeb8 100644
--- a/tools/helpers/Makefile
+++ b/tools/helpers/Makefile
@@ -58,7 +58,6 @@ init-dom0less: $(INIT_DOM0LESS_OBJS)
get_overlay: $(SHARE_OVERLAY_OBJS)
$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenvchan) $(LDLIBS_libxenstore) $(LDLIBS_libxenctrl) $(LDLIBS_libxengnttab) $(APPEND_LDFLAGS)
-
.PHONY: install
install: all
$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
@@ -67,6 +66,7 @@ install: all
.PHONY: uninstall
uninstall:
for i in $(TARGETS); do rm -f $(DESTDIR)$(LIBEXEC_BIN)/$$i; done
+ $(RM) $(DESTDIR)$(LIBEXEC_BIN)/get_overlay.sh
.PHONY: clean
clean:
diff --git a/tools/helpers/get_overlay.sh b/tools/helpers/get_overlay.sh
new file mode 100755
index 0000000000..2e8c6ecefd
--- /dev/null
+++ b/tools/helpers/get_overlay.sh
@@ -0,0 +1,81 @@
+#!/bin/sh
+
+modprobe xen_gntalloc
+modprobe xen_gntdev
+
+while :
+do
+ overlay_node_name=""
+ type_overlay="normal"
+ is_partial_dtb=""
+
+ output=`/usr/lib/xen/bin/get_overlay 0`
+
+ if test $? -ne 0
+ then
+ echo error
+ exit 1
+ fi
+
+ if test -z "$output"
+ then
+ echo ""
+ exit 1
+ fi
+
+ # output: add overlay-name normal partial
+ operation=`echo $output | cut -d " " -f 1`
+ overlay_node_name=`echo $output | cut -d " " -f 2`
+ type_overlay=`echo $output | cut -d " " -f 3`
+ is_partial_dtb=`echo $output | cut -d " " -f 4`
+
+ if test -z "$operation" || test -z "$overlay_node_name"
+ then
+ echo "invalid ops"
+ exit 1
+ fi
+
+ if test $operation = "add"
+ then
+ echo "Overlay received"
+
+ if test "$type_overlay" = "normal"
+ then
+ final_path="/sys/kernel/config/device-tree/overlays/$overlay_node_name"
+ mkdir -p $final_path
+ cat overlay.dtbo > $final_path/dtbo
+ else
+ # fpga overlay
+ cp overlay.dtbo lib/firmware/
+ mkdir /configfs
+ mount -t configfs configfs /configfs
+ cd /configfs/device-tree/overlays/
+
+ if test "$is_partial_dtb"
+ then
+ mkdir partial
+ echo 1 > /sys/class/fpga_manager/fpga0/flags
+ echo -n "overlay.dtbo" > /configfs/device-tree/overlays/partial
+ else
+ mkdir full
+ echo -n "overlay.dtbo" > /configfs/device-tree/overlays/full
+ fi
+ fi
+ elif test $operation = "remove"
+ then
+ if test "$type_overlay" = "normal"
+ then
+ # implement remove
+ path=/sys/kernel/config/device-tree/overlays/$overlay_node_name/dtbo
+ if ! test -f $path
+ then
+ echo "error: path doesn't exist"
+ exit 1
+ fi
+ rm $path
+ fi
+ else
+ echo "operation unsupported"
+ exit 1
+ fi
+done
--
2.34.1
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 15/15] docs: add device tree overlay documentation
2024-04-24 3:34 [PATCH 00/15] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
` (13 preceding siblings ...)
2024-04-24 3:34 ` [PATCH 14/15] add a domU script to fetch overlays and applying them to linux Henry Wang
@ 2024-04-24 3:34 ` Henry Wang
2024-04-24 6:29 ` [PATCH 00/15] Remaining patches for dynamic node programming using overlay dtbo Jan Beulich
15 siblings, 0 replies; 58+ messages in thread
From: Henry Wang @ 2024-04-24 3:34 UTC (permalink / raw)
To: xen-devel
Cc: Vikram Garhwal, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel, Volodymyr Babchuk,
Stefano Stabellini, 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>
---
docs/misc/arm/overlay.txt | 172 ++++++++++++++++++++++++++++++++++++++
1 file changed, 172 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..0351f82a19
--- /dev/null
+++ b/docs/misc/arm/overlay.txt
@@ -0,0 +1,172 @@
+# Device Tree Overlays support in Xen
+
+Xen now supports dynamic device assignment to running domains i.e.
+adding/removing nodes (using .dtbo) in Xen device tree, and assigning
+them to a running domain with given $domid.
+
+Xen supports two modes of operation: normal mode and expert mode for
+assigning nodes to domU. More on this below.
+
+Dynamic node assignment works in following ways:
+
+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 the dtbo to Xen device tree and assign the
+ dbto's node resources to the user-provided $domid.
+4. For normal mode, Xen tools share the modified dtbo with domU. domU needs
+ to run get_overlay.sh to get the dtbo from dom0 and apply the
+ overlay. get_overlay.sh uses get_overlay application for data transfer
+ between dom0 and domU.
+
+
+# Overlay Sharing protocol based on Xenstore
+
+The overlay sharing protocol with domU works in the following ways:
+
+1. get_overlay creates Xenstore path data/overlay and creates the
+ following nodes under data/overlay path:
+ a. receiver-status
+ b. sender-status
+ c. overlay-size
+ d. overlay-name
+ e. overlay-type
+ f. overlay-partial
+ and write "waiting" on receiver-status and "not_ready" to sender_status.
+
+2. libxl waits for "waiting" status on receiver-status, then writes
+ "overlay-size" with dtbo size and "ready" on "sender-status".
+
+3. get_overlay waits for "sender-status" to "ready", then allocates the
+ pages, next it shares the pages with dom0 (the page-ref num) by creating
+ page-ref node under /data/overlay and finally writes "page-refs" to
+ "receiver_status".
+
+4. libxl waits for "receiver-status" to become "page-refs" and copies
+ the data to buffer allocated with page_refs. libxl also writes the
+ "overlay-name", "overlay-type", and "overlay-partial" nodes with
+ user-provided information. Lastly, libxl writes "done" to
+ "sender-status".
+
+6. get_overlay waits for "sender-status" to be "done".
+
+7. get_overlay copies the data and writes it to file.
+
+8. Finally, get_overlay unshares the pages with dom0.
+
+Note: get_overlay application needs two drivers xen_gntdev and xen_gntalloc in
+Linux. These can be loaded using modprobe xen_gntalloc and modprobe xen_gntdev.
+
+
+# Examples
+
+Here are a few examples on how to use it.
+
+## Dom0 device add
+
+For assigning a device tree overlay to dom0, enter the following:
+
+ (dom0) xl dt-overlay add overlay.dtbo 0
+
+This will allocate the devices mentioned in overlay.dtbo to Xen device
+tree and will assign these devices to dom0.
+
+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, do the following:
+
+ (dom0) xl dt-overlay remove overlay.dtbo
+
+NOTE: The user is expected to unload any Linux kernel modules which
+might be accessing the devices in overlay.dtbo. Removing devices without
+unloading the modules might result in a crash.
+
+The following is an incorrect sequence:
+
+ (dom0) xl dt-overlay add overlay.dtbo 0
+ (dom0) xl dt-overlay remove overlay.dtbo
+
+The last command only removed the nodes from the Xen dtb but it did not
+deassigning irq/iommus from dom0. This will result in unhandled
+behavior. The correct sequence is to deassign the nodes from dom0:
+
+ (dom0) xl dt-overlay remove overlay.dtbo 0
+
+
+## DomU device add/remove
+
+There are two modes supported for domU use cases: expert mode and normal
+mode.
+
+
+### Expert mode
+
+All the nodes in dtbo will be assigned to a domain; the user will need
+to prepare the dtb for the domU. User will also need to modprobe the
+relevant drivers.
+
+Example for domU device add:
+
+ (dom0) xl dt-overlay add 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_linux.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 remove overlay.dtbo $domid
+
+
+### Normal mode
+
+Libxl modifies the dtbo suitable for the domU. Currently, it does basic
+modifications like updating "target-path" and "interrupt-parent" to make
+them compatible with the domU device tree. Please note that this might
+not work for nodes which need more complex adjustments. The user needs
+to make any required changes for complex overlays and modprobe the
+required Linux modules.
+
+For normal mode, the user is also required to input below three parameters:
+
+a. overlay_node: the name
+b. overlay_type: whether the nodes are fpga nodes or normal nodes
+c. partial or full overlay type: only needed for fpga overlays
+
+Example for domU overlay add:
+
+ (dom0) xl dt-overlay add overlay.dtbo $domid overlay_node_name overlay_type is_partial
+ (dom0) xl console $domid # go to $domid console
+ (domU) ./usr/lib/xen/bin/get_overlay.sh
+
+Finally if needed, the relevant Linux kernel drive can be loaded:
+
+ (domU) modprobe module_name.ko
+
+Example for domU overlay remove:
+
+ (dom0) xl dt-overlay remove overlay.dtbo $domid overlay_node_name overlay_type is_partial
+
+The get_overlay.sh script automates the following:
+
+a. gets the modified overlay.dtbo from dom0 using Xenstore transactions
+b. applies overlay.dtbo to the domU Linux device tree depending on what
+ type of overlay it is
+c. removes the overlay nodes from device tree when the user requests it
--
2.34.1
^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH 01/15] xen/commom/dt-overlay: Fix missing lock when remove the device
2024-04-24 3:34 ` [PATCH 01/15] xen/commom/dt-overlay: Fix missing lock when remove the device Henry Wang
@ 2024-04-24 5:58 ` Jan Beulich
2024-04-24 6:02 ` Henry Wang
0 siblings, 1 reply; 58+ messages in thread
From: Jan Beulich @ 2024-04-24 5:58 UTC (permalink / raw)
To: Henry Wang; +Cc: Stefano Stabellini, Julien Grall, xen-devel
On 24.04.2024 05:34, Henry Wang wrote:
> --- a/xen/common/dt-overlay.c
> +++ b/xen/common/dt-overlay.c
> @@ -381,9 +381,14 @@ static int remove_node_resources(struct dt_device_node *device_node)
> {
> if ( dt_device_is_protected(device_node) )
> {
> + write_lock(&dt_host_lock);
> rc = iommu_remove_dt_device(device_node);
Any particular reason you add two call sites to the unlock function,
instead of putting it here?
Jan
> if ( rc < 0 )
> + {
> + write_unlock(&dt_host_lock);
> return rc;
> + }
> + write_unlock(&dt_host_lock);
> }
> }
>
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 01/15] xen/commom/dt-overlay: Fix missing lock when remove the device
2024-04-24 5:58 ` Jan Beulich
@ 2024-04-24 6:02 ` Henry Wang
0 siblings, 0 replies; 58+ messages in thread
From: Henry Wang @ 2024-04-24 6:02 UTC (permalink / raw)
To: Jan Beulich; +Cc: Stefano Stabellini, Julien Grall, xen-devel
Hi Jan,
On 4/24/2024 1:58 PM, Jan Beulich wrote:
> On 24.04.2024 05:34, Henry Wang wrote:
>> --- a/xen/common/dt-overlay.c
>> +++ b/xen/common/dt-overlay.c
>> @@ -381,9 +381,14 @@ static int remove_node_resources(struct dt_device_node *device_node)
>> {
>> if ( dt_device_is_protected(device_node) )
>> {
>> + write_lock(&dt_host_lock);
>> rc = iommu_remove_dt_device(device_node);
> Any particular reason you add two call sites to the unlock function,
> instead of putting it here?
Oh...you are correct. It is indeed better to put the unlock here. If
this is the only comment for this patch, can I respin this only patch as
a v1.1 or would one of the committers be ok to fix on commit? Sorry for
the trouble and thanks for the suggestion.
Kind regards,
Henry
> Jan
>
>> if ( rc < 0 )
>> + {
>> + write_unlock(&dt_host_lock);
>> return rc;
>> + }
>> + write_unlock(&dt_host_lock);
>> }
>> }
>>
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 07/15] xen/overlay: Enable device tree overlay assignment to running domains
2024-04-24 3:34 ` [PATCH 07/15] xen/overlay: Enable device tree overlay assignment to running domains Henry Wang
@ 2024-04-24 6:05 ` Jan Beulich
2024-04-29 3:36 ` Henry Wang
0 siblings, 1 reply; 58+ messages in thread
From: Jan Beulich @ 2024-04-24 6:05 UTC (permalink / raw)
To: Henry Wang, Vikram Garhwal
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Vikram Garhwal,
xen-devel
On 24.04.2024 05:34, Henry Wang wrote:
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -1197,7 +1197,9 @@ struct xen_sysctl_dt_overlay {
> #define XEN_SYSCTL_DT_OVERLAY_ADD 1
> #define XEN_SYSCTL_DT_OVERLAY_REMOVE 2
> uint8_t overlay_op; /* IN: Add or remove. */
> - uint8_t pad[3]; /* IN: Must be zero. */
> + bool domain_mapping; /* IN: True of False. */
> + uint8_t pad[2]; /* IN: Must be zero. */
> + uint32_t domain_id;
> };
If you merely re-purposed padding fields, all would be fine without
bumping the interface version. Yet you don't, albeit for an unclear
reason: Why uint32_t rather than domid_t? And on top of that - why a
separate boolean when you could use e.g. DOMID_INVALID to indicate
"no domain mapping"?
That said - anything taking a domain ID is certainly suspicious in a
sysctl. Judging from the description you really mean this to be a
domctl. Anything else will require extra justification.
Jan
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 11/15] tools/helpers: Add get_overlay
2024-04-24 3:34 ` [PATCH 11/15] tools/helpers: Add get_overlay Henry Wang
@ 2024-04-24 6:08 ` Jan Beulich
2024-04-25 0:43 ` Henry Wang
0 siblings, 1 reply; 58+ messages in thread
From: Jan Beulich @ 2024-04-24 6:08 UTC (permalink / raw)
To: Henry Wang, Vikram Garhwal; +Cc: Anthony PERARD, Stefano Stabellini, xen-devel
On 24.04.2024 05:34, Henry Wang wrote:
> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>
> This user level application copies the overlay dtbo shared by dom0 while doing
> overlay node assignment operation. It uses xenstore to communicate with dom0.
> More information on the protocol is writtien in docs/misc/overlay.txt file.
>
> 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>
> ---
> tools/helpers/Makefile | 8 +
> tools/helpers/get_overlay.c | 393 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 401 insertions(+)
> create mode 100644 tools/helpers/get_overlay.c
As mentioned before on various occasions - new files preferably use dashes as
separators in preference to underscores. You not doing so is particularly
puzzling seeing ...
> --- a/tools/helpers/Makefile
> +++ b/tools/helpers/Makefile
> @@ -12,6 +12,7 @@ TARGETS += init-xenstore-domain
> endif
> ifeq ($(CONFIG_ARM),y)
> TARGETS += init-dom0less
> +TARGETS += get_overlay
... patch context here (demonstrating a whopping 3 dashes used in similar
cases).
Jan
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 14/15] add a domU script to fetch overlays and applying them to linux
2024-04-24 3:34 ` [PATCH 14/15] add a domU script to fetch overlays and applying them to linux Henry Wang
@ 2024-04-24 6:16 ` Jan Beulich
2024-04-25 0:54 ` Henry Wang
0 siblings, 1 reply; 58+ messages in thread
From: Jan Beulich @ 2024-04-24 6:16 UTC (permalink / raw)
To: Henry Wang, Vikram Garhwal; +Cc: Anthony PERARD, Stefano Stabellini, xen-devel
On 24.04.2024 05:34, Henry Wang wrote:
> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>
> Introduce a shell script that runs in the background and calls
> get_overlay to retrive overlays and add them (or remove them) to Linux
> device tree (running as a domU).
>
> 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>
> ---
> tools/helpers/Makefile | 2 +-
> tools/helpers/get_overlay.sh | 81 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 82 insertions(+), 1 deletion(-)
> create mode 100755 tools/helpers/get_overlay.sh
Besides the same naming issue as in the earlier patch, the script also
looks very Linux-ish. Yet ...
> --- a/tools/helpers/Makefile
> +++ b/tools/helpers/Makefile
> @@ -58,7 +58,6 @@ init-dom0less: $(INIT_DOM0LESS_OBJS)
> get_overlay: $(SHARE_OVERLAY_OBJS)
> $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenvchan) $(LDLIBS_libxenstore) $(LDLIBS_libxenctrl) $(LDLIBS_libxengnttab) $(APPEND_LDFLAGS)
>
> -
> .PHONY: install
> install: all
> $(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
> @@ -67,6 +66,7 @@ install: all
> .PHONY: uninstall
> uninstall:
> for i in $(TARGETS); do rm -f $(DESTDIR)$(LIBEXEC_BIN)/$$i; done
> + $(RM) $(DESTDIR)$(LIBEXEC_BIN)/get_overlay.sh
>
> .PHONY: clean
> clean:
... you touching only the uninstall target, it's not even clear to me
how (and under what conditions) the script is going to make it into
$(DESTDIR)$(LIBEXEC_BIN)/. Did you mean to add to $(TARGETS), perhaps,
alongside the earlier added get-overlay binary?
Jan
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 06/15] rangeset: Move struct range and struct rangeset to headerfile
2024-04-24 3:34 ` [PATCH 06/15] rangeset: Move struct range and struct rangeset to headerfile Henry Wang
@ 2024-04-24 6:22 ` Jan Beulich
2024-04-25 0:47 ` Henry Wang
0 siblings, 1 reply; 58+ messages in thread
From: Jan Beulich @ 2024-04-24 6:22 UTC (permalink / raw)
To: Henry Wang, Vikram Garhwal
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
xen-devel
On 24.04.2024 05:34, Henry Wang wrote:
> From: Vikram Garhwal <vikram.garhwal@amd.com>
>
> Move struct range, rangeset and removed static from first_range and next_range().
NAK, for going against what we do elsewhere (limiting exposure of internals).
At least as long as the justification isn't any better than ...
> IRQs and IOMEMs for nodes are stored as rangeset in the dynamic node addition
> part. While removing the nodes we need to access every IRQ and IOMEM ranges to
> unmap IRQ and IOMEM from the domain.
... this. You're aware of rangeset_report_ranges() and rangeset_consume_ranges(),
aren't you? If neither is suitable for your purpose, can you explain what you
need in addition?
Jan
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 00/15] Remaining patches for dynamic node programming using overlay dtbo
2024-04-24 3:34 [PATCH 00/15] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
` (14 preceding siblings ...)
2024-04-24 3:34 ` [PATCH 15/15] docs: add device tree overlay documentation Henry Wang
@ 2024-04-24 6:29 ` Jan Beulich
15 siblings, 0 replies; 58+ messages in thread
From: Jan Beulich @ 2024-04-24 6:29 UTC (permalink / raw)
To: Henry Wang
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Anthony PERARD, Juergen Gross, Andrew Cooper,
George Dunlap, xen-devel
On 24.04.2024 05:34, Henry Wang wrote:
> 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 to a running domain and will call unmap_mmio_regions()
> to remove the mapping.
>
> Also, documentation of the "dynamic node programming using overlay dtbo"
> feature is added.
>
> Patch 1 is a fix of [1] which is noticed during my local test, details
> please see the commit message.
>
> Gitlab CI for this series can be found in [2].
>
> [1] https://lore.kernel.org/xen-devel/20230906011631.30310-1-vikram.garhwal@amd.com/
> [2] https://gitlab.com/xen-project/people/henryw/xen/-/pipelines/1265297506
>
> Henry Wang (1):
> xen/commom/dt-overlay: Fix missing lock when remove the device
>
> Vikram Garhwal (14):
> xen/arm/gic: Enable interrupt assignment to running VM
> xen/arm: Always enable IOMMU
> tools/libs/light: Always enable IOMMU
> tools/libs/light: Increase nr_spi to 160
> rangeset: Move struct range and struct rangeset to headerfile
> xen/overlay: Enable device tree overlay assignment to running domains
> tools: Add domain_id and expert mode for overlay operations
> tools/libs/light: Modify dtbo to domU linux dtbo format
> tools/xl: Share overlay with domU
> tools/helpers: Add get_overlay
> get_overlay: remove domU overlay
> xl/overlay: add remove operation to xenstore
> add a domU script to fetch overlays and applying them to linux
> docs: add device tree overlay documentation
For all the replies I sent, Vikram's addresses - Xilinx or AMD - bounced.
What's the value of Cc-ing dead addresses?
Jan
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM
2024-04-24 3:34 ` [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM Henry Wang
@ 2024-04-24 12:58 ` Julien Grall
2024-04-25 7:06 ` Henry Wang
0 siblings, 1 reply; 58+ messages in thread
From: Julien Grall @ 2024-04-24 12:58 UTC (permalink / raw)
To: Henry Wang, xen-devel
Cc: Vikram Garhwal, Stefano Stabellini, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk, Stefano Stabellini
Hi Henry,
On 24/04/2024 04:34, Henry Wang wrote:
> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>
> Enable interrupt assign/remove for running VMs in CONFIG_OVERLAY_DTB.
>
> Currently, irq_route and mapping is only allowed at the domain creation. Adding
> exception for CONFIG_OVERLAY_DTB.
AFAICT, this is mostly reverting b8577547236f ("xen/arm: Restrict when a
physical IRQ can be routed/removed from/to a domain").
>
> 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>
> ---
> xen/arch/arm/gic.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 44c40e86de..a775f886ed 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -140,8 +140,10 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
> * back to the physical IRQ. To prevent get unsync, restrict the
> * routing to when the Domain is been created.
> */
The above comment explains why the check was added. But the commit
message doesn't explain why this can be disregarded for your use-case.
Looking at the history, I don't think you can simply remove the checks.
Regardless that...
> +#ifndef CONFIG_OVERLAY_DTB
... I am against such #ifdef. A distros may want to have OVERLAY_DTB
enabled, yet the user will not use it.
Instead, you want to remove the check once the code can properly handle
routing an IRQ the domain is created or ...
> if ( d->creation_finished )
> return -EBUSY;
> +#endif
>
> ret = vgic_connect_hw_irq(d, NULL, virq, desc, true);
> if ( ret )
> @@ -171,8 +173,10 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
> * Removing an interrupt while the domain is running may have
> * undesirable effect on the vGIC emulation.
> */
> +#ifndef CONFIG_OVERLAY_DTB
> if ( !d->is_dying )
> return -EBUSY;
> +#endif
... removed before they domain is destroyed.
>
> desc->handler->shutdown(desc);
>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 03/15] xen/arm: Always enable IOMMU
2024-04-24 3:34 ` [PATCH 03/15] xen/arm: Always enable IOMMU Henry Wang
@ 2024-04-24 13:03 ` Julien Grall
2024-04-25 1:02 ` Henry Wang
0 siblings, 1 reply; 58+ messages in thread
From: Julien Grall @ 2024-04-24 13:03 UTC (permalink / raw)
To: Henry Wang, xen-devel
Cc: Vikram Garhwal, Stefano Stabellini, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk, Stefano Stabellini
Hi Henry,
On 24/04/2024 04:34, Henry Wang wrote:
> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>
> For overlay with iommu functionality to work with running VMs, we need to enable
> IOMMU by default for the domains.
>
> 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>
> ---
> xen/arch/arm/dom0less-build.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index fb63ec6fd1..2d1fd1e214 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -894,7 +894,8 @@ 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") &&
> + if ( (IS_ENABLED(CONFIG_OVERLAY_DTB) ||
Similar to the first patch, building Xen with the DTB overlay doesn't
mean the user will want to use it (think of distros that may want to
provide a generic Xen).
Instead, we should introduce a new DT property "passthrough" that would
indicate whether the IOMMU should be used.
To be futureproof, I would match the values used by xl.cfg (see
docs/man/xl.cfg.5.pod.in).
> + dt_find_compatible_node(node, NULL, "multiboot,device-tree")) &&
> iommu_enabled )
> d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 11/15] tools/helpers: Add get_overlay
2024-04-24 6:08 ` Jan Beulich
@ 2024-04-25 0:43 ` Henry Wang
2024-04-26 1:45 ` Stewart Hildebrand
0 siblings, 1 reply; 58+ messages in thread
From: Henry Wang @ 2024-04-25 0:43 UTC (permalink / raw)
To: Jan Beulich, Vikram Garhwal; +Cc: Anthony PERARD, Stefano Stabellini, xen-devel
Hi Jan,
On 4/24/2024 2:08 PM, Jan Beulich wrote:
> On 24.04.2024 05:34, Henry Wang wrote:
>> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>>
>> This user level application copies the overlay dtbo shared by dom0 while doing
>> overlay node assignment operation. It uses xenstore to communicate with dom0.
>> More information on the protocol is writtien in docs/misc/overlay.txt file.
>>
>> 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>
>> ---
>> tools/helpers/Makefile | 8 +
>> tools/helpers/get_overlay.c | 393 ++++++++++++++++++++++++++++++++++++
>> 2 files changed, 401 insertions(+)
>> create mode 100644 tools/helpers/get_overlay.c
> As mentioned before on various occasions - new files preferably use dashes as
> separators in preference to underscores. You not doing so is particularly
> puzzling seeing ...
>
>> --- a/tools/helpers/Makefile
>> +++ b/tools/helpers/Makefile
>> @@ -12,6 +12,7 @@ TARGETS += init-xenstore-domain
>> endif
>> ifeq ($(CONFIG_ARM),y)
>> TARGETS += init-dom0less
>> +TARGETS += get_overlay
> ... patch context here (demonstrating a whopping 3 dashes used in similar
> cases).
I am not very sure why Vikram used "_" in the original patch. However I
agree you are correct. Since I am currently doing the follow up of this
series, I will use "-" in v2 as suggested. Thanks.
Kind regards,
Henry
>
> Jan
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 06/15] rangeset: Move struct range and struct rangeset to headerfile
2024-04-24 6:22 ` Jan Beulich
@ 2024-04-25 0:47 ` Henry Wang
0 siblings, 0 replies; 58+ messages in thread
From: Henry Wang @ 2024-04-25 0:47 UTC (permalink / raw)
To: Jan Beulich, Vikram Garhwal
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
xen-devel
Hi Jan,
On 4/24/2024 2:22 PM, Jan Beulich wrote:
> On 24.04.2024 05:34, Henry Wang wrote:
>> From: Vikram Garhwal <vikram.garhwal@amd.com>
>>
>> Move struct range, rangeset and removed static from first_range and next_range().
> NAK, for going against what we do elsewhere (limiting exposure of internals).
> At least as long as the justification isn't any better than ...
>
>> IRQs and IOMEMs for nodes are stored as rangeset in the dynamic node addition
>> part. While removing the nodes we need to access every IRQ and IOMEM ranges to
>> unmap IRQ and IOMEM from the domain.
> ... this. You're aware of rangeset_report_ranges() and rangeset_consume_ranges(),
> aren't you? If neither is suitable for your purpose, can you explain what you
> need in addition?
I understand your concern. I will check if I can refactor this patch
using the suggested helpers. Thanks!
Kind regards,
Henry
>
> Jan
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 14/15] add a domU script to fetch overlays and applying them to linux
2024-04-24 6:16 ` Jan Beulich
@ 2024-04-25 0:54 ` Henry Wang
2024-04-25 6:46 ` Jan Beulich
0 siblings, 1 reply; 58+ messages in thread
From: Henry Wang @ 2024-04-25 0:54 UTC (permalink / raw)
To: Jan Beulich; +Cc: Anthony PERARD, Stefano Stabellini, xen-devel
Hi Jan,
On 4/24/2024 2:16 PM, Jan Beulich wrote:
> On 24.04.2024 05:34, Henry Wang wrote:
>> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>>
>> Introduce a shell script that runs in the background and calls
>> get_overlay to retrive overlays and add them (or remove them) to Linux
>> device tree (running as a domU).
>>
>> 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>
>> ---
>> tools/helpers/Makefile | 2 +-
>> tools/helpers/get_overlay.sh | 81 ++++++++++++++++++++++++++++++++++++
>> 2 files changed, 82 insertions(+), 1 deletion(-)
>> create mode 100755 tools/helpers/get_overlay.sh
> Besides the same naming issue as in the earlier patch, the script also
> looks very Linux-ish. Yet ...
I will fix the naming issue in v2. Would you mind elaborating a bit more
about the "Linux-ish" concern? I guess this is because the original use
case is on Linux, should I do anything about this?
>> --- a/tools/helpers/Makefile
>> +++ b/tools/helpers/Makefile
>> @@ -58,7 +58,6 @@ init-dom0less: $(INIT_DOM0LESS_OBJS)
>> get_overlay: $(SHARE_OVERLAY_OBJS)
>> $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenvchan) $(LDLIBS_libxenstore) $(LDLIBS_libxenctrl) $(LDLIBS_libxengnttab) $(APPEND_LDFLAGS)
>>
>> -
>> .PHONY: install
>> install: all
>> $(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
>> @@ -67,6 +66,7 @@ install: all
>> .PHONY: uninstall
>> uninstall:
>> for i in $(TARGETS); do rm -f $(DESTDIR)$(LIBEXEC_BIN)/$$i; done
>> + $(RM) $(DESTDIR)$(LIBEXEC_BIN)/get_overlay.sh
>>
>> .PHONY: clean
>> clean:
> ... you touching only the uninstall target, it's not even clear to me
> how (and under what conditions) the script is going to make it into
> $(DESTDIR)$(LIBEXEC_BIN)/. Did you mean to add to $(TARGETS), perhaps,
> alongside the earlier added get-overlay binary?
You are right, I think the get-overlay binary and this script should be
installed if DTB overlay is supported. Checking the code, I found
LIBXL_HAVE_DT_OVERLAY which can indicate if we have this feature
supported in libxl. Do you think it is a good idea to use it to install
these two files in Makefile? Thanks.
Kind regards,
Henry
>
> Jan
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 03/15] xen/arm: Always enable IOMMU
2024-04-24 13:03 ` Julien Grall
@ 2024-04-25 1:02 ` Henry Wang
0 siblings, 0 replies; 58+ messages in thread
From: Henry Wang @ 2024-04-25 1:02 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Stefano Stabellini
Hi Julien,
On 4/24/2024 9:03 PM, Julien Grall wrote:
> Hi Henry,
>
> On 24/04/2024 04:34, Henry Wang wrote:
>> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>>
>> For overlay with iommu functionality to work with running VMs, we
>> need to enable
>> IOMMU by default for the domains.
>>
>> 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>
>> ---
>> xen/arch/arm/dom0less-build.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/dom0less-build.c
>> b/xen/arch/arm/dom0less-build.c
>> index fb63ec6fd1..2d1fd1e214 100644
>> --- a/xen/arch/arm/dom0less-build.c
>> +++ b/xen/arch/arm/dom0less-build.c
>> @@ -894,7 +894,8 @@ 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") &&
>> + if ( (IS_ENABLED(CONFIG_OVERLAY_DTB) ||
>
> Similar to the first patch, building Xen with the DTB overlay doesn't
> mean the user will want to use it (think of distros that may want to
> provide a generic Xen).
>
> Instead, we should introduce a new DT property "passthrough" that
> would indicate whether the IOMMU should be used.
>
> To be futureproof, I would match the values used by xl.cfg (see
> docs/man/xl.cfg.5.pod.in).
That sounds good. I can introduce a new DT property as suggested. Thanks
for the suggestion!
Kind regards,
Henry
>
>> + dt_find_compatible_node(node, NULL, "multiboot,device-tree")) &&
>> iommu_enabled )
>> d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>
> Cheers,
>
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 14/15] add a domU script to fetch overlays and applying them to linux
2024-04-25 0:54 ` Henry Wang
@ 2024-04-25 6:46 ` Jan Beulich
2024-04-25 7:06 ` Henry Wang
0 siblings, 1 reply; 58+ messages in thread
From: Jan Beulich @ 2024-04-25 6:46 UTC (permalink / raw)
To: Henry Wang; +Cc: Anthony PERARD, Stefano Stabellini, xen-devel
On 25.04.2024 02:54, Henry Wang wrote:
> On 4/24/2024 2:16 PM, Jan Beulich wrote:
>> On 24.04.2024 05:34, Henry Wang wrote:
>>> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>>>
>>> Introduce a shell script that runs in the background and calls
>>> get_overlay to retrive overlays and add them (or remove them) to Linux
>>> device tree (running as a domU).
>>>
>>> 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>
>>> ---
>>> tools/helpers/Makefile | 2 +-
>>> tools/helpers/get_overlay.sh | 81 ++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 82 insertions(+), 1 deletion(-)
>>> create mode 100755 tools/helpers/get_overlay.sh
>> Besides the same naming issue as in the earlier patch, the script also
>> looks very Linux-ish. Yet ...
>
> I will fix the naming issue in v2. Would you mind elaborating a bit more
> about the "Linux-ish" concern? I guess this is because the original use
> case is on Linux, should I do anything about this?
Well, the script won't work on other than Linux, will it? Therefore ...
>>> --- a/tools/helpers/Makefile
>>> +++ b/tools/helpers/Makefile
>>> @@ -58,7 +58,6 @@ init-dom0less: $(INIT_DOM0LESS_OBJS)
>>> get_overlay: $(SHARE_OVERLAY_OBJS)
>>> $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenvchan) $(LDLIBS_libxenstore) $(LDLIBS_libxenctrl) $(LDLIBS_libxengnttab) $(APPEND_LDFLAGS)
>>>
>>> -
>>> .PHONY: install
>>> install: all
>>> $(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
>>> @@ -67,6 +66,7 @@ install: all
>>> .PHONY: uninstall
>>> uninstall:
>>> for i in $(TARGETS); do rm -f $(DESTDIR)$(LIBEXEC_BIN)/$$i; done
>>> + $(RM) $(DESTDIR)$(LIBEXEC_BIN)/get_overlay.sh
>>>
>>> .PHONY: clean
>>> clean:
>> ... you touching only the uninstall target, it's not even clear to me
>> how (and under what conditions) the script is going to make it into
>> $(DESTDIR)$(LIBEXEC_BIN)/. Did you mean to add to $(TARGETS), perhaps,
>> alongside the earlier added get-overlay binary?
... it first of needs to become clear under what conditions it is actually
going to be installed.
> You are right, I think the get-overlay binary and this script should be
> installed if DTB overlay is supported. Checking the code, I found
> LIBXL_HAVE_DT_OVERLAY which can indicate if we have this feature
> supported in libxl. Do you think it is a good idea to use it to install
> these two files in Makefile? Thanks.
Counter question: If it's not going to be installed, how are people going
to make use of it? If the script is intended for manual use only, I think
that would want saying in the description. Yet then I couldn't see why
the uninstall goal would need modifying.
As to LIBXL_HAVE_DT_OVERLAY - that's not accessible from a Makefile, I
guess?
Jan
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 14/15] add a domU script to fetch overlays and applying them to linux
2024-04-25 6:46 ` Jan Beulich
@ 2024-04-25 7:06 ` Henry Wang
0 siblings, 0 replies; 58+ messages in thread
From: Henry Wang @ 2024-04-25 7:06 UTC (permalink / raw)
To: Jan Beulich; +Cc: Anthony PERARD, Stefano Stabellini, xen-devel
Hi Jan,
On 4/25/2024 2:46 PM, Jan Beulich wrote:
> On 25.04.2024 02:54, Henry Wang wrote:
>> On 4/24/2024 2:16 PM, Jan Beulich wrote:
>>> On 24.04.2024 05:34, Henry Wang wrote:
>>>> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>>>>
>>>> Introduce a shell script that runs in the background and calls
>>>> get_overlay to retrive overlays and add them (or remove them) to Linux
>>>> device tree (running as a domU).
>>>>
>>>> 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>
>>>> ---
>>>> tools/helpers/Makefile | 2 +-
>>>> tools/helpers/get_overlay.sh | 81 ++++++++++++++++++++++++++++++++++++
>>>> 2 files changed, 82 insertions(+), 1 deletion(-)
>>>> create mode 100755 tools/helpers/get_overlay.sh
>>> Besides the same naming issue as in the earlier patch, the script also
>>> looks very Linux-ish. Yet ...
>> I will fix the naming issue in v2. Would you mind elaborating a bit more
>> about the "Linux-ish" concern? I guess this is because the original use
>> case is on Linux, should I do anything about this?
> Well, the script won't work on other than Linux, will it? Therefore ...
>
>>>> --- a/tools/helpers/Makefile
>>>> +++ b/tools/helpers/Makefile
>>>> @@ -58,7 +58,6 @@ init-dom0less: $(INIT_DOM0LESS_OBJS)
>>>> get_overlay: $(SHARE_OVERLAY_OBJS)
>>>> $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenvchan) $(LDLIBS_libxenstore) $(LDLIBS_libxenctrl) $(LDLIBS_libxengnttab) $(APPEND_LDFLAGS)
>>>>
>>>> -
>>>> .PHONY: install
>>>> install: all
>>>> $(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
>>>> @@ -67,6 +66,7 @@ install: all
>>>> .PHONY: uninstall
>>>> uninstall:
>>>> for i in $(TARGETS); do rm -f $(DESTDIR)$(LIBEXEC_BIN)/$$i; done
>>>> + $(RM) $(DESTDIR)$(LIBEXEC_BIN)/get_overlay.sh
>>>>
>>>> .PHONY: clean
>>>> clean:
>>> ... you touching only the uninstall target, it's not even clear to me
>>> how (and under what conditions) the script is going to make it into
>>> $(DESTDIR)$(LIBEXEC_BIN)/. Did you mean to add to $(TARGETS), perhaps,
>>> alongside the earlier added get-overlay binary?
> ... it first of needs to become clear under what conditions it is actually
> going to be installed.
>
>> You are right, I think the get-overlay binary and this script should be
>> installed if DTB overlay is supported. Checking the code, I found
>> LIBXL_HAVE_DT_OVERLAY which can indicate if we have this feature
>> supported in libxl. Do you think it is a good idea to use it to install
>> these two files in Makefile? Thanks.
> Counter question: If it's not going to be installed, how are people going
> to make use of it? If the script is intended for manual use only, I think
> that would want saying in the description. Yet then I couldn't see why
> the uninstall goal would need modifying.
Checking the code again, I feel like this is a mistake actually. I think
this script should be installed together with the get-overlay
application as the script actually calls get-overlay. The uninstall goal
should remain untouched. I will fix this in v2.
> As to LIBXL_HAVE_DT_OVERLAY - that's not accessible from a Makefile, I
> guess?
Yes.
Kind regards,
Henry
>
> Jan
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM
2024-04-24 12:58 ` Julien Grall
@ 2024-04-25 7:06 ` Henry Wang
2024-04-25 14:28 ` Julien Grall
0 siblings, 1 reply; 58+ messages in thread
From: Henry Wang @ 2024-04-25 7:06 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Stefano Stabellini
Hi Julien,
On 4/24/2024 8:58 PM, Julien Grall wrote:
> Hi Henry,
>
> On 24/04/2024 04:34, Henry Wang wrote:
>> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>>
>> Enable interrupt assign/remove for running VMs in CONFIG_OVERLAY_DTB.
>>
>> Currently, irq_route and mapping is only allowed at the domain
>> creation. Adding
>> exception for CONFIG_OVERLAY_DTB.
>
> AFAICT, this is mostly reverting b8577547236f ("xen/arm: Restrict when
> a physical IRQ can be routed/removed from/to a domain").
>
>>
>> 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>
>> ---
>> xen/arch/arm/gic.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index 44c40e86de..a775f886ed 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -140,8 +140,10 @@ int gic_route_irq_to_guest(struct domain *d,
>> unsigned int virq,
>> * back to the physical IRQ. To prevent get unsync, restrict the
>> * routing to when the Domain is been created.
>> */
>
> The above comment explains why the check was added. But the commit
> message doesn't explain why this can be disregarded for your use-case.
>
> Looking at the history, I don't think you can simply remove the checks.
>
> Regardless that...
>
>> +#ifndef CONFIG_OVERLAY_DTB
>
> ... I am against such #ifdef. A distros may want to have OVERLAY_DTB
> enabled, yet the user will not use it.
>
> Instead, you want to remove the check once the code can properly
> handle routing an IRQ the domain is created or ...
>
>> if ( d->creation_finished )
>> return -EBUSY;
>> +#endif
>> ret = vgic_connect_hw_irq(d, NULL, virq, desc, true);
>> if ( ret )
>> @@ -171,8 +173,10 @@ int gic_remove_irq_from_guest(struct domain *d,
>> unsigned int virq,
>> * Removing an interrupt while the domain is running may have
>> * undesirable effect on the vGIC emulation.
>> */
>> +#ifndef CONFIG_OVERLAY_DTB
>> if ( !d->is_dying )
>> return -EBUSY;
>> +#endif
>
> ... removed before they domain is destroyed.
Thanks for your feeedback. After checking the b8577547236f commit
message I think I now understand your point. Do you have any suggestion
about how can I properly add the support to route/remove the IRQ to
running domains? Thanks.
Kind regards,
Henry
>
>> desc->handler->shutdown(desc);
>
> Cheers,
>
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM
2024-04-25 7:06 ` Henry Wang
@ 2024-04-25 14:28 ` Julien Grall
2024-04-30 3:50 ` Henry Wang
0 siblings, 1 reply; 58+ messages in thread
From: Julien Grall @ 2024-04-25 14:28 UTC (permalink / raw)
To: Henry Wang, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Stefano Stabellini
Hi,
On 25/04/2024 08:06, Henry Wang wrote:
> Hi Julien,
>
> On 4/24/2024 8:58 PM, Julien Grall wrote:
>> Hi Henry,
>>
>> On 24/04/2024 04:34, Henry Wang wrote:
>>> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>>>
>>> Enable interrupt assign/remove for running VMs in CONFIG_OVERLAY_DTB.
>>>
>>> Currently, irq_route and mapping is only allowed at the domain
>>> creation. Adding
>>> exception for CONFIG_OVERLAY_DTB.
>>
>> AFAICT, this is mostly reverting b8577547236f ("xen/arm: Restrict when
>> a physical IRQ can be routed/removed from/to a domain").
>>
>>>
>>> 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>
>>> ---
>>> xen/arch/arm/gic.c | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>> index 44c40e86de..a775f886ed 100644
>>> --- a/xen/arch/arm/gic.c
>>> +++ b/xen/arch/arm/gic.c
>>> @@ -140,8 +140,10 @@ int gic_route_irq_to_guest(struct domain *d,
>>> unsigned int virq,
>>> * back to the physical IRQ. To prevent get unsync, restrict the
>>> * routing to when the Domain is been created.
>>> */
>>
>> The above comment explains why the check was added. But the commit
>> message doesn't explain why this can be disregarded for your use-case.
>>
>> Looking at the history, I don't think you can simply remove the checks.
>>
>> Regardless that...
>>
>>> +#ifndef CONFIG_OVERLAY_DTB
>>
>> ... I am against such #ifdef. A distros may want to have OVERLAY_DTB
>> enabled, yet the user will not use it.
>>
>> Instead, you want to remove the check once the code can properly
>> handle routing an IRQ the domain is created or ...
>>
>>> if ( d->creation_finished )
>>> return -EBUSY;
>>> +#endif
>>> ret = vgic_connect_hw_irq(d, NULL, virq, desc, true);
>>> if ( ret )
>>> @@ -171,8 +173,10 @@ int gic_remove_irq_from_guest(struct domain *d,
>>> unsigned int virq,
>>> * Removing an interrupt while the domain is running may have
>>> * undesirable effect on the vGIC emulation.
>>> */
>>> +#ifndef CONFIG_OVERLAY_DTB
>>> if ( !d->is_dying )
>>> return -EBUSY;
>>> +#endif
>>
>> ... removed before they domain is destroyed.
>
> Thanks for your feeedback. After checking the b8577547236f commit
> message I think I now understand your point. Do you have any suggestion
> about how can I properly add the support to route/remove the IRQ to
> running domains? Thanks.
I haven't really look at that code in quite a while. I think we need to
make sure that the virtual and physical IRQ state matches at the time we
do the routing.
I am undecided on whether we want to simply prevent the action to happen
or try to reset the state.
There is also the question of what to do if the guest is enabling the
vIRQ before it is routed.
Overall, someone needs to spend some time reading the code and then make
a proposal (this could be just documentation if we believe it is safe to
do). Both the current vGIC and the new one may need an update.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 11/15] tools/helpers: Add get_overlay
2024-04-25 0:43 ` Henry Wang
@ 2024-04-26 1:45 ` Stewart Hildebrand
2024-04-26 1:48 ` Henry Wang
0 siblings, 1 reply; 58+ messages in thread
From: Stewart Hildebrand @ 2024-04-26 1:45 UTC (permalink / raw)
To: Henry Wang, Jan Beulich; +Cc: Anthony PERARD, Stefano Stabellini, xen-devel
On 4/24/24 20:43, Henry Wang wrote:
> Hi Jan,
>
> On 4/24/2024 2:08 PM, Jan Beulich wrote:
>> On 24.04.2024 05:34, Henry Wang wrote:
>>> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>>>
>>> This user level application copies the overlay dtbo shared by dom0 while doing
>>> overlay node assignment operation. It uses xenstore to communicate with dom0.
>>> More information on the protocol is writtien in docs/misc/overlay.txt file.
>>>
>>> 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>
>>> ---
>>> tools/helpers/Makefile | 8 +
>>> tools/helpers/get_overlay.c | 393 ++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 401 insertions(+)
>>> create mode 100644 tools/helpers/get_overlay.c
>> As mentioned before on various occasions - new files preferably use dashes as
>> separators in preference to underscores. You not doing so is particularly
>> puzzling seeing ...
>>
>>> --- a/tools/helpers/Makefile
>>> +++ b/tools/helpers/Makefile
>>> @@ -12,6 +12,7 @@ TARGETS += init-xenstore-domain
>>> endif
>>> ifeq ($(CONFIG_ARM),y)
>>> TARGETS += init-dom0less
>>> +TARGETS += get_overlay
>> ... patch context here (demonstrating a whopping 3 dashes used in similar
>> cases).
>
> I am not very sure why Vikram used "_" in the original patch. However I agree you are correct. Since I am currently doing the follow up of this series, I will use "-" in v2 as suggested. Thanks.
Please also add tools/helpers/get-overlay to .gitignore
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 11/15] tools/helpers: Add get_overlay
2024-04-26 1:45 ` Stewart Hildebrand
@ 2024-04-26 1:48 ` Henry Wang
0 siblings, 0 replies; 58+ messages in thread
From: Henry Wang @ 2024-04-26 1:48 UTC (permalink / raw)
To: Stewart Hildebrand, Jan Beulich
Cc: Anthony PERARD, Stefano Stabellini, xen-devel
Hi Stewart,
On 4/26/2024 9:45 AM, Stewart Hildebrand wrote:
> On 4/24/24 20:43, Henry Wang wrote:
>> Hi Jan,
>>
>> On 4/24/2024 2:08 PM, Jan Beulich wrote:
>>> On 24.04.2024 05:34, Henry Wang wrote:
>>>> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>>>>
>>>> This user level application copies the overlay dtbo shared by dom0 while doing
>>>> overlay node assignment operation. It uses xenstore to communicate with dom0.
>>>> More information on the protocol is writtien in docs/misc/overlay.txt file.
>>>>
>>>> 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>
>>>> ---
>>>> tools/helpers/Makefile | 8 +
>>>> tools/helpers/get_overlay.c | 393 ++++++++++++++++++++++++++++++++++++
>>>> 2 files changed, 401 insertions(+)
>>>> create mode 100644 tools/helpers/get_overlay.c
>>> As mentioned before on various occasions - new files preferably use dashes as
>>> separators in preference to underscores. You not doing so is particularly
>>> puzzling seeing ...
>>>
>>>> --- a/tools/helpers/Makefile
>>>> +++ b/tools/helpers/Makefile
>>>> @@ -12,6 +12,7 @@ TARGETS += init-xenstore-domain
>>>> endif
>>>> ifeq ($(CONFIG_ARM),y)
>>>> TARGETS += init-dom0less
>>>> +TARGETS += get_overlay
>>> ... patch context here (demonstrating a whopping 3 dashes used in similar
>>> cases).
>> I am not very sure why Vikram used "_" in the original patch. However I agree you are correct. Since I am currently doing the follow up of this series, I will use "-" in v2 as suggested. Thanks.
> Please also add tools/helpers/get-overlay to .gitignore
Thanks for the reminder! Yes sure I will add it.
Kind regards,
Henry
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 07/15] xen/overlay: Enable device tree overlay assignment to running domains
2024-04-24 6:05 ` Jan Beulich
@ 2024-04-29 3:36 ` Henry Wang
2024-04-29 6:43 ` Jan Beulich
2024-04-29 17:34 ` Julien Grall
0 siblings, 2 replies; 58+ messages in thread
From: Henry Wang @ 2024-04-29 3:36 UTC (permalink / raw)
To: Jan Beulich, Julien Grall, Stefano Stabellini
Cc: Bertrand Marquis, Michal Orzel, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, xen-devel
Hi Jan, Julien, Stefano,
On 4/24/2024 2:05 PM, Jan Beulich wrote:
> On 24.04.2024 05:34, Henry Wang wrote:
>> --- a/xen/include/public/sysctl.h
>> +++ b/xen/include/public/sysctl.h
>> @@ -1197,7 +1197,9 @@ struct xen_sysctl_dt_overlay {
>> #define XEN_SYSCTL_DT_OVERLAY_ADD 1
>> #define XEN_SYSCTL_DT_OVERLAY_REMOVE 2
>> uint8_t overlay_op; /* IN: Add or remove. */
>> - uint8_t pad[3]; /* IN: Must be zero. */
>> + bool domain_mapping; /* IN: True of False. */
>> + uint8_t pad[2]; /* IN: Must be zero. */
>> + uint32_t domain_id;
>> };
> If you merely re-purposed padding fields, all would be fine without
> bumping the interface version. Yet you don't, albeit for an unclear
> reason: Why uint32_t rather than domid_t? And on top of that - why a
> separate boolean when you could use e.g. DOMID_INVALID to indicate
> "no domain mapping"?
I think both of your suggestion make great sense. I will follow the
suggestion in v2.
> That said - anything taking a domain ID is certainly suspicious in a
> sysctl. Judging from the description you really mean this to be a
> domctl. Anything else will require extra justification.
I also think a domctl is better. I had a look at the history of the
already merged series, it looks like in the first version of merged part
1 [1], the hypercall was implemented as the domctl in the beginning but
later in v2 changed to sysctl. I think this makes sense as the scope of
that time is just to make Xen aware of the device tree node via Xen
device tree.
However this is now a problem for the current part where the scope (and
the end goal) is extended to assign the added device to Linux Dom0/DomU
via device tree overlays. I am not sure which way is better, should we
repurposing the sysctl to domctl or maybe add another domctl (I am
worrying about the duplication because basically we need the same sysctl
functionality but now with a domid in it)? What do you think?
@Stefano: Since I am not 100% if I understand the whole story behind
this feature, would you mind checking if I am providing correct
information above and sharing your opinions on this? Thank you very much!
[1]
https://lore.kernel.org/xen-devel/13240b69-f7bb-6a64-b89c-b7c2cbb7e465@xen.org/
Kind regards,
Henry
> Jan
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 07/15] xen/overlay: Enable device tree overlay assignment to running domains
2024-04-29 3:36 ` Henry Wang
@ 2024-04-29 6:43 ` Jan Beulich
2024-04-29 17:34 ` Julien Grall
1 sibling, 0 replies; 58+ messages in thread
From: Jan Beulich @ 2024-04-29 6:43 UTC (permalink / raw)
To: Henry Wang
Cc: Bertrand Marquis, Michal Orzel, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, xen-devel, Julien Grall, Stefano Stabellini
On 29.04.2024 05:36, Henry Wang wrote:
> Hi Jan, Julien, Stefano,
>
> On 4/24/2024 2:05 PM, Jan Beulich wrote:
>> On 24.04.2024 05:34, Henry Wang wrote:
>>> --- a/xen/include/public/sysctl.h
>>> +++ b/xen/include/public/sysctl.h
>>> @@ -1197,7 +1197,9 @@ struct xen_sysctl_dt_overlay {
>>> #define XEN_SYSCTL_DT_OVERLAY_ADD 1
>>> #define XEN_SYSCTL_DT_OVERLAY_REMOVE 2
>>> uint8_t overlay_op; /* IN: Add or remove. */
>>> - uint8_t pad[3]; /* IN: Must be zero. */
>>> + bool domain_mapping; /* IN: True of False. */
>>> + uint8_t pad[2]; /* IN: Must be zero. */
>>> + uint32_t domain_id;
>>> };
>> If you merely re-purposed padding fields, all would be fine without
>> bumping the interface version. Yet you don't, albeit for an unclear
>> reason: Why uint32_t rather than domid_t? And on top of that - why a
>> separate boolean when you could use e.g. DOMID_INVALID to indicate
>> "no domain mapping"?
>
> I think both of your suggestion make great sense. I will follow the
> suggestion in v2.
>
>> That said - anything taking a domain ID is certainly suspicious in a
>> sysctl. Judging from the description you really mean this to be a
>> domctl. Anything else will require extra justification.
>
> I also think a domctl is better. I had a look at the history of the
> already merged series, it looks like in the first version of merged part
> 1 [1], the hypercall was implemented as the domctl in the beginning but
> later in v2 changed to sysctl. I think this makes sense as the scope of
> that time is just to make Xen aware of the device tree node via Xen
> device tree.
>
> However this is now a problem for the current part where the scope (and
> the end goal) is extended to assign the added device to Linux Dom0/DomU
> via device tree overlays. I am not sure which way is better, should we
> repurposing the sysctl to domctl or maybe add another domctl (I am
> worrying about the duplication because basically we need the same sysctl
> functionality but now with a domid in it)? What do you think?
I'm taking it that what is a sysctl right now legitimately is. Therefore
folding both into domctl would at least be bending the rules of what
should be sysctl and what domctl. It would need clarifying what (fake)
domain such a (folded) domctl ought to operate on for the case that's
currently a sysctl. That then may (or may not) be justification for such
folding.
Jan
> @Stefano: Since I am not 100% if I understand the whole story behind
> this feature, would you mind checking if I am providing correct
> information above and sharing your opinions on this? Thank you very much!
>
> [1]
> https://lore.kernel.org/xen-devel/13240b69-f7bb-6a64-b89c-b7c2cbb7e465@xen.org/
>
> Kind regards,
> Henry
>
>> Jan
>
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 07/15] xen/overlay: Enable device tree overlay assignment to running domains
2024-04-29 3:36 ` Henry Wang
2024-04-29 6:43 ` Jan Beulich
@ 2024-04-29 17:34 ` Julien Grall
2024-04-30 4:00 ` Henry Wang
1 sibling, 1 reply; 58+ messages in thread
From: Julien Grall @ 2024-04-29 17:34 UTC (permalink / raw)
To: Henry Wang, Jan Beulich, Stefano Stabellini
Cc: Bertrand Marquis, Michal Orzel, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, xen-devel
On 29/04/2024 04:36, Henry Wang wrote:
> Hi Jan, Julien, Stefano,
Hi Henry,
> On 4/24/2024 2:05 PM, Jan Beulich wrote:
>> On 24.04.2024 05:34, Henry Wang wrote:
>>> --- a/xen/include/public/sysctl.h
>>> +++ b/xen/include/public/sysctl.h
>>> @@ -1197,7 +1197,9 @@ struct xen_sysctl_dt_overlay {
>>> #define XEN_SYSCTL_DT_OVERLAY_ADD 1
>>> #define XEN_SYSCTL_DT_OVERLAY_REMOVE 2
>>> uint8_t overlay_op; /* IN: Add or remove. */
>>> - uint8_t pad[3]; /* IN: Must be zero. */
>>> + bool domain_mapping; /* IN: True of False. */
>>> + uint8_t pad[2]; /* IN: Must be zero. */
>>> + uint32_t domain_id;
>>> };
>> If you merely re-purposed padding fields, all would be fine without
>> bumping the interface version. Yet you don't, albeit for an unclear
>> reason: Why uint32_t rather than domid_t? And on top of that - why a
>> separate boolean when you could use e.g. DOMID_INVALID to indicate
>> "no domain mapping"?
>
> I think both of your suggestion make great sense. I will follow the
> suggestion in v2.
>
>> That said - anything taking a domain ID is certainly suspicious in a
>> sysctl. Judging from the description you really mean this to be a
>> domctl. Anything else will require extra justification.
>
> I also think a domctl is better. I had a look at the history of the
> already merged series, it looks like in the first version of merged part
> 1 [1], the hypercall was implemented as the domctl in the beginning but
> later in v2 changed to sysctl. I think this makes sense as the scope of
> that time is just to make Xen aware of the device tree node via Xen
> device tree.
>
> However this is now a problem for the current part where the scope (and
> the end goal) is extended to assign the added device to Linux Dom0/DomU
> via device tree overlays. I am not sure which way is better, should we
> repurposing the sysctl to domctl or maybe add another domctl (I am
> worrying about the duplication because basically we need the same sysctl
> functionality but now with a domid in it)? What do you think?
I am not entirely sure this is a good idea to try to add the device in
Xen and attach it to the guests at the same time. Imagine the following
situation:
1) Add and attach devices
2) The domain is rebooted
3) Detach and remove devices
After step 2, you technically have a new domain. You could have also a
case where this is a completely different guest. So the flow would look
a little bit weird (you create the DT overlay with domain A but remove
with domain B).
So, at the moment, it feels like the add/attach (resp detech/remove)
operations should happen separately.
Can you clarify why you want to add devices to Xen and attach to a guest
within a single hypercall?
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM
2024-04-25 14:28 ` Julien Grall
@ 2024-04-30 3:50 ` Henry Wang
2024-04-30 20:13 ` Julien Grall
0 siblings, 1 reply; 58+ messages in thread
From: Henry Wang @ 2024-04-30 3:50 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Stefano Stabellini
Hi Julien,
Sorry for the late reply,
On 4/25/2024 10:28 PM, Julien Grall wrote:
> Hi,
>
> On 25/04/2024 08:06, Henry Wang wrote:
>> Hi Julien,
>>
>> On 4/24/2024 8:58 PM, Julien Grall wrote:
>>> Hi Henry,
>>>
>>> On 24/04/2024 04:34, Henry Wang wrote:
>>>> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>>>>
>>>> Enable interrupt assign/remove for running VMs in CONFIG_OVERLAY_DTB.
>>>>
>>>> Currently, irq_route and mapping is only allowed at the domain
>>>> creation. Adding
>>>> exception for CONFIG_OVERLAY_DTB.
>>>
>>> AFAICT, this is mostly reverting b8577547236f ("xen/arm: Restrict
>>> when a physical IRQ can be routed/removed from/to a domain").
>>>
>>>>
>>>> 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>
>>>> ---
>>>> xen/arch/arm/gic.c | 4 ++++
>>>> 1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>>> index 44c40e86de..a775f886ed 100644
>>>> --- a/xen/arch/arm/gic.c
>>>> +++ b/xen/arch/arm/gic.c
>>>> @@ -140,8 +140,10 @@ int gic_route_irq_to_guest(struct domain *d,
>>>> unsigned int virq,
>>>> * back to the physical IRQ. To prevent get unsync, restrict the
>>>> * routing to when the Domain is been created.
>>>> */
>>>
>>> The above comment explains why the check was added. But the commit
>>> message doesn't explain why this can be disregarded for your use-case.
>>>
>>> Looking at the history, I don't think you can simply remove the checks.
>>>
>>> Regardless that...
>>>
>>>> +#ifndef CONFIG_OVERLAY_DTB
>>>
>>> ... I am against such #ifdef. A distros may want to have OVERLAY_DTB
>>> enabled, yet the user will not use it.
>>>
>>> Instead, you want to remove the check once the code can properly
>>> handle routing an IRQ the domain is created or ...
>>>
>>>> if ( d->creation_finished )
>>>> return -EBUSY;
>>>> +#endif
>>>> ret = vgic_connect_hw_irq(d, NULL, virq, desc, true);
>>>> if ( ret )
>>>> @@ -171,8 +173,10 @@ int gic_remove_irq_from_guest(struct domain
>>>> *d, unsigned int virq,
>>>> * Removing an interrupt while the domain is running may have
>>>> * undesirable effect on the vGIC emulation.
>>>> */
>>>> +#ifndef CONFIG_OVERLAY_DTB
>>>> if ( !d->is_dying )
>>>> return -EBUSY;
>>>> +#endif
>>>
>>> ... removed before they domain is destroyed.
>>
>> Thanks for your feeedback. After checking the b8577547236f commit
>> message I think I now understand your point. Do you have any
>> suggestion about how can I properly add the support to route/remove
>> the IRQ to running domains? Thanks.
I spent some time going through the GIC/vGIC code and had some
discussions with Stefano and Stewart during the last couple of days, let
me see if I can describe the use case properly now to continue the
discussion:
We have some use cases that requires assigning devices to domains after
domain boot time. For example, suppose there is an FPGA on the board
which can simulate a device, and the bitstream for the FPGA is provided
and programmed after domain boot. So we need a way to assign the device
to the running domain. This series tries to implement this use case by
using device tree overlay - users can firstly add the overlay to Xen
dtb, assign the device in the overlay to a domain by the xl command,
then apply the overlay to Linux.
> I haven't really look at that code in quite a while. I think we need
> to make sure that the virtual and physical IRQ state matches at the
> time we do the routing.
>
> I am undecided on whether we want to simply prevent the action to
> happen or try to reset the state.
>
> There is also the question of what to do if the guest is enabling the
> vIRQ before it is routed.
Sorry for bothering, would you mind elaborating a bit more about the two
cases that you mentioned above? Commit b8577547236f ("xen/arm: Restrict
when a physical IRQ can be routed/removed from/to a domain") only said
there will be undesirable effects, so I am not sure if I understand the
concerns raised above and the consequences of these two use cases. I am
probably wrong, I think when we add the overlay, we are probably fine as
the interrupt is not being used before. Also since we only load the
device driver after the IRQ is routed to the guest, I am not sure the
guest can enable the vIRQ before it is routed.
Kind regards,
Henry
> Overall, someone needs to spend some time reading the code and then
> make a proposal (this could be just documentation if we believe it is
> safe to do). Both the current vGIC and the new one may need an update.
>
> Cheers,
>
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 07/15] xen/overlay: Enable device tree overlay assignment to running domains
2024-04-29 17:34 ` Julien Grall
@ 2024-04-30 4:00 ` Henry Wang
2024-04-30 9:47 ` Julien Grall
2024-05-02 18:02 ` Stefano Stabellini
0 siblings, 2 replies; 58+ messages in thread
From: Henry Wang @ 2024-04-30 4:00 UTC (permalink / raw)
To: Julien Grall, Jan Beulich, Stefano Stabellini
Cc: Bertrand Marquis, Michal Orzel, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, xen-devel
Hi Julien,
On 4/30/2024 1:34 AM, Julien Grall wrote:
> On 29/04/2024 04:36, Henry Wang wrote:
>> Hi Jan, Julien, Stefano,
>
> Hi Henry,
>
>> On 4/24/2024 2:05 PM, Jan Beulich wrote:
>>> On 24.04.2024 05:34, Henry Wang wrote:
>>>> --- a/xen/include/public/sysctl.h
>>>> +++ b/xen/include/public/sysctl.h
>>>> @@ -1197,7 +1197,9 @@ struct xen_sysctl_dt_overlay {
>>>> #define XEN_SYSCTL_DT_OVERLAY_ADD 1
>>>> #define XEN_SYSCTL_DT_OVERLAY_REMOVE 2
>>>> uint8_t overlay_op; /* IN: Add or remove. */
>>>> - uint8_t pad[3]; /* IN: Must be zero. */
>>>> + bool domain_mapping; /* IN: True of False. */
>>>> + uint8_t pad[2]; /* IN: Must be zero. */
>>>> + uint32_t domain_id;
>>>> };
>>> If you merely re-purposed padding fields, all would be fine without
>>> bumping the interface version. Yet you don't, albeit for an unclear
>>> reason: Why uint32_t rather than domid_t? And on top of that - why a
>>> separate boolean when you could use e.g. DOMID_INVALID to indicate
>>> "no domain mapping"?
>>
>> I think both of your suggestion make great sense. I will follow the
>> suggestion in v2.
>>
>>> That said - anything taking a domain ID is certainly suspicious in a
>>> sysctl. Judging from the description you really mean this to be a
>>> domctl. Anything else will require extra justification.
>>
>> I also think a domctl is better. I had a look at the history of the
>> already merged series, it looks like in the first version of merged
>> part 1 [1], the hypercall was implemented as the domctl in the
>> beginning but later in v2 changed to sysctl. I think this makes sense
>> as the scope of that time is just to make Xen aware of the device
>> tree node via Xen device tree.
>>
>> However this is now a problem for the current part where the
>> scope (and the end goal) is extended to assign the added device to
>> Linux Dom0/DomU via device tree overlays. I am not sure which way is
>> better, should we repurposing the sysctl to domctl or maybe add
>> another domctl (I am worrying about the duplication because basically
>> we need the same sysctl functionality but now with a domid in it)?
>> What do you think?
>
> I am not entirely sure this is a good idea to try to add the device in
> Xen and attach it to the guests at the same time.
> Imagine the following situation:
>
> 1) Add and attach devices
> 2) The domain is rebooted
> 3) Detach and remove devices
>
> After step 2, you technically have a new domain. You could have also a
> case where this is a completely different guest. So the flow would
> look a little bit weird (you create the DT overlay with domain A but
> remove with domain B).
>
> So, at the moment, it feels like the add/attach (resp detech/remove)
> operations should happen separately.
>
> Can you clarify why you want to add devices to Xen and attach to a
> guest within a single hypercall?
Sorry I don't know if there is any specific thoughts on the design of
using a single hypercall to do both add devices to Xen device tree and
assign the device to the guest. In fact seeing your above comments, I
think separating these two functionality to two xl commands using
separated hypercalls would indeed be a better idea. Thank you for the
suggestion!
To make sure I understand correctly, would you mind confirming if below
actions for v2 make sense to you? Thanks!
- Only use the XEN_SYSCTL_DT_OVERLAY_{ADD, REMOVE} sysctls to add/remove
overlay to Xen device tree
- Introduce the xl dt-overlay attach <domid> command and respective
domctls to do the device assignment for the overlay to domain.
Kind regards,
Henry
>
> Cheers,
>
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 07/15] xen/overlay: Enable device tree overlay assignment to running domains
2024-04-30 4:00 ` Henry Wang
@ 2024-04-30 9:47 ` Julien Grall
2024-05-06 5:26 ` Henry Wang
2024-05-02 18:02 ` Stefano Stabellini
1 sibling, 1 reply; 58+ messages in thread
From: Julien Grall @ 2024-04-30 9:47 UTC (permalink / raw)
To: Henry Wang, Jan Beulich, Stefano Stabellini
Cc: Bertrand Marquis, Michal Orzel, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, xen-devel
On 30/04/2024 05:00, Henry Wang wrote:
> Hi Julien,
Hi Henry,
> On 4/30/2024 1:34 AM, Julien Grall wrote:
>> On 29/04/2024 04:36, Henry Wang wrote:
>>> Hi Jan, Julien, Stefano,
>>
>> Hi Henry,
>>
>>> On 4/24/2024 2:05 PM, Jan Beulich wrote:
>>>> On 24.04.2024 05:34, Henry Wang wrote:
>>>>> --- a/xen/include/public/sysctl.h
>>>>> +++ b/xen/include/public/sysctl.h
>>>>> @@ -1197,7 +1197,9 @@ struct xen_sysctl_dt_overlay {
>>>>> #define XEN_SYSCTL_DT_OVERLAY_ADD 1
>>>>> #define XEN_SYSCTL_DT_OVERLAY_REMOVE 2
>>>>> uint8_t overlay_op; /* IN: Add or remove. */
>>>>> - uint8_t pad[3]; /* IN: Must be zero. */
>>>>> + bool domain_mapping; /* IN: True of False. */
>>>>> + uint8_t pad[2]; /* IN: Must be zero. */
>>>>> + uint32_t domain_id;
>>>>> };
>>>> If you merely re-purposed padding fields, all would be fine without
>>>> bumping the interface version. Yet you don't, albeit for an unclear
>>>> reason: Why uint32_t rather than domid_t? And on top of that - why a
>>>> separate boolean when you could use e.g. DOMID_INVALID to indicate
>>>> "no domain mapping"?
>>>
>>> I think both of your suggestion make great sense. I will follow the
>>> suggestion in v2.
>>>
>>>> That said - anything taking a domain ID is certainly suspicious in a
>>>> sysctl. Judging from the description you really mean this to be a
>>>> domctl. Anything else will require extra justification.
>>>
>>> I also think a domctl is better. I had a look at the history of the
>>> already merged series, it looks like in the first version of merged
>>> part 1 [1], the hypercall was implemented as the domctl in the
>>> beginning but later in v2 changed to sysctl. I think this makes sense
>>> as the scope of that time is just to make Xen aware of the device
>>> tree node via Xen device tree.
>>>
>>> However this is now a problem for the current part where the
>>> scope (and the end goal) is extended to assign the added device to
>>> Linux Dom0/DomU via device tree overlays. I am not sure which way is
>>> better, should we repurposing the sysctl to domctl or maybe add
>>> another domctl (I am worrying about the duplication because basically
>>> we need the same sysctl functionality but now with a domid in it)?
>>> What do you think?
>>
>> I am not entirely sure this is a good idea to try to add the device in
>> Xen and attach it to the guests at the same time. Imagine the
>> following situation:
>>
>> 1) Add and attach devices
>> 2) The domain is rebooted
>> 3) Detach and remove devices
>>
>> After step 2, you technically have a new domain. You could have also a
>> case where this is a completely different guest. So the flow would
>> look a little bit weird (you create the DT overlay with domain A but
>> remove with domain B).
>>
>> So, at the moment, it feels like the add/attach (resp detech/remove)
>> operations should happen separately.
Thinking a bit more about it, there is another problem with the single
hypercall appproach. The MMIOs will be mapped 1:1 to the guest. These
region may clash with other part of the layout for domain created by the
toolstack
and dom0less (if the 1:1 option has not been enabled).
I guess for that add, it would be possible to specify the mapping in the
Device-Tree. But that would not work for the removal (this may be a
different domain).
On a somewhat similar topic, the number of IRQs supported by the vGIC is
fixed at boot. How would that work with this patch?
>>
>> Can you clarify why you want to add devices to Xen and attach to a
>> guest within a single hypercall?
>
> Sorry I don't know if there is any specific thoughts on the design of
> using a single hypercall to do both add devices to Xen device tree and
> assign the device to the guest. In fact seeing your above comments, I
> think separating these two functionality to two xl commands using
> separated hypercalls would indeed be a better idea. Thank you for the
> suggestion!
>
> To make sure I understand correctly, would you mind confirming if below
> actions for v2 make sense to you? Thanks!
> - Only use the XEN_SYSCTL_DT_OVERLAY_{ADD, REMOVE} sysctls to add/remove
> overlay to Xen device tree
Note that this would attach the devices to dom0 first. Maybe this is why
it was decided to merge the two operations? An option would be to allow
the devices to be attached to no-one.
> - Introduce the xl dt-overlay attach <domid> command and respective
> domctls to do the device assignment for the overlay to domain.
We already have domctls to route IRQs and map MMIOs. So do we actually
need new domctls?
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM
2024-04-30 3:50 ` Henry Wang
@ 2024-04-30 20:13 ` Julien Grall
2024-05-06 8:32 ` Henry Wang
0 siblings, 1 reply; 58+ messages in thread
From: Julien Grall @ 2024-04-30 20:13 UTC (permalink / raw)
To: Henry Wang, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Stefano Stabellini
Hi Henry,
On 30/04/2024 04:50, Henry Wang wrote:
> On 4/25/2024 10:28 PM, Julien Grall wrote:
>>> Thanks for your feeedback. After checking the b8577547236f commit
>>> message I think I now understand your point. Do you have any
>>> suggestion about how can I properly add the support to route/remove
>>> the IRQ to running domains? Thanks.
>
> I spent some time going through the GIC/vGIC code and had some
> discussions with Stefano and Stewart during the last couple of days, let
> me see if I can describe the use case properly now to continue the
> discussion:
>
> We have some use cases that requires assigning devices to domains after
> domain boot time. For example, suppose there is an FPGA on the board
> which can simulate a device, and the bitstream for the FPGA is provided
> and programmed after domain boot. So we need a way to assign the device
> to the running domain. This series tries to implement this use case by
> using device tree overlay - users can firstly add the overlay to Xen
> dtb, assign the device in the overlay to a domain by the xl command,
> then apply the overlay to Linux.
Thanks for the description! This helps to understand your goal :).
>
>> I haven't really look at that code in quite a while. I think we need
>> to make sure that the virtual and physical IRQ state matches at the
>> time we do the routing.
>>
>> I am undecided on whether we want to simply prevent the action to
>> happen or try to reset the state.
>>
>> There is also the question of what to do if the guest is enabling the
>> vIRQ before it is routed.
>
> Sorry for bothering, would you mind elaborating a bit more about the two
> cases that you mentioned above? Commit b8577547236f ("xen/arm: Restrict
> when a physical IRQ can be routed/removed from/to a domain") only said
> there will be undesirable effects, so I am not sure if I understand the
> concerns raised above and the consequences of these two use cases.
I will try to explain them below after I answer the rest.
> I am
> probably wrong, I think when we add the overlay, we are probably fine as
> the interrupt is not being used before.
What if the DT overlay is unloaded and then reloaded? Wouldn't the same
interrupt be re-used? As a more generic case, this could also be a new
bitstream for the FPGA.
But even if the interrupt is brand new every time for the DT overlay,
you are effectively relaxing the check for every user (such as
XEN_DOMCTL_bind_pt_irq). So the interrupt re-use case needs to be taken
into account.
> Also since we only load the
> device driver after the IRQ is routed to the guest,
This is what a well-behave guest will do. However, we need to think what
will happen if a guest misbehaves. I am not concerned about a guest only
impacting itself, I am more concerned about the case where the rest of
the system is impacted.
> I am not sure the
> guest can enable the vIRQ before it is routed.
Xen allows the guest to enable a vIRQ even if there is no pIRQ assigned.
Thanksfully, it looks like the vgic_connect_hw_irq(), in both the
current and new vGIC, will return an error if we are trying to route a
pIRQ to an already enabled vIRQ.
But we need to investigate all the possible scenarios to make sure that
any inconsistencies between the physical state and virtual state
(including the LRs) will not result to bigger problem.
The one that comes to my mind is: The physical interrupt is de-assigned
from the guest before it was EOIed. In this case, the interrupt will
still be in the LR with the HW bit set. This would allow the guest to
EOI the interrupt even if it is routed to someone else. It is unclear
what would be the impact on the other guest.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 04/15] tools/libs/light: Always enable IOMMU
2024-04-24 3:34 ` [PATCH 04/15] tools/libs/light: " Henry Wang
@ 2024-05-01 13:47 ` Anthony PERARD
2024-05-06 3:17 ` Henry Wang
0 siblings, 1 reply; 58+ messages in thread
From: Anthony PERARD @ 2024-05-01 13:47 UTC (permalink / raw)
To: Henry Wang; +Cc: xen-devel, Vikram Garhwal, Juergen Gross
On Wed, Apr 24, 2024 at 11:34:38AM +0800, Henry Wang wrote:
> For overlay with iommu functionality to work with running VMs, we need
> to enable IOMMU when iomem presents for the domains.
>
> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
> Signed-off-by: Henry Wang <xin.wang2@amd.com>
> ---
> tools/libs/light/libxl_arm.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 1cb89fa584..dd5c9f4917 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -222,6 +222,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> config->arch.sve_vl = d_config->b_info.arch_arm.sve_vl / 128U;
> }
>
> +#ifdef LIBXL_HAVE_DT_OVERLAY
libxl_arm.c is only build on Arm, so this should be defined, so no need
to check.
> + if (d_config->b_info.num_iomem) {
> + config->flags |= XEN_DOMCTL_CDF_iommu;
Is this doing the same thing as the previous patch?
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 05/15] tools/libs/light: Increase nr_spi to 160
2024-04-24 3:34 ` [PATCH 05/15] tools/libs/light: Increase nr_spi to 160 Henry Wang
@ 2024-05-01 13:58 ` Anthony PERARD
2024-05-06 5:17 ` Henry Wang
0 siblings, 1 reply; 58+ messages in thread
From: Anthony PERARD @ 2024-05-01 13:58 UTC (permalink / raw)
To: Henry Wang; +Cc: xen-devel, Vikram Garhwal, Juergen Gross, Stefano Stabellini
On Wed, Apr 24, 2024 at 11:34:39AM +0800, Henry Wang wrote:
> Increase number of spi to 160 i.e. gic_number_lines() for Xilinx ZynqMP - 32.
> This was done to allocate and assign IRQs to a running domain.
>
> 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>
> ---
> tools/libs/light/libxl_arm.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index dd5c9f4917..50dbd0f2a9 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -181,7 +181,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>
> LOG(DEBUG, "Configure the domain");
>
> - config->arch.nr_spis = nr_spis;
> + /* gic_number_lines() is 192 for Xilinx ZynqMP. min nr_spis = 192 - 32. */
> + config->arch.nr_spis = MAX(nr_spis, 160);
Is there a way that that Xen or libxl could find out what the minimum
number of SPI needs to be? Are we going to have to increase that minimum
number every time a new platform comes along?
It doesn't appear that libxl is using that `nr_spis` value and it is
probably just given to Xen. So my guess is that Xen could simply take
care of the minimum value, gic_number_lines() seems to be a Xen
function.
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 08/15] tools: Add domain_id and expert mode for overlay operations
2024-04-24 3:34 ` [PATCH 08/15] tools: Add domain_id and expert mode for overlay operations Henry Wang
@ 2024-05-01 14:46 ` Anthony PERARD
2024-05-06 5:51 ` Henry Wang
0 siblings, 1 reply; 58+ messages in thread
From: Anthony PERARD @ 2024-05-01 14:46 UTC (permalink / raw)
To: Henry Wang; +Cc: xen-devel, Vikram Garhwal, Juergen Gross, Stefano Stabellini
On Wed, Apr 24, 2024 at 11:34:42AM +0800, Henry Wang wrote:
> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>
> Add domain_id and expert mode for overlay assignment. This enables dynamic
> programming of nodes during runtime.
>
> Take the opportunity to fix the name mismatch in the xl command, the
> command name should be "dt-overlay" instead of "dt_overlay".
I don't like much these unrelated / opportunistic changes in a patch,
I'd rather have a separate patch. And in this case, if it was on a
separate patch, that separated patch could gain: Fixes: 61765a07e3d8
("tools/xl: Add new xl command overlay for device tree overlay support")
and potentially backported.
> 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>
> ---
> tools/include/libxl.h | 8 +++++--
> tools/include/xenctrl.h | 5 +++--
> tools/libs/ctrl/xc_dt_overlay.c | 7 ++++--
> tools/libs/light/libxl_dt_overlay.c | 17 +++++++++++----
> tools/xl/xl_vmcontrol.c | 34 ++++++++++++++++++++++++++---
> 5 files changed, 58 insertions(+), 13 deletions(-)
>
> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
> index 62cb07dea6..59a3e1b37c 100644
> --- a/tools/include/libxl.h
> +++ b/tools/include/libxl.h
> @@ -2549,8 +2549,12 @@ 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,
> - uint32_t overlay_size, uint8_t overlay_op);
> +#define LIBXL_DT_OVERLAY_ADD 1
> +#define LIBXL_DT_OVERLAY_REMOVE 2
> +
> +int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domain_id, void *overlay,
> + uint32_t overlay_size, uint8_t overlay_op, bool auto_mode,
> + bool domain_mapping);
Sorry, you cannot change the API of an existing libxl function without
providing something backward compatible. We have already a few example
of this changes in libxl.h, e.g.: fded24ea8315 ("libxl: Make libxl_set_vcpuonline async")
So, providing a wrapper called libxl_dt_overlay_0x041800() which call
the new function.
> #endif
>
> /*
> diff --git a/tools/libs/light/libxl_dt_overlay.c b/tools/libs/light/libxl_dt_overlay.c
> index a6c709a6dc..cdb62b28cf 100644
> --- a/tools/libs/light/libxl_dt_overlay.c
> +++ b/tools/libs/light/libxl_dt_overlay.c
> @@ -57,10 +58,18 @@ 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);
> + /* Check if user entered a valid domain id. */
> + rc = libxl_domain_info(CTX, NULL, domid);
> + if (rc == ERROR_DOMAIN_NOTFOUND) {
Why do you check specifically for "domain not found", what about other
error?
> + LOGD(ERROR, domid, "Non-existant domain.");
> + return ERROR_FAIL;
Use `goto out`, and you can let the function return
ERROR_DOMAIN_NOTFOUND if that the error, we can just propagate the `rc`
from libxl_domain_info().
> + }
> +
> + r = xc_dt_overlay(ctx->xch, domid, overlay_dt, overlay_dt_size, overlay_op,
> + domain_mapping);
>
> if (r) {
> - LOG(ERROR, "%s: Adding/Removing overlay dtb failed.", __func__);
> + LOG(ERROR, "domain%d: Adding/Removing overlay dtb failed.", domid);
You could replace the macro by LOGD, instead of handwriting "domain%d".
> rc = ERROR_FAIL;
> }
>
> diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
> index 98f6bd2e76..9674383ec3 100644
> --- a/tools/xl/xl_vmcontrol.c
> +++ b/tools/xl/xl_vmcontrol.c
> @@ -1270,21 +1270,48 @@ int main_dt_overlay(int argc, char **argv)
> {
> const char *overlay_ops = NULL;
> const char *overlay_config_file = NULL;
> + uint32_t domain_id = 0;
> void *overlay_dtb = NULL;
> int rc;
> + bool auto_mode = true;
> + bool domain_mapping = false;
> uint8_t op;
> int overlay_dtb_size = 0;
> const int overlay_add_op = 1;
> const int overlay_remove_op = 2;
>
> - if (argc < 2) {
> - help("dt_overlay");
> + if (argc < 3) {
> + help("dt-overlay");
> return EXIT_FAILURE;
> }
>
> + if (argc > 5) {
> + fprintf(stderr, "Too many arguments\n");
> + return ERROR_FAIL;
> + }
> +
> overlay_ops = argv[1];
> overlay_config_file = argv[2];
>
> + if (!strcmp(argv[argc - 1], "-e"))
> + auto_mode = false;
> +
> + if (argc == 4 || !auto_mode) {
> + domain_id = find_domain(argv[argc-1]);
> + domain_mapping = true;
> + }
> +
> + if (argc == 5 || !auto_mode) {
> + domain_id = find_domain(argv[argc-2]);
> + domain_mapping = true;
> + }
Sorry, I can't review that changes, this needs a change in the help
message of dt-overlay, and something in the man page. (and that argument
parsing looks convoluted).
> +
> + /* User didn't prove any overlay operation. */
I guess you meant "provide" instead of prove. But the comment can be
discarded, it doesn't explain anything useful that the error message
doesn't already explain.
> + if (overlay_ops == NULL) {
> + fprintf(stderr, "No overlay operation mode provided\n");
> + return ERROR_FAIL;
That should be EXIT_FAILURE instead. (and I realise that the function
already return ERROR_FAIL by mistake in several places. (ERROR_FAIL is a
libxl error return value of -3, which isn't really a good exit value for
CLI programmes.))
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 09/15] tools/libs/light: Modify dtbo to domU linux dtbo format
2024-04-24 3:34 ` [PATCH 09/15] tools/libs/light: Modify dtbo to domU linux dtbo format Henry Wang
@ 2024-05-01 15:09 ` Anthony PERARD
2024-05-06 5:40 ` Henry Wang
0 siblings, 1 reply; 58+ messages in thread
From: Anthony PERARD @ 2024-05-01 15:09 UTC (permalink / raw)
To: Henry Wang; +Cc: xen-devel, Vikram Garhwal, Juergen Gross, Stefano Stabellini
On Wed, Apr 24, 2024 at 11:34:43AM +0800, Henry Wang wrote:
> diff --git a/tools/libs/light/libxl_dt_overlay.c b/tools/libs/light/libxl_dt_overlay.c
> index cdb62b28cf..eaf11a0f9c 100644
> --- a/tools/libs/light/libxl_dt_overlay.c
> +++ b/tools/libs/light/libxl_dt_overlay.c
> @@ -41,6 +42,69 @@ static int check_overlay_fdt(libxl__gc *gc, void *fdt, size_t size)
> return 0;
> }
>
> +static int modify_overlay_for_domU(libxl__gc *gc, void *overlay_dt_domU,
> + size_t size)
> +{
> + int rc = 0;
> + int virtual_interrupt_parent = GUEST_PHANDLE_GIC;
> + const struct fdt_property *fdt_prop_node = NULL;
> + int overlay;
> + int prop_len = 0;
> + int subnode = 0;
> + int fragment;
> + const char *prop_name;
> + const char *target_path = "/";
> +
> + fdt_for_each_subnode(fragment, overlay_dt_domU, 0) {
> + prop_name = fdt_getprop(overlay_dt_domU, fragment, "target-path",
> + &prop_len);
> + if (prop_name == NULL) {
> + LOG(ERROR, "target-path property not found\n");
LOG* macros already takes care of adding \n, no need to add an extra
one.
> + rc = ERROR_FAIL;
> + goto err;
> + }
> +
> + /* Change target path for domU dtb. */
> + rc = fdt_setprop_string(overlay_dt_domU, fragment, "target-path",
fdt_setprop_string() isn't a libxl function, store the return value in a
variable named `r` instead.
> + target_path);
> + if (rc) {
> + LOG(ERROR, "Setting interrupt parent property failed for %s\n",
> + prop_name);
> + goto err;
> + }
> +
> + overlay = fdt_subnode_offset(overlay_dt_domU, fragment, "__overlay__");
> +
> + fdt_for_each_subnode(subnode, overlay_dt_domU, overlay)
> + {
> + const char *node_name = fdt_get_name(overlay_dt_domU, subnode,
> + NULL);
> +
> + fdt_prop_node = fdt_getprop(overlay_dt_domU, subnode,
> + "interrupt-parent", &prop_len);
> + if (fdt_prop_node == NULL) {
> + LOG(DETAIL, "%s property not found for %s. Skip to next node\n",
> + "interrupt-parent", node_name);
Why do you have "interrupt-parent" in a separate argument? Do you meant
to do something like
const char *some_name = "interrupt-parent";
and use that in the 4 different places that this string is used? (Using
a variable mean that we (or the compiler) can make sure that they are
all spelled correctly.
> + continue;
> + }
> +
> + rc = fdt_setprop_inplace_u32(overlay_dt_domU, subnode,
> + "interrupt-parent",
> + virtual_interrupt_parent);
> + if (rc) {
> + LOG(ERROR, "Setting interrupt parent property failed for %s\n",
> + "interrupt-parent");
> + goto err;
> + }
> + }
> + }
> +
> +return 0;
Missed indentation.
> +
> +err:
> + return rc;
A few things, looks like `rc` is always going to be ERROR_FAIL here,
unless you find an libxl_error code that better describe the error, so
you could forgo the `rc` variable.
Also, if you don't need to clean up anything in the function or have a
generic error message, you could simply "return " instead of using the
"goto" style.
> +}
> +
> int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domid, void *overlay_dt,
> uint32_t overlay_dt_size, uint8_t overlay_op,
> bool auto_mode, bool domain_mapping)
> @@ -73,6 +137,15 @@ int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domid, void *overlay_dt,
> rc = ERROR_FAIL;
> }
>
> + /*
> + * auto_mode doesn't apply to dom0 as dom0 can get the physical
> + * description of the hardware.
> + */
> + if (domid && auto_mode) {
> + if (overlay_op == LIBXL_DT_OVERLAY_ADD)
Shouldn't libxl complain if the operation is different?
> + rc = modify_overlay_for_domU(gc, overlay_dt, overlay_dt_size);
> + }
> +
> out:
> GC_FREE;
> return rc;
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 07/15] xen/overlay: Enable device tree overlay assignment to running domains
2024-04-30 4:00 ` Henry Wang
2024-04-30 9:47 ` Julien Grall
@ 2024-05-02 18:02 ` Stefano Stabellini
2024-05-06 3:14 ` Henry Wang
1 sibling, 1 reply; 58+ messages in thread
From: Stefano Stabellini @ 2024-05-02 18:02 UTC (permalink / raw)
To: Henry Wang
Cc: Julien Grall, Jan Beulich, Stefano Stabellini, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk, Andrew Cooper, George Dunlap,
xen-devel
[-- Attachment #1: Type: text/plain, Size: 4302 bytes --]
On Tue, 30 Apr 2024, Henry Wang wrote:
> Hi Julien,
>
> On 4/30/2024 1:34 AM, Julien Grall wrote:
> > On 29/04/2024 04:36, Henry Wang wrote:
> > > Hi Jan, Julien, Stefano,
> >
> > Hi Henry,
> >
> > > On 4/24/2024 2:05 PM, Jan Beulich wrote:
> > > > On 24.04.2024 05:34, Henry Wang wrote:
> > > > > --- a/xen/include/public/sysctl.h
> > > > > +++ b/xen/include/public/sysctl.h
> > > > > @@ -1197,7 +1197,9 @@ struct xen_sysctl_dt_overlay {
> > > > > #define XEN_SYSCTL_DT_OVERLAY_ADD 1
> > > > > #define XEN_SYSCTL_DT_OVERLAY_REMOVE 2
> > > > > uint8_t overlay_op; /* IN: Add or remove. */
> > > > > - uint8_t pad[3]; /* IN: Must be zero. */
> > > > > + bool domain_mapping; /* IN: True of False. */
> > > > > + uint8_t pad[2]; /* IN: Must be zero. */
> > > > > + uint32_t domain_id;
> > > > > };
> > > > If you merely re-purposed padding fields, all would be fine without
> > > > bumping the interface version. Yet you don't, albeit for an unclear
> > > > reason: Why uint32_t rather than domid_t? And on top of that - why a
> > > > separate boolean when you could use e.g. DOMID_INVALID to indicate
> > > > "no domain mapping"?
> > >
> > > I think both of your suggestion make great sense. I will follow the
> > > suggestion in v2.
> > >
> > > > That said - anything taking a domain ID is certainly suspicious in a
> > > > sysctl. Judging from the description you really mean this to be a
> > > > domctl. Anything else will require extra justification.
> > >
> > > I also think a domctl is better. I had a look at the history of the
> > > already merged series, it looks like in the first version of merged part 1
> > > [1], the hypercall was implemented as the domctl in the beginning but
> > > later in v2 changed to sysctl. I think this makes sense as the scope of
> > > that time is just to make Xen aware of the device tree node via Xen device
> > > tree.
> > >
> > > However this is now a problem for the current part where the scope (and
> > > the end goal) is extended to assign the added device to Linux Dom0/DomU
> > > via device tree overlays. I am not sure which way is better, should we
> > > repurposing the sysctl to domctl or maybe add another domctl (I am
> > > worrying about the duplication because basically we need the same sysctl
> > > functionality but now with a domid in it)? What do you think?
> >
> > I am not entirely sure this is a good idea to try to add the device in Xen
> > and attach it to the guests at the same time. Imagine the following
> > situation:
> >
> > 1) Add and attach devices
> > 2) The domain is rebooted
> > 3) Detach and remove devices
> >
> > After step 2, you technically have a new domain. You could have also a case
> > where this is a completely different guest. So the flow would look a little
> > bit weird (you create the DT overlay with domain A but remove with domain
> > B).
> >
> > So, at the moment, it feels like the add/attach (resp detech/remove)
> > operations should happen separately.
> >
> > Can you clarify why you want to add devices to Xen and attach to a guest
> > within a single hypercall?
>
> Sorry I don't know if there is any specific thoughts on the design of using a
> single hypercall to do both add devices to Xen device tree and assign the
> device to the guest. In fact seeing your above comments, I think separating
> these two functionality to two xl commands using separated hypercalls would
> indeed be a better idea. Thank you for the suggestion!
>
> To make sure I understand correctly, would you mind confirming if below
> actions for v2 make sense to you? Thanks!
> - Only use the XEN_SYSCTL_DT_OVERLAY_{ADD, REMOVE} sysctls to add/remove
> overlay to Xen device tree
> - Introduce the xl dt-overlay attach <domid> command and respective domctls to
> do the device assignment for the overlay to domain.
I think two hypercalls is OK. The original idea was to have a single xl
command to do the operation for user convenience (even that is not a
hard requirement) but that can result easily in two hypercalls.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 07/15] xen/overlay: Enable device tree overlay assignment to running domains
2024-05-02 18:02 ` Stefano Stabellini
@ 2024-05-06 3:14 ` Henry Wang
0 siblings, 0 replies; 58+ messages in thread
From: Henry Wang @ 2024-05-06 3:14 UTC (permalink / raw)
To: Stefano Stabellini, Julien Grall
Cc: Jan Beulich, Bertrand Marquis, Michal Orzel, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, xen-devel
Hi Stefano, Julien,
On 5/3/2024 2:02 AM, Stefano Stabellini wrote:
> On Tue, 30 Apr 2024, Henry Wang wrote:
>> Hi Julien,
>>
>> On 4/30/2024 1:34 AM, Julien Grall wrote:
>>> On 29/04/2024 04:36, Henry Wang wrote:
>>>> Hi Jan, Julien, Stefano,
>>> Hi Henry,
>>>
>>>> On 4/24/2024 2:05 PM, Jan Beulich wrote:
>>>>> On 24.04.2024 05:34, Henry Wang wrote:
>>>>>> --- a/xen/include/public/sysctl.h
>>>>>> +++ b/xen/include/public/sysctl.h
>>>>>> @@ -1197,7 +1197,9 @@ struct xen_sysctl_dt_overlay {
>>>>>> #define XEN_SYSCTL_DT_OVERLAY_ADD 1
>>>>>> #define XEN_SYSCTL_DT_OVERLAY_REMOVE 2
>>>>>> uint8_t overlay_op; /* IN: Add or remove. */
>>>>>> - uint8_t pad[3]; /* IN: Must be zero. */
>>>>>> + bool domain_mapping; /* IN: True of False. */
>>>>>> + uint8_t pad[2]; /* IN: Must be zero. */
>>>>>> + uint32_t domain_id;
>>>>>> };
>>>>> If you merely re-purposed padding fields, all would be fine without
>>>>> bumping the interface version. Yet you don't, albeit for an unclear
>>>>> reason: Why uint32_t rather than domid_t? And on top of that - why a
>>>>> separate boolean when you could use e.g. DOMID_INVALID to indicate
>>>>> "no domain mapping"?
>>>> I think both of your suggestion make great sense. I will follow the
>>>> suggestion in v2.
>>>>
>>>>> That said - anything taking a domain ID is certainly suspicious in a
>>>>> sysctl. Judging from the description you really mean this to be a
>>>>> domctl. Anything else will require extra justification.
>>>> I also think a domctl is better. I had a look at the history of the
>>>> already merged series, it looks like in the first version of merged part 1
>>>> [1], the hypercall was implemented as the domctl in the beginning but
>>>> later in v2 changed to sysctl. I think this makes sense as the scope of
>>>> that time is just to make Xen aware of the device tree node via Xen device
>>>> tree.
>>>>
>>>> However this is now a problem for the current part where the scope (and
>>>> the end goal) is extended to assign the added device to Linux Dom0/DomU
>>>> via device tree overlays. I am not sure which way is better, should we
>>>> repurposing the sysctl to domctl or maybe add another domctl (I am
>>>> worrying about the duplication because basically we need the same sysctl
>>>> functionality but now with a domid in it)? What do you think?
>>> I am not entirely sure this is a good idea to try to add the device in Xen
>>> and attach it to the guests at the same time. Imagine the following
>>> situation:
>>>
>>> 1) Add and attach devices
>>> 2) The domain is rebooted
>>> 3) Detach and remove devices
>>>
>>> After step 2, you technically have a new domain. You could have also a case
>>> where this is a completely different guest. So the flow would look a little
>>> bit weird (you create the DT overlay with domain A but remove with domain
>>> B).
>>>
>>> So, at the moment, it feels like the add/attach (resp detech/remove)
>>> operations should happen separately.
>>>
>>> Can you clarify why you want to add devices to Xen and attach to a guest
>>> within a single hypercall?
>> Sorry I don't know if there is any specific thoughts on the design of using a
>> single hypercall to do both add devices to Xen device tree and assign the
>> device to the guest. In fact seeing your above comments, I think separating
>> these two functionality to two xl commands using separated hypercalls would
>> indeed be a better idea. Thank you for the suggestion!
>>
>> To make sure I understand correctly, would you mind confirming if below
>> actions for v2 make sense to you? Thanks!
>> - Only use the XEN_SYSCTL_DT_OVERLAY_{ADD, REMOVE} sysctls to add/remove
>> overlay to Xen device tree
>> - Introduce the xl dt-overlay attach <domid> command and respective domctls to
>> do the device assignment for the overlay to domain.
> I think two hypercalls is OK. The original idea was to have a single xl
> command to do the operation for user convenience (even that is not a
> hard requirement) but that can result easily in two hypercalls.
Ok, sounds good. I will break the command to two hypercalls and try to
reuse the existing domctls for assign/remove IRQ/MMIO ranges.
Kind regards,
Henry
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 04/15] tools/libs/light: Always enable IOMMU
2024-05-01 13:47 ` Anthony PERARD
@ 2024-05-06 3:17 ` Henry Wang
0 siblings, 0 replies; 58+ messages in thread
From: Henry Wang @ 2024-05-06 3:17 UTC (permalink / raw)
To: Anthony PERARD; +Cc: xen-devel, Juergen Gross
Hi Anthony,
On 5/1/2024 9:47 PM, Anthony PERARD wrote:
> On Wed, Apr 24, 2024 at 11:34:38AM +0800, Henry Wang wrote:
>> For overlay with iommu functionality to work with running VMs, we need
>> to enable IOMMU when iomem presents for the domains.
>>
>> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
>> Signed-off-by: Henry Wang <xin.wang2@amd.com>
>> ---
>> tools/libs/light/libxl_arm.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>> index 1cb89fa584..dd5c9f4917 100644
>> --- a/tools/libs/light/libxl_arm.c
>> +++ b/tools/libs/light/libxl_arm.c
>> @@ -222,6 +222,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>> config->arch.sve_vl = d_config->b_info.arch_arm.sve_vl / 128U;
>> }
>>
>> +#ifdef LIBXL_HAVE_DT_OVERLAY
> libxl_arm.c is only build on Arm, so this should be defined, so no need
> to check.
Ah sure, I was just thought in the future RISC-V/PPC may have the same,
but you are correct. I will remove the check.
>> + if (d_config->b_info.num_iomem) {
>> + config->flags |= XEN_DOMCTL_CDF_iommu;
> Is this doing the same thing as the previous patch?
I think so, yes, we need the IOMMU flag to be set if we want to assign a
device from a DT node protected by IOMMU.
Kind regards,
Henry
>
> Thanks,
>
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 05/15] tools/libs/light: Increase nr_spi to 160
2024-05-01 13:58 ` Anthony PERARD
@ 2024-05-06 5:17 ` Henry Wang
2024-05-07 14:35 ` Julien Grall
0 siblings, 1 reply; 58+ messages in thread
From: Henry Wang @ 2024-05-06 5:17 UTC (permalink / raw)
To: Anthony PERARD, Stefano Stabellini, Julien Grall, Michal Orzel,
Bertrand Marquis
Cc: xen-devel, Vikram Garhwal, Juergen Gross, Stefano Stabellini
Hi Anthony,
(+Arm maintainers)
On 5/1/2024 9:58 PM, Anthony PERARD wrote:
> On Wed, Apr 24, 2024 at 11:34:39AM +0800, Henry Wang wrote:
>> Increase number of spi to 160 i.e. gic_number_lines() for Xilinx ZynqMP - 32.
>> This was done to allocate and assign IRQs to a running domain.
>>
>> 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>
>> ---
>> tools/libs/light/libxl_arm.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>> index dd5c9f4917..50dbd0f2a9 100644
>> --- a/tools/libs/light/libxl_arm.c
>> +++ b/tools/libs/light/libxl_arm.c
>> @@ -181,7 +181,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>>
>> LOG(DEBUG, "Configure the domain");
>>
>> - config->arch.nr_spis = nr_spis;
>> + /* gic_number_lines() is 192 for Xilinx ZynqMP. min nr_spis = 192 - 32. */
>> + config->arch.nr_spis = MAX(nr_spis, 160);
> Is there a way that that Xen or libxl could find out what the minimum
> number of SPI needs to be?
I am afraid currently there is none.
> Are we going to have to increase that minimum
> number every time a new platform comes along?
>
> It doesn't appear that libxl is using that `nr_spis` value and it is
> probably just given to Xen. So my guess is that Xen could simply take
> care of the minimum value, gic_number_lines() seems to be a Xen
> function.
Xen will take care of the value of nr_spis for dom0 in create_dom0()
dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32;
and also for dom0less domUs in create_domUs().
However, it looks like Xen will not take care of the mininum value for
libxl guests, the value from config->arch.nr_spis in guest config file
will be directly passed to the domain_vgic_init() function from
arch_domain_create().
I agree with you that we shouldn't just bump the number everytime when
we have a new platform. Therefore, would it be a good idea to move the
logic in this patch to arch_sanitise_domain_config()?
Kind regards,
Henry
>
> Thanks,
>
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 07/15] xen/overlay: Enable device tree overlay assignment to running domains
2024-04-30 9:47 ` Julien Grall
@ 2024-05-06 5:26 ` Henry Wang
0 siblings, 0 replies; 58+ messages in thread
From: Henry Wang @ 2024-05-06 5:26 UTC (permalink / raw)
To: Julien Grall, Jan Beulich, Stefano Stabellini
Cc: Bertrand Marquis, Michal Orzel, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, xen-devel
Hi Julien,
On 4/30/2024 5:47 PM, Julien Grall wrote:
> On 30/04/2024 05:00, Henry Wang wrote:
>> Hi Julien,
>
> Hi Henry,
>
>> On 4/30/2024 1:34 AM, Julien Grall wrote:
>>> On 29/04/2024 04:36, Henry Wang wrote:
>>>> Hi Jan, Julien, Stefano,
>>>
>>> Hi Henry,
>>>
>>>> On 4/24/2024 2:05 PM, Jan Beulich wrote:
>>>>> On 24.04.2024 05:34, Henry Wang wrote:
>>>>>> --- a/xen/include/public/sysctl.h
>>>>>> +++ b/xen/include/public/sysctl.h
>>>>>> @@ -1197,7 +1197,9 @@ struct xen_sysctl_dt_overlay {
>>>>>> #define XEN_SYSCTL_DT_OVERLAY_ADD 1
>>>>>> #define XEN_SYSCTL_DT_OVERLAY_REMOVE 2
>>>>>> uint8_t overlay_op; /* IN: Add or
>>>>>> remove. */
>>>>>> - uint8_t pad[3]; /* IN: Must be zero. */
>>>>>> + bool domain_mapping; /* IN: True of
>>>>>> False. */
>>>>>> + uint8_t pad[2]; /* IN: Must be zero. */
>>>>>> + uint32_t domain_id;
>>>>>> };
>>>>> If you merely re-purposed padding fields, all would be fine without
>>>>> bumping the interface version. Yet you don't, albeit for an unclear
>>>>> reason: Why uint32_t rather than domid_t? And on top of that - why a
>>>>> separate boolean when you could use e.g. DOMID_INVALID to indicate
>>>>> "no domain mapping"?
>>>>
>>>> I think both of your suggestion make great sense. I will follow the
>>>> suggestion in v2.
>>>>
>>>>> That said - anything taking a domain ID is certainly suspicious in a
>>>>> sysctl. Judging from the description you really mean this to be a
>>>>> domctl. Anything else will require extra justification.
>>>>
>>>> I also think a domctl is better. I had a look at the history of the
>>>> already merged series, it looks like in the first version of merged
>>>> part 1 [1], the hypercall was implemented as the domctl in the
>>>> beginning but later in v2 changed to sysctl. I think this makes
>>>> sense as the scope of that time is just to make Xen aware of the
>>>> device tree node via Xen device tree.
>>>>
>>>> However this is now a problem for the current part where the
>>>> scope (and the end goal) is extended to assign the added device to
>>>> Linux Dom0/DomU via device tree overlays. I am not sure which way
>>>> is better, should we repurposing the sysctl to domctl or maybe add
>>>> another domctl (I am worrying about the duplication because
>>>> basically we need the same sysctl functionality but now with a
>>>> domid in it)? What do you think?
>>>
>>> I am not entirely sure this is a good idea to try to add the device
>>> in Xen and attach it to the guests at the same time. Imagine the
>>> following situation:
>>>
>>> 1) Add and attach devices
>>> 2) The domain is rebooted
>>> 3) Detach and remove devices
>>>
>>> After step 2, you technically have a new domain. You could have also
>>> a case where this is a completely different guest. So the flow would
>>> look a little bit weird (you create the DT overlay with domain A but
>>> remove with domain B).
>>>
>>> So, at the moment, it feels like the add/attach (resp detech/remove)
>>> operations should happen separately.
>
> Thinking a bit more about it, there is another problem with the single
> hypercall appproach. The MMIOs will be mapped 1:1 to the guest. These
> region may clash with other part of the layout for domain created by
> the toolstack
> and dom0less (if the 1:1 option has not been enabled).
>
> I guess for that add, it would be possible to specify the mapping in
> the Device-Tree. But that would not work for the removal (this may be
> a different domain).
>
> On a somewhat similar topic, the number of IRQs supported by the vGIC
> is fixed at boot. How would that work with this patch?
Seeing your comment here I now realized patch #5 is to address this
issue. But I think we need to have a complete rework of the original
patch to make the feature portable. We can continue the discussion in
patch 5.
>>>
>>> Can you clarify why you want to add devices to Xen and attach to a
>>> guest within a single hypercall?
>>
>> Sorry I don't know if there is any specific thoughts on the design of
>> using a single hypercall to do both add devices to Xen device tree
>> and assign the device to the guest. In fact seeing your above
>> comments, I think separating these two functionality to two xl
>> commands using separated hypercalls would indeed be a better idea.
>> Thank you for the suggestion!
>>
>> To make sure I understand correctly, would you mind confirming if
>> below actions for v2 make sense to you? Thanks!
>> - Only use the XEN_SYSCTL_DT_OVERLAY_{ADD, REMOVE} sysctls to
>> add/remove overlay to Xen device tree
>
> Note that this would attach the devices to dom0 first. Maybe this is
> why it was decided to merge the two operations? An option would be to
> allow the devices to be attached to no-one.
>
>> - Introduce the xl dt-overlay attach <domid> command and respective
>> domctls to do the device assignment for the overlay to domain.
>
> We already have domctls to route IRQs and map MMIOs. So do we actually
> need new domctls?
No I don't think so, like you and Stefano said in the other thread, I
think I need to split the command to different hypercalls instead of
only one hypercall and reuse the existing domctl.
Kind regards,
Henry
>
> Cheers,
>
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 09/15] tools/libs/light: Modify dtbo to domU linux dtbo format
2024-05-01 15:09 ` Anthony PERARD
@ 2024-05-06 5:40 ` Henry Wang
0 siblings, 0 replies; 58+ messages in thread
From: Henry Wang @ 2024-05-06 5:40 UTC (permalink / raw)
To: Anthony PERARD; +Cc: xen-devel, Juergen Gross, Stefano Stabellini
Hi Anthony,
On 5/1/2024 11:09 PM, Anthony PERARD wrote:
> On Wed, Apr 24, 2024 at 11:34:43AM +0800, Henry Wang wrote:
>> diff --git a/tools/libs/light/libxl_dt_overlay.c b/tools/libs/light/libxl_dt_overlay.c
>> index cdb62b28cf..eaf11a0f9c 100644
>> --- a/tools/libs/light/libxl_dt_overlay.c
>> +++ b/tools/libs/light/libxl_dt_overlay.c
>> @@ -41,6 +42,69 @@ static int check_overlay_fdt(libxl__gc *gc, void *fdt, size_t size)
>> return 0;
>> }
>>
>> +static int modify_overlay_for_domU(libxl__gc *gc, void *overlay_dt_domU,
>> + size_t size)
>> +{
>> + int rc = 0;
>> + int virtual_interrupt_parent = GUEST_PHANDLE_GIC;
>> + const struct fdt_property *fdt_prop_node = NULL;
>> + int overlay;
>> + int prop_len = 0;
>> + int subnode = 0;
>> + int fragment;
>> + const char *prop_name;
>> + const char *target_path = "/";
>> +
>> + fdt_for_each_subnode(fragment, overlay_dt_domU, 0) {
>> + prop_name = fdt_getprop(overlay_dt_domU, fragment, "target-path",
>> + &prop_len);
>> + if (prop_name == NULL) {
>> + LOG(ERROR, "target-path property not found\n");
> LOG* macros already takes care of adding \n, no need to add an extra
> one.
Sure, I will remove the "\n".
>
>> + rc = ERROR_FAIL;
>> + goto err;
>> + }
>> +
>> + /* Change target path for domU dtb. */
>> + rc = fdt_setprop_string(overlay_dt_domU, fragment, "target-path",
> fdt_setprop_string() isn't a libxl function, store the return value in a
> variable named `r` instead.'
Thanks for spotting this. Will change it to `r`.
>> + target_path);
>> + if (rc) {
>> + LOG(ERROR, "Setting interrupt parent property failed for %s\n",
>> + prop_name);
>> + goto err;
>> + }
>> +
>> + overlay = fdt_subnode_offset(overlay_dt_domU, fragment, "__overlay__");
>> +
>> + fdt_for_each_subnode(subnode, overlay_dt_domU, overlay)
>> + {
>> + const char *node_name = fdt_get_name(overlay_dt_domU, subnode,
>> + NULL);
>> +
>> + fdt_prop_node = fdt_getprop(overlay_dt_domU, subnode,
>> + "interrupt-parent", &prop_len);
>> + if (fdt_prop_node == NULL) {
>> + LOG(DETAIL, "%s property not found for %s. Skip to next node\n",
>> + "interrupt-parent", node_name);
> Why do you have "interrupt-parent" in a separate argument? Do you meant
> to do something like
> const char *some_name = "interrupt-parent";
> and use that in the 4 different places that this string is used? (Using
> a variable mean that we (or the compiler) can make sure that they are
> all spelled correctly.
Great suggestion! I will do this way.
>> + continue;
>> + }
>> +
>> + rc = fdt_setprop_inplace_u32(overlay_dt_domU, subnode,
>> + "interrupt-parent",
>> + virtual_interrupt_parent);
>> + if (rc) {
>> + LOG(ERROR, "Setting interrupt parent property failed for %s\n",
>> + "interrupt-parent");
>> + goto err;
>> + }
>> + }
>> + }
>> +
>> +return 0;
> Missed indentation.
Will correct it.
>> +
>> +err:
>> + return rc;
> A few things, looks like `rc` is always going to be ERROR_FAIL here,
> unless you find an libxl_error code that better describe the error, so
> you could forgo the `rc` variable.
>
> Also, if you don't need to clean up anything in the function or have a
> generic error message, you could simply "return " instead of using the
> "goto" style.
Sure, I will simply use return because I don't really think there is
anything to be cleaned up.
>> +}
>> +
>> int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domid, void *overlay_dt,
>> uint32_t overlay_dt_size, uint8_t overlay_op,
>> bool auto_mode, bool domain_mapping)
>> @@ -73,6 +137,15 @@ int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domid, void *overlay_dt,
>> rc = ERROR_FAIL;
>> }
>>
>> + /*
>> + * auto_mode doesn't apply to dom0 as dom0 can get the physical
>> + * description of the hardware.
>> + */
>> + if (domid && auto_mode) {
>> + if (overlay_op == LIBXL_DT_OVERLAY_ADD)
> Shouldn't libxl complain if the operation is different?
I will add corresponding error handling code here. Thanks!
Kind regards,
Henry
>> + rc = modify_overlay_for_domU(gc, overlay_dt, overlay_dt_size);
>> + }
>> +
>> out:
>> GC_FREE;
>> return rc;
> Thanks,
>
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 08/15] tools: Add domain_id and expert mode for overlay operations
2024-05-01 14:46 ` Anthony PERARD
@ 2024-05-06 5:51 ` Henry Wang
0 siblings, 0 replies; 58+ messages in thread
From: Henry Wang @ 2024-05-06 5:51 UTC (permalink / raw)
To: Anthony PERARD
Cc: xen-devel, Vikram Garhwal, Juergen Gross, Stefano Stabellini
Hi Anthony,
On 5/1/2024 10:46 PM, Anthony PERARD wrote:
> On Wed, Apr 24, 2024 at 11:34:42AM +0800, Henry Wang wrote:
>> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>>
>> Add domain_id and expert mode for overlay assignment. This enables dynamic
>> programming of nodes during runtime.
>>
>> Take the opportunity to fix the name mismatch in the xl command, the
>> command name should be "dt-overlay" instead of "dt_overlay".
> I don't like much these unrelated / opportunistic changes in a patch,
> I'd rather have a separate patch. And in this case, if it was on a
> separate patch, that separated patch could gain: Fixes: 61765a07e3d8
> ("tools/xl: Add new xl command overlay for device tree overlay support")
> and potentially backported.
Ok. I can split this part to a separated commit.
>> 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>
>> ---
>> tools/include/libxl.h | 8 +++++--
>> tools/include/xenctrl.h | 5 +++--
>> tools/libs/ctrl/xc_dt_overlay.c | 7 ++++--
>> tools/libs/light/libxl_dt_overlay.c | 17 +++++++++++----
>> tools/xl/xl_vmcontrol.c | 34 ++++++++++++++++++++++++++---
>> 5 files changed, 58 insertions(+), 13 deletions(-)
>>
>> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
>> index 62cb07dea6..59a3e1b37c 100644
>> --- a/tools/include/libxl.h
>> +++ b/tools/include/libxl.h
>> @@ -2549,8 +2549,12 @@ 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,
>> - uint32_t overlay_size, uint8_t overlay_op);
>> +#define LIBXL_DT_OVERLAY_ADD 1
>> +#define LIBXL_DT_OVERLAY_REMOVE 2
>> +
>> +int libxl_dt_overlay(libxl_ctx *ctx, uint32_t domain_id, void *overlay,
>> + uint32_t overlay_size, uint8_t overlay_op, bool auto_mode,
>> + bool domain_mapping);
> Sorry, you cannot change the API of an existing libxl function without
> providing something backward compatible. We have already a few example
> of this changes in libxl.h, e.g.: fded24ea8315 ("libxl: Make libxl_set_vcpuonline async")
> So, providing a wrapper called libxl_dt_overlay_0x041800() which call
> the new function.
Ok, I will add an wrapper.
>> #endif
>>
>> /*
>> diff --git a/tools/libs/light/libxl_dt_overlay.c b/tools/libs/light/libxl_dt_overlay.c
>> index a6c709a6dc..cdb62b28cf 100644
>> --- a/tools/libs/light/libxl_dt_overlay.c
>> +++ b/tools/libs/light/libxl_dt_overlay.c
>> @@ -57,10 +58,18 @@ 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);
>> + /* Check if user entered a valid domain id. */
>> + rc = libxl_domain_info(CTX, NULL, domid);
>> + if (rc == ERROR_DOMAIN_NOTFOUND) {
> Why do you check specifically for "domain not found", what about other
> error?
I agree this is indeed very confusing...I will rewrite this part
properly in the next version.
>> + LOGD(ERROR, domid, "Non-existant domain.");
>> + return ERROR_FAIL;
> Use `goto out`, and you can let the function return
> ERROR_DOMAIN_NOTFOUND if that the error, we can just propagate the `rc`
> from libxl_domain_info().
Sure, will do the suggested way.
>> + }
>> +
>> + r = xc_dt_overlay(ctx->xch, domid, overlay_dt, overlay_dt_size, overlay_op,
>> + domain_mapping);
>>
>> if (r) {
>> - LOG(ERROR, "%s: Adding/Removing overlay dtb failed.", __func__);
>> + LOG(ERROR, "domain%d: Adding/Removing overlay dtb failed.", domid);
> You could replace the macro by LOGD, instead of handwriting "domain%d".
Great suggestion. I will use LOGD.
>> rc = ERROR_FAIL;
>> }
>>
>> diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
>> index 98f6bd2e76..9674383ec3 100644
>> --- a/tools/xl/xl_vmcontrol.c
>> +++ b/tools/xl/xl_vmcontrol.c
>> @@ -1270,21 +1270,48 @@ int main_dt_overlay(int argc, char **argv)
>> {
>> const char *overlay_ops = NULL;
>> const char *overlay_config_file = NULL;
>> + uint32_t domain_id = 0;
>> void *overlay_dtb = NULL;
>> int rc;
>> + bool auto_mode = true;
>> + bool domain_mapping = false;
>> uint8_t op;
>> int overlay_dtb_size = 0;
>> const int overlay_add_op = 1;
>> const int overlay_remove_op = 2;
>>
>> - if (argc < 2) {
>> - help("dt_overlay");
>> + if (argc < 3) {
>> + help("dt-overlay");
>> return EXIT_FAILURE;
>> }
>>
>> + if (argc > 5) {
>> + fprintf(stderr, "Too many arguments\n");
>> + return ERROR_FAIL;
>> + }
>> +
>> overlay_ops = argv[1];
>> overlay_config_file = argv[2];
>>
>> + if (!strcmp(argv[argc - 1], "-e"))
>> + auto_mode = false;
>> +
>> + if (argc == 4 || !auto_mode) {
>> + domain_id = find_domain(argv[argc-1]);
>> + domain_mapping = true;
>> + }
>> +
>> + if (argc == 5 || !auto_mode) {
>> + domain_id = find_domain(argv[argc-2]);
>> + domain_mapping = true;
>> + }
> Sorry, I can't review that changes, this needs a change in the help
> message of dt-overlay, and something in the man page. (and that argument
> parsing looks convoluted).
I will rework this part to see if I can make it better.
>> +
>> + /* User didn't prove any overlay operation. */
> I guess you meant "provide" instead of prove. But the comment can be
> discarded, it doesn't explain anything useful that the error message
> doesn't already explain.
I think your comment is correct, I will just drop it.
>> + if (overlay_ops == NULL) {
>> + fprintf(stderr, "No overlay operation mode provided\n");
>> + return ERROR_FAIL;
> That should be EXIT_FAILURE instead. (and I realise that the function
> already return ERROR_FAIL by mistake in several places. (ERROR_FAIL is a
> libxl error return value of -3, which isn't really a good exit value for
> CLI programmes.))
Thanks. Will use EXIT_FAILURE.
Kind regards,
Henry
>
> Thanks,
>
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM
2024-04-30 20:13 ` Julien Grall
@ 2024-05-06 8:32 ` Henry Wang
2024-05-07 21:54 ` Julien Grall
0 siblings, 1 reply; 58+ messages in thread
From: Henry Wang @ 2024-05-06 8:32 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Stefano Stabellini
Hi Julien,
On 5/1/2024 4:13 AM, Julien Grall wrote:
> Hi Henry,
>
> On 30/04/2024 04:50, Henry Wang wrote:
>> On 4/25/2024 10:28 PM, Julien Grall wrote:
>>>> Thanks for your feeedback. After checking the b8577547236f commit
>>>> message I think I now understand your point. Do you have any
>>>> suggestion about how can I properly add the support to route/remove
>>>> the IRQ to running domains? Thanks.
>>
>> I spent some time going through the GIC/vGIC code and had some
>> discussions with Stefano and Stewart during the last couple of days,
>> let me see if I can describe the use case properly now to continue
>> the discussion:
>>
>> We have some use cases that requires assigning devices to domains
>> after domain boot time. For example, suppose there is an FPGA on the
>> board which can simulate a device, and the bitstream for the FPGA is
>> provided and programmed after domain boot. So we need a way to assign
>> the device to the running domain. This series tries to implement this
>> use case by using device tree overlay - users can firstly add the
>> overlay to Xen dtb, assign the device in the overlay to a domain by
>> the xl command, then apply the overlay to Linux.
>
> Thanks for the description! This helps to understand your goal :).
Thank you very much for spending your time on discussing this and
provide these valuable comments!
>>
>>> I haven't really look at that code in quite a while. I think we need
>>> to make sure that the virtual and physical IRQ state matches at the
>>> time we do the routing.
>>>
>>> I am undecided on whether we want to simply prevent the action to
>>> happen or try to reset the state.
>>>
>>> There is also the question of what to do if the guest is enabling
>>> the vIRQ before it is routed.
>>
>> Sorry for bothering, would you mind elaborating a bit more about the
>> two cases that you mentioned above? Commit b8577547236f ("xen/arm:
>> Restrict when a physical IRQ can be routed/removed from/to a domain")
>> only said there will be undesirable effects, so I am not sure if I
>> understand the concerns raised above and the consequences of these
>> two use cases.
>
> I will try to explain them below after I answer the rest.
>
>> I am probably wrong, I think when we add the overlay, we are probably
>> fine as the interrupt is not being used before.
>
> What if the DT overlay is unloaded and then reloaded? Wouldn't the
> same interrupt be re-used? As a more generic case, this could also be
> a new bitstream for the FPGA.
>
> But even if the interrupt is brand new every time for the DT overlay,
> you are effectively relaxing the check for every user (such as
> XEN_DOMCTL_bind_pt_irq). So the interrupt re-use case needs to be
> taken into account.
I agree. I think IIUC, with your explanation here and below, could we
simplify the problem to how to properly handle the removal of the IRQ
from a running guest, if we always properly remove and clean up the
information when remove the IRQ from the guest? In this way, the IRQ can
always be viewed as a brand new one when we add it back. Then the only
corner case that we need to take care of would be...
>> Also since we only load the device driver after the IRQ is routed to
>> the guest,
>
> This is what a well-behave guest will do. However, we need to think
> what will happen if a guest misbehaves. I am not concerned about a
> guest only impacting itself, I am more concerned about the case where
> the rest of the system is impacted.
>
>> I am not sure the guest can enable the vIRQ before it is routed.
>
> Xen allows the guest to enable a vIRQ even if there is no pIRQ
> assigned. Thanksfully, it looks like the vgic_connect_hw_irq(), in
> both the current and new vGIC, will return an error if we are trying
> to route a pIRQ to an already enabled vIRQ.
>
> But we need to investigate all the possible scenarios to make sure
> that any inconsistencies between the physical state and virtual state
> (including the LRs) will not result to bigger problem.
>
> The one that comes to my mind is: The physical interrupt is
> de-assigned from the guest before it was EOIed. In this case, the
> interrupt will still be in the LR with the HW bit set. This would
> allow the guest to EOI the interrupt even if it is routed to someone
> else. It is unclear what would be the impact on the other guest.
...same as this case, i.e.
test_bit(_IRQ_INPROGRESS, &desc->status) || !test_bit(_IRQ_DISABLED,
&desc->status)) when we try to remove the IRQ from a running domain.
we have 3 possible states which can be read from LR for this case :
active, pending, pending and active.
- I don't think we can do anything about the active state, so we should
return -EBUSY and reject the whole operation of removing the IRQ from
running guest, and user can always retry this operation.
- For the pending (and active) case, can we clear the LR and point the
LR for the pending_irq to invalid?
Kind regards,
Henry
>
> Cheers,
>
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 05/15] tools/libs/light: Increase nr_spi to 160
2024-05-06 5:17 ` Henry Wang
@ 2024-05-07 14:35 ` Julien Grall
2024-05-08 0:46 ` Henry Wang
0 siblings, 1 reply; 58+ messages in thread
From: Julien Grall @ 2024-05-07 14:35 UTC (permalink / raw)
To: Henry Wang, Anthony PERARD, Stefano Stabellini, Michal Orzel,
Bertrand Marquis
Cc: xen-devel, Vikram Garhwal, Juergen Gross, Stefano Stabellini
Hi,
On 06/05/2024 06:17, Henry Wang wrote:
> On 5/1/2024 9:58 PM, Anthony PERARD wrote:
>> On Wed, Apr 24, 2024 at 11:34:39AM +0800, Henry Wang wrote:
>>> Increase number of spi to 160 i.e. gic_number_lines() for Xilinx
>>> ZynqMP - 32.
>>> This was done to allocate and assign IRQs to a running domain.
>>>
>>> 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>
>>> ---
>>> tools/libs/light/libxl_arm.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>>> index dd5c9f4917..50dbd0f2a9 100644
>>> --- a/tools/libs/light/libxl_arm.c
>>> +++ b/tools/libs/light/libxl_arm.c
>>> @@ -181,7 +181,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>>> LOG(DEBUG, "Configure the domain");
>>> - config->arch.nr_spis = nr_spis;
>>> + /* gic_number_lines() is 192 for Xilinx ZynqMP. min nr_spis =
>>> 192 - 32. */
>>> + config->arch.nr_spis = MAX(nr_spis, 160);
>> Is there a way that that Xen or libxl could find out what the minimum
>> number of SPI needs to be?
>
> I am afraid currently there is none.
>
>> Are we going to have to increase that minimum
>> number every time a new platform comes along?
>>
>> It doesn't appear that libxl is using that `nr_spis` value and it is
>> probably just given to Xen. So my guess is that Xen could simply take
>> care of the minimum value, gic_number_lines() seems to be a Xen
>> function.
>
> Xen will take care of the value of nr_spis for dom0 in create_dom0()
> dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) - 32;
> and also for dom0less domUs in create_domUs().
>
> However, it looks like Xen will not take care of the mininum value for
> libxl guests, the value from config->arch.nr_spis in guest config file
> will be directly passed to the domain_vgic_init() function from
> arch_domain_create().
>
> I agree with you that we shouldn't just bump the number everytime when
> we have a new platform. Therefore, would it be a good idea to move the
> logic in this patch to arch_sanitise_domain_config()?
Xen domains are supposed to be platform agnostics and therefore the
numbers of SPIs should not be based on the HW.
Furthermore, with your proposal we would end up to allocate data
structure for N SPIs when a domain may never needs any SPIs (such as if
passthrough is not in-use). This is more likely for domain created by
the toolstack than from Xen directly.
Instead, we should introduce a new XL configuration to let the user
decide the number of SPIs. I would suggest to name "nr_spis" to match
the DT bindings.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM
2024-05-06 8:32 ` Henry Wang
@ 2024-05-07 21:54 ` Julien Grall
2024-05-08 7:49 ` Henry Wang
0 siblings, 1 reply; 58+ messages in thread
From: Julien Grall @ 2024-05-07 21:54 UTC (permalink / raw)
To: Henry Wang, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Stefano Stabellini
Hi Henry,
On 06/05/2024 09:32, Henry Wang wrote:
> On 5/1/2024 4:13 AM, Julien Grall wrote:
>> Hi Henry,
>>
>> On 30/04/2024 04:50, Henry Wang wrote:
>>> On 4/25/2024 10:28 PM, Julien Grall wrote:
>>>>> Thanks for your feeedback. After checking the b8577547236f commit
>>>>> message I think I now understand your point. Do you have any
>>>>> suggestion about how can I properly add the support to route/remove
>>>>> the IRQ to running domains? Thanks.
>>>
>>> I spent some time going through the GIC/vGIC code and had some
>>> discussions with Stefano and Stewart during the last couple of days,
>>> let me see if I can describe the use case properly now to continue
>>> the discussion:
>>>
>>> We have some use cases that requires assigning devices to domains
>>> after domain boot time. For example, suppose there is an FPGA on the
>>> board which can simulate a device, and the bitstream for the FPGA is
>>> provided and programmed after domain boot. So we need a way to assign
>>> the device to the running domain. This series tries to implement this
>>> use case by using device tree overlay - users can firstly add the
>>> overlay to Xen dtb, assign the device in the overlay to a domain by
>>> the xl command, then apply the overlay to Linux.
>>
>> Thanks for the description! This helps to understand your goal :).
>
> Thank you very much for spending your time on discussing this and
> provide these valuable comments!
>
>>>
>>>> I haven't really look at that code in quite a while. I think we need
>>>> to make sure that the virtual and physical IRQ state matches at the
>>>> time we do the routing.
>>>>
>>>> I am undecided on whether we want to simply prevent the action to
>>>> happen or try to reset the state.
>>>>
>>>> There is also the question of what to do if the guest is enabling
>>>> the vIRQ before it is routed.
>>>
>>> Sorry for bothering, would you mind elaborating a bit more about the
>>> two cases that you mentioned above? Commit b8577547236f ("xen/arm:
>>> Restrict when a physical IRQ can be routed/removed from/to a domain")
>>> only said there will be undesirable effects, so I am not sure if I
>>> understand the concerns raised above and the consequences of these
>>> two use cases.
>>
>> I will try to explain them below after I answer the rest.
>>
>>> I am probably wrong, I think when we add the overlay, we are probably
>>> fine as the interrupt is not being used before.
>>
>> What if the DT overlay is unloaded and then reloaded? Wouldn't the
>> same interrupt be re-used? As a more generic case, this could also be
>> a new bitstream for the FPGA.
>>
>> But even if the interrupt is brand new every time for the DT overlay,
>> you are effectively relaxing the check for every user (such as
>> XEN_DOMCTL_bind_pt_irq). So the interrupt re-use case needs to be
>> taken into account.
>
> I agree. I think IIUC, with your explanation here and below, could we
> simplify the problem to how to properly handle the removal of the IRQ
> from a running guest, if we always properly remove and clean up the
> information when remove the IRQ from the guest? In this way, the IRQ can
> always be viewed as a brand new one when we add it back.
If we can make sure the virtual IRQ and physical IRQ is cleaned then yes.
> Then the only
> corner case that we need to take care of would be...
Can you clarify whether you say the "only corner case" because you
looked at the code? Or is it just because I mentioned only one?
>
>>> Also since we only load the device driver after the IRQ is routed to
>>> the guest,
>>
>> This is what a well-behave guest will do. However, we need to think
>> what will happen if a guest misbehaves. I am not concerned about a
>> guest only impacting itself, I am more concerned about the case where
>> the rest of the system is impacted.
>>
>>> I am not sure the guest can enable the vIRQ before it is routed.
>>
>> Xen allows the guest to enable a vIRQ even if there is no pIRQ
>> assigned. Thanksfully, it looks like the vgic_connect_hw_irq(), in
>> both the current and new vGIC, will return an error if we are trying
>> to route a pIRQ to an already enabled vIRQ.
>>
>> But we need to investigate all the possible scenarios to make sure
>> that any inconsistencies between the physical state and virtual state
>> (including the LRs) will not result to bigger problem.
>>
>> The one that comes to my mind is: The physical interrupt is
>> de-assigned from the guest before it was EOIed. In this case, the
>> interrupt will still be in the LR with the HW bit set. This would
>> allow the guest to EOI the interrupt even if it is routed to someone
>> else. It is unclear what would be the impact on the other guest.
>
> ...same as this case, i.e.
> test_bit(_IRQ_INPROGRESS, &desc->status) || !test_bit(_IRQ_DISABLED,
> &desc->status)) when we try to remove the IRQ from a running domain.
We already call ->shutdown() which will disable the IRQ. So don't we
only need to take care of _IRQ_INPROGRESS?
[...]
> we have 3 possible states which can be read from LR for this case :
> active, pending, pending and active.
> - I don't think we can do anything about the active state, so we should
> return -EBUSY and reject the whole operation of removing the IRQ from
> running guest, and user can always retry this operation.
This would mean a malicious/buggy guest would be able to prevent a
device to be de-assigned. This is not a good idea in particular when the
domain is dying.
That said, I think you can handle this case. The LR has a bit to
indicate whether the pIRQ needs to be EOIed. You can clear it and this
would prevent the guest to touch the pIRQ. There might be other clean-up
to do in the vGIC datastructure.
Anyway, we don't have to handle removing an active IRQ when the domain
is still running (although we do when the domain is destroying). But I
think this would need to be solved before the feature is (security)
supported.
> - For the pending (and active) case,
Shouldn't the pending and active case handled the same way as the active
case?
> can we clear the LR and point the
> LR for the pending_irq to invalid?
LRs can be cleared. You will need to find which vCPU was used for the
injection and then pause it so the LR can be safely updated.
There will also be some private state to clear. I don't know how easy it
will be. However, we decided to not do anything for ICPENDR (which
requires a similar behavior) as this was complex (?) to do with the
existing vGIC.
I vaguely remember we had some discussions on the ML. I didn't look for
them though.
Anyway, same as above, this could possibly handled later on. But this
would probably need to be solved before the feature is (security supported).
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 05/15] tools/libs/light: Increase nr_spi to 160
2024-05-07 14:35 ` Julien Grall
@ 2024-05-08 0:46 ` Henry Wang
0 siblings, 0 replies; 58+ messages in thread
From: Henry Wang @ 2024-05-08 0:46 UTC (permalink / raw)
To: Julien Grall, Anthony PERARD, Stefano Stabellini, Michal Orzel,
Bertrand Marquis
Cc: xen-devel, Vikram Garhwal, Juergen Gross, Stefano Stabellini
Hi Julien,
On 5/7/2024 10:35 PM, Julien Grall wrote:
> Hi,
>
> On 06/05/2024 06:17, Henry Wang wrote:
>> On 5/1/2024 9:58 PM, Anthony PERARD wrote:
>>> On Wed, Apr 24, 2024 at 11:34:39AM +0800, Henry Wang wrote:
>>>> Increase number of spi to 160 i.e. gic_number_lines() for Xilinx
>>>> ZynqMP - 32.
>>>> This was done to allocate and assign IRQs to a running domain.
>>>>
>>>> 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>
>>>> ---
>>>> tools/libs/light/libxl_arm.c | 3 ++-
>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/libs/light/libxl_arm.c
>>>> b/tools/libs/light/libxl_arm.c
>>>> index dd5c9f4917..50dbd0f2a9 100644
>>>> --- a/tools/libs/light/libxl_arm.c
>>>> +++ b/tools/libs/light/libxl_arm.c
>>>> @@ -181,7 +181,8 @@ int libxl__arch_domain_prepare_config(libxl__gc
>>>> *gc,
>>>> LOG(DEBUG, "Configure the domain");
>>>> - config->arch.nr_spis = nr_spis;
>>>> + /* gic_number_lines() is 192 for Xilinx ZynqMP. min nr_spis =
>>>> 192 - 32. */
>>>> + config->arch.nr_spis = MAX(nr_spis, 160);
>>> Is there a way that that Xen or libxl could find out what the minimum
>>> number of SPI needs to be?
>>
>> I am afraid currently there is none.
>>
>>> Are we going to have to increase that minimum
>>> number every time a new platform comes along?
>>>
>>> It doesn't appear that libxl is using that `nr_spis` value and it is
>>> probably just given to Xen. So my guess is that Xen could simply take
>>> care of the minimum value, gic_number_lines() seems to be a Xen
>>> function.
>>
>> Xen will take care of the value of nr_spis for dom0 in create_dom0()
>> dom0_cfg.arch.nr_spis = min(gic_number_lines(), (unsigned int) 992) -
>> 32;
>> and also for dom0less domUs in create_domUs().
>>
>> However, it looks like Xen will not take care of the mininum value
>> for libxl guests, the value from config->arch.nr_spis in guest config
>> file will be directly passed to the domain_vgic_init() function from
>> arch_domain_create().
>>
>> I agree with you that we shouldn't just bump the number everytime
>> when we have a new platform. Therefore, would it be a good idea to
>> move the logic in this patch to arch_sanitise_domain_config()?
>
> Xen domains are supposed to be platform agnostics and therefore the
> numbers of SPIs should not be based on the HW.
>
> Furthermore, with your proposal we would end up to allocate data
> structure for N SPIs when a domain may never needs any SPIs (such as
> if passthrough is not in-use). This is more likely for domain created
> by the toolstack than from Xen directly.
Agreed on both comments.
> Instead, we should introduce a new XL configuration to let the user
> decide the number of SPIs. I would suggest to name "nr_spis" to match
> the DT bindings.
Sure, I will introduce a new xl config for this to replace this patch.
Thank you for the suggestion.
Kind regards,
Henry
>
> Cheers,
>
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM
2024-05-07 21:54 ` Julien Grall
@ 2024-05-08 7:49 ` Henry Wang
0 siblings, 0 replies; 58+ messages in thread
From: Henry Wang @ 2024-05-08 7:49 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Stefano Stabellini
Hi Julien,
On 5/8/2024 5:54 AM, Julien Grall wrote:
> Hi Henry,
>>> What if the DT overlay is unloaded and then reloaded? Wouldn't the
>>> same interrupt be re-used? As a more generic case, this could also
>>> be a new bitstream for the FPGA.
>>>
>>> But even if the interrupt is brand new every time for the DT
>>> overlay, you are effectively relaxing the check for every user (such
>>> as XEN_DOMCTL_bind_pt_irq). So the interrupt re-use case needs to be
>>> taken into account.
>>
>> I agree. I think IIUC, with your explanation here and below, could we
>> simplify the problem to how to properly handle the removal of the IRQ
>> from a running guest, if we always properly remove and clean up the
>> information when remove the IRQ from the guest? In this way, the IRQ
>> can always be viewed as a brand new one when we add it back.
>
> If we can make sure the virtual IRQ and physical IRQ is cleaned then yes.
>
>> Then the only corner case that we need to take care of would be...
>
> Can you clarify whether you say the "only corner case" because you
> looked at the code? Or is it just because I mentioned only one?
Well, I indeed checked the code and to my best knowledge the corner case
that you pointed out would be the only one I can think of.
>>> Xen allows the guest to enable a vIRQ even if there is no pIRQ
>>> assigned. Thanksfully, it looks like the vgic_connect_hw_irq(), in
>>> both the current and new vGIC, will return an error if we are trying
>>> to route a pIRQ to an already enabled vIRQ.
>>>
>>> But we need to investigate all the possible scenarios to make sure
>>> that any inconsistencies between the physical state and virtual
>>> state (including the LRs) will not result to bigger problem.
>>>
>>> The one that comes to my mind is: The physical interrupt is
>>> de-assigned from the guest before it was EOIed. In this case, the
>>> interrupt will still be in the LR with the HW bit set. This would
>>> allow the guest to EOI the interrupt even if it is routed to someone
>>> else. It is unclear what would be the impact on the other guest.
>>
>> ...same as this case, i.e.
>> test_bit(_IRQ_INPROGRESS, &desc->status) || !test_bit(_IRQ_DISABLED,
>> &desc->status)) when we try to remove the IRQ from a running domain.
>
> We already call ->shutdown() which will disable the IRQ. So don't we
> only need to take care of _IRQ_INPROGRESS?
Yes you are correct.
>> we have 3 possible states which can be read from LR for this case :
>> active, pending, pending and active.
>> - I don't think we can do anything about the active state, so we
>> should return -EBUSY and reject the whole operation of removing the
>> IRQ from running guest, and user can always retry this operation.
>
> This would mean a malicious/buggy guest would be able to prevent a
> device to be de-assigned. This is not a good idea in particular when
> the domain is dying.
>
> That said, I think you can handle this case. The LR has a bit to
> indicate whether the pIRQ needs to be EOIed. You can clear it and this
> would prevent the guest to touch the pIRQ. There might be other
> clean-up to do in the vGIC datastructure.
I probably misunderstood this sentence, do you mean the EOI bit in the
pINTID field? I think this bit is only available when the HW bit of LR
is 0, but in our case the HW is supposed to be 1 (as indicated as your
previous comment). Would you mind clarifying a bit more? Thanks!
> Anyway, we don't have to handle removing an active IRQ when the domain
> is still running (although we do when the domain is destroying). But I
> think this would need to be solved before the feature is (security)
> supported.
>
>> - For the pending (and active) case,
>
> Shouldn't the pending and active case handled the same way as the
> active case?
Sorry, yes you are correct.
Kind regards,
Henry
^ permalink raw reply [flat|nested] 58+ messages in thread
end of thread, other threads:[~2024-05-08 7:50 UTC | newest]
Thread overview: 58+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-24 3:34 [PATCH 00/15] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
2024-04-24 3:34 ` [PATCH 01/15] xen/commom/dt-overlay: Fix missing lock when remove the device Henry Wang
2024-04-24 5:58 ` Jan Beulich
2024-04-24 6:02 ` Henry Wang
2024-04-24 3:34 ` [PATCH 02/15] xen/arm/gic: Enable interrupt assignment to running VM Henry Wang
2024-04-24 12:58 ` Julien Grall
2024-04-25 7:06 ` Henry Wang
2024-04-25 14:28 ` Julien Grall
2024-04-30 3:50 ` Henry Wang
2024-04-30 20:13 ` Julien Grall
2024-05-06 8:32 ` Henry Wang
2024-05-07 21:54 ` Julien Grall
2024-05-08 7:49 ` Henry Wang
2024-04-24 3:34 ` [PATCH 03/15] xen/arm: Always enable IOMMU Henry Wang
2024-04-24 13:03 ` Julien Grall
2024-04-25 1:02 ` Henry Wang
2024-04-24 3:34 ` [PATCH 04/15] tools/libs/light: " Henry Wang
2024-05-01 13:47 ` Anthony PERARD
2024-05-06 3:17 ` Henry Wang
2024-04-24 3:34 ` [PATCH 05/15] tools/libs/light: Increase nr_spi to 160 Henry Wang
2024-05-01 13:58 ` Anthony PERARD
2024-05-06 5:17 ` Henry Wang
2024-05-07 14:35 ` Julien Grall
2024-05-08 0:46 ` Henry Wang
2024-04-24 3:34 ` [PATCH 06/15] rangeset: Move struct range and struct rangeset to headerfile Henry Wang
2024-04-24 6:22 ` Jan Beulich
2024-04-25 0:47 ` Henry Wang
2024-04-24 3:34 ` [PATCH 07/15] xen/overlay: Enable device tree overlay assignment to running domains Henry Wang
2024-04-24 6:05 ` Jan Beulich
2024-04-29 3:36 ` Henry Wang
2024-04-29 6:43 ` Jan Beulich
2024-04-29 17:34 ` Julien Grall
2024-04-30 4:00 ` Henry Wang
2024-04-30 9:47 ` Julien Grall
2024-05-06 5:26 ` Henry Wang
2024-05-02 18:02 ` Stefano Stabellini
2024-05-06 3:14 ` Henry Wang
2024-04-24 3:34 ` [PATCH 08/15] tools: Add domain_id and expert mode for overlay operations Henry Wang
2024-05-01 14:46 ` Anthony PERARD
2024-05-06 5:51 ` Henry Wang
2024-04-24 3:34 ` [PATCH 09/15] tools/libs/light: Modify dtbo to domU linux dtbo format Henry Wang
2024-05-01 15:09 ` Anthony PERARD
2024-05-06 5:40 ` Henry Wang
2024-04-24 3:34 ` [PATCH 10/15] tools/xl: Share overlay with domU Henry Wang
2024-04-24 3:34 ` [PATCH 11/15] tools/helpers: Add get_overlay Henry Wang
2024-04-24 6:08 ` Jan Beulich
2024-04-25 0:43 ` Henry Wang
2024-04-26 1:45 ` Stewart Hildebrand
2024-04-26 1:48 ` Henry Wang
2024-04-24 3:34 ` [PATCH 12/15] get_overlay: remove domU overlay Henry Wang
2024-04-24 3:34 ` [PATCH 13/15] xl/overlay: add remove operation to xenstore Henry Wang
2024-04-24 3:34 ` [PATCH 14/15] add a domU script to fetch overlays and applying them to linux Henry Wang
2024-04-24 6:16 ` Jan Beulich
2024-04-25 0:54 ` Henry Wang
2024-04-25 6:46 ` Jan Beulich
2024-04-25 7:06 ` Henry Wang
2024-04-24 3:34 ` [PATCH 15/15] docs: add device tree overlay documentation Henry Wang
2024-04-24 6:29 ` [PATCH 00/15] Remaining patches for dynamic node programming using overlay dtbo Jan Beulich
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 NNTP newsgroup(s).