* [PATCH v4 1/9] tools/xl: Correct the help information and exit code of the dt-overlay command
2024-05-23 7:40 [PATCH v4 0/9] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
@ 2024-05-23 7:40 ` Henry Wang
2024-05-24 0:55 ` Stefano Stabellini
2024-05-23 7:40 ` [PATCH v4 2/9] xen/arm, doc: Add a DT property to specify IOMMU for Dom0less domUs Henry Wang
` (8 subsequent siblings)
9 siblings, 1 reply; 23+ messages in thread
From: Henry Wang @ 2024-05-23 7:40 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Henry Wang, Jason Andryuk
Fix the name mismatch in the xl dt-overlay command, the
command name should be "dt-overlay" instead of "dt_overlay".
Add the missing "," in the cmdtable.
Fix the exit code of the dt-overlay command, use EXIT_FAILURE
instead of ERROR_FAIL.
Fixes: 61765a07e3d8 ("tools/xl: Add new xl command overlay for device tree overlay support")
Suggested-by: Anthony PERARD <anthony@xenproject.org>
Signed-off-by: Henry Wang <xin.wang2@amd.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
---
v4:
- No change.
v3:
- Add Jason's Reviewed-by tag.
v2:
- New patch
---
tools/xl/xl_cmdtable.c | 2 +-
tools/xl/xl_vmcontrol.c | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
index 62bdb2aeaa..1f3c6b5897 100644
--- a/tools/xl/xl_cmdtable.c
+++ b/tools/xl/xl_cmdtable.c
@@ -635,7 +635,7 @@ const struct cmd_spec cmd_table[] = {
{ "dt-overlay",
&main_dt_overlay, 0, 1,
"Add/Remove a device tree overlay",
- "add/remove <.dtbo>"
+ "add/remove <.dtbo>",
"-h print this help\n"
},
#endif
diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index 98f6bd2e76..02575d5d36 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -1278,7 +1278,7 @@ int main_dt_overlay(int argc, char **argv)
const int overlay_remove_op = 2;
if (argc < 2) {
- help("dt_overlay");
+ help("dt-overlay");
return EXIT_FAILURE;
}
@@ -1302,11 +1302,11 @@ int main_dt_overlay(int argc, char **argv)
fprintf(stderr, "failed to read the overlay device tree file %s\n",
overlay_config_file);
free(overlay_dtb);
- return ERROR_FAIL;
+ return EXIT_FAILURE;
}
} else {
fprintf(stderr, "overlay dtbo file not provided\n");
- return ERROR_FAIL;
+ return EXIT_FAILURE;
}
rc = libxl_dt_overlay(ctx, overlay_dtb, overlay_dtb_size, op);
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v4 1/9] tools/xl: Correct the help information and exit code of the dt-overlay command
2024-05-23 7:40 ` [PATCH v4 1/9] tools/xl: Correct the help information and exit code of the dt-overlay command Henry Wang
@ 2024-05-24 0:55 ` Stefano Stabellini
0 siblings, 0 replies; 23+ messages in thread
From: Stefano Stabellini @ 2024-05-24 0:55 UTC (permalink / raw)
To: Henry Wang; +Cc: xen-devel, Anthony PERARD, Jason Andryuk
On Thu, 23 May 2024, Henry Wang wrote:
> Fix the name mismatch in the xl dt-overlay command, the
> command name should be "dt-overlay" instead of "dt_overlay".
> Add the missing "," in the cmdtable.
>
> Fix the exit code of the dt-overlay command, use EXIT_FAILURE
> instead of ERROR_FAIL.
>
> Fixes: 61765a07e3d8 ("tools/xl: Add new xl command overlay for device tree overlay support")
> Suggested-by: Anthony PERARD <anthony@xenproject.org>
> Signed-off-by: Henry Wang <xin.wang2@amd.com>
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> v4:
> - No change.
> v3:
> - Add Jason's Reviewed-by tag.
> v2:
> - New patch
> ---
> tools/xl/xl_cmdtable.c | 2 +-
> tools/xl/xl_vmcontrol.c | 6 +++---
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
> index 62bdb2aeaa..1f3c6b5897 100644
> --- a/tools/xl/xl_cmdtable.c
> +++ b/tools/xl/xl_cmdtable.c
> @@ -635,7 +635,7 @@ const struct cmd_spec cmd_table[] = {
> { "dt-overlay",
> &main_dt_overlay, 0, 1,
> "Add/Remove a device tree overlay",
> - "add/remove <.dtbo>"
> + "add/remove <.dtbo>",
> "-h print this help\n"
> },
> #endif
> diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
> index 98f6bd2e76..02575d5d36 100644
> --- a/tools/xl/xl_vmcontrol.c
> +++ b/tools/xl/xl_vmcontrol.c
> @@ -1278,7 +1278,7 @@ int main_dt_overlay(int argc, char **argv)
> const int overlay_remove_op = 2;
>
> if (argc < 2) {
> - help("dt_overlay");
> + help("dt-overlay");
> return EXIT_FAILURE;
> }
>
> @@ -1302,11 +1302,11 @@ int main_dt_overlay(int argc, char **argv)
> fprintf(stderr, "failed to read the overlay device tree file %s\n",
> overlay_config_file);
> free(overlay_dtb);
> - return ERROR_FAIL;
> + return EXIT_FAILURE;
> }
> } else {
> fprintf(stderr, "overlay dtbo file not provided\n");
> - return ERROR_FAIL;
> + return EXIT_FAILURE;
> }
>
> rc = libxl_dt_overlay(ctx, overlay_dtb, overlay_dtb_size, op);
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v4 2/9] xen/arm, doc: Add a DT property to specify IOMMU for Dom0less domUs
2024-05-23 7:40 [PATCH v4 0/9] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
2024-05-23 7:40 ` [PATCH v4 1/9] tools/xl: Correct the help information and exit code of the dt-overlay command Henry Wang
@ 2024-05-23 7:40 ` Henry Wang
2024-05-23 21:23 ` Julien Grall
2024-05-23 7:40 ` [PATCH v4 3/9] tools/arm: Introduce the "nr_spis" xl config entry Henry Wang
` (7 subsequent siblings)
9 siblings, 1 reply; 23+ messages in thread
From: Henry Wang @ 2024-05-23 7:40 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Henry Wang
There are some use cases in which the dom0less domUs need to have
the XEN_DOMCTL_CDF_iommu set at the domain construction time. For
example, the dynamic dtbo feature allows the domain to be assigned
a device that is behind the IOMMU at runtime. For these use cases,
we need to have a way to specify the domain will need the IOMMU
mapping at domain construction time.
Introduce a "passthrough" DT property for Dom0less DomUs following
the same entry as the xl.cfg. Currently only provide two options,
i.e. "enable" and "disable". Set the XEN_DOMCTL_CDF_iommu at domain
construction time based on the property.
Signed-off-by: Henry Wang <xin.wang2@amd.com>
---
v4:
- No change.
v3:
- Use a separate variable to cache the condition from the "passthrough"
flag separately to improve readability.
- Update the doc to explain the default condition more clearly.
v2:
- New patch to replace the original patch in v1:
"[PATCH 03/15] xen/arm: Always enable IOMMU"
---
docs/misc/arm/device-tree/booting.txt | 16 ++++++++++++++++
xen/arch/arm/dom0less-build.c | 11 +++++++++--
2 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index bbd955e9c2..f1fd069c87 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -260,6 +260,22 @@ with the following properties:
value specified by Xen command line parameter gnttab_max_maptrack_frames
(or its default value if unspecified, i.e. 1024) is used.
+- passthrough
+
+ A string property specifying whether IOMMU mappings are enabled for the
+ domain and hence whether it will be enabled for passthrough hardware.
+ Possible property values are:
+
+ - "enabled"
+ IOMMU mappings are enabled for the domain. Note that this option is the
+ default if the user provides the device partial passthrough device tree
+ for the domain.
+
+ - "disabled"
+ IOMMU mappings are disabled for the domain and so hardware may not be
+ passed through. This option is the default if this property is missing
+ and the user does not provide the device partial device tree for the domain.
+
Under the "xen,domain" compatible node, one or more sub-nodes are present
for the DomU kernel and ramdisk.
diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
index 74f053c242..5830a7051d 100644
--- a/xen/arch/arm/dom0less-build.c
+++ b/xen/arch/arm/dom0less-build.c
@@ -848,6 +848,8 @@ static int __init construct_domU(struct domain *d,
void __init create_domUs(void)
{
struct dt_device_node *node;
+ const char *dom0less_iommu;
+ bool iommu = false;
const struct dt_device_node *cpupool_node,
*chosen = dt_find_node_by_path("/chosen");
@@ -895,8 +897,13 @@ void __init create_domUs(void)
panic("Missing property 'cpus' for domain %s\n",
dt_node_name(node));
- if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") &&
- iommu_enabled )
+ if ( !dt_property_read_string(node, "passthrough", &dom0less_iommu) &&
+ !strcmp(dom0less_iommu, "enabled") )
+ iommu = true;
+
+ if ( iommu_enabled &&
+ (iommu || dt_find_compatible_node(node, NULL,
+ "multiboot,device-tree")) )
d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
if ( !dt_property_read_u32(node, "nr_spis", &d_cfg.arch.nr_spis) )
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v4 2/9] xen/arm, doc: Add a DT property to specify IOMMU for Dom0less domUs
2024-05-23 7:40 ` [PATCH v4 2/9] xen/arm, doc: Add a DT property to specify IOMMU for Dom0less domUs Henry Wang
@ 2024-05-23 21:23 ` Julien Grall
0 siblings, 0 replies; 23+ messages in thread
From: Julien Grall @ 2024-05-23 21:23 UTC (permalink / raw)
To: Henry Wang, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
Hi Henry,
On 23/05/2024 08:40, Henry Wang wrote:
> There are some use cases in which the dom0less domUs need to have
> the XEN_DOMCTL_CDF_iommu set at the domain construction time. For
> example, the dynamic dtbo feature allows the domain to be assigned
> a device that is behind the IOMMU at runtime. For these use cases,
> we need to have a way to specify the domain will need the IOMMU
> mapping at domain construction time.
>
> Introduce a "passthrough" DT property for Dom0less DomUs following
> the same entry as the xl.cfg. Currently only provide two options,
> i.e. "enable" and "disable". Set the XEN_DOMCTL_CDF_iommu at domain
> construction time based on the property.
>
> Signed-off-by: Henry Wang <xin.wang2@amd.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v4 3/9] tools/arm: Introduce the "nr_spis" xl config entry
2024-05-23 7:40 [PATCH v4 0/9] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
2024-05-23 7:40 ` [PATCH v4 1/9] tools/xl: Correct the help information and exit code of the dt-overlay command Henry Wang
2024-05-23 7:40 ` [PATCH v4 2/9] xen/arm, doc: Add a DT property to specify IOMMU for Dom0less domUs Henry Wang
@ 2024-05-23 7:40 ` Henry Wang
2024-05-23 23:05 ` Julien Grall
2024-05-23 7:40 ` [PATCH v4 4/9] xen/arm/gic: Allow adding interrupt to running VMs Henry Wang
` (6 subsequent siblings)
9 siblings, 1 reply; 23+ messages in thread
From: Henry Wang @ 2024-05-23 7:40 UTC (permalink / raw)
To: xen-devel
Cc: Anthony PERARD, George Dunlap, Nick Rosbrook, Juergen Gross,
Henry Wang, Jason Andryuk
Currently, the number of SPIs allocated to the domain is only
configurable for Dom0less DomUs. Xen domains are supposed to be
platform agnostics and therefore the numbers of SPIs for libxl
guests should not be based on the hardware.
Introduce a new xl config entry for Arm to provide a method for
user to decide the number of SPIs. This would help to avoid
bumping the `config->arch.nr_spis` in libxl everytime there is a
new platform with increased SPI numbers.
Update the doc and the golang bindings accordingly.
Signed-off-by: Henry Wang <xin.wang2@amd.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
---
v4:
- Add Jason's Reviewed-by tag.
v3:
- Reword documentation to avoid ambiguity.
v2:
- New patch to replace the original patch in v1:
"[PATCH 05/15] tools/libs/light: Increase nr_spi to 160"
---
docs/man/xl.cfg.5.pod.in | 14 ++++++++++++++
tools/golang/xenlight/helpers.gen.go | 2 ++
tools/golang/xenlight/types.gen.go | 1 +
tools/libs/light/libxl_arm.c | 4 ++--
tools/libs/light/libxl_types.idl | 1 +
tools/xl/xl_parse.c | 3 +++
6 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 8f2b375ce9..416d582844 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -3072,6 +3072,20 @@ raised.
=back
+=over 4
+
+=item B<nr_spis="NR_SPIS">
+
+An optional 32-bit integer parameter specifying the number of SPIs (Shared
+Peripheral Interrupts) to allocate for the domain. If the value specified by
+the `nr_spis` parameter is smaller than the number of SPIs calculated by the
+toolstack based on the devices allocated for the domain, or the `nr_spis`
+parameter is not specified, the value calculated by the toolstack will be used
+for the domain. Otherwise, the value specified by the `nr_spis` parameter will
+be used.
+
+=back
+
=head3 x86
=over 4
diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
index b9cb5b33c7..fe5110474d 100644
--- a/tools/golang/xenlight/helpers.gen.go
+++ b/tools/golang/xenlight/helpers.gen.go
@@ -1154,6 +1154,7 @@ return fmt.Errorf("invalid union key '%v'", x.Type)}
x.ArchArm.GicVersion = GicVersion(xc.arch_arm.gic_version)
x.ArchArm.Vuart = VuartType(xc.arch_arm.vuart)
x.ArchArm.SveVl = SveType(xc.arch_arm.sve_vl)
+x.ArchArm.NrSpis = uint32(xc.arch_arm.nr_spis)
if err := x.ArchX86.MsrRelaxed.fromC(&xc.arch_x86.msr_relaxed);err != nil {
return fmt.Errorf("converting field ArchX86.MsrRelaxed: %v", err)
}
@@ -1670,6 +1671,7 @@ return fmt.Errorf("invalid union key '%v'", x.Type)}
xc.arch_arm.gic_version = C.libxl_gic_version(x.ArchArm.GicVersion)
xc.arch_arm.vuart = C.libxl_vuart_type(x.ArchArm.Vuart)
xc.arch_arm.sve_vl = C.libxl_sve_type(x.ArchArm.SveVl)
+xc.arch_arm.nr_spis = C.uint32_t(x.ArchArm.NrSpis)
if err := x.ArchX86.MsrRelaxed.toC(&xc.arch_x86.msr_relaxed); err != nil {
return fmt.Errorf("converting field ArchX86.MsrRelaxed: %v", err)
}
diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go
index 5b293755d7..c9e45b306f 100644
--- a/tools/golang/xenlight/types.gen.go
+++ b/tools/golang/xenlight/types.gen.go
@@ -597,6 +597,7 @@ ArchArm struct {
GicVersion GicVersion
Vuart VuartType
SveVl SveType
+NrSpis uint32
}
ArchX86 struct {
MsrRelaxed Defbool
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index 1cb89fa584..a4029e3ac8 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -181,8 +181,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
LOG(DEBUG, "Configure the domain");
- config->arch.nr_spis = nr_spis;
- LOG(DEBUG, " - Allocate %u SPIs", nr_spis);
+ config->arch.nr_spis = max(nr_spis, d_config->b_info.arch_arm.nr_spis);
+ LOG(DEBUG, " - Allocate %u SPIs", config->arch.nr_spis);
switch (d_config->b_info.arch_arm.gic_version) {
case LIBXL_GIC_VERSION_DEFAULT:
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 79e9c656cc..4e65e6fda5 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -722,6 +722,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
("vuart", libxl_vuart_type),
("sve_vl", libxl_sve_type),
+ ("nr_spis", uint32),
])),
("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
])),
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index c504ab3711..e3a4800f6e 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -2935,6 +2935,9 @@ skip_usbdev:
}
}
+ if (!xlu_cfg_get_long (config, "nr_spis", &l, 0))
+ b_info->arch_arm.nr_spis = l;
+
parse_vkb_list(config, d_config);
d_config->virtios = NULL;
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v4 3/9] tools/arm: Introduce the "nr_spis" xl config entry
2024-05-23 7:40 ` [PATCH v4 3/9] tools/arm: Introduce the "nr_spis" xl config entry Henry Wang
@ 2024-05-23 23:05 ` Julien Grall
2024-05-24 2:19 ` Stefano Stabellini
0 siblings, 1 reply; 23+ messages in thread
From: Julien Grall @ 2024-05-23 23:05 UTC (permalink / raw)
To: Henry Wang, xen-devel
Cc: Anthony PERARD, George Dunlap, Nick Rosbrook, Juergen Gross,
Jason Andryuk
Hi Henry,
On 23/05/2024 08:40, Henry Wang wrote:
> Currently, the number of SPIs allocated to the domain is only
> configurable for Dom0less DomUs. Xen domains are supposed to be
> platform agnostics and therefore the numbers of SPIs for libxl
> guests should not be based on the hardware.
>
> Introduce a new xl config entry for Arm to provide a method for
> user to decide the number of SPIs. This would help to avoid
> bumping the `config->arch.nr_spis` in libxl everytime there is a
> new platform with increased SPI numbers.
>
> Update the doc and the golang bindings accordingly.
>
> Signed-off-by: Henry Wang <xin.wang2@amd.com>
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
> ---
> v4:
> - Add Jason's Reviewed-by tag.
> v3:
> - Reword documentation to avoid ambiguity.
> v2:
> - New patch to replace the original patch in v1:
> "[PATCH 05/15] tools/libs/light: Increase nr_spi to 160"
> ---
> docs/man/xl.cfg.5.pod.in | 14 ++++++++++++++
> tools/golang/xenlight/helpers.gen.go | 2 ++
> tools/golang/xenlight/types.gen.go | 1 +
> tools/libs/light/libxl_arm.c | 4 ++--
> tools/libs/light/libxl_types.idl | 1 +
> tools/xl/xl_parse.c | 3 +++
> 6 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index 8f2b375ce9..416d582844 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -3072,6 +3072,20 @@ raised.
>
> =back
>
> +=over 4
> +
> +=item B<nr_spis="NR_SPIS">
> +
> +An optional 32-bit integer parameter specifying the number of SPIs (Shared
We can't support that much SPIs :). The limit would be 991 SPIs.
> +Peripheral Interrupts) to allocate for the domain. If the value specified by
> +the `nr_spis` parameter is smaller than the number of SPIs calculated by the
> +toolstack based on the devices allocated for the domain, or the `nr_spis`
> +parameter is not specified, the value calculated by the toolstack will be used
> +for the domain. Otherwise, the value specified by the `nr_spis` parameter will
> +be used.
I think it would be worth mentioning that the number of SPIs should
match the highest interrupt ID that will be assigned to the domain
(rather than the number of SPIs planned to be assigned).
> +
> +=back
> +
> =head3 x86
>
> =over 4
> diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
> index b9cb5b33c7..fe5110474d 100644
> --- a/tools/golang/xenlight/helpers.gen.go
> +++ b/tools/golang/xenlight/helpers.gen.go
> @@ -1154,6 +1154,7 @@ return fmt.Errorf("invalid union key '%v'", x.Type)}
> x.ArchArm.GicVersion = GicVersion(xc.arch_arm.gic_version)
> x.ArchArm.Vuart = VuartType(xc.arch_arm.vuart)
> x.ArchArm.SveVl = SveType(xc.arch_arm.sve_vl)
> +x.ArchArm.NrSpis = uint32(xc.arch_arm.nr_spis)
> if err := x.ArchX86.MsrRelaxed.fromC(&xc.arch_x86.msr_relaxed);err != nil {
> return fmt.Errorf("converting field ArchX86.MsrRelaxed: %v", err)
> }
> @@ -1670,6 +1671,7 @@ return fmt.Errorf("invalid union key '%v'", x.Type)}
> xc.arch_arm.gic_version = C.libxl_gic_version(x.ArchArm.GicVersion)
> xc.arch_arm.vuart = C.libxl_vuart_type(x.ArchArm.Vuart)
> xc.arch_arm.sve_vl = C.libxl_sve_type(x.ArchArm.SveVl)
> +xc.arch_arm.nr_spis = C.uint32_t(x.ArchArm.NrSpis)
> if err := x.ArchX86.MsrRelaxed.toC(&xc.arch_x86.msr_relaxed); err != nil {
> return fmt.Errorf("converting field ArchX86.MsrRelaxed: %v", err)
> }
> diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go
> index 5b293755d7..c9e45b306f 100644
> --- a/tools/golang/xenlight/types.gen.go
> +++ b/tools/golang/xenlight/types.gen.go
> @@ -597,6 +597,7 @@ ArchArm struct {
> GicVersion GicVersion
> Vuart VuartType
> SveVl SveType
> +NrSpis uint32
> }
> ArchX86 struct {
> MsrRelaxed Defbool
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 1cb89fa584..a4029e3ac8 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -181,8 +181,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>
> LOG(DEBUG, "Configure the domain");
>
> - config->arch.nr_spis = nr_spis;
> - LOG(DEBUG, " - Allocate %u SPIs", nr_spis);
> + config->arch.nr_spis = max(nr_spis, d_config->b_info.arch_arm.nr_spis);
I am not entirely sure about using max(). To me if the user specifies a
lower limit, then we should throw an error because this is likely an
indication that the SPIs they will want to assign will clash with the
emulated ones.
So it would be better to warn at domain creation rather than waiting
until the IRQs are assigned.
I would like Anthony's opinion on this one. Given he is away this month,
I guess we could get this patch merged (with other comments addressed)
and have a follow-up if wanted before 4.19.
> + LOG(DEBUG, " - Allocate %u SPIs", config->arch.nr_spis);
>
> switch (d_config->b_info.arch_arm.gic_version) {
> case LIBXL_GIC_VERSION_DEFAULT:
> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> index 79e9c656cc..4e65e6fda5 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -722,6 +722,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
> ("vuart", libxl_vuart_type),
> ("sve_vl", libxl_sve_type),
> + ("nr_spis", uint32),
From my understandig, any change in the .idl requires a corresponding
LIBXL_HAVE_... in include/libxl.h. This is in order to allow external
toolstack (such as libvirt) to be able to select at build time between
multiple version of libxl.
> ])),
> ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
> ])),
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index c504ab3711..e3a4800f6e 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -2935,6 +2935,9 @@ skip_usbdev:
> }
> }
>
> + if (!xlu_cfg_get_long (config, "nr_spis", &l, 0))
> + b_info->arch_arm.nr_spis = l;
> +
> parse_vkb_list(config, d_config);
>
> d_config->virtios = NULL;
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v4 3/9] tools/arm: Introduce the "nr_spis" xl config entry
2024-05-23 23:05 ` Julien Grall
@ 2024-05-24 2:19 ` Stefano Stabellini
0 siblings, 0 replies; 23+ messages in thread
From: Stefano Stabellini @ 2024-05-24 2:19 UTC (permalink / raw)
To: Julien Grall
Cc: Henry Wang, xen-devel, Anthony PERARD, George Dunlap,
Nick Rosbrook, Juergen Gross, Jason Andryuk
On Fri, 24 May 2024, Julien Grall wrote:
> Hi Henry,
>
> On 23/05/2024 08:40, Henry Wang wrote:
> > Currently, the number of SPIs allocated to the domain is only
> > configurable for Dom0less DomUs. Xen domains are supposed to be
> > platform agnostics and therefore the numbers of SPIs for libxl
> > guests should not be based on the hardware.
> >
> > Introduce a new xl config entry for Arm to provide a method for
> > user to decide the number of SPIs. This would help to avoid
> > bumping the `config->arch.nr_spis` in libxl everytime there is a
> > new platform with increased SPI numbers.
> >
> > Update the doc and the golang bindings accordingly.
> >
> > Signed-off-by: Henry Wang <xin.wang2@amd.com>
> > Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
> > ---
> > v4:
> > - Add Jason's Reviewed-by tag.
> > v3:
> > - Reword documentation to avoid ambiguity.
> > v2:
> > - New patch to replace the original patch in v1:
> > "[PATCH 05/15] tools/libs/light: Increase nr_spi to 160"
> > ---
> > docs/man/xl.cfg.5.pod.in | 14 ++++++++++++++
> > tools/golang/xenlight/helpers.gen.go | 2 ++
> > tools/golang/xenlight/types.gen.go | 1 +
> > tools/libs/light/libxl_arm.c | 4 ++--
> > tools/libs/light/libxl_types.idl | 1 +
> > tools/xl/xl_parse.c | 3 +++
> > 6 files changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> > index 8f2b375ce9..416d582844 100644
> > --- a/docs/man/xl.cfg.5.pod.in
> > +++ b/docs/man/xl.cfg.5.pod.in
> > @@ -3072,6 +3072,20 @@ raised.
> > =back
> > +=over 4
> > +
> > +=item B<nr_spis="NR_SPIS">
> > +
> > +An optional 32-bit integer parameter specifying the number of SPIs (Shared
>
> We can't support that much SPIs :). The limit would be 991 SPIs.
I change it
> > +Peripheral Interrupts) to allocate for the domain. If the value specified
> > by
> > +the `nr_spis` parameter is smaller than the number of SPIs calculated by
> > the
> > +toolstack based on the devices allocated for the domain, or the `nr_spis`
> > +parameter is not specified, the value calculated by the toolstack will be
> > used
> > +for the domain. Otherwise, the value specified by the `nr_spis` parameter
> > will
> > +be used.
>
> I think it would be worth mentioning that the number of SPIs should match the
> highest interrupt ID that will be assigned to the domain (rather than the
> number of SPIs planned to be assigned).
I added it
> > +
> > +=back
> > +
> > =head3 x86
> > =over 4
> > diff --git a/tools/golang/xenlight/helpers.gen.go
> > b/tools/golang/xenlight/helpers.gen.go
> > index b9cb5b33c7..fe5110474d 100644
> > --- a/tools/golang/xenlight/helpers.gen.go
> > +++ b/tools/golang/xenlight/helpers.gen.go
> > @@ -1154,6 +1154,7 @@ return fmt.Errorf("invalid union key '%v'", x.Type)}
> > x.ArchArm.GicVersion = GicVersion(xc.arch_arm.gic_version)
> > x.ArchArm.Vuart = VuartType(xc.arch_arm.vuart)
> > x.ArchArm.SveVl = SveType(xc.arch_arm.sve_vl)
> > +x.ArchArm.NrSpis = uint32(xc.arch_arm.nr_spis)
> > if err := x.ArchX86.MsrRelaxed.fromC(&xc.arch_x86.msr_relaxed);err != nil
> > {
> > return fmt.Errorf("converting field ArchX86.MsrRelaxed: %v", err)
> > }
> > @@ -1670,6 +1671,7 @@ return fmt.Errorf("invalid union key '%v'", x.Type)}
> > xc.arch_arm.gic_version = C.libxl_gic_version(x.ArchArm.GicVersion)
> > xc.arch_arm.vuart = C.libxl_vuart_type(x.ArchArm.Vuart)
> > xc.arch_arm.sve_vl = C.libxl_sve_type(x.ArchArm.SveVl)
> > +xc.arch_arm.nr_spis = C.uint32_t(x.ArchArm.NrSpis)
> > if err := x.ArchX86.MsrRelaxed.toC(&xc.arch_x86.msr_relaxed); err != nil {
> > return fmt.Errorf("converting field ArchX86.MsrRelaxed: %v", err)
> > }
> > diff --git a/tools/golang/xenlight/types.gen.go
> > b/tools/golang/xenlight/types.gen.go
> > index 5b293755d7..c9e45b306f 100644
> > --- a/tools/golang/xenlight/types.gen.go
> > +++ b/tools/golang/xenlight/types.gen.go
> > @@ -597,6 +597,7 @@ ArchArm struct {
> > GicVersion GicVersion
> > Vuart VuartType
> > SveVl SveType
> > +NrSpis uint32
> > }
> > ArchX86 struct {
> > MsrRelaxed Defbool
> > diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> > index 1cb89fa584..a4029e3ac8 100644
> > --- a/tools/libs/light/libxl_arm.c
> > +++ b/tools/libs/light/libxl_arm.c
> > @@ -181,8 +181,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> > LOG(DEBUG, "Configure the domain");
> > - config->arch.nr_spis = nr_spis;
> > - LOG(DEBUG, " - Allocate %u SPIs", nr_spis);
> > + config->arch.nr_spis = max(nr_spis, d_config->b_info.arch_arm.nr_spis);
>
> I am not entirely sure about using max(). To me if the user specifies a lower
> limit, then we should throw an error because this is likely an indication that
> the SPIs they will want to assign will clash with the emulated ones.
>
> So it would be better to warn at domain creation rather than waiting until the
> IRQs are assigned.
>
> I would like Anthony's opinion on this one. Given he is away this month, I
> guess we could get this patch merged (with other comments addressed) and have
> a follow-up if wanted before 4.19.
I left it as is for now
> > + LOG(DEBUG, " - Allocate %u SPIs", config->arch.nr_spis);
> > switch (d_config->b_info.arch_arm.gic_version) {
> > case LIBXL_GIC_VERSION_DEFAULT:
> > diff --git a/tools/libs/light/libxl_types.idl
> > b/tools/libs/light/libxl_types.idl
> > index 79e9c656cc..4e65e6fda5 100644
> > --- a/tools/libs/light/libxl_types.idl
> > +++ b/tools/libs/light/libxl_types.idl
> > @@ -722,6 +722,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> > ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
> > ("vuart", libxl_vuart_type),
> > ("sve_vl", libxl_sve_type),
> > + ("nr_spis", uint32),
>
> From my understandig, any change in the .idl requires a corresponding
> LIBXL_HAVE_... in include/libxl.h. This is in order to allow external
> toolstack (such as libvirt) to be able to select at build time between
> multiple version of libxl.
I added it
> > ])),
> > ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
> > ])),
> > diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> > index c504ab3711..e3a4800f6e 100644
> > --- a/tools/xl/xl_parse.c
> > +++ b/tools/xl/xl_parse.c
> > @@ -2935,6 +2935,9 @@ skip_usbdev:
> > }
> > }
> > + if (!xlu_cfg_get_long (config, "nr_spis", &l, 0))
> > + b_info->arch_arm.nr_spis = l;
> > +
> > parse_vkb_list(config, d_config);
> > d_config->virtios = NULL;
>
> Cheers,
>
> --
> Julien Grall
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v4 4/9] xen/arm/gic: Allow adding interrupt to running VMs
2024-05-23 7:40 [PATCH v4 0/9] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
` (2 preceding siblings ...)
2024-05-23 7:40 ` [PATCH v4 3/9] tools/arm: Introduce the "nr_spis" xl config entry Henry Wang
@ 2024-05-23 7:40 ` Henry Wang
2024-05-23 21:27 ` Julien Grall
2024-05-23 7:40 ` [PATCH v4 5/9] xen/arm: Add XEN_DOMCTL_dt_overlay and device attachment to domains Henry Wang
` (5 subsequent siblings)
9 siblings, 1 reply; 23+ messages in thread
From: Henry Wang @ 2024-05-23 7:40 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Henry Wang
Currently, adding physical interrupts are only allowed at
the domain creation time. For use cases such as dynamic device
tree overlay addition, the adding of physical IRQ to
running domains should be allowed.
Drop the above-mentioned domain creation check. Since this
will introduce interrupt state unsync issues for cases when the
interrupt is active or pending in the guest, therefore for these
cases we simply reject the operation. Do it for both new and old
vGIC implementations.
Signed-off-by: Henry Wang <xin.wang2@amd.com>
---
v4:
- Split the original patch, only do the adding IRQ stuff in this
patch.
v3:
- Update in-code comments.
- Correct the if conditions.
- Add taking/releasing the vgic lock of the vcpu.
v2:
- Reject the case where the IRQ is active or pending in guest.
---
xen/arch/arm/gic-vgic.c | 9 +++++++--
xen/arch/arm/gic.c | 8 --------
xen/arch/arm/vgic/vgic.c | 7 +++++--
3 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
index 56490dbc43..b99e287224 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -442,9 +442,14 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, unsigned int virq,
if ( connect )
{
- /* The VIRQ should not be already enabled by the guest */
+ /*
+ * The VIRQ should not be already enabled by the guest nor
+ * active/pending in the guest.
+ */
if ( !p->desc &&
- !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
+ !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
+ !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) &&
+ !test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
p->desc = desc;
else
ret = -EBUSY;
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 44c40e86de..b3467a76ae 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -135,14 +135,6 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
ASSERT(virq < vgic_num_irqs(d));
ASSERT(!is_lpi(virq));
- /*
- * When routing an IRQ to guest, the virtual state is not synced
- * back to the physical IRQ. To prevent get unsync, restrict the
- * routing to when the Domain is been created.
- */
- if ( d->creation_finished )
- return -EBUSY;
-
ret = vgic_connect_hw_irq(d, NULL, virq, desc, true);
if ( ret )
return ret;
diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
index b9463a5f27..048e12c562 100644
--- a/xen/arch/arm/vgic/vgic.c
+++ b/xen/arch/arm/vgic/vgic.c
@@ -876,8 +876,11 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *vcpu,
if ( connect ) /* assign a mapped IRQ */
{
- /* The VIRQ should not be already enabled by the guest */
- if ( !irq->hw && !irq->enabled )
+ /*
+ * The VIRQ should not be already enabled by the guest nor
+ * active/pending in the guest
+ */
+ if ( !irq->hw && !irq->enabled && !irq->active && !irq->pending_latch )
{
irq->hw = true;
irq->hwintid = desc->irq;
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v4 4/9] xen/arm/gic: Allow adding interrupt to running VMs
2024-05-23 7:40 ` [PATCH v4 4/9] xen/arm/gic: Allow adding interrupt to running VMs Henry Wang
@ 2024-05-23 21:27 ` Julien Grall
0 siblings, 0 replies; 23+ messages in thread
From: Julien Grall @ 2024-05-23 21:27 UTC (permalink / raw)
To: Henry Wang, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
Hi Henry,
On 23/05/2024 08:40, Henry Wang wrote:
> Currently, adding physical interrupts are only allowed at
> the domain creation time. For use cases such as dynamic device
> tree overlay addition, the adding of physical IRQ to
> running domains should be allowed.
>
> Drop the above-mentioned domain creation check. Since this
> will introduce interrupt state unsync issues for cases when the
> interrupt is active or pending in the guest, therefore for these
> cases we simply reject the operation. Do it for both new and old
> vGIC implementations.
>
> Signed-off-by: Henry Wang <xin.wang2@amd.com>
With one remark below:
Reviewed-by: Julien Grall <jgrall@amazon.com>
> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
> index b9463a5f27..048e12c562 100644
> --- a/xen/arch/arm/vgic/vgic.c
> +++ b/xen/arch/arm/vgic/vgic.c
> @@ -876,8 +876,11 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *vcpu,
>
> if ( connect ) /* assign a mapped IRQ */
> {
> - /* The VIRQ should not be already enabled by the guest */
> - if ( !irq->hw && !irq->enabled )
> + /*
> + * The VIRQ should not be already enabled by the guest nor
> + * active/pending in the guest
Typo: Missing full stop.
It can be fixed on commit.
> + */
> + if ( !irq->hw && !irq->enabled && !irq->active && !irq->pending_latch )
> {
> irq->hw = true;
> irq->hwintid = desc->irq;
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v4 5/9] xen/arm: Add XEN_DOMCTL_dt_overlay and device attachment to domains
2024-05-23 7:40 [PATCH v4 0/9] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
` (3 preceding siblings ...)
2024-05-23 7:40 ` [PATCH v4 4/9] xen/arm/gic: Allow adding interrupt to running VMs Henry Wang
@ 2024-05-23 7:40 ` Henry Wang
2024-05-23 21:55 ` Julien Grall
2024-05-23 7:40 ` [PATCH v4 6/9] xen/arm/gic: Allow removing interrupt to running VMs Henry Wang
` (4 subsequent siblings)
9 siblings, 1 reply; 23+ messages in thread
From: Henry Wang @ 2024-05-23 7:40 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
Henry Wang
In order to support the dynamic dtbo device assignment to a running
VM, the add/remove of the DT overlay and the attach/detach of the
device from the DT overlay should happen separately. Therefore,
repurpose the existing XEN_SYSCTL_dt_overlay to only add the DT
overlay to Xen device tree, instead of assigning the device to the
hardware domain at the same time. Add the XEN_DOMCTL_dt_overlay with
operations XEN_DOMCTL_DT_OVERLAY_ATTACH to do the device assignment
to the domain.
The hypervisor firstly checks the DT overlay passed from the toolstack
is valid. Then the device nodes are retrieved from the overlay tracker
based on the DT overlay. The attach of the device is implemented by
mapping the IRQ and IOMMU resources.
Signed-off-by: Henry Wang <xin.wang2@amd.com>
Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
---
v4:
- Split the original patch, only do the device attachment.
v3:
- Style fixes for arch-selection #ifdefs.
- Do not include public/domctl.h, only add a forward declaration of
struct xen_domctl_dt_overlay.
- Extract the overlay track entry finding logic to a function, drop
the unused variables.
- Use op code 1&2 for XEN_DOMCTL_DT_OVERLAY_{ATTACH,DETACH}.
v2:
- New patch.
---
xen/arch/arm/domctl.c | 3 +
xen/common/dt-overlay.c | 199 ++++++++++++++++++++++++++---------
xen/include/public/domctl.h | 14 +++
xen/include/public/sysctl.h | 11 +-
xen/include/xen/dt-overlay.h | 7 ++
5 files changed, 176 insertions(+), 58 deletions(-)
diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
index ad56efb0f5..12a12ee781 100644
--- a/xen/arch/arm/domctl.c
+++ b/xen/arch/arm/domctl.c
@@ -5,6 +5,7 @@
* Copyright (c) 2012, Citrix Systems
*/
+#include <xen/dt-overlay.h>
#include <xen/errno.h>
#include <xen/guest_access.h>
#include <xen/hypercall.h>
@@ -176,6 +177,8 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
return rc;
}
+ case XEN_DOMCTL_dt_overlay:
+ return dt_overlay_domctl(d, &domctl->u.dt_overlay);
default:
return subarch_do_domctl(domctl, d, u_domctl);
}
diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c
index 9cece79067..1087f9b502 100644
--- a/xen/common/dt-overlay.c
+++ b/xen/common/dt-overlay.c
@@ -356,6 +356,42 @@ static int overlay_get_nodes_info(const void *fdto, char **nodes_full_path)
return 0;
}
+/* This function should be called with the overlay_lock taken */
+static struct overlay_track *
+find_track_entry_from_tracker(const void *overlay_fdt,
+ uint32_t overlay_fdt_size)
+{
+ struct overlay_track *entry, *temp;
+ bool found_entry = false;
+
+ ASSERT(spin_is_locked(&overlay_lock));
+
+ /*
+ * First check if dtbo is correct i.e. it should one of the dtbo which was
+ * used when dynamically adding the node.
+ * Limitation: Cases with same node names but different property are not
+ * supported currently. We are relying on user to provide the same dtbo
+ * as it was used when adding the nodes.
+ */
+ list_for_each_entry_safe( entry, temp, &overlay_tracker, entry )
+ {
+ if ( memcmp(entry->overlay_fdt, overlay_fdt, overlay_fdt_size) == 0 )
+ {
+ found_entry = true;
+ break;
+ }
+ }
+
+ if ( !found_entry )
+ {
+ printk(XENLOG_ERR "Cannot find any matching tracker with input dtbo."
+ " Operation is supported only for prior added dtbo.\n");
+ return NULL;
+ }
+
+ return entry;
+}
+
/* Check if node itself can be removed and remove node from IOMMU. */
static int remove_node_resources(struct dt_device_node *device_node)
{
@@ -485,8 +521,7 @@ static long handle_remove_overlay_nodes(const void *overlay_fdt,
uint32_t overlay_fdt_size)
{
int rc;
- struct overlay_track *entry, *temp, *track;
- bool found_entry = false;
+ struct overlay_track *entry;
rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size);
if ( rc )
@@ -494,29 +529,10 @@ static long handle_remove_overlay_nodes(const void *overlay_fdt,
spin_lock(&overlay_lock);
- /*
- * First check if dtbo is correct i.e. it should one of the dtbo which was
- * used when dynamically adding the node.
- * Limitation: Cases with same node names but different property are not
- * supported currently. We are relying on user to provide the same dtbo
- * as it was used when adding the nodes.
- */
- list_for_each_entry_safe( entry, temp, &overlay_tracker, entry )
- {
- if ( memcmp(entry->overlay_fdt, overlay_fdt, overlay_fdt_size) == 0 )
- {
- track = entry;
- found_entry = true;
- break;
- }
- }
-
- if ( !found_entry )
+ entry = find_track_entry_from_tracker(overlay_fdt, overlay_fdt_size);
+ if ( entry == NULL )
{
rc = -EINVAL;
-
- printk(XENLOG_ERR "Cannot find any matching tracker with input dtbo."
- " Removing nodes is supported only for prior added dtbo.\n");
goto out;
}
@@ -620,15 +636,7 @@ static long add_nodes(struct overlay_track *tr, char **nodes_full_path)
return -EFAULT;
}
- rc = handle_device(hardware_domain, overlay_node, p2m_mmio_direct_c,
- tr->iomem_ranges,
- tr->irq_ranges);
write_unlock(&dt_host_lock);
- if ( rc )
- {
- printk(XENLOG_ERR "Adding IRQ and IOMMU failed\n");
- return rc;
- }
/* Keep overlay_node address in tracker. */
tr->nodes_address[j] = (unsigned long)overlay_node;
@@ -638,9 +646,7 @@ static long add_nodes(struct overlay_track *tr, char **nodes_full_path)
}
/*
* Adds device tree nodes under target node.
- * We use tr->dt_host_new to unflatten the updated device_tree_flattened. This
- * is done to avoid the removal of device_tree generation, iomem regions mapping
- * to hardware domain done by handle_node().
+ * We use tr->dt_host_new to unflatten the updated device_tree_flattened.
*/
static long handle_add_overlay_nodes(void *overlay_fdt,
uint32_t overlay_fdt_size)
@@ -774,20 +780,6 @@ static long handle_add_overlay_nodes(void *overlay_fdt,
goto err;
}
- tr->irq_ranges = rangeset_new(hardware_domain, "Overlays: Interrupts", 0);
- if (tr->irq_ranges == NULL)
- {
- printk(XENLOG_ERR "Creating IRQ rangeset failed");
- goto err;
- }
-
- tr->iomem_ranges = rangeset_new(hardware_domain, "Overlay: I/O Memory", 0);
- if (tr->iomem_ranges == NULL)
- {
- printk(XENLOG_ERR "Creating IOMMU rangeset failed");
- goto err;
- }
-
rc = add_nodes(tr, nodes_full_path);
if ( rc )
{
@@ -843,14 +835,83 @@ static long handle_add_overlay_nodes(void *overlay_fdt,
xfree(tr->nodes_address);
xfree(tr->fdt);
- rangeset_destroy(tr->irq_ranges);
- rangeset_destroy(tr->iomem_ranges);
-
xfree(tr);
return rc;
}
+static long handle_attach_overlay_nodes(struct domain *d,
+ const void *overlay_fdt,
+ uint32_t overlay_fdt_size)
+{
+ int rc;
+ unsigned int j;
+ struct overlay_track *entry;
+
+ rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size);
+ if ( rc )
+ return rc;
+
+ spin_lock(&overlay_lock);
+
+ entry = find_track_entry_from_tracker(overlay_fdt, overlay_fdt_size);
+ if ( entry == NULL )
+ {
+ rc = -EINVAL;
+ goto out;
+ }
+
+ entry->irq_ranges = rangeset_new(d, "Overlays: Interrupts", 0);
+ if (entry->irq_ranges == NULL)
+ {
+ rc = -ENOMEM;
+ printk(XENLOG_ERR "Creating IRQ rangeset failed");
+ goto out;
+ }
+
+ entry->iomem_ranges = rangeset_new(d, "Overlay: I/O Memory", 0);
+ if (entry->iomem_ranges == NULL)
+ {
+ rc = -ENOMEM;
+ printk(XENLOG_ERR "Creating IOMMU rangeset failed");
+ goto out;
+ }
+
+ for ( j = 0; j < entry->num_nodes; j++ )
+ {
+ struct dt_device_node *overlay_node;
+
+ overlay_node = (struct dt_device_node *)entry->nodes_address[j];
+ if ( overlay_node == NULL )
+ {
+ rc = -EINVAL;
+ goto out;
+ }
+
+ write_lock(&dt_host_lock);
+ rc = handle_device(d, overlay_node, p2m_mmio_direct_c,
+ entry->iomem_ranges, entry->irq_ranges);
+ write_unlock(&dt_host_lock);
+ if ( rc )
+ {
+ printk(XENLOG_ERR "Adding IRQ and IOMMU failed\n");
+ goto out;
+ }
+ }
+
+ spin_unlock(&overlay_lock);
+
+ return 0;
+
+ out:
+ spin_unlock(&overlay_lock);
+
+ rangeset_destroy(entry->irq_ranges);
+ rangeset_destroy(entry->iomem_ranges);
+
+ return rc;
+}
+
long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op)
{
long ret;
@@ -890,6 +951,42 @@ long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op)
return ret;
}
+long dt_overlay_domctl(struct domain *d, struct xen_domctl_dt_overlay *op)
+{
+ long ret;
+ void *overlay_fdt;
+
+ if ( op->overlay_op != XEN_DOMCTL_DT_OVERLAY_ATTACH )
+ return -EOPNOTSUPP;
+
+ if ( op->overlay_fdt_size == 0 || op->overlay_fdt_size > KB(500) )
+ return -EINVAL;
+
+ if ( op->pad[0] || op->pad[1] || op->pad[2] )
+ return -EINVAL;
+
+ overlay_fdt = xmalloc_bytes(op->overlay_fdt_size);
+
+ if ( overlay_fdt == NULL )
+ return -ENOMEM;
+
+ ret = copy_from_guest(overlay_fdt, op->overlay_fdt, op->overlay_fdt_size);
+ if ( ret )
+ {
+ gprintk(XENLOG_ERR, "copy from guest failed\n");
+ xfree(overlay_fdt);
+
+ return -EFAULT;
+ }
+
+ if ( op->overlay_op == XEN_DOMCTL_DT_OVERLAY_ATTACH )
+ ret = handle_attach_overlay_nodes(d, overlay_fdt, op->overlay_fdt_size);
+
+ xfree(overlay_fdt);
+
+ return ret;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index a33f9ec32b..ac3c2a7c4c 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1190,6 +1190,16 @@ struct xen_domctl_vmtrace_op {
typedef struct xen_domctl_vmtrace_op xen_domctl_vmtrace_op_t;
DEFINE_XEN_GUEST_HANDLE(xen_domctl_vmtrace_op_t);
+#if defined(__arm__) || defined(__aarch64__)
+struct xen_domctl_dt_overlay {
+ XEN_GUEST_HANDLE_64(const_void) overlay_fdt; /* IN: overlay fdt. */
+ uint32_t overlay_fdt_size; /* IN: Overlay dtb size. */
+#define XEN_DOMCTL_DT_OVERLAY_ATTACH 1
+ uint8_t overlay_op; /* IN: Attach. */
+ uint8_t pad[3]; /* IN: Must be zero. */
+};
+#endif
+
struct xen_domctl {
uint32_t cmd;
#define XEN_DOMCTL_createdomain 1
@@ -1277,6 +1287,7 @@ struct xen_domctl {
#define XEN_DOMCTL_vmtrace_op 84
#define XEN_DOMCTL_get_paging_mempool_size 85
#define XEN_DOMCTL_set_paging_mempool_size 86
+#define XEN_DOMCTL_dt_overlay 87
#define XEN_DOMCTL_gdbsx_guestmemio 1000
#define XEN_DOMCTL_gdbsx_pausevcpu 1001
#define XEN_DOMCTL_gdbsx_unpausevcpu 1002
@@ -1339,6 +1350,9 @@ struct xen_domctl {
struct xen_domctl_vuart_op vuart_op;
struct xen_domctl_vmtrace_op vmtrace_op;
struct xen_domctl_paging_mempool paging_mempool;
+#if defined(__arm__) || defined(__aarch64__)
+ struct xen_domctl_dt_overlay dt_overlay;
+#endif
uint8_t pad[128];
} u;
};
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index febaa4b16a..3a6e7d48f0 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -1184,14 +1184,11 @@ typedef struct xen_sysctl_cpu_policy xen_sysctl_cpu_policy_t;
DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpu_policy_t);
#endif
-#if defined(__arm__) || defined (__aarch64__)
+#if defined(__arm__) || defined(__aarch64__)
/*
* XEN_SYSCTL_dt_overlay
- * Performs addition/removal of device tree nodes under parent node using dtbo.
- * This does in three steps:
- * - Adds/Removes the nodes from dt_host.
- * - Adds/Removes IRQ permission for the nodes.
- * - Adds/Removes MMIO accesses.
+ * Performs addition/removal of device tree nodes under parent node using dtbo
+ * from dt_host.
*/
struct xen_sysctl_dt_overlay {
XEN_GUEST_HANDLE_64(const_void) overlay_fdt; /* IN: overlay fdt. */
@@ -1265,7 +1262,7 @@ struct xen_sysctl {
struct xen_sysctl_cpu_policy cpu_policy;
#endif
-#if defined(__arm__) || defined (__aarch64__)
+#if defined(__arm__) || defined(__aarch64__)
struct xen_sysctl_dt_overlay dt_overlay;
#endif
uint8_t pad[128];
diff --git a/xen/include/xen/dt-overlay.h b/xen/include/xen/dt-overlay.h
index c0567741ee..7f0f3741b5 100644
--- a/xen/include/xen/dt-overlay.h
+++ b/xen/include/xen/dt-overlay.h
@@ -39,15 +39,22 @@ struct overlay_track {
};
struct xen_sysctl_dt_overlay;
+struct xen_domctl_dt_overlay;
#ifdef CONFIG_OVERLAY_DTB
long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op);
+long dt_overlay_domctl(struct domain *d, struct xen_domctl_dt_overlay *op);
#else
#include <xen/errno.h>
static inline long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op)
{
return -EOPNOTSUPP;
}
+static inline long dt_overlay_domctl(struct domain *d,
+ struct xen_domctl_dt_overlay *op)
+{
+ return -EOPNOTSUPP;
+}
#endif
#endif /* __XEN_DT_OVERLAY_H__ */
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v4 5/9] xen/arm: Add XEN_DOMCTL_dt_overlay and device attachment to domains
2024-05-23 7:40 ` [PATCH v4 5/9] xen/arm: Add XEN_DOMCTL_dt_overlay and device attachment to domains Henry Wang
@ 2024-05-23 21:55 ` Julien Grall
2024-05-24 2:19 ` Stefano Stabellini
0 siblings, 1 reply; 23+ messages in thread
From: Julien Grall @ 2024-05-23 21:55 UTC (permalink / raw)
To: Henry Wang, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Jan Beulich
Hi Henry,
On 23/05/2024 08:40, Henry Wang wrote:
> In order to support the dynamic dtbo device assignment to a running
> VM, the add/remove of the DT overlay and the attach/detach of the
> device from the DT overlay should happen separately. Therefore,
> repurpose the existing XEN_SYSCTL_dt_overlay to only add the DT
> overlay to Xen device tree
I think it would be worth mentioning in the commit message why changing
the sysctl behavior is fine. The feature is experimental and therefore
breaking compatibility is ok.
> , instead of assigning the device to the
> hardware domain at the same time. Add the XEN_DOMCTL_dt_overlay with
> operations XEN_DOMCTL_DT_OVERLAY_ATTACH to do the device assignment
> to the domain.
>
> The hypervisor firstly checks the DT overlay passed from the toolstack
> is valid. Then the device nodes are retrieved from the overlay tracker
> based on the DT overlay. The attach of the device is implemented by
> mapping the IRQ and IOMMU resources.
So, the expectation is the user will always want to attach all the
devices in the overlay to a single domain. Is that correct?
>
> Signed-off-by: Henry Wang <xin.wang2@amd.com>
> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
> ---
> v4:
> - Split the original patch, only do the device attachment.
> v3:
> - Style fixes for arch-selection #ifdefs.
> - Do not include public/domctl.h, only add a forward declaration of
> struct xen_domctl_dt_overlay.
> - Extract the overlay track entry finding logic to a function, drop
> the unused variables.
> - Use op code 1&2 for XEN_DOMCTL_DT_OVERLAY_{ATTACH,DETACH}.
> v2:
> - New patch.
> ---
> xen/arch/arm/domctl.c | 3 +
> xen/common/dt-overlay.c | 199 ++++++++++++++++++++++++++---------
> xen/include/public/domctl.h | 14 +++
> xen/include/public/sysctl.h | 11 +-
> xen/include/xen/dt-overlay.h | 7 ++
> 5 files changed, 176 insertions(+), 58 deletions(-)
>
> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
> index ad56efb0f5..12a12ee781 100644
> --- a/xen/arch/arm/domctl.c
> +++ b/xen/arch/arm/domctl.c
> @@ -5,6 +5,7 @@
> * Copyright (c) 2012, Citrix Systems
> */
>
> +#include <xen/dt-overlay.h>
> #include <xen/errno.h>
> #include <xen/guest_access.h>
> #include <xen/hypercall.h>
> @@ -176,6 +177,8 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>
> return rc;
> }
> + case XEN_DOMCTL_dt_overlay:
> + return dt_overlay_domctl(d, &domctl->u.dt_overlay);
> default:
> return subarch_do_domctl(domctl, d, u_domctl);
> }
> diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c
> index 9cece79067..1087f9b502 100644
> --- a/xen/common/dt-overlay.c
> +++ b/xen/common/dt-overlay.c
> @@ -356,6 +356,42 @@ static int overlay_get_nodes_info(const void *fdto, char **nodes_full_path)
> return 0;
> }
>
> +/* This function should be called with the overlay_lock taken */
> +static struct overlay_track *
> +find_track_entry_from_tracker(const void *overlay_fdt,
> + uint32_t overlay_fdt_size)
> +{
> + struct overlay_track *entry, *temp;
> + bool found_entry = false;
> +
> + ASSERT(spin_is_locked(&overlay_lock));
> +
> + /*
> + * First check if dtbo is correct i.e. it should one of the dtbo which was
> + * used when dynamically adding the node.
> + * Limitation: Cases with same node names but different property are not
> + * supported currently. We are relying on user to provide the same dtbo
> + * as it was used when adding the nodes.
> + */
> + list_for_each_entry_safe( entry, temp, &overlay_tracker, entry )
> + {
> + if ( memcmp(entry->overlay_fdt, overlay_fdt, overlay_fdt_size) == 0 )
> + {
> + found_entry = true;
> + break;
> + }
> + }
> +
> + if ( !found_entry )
> + {
> + printk(XENLOG_ERR "Cannot find any matching tracker with input dtbo."
> + " Operation is supported only for prior added dtbo.\n");
> + return NULL;
> + }
> +
> + return entry;
> +}
> +
> /* Check if node itself can be removed and remove node from IOMMU. */
> static int remove_node_resources(struct dt_device_node *device_node)
> {
> @@ -485,8 +521,7 @@ static long handle_remove_overlay_nodes(const void *overlay_fdt,
> uint32_t overlay_fdt_size)
> {
> int rc;
> - struct overlay_track *entry, *temp, *track;
> - bool found_entry = false;
> + struct overlay_track *entry;
>
> rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size);
> if ( rc )
> @@ -494,29 +529,10 @@ static long handle_remove_overlay_nodes(const void *overlay_fdt,
>
> spin_lock(&overlay_lock);
>
> - /*
> - * First check if dtbo is correct i.e. it should one of the dtbo which was
> - * used when dynamically adding the node.
> - * Limitation: Cases with same node names but different property are not
> - * supported currently. We are relying on user to provide the same dtbo
> - * as it was used when adding the nodes.
> - */
> - list_for_each_entry_safe( entry, temp, &overlay_tracker, entry )
> - {
> - if ( memcmp(entry->overlay_fdt, overlay_fdt, overlay_fdt_size) == 0 )
> - {
> - track = entry;
> - found_entry = true;
> - break;
> - }
> - }
> -
> - if ( !found_entry )
> + entry = find_track_entry_from_tracker(overlay_fdt, overlay_fdt_size);
> + if ( entry == NULL )
> {
> rc = -EINVAL;
> -
> - printk(XENLOG_ERR "Cannot find any matching tracker with input dtbo."
> - " Removing nodes is supported only for prior added dtbo.\n");
> goto out;
>
> }
> @@ -620,15 +636,7 @@ static long add_nodes(struct overlay_track *tr, char **nodes_full_path)
> return -EFAULT;
> }
>
> - rc = handle_device(hardware_domain, overlay_node, p2m_mmio_direct_c,
> - tr->iomem_ranges,
> - tr->irq_ranges);
> write_unlock(&dt_host_lock);
> - if ( rc )
> - {
> - printk(XENLOG_ERR "Adding IRQ and IOMMU failed\n");
> - return rc;
> - }
>
> /* Keep overlay_node address in tracker. */
> tr->nodes_address[j] = (unsigned long)overlay_node;
> @@ -638,9 +646,7 @@ static long add_nodes(struct overlay_track *tr, char **nodes_full_path)
> }
> /*
> * Adds device tree nodes under target node.
> - * We use tr->dt_host_new to unflatten the updated device_tree_flattened. This
> - * is done to avoid the removal of device_tree generation, iomem regions mapping
> - * to hardware domain done by handle_node().
> + * We use tr->dt_host_new to unflatten the updated device_tree_flattened.
> */
> static long handle_add_overlay_nodes(void *overlay_fdt,
> uint32_t overlay_fdt_size)
> @@ -774,20 +780,6 @@ static long handle_add_overlay_nodes(void *overlay_fdt,
> goto err;
> }
>
> - tr->irq_ranges = rangeset_new(hardware_domain, "Overlays: Interrupts", 0);
> - if (tr->irq_ranges == NULL)
> - {
> - printk(XENLOG_ERR "Creating IRQ rangeset failed");
> - goto err;
> - }
> -
> - tr->iomem_ranges = rangeset_new(hardware_domain, "Overlay: I/O Memory", 0);
> - if (tr->iomem_ranges == NULL)
> - {
> - printk(XENLOG_ERR "Creating IOMMU rangeset failed");
> - goto err;
> - }
> -
> rc = add_nodes(tr, nodes_full_path);
> if ( rc )
> {
> @@ -843,14 +835,83 @@ static long handle_add_overlay_nodes(void *overlay_fdt,
> xfree(tr->nodes_address);
> xfree(tr->fdt);
>
> - rangeset_destroy(tr->irq_ranges);
> - rangeset_destroy(tr->iomem_ranges);
> -
> xfree(tr);
>
> return rc;
> }
>
> +static long handle_attach_overlay_nodes(struct domain *d,
> + const void *overlay_fdt,
> + uint32_t overlay_fdt_size)
> +{
> + int rc;
> + unsigned int j;
> + struct overlay_track *entry;
> +
> + rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size);
> + if ( rc )
> + return rc;
> +
> + spin_lock(&overlay_lock);
> +
> + entry = find_track_entry_from_tracker(overlay_fdt, overlay_fdt_size);
> + if ( entry == NULL )
> + {
> + rc = -EINVAL;
> + goto out;
> + }
> +
> + entry->irq_ranges = rangeset_new(d, "Overlays: Interrupts", 0);
> + if (entry->irq_ranges == NULL)
> + {
> + rc = -ENOMEM;
> + printk(XENLOG_ERR "Creating IRQ rangeset failed");
> + goto out;
> + }
> +
> + entry->iomem_ranges = rangeset_new(d, "Overlay: I/O Memory", 0);
> + if (entry->iomem_ranges == NULL)
> + {
> + rc = -ENOMEM;
> + printk(XENLOG_ERR "Creating IOMMU rangeset failed");
> + goto out;
> + }
> +
> + for ( j = 0; j < entry->num_nodes; j++ )
> + {
> + struct dt_device_node *overlay_node;
> +
> + overlay_node = (struct dt_device_node *)entry->nodes_address[j];
> + if ( overlay_node == NULL )
> + {
> + rc = -EINVAL;
> + goto out;
> + }
> +
> + write_lock(&dt_host_lock);
> + rc = handle_device(d, overlay_node, p2m_mmio_direct_c,
> + entry->iomem_ranges, entry->irq_ranges);
> + write_unlock(&dt_host_lock);
> + if ( rc )
> + {
> + printk(XENLOG_ERR "Adding IRQ and IOMMU failed\n");
> + goto out;
> + }
> + }
> +
> + spin_unlock(&overlay_lock);
> +
> + return 0;
> +
> + out:
> + spin_unlock(&overlay_lock);
> +
> + rangeset_destroy(entry->irq_ranges);
> + rangeset_destroy(entry->iomem_ranges);
> +
> + return rc;
> +}
> +
> long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op)
> {
> long ret;
> @@ -890,6 +951,42 @@ long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op)
> return ret;
> }
>
> +long dt_overlay_domctl(struct domain *d, struct xen_domctl_dt_overlay *op)
> +{
> + long ret;
> + void *overlay_fdt;
> +
> + if ( op->overlay_op != XEN_DOMCTL_DT_OVERLAY_ATTACH )
> + return -EOPNOTSUPP;
> +
> + if ( op->overlay_fdt_size == 0 || op->overlay_fdt_size > KB(500) )
Let's avoid to hardocode KB(500) a second time (it alredy exists in the
sysctl path).
But I would actually consider to allow up to 512KB because...
> + return -EINVAL;
> +
> + if ( op->pad[0] || op->pad[1] || op->pad[2] )
> + return -EINVAL;
> +
> + overlay_fdt = xmalloc_bytes(op->overlay_fdt_size);
... xmalloc_bytes() will be more efficient. If the size is not power of
2 pages, it will free the leftover. Anyway, this could be a follow-up patch.
> +
> + if ( overlay_fdt == NULL )
> + return -ENOMEM;
> +
> + ret = copy_from_guest(overlay_fdt, op->overlay_fdt, op->overlay_fdt_size);
> + if ( ret )
> + {
> + gprintk(XENLOG_ERR, "copy from guest failed\n");
> + xfree(overlay_fdt);
> +
> + return -EFAULT;
> + }
> +
> + if ( op->overlay_op == XEN_DOMCTL_DT_OVERLAY_ATTACH )
> + ret = handle_attach_overlay_nodes(d, overlay_fdt, op->overlay_fdt_size);
I think you would return 0 if the operation is not supported. But I
think we need to return -EOPNOTSUPP.
> +
> + xfree(overlay_fdt);
> +
> + return ret;
> +}
> +
> /*
> * Local variables:
> * mode: C
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index a33f9ec32b..ac3c2a7c4c 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
If I am not mistkane, this is the first change in the domctl header for
4.19. So you want to bump XEN_DOMCTL_INTERFACE_VERSION.
> @@ -1190,6 +1190,16 @@ struct xen_domctl_vmtrace_op {
> typedef struct xen_domctl_vmtrace_op xen_domctl_vmtrace_op_t;
> DEFINE_XEN_GUEST_HANDLE(xen_domctl_vmtrace_op_t);
>
> +#if defined(__arm__) || defined(__aarch64__)
> +struct xen_domctl_dt_overlay {
> + XEN_GUEST_HANDLE_64(const_void) overlay_fdt; /* IN: overlay fdt. */
> + uint32_t overlay_fdt_size; /* IN: Overlay dtb size. */
> +#define XEN_DOMCTL_DT_OVERLAY_ATTACH 1
OOI, why not starting from 0?
> + uint8_t overlay_op; /* IN: Attach. */
> + uint8_t pad[3]; /* IN: Must be zero. */
> +};
> +#endif
> +
> struct xen_domctl {
> uint32_t cmd;
> #define XEN_DOMCTL_createdomain 1
> @@ -1277,6 +1287,7 @@ struct xen_domctl {
> #define XEN_DOMCTL_vmtrace_op 84
> #define XEN_DOMCTL_get_paging_mempool_size 85
> #define XEN_DOMCTL_set_paging_mempool_size 86
> +#define XEN_DOMCTL_dt_overlay 87
> #define XEN_DOMCTL_gdbsx_guestmemio 1000
> #define XEN_DOMCTL_gdbsx_pausevcpu 1001
> #define XEN_DOMCTL_gdbsx_unpausevcpu 1002
> @@ -1339,6 +1350,9 @@ struct xen_domctl {
> struct xen_domctl_vuart_op vuart_op;
> struct xen_domctl_vmtrace_op vmtrace_op;
> struct xen_domctl_paging_mempool paging_mempool;
> +#if defined(__arm__) || defined(__aarch64__)
> + struct xen_domctl_dt_overlay dt_overlay;
> +#endif
> uint8_t pad[128];
> } u;
> };
> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
> index febaa4b16a..3a6e7d48f0 100644
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -1184,14 +1184,11 @@ typedef struct xen_sysctl_cpu_policy xen_sysctl_cpu_policy_t;
> DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpu_policy_t);
> #endif
>
> -#if defined(__arm__) || defined (__aarch64__)
> +#if defined(__arm__) || defined(__aarch64__)
This seems like unrelated change. Maybe mention in the commit message
that you are fixing the coding style?
> /*
> * XEN_SYSCTL_dt_overlay
> - * Performs addition/removal of device tree nodes under parent node using dtbo.
> - * This does in three steps:
> - * - Adds/Removes the nodes from dt_host.
> - * - Adds/Removes IRQ permission for the nodes.
> - * - Adds/Removes MMIO accesses.
> + * Performs addition/removal of device tree nodes under parent node using dtbo
> + * from dt_host.
> */
> struct xen_sysctl_dt_overlay {
> XEN_GUEST_HANDLE_64(const_void) overlay_fdt; /* IN: overlay fdt. */
> @@ -1265,7 +1262,7 @@ struct xen_sysctl {
> struct xen_sysctl_cpu_policy cpu_policy;
> #endif
>
> -#if defined(__arm__) || defined (__aarch64__)
> +#if defined(__arm__) || defined(__aarch64__)
> struct xen_sysctl_dt_overlay dt_overlay;
> #endif
> uint8_t pad[128];
> diff --git a/xen/include/xen/dt-overlay.h b/xen/include/xen/dt-overlay.h
> index c0567741ee..7f0f3741b5 100644
> --- a/xen/include/xen/dt-overlay.h
> +++ b/xen/include/xen/dt-overlay.h
> @@ -39,15 +39,22 @@ struct overlay_track {
> };
>
> struct xen_sysctl_dt_overlay;
> +struct xen_domctl_dt_overlay;
>
> #ifdef CONFIG_OVERLAY_DTB
> long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op);
> +long dt_overlay_domctl(struct domain *d, struct xen_domctl_dt_overlay *op);
> #else
> #include <xen/errno.h>
> static inline long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op)
> {
> return -EOPNOTSUPP;
> }
NIT: Newline here please.
> +static inline long dt_overlay_domctl(struct domain *d,
> + struct xen_domctl_dt_overlay *op)
> +{
> + return -EOPNOTSUPP;
> +}
> #endif
>
> #endif /* __XEN_DT_OVERLAY_H__ */
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v4 5/9] xen/arm: Add XEN_DOMCTL_dt_overlay and device attachment to domains
2024-05-23 21:55 ` Julien Grall
@ 2024-05-24 2:19 ` Stefano Stabellini
0 siblings, 0 replies; 23+ messages in thread
From: Stefano Stabellini @ 2024-05-24 2:19 UTC (permalink / raw)
To: Julien Grall
Cc: Henry Wang, xen-devel, Stefano Stabellini, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk, Andrew Cooper, George Dunlap,
Jan Beulich
On Thu, 23 May 2024, Julien Grall wrote:
> Hi Henry,
>
> On 23/05/2024 08:40, Henry Wang wrote:
> > In order to support the dynamic dtbo device assignment to a running
> > VM, the add/remove of the DT overlay and the attach/detach of the
> > device from the DT overlay should happen separately. Therefore,
> > repurpose the existing XEN_SYSCTL_dt_overlay to only add the DT
> > overlay to Xen device tree
>
> I think it would be worth mentioning in the commit message why changing the
> sysctl behavior is fine. The feature is experimental and therefore breaking
> compatibility is ok.
Added
> > , instead of assigning the device to the
> > hardware domain at the same time. Add the XEN_DOMCTL_dt_overlay with
> > operations XEN_DOMCTL_DT_OVERLAY_ATTACH to do the device assignment
> > to the domain.
> >
> > The hypervisor firstly checks the DT overlay passed from the toolstack
> > is valid. Then the device nodes are retrieved from the overlay tracker
> > based on the DT overlay. The attach of the device is implemented by
> > mapping the IRQ and IOMMU resources.
>
> So, the expectation is the user will always want to attach all the devices in
> the overlay to a single domain. Is that correct?
Yes, also added to the commit message
> >
> > Signed-off-by: Henry Wang <xin.wang2@amd.com>
> > Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
> > ---
> > v4:
> > - Split the original patch, only do the device attachment.
> > v3:
> > - Style fixes for arch-selection #ifdefs.
> > - Do not include public/domctl.h, only add a forward declaration of
> > struct xen_domctl_dt_overlay.
> > - Extract the overlay track entry finding logic to a function, drop
> > the unused variables.
> > - Use op code 1&2 for XEN_DOMCTL_DT_OVERLAY_{ATTACH,DETACH}.
> > v2:
> > - New patch.
> > ---
> > xen/arch/arm/domctl.c | 3 +
> > xen/common/dt-overlay.c | 199 ++++++++++++++++++++++++++---------
> > xen/include/public/domctl.h | 14 +++
> > xen/include/public/sysctl.h | 11 +-
> > xen/include/xen/dt-overlay.h | 7 ++
> > 5 files changed, 176 insertions(+), 58 deletions(-)
> >
> > diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
> > index ad56efb0f5..12a12ee781 100644
> > --- a/xen/arch/arm/domctl.c
> > +++ b/xen/arch/arm/domctl.c
> > @@ -5,6 +5,7 @@
> > * Copyright (c) 2012, Citrix Systems
> > */
> > +#include <xen/dt-overlay.h>
> > #include <xen/errno.h>
> > #include <xen/guest_access.h>
> > #include <xen/hypercall.h>
> > @@ -176,6 +177,8 @@ long arch_do_domctl(struct xen_domctl *domctl, struct
> > domain *d,
> > return rc;
> > }
> > + case XEN_DOMCTL_dt_overlay:
> > + return dt_overlay_domctl(d, &domctl->u.dt_overlay);
> > default:
> > return subarch_do_domctl(domctl, d, u_domctl);
> > }
> > diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c
> > index 9cece79067..1087f9b502 100644
> > --- a/xen/common/dt-overlay.c
> > +++ b/xen/common/dt-overlay.c
> > @@ -356,6 +356,42 @@ static int overlay_get_nodes_info(const void *fdto,
> > char **nodes_full_path)
> > return 0;
> > }
> > +/* This function should be called with the overlay_lock taken */
> > +static struct overlay_track *
> > +find_track_entry_from_tracker(const void *overlay_fdt,
> > + uint32_t overlay_fdt_size)
> > +{
> > + struct overlay_track *entry, *temp;
> > + bool found_entry = false;
> > +
> > + ASSERT(spin_is_locked(&overlay_lock));
> > +
> > + /*
> > + * First check if dtbo is correct i.e. it should one of the dtbo which
> > was
> > + * used when dynamically adding the node.
> > + * Limitation: Cases with same node names but different property are
> > not
> > + * supported currently. We are relying on user to provide the same dtbo
> > + * as it was used when adding the nodes.
> > + */
> > + list_for_each_entry_safe( entry, temp, &overlay_tracker, entry )
> > + {
> > + if ( memcmp(entry->overlay_fdt, overlay_fdt, overlay_fdt_size) == 0
> > )
> > + {
> > + found_entry = true;
> > + break;
> > + }
> > + }
> > +
> > + if ( !found_entry )
> > + {
> > + printk(XENLOG_ERR "Cannot find any matching tracker with input
> > dtbo."
> > + " Operation is supported only for prior added dtbo.\n");
> > + return NULL;
> > + }
> > +
> > + return entry;
> > +}
> > +
> > /* Check if node itself can be removed and remove node from IOMMU. */
> > static int remove_node_resources(struct dt_device_node *device_node)
> > {
> > @@ -485,8 +521,7 @@ static long handle_remove_overlay_nodes(const void
> > *overlay_fdt,
> > uint32_t overlay_fdt_size)
> > {
> > int rc;
> > - struct overlay_track *entry, *temp, *track;
> > - bool found_entry = false;
> > + struct overlay_track *entry;
> > rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size);
> > if ( rc )
> > @@ -494,29 +529,10 @@ static long handle_remove_overlay_nodes(const void
> > *overlay_fdt,
> > spin_lock(&overlay_lock);
> > - /*
> > - * First check if dtbo is correct i.e. it should one of the dtbo which
> > was
> > - * used when dynamically adding the node.
> > - * Limitation: Cases with same node names but different property are
> > not
> > - * supported currently. We are relying on user to provide the same dtbo
> > - * as it was used when adding the nodes.
> > - */
> > - list_for_each_entry_safe( entry, temp, &overlay_tracker, entry )
> > - {
> > - if ( memcmp(entry->overlay_fdt, overlay_fdt, overlay_fdt_size) == 0
> > )
> > - {
> > - track = entry;
> > - found_entry = true;
> > - break;
> > - }
> > - }
> > -
> > - if ( !found_entry )
> > + entry = find_track_entry_from_tracker(overlay_fdt, overlay_fdt_size);
> > + if ( entry == NULL )
> > {
> > rc = -EINVAL;
> > -
> > - printk(XENLOG_ERR "Cannot find any matching tracker with input
> > dtbo."
> > - " Removing nodes is supported only for prior added
> > dtbo.\n");
> > goto out;
> > }
> > @@ -620,15 +636,7 @@ static long add_nodes(struct overlay_track *tr, char
> > **nodes_full_path)
> > return -EFAULT;
> > }
> > - rc = handle_device(hardware_domain, overlay_node,
> > p2m_mmio_direct_c,
> > - tr->iomem_ranges,
> > - tr->irq_ranges);
> > write_unlock(&dt_host_lock);
> > - if ( rc )
> > - {
> > - printk(XENLOG_ERR "Adding IRQ and IOMMU failed\n");
> > - return rc;
> > - }
> > /* Keep overlay_node address in tracker. */
> > tr->nodes_address[j] = (unsigned long)overlay_node;
> > @@ -638,9 +646,7 @@ static long add_nodes(struct overlay_track *tr, char
> > **nodes_full_path)
> > }
> > /*
> > * Adds device tree nodes under target node.
> > - * We use tr->dt_host_new to unflatten the updated device_tree_flattened.
> > This
> > - * is done to avoid the removal of device_tree generation, iomem regions
> > mapping
> > - * to hardware domain done by handle_node().
> > + * We use tr->dt_host_new to unflatten the updated device_tree_flattened.
> > */
> > static long handle_add_overlay_nodes(void *overlay_fdt,
> > uint32_t overlay_fdt_size)
> > @@ -774,20 +780,6 @@ static long handle_add_overlay_nodes(void *overlay_fdt,
> > goto err;
> > }
> > - tr->irq_ranges = rangeset_new(hardware_domain, "Overlays:
> > Interrupts", 0);
> > - if (tr->irq_ranges == NULL)
> > - {
> > - printk(XENLOG_ERR "Creating IRQ rangeset failed");
> > - goto err;
> > - }
> > -
> > - tr->iomem_ranges = rangeset_new(hardware_domain, "Overlay: I/O Memory",
> > 0);
> > - if (tr->iomem_ranges == NULL)
> > - {
> > - printk(XENLOG_ERR "Creating IOMMU rangeset failed");
> > - goto err;
> > - }
> > -
> > rc = add_nodes(tr, nodes_full_path);
> > if ( rc )
> > {
> > @@ -843,14 +835,83 @@ static long handle_add_overlay_nodes(void
> > *overlay_fdt,
> > xfree(tr->nodes_address);
> > xfree(tr->fdt);
> > - rangeset_destroy(tr->irq_ranges);
> > - rangeset_destroy(tr->iomem_ranges);
> > -
> > xfree(tr);
> > return rc;
> > }
> > +static long handle_attach_overlay_nodes(struct domain *d,
> > + const void *overlay_fdt,
> > + uint32_t overlay_fdt_size)
> > +{
> > + int rc;
> > + unsigned int j;
> > + struct overlay_track *entry;
> > +
> > + rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size);
> > + if ( rc )
> > + return rc;
> > +
> > + spin_lock(&overlay_lock);
> > +
> > + entry = find_track_entry_from_tracker(overlay_fdt, overlay_fdt_size);
> > + if ( entry == NULL )
> > + {
> > + rc = -EINVAL;
> > + goto out;
> > + }
> > +
> > + entry->irq_ranges = rangeset_new(d, "Overlays: Interrupts", 0);
> > + if (entry->irq_ranges == NULL)
> > + {
> > + rc = -ENOMEM;
> > + printk(XENLOG_ERR "Creating IRQ rangeset failed");
> > + goto out;
> > + }
> > +
> > + entry->iomem_ranges = rangeset_new(d, "Overlay: I/O Memory", 0);
> > + if (entry->iomem_ranges == NULL)
> > + {
> > + rc = -ENOMEM;
> > + printk(XENLOG_ERR "Creating IOMMU rangeset failed");
> > + goto out;
> > + }
> > +
> > + for ( j = 0; j < entry->num_nodes; j++ )
> > + {
> > + struct dt_device_node *overlay_node;
> > +
> > + overlay_node = (struct dt_device_node *)entry->nodes_address[j];
> > + if ( overlay_node == NULL )
> > + {
> > + rc = -EINVAL;
> > + goto out;
> > + }
> > +
> > + write_lock(&dt_host_lock);
> > + rc = handle_device(d, overlay_node, p2m_mmio_direct_c,
> > + entry->iomem_ranges, entry->irq_ranges);
> > + write_unlock(&dt_host_lock);
> > + if ( rc )
> > + {
> > + printk(XENLOG_ERR "Adding IRQ and IOMMU failed\n");
> > + goto out;
> > + }
> > + }
> > +
> > + spin_unlock(&overlay_lock);
> > +
> > + return 0;
> > +
> > + out:
> > + spin_unlock(&overlay_lock);
> > +
> > + rangeset_destroy(entry->irq_ranges);
> > + rangeset_destroy(entry->iomem_ranges);
> > +
> > + return rc;
> > +}
> > +
> > long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op)
> > {
> > long ret;
> > @@ -890,6 +951,42 @@ long dt_overlay_sysctl(struct xen_sysctl_dt_overlay
> > *op)
> > return ret;
> > }
> > +long dt_overlay_domctl(struct domain *d, struct xen_domctl_dt_overlay
> > *op)
> > +{
> > + long ret;
> > + void *overlay_fdt;
> > +
> > + if ( op->overlay_op != XEN_DOMCTL_DT_OVERLAY_ATTACH )
> > + return -EOPNOTSUPP;
> > +
> > + if ( op->overlay_fdt_size == 0 || op->overlay_fdt_size > KB(500) )
>
> Let's avoid to hardocode KB(500) a second time (it alredy exists in the sysctl
> path).
>
> But I would actually consider to allow up to 512KB because...
I left it as is because I wasn't sure what you wanted as a change. Did
you mean that you would like the check to be removed, or did you mean
that you would like an #define instead of writing KB(500)?
It is fine either way for me, or to keep it as is, please make the
desired change on commit.
> > + return -EINVAL;
> > +
> > + if ( op->pad[0] || op->pad[1] || op->pad[2] )
> > + return -EINVAL;
> > +
> > + overlay_fdt = xmalloc_bytes(op->overlay_fdt_size);
>
> ... xmalloc_bytes() will be more efficient. If the size is not power of 2
> pages, it will free the leftover. Anyway, this could be a follow-up patch.
Leaving this as is for now
> > +
> > + if ( overlay_fdt == NULL )
> > + return -ENOMEM;
> > +
> > + ret = copy_from_guest(overlay_fdt, op->overlay_fdt,
> > op->overlay_fdt_size);
> > + if ( ret )
> > + {
> > + gprintk(XENLOG_ERR, "copy from guest failed\n");
> > + xfree(overlay_fdt);
> > +
> > + return -EFAULT;
> > + }
> > +
> > + if ( op->overlay_op == XEN_DOMCTL_DT_OVERLAY_ATTACH )
> > + ret = handle_attach_overlay_nodes(d, overlay_fdt,
> > op->overlay_fdt_size);
>
> I think you would return 0 if the operation is not supported. But I think we
> need to return -EOPNOTSUPP.
I made the change
> > +
> > + xfree(overlay_fdt);
> > +
> > + return ret;
> > +}
> > +
> > /*
> > * Local variables:
> > * mode: C
> > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> > index a33f9ec32b..ac3c2a7c4c 100644
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
>
> If I am not mistkane, this is the first change in the domctl header for 4.19.
> So you want to bump XEN_DOMCTL_INTERFACE_VERSION.
Done
> > @@ -1190,6 +1190,16 @@ struct xen_domctl_vmtrace_op {
> > typedef struct xen_domctl_vmtrace_op xen_domctl_vmtrace_op_t;
> > DEFINE_XEN_GUEST_HANDLE(xen_domctl_vmtrace_op_t);
> > +#if defined(__arm__) || defined(__aarch64__)
> > +struct xen_domctl_dt_overlay {
> > + XEN_GUEST_HANDLE_64(const_void) overlay_fdt; /* IN: overlay fdt. */
> > + uint32_t overlay_fdt_size; /* IN: Overlay dtb size. */
> > +#define XEN_DOMCTL_DT_OVERLAY_ATTACH 1
>
> OOI, why not starting from 0?
I think it was to avoid the risk of confusing zero as uninitialized with
a real operation.
> > + uint8_t overlay_op; /* IN: Attach. */
> > + uint8_t pad[3]; /* IN: Must be zero. */
> > +};
> > +#endif
> > +
> > struct xen_domctl {
> > uint32_t cmd;
> > #define XEN_DOMCTL_createdomain 1
> > @@ -1277,6 +1287,7 @@ struct xen_domctl {
> > #define XEN_DOMCTL_vmtrace_op 84
> > #define XEN_DOMCTL_get_paging_mempool_size 85
> > #define XEN_DOMCTL_set_paging_mempool_size 86
> > +#define XEN_DOMCTL_dt_overlay 87
> > #define XEN_DOMCTL_gdbsx_guestmemio 1000
> > #define XEN_DOMCTL_gdbsx_pausevcpu 1001
> > #define XEN_DOMCTL_gdbsx_unpausevcpu 1002
> > @@ -1339,6 +1350,9 @@ struct xen_domctl {
> > struct xen_domctl_vuart_op vuart_op;
> > struct xen_domctl_vmtrace_op vmtrace_op;
> > struct xen_domctl_paging_mempool paging_mempool;
> > +#if defined(__arm__) || defined(__aarch64__)
> > + struct xen_domctl_dt_overlay dt_overlay;
> > +#endif
> > uint8_t pad[128];
> > } u;
> > };
> > diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
> > index febaa4b16a..3a6e7d48f0 100644
> > --- a/xen/include/public/sysctl.h
> > +++ b/xen/include/public/sysctl.h
> > @@ -1184,14 +1184,11 @@ typedef struct xen_sysctl_cpu_policy
> > xen_sysctl_cpu_policy_t;
> > DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpu_policy_t);
> > #endif
> > -#if defined(__arm__) || defined (__aarch64__)
> > +#if defined(__arm__) || defined(__aarch64__)
>
> This seems like unrelated change. Maybe mention in the commit message that you
> are fixing the coding style?
Done
> > /*
> > * XEN_SYSCTL_dt_overlay
> > - * Performs addition/removal of device tree nodes under parent node using
> > dtbo.
> > - * This does in three steps:
> > - * - Adds/Removes the nodes from dt_host.
> > - * - Adds/Removes IRQ permission for the nodes.
> > - * - Adds/Removes MMIO accesses.
> > + * Performs addition/removal of device tree nodes under parent node using
> > dtbo
> > + * from dt_host.
> > */
> > struct xen_sysctl_dt_overlay {
> > XEN_GUEST_HANDLE_64(const_void) overlay_fdt; /* IN: overlay fdt. */
> > @@ -1265,7 +1262,7 @@ struct xen_sysctl {
> > struct xen_sysctl_cpu_policy cpu_policy;
> > #endif
> > -#if defined(__arm__) || defined (__aarch64__)
> > +#if defined(__arm__) || defined(__aarch64__)
> > struct xen_sysctl_dt_overlay dt_overlay;
> > #endif
> > uint8_t pad[128];
> > diff --git a/xen/include/xen/dt-overlay.h b/xen/include/xen/dt-overlay.h
> > index c0567741ee..7f0f3741b5 100644
> > --- a/xen/include/xen/dt-overlay.h
> > +++ b/xen/include/xen/dt-overlay.h
> > @@ -39,15 +39,22 @@ struct overlay_track {
> > };
> > struct xen_sysctl_dt_overlay;
> > +struct xen_domctl_dt_overlay;
> > #ifdef CONFIG_OVERLAY_DTB
> > long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op);
> > +long dt_overlay_domctl(struct domain *d, struct xen_domctl_dt_overlay *op);
> > #else
> > #include <xen/errno.h>
> > static inline long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op)
> > {
> > return -EOPNOTSUPP;
> > }
>
> NIT: Newline here please.
Done
>
> > +static inline long dt_overlay_domctl(struct domain *d,
> > + struct xen_domctl_dt_overlay *op)
> > +{
> > + return -EOPNOTSUPP;
> > +}
> > #endif
> > #endif /* __XEN_DT_OVERLAY_H__ */
>
> Cheers,
>
> --
> Julien Grall
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v4 6/9] xen/arm/gic: Allow removing interrupt to running VMs
2024-05-23 7:40 [PATCH v4 0/9] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
` (4 preceding siblings ...)
2024-05-23 7:40 ` [PATCH v4 5/9] xen/arm: Add XEN_DOMCTL_dt_overlay and device attachment to domains Henry Wang
@ 2024-05-23 7:40 ` Henry Wang
2024-05-23 7:40 ` [PATCH v4 7/9] xen/arm: Support device detachment from domains Henry Wang
` (3 subsequent siblings)
9 siblings, 0 replies; 23+ messages in thread
From: Henry Wang @ 2024-05-23 7:40 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Henry Wang
From: Vikram Garhwal <fnu.vikram@xilinx.com>
Currently, removing physical interrupts are only allowed at
the domain destroy time. For use cases such as dynamic device
tree overlay removing, the removing of physical IRQ to
running domains should be allowed.
Move the above-mentioned domain dying check to vgic_connect_hw_irq().
Similarly as the routing interrupt to running domains, reject the
operation if the IRQ is active or pending in the guest. Do it for
both new and old vGIC implementations. Since now vgic_connect_hw_irq()
may reject the invalid operation case, move the clear of
_IRQ_INPROGRESS flag in gic_remove_irq_from_guest() to after the
successful execution of vgic_connect_hw_irq().
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>
---
v4:
- Split the original patch, only do the removing IRQ stuff in this
patch.
- Move the clear of _IRQ_INPROGRESS flag in gic_remove_irq_from_guest()
to after the successful execution of vgic_connect_hw_irq().
- Special case the d->is_dying check.
---
xen/arch/arm/gic-vgic.c | 27 ++++++++++++++++++++++++---
xen/arch/arm/gic.c | 9 +--------
xen/arch/arm/vgic/vgic.c | 24 ++++++++++++++++++++----
3 files changed, 45 insertions(+), 15 deletions(-)
diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
index b99e287224..56b6a3d5b0 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -439,6 +439,14 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, unsigned int virq,
/* We are taking to rank lock to prevent parallel connections. */
vgic_lock_rank(v_target, rank, flags);
+ /* Return with error if the IRQ is being migrated. */
+ if( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
+ {
+ vgic_unlock_rank(v_target, rank, flags);
+ return -EBUSY;
+ }
+
+ spin_lock(&v_target->arch.vgic.lock);
if ( connect )
{
@@ -456,12 +464,25 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, unsigned int virq,
}
else
{
- if ( desc && p->desc != desc )
- ret = -EINVAL;
+ if ( d->is_dying )
+ {
+ if ( desc && p->desc != desc )
+ ret = -EINVAL;
+ else
+ p->desc = NULL;
+ }
else
- p->desc = NULL;
+ {
+ if ( (desc && p->desc != desc) ||
+ test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ||
+ test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
+ ret = -EINVAL;
+ else
+ p->desc = NULL;
+ }
}
+ spin_unlock(&v_target->arch.vgic.lock);
vgic_unlock_rank(v_target, rank, flags);
return ret;
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index b3467a76ae..8633f14bdd 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -159,24 +159,17 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
ASSERT(test_bit(_IRQ_GUEST, &desc->status));
ASSERT(!is_lpi(virq));
- /*
- * Removing an interrupt while the domain is running may have
- * undesirable effect on the vGIC emulation.
- */
- if ( !d->is_dying )
- return -EBUSY;
-
desc->handler->shutdown(desc);
/* EOI the IRQ if it has not been done by the guest */
if ( test_bit(_IRQ_INPROGRESS, &desc->status) )
gic_hw_ops->deactivate_irq(desc);
- clear_bit(_IRQ_INPROGRESS, &desc->status);
ret = vgic_connect_hw_irq(d, NULL, virq, desc, false);
if ( ret )
return ret;
+ clear_bit(_IRQ_INPROGRESS, &desc->status);
clear_bit(_IRQ_GUEST, &desc->status);
desc->handler = &no_irq_type;
diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
index 048e12c562..0c324b58f7 100644
--- a/xen/arch/arm/vgic/vgic.c
+++ b/xen/arch/arm/vgic/vgic.c
@@ -890,14 +890,30 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *vcpu,
}
else /* remove a mapped IRQ */
{
- if ( desc && irq->hwintid != desc->irq )
+ if ( d->is_dying )
{
- ret = -EINVAL;
+ if ( desc && irq->hwintid != desc->irq )
+ {
+ ret = -EINVAL;
+ }
+ else
+ {
+ irq->hw = false;
+ irq->hwintid = 0;
+ }
}
else
{
- irq->hw = false;
- irq->hwintid = 0;
+ if ( (desc && irq->hwintid != desc->irq) ||
+ irq->active || irq->pending_latch )
+ {
+ ret = -EINVAL;
+ }
+ else
+ {
+ irq->hw = false;
+ irq->hwintid = 0;
+ }
}
}
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v4 7/9] xen/arm: Support device detachment from domains
2024-05-23 7:40 [PATCH v4 0/9] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
` (5 preceding siblings ...)
2024-05-23 7:40 ` [PATCH v4 6/9] xen/arm/gic: Allow removing interrupt to running VMs Henry Wang
@ 2024-05-23 7:40 ` Henry Wang
2024-05-23 22:42 ` Julien Grall
2024-05-23 7:40 ` [PATCH v4 8/9] tools: Introduce the "xl dt-overlay {attach,detach}" commands Henry Wang
` (2 subsequent siblings)
9 siblings, 1 reply; 23+ messages in thread
From: Henry Wang @ 2024-05-23 7:40 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Andrew Cooper, George Dunlap,
Jan Beulich, Henry Wang
Similarly as the device attachment from DT overlay to domain, this
commit implements the device detachment from domain. The DOMCTL
XEN_DOMCTL_dt_overlay op is extended to have the operation
XEN_DOMCTL_DT_OVERLAY_DETACH. The detachment of the device is
implemented by unmapping the IRQ and IOMMU resources. Note that with
these changes, the device de-registration from the IOMMU driver should
only happen at the time when the DT overlay is removed from the Xen
device tree.
Signed-off-by: Henry Wang <xin.wang2@amd.com>
Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
---
v4:
- Split the original patch, only do device detachment from domain.
---
xen/common/dt-overlay.c | 243 ++++++++++++++++++++++++++++--------
xen/include/public/domctl.h | 3 +-
2 files changed, 194 insertions(+), 52 deletions(-)
diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c
index 1087f9b502..693b6e4777 100644
--- a/xen/common/dt-overlay.c
+++ b/xen/common/dt-overlay.c
@@ -392,24 +392,100 @@ find_track_entry_from_tracker(const void *overlay_fdt,
return entry;
}
+static int remove_irq(unsigned long s, unsigned long e, void *data)
+{
+ struct domain *d = data;
+ int rc = 0;
+
+ /*
+ * IRQ should always have access unless there are duplication of
+ * of irqs in device tree. There are few cases of xen device tree
+ * where there are duplicate interrupts for the same node.
+ */
+ if (!irq_access_permitted(d, s))
+ return 0;
+ /*
+ * TODO: We don't handle shared IRQs for now. So, it is assumed that
+ * the IRQs was not shared with another domain.
+ */
+ rc = irq_deny_access(d, s);
+ if ( rc )
+ {
+ printk(XENLOG_ERR "unable to revoke access for irq %ld\n", s);
+ return rc;
+ }
+
+ rc = release_guest_irq(d, s);
+ if ( rc )
+ {
+ printk(XENLOG_ERR "unable to release irq %ld\n", s);
+ return rc;
+ }
+
+ return rc;
+}
+
+static int remove_all_irqs(struct rangeset *irq_ranges, struct domain *d)
+{
+ return rangeset_report_ranges(irq_ranges, 0, ~0UL, remove_irq, d);
+}
+
+static int remove_iomem(unsigned long s, unsigned long e, void *data)
+{
+ struct domain *d = data;
+ int rc = 0;
+ p2m_type_t t;
+ mfn_t mfn;
+
+ mfn = p2m_lookup(d, _gfn(s), &t);
+ if ( mfn_x(mfn) == 0 || mfn_x(mfn) == ~0UL )
+ return -EINVAL;
+
+ rc = iomem_deny_access(d, s, e);
+ if ( rc )
+ {
+ printk(XENLOG_ERR "Unable to remove %pd access to %#lx - %#lx\n",
+ d, s, e);
+ return rc;
+ }
+
+ rc = unmap_mmio_regions(d, _gfn(s), e - s, _mfn(s));
+ if ( rc )
+ return rc;
+
+ return rc;
+}
+
+static int remove_all_iomems(struct rangeset *iomem_ranges, struct domain *d)
+{
+ return rangeset_report_ranges(iomem_ranges, 0, ~0UL, remove_iomem, d);
+}
+
/* Check if node itself can be removed and remove node from IOMMU. */
-static int remove_node_resources(struct dt_device_node *device_node)
+static int remove_node_resources(struct dt_device_node *device_node,
+ struct domain *d)
{
int rc = 0;
unsigned int len;
domid_t domid;
- domid = dt_device_used_by(device_node);
+ if ( !d )
+ {
+ domid = dt_device_used_by(device_node);
- dt_dprintk("Checking if node %s is used by any domain\n",
- device_node->full_name);
+ dt_dprintk("Checking if node %s is used by any domain\n",
+ device_node->full_name);
- /* Remove the node if only it's assigned to hardware domain or domain io. */
- if ( domid != hardware_domain->domain_id && domid != DOMID_IO )
- {
- printk(XENLOG_ERR "Device %s is being used by domain %u. Removing nodes failed\n",
- device_node->full_name, domid);
- return -EINVAL;
+ /*
+ * We also check if device is assigned to DOMID_IO as when a domain
+ * is destroyed device is assigned to DOMID_IO.
+ */
+ if ( domid != DOMID_IO )
+ {
+ printk(XENLOG_ERR "Device %s is being assigned to %u. Device is assigned to %d\n",
+ device_node->full_name, DOMID_IO, domid);
+ return -EINVAL;
+ }
}
/* Check if iommu property exists. */
@@ -417,9 +493,12 @@ static int remove_node_resources(struct dt_device_node *device_node)
{
if ( dt_device_is_protected(device_node) )
{
- rc = iommu_remove_dt_device(device_node);
- if ( rc < 0 )
- return rc;
+ if ( !list_empty(&device_node->domain_list) )
+ {
+ rc = iommu_deassign_dt_device(d, device_node);
+ if ( rc < 0 )
+ return rc;
+ }
}
}
@@ -428,7 +507,8 @@ static int remove_node_resources(struct dt_device_node *device_node)
/* Remove all descendants from IOMMU. */
static int
-remove_descendant_nodes_resources(const struct dt_device_node *device_node)
+remove_descendant_nodes_resources(const struct dt_device_node *device_node,
+ struct domain *d)
{
int rc = 0;
struct dt_device_node *child_node;
@@ -438,12 +518,12 @@ remove_descendant_nodes_resources(const struct dt_device_node *device_node)
{
if ( child_node->child )
{
- rc = remove_descendant_nodes_resources(child_node);
+ rc = remove_descendant_nodes_resources(child_node, d);
if ( rc )
return rc;
}
- rc = remove_node_resources(child_node);
+ rc = remove_node_resources(child_node, d);
if ( rc )
return rc;
}
@@ -456,8 +536,7 @@ static int remove_nodes(const struct overlay_track *tracker)
{
int rc = 0;
struct dt_device_node *overlay_node;
- unsigned int j;
- struct domain *d = hardware_domain;
+ unsigned int j, len;
for ( j = 0; j < tracker->num_nodes; j++ )
{
@@ -467,18 +546,15 @@ static int remove_nodes(const struct overlay_track *tracker)
write_lock(&dt_host_lock);
- rc = remove_descendant_nodes_resources(overlay_node);
- if ( rc )
+ /* Check if iommu property exists. */
+ if ( dt_get_property(overlay_node, "iommus", &len) )
{
- write_unlock(&dt_host_lock);
- return rc;
- }
-
- rc = remove_node_resources(overlay_node);
- if ( rc )
- {
- write_unlock(&dt_host_lock);
- return rc;
+ if ( dt_device_is_protected(overlay_node) )
+ {
+ rc = iommu_remove_dt_device(overlay_node);
+ if ( rc < 0 )
+ return rc;
+ }
}
dt_dprintk("Removing node: %s\n", overlay_node->full_name);
@@ -493,22 +569,6 @@ static int remove_nodes(const struct overlay_track *tracker)
write_unlock(&dt_host_lock);
}
- /* Remove IRQ access. */
- if ( tracker->irq_ranges )
- {
- rc = rangeset_consume_ranges(tracker->irq_ranges, irq_remove_cb, d);
- if ( rc )
- return rc;
- }
-
- /* Remove mmio access. */
- if ( tracker->iomem_ranges )
- {
- rc = rangeset_consume_ranges(tracker->iomem_ranges, iomem_remove_cb, d);
- if ( rc )
- return rc;
- }
-
return rc;
}
@@ -534,7 +594,6 @@ static long handle_remove_overlay_nodes(const void *overlay_fdt,
{
rc = -EINVAL;
goto out;
-
}
rc = remove_nodes(entry);
@@ -552,9 +611,6 @@ static long handle_remove_overlay_nodes(const void *overlay_fdt,
xfree(entry->nodes_address);
- rangeset_destroy(entry->irq_ranges);
- rangeset_destroy(entry->iomem_ranges);
-
xfree(entry);
out:
@@ -840,6 +896,88 @@ static long handle_add_overlay_nodes(void *overlay_fdt,
return rc;
}
+static long handle_detach_overlay_nodes(struct domain *d,
+ const void *overlay_fdt,
+ uint32_t overlay_fdt_size)
+{
+ int rc;
+ unsigned int j;
+ struct overlay_track *entry;
+
+ rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size);
+ if ( rc )
+ return rc;
+
+ spin_lock(&overlay_lock);
+
+ entry = find_track_entry_from_tracker(overlay_fdt, overlay_fdt_size);
+ if ( entry == NULL )
+ {
+ rc = -EINVAL;
+ goto out;
+ }
+
+ for ( j = 0; j < entry->num_nodes; j++ )
+ {
+ struct dt_device_node *overlay_node;
+
+ overlay_node = (struct dt_device_node *)entry->nodes_address[j];
+ if ( overlay_node == NULL )
+ {
+ rc = -EINVAL;
+ goto out;
+ }
+
+ write_lock(&dt_host_lock);
+ rc = remove_descendant_nodes_resources(overlay_node, d);
+ if ( rc )
+ {
+ write_unlock(&dt_host_lock);
+ goto out;
+ }
+
+ rc = remove_node_resources(overlay_node, d);
+ if ( rc )
+ {
+ write_unlock(&dt_host_lock);
+ goto out;
+ }
+ write_unlock(&dt_host_lock);
+
+ rc = remove_all_irqs(entry->irq_ranges, d);
+ if ( rc )
+ goto out;
+
+ rc = remove_all_iomems(entry->iomem_ranges, d);
+ if ( rc )
+ goto out;
+ }
+
+ /* Remove IRQ access. */
+ if ( entry->irq_ranges )
+ {
+ rc = rangeset_consume_ranges(entry->irq_ranges, irq_remove_cb, d);
+ if ( rc )
+ goto out;
+ }
+
+ /* Remove mmio access. */
+ if ( entry->iomem_ranges )
+ {
+ rc = rangeset_consume_ranges(entry->iomem_ranges, iomem_remove_cb, d);
+ if ( rc )
+ goto out;
+ }
+
+ rangeset_destroy(entry->irq_ranges);
+ rangeset_destroy(entry->iomem_ranges);
+
+ out:
+ spin_unlock(&overlay_lock);
+
+ return rc;
+}
+
static long handle_attach_overlay_nodes(struct domain *d,
const void *overlay_fdt,
uint32_t overlay_fdt_size)
@@ -956,7 +1094,8 @@ long dt_overlay_domctl(struct domain *d, struct xen_domctl_dt_overlay *op)
long ret;
void *overlay_fdt;
- if ( op->overlay_op != XEN_DOMCTL_DT_OVERLAY_ATTACH )
+ if ( op->overlay_op != XEN_DOMCTL_DT_OVERLAY_ATTACH &&
+ op->overlay_op != XEN_DOMCTL_DT_OVERLAY_DETACH )
return -EOPNOTSUPP;
if ( op->overlay_fdt_size == 0 || op->overlay_fdt_size > KB(500) )
@@ -979,7 +1118,9 @@ long dt_overlay_domctl(struct domain *d, struct xen_domctl_dt_overlay *op)
return -EFAULT;
}
- if ( op->overlay_op == XEN_DOMCTL_DT_OVERLAY_ATTACH )
+ if ( op->overlay_op == XEN_DOMCTL_DT_OVERLAY_DETACH )
+ ret = handle_detach_overlay_nodes(d, overlay_fdt, op->overlay_fdt_size);
+ else
ret = handle_attach_overlay_nodes(d, overlay_fdt, op->overlay_fdt_size);
xfree(overlay_fdt);
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index ac3c2a7c4c..e91f743fa4 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1195,7 +1195,8 @@ struct xen_domctl_dt_overlay {
XEN_GUEST_HANDLE_64(const_void) overlay_fdt; /* IN: overlay fdt. */
uint32_t overlay_fdt_size; /* IN: Overlay dtb size. */
#define XEN_DOMCTL_DT_OVERLAY_ATTACH 1
- uint8_t overlay_op; /* IN: Attach. */
+#define XEN_DOMCTL_DT_OVERLAY_DETACH 2
+ uint8_t overlay_op; /* IN: Attach/Detach. */
uint8_t pad[3]; /* IN: Must be zero. */
};
#endif
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v4 7/9] xen/arm: Support device detachment from domains
2024-05-23 7:40 ` [PATCH v4 7/9] xen/arm: Support device detachment from domains Henry Wang
@ 2024-05-23 22:42 ` Julien Grall
0 siblings, 0 replies; 23+ messages in thread
From: Julien Grall @ 2024-05-23 22:42 UTC (permalink / raw)
To: Henry Wang, xen-devel
Cc: Stefano Stabellini, Andrew Cooper, George Dunlap, Jan Beulich
Hi Henry,
On 23/05/2024 08:40, Henry Wang wrote:
> Similarly as the device attachment from DT overlay to domain, this
> commit implements the device detachment from domain. The DOMCTL
> XEN_DOMCTL_dt_overlay op is extended to have the operation
> XEN_DOMCTL_DT_OVERLAY_DETACH. The detachment of the device is
> implemented by unmapping the IRQ and IOMMU resources. Note that with
> these changes, the device de-registration from the IOMMU driver should
> only happen at the time when the DT overlay is removed from the Xen
> device tree.
>
> Signed-off-by: Henry Wang <xin.wang2@amd.com>
> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
> ---
> v4:
> - Split the original patch, only do device detachment from domain.
> ---
> xen/common/dt-overlay.c | 243 ++++++++++++++++++++++++++++--------
> xen/include/public/domctl.h | 3 +-
> 2 files changed, 194 insertions(+), 52 deletions(-)
>
> diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c
> index 1087f9b502..693b6e4777 100644
> --- a/xen/common/dt-overlay.c
> +++ b/xen/common/dt-overlay.c
> @@ -392,24 +392,100 @@ find_track_entry_from_tracker(const void *overlay_fdt,
> return entry;
> }
>
> +static int remove_irq(unsigned long s, unsigned long e, void *data)
> +{
> + struct domain *d = data;
> + int rc = 0;
> +
> + /*
> + * IRQ should always have access unless there are duplication of
> + * of irqs in device tree. There are few cases of xen device tree
> + * where there are duplicate interrupts for the same node.
> + */
> + if (!irq_access_permitted(d, s))
Because of this check, it means that ...
> + return 0;
> + /*
> + * TODO: We don't handle shared IRQs for now. So, it is assumed that
> + * the IRQs was not shared with another domain.
> + */
> + rc = irq_deny_access(d, s);
> + if ( rc )
> + {
> + printk(XENLOG_ERR "unable to revoke access for irq %ld\n", s);
> + return rc;
> + }
> +
> + rc = release_guest_irq(d, s);
... release_guest_irq() fails on the next retry it will pass. I don't
think this is what we want.
Instead, we probably want to re-order the call.
> + if ( rc )
> + {
> + printk(XENLOG_ERR "unable to release irq %ld\n", s);
> + return rc;
> + }
> +
> + return rc;
> +}
> +
> +static int remove_all_irqs(struct rangeset *irq_ranges, struct domain *d)
> +{
> + return rangeset_report_ranges(irq_ranges, 0, ~0UL, remove_irq, d);
> +}
> +
> +static int remove_iomem(unsigned long s, unsigned long e, void *data)
> +{
> + struct domain *d = data;
> + int rc = 0;
> + p2m_type_t t;
> + mfn_t mfn;
> +
> + mfn = p2m_lookup(d, _gfn(s), &t);
What are you trying to addres with this check? For instance, the fact
that the first MFN is mapped, doesn't guarantee the rest is.
> + if ( mfn_x(mfn) == 0 || mfn_x(mfn) == ~0UL )
I don't understand why we are checking for 0 here. In theory, it is
valid MFN. Also, the second part wants to be INVALID_MFN.
> + return -EINVAL;
> +
> + rc = iomem_deny_access(d, s, e);
iomem_deny_access() works on MFN but here you pass an MFN. Are you
assuming the GFN == MFN? How would that work for domains that are not
direct mapped?
> + if ( rc )
> + {
> + printk(XENLOG_ERR "Unable to remove %pd access to %#lx - %#lx\n",
> + d, s, e);
> + return rc;
> + }
> +
> + rc = unmap_mmio_regions(d, _gfn(s), e - s, _mfn(s));
> + if ( rc )
> + return rc;
> +
> + return rc;
> +}
> +
> +static int remove_all_iomems(struct rangeset *iomem_ranges, struct domain *d)
> +{
> + return rangeset_report_ranges(iomem_ranges, 0, ~0UL, remove_iomem, d);
> +}
> +
> /* Check if node itself can be removed and remove node from IOMMU. */
> -static int remove_node_resources(struct dt_device_node *device_node)
> +static int remove_node_resources(struct dt_device_node *device_node,
> + struct domain *d)
> {
> int rc = 0;
> unsigned int len;
> domid_t domid;
>
> - domid = dt_device_used_by(device_node);
> + if ( !d )
I looked at the code, I am a bit unsure how "d" can be NULL. Do you have
any pointer?
> + {
> + domid = dt_device_used_by(device_node);
>
> - dt_dprintk("Checking if node %s is used by any domain\n",
> - device_node->full_name);
> + dt_dprintk("Checking if node %s is used by any domain\n",
> + device_node->full_name);
>
> - /* Remove the node if only it's assigned to hardware domain or domain io. */
> - if ( domid != hardware_domain->domain_id && domid != DOMID_IO )
> - {
> - printk(XENLOG_ERR "Device %s is being used by domain %u. Removing nodes failed\n",
> - device_node->full_name, domid);
> - return -EINVAL;
> + /*
> + * We also check if device is assigned to DOMID_IO as when a domain
> + * is destroyed device is assigned to DOMID_IO.
> + */
> + if ( domid != DOMID_IO )
> + {
> + printk(XENLOG_ERR "Device %s is being assigned to %u. Device is assigned to %d\n",
> + device_node->full_name, DOMID_IO, domid);
> + return -EINVAL;
> + }
> }
>
> /* Check if iommu property exists. */
> @@ -417,9 +493,12 @@ static int remove_node_resources(struct dt_device_node *device_node)
> {
> if ( dt_device_is_protected(device_node) )
> {
> - rc = iommu_remove_dt_device(device_node);
> - if ( rc < 0 )
> - return rc;
> + if ( !list_empty(&device_node->domain_list) )
> + {
> + rc = iommu_deassign_dt_device(d, device_node);
> + if ( rc < 0 )
> + return rc;
> + }
> }
> }
>
> @@ -428,7 +507,8 @@ static int remove_node_resources(struct dt_device_node *device_node)
>
> /* Remove all descendants from IOMMU. */
> static int
> -remove_descendant_nodes_resources(const struct dt_device_node *device_node)
> +remove_descendant_nodes_resources(const struct dt_device_node *device_node,
> + struct domain *d)
> {
> int rc = 0;
> struct dt_device_node *child_node;
> @@ -438,12 +518,12 @@ remove_descendant_nodes_resources(const struct dt_device_node *device_node)
> {
> if ( child_node->child )
> {
> - rc = remove_descendant_nodes_resources(child_node);
> + rc = remove_descendant_nodes_resources(child_node, d);
> if ( rc )
> return rc;
> }
>
> - rc = remove_node_resources(child_node);
> + rc = remove_node_resources(child_node, d);
> if ( rc )
> return rc;
> }
> @@ -456,8 +536,7 @@ static int remove_nodes(const struct overlay_track *tracker)
> {
> int rc = 0;
> struct dt_device_node *overlay_node;
> - unsigned int j;
> - struct domain *d = hardware_domain;
> + unsigned int j, len;
>
> for ( j = 0; j < tracker->num_nodes; j++ )
> {
> @@ -467,18 +546,15 @@ static int remove_nodes(const struct overlay_track *tracker)
>
> write_lock(&dt_host_lock);
>
> - rc = remove_descendant_nodes_resources(overlay_node);
> - if ( rc )
> + /* Check if iommu property exists. */
> + if ( dt_get_property(overlay_node, "iommus", &len) )
Why do we need to check for the property in the "iommus" rather than ...
> {
> - write_unlock(&dt_host_lock);
> - return rc;
> - }
> -
> - rc = remove_node_resources(overlay_node);
> - if ( rc )
> - {
> - write_unlock(&dt_host_lock);
> - return rc;
> + if ( dt_device_is_protected(overlay_node) )
... relying on dt_device_is_protected()?
> + {
> + rc = iommu_remove_dt_device(overlay_node);
> + if ( rc < 0 )
> + return rc;
> + }
> }
>
> dt_dprintk("Removing node: %s\n", overlay_node->full_name);
> @@ -493,22 +569,6 @@ static int remove_nodes(const struct overlay_track *tracker)
> write_unlock(&dt_host_lock);
> }
>
> - /* Remove IRQ access. */
> - if ( tracker->irq_ranges )
> - {
> - rc = rangeset_consume_ranges(tracker->irq_ranges, irq_remove_cb, d);
> - if ( rc )
> - return rc;
> - }
> -
> - /* Remove mmio access. */
> - if ( tracker->iomem_ranges )
> - {
> - rc = rangeset_consume_ranges(tracker->iomem_ranges, iomem_remove_cb, d);
> - if ( rc )
> - return rc;
> - }
> -
> return rc;
> }
>
> @@ -534,7 +594,6 @@ static long handle_remove_overlay_nodes(const void *overlay_fdt,
> {
> rc = -EINVAL;
> goto out;
> -
Valid but unrelated change?
> }
>
> rc = remove_nodes(entry);
> @@ -552,9 +611,6 @@ static long handle_remove_overlay_nodes(const void *overlay_fdt,
>
> xfree(entry->nodes_address);
>
> - rangeset_destroy(entry->irq_ranges);
> - rangeset_destroy(entry->iomem_ranges);
> -
> xfree(entry);
>
> out:
> @@ -840,6 +896,88 @@ static long handle_add_overlay_nodes(void *overlay_fdt,
> return rc;
> }
>
> +static long handle_detach_overlay_nodes(struct domain *d,
> + const void *overlay_fdt,
> + uint32_t overlay_fdt_size)
> +{
> + int rc;
> + unsigned int j;
> + struct overlay_track *entry;
> +
> + rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size);
> + if ( rc )
> + return rc;
> +
> + spin_lock(&overlay_lock);
> +
> + entry = find_track_entry_from_tracker(overlay_fdt, overlay_fdt_size);
> + if ( entry == NULL )
> + {
> + rc = -EINVAL;
> + goto out;
> + }
> +
> + for ( j = 0; j < entry->num_nodes; j++ )
> + {
> + struct dt_device_node *overlay_node;
> +
> + overlay_node = (struct dt_device_node *)entry->nodes_address[j];
Can you remind me why nodes_address is storing unsigned long rather
struct dt_device_node?
> + if ( overlay_node == NULL )
> + {
> + rc = -EINVAL;
> + goto out;
> + }
> +
> + write_lock(&dt_host_lock);
> + rc = remove_descendant_nodes_resources(overlay_node, d);
> + if ( rc )
> + {
> + write_unlock(&dt_host_lock);
> + goto out;
> + }
> +
> + rc = remove_node_resources(overlay_node, d);
> + if ( rc )
> + {
> + write_unlock(&dt_host_lock);
> + goto out;
> + }
> + write_unlock(&dt_host_lock);
> +
> + rc = remove_all_irqs(entry->irq_ranges, d);
I am really confused. This is called in the for loop but entry doesn't
seem to change. So why do we need to call it for every iteration? Also...
> + if ( rc )
> + goto out;
> +
> + rc = remove_all_iomems(entry->iomem_ranges, d);
> + if ( rc )
> + goto out;
> + }
> +
> + /* Remove IRQ access. */
> + if ( entry->irq_ranges )
> + {
> + rc = rangeset_consume_ranges(entry->irq_ranges, irq_remove_cb, d);
... remove_all_irqs() will revert the permission. So why do we need to
do it again?
> + if ( rc )
> + goto out;
> + }
> +
> + /* Remove mmio access. */
> + if ( entry->iomem_ranges )
> + {
> + rc = rangeset_consume_ranges(entry->iomem_ranges, iomem_remove_cb, d);
Same question here.
> + if ( rc )
> + goto out;
> + }
> +
> + rangeset_destroy(entry->irq_ranges);
> + rangeset_destroy(entry->iomem_ranges);
> +
> + out:
> + spin_unlock(&overlay_lock);
> +
> + return rc;
> +}
> +
> static long handle_attach_overlay_nodes(struct domain *d,
> const void *overlay_fdt,
> uint32_t overlay_fdt_size)
> @@ -956,7 +1094,8 @@ long dt_overlay_domctl(struct domain *d, struct xen_domctl_dt_overlay *op)
> long ret;
> void *overlay_fdt;
>
> - if ( op->overlay_op != XEN_DOMCTL_DT_OVERLAY_ATTACH )
> + if ( op->overlay_op != XEN_DOMCTL_DT_OVERLAY_ATTACH &&
> + op->overlay_op != XEN_DOMCTL_DT_OVERLAY_DETACH )
> return -EOPNOTSUPP;
>
> if ( op->overlay_fdt_size == 0 || op->overlay_fdt_size > KB(500) )
> @@ -979,7 +1118,9 @@ long dt_overlay_domctl(struct domain *d, struct xen_domctl_dt_overlay *op)
> return -EFAULT;
> }
>
> - if ( op->overlay_op == XEN_DOMCTL_DT_OVERLAY_ATTACH )
> + if ( op->overlay_op == XEN_DOMCTL_DT_OVERLAY_DETACH )
> + ret = handle_detach_overlay_nodes(d, overlay_fdt, op->overlay_fdt_size);
> + else
> ret = handle_attach_overlay_nodes(d, overlay_fdt, op->overlay_fdt_size);
>
> xfree(overlay_fdt);
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index ac3c2a7c4c..e91f743fa4 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1195,7 +1195,8 @@ struct xen_domctl_dt_overlay {
> XEN_GUEST_HANDLE_64(const_void) overlay_fdt; /* IN: overlay fdt. */
> uint32_t overlay_fdt_size; /* IN: Overlay dtb size. */
> #define XEN_DOMCTL_DT_OVERLAY_ATTACH 1
> - uint8_t overlay_op; /* IN: Attach. */
> +#define XEN_DOMCTL_DT_OVERLAY_DETACH 2
> + uint8_t overlay_op; /* IN: Attach/Detach. */
Changing the comment for every operation we add is not really scalable
:). If you want an explanation, can it be generic?
> uint8_t pad[3]; /* IN: Must be zero. */
> };
> #endif
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v4 8/9] tools: Introduce the "xl dt-overlay {attach,detach}" commands
2024-05-23 7:40 [PATCH v4 0/9] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
` (6 preceding siblings ...)
2024-05-23 7:40 ` [PATCH v4 7/9] xen/arm: Support device detachment from domains Henry Wang
@ 2024-05-23 7:40 ` Henry Wang
2024-05-23 23:11 ` Julien Grall
2024-05-23 7:40 ` [PATCH v4 9/9] docs: Add device tree overlay documentation Henry Wang
2024-05-23 21:29 ` [PATCH v4 0/9] Remaining patches for dynamic node programming using overlay dtbo Julien Grall
9 siblings, 1 reply; 23+ messages in thread
From: Henry Wang @ 2024-05-23 7:40 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Juergen Gross, Henry Wang, Jason Andryuk
With the XEN_DOMCTL_dt_overlay DOMCTL added, users should be able to
attach/detach devices from the provided DT overlay to domains.
Support this by introducing a new set of "xl dt-overlay" commands and
related documentation, i.e. "xl dt-overlay {attach,detach}". Slightly
rework the command option parsing logic.
Signed-off-by: Henry Wang <xin.wang2@amd.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
---
v4:
- Add Jason's Reviewed-by tag.
v3:
- Introduce new API libxl_dt_overlay_domain() and co., instead of
reusing existing API libxl_dt_overlay().
- Add in-code comments for the LIBXL_DT_OVERLAY_* macros.
- Use find_domain() to avoid getting domain_id from strtol().
v2:
- New patch.
---
tools/include/libxl.h | 10 +++++++
tools/include/xenctrl.h | 3 +++
tools/libs/ctrl/xc_dt_overlay.c | 31 +++++++++++++++++++++
tools/libs/light/libxl_dt_overlay.c | 28 +++++++++++++++++++
tools/xl/xl_cmdtable.c | 4 +--
tools/xl/xl_vmcontrol.c | 42 ++++++++++++++++++++---------
6 files changed, 104 insertions(+), 14 deletions(-)
diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index 62cb07dea6..6cc6d6bf6a 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -2549,8 +2549,18 @@ 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__)
+/* Values should keep consistent with the op from XEN_SYSCTL_dt_overlay */
+#define LIBXL_DT_OVERLAY_ADD 1
+#define LIBXL_DT_OVERLAY_REMOVE 2
int libxl_dt_overlay(libxl_ctx *ctx, void *overlay,
uint32_t overlay_size, uint8_t overlay_op);
+
+/* Values should keep consistent with the op from XEN_DOMCTL_dt_overlay */
+#define LIBXL_DT_OVERLAY_DOMAIN_ATTACH 1
+#define LIBXL_DT_OVERLAY_DOMAIN_DETACH 2
+int libxl_dt_overlay_domain(libxl_ctx *ctx, uint32_t domain_id,
+ void *overlay_dt, uint32_t overlay_dt_size,
+ uint8_t overlay_op);
#endif
/*
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 4996855944..9ceca0cffc 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2657,6 +2657,9 @@ int xc_domain_cacheflush(xc_interface *xch, uint32_t domid,
#if defined(__arm__) || defined(__aarch64__)
int xc_dt_overlay(xc_interface *xch, void *overlay_fdt,
uint32_t overlay_fdt_size, uint8_t overlay_op);
+int xc_dt_overlay_domain(xc_interface *xch, void *overlay_fdt,
+ uint32_t overlay_fdt_size, uint8_t overlay_op,
+ uint32_t domain_id);
#endif
/* Compat shims */
diff --git a/tools/libs/ctrl/xc_dt_overlay.c b/tools/libs/ctrl/xc_dt_overlay.c
index c2224c4d15..ea1da522d1 100644
--- a/tools/libs/ctrl/xc_dt_overlay.c
+++ b/tools/libs/ctrl/xc_dt_overlay.c
@@ -48,3 +48,34 @@ err:
return err;
}
+
+int xc_dt_overlay_domain(xc_interface *xch, void *overlay_fdt,
+ uint32_t overlay_fdt_size, uint8_t overlay_op,
+ uint32_t domain_id)
+{
+ int err;
+ struct xen_domctl domctl = {
+ .cmd = XEN_DOMCTL_dt_overlay,
+ .domain = domain_id,
+ .u.dt_overlay = {
+ .overlay_op = overlay_op,
+ .overlay_fdt_size = overlay_fdt_size,
+ }
+ };
+
+ DECLARE_HYPERCALL_BOUNCE(overlay_fdt, overlay_fdt_size,
+ XC_HYPERCALL_BUFFER_BOUNCE_IN);
+
+ if ( (err = xc_hypercall_bounce_pre(xch, overlay_fdt)) )
+ goto err;
+
+ set_xen_guest_handle(domctl.u.dt_overlay.overlay_fdt, overlay_fdt);
+
+ if ( (err = do_domctl(xch, &domctl)) != 0 )
+ PERROR("%s failed", __func__);
+
+err:
+ xc_hypercall_bounce_post(xch, overlay_fdt);
+
+ return err;
+}
diff --git a/tools/libs/light/libxl_dt_overlay.c b/tools/libs/light/libxl_dt_overlay.c
index a6c709a6dc..00503b76bd 100644
--- a/tools/libs/light/libxl_dt_overlay.c
+++ b/tools/libs/light/libxl_dt_overlay.c
@@ -69,3 +69,31 @@ out:
return rc;
}
+int libxl_dt_overlay_domain(libxl_ctx *ctx, uint32_t domain_id,
+ void *overlay_dt, uint32_t overlay_dt_size,
+ uint8_t overlay_op)
+{
+ int rc;
+ int r;
+ GC_INIT(ctx);
+
+ if (check_overlay_fdt(gc, overlay_dt, overlay_dt_size)) {
+ LOG(ERROR, "Overlay DTB check failed");
+ rc = ERROR_FAIL;
+ goto out;
+ } else {
+ LOG(DEBUG, "Overlay DTB check passed");
+ rc = 0;
+ }
+
+ r = xc_dt_overlay_domain(ctx->xch, overlay_dt, overlay_dt_size, overlay_op,
+ domain_id);
+ if (r) {
+ LOG(ERROR, "%s: Attaching/Detaching overlay dtb failed.", __func__);
+ rc = ERROR_FAIL;
+ }
+
+out:
+ GC_FREE;
+ return rc;
+}
diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
index 1f3c6b5897..37770b20e3 100644
--- a/tools/xl/xl_cmdtable.c
+++ b/tools/xl/xl_cmdtable.c
@@ -634,8 +634,8 @@ const struct cmd_spec cmd_table[] = {
#ifdef LIBXL_HAVE_DT_OVERLAY
{ "dt-overlay",
&main_dt_overlay, 0, 1,
- "Add/Remove a device tree overlay",
- "add/remove <.dtbo>",
+ "Add/Remove a device tree overlay to Xen device tree, attach/detach the device to a domain",
+ "<operation=add|remove> <.dtbo> OR <operation=attach|detach> <.dtbo> <Domain>",
"-h print this help\n"
},
#endif
diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index 02575d5d36..ccfeec39f1 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -1268,32 +1268,46 @@ int main_create(int argc, char **argv)
#ifdef LIBXL_HAVE_DT_OVERLAY
int main_dt_overlay(int argc, char **argv)
{
- const char *overlay_ops = NULL;
const char *overlay_config_file = NULL;
void *overlay_dtb = NULL;
int rc;
uint8_t op;
int overlay_dtb_size = 0;
- const int overlay_add_op = 1;
- const int overlay_remove_op = 2;
+ uint32_t domain_id = 0;
+ bool domain_op = false;
if (argc < 2) {
help("dt-overlay");
return EXIT_FAILURE;
}
- overlay_ops = argv[1];
- overlay_config_file = argv[2];
-
- if (strcmp(overlay_ops, "add") == 0)
- op = overlay_add_op;
- else if (strcmp(overlay_ops, "remove") == 0)
- op = overlay_remove_op;
- else {
+ if (strcmp(argv[optind], "add") == 0)
+ op = LIBXL_DT_OVERLAY_ADD;
+ else if (strcmp(argv[optind], "remove") == 0)
+ op = LIBXL_DT_OVERLAY_REMOVE;
+ else if (strcmp(argv[optind], "attach") == 0) {
+ op = LIBXL_DT_OVERLAY_DOMAIN_ATTACH;
+ domain_op = true;
+ } else if (strcmp(argv[optind], "detach") == 0) {
+ op = LIBXL_DT_OVERLAY_DOMAIN_DETACH;
+ domain_op = true;
+ } else {
fprintf(stderr, "Invalid dt overlay operation\n");
return EXIT_FAILURE;
}
+ overlay_config_file = argv[optind+1];
+
+ if (domain_op) {
+ if (argc <= optind + 2) {
+ fprintf(stderr, "Missing domain ID\n");
+ help("dt-overlay");
+ return EXIT_FAILURE;
+ } else {
+ domain_id = find_domain(argv[optind+2]);
+ }
+ }
+
if (overlay_config_file) {
rc = libxl_read_file_contents(ctx, overlay_config_file,
&overlay_dtb, &overlay_dtb_size);
@@ -1309,7 +1323,11 @@ int main_dt_overlay(int argc, char **argv)
return EXIT_FAILURE;
}
- rc = libxl_dt_overlay(ctx, overlay_dtb, overlay_dtb_size, op);
+ if (!domain_op)
+ rc = libxl_dt_overlay(ctx, overlay_dtb, overlay_dtb_size, op);
+ else
+ rc = libxl_dt_overlay_domain(ctx, domain_id, overlay_dtb,
+ overlay_dtb_size, op);
free(overlay_dtb);
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v4 8/9] tools: Introduce the "xl dt-overlay {attach,detach}" commands
2024-05-23 7:40 ` [PATCH v4 8/9] tools: Introduce the "xl dt-overlay {attach,detach}" commands Henry Wang
@ 2024-05-23 23:11 ` Julien Grall
2024-05-24 2:19 ` Stefano Stabellini
0 siblings, 1 reply; 23+ messages in thread
From: Julien Grall @ 2024-05-23 23:11 UTC (permalink / raw)
To: Henry Wang, xen-devel; +Cc: Anthony PERARD, Juergen Gross, Jason Andryuk
Hi Henry,
On 23/05/2024 08:40, Henry Wang wrote:
> With the XEN_DOMCTL_dt_overlay DOMCTL added, users should be able to
> attach/detach devices from the provided DT overlay to domains.
> Support this by introducing a new set of "xl dt-overlay" commands and
> related documentation, i.e. "xl dt-overlay {attach,detach}". Slightly
> rework the command option parsing logic.
>
> Signed-off-by: Henry Wang <xin.wang2@amd.com>
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
> ---
> v4:
> - Add Jason's Reviewed-by tag.
> v3:
> - Introduce new API libxl_dt_overlay_domain() and co., instead of
> reusing existing API libxl_dt_overlay().
> - Add in-code comments for the LIBXL_DT_OVERLAY_* macros.
> - Use find_domain() to avoid getting domain_id from strtol().
> v2:
> - New patch.
> ---
> tools/include/libxl.h | 10 +++++++
> tools/include/xenctrl.h | 3 +++
> tools/libs/ctrl/xc_dt_overlay.c | 31 +++++++++++++++++++++
> tools/libs/light/libxl_dt_overlay.c | 28 +++++++++++++++++++
> tools/xl/xl_cmdtable.c | 4 +--
> tools/xl/xl_vmcontrol.c | 42 ++++++++++++++++++++---------
> 6 files changed, 104 insertions(+), 14 deletions(-)
>
> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
> index 62cb07dea6..6cc6d6bf6a 100644
> --- a/tools/include/libxl.h
> +++ b/tools/include/libxl.h
I think you also need to introduce LIBXL_HAVE_...
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v4 8/9] tools: Introduce the "xl dt-overlay {attach,detach}" commands
2024-05-23 23:11 ` Julien Grall
@ 2024-05-24 2:19 ` Stefano Stabellini
0 siblings, 0 replies; 23+ messages in thread
From: Stefano Stabellini @ 2024-05-24 2:19 UTC (permalink / raw)
To: Julien Grall
Cc: Henry Wang, xen-devel, Anthony PERARD, Juergen Gross,
Jason Andryuk
On Fri, 24 May 2024, Julien Grall wrote:
> Hi Henry,
>
> On 23/05/2024 08:40, Henry Wang wrote:
> > With the XEN_DOMCTL_dt_overlay DOMCTL added, users should be able to
> > attach/detach devices from the provided DT overlay to domains.
> > Support this by introducing a new set of "xl dt-overlay" commands and
> > related documentation, i.e. "xl dt-overlay {attach,detach}". Slightly
> > rework the command option parsing logic.
> >
> > Signed-off-by: Henry Wang <xin.wang2@amd.com>
> > Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> > ---
> > v4:
> > - Add Jason's Reviewed-by tag.
> > v3:
> > - Introduce new API libxl_dt_overlay_domain() and co., instead of
> > reusing existing API libxl_dt_overlay().
> > - Add in-code comments for the LIBXL_DT_OVERLAY_* macros.
> > - Use find_domain() to avoid getting domain_id from strtol().
> > v2:
> > - New patch.
> > ---
> > tools/include/libxl.h | 10 +++++++
> > tools/include/xenctrl.h | 3 +++
> > tools/libs/ctrl/xc_dt_overlay.c | 31 +++++++++++++++++++++
> > tools/libs/light/libxl_dt_overlay.c | 28 +++++++++++++++++++
> > tools/xl/xl_cmdtable.c | 4 +--
> > tools/xl/xl_vmcontrol.c | 42 ++++++++++++++++++++---------
> > 6 files changed, 104 insertions(+), 14 deletions(-)
> >
> > diff --git a/tools/include/libxl.h b/tools/include/libxl.h
> > index 62cb07dea6..6cc6d6bf6a 100644
> > --- a/tools/include/libxl.h
> > +++ b/tools/include/libxl.h
>
> I think you also need to introduce LIBXL_HAVE_...
Added
I have removed the LIBXL_DT_OVERLAY_DOMAIN_DETACH and the relate
mentions. I kept Jasons' ack.
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v4 9/9] docs: Add device tree overlay documentation
2024-05-23 7:40 [PATCH v4 0/9] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
` (7 preceding siblings ...)
2024-05-23 7:40 ` [PATCH v4 8/9] tools: Introduce the "xl dt-overlay {attach,detach}" commands Henry Wang
@ 2024-05-23 7:40 ` Henry Wang
2024-05-23 22:48 ` Julien Grall
2024-05-23 21:29 ` [PATCH v4 0/9] Remaining patches for dynamic node programming using overlay dtbo Julien Grall
9 siblings, 1 reply; 23+ messages in thread
From: Henry Wang @ 2024-05-23 7:40 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Henry Wang
From: Vikram Garhwal <fnu.vikram@xilinx.com>
Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
Signed-off-by: Henry Wang <xin.wang2@amd.com>
---
v4:
- No change.
v3:
- No change.
v2:
- Update the content based on the changes in this version.
---
docs/misc/arm/overlay.txt | 99 +++++++++++++++++++++++++++++++++++++++
1 file changed, 99 insertions(+)
create mode 100644 docs/misc/arm/overlay.txt
diff --git a/docs/misc/arm/overlay.txt b/docs/misc/arm/overlay.txt
new file mode 100644
index 0000000000..811a6de369
--- /dev/null
+++ b/docs/misc/arm/overlay.txt
@@ -0,0 +1,99 @@
+# Device Tree Overlays support in Xen
+
+Xen now supports dynamic device assignment to running domains,
+i.e. adding/removing nodes (using .dtbo) to/from Xen device tree, and
+attaching/detaching them to/from a running domain with given $domid.
+
+Dynamic node assignment works in two steps:
+
+## Add/Remove device tree overlay to/from Xen device tree
+
+1. Xen tools check the dtbo given and parse all other user provided arguments
+2. Xen tools pass the dtbo to Xen hypervisor via hypercall.
+3. Xen hypervisor applies/removes the dtbo to/from Xen device tree.
+
+## Attach/Detach device from the DT overlay to/from domain
+
+1. Xen tools check the dtbo given and parse all other user provided arguments
+2. Xen tools pass the dtbo to Xen hypervisor via hypercall.
+3. Xen hypervisor attach/detach the device to/from the user-provided $domid by
+ mapping/unmapping node resources in the DT overlay.
+
+# Examples
+
+Here are a few examples on how to use it.
+
+## Dom0 device add
+
+For assigning a device tree overlay to Dom0, user should firstly properly
+prepare the DT overlay. More information about device tree overlays can be
+found in [1]. Then, in Dom0, enter the following:
+
+ (dom0) xl dt-overlay add overlay.dtbo
+
+This will allocate the devices mentioned in overlay.dtbo to Xen device tree.
+
+To assign the newly added device from the dtbo to Dom0:
+
+ (dom0) xl dt-overlay attach overlay.dtbo 0
+
+Next, if the user wants to add the same device tree overlay to dom0
+Linux, execute the following:
+
+ (dom0) mkdir -p /sys/kernel/config/device-tree/overlays/new_overlay
+ (dom0) cat overlay.dtbo > /sys/kernel/config/device-tree/overlays/new_overlay/dtbo
+
+Finally if needed, the relevant Linux kernel drive can be loaded using:
+
+ (dom0) modprobe module_name.ko
+
+## Dom0 device remove
+
+For removing the device from Dom0, first detach the device from Dom0:
+
+ (dom0) xl dt-overlay detach overlay.dtbo 0
+
+NOTE: The user is expected to unload any Linux kernel modules which
+might be accessing the devices in overlay.dtbo before detach the device.
+Detaching devices without unloading the modules might result in a crash.
+
+Then remove the overlay from Xen device tree:
+
+ (dom0) xl dt-overlay remove overlay.dtbo
+
+## DomU device add/remove
+
+All the nodes in dtbo will be assigned to a domain; the user will need
+to prepare the dtb for the domU. For example, the `interrupt-parent` property
+of the DomU overlay should be changed to the Xen hardcoded value `0xfde8`.
+Below assumes the properly written DomU dtbo is `overlay_domu.dtbo`.
+
+User will need to create the DomU with below properties properly configured
+in the xl config file:
+- `iomem`
+- `passthrough` (if IOMMU is needed)
+
+User will also need to modprobe the relevant drivers.
+
+Example for domU device add:
+
+ (dom0) xl dt-overlay add overlay.dtbo # If not executed before
+ (dom0) xl dt-overlay attach overlay.dtbo $domid
+ (dom0) xl console $domid # To access $domid console
+
+Next, if the user needs to modify/prepare the overlay.dtbo suitable for
+the domU:
+
+ (domU) mkdir -p /sys/kernel/config/device-tree/overlays/new_overlay
+ (domU) cat overlay_domu.dtbo > /sys/kernel/config/device-tree/overlays/new_overlay/dtbo
+
+Finally, if needed, the relevant Linux kernel drive can be probed:
+
+ (domU) modprobe module_name.ko
+
+Example for domU overlay remove:
+
+ (dom0) xl dt-overlay detach overlay.dtbo $domid
+ (dom0) xl dt-overlay remove overlay.dtbo
+
+[1] https://www.kernel.org/doc/Documentation/devicetree/overlay-notes.txt
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v4 9/9] docs: Add device tree overlay documentation
2024-05-23 7:40 ` [PATCH v4 9/9] docs: Add device tree overlay documentation Henry Wang
@ 2024-05-23 22:48 ` Julien Grall
2024-05-24 2:19 ` Stefano Stabellini
0 siblings, 1 reply; 23+ messages in thread
From: Julien Grall @ 2024-05-23 22:48 UTC (permalink / raw)
To: Henry Wang, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
Hi Henry,
On 23/05/2024 08:40, Henry Wang wrote:
> 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>
> ---
> v4:
> - No change.
> v3:
> - No change.
> v2:
> - Update the content based on the changes in this version.
> ---
> docs/misc/arm/overlay.txt | 99 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 99 insertions(+)
> create mode 100644 docs/misc/arm/overlay.txt
>
> diff --git a/docs/misc/arm/overlay.txt b/docs/misc/arm/overlay.txt
> new file mode 100644
> index 0000000000..811a6de369
> --- /dev/null
> +++ b/docs/misc/arm/overlay.txt
> @@ -0,0 +1,99 @@
> +# Device Tree Overlays support in Xen
> +
> +Xen now supports dynamic device assignment to running domains,
This reads as we "support" the feature. I would prefer if we write "Xen
expirementally supports..." or similar.
> +i.e. adding/removing nodes (using .dtbo) to/from Xen device tree, and
> +attaching/detaching them to/from a running domain with given $domid.
> +
> +Dynamic node assignment works in two steps:
> +
> +## Add/Remove device tree overlay to/from Xen device tree
> +
> +1. Xen tools check the dtbo given and parse all other user provided arguments
> +2. Xen tools pass the dtbo to Xen hypervisor via hypercall.
> +3. Xen hypervisor applies/removes the dtbo to/from Xen device tree.
> +
> +## Attach/Detach device from the DT overlay to/from domain
> +
> +1. Xen tools check the dtbo given and parse all other user provided arguments
> +2. Xen tools pass the dtbo to Xen hypervisor via hypercall.
> +3. Xen hypervisor attach/detach the device to/from the user-provided $domid by
> + mapping/unmapping node resources in the DT overlay.
> +
> +# Examples
> +
> +Here are a few examples on how to use it.
> +
> +## Dom0 device add
> +
> +For assigning a device tree overlay to Dom0, user should firstly properly
> +prepare the DT overlay. More information about device tree overlays can be
> +found in [1]. Then, in Dom0, enter the following:
> +
> + (dom0) xl dt-overlay add overlay.dtbo
> +
> +This will allocate the devices mentioned in overlay.dtbo to Xen device tree.
> +
> +To assign the newly added device from the dtbo to Dom0:
> +
> + (dom0) xl dt-overlay attach overlay.dtbo 0
> +
> +Next, if the user wants to add the same device tree overlay to dom0
> +Linux, execute the following:
> +
> + (dom0) mkdir -p /sys/kernel/config/device-tree/overlays/new_overlay
> + (dom0) cat overlay.dtbo > /sys/kernel/config/device-tree/overlays/new_overlay/dtbo
> +
> +Finally if needed, the relevant Linux kernel drive can be loaded using:
> +
> + (dom0) modprobe module_name.ko
> +
> +## Dom0 device remove
> +
> +For removing the device from Dom0, first detach the device from Dom0:
> +
> + (dom0) xl dt-overlay detach overlay.dtbo 0
> +
> +NOTE: The user is expected to unload any Linux kernel modules which
> +might be accessing the devices in overlay.dtbo before detach the device.
> +Detaching devices without unloading the modules might result in a crash.
> +
> +Then remove the overlay from Xen device tree:
> +
> + (dom0) xl dt-overlay remove overlay.dtbo
> +
> +## DomU device add/remove
> +
> +All the nodes in dtbo will be assigned to a domain; the user will need
> +to prepare the dtb for the domU. For example, the `interrupt-parent` property
> +of the DomU overlay should be changed to the Xen hardcoded value `0xfde8`.
> +Below assumes the properly written DomU dtbo is `overlay_domu.dtbo`.
> +
> +User will need to create the DomU with below properties properly configured
> +in the xl config file:
> +- `iomem`
I don't quite understand how the user can specify the MMIO region if the
device is attached after the domain is created.
> +- `passthrough` (if IOMMU is needed)
> +
> +User will also need to modprobe the relevant drivers.
> +
> +Example for domU device add:
> +
> + (dom0) xl dt-overlay add overlay.dtbo # If not executed before
> + (dom0) xl dt-overlay attach overlay.dtbo $domid
Can how clarify how the MMIO will be mapped? Is it direct mapped? If so,
couldn't this result to clash with other part of the address space (e.g.
RAM?).
> + (dom0) xl console $domid # To access $domid console
> +
> +Next, if the user needs to modify/prepare the overlay.dtbo suitable for
> +the domU:
> +
> + (domU) mkdir -p /sys/kernel/config/device-tree/overlays/new_overlay
> + (domU) cat overlay_domu.dtbo > /sys/kernel/config/device-tree/overlays/new_overlay/dtbo
> +
> +Finally, if needed, the relevant Linux kernel drive can be probed:
> +
> + (domU) modprobe module_name.ko
> +
> +Example for domU overlay remove:
> +
> + (dom0) xl dt-overlay detach overlay.dtbo $domid
> + (dom0) xl dt-overlay remove overlay.dtbo
I assume we have safety check in place to ensure we can't remove the
device if it is already attached. Is that correct?
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 9/9] docs: Add device tree overlay documentation
2024-05-23 22:48 ` Julien Grall
@ 2024-05-24 2:19 ` Stefano Stabellini
0 siblings, 0 replies; 23+ messages in thread
From: Stefano Stabellini @ 2024-05-24 2:19 UTC (permalink / raw)
To: Julien Grall
Cc: Henry Wang, xen-devel, Stefano Stabellini, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk
On Thu, 23 May 2024, Julien Grall wrote:
> Hi Henry,
>
> On 23/05/2024 08:40, Henry Wang wrote:
> > 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>
> > ---
> > v4:
> > - No change.
> > v3:
> > - No change.
> > v2:
> > - Update the content based on the changes in this version.
> > ---
> > docs/misc/arm/overlay.txt | 99 +++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 99 insertions(+)
> > create mode 100644 docs/misc/arm/overlay.txt
> >
> > diff --git a/docs/misc/arm/overlay.txt b/docs/misc/arm/overlay.txt
> > new file mode 100644
> > index 0000000000..811a6de369
> > --- /dev/null
> > +++ b/docs/misc/arm/overlay.txt
> > @@ -0,0 +1,99 @@
> > +# Device Tree Overlays support in Xen
> > +
> > +Xen now supports dynamic device assignment to running domains,
>
> This reads as we "support" the feature. I would prefer if we write "Xen
> expirementally supports..." or similar.
Done
> > +i.e. adding/removing nodes (using .dtbo) to/from Xen device tree, and
> > +attaching/detaching them to/from a running domain with given $domid.
> > +
> > +Dynamic node assignment works in two steps:
> > +
> > +## Add/Remove device tree overlay to/from Xen device tree
> > +
> > +1. Xen tools check the dtbo given and parse all other user provided
> > arguments
> > +2. Xen tools pass the dtbo to Xen hypervisor via hypercall.
> > +3. Xen hypervisor applies/removes the dtbo to/from Xen device tree.
> > +
> > +## Attach/Detach device from the DT overlay to/from domain
> > +
> > +1. Xen tools check the dtbo given and parse all other user provided
> > arguments
> > +2. Xen tools pass the dtbo to Xen hypervisor via hypercall.
> > +3. Xen hypervisor attach/detach the device to/from the user-provided $domid
> > by
> > + mapping/unmapping node resources in the DT overlay.
> > +
> > +# Examples
> > +
> > +Here are a few examples on how to use it.
> > +
> > +## Dom0 device add
> > +
> > +For assigning a device tree overlay to Dom0, user should firstly properly
> > +prepare the DT overlay. More information about device tree overlays can be
> > +found in [1]. Then, in Dom0, enter the following:
> > +
> > + (dom0) xl dt-overlay add overlay.dtbo
> > +
> > +This will allocate the devices mentioned in overlay.dtbo to Xen device
> > tree.
> > +
> > +To assign the newly added device from the dtbo to Dom0:
> > +
> > + (dom0) xl dt-overlay attach overlay.dtbo 0
> > +
> > +Next, if the user wants to add the same device tree overlay to dom0
> > +Linux, execute the following:
> > +
> > + (dom0) mkdir -p /sys/kernel/config/device-tree/overlays/new_overlay
> > + (dom0) cat overlay.dtbo >
> > /sys/kernel/config/device-tree/overlays/new_overlay/dtbo
> > +
> > +Finally if needed, the relevant Linux kernel drive can be loaded using:
> > +
> > + (dom0) modprobe module_name.ko
> > +
> > +## Dom0 device remove
> > +
> > +For removing the device from Dom0, first detach the device from Dom0:
> > +
> > + (dom0) xl dt-overlay detach overlay.dtbo 0
> > +
> > +NOTE: The user is expected to unload any Linux kernel modules which
> > +might be accessing the devices in overlay.dtbo before detach the device.
> > +Detaching devices without unloading the modules might result in a crash.
> > +
> > +Then remove the overlay from Xen device tree:
> > +
> > + (dom0) xl dt-overlay remove overlay.dtbo
> > +
> > +## DomU device add/remove
> > +
> > +All the nodes in dtbo will be assigned to a domain; the user will need
> > +to prepare the dtb for the domU. For example, the `interrupt-parent`
> > property
> > +of the DomU overlay should be changed to the Xen hardcoded value `0xfde8`.
> > +Below assumes the properly written DomU dtbo is `overlay_domu.dtbo`.
> > +
> > +User will need to create the DomU with below properties properly configured
> > +in the xl config file:
> > +- `iomem`
>
> I don't quite understand how the user can specify the MMIO region if the
> device is attached after the domain is created.
I think this was meant for a domain about to be created (not already
running). I clarified.
>
> > +- `passthrough` (if IOMMU is needed)
> > +
> > +User will also need to modprobe the relevant drivers.
> > +
> > +Example for domU device add:
> > +
> > + (dom0) xl dt-overlay add overlay.dtbo # If not executed
> > before
> > + (dom0) xl dt-overlay attach overlay.dtbo $domid
>
> Can how clarify how the MMIO will be mapped? Is it direct mapped? If so,
> couldn't this result to clash with other part of the address space (e.g.
> RAM?).
Yes, it is reusing the same code as dom0, which makes the code nice but
it doesn't support non-1:1 mappings. I think those should be done via
the xen,reg property. My suggestion would be this:
- if xen,reg is present, use it
- if xen,reg is not present, fall back to 1:1 mapping based on reg
For the next version of the series, I'd just document the current
limitation of the implementation. I added this to patch #4:
diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c
index 0f8b25ccb4..c2b03865a7 100644
--- a/xen/common/dt-overlay.c
+++ b/xen/common/dt-overlay.c
@@ -845,7 +845,7 @@ static long handle_attach_overlay_nodes(struct domain *d,
uint32_t overlay_fdt_size)
{
int rc;
- unsigned int j;
+ unsigned int j, len;
struct overlay_track *entry;
rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size);
@@ -888,6 +888,12 @@ static long handle_attach_overlay_nodes(struct domain *d,
goto out;
}
+ if ( dt_get_property(overlay_node, "xen,reg", &len) )
+ {
+ printk(XENLOG_ERR "xen,reg not supported yet in overlay\n");
+ rc = -EOPNOTSUPP;
+ goto out;
+ }
write_lock(&dt_host_lock);
rc = handle_device(d, overlay_node, p2m_mmio_direct_c,
entry->iomem_ranges, entry->irq_ranges);
> > + (dom0) xl console $domid # To access $domid
> > console
> > +
> > +Next, if the user needs to modify/prepare the overlay.dtbo suitable for
> > +the domU:
> > +
> > + (domU) mkdir -p /sys/kernel/config/device-tree/overlays/new_overlay
> > + (domU) cat overlay_domu.dtbo >
> > /sys/kernel/config/device-tree/overlays/new_overlay/dtbo
> > +
> > +Finally, if needed, the relevant Linux kernel drive can be probed:
> > +
> > + (domU) modprobe module_name.ko
> > +
> > +Example for domU overlay remove:
> > +
> > + (dom0) xl dt-overlay detach overlay.dtbo $domid
> > + (dom0) xl dt-overlay remove overlay.dtbo
>
> I assume we have safety check in place to ensure we can't remove the device if
> it is already attached. Is that correct?
I'll remove this part of the doc.
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v4 0/9] Remaining patches for dynamic node programming using overlay dtbo
2024-05-23 7:40 [PATCH v4 0/9] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
` (8 preceding siblings ...)
2024-05-23 7:40 ` [PATCH v4 9/9] docs: Add device tree overlay documentation Henry Wang
@ 2024-05-23 21:29 ` Julien Grall
9 siblings, 0 replies; 23+ messages in thread
From: Julien Grall @ 2024-05-23 21:29 UTC (permalink / raw)
To: Henry Wang, xen-devel
Cc: Anthony PERARD, Stefano Stabellini, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk, George Dunlap, Nick Rosbrook,
Juergen Gross, Andrew Cooper, Jan Beulich
On 23/05/2024 08:40, Henry Wang wrote:
> Hi all,
Hi Henry,
> This is the remaining series for the full functional "dynamic node
> programming using overlay dtbo" feature. The first part [1] has
> already been merged.
>
> Quoting from the original series, the first part has already made
> Xen aware of new device tree node which means updating the dt_host
> with overlay node information, and in this series, the goal is to
> map IRQ and IOMMU during runtime, where we will do the actual IOMMU
> and IRQ mapping and unmapping to a running domain. Also, documentation
> of the "dynamic node programming using overlay dtbo" feature is added.
>
> During the discussion in v3, I was recommended to split the overlay
> devices attach/detach to/from running domains to separated patches [3].
> But I decided to only expose the xl user interfaces together to the
> users after device attach/detach is fully functional, so I didn't
> split the toolstack patch (#8).
So I was asking to split so we can get some of the work merged for 4.19.
Can you clarify, whether the intention is to merge only patches #1-5?
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 23+ messages in thread