Re: [Xen-devel] [PATCH net-next 00/22] net: fix return type of ndo_start_xmit function
On 09/20/2018 07:32 AM, YueHaibing wrote: > The method ndo_start_xmit() is defined as returning an 'netdev_tx_t', > which is a typedef for an enum type, so make sure the implementation in > this driver has returns 'netdev_tx_t' value, and change the function > return type to netdev_tx_t. > May be I missed smth, but it's acceptable to report standard error codes from .xmit() callback as per dev_xmit_complete(). -- regards, -grygorii ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [PATCH v5 1/3] xen/device-tree: Let DT reserve map entries overlap reserved-memory
Hi All,
On 04.11.24 12:49, Michal Orzel wrote:
On 27/09/2024 00:24, Shawn Anastasio wrote:
Commit 53dc37829c31 ("xen/arm: Add DT reserve map regions to
bootinfo.reserved_mem") changes the way reserve map regions are tracked,
and as a result broke bootfdt's ability to handle device trees in which
the reserve map and the `reserved-memory` node contain the same entries
as each other, as is the case on PPC when booted by skiboot.
Fix this behavior by moving the reserve map check to after the DT has
been parsed and by explicitly allowing overlap with entries created by
`reserved-memory` nodes.
Fixes: 53dc37829c31 ("xen/arm: Add DT reserve map regions to
bootinfo.reserved_mem")
Signed-off-by: Shawn Anastasio
---
xen/common/device-tree/bootfdt.c | 28 +++-
xen/common/device-tree/bootinfo.c | 11 +--
xen/include/xen/bootfdt.h | 3 ++-
3 files changed, 34 insertions(+), 8 deletions(-)
diff --git a/xen/common/device-tree/bootfdt.c b/xen/common/device-tree/bootfdt.c
index 911a630e7d..2a51ee44a3 100644
--- a/xen/common/device-tree/bootfdt.c
+++ b/xen/common/device-tree/bootfdt.c
@@ -177,7 +177,7 @@ static int __init device_tree_get_meminfo(const void *fdt,
int node,
{
device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
if ( mem == bootinfo_get_reserved_mem() &&
- check_reserved_regions_overlap(start, size) )
+ check_reserved_regions_overlap(start, size, NULL) )
return -EINVAL;
/* Some DT may describe empty bank, ignore them */
if ( !size )
@@ -590,14 +590,36 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t
paddr)
if ( nr_rsvd < 0 )
panic("Parsing FDT memory reserve map failed (%d)\n", nr_rsvd);
+ret = device_tree_for_each_node(fdt, 0, early_scan_node, NULL);
This should be moved before fdt_num_mem_rsv so that the program flow makes
sense. In your case nr_rsvd is
not used immediately after.
+if ( ret )
+panic("Early FDT parsing failed (%d)\n", ret);
+
for ( i = 0; i < nr_rsvd; i++ )
{
+const struct membanks *overlap = NULL;
struct membank *bank;
paddr_t s, sz;
if ( fdt_get_mem_rsv_paddr(device_tree_flattened, i, &s, &sz) < 0 )
continue;
+if ( check_reserved_regions_overlap(s, sz, &overlap) )
+{
+if ( overlap == bootinfo_get_reserved_mem() )
+{
+/*
+ * Some valid device trees, such as those generated by
OpenPOWER
+ * skiboot firmware, expose all reserved memory regions in the
+ * FDT memory reservation block (here) AND in the
+ * reserved-memory node which has already been parsed. Thus,
any
+ * overlaps in the mem_reserved banks should be ignored.
+ */
+ continue;
I think this is incorrect. Imagine this scenario:
/memreserve/ 0x4000 0x4000;
and /reserved-memory/foo with:
reg = <0x0 0x7000 0x0 0x1000>;
You would ignore the entire region described with /memreserve/ even though it
overlaps just the last page.
The problem you're describing is about regions that match 1:1 in /memreserve/
and /reserved-memory/.
Therefore I think you should check that the overlapped regions match exactly.
I've also discovered an issue with Commit 53dc37829c31 ("xen/arm: Add DT
reserve map
regions to bootinfo.reserved_mem") - the bootloader adds Initrd in
FDT reserved map which then conflicts with Initrd module (ARM64).
This patch, as is, doesn't fix an issue for me:
(XEN) Checking for initrd in /chosen
(XEN) Initrd 8440-86152ac6
(XEN) Region: [0x008440, 0x0086152ac6) overlapping with mod[2]:
[0x008440, 0x0086152ac6)
(XEN)
(XEN)
(XEN) Panic on CPU 0:
(XEN) FDT reserve map overlapped with membanks/modules
(XEN)
So I did fast try of Michal Orzel suggestion and it seems working for me.
And if it's working for PPC - may be that's it (feel free to incorporate). Diff
below.
(XEN) Checking for initrd in /chosen
(XEN) Initrd 8440-86152ac6
(XEN) RAM: 4800 - bfff
(XEN) RAM: 00048000 - 0004
(XEN) RAM: 0006 - 0006
(XEN)
(XEN) MODULE[0]: 4808 - 481ec000 Xen
(XEN) MODULE[1]: 4800 - 4801e080 Device Tree
(XEN) MODULE[2]: 8440 - 86152ac6 Ramdisk
(XEN) MODULE[3]: 4830 - 4a30 Kernel
(XEN) MODULE[4]: 4807 - 4808 XSM
(XEN) RESVD[0]: 6000 - 7fff
(XEN) RESVD[1]: b000 - bfff
(XEN) RESVD[2]: a000 - afff
...
(XEN) *** LOADING DOMAIN 0 ***
(XEN) Loading d0 kernel from boot module @ 4830
(XEN) Loading ramdisk
Re: [PATCH v5 1/3] xen/device-tree: Let DT reserve map entries overlap reserved-memory
On 04.11.24 14:39, Grygorii Strashko wrote:
Hi All,
On 04.11.24 12:49, Michal Orzel wrote:
On 27/09/2024 00:24, Shawn Anastasio wrote:
Commit 53dc37829c31 ("xen/arm: Add DT reserve map regions to
bootinfo.reserved_mem") changes the way reserve map regions are tracked,
and as a result broke bootfdt's ability to handle device trees in which
the reserve map and the `reserved-memory` node contain the same entries
as each other, as is the case on PPC when booted by skiboot.
Fix this behavior by moving the reserve map check to after the DT has
been parsed and by explicitly allowing overlap with entries created by
`reserved-memory` nodes.
Fixes: 53dc37829c31 ("xen/arm: Add DT reserve map regions to
bootinfo.reserved_mem")
Signed-off-by: Shawn Anastasio
---
xen/common/device-tree/bootfdt.c | 28 +++-
xen/common/device-tree/bootinfo.c | 11 +--
xen/include/xen/bootfdt.h | 3 ++-
3 files changed, 34 insertions(+), 8 deletions(-)
diff --git a/xen/common/device-tree/bootfdt.c b/xen/common/device-tree/bootfdt.c
index 911a630e7d..2a51ee44a3 100644
--- a/xen/common/device-tree/bootfdt.c
+++ b/xen/common/device-tree/bootfdt.c
@@ -177,7 +177,7 @@ static int __init device_tree_get_meminfo(const void *fdt,
int node,
{
device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
if ( mem == bootinfo_get_reserved_mem() &&
- check_reserved_regions_overlap(start, size) )
+ check_reserved_regions_overlap(start, size, NULL) )
return -EINVAL;
/* Some DT may describe empty bank, ignore them */
if ( !size )
@@ -590,14 +590,36 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t
paddr)
if ( nr_rsvd < 0 )
panic("Parsing FDT memory reserve map failed (%d)\n", nr_rsvd);
+ ret = device_tree_for_each_node(fdt, 0, early_scan_node, NULL);
This should be moved before fdt_num_mem_rsv so that the program flow makes
sense. In your case nr_rsvd is
not used immediately after.
+ if ( ret )
+ panic("Early FDT parsing failed (%d)\n", ret);
+
for ( i = 0; i < nr_rsvd; i++ )
{
+ const struct membanks *overlap = NULL;
struct membank *bank;
paddr_t s, sz;
if ( fdt_get_mem_rsv_paddr(device_tree_flattened, i, &s, &sz) < 0 )
continue;
+ if ( check_reserved_regions_overlap(s, sz, &overlap) )
+ {
+ if ( overlap == bootinfo_get_reserved_mem() )
+ {
+ /*
+ * Some valid device trees, such as those generated by
OpenPOWER
+ * skiboot firmware, expose all reserved memory regions in the
+ * FDT memory reservation block (here) AND in the
+ * reserved-memory node which has already been parsed. Thus,
any
+ * overlaps in the mem_reserved banks should be ignored.
+ */
+ continue;
I think this is incorrect. Imagine this scenario:
/memreserve/ 0x4000 0x4000;
and /reserved-memory/foo with:
reg = <0x0 0x7000 0x0 0x1000>;
You would ignore the entire region described with /memreserve/ even though it
overlaps just the last page.
The problem you're describing is about regions that match 1:1 in /memreserve/
and /reserved-memory/.
Therefore I think you should check that the overlapped regions match exactly.
I've also discovered an issue with Commit 53dc37829c31 ("xen/arm: Add DT
reserve map
regions to bootinfo.reserved_mem") - the bootloader adds Initrd in
FDT reserved map which then conflicts with Initrd module (ARM64).
This patch, as is, doesn't fix an issue for me:
(XEN) Checking for initrd in /chosen
(XEN) Initrd 8440-86152ac6
(XEN) Region: [0x008440, 0x0086152ac6) overlapping with mod[2]:
[0x008440, 0x0086152ac6)
(XEN)
(XEN)
(XEN) Panic on CPU 0:
(XEN) FDT reserve map overlapped with membanks/modules
(XEN)
So I did fast try of Michal Orzel suggestion and it seems working for me.
And if it's working for PPC - may be that's it (feel free to incorporate). Diff
below.
(XEN) Checking for initrd in /chosen
(XEN) Initrd 8440-86152ac6
(XEN) RAM: 4800 - bfff
(XEN) RAM: 00048000 - 0004
(XEN) RAM: 0006 - 0006
(XEN)
(XEN) MODULE[0]: 4808 - 481ec000 Xen
(XEN) MODULE[1]: 4800 - 4801e080 Device Tree
(XEN) MODULE[2]: 8440 - 86152ac6 Ramdisk
(XEN) MODULE[3]: 4830 - 4a30 Kernel
(XEN) MODULE[4]: 4807 - 4808 XSM
(XEN) RESVD[0]: 6000 - 7fff
(XEN) RESVD[1]: b000 - 00
Re: [PATCH v5 1/3] xen/device-tree: Let DT reserve map entries overlap reserved-memory
On 05.11.24 12:42, Michal Orzel wrote:
On 04/11/2024 13:39, Grygorii Strashko wrote:
Hi All,
On 04.11.24 12:49, Michal Orzel wrote:
On 27/09/2024 00:24, Shawn Anastasio wrote:
Commit 53dc37829c31 ("xen/arm: Add DT reserve map regions to
bootinfo.reserved_mem") changes the way reserve map regions are tracked,
and as a result broke bootfdt's ability to handle device trees in which
the reserve map and the `reserved-memory` node contain the same entries
as each other, as is the case on PPC when booted by skiboot.
Fix this behavior by moving the reserve map check to after the DT has
been parsed and by explicitly allowing overlap with entries created by
`reserved-memory` nodes.
Fixes: 53dc37829c31 ("xen/arm: Add DT reserve map regions to
bootinfo.reserved_mem")
Signed-off-by: Shawn Anastasio
---
xen/common/device-tree/bootfdt.c | 28 +++-
xen/common/device-tree/bootinfo.c | 11 +--
xen/include/xen/bootfdt.h | 3 ++-
3 files changed, 34 insertions(+), 8 deletions(-)
diff --git a/xen/common/device-tree/bootfdt.c b/xen/common/device-tree/bootfdt.c
index 911a630e7d..2a51ee44a3 100644
--- a/xen/common/device-tree/bootfdt.c
+++ b/xen/common/device-tree/bootfdt.c
@@ -177,7 +177,7 @@ static int __init device_tree_get_meminfo(const void *fdt,
int node,
{
device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
if ( mem == bootinfo_get_reserved_mem() &&
- check_reserved_regions_overlap(start, size) )
+ check_reserved_regions_overlap(start, size, NULL) )
return -EINVAL;
/* Some DT may describe empty bank, ignore them */
if ( !size )
@@ -590,14 +590,36 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t
paddr)
if ( nr_rsvd < 0 )
panic("Parsing FDT memory reserve map failed (%d)\n", nr_rsvd);
+ret = device_tree_for_each_node(fdt, 0, early_scan_node, NULL);
This should be moved before fdt_num_mem_rsv so that the program flow makes
sense. In your case nr_rsvd is
not used immediately after.
+if ( ret )
+panic("Early FDT parsing failed (%d)\n", ret);
+
for ( i = 0; i < nr_rsvd; i++ )
{
+const struct membanks *overlap = NULL;
struct membank *bank;
paddr_t s, sz;
if ( fdt_get_mem_rsv_paddr(device_tree_flattened, i, &s, &sz) < 0 )
continue;
+if ( check_reserved_regions_overlap(s, sz, &overlap) )
+{
+if ( overlap == bootinfo_get_reserved_mem() )
+{
+/*
+ * Some valid device trees, such as those generated by
OpenPOWER
+ * skiboot firmware, expose all reserved memory regions in the
+ * FDT memory reservation block (here) AND in the
+ * reserved-memory node which has already been parsed. Thus,
any
+ * overlaps in the mem_reserved banks should be ignored.
+ */
+ continue;
I think this is incorrect. Imagine this scenario:
/memreserve/ 0x4000 0x4000;
and /reserved-memory/foo with:
reg = <0x0 0x7000 0x0 0x1000>;
You would ignore the entire region described with /memreserve/ even though it
overlaps just the last page.
The problem you're describing is about regions that match 1:1 in /memreserve/
and /reserved-memory/.
Therefore I think you should check that the overlapped regions match exactly.
I've also discovered an issue with Commit 53dc37829c31 ("xen/arm: Add DT
reserve map
regions to bootinfo.reserved_mem") - the bootloader adds Initrd in
FDT reserved map which then conflicts with Initrd module (ARM64).
This patch, as is, doesn't fix an issue for me:
(XEN) Checking for initrd in /chosen
(XEN) Initrd 8440-86152ac6
(XEN) Region: [0x008440, 0x0086152ac6) overlapping with mod[2]:
[0x008440, 0x0086152ac6)
(XEN)
(XEN)
(XEN) Panic on CPU 0:
(XEN) FDT reserve map overlapped with membanks/modules
(XEN)
So I did fast try of Michal Orzel suggestion and it seems working for me.
And if it's working for PPC - may be that's it (feel free to incorporate). Diff
below.
(XEN) Checking for initrd in /chosen
(XEN) Initrd 8440-86152ac6
(XEN) RAM: 4800 - bfff
(XEN) RAM: 00048000 - 0004
(XEN) RAM: 0006 - 0006
(XEN)
(XEN) MODULE[0]: 4808 - 481ec000 Xen
(XEN) MODULE[1]: 4800 - 4801e080 Device Tree
(XEN) MODULE[2]: 8440 - 86152ac6 Ramdisk
(XEN) MODULE[3]: 4830 - 4a30 Kernel
(XEN) MODULE[4]: 4807 - 4808 XSM
(XEN) RESVD[0]: 60
Re: [PATCH v2 3/8] xen/arm: Add SCMI over SMC calls handling layer
Hi
I'd be apprcieated if could consider my comments below.
On 30.09.24 14:47, Andrei Cherechesu (OSS) wrote:
From: Andrei Cherechesu
Introduce the SCMI layer to have some basic degree of awareness
about SMC calls that are based on the ARM System Control and
Management Interface (SCMI) specification (DEN0056E).
The SCMI specification includes various protocols for managing
system-level resources, such as: clocks, pins, reset, system power,
power domains, performance domains, etc. The clients are named
"SCMI agents" and the server is named "SCMI platform".
Only support the shared-memory based transport with SMCs as
the doorbell mechanism for notifying the platform. Also, this
implementation only handles the "arm,scmi-smc" compatible,
requiring the following properties:
- "arm,smc-id" (unique SMC ID)
- "shmem" (one or more phandles pointing to shmem zones
for each channel)
The initialization is done as 'presmp_initcall', since we need
SMCs and PSCI should already probe EL3 FW for supporting SMCCC.
If no "arm,scmi-smc" compatible node is found in Dom0's
DT, the initialization fails silently, as it's not mandatory.
Otherwise, we get the 'arm,smc-id' DT property from the node,
to know the SCMI SMC ID we handle. The 'shmem' memory ranges
are not validated, as the SMC calls are only passed through
to EL3 FW if coming from Dom0 and as if Dom0 would be natively
running.
Signed-off-by: Andrei Cherechesu
Reviewed-by: Stefano Stabellini
---
xen/arch/arm/Kconfig| 10 ++
xen/arch/arm/Makefile | 1 +
xen/arch/arm/include/asm/scmi-smc.h | 52 +
xen/arch/arm/scmi-smc.c | 163
Could it be moved in separate folder - for example "sci" or "firmware"?
There are definitely more SCMI specific code will be added in the future
as this solution is little bit too simplified.
4 files changed, 226 insertions(+)
create mode 100644 xen/arch/arm/include/asm/scmi-smc.h
create mode 100644 xen/arch/arm/scmi-smc.c
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 323c967361..adf53e2de1 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -245,6 +245,16 @@ config PARTIAL_EMULATION
not been emulated to their complete functionality. Enabling this might
result in unwanted/non-spec compliant behavior.
+config SCMI_SMC
Could you please rename it to clearly specify that it is only dom0/hwdom
specific? Like SCMI_SMC_DOM0 or SCMI_SMC_HW_DOM.
+ bool "Enable forwarding SCMI over SMC calls from Dom0 to EL3 firmware"
+ default y
+ help
+ This option enables basic awareness for SCMI calls using SMC as
+ doorbell mechanism and Shared Memory for transport ("arm,scmi-smc"
+ compatible only). The value of "arm,smc-id" DT property from SCMI
+ firmware node is used to trap and forward corresponding SCMI SMCs
+ to firmware running at EL3, if the call comes from Dom0.
+
endmenu
menu "ARM errata workaround via the alternative framework"
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 7792bff597..b85ad9c13f 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -45,6 +45,7 @@ obj-y += platform_hypercall.o
obj-y += physdev.o
obj-y += processor.o
obj-y += psci.o
+obj-$(CONFIG_SCMI_SMC) += scmi-smc.o
obj-y += setup.o
obj-y += shutdown.o
obj-y += smp.o
diff --git a/xen/arch/arm/include/asm/scmi-smc.h
b/xen/arch/arm/include/asm/scmi-smc.h
new file mode 100644
index 00..c6c0079e86
--- /dev/null
+++ b/xen/arch/arm/include/asm/scmi-smc.h
@@ -0,0 +1,52 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * xen/arch/arm/include/asm/scmi-smc.h
+ *
+ * ARM System Control and Management Interface (SCMI) over SMC
+ * Generic handling layer
+ *
+ * Andrei Cherechesu
+ * Copyright 2024 NXP
+ */
+
+#ifndef __ASM_SCMI_SMC_H__
+#define __ASM_SCMI_SMC_H__
+
+#include
+#include
+
+#ifdef CONFIG_SCMI_SMC
+
+bool scmi_is_enabled(void);
+bool scmi_is_valid_smc_id(uint32_t fid);
+bool scmi_handle_smc(struct cpu_user_regs *regs);
+
+#else
+
+static inline bool scmi_is_enabled(void)
+{
+return false;
+}
+
+static inline bool scmi_is_valid_smc_id(uint32_t fid)
+{
+return false;
+}
+
+static inline bool scmi_handle_smc(struct cpu_user_regs *regs)
I propose to add "struct domain *d" as the first parameter to make it
more abstract from Xen internals.
+{
+return false;
+}
+
+#endif /* CONFIG_SCMI_SMC */
+
+#endif /* __ASM_SCMI_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/scmi-smc.c b/xen/arch/arm/scmi-smc.c
new file mode 100644
index 00..373ca7ba5f
--- /dev/null
+++ b/xen/arch/arm/scmi-smc.c
@@ -0,0 +1,163 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * xen/arch/arm/scmi-smc.c
+ *
+ * ARM System Control and Management Interface (SCMI) over SMC
+ * Ge
Re: [PATCH] xen/device-tree: Allow exact match for overlapping regions
Hi All,
On 07.11.24 11:42, Grygorii Strashko wrote:
On 06.11.24 17:16, Luca Fancellu wrote:
Hi Michal,
So we have 2 separate issues. I don't particularly like the concept of
introducing MEMBANK_NONE
and the changes below look a bit too much for me, given that for boot modules
we can only have
/memreserve/ matching initrd.
Shawn patch fixes the first issue. AFAICT the second issue can be fixed by
below simple patch:
diff --git a/xen/common/device-tree/bootfdt.c b/xen/common/device-tree/bootfdt.c
index 927f59c64b0d..d8bd8c44bd35 100644
--- a/xen/common/device-tree/bootfdt.c
+++ b/xen/common/device-tree/bootfdt.c
@@ -586,6 +586,10 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t paddr)
add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), false);
+ ret = device_tree_for_each_node(fdt, 0, early_scan_node, NULL);
+ if ( ret )
+ panic("Early FDT parsing failed (%d)\n", ret);
+
nr_rsvd = fdt_num_mem_rsv(fdt);
if ( nr_rsvd < 0 )
panic("Parsing FDT memory reserve map failed (%d)\n", nr_rsvd);
@@ -594,10 +598,14 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t
paddr)
{
struct membank *bank;
paddr_t s, sz;
+ const struct bootmodule *mod =
boot_module_find_by_kind(BOOTMOD_RAMDISK);
if ( fdt_get_mem_rsv_paddr(device_tree_flattened, i, &s, &sz) < 0 )
continue;
+ if ( mod && (mod->start == s) && (mod->size == sz) )
+ continue;
Ok I see, we skip the /memreserve/ entry if it matches the ramdisk, fair
enough, I don’t have
a strong opinion on how we do that, the important thing is just to unblock the
users experiencing
this issue.
Don't know if my opinion would matter here, but Luca's patch looks more generic
and logically solid for me.
While handling only "ramdisk" somewhere in the middle - looks more like a hack.
Any way, it's up to you.
Sorry, that I'm disturbing you, but is there going to be any conclusion
regarding this patch?
Best regards,
-Grygorii
Re: [PATCH] xen/device-tree: Allow exact match for overlapping regions
On 06.11.24 15:41, Luca Fancellu wrote:
There are some cases where the device tree exposes a memory range
in both /memreserve/ and reserved-memory node, in this case the
current code will stop Xen to boot since it will find that the
latter range is clashing with the already recorded /memreserve/
ranges.
Furthermore, u-boot lists boot modules ranges, such as ramdisk,
in the /memreserve/ part and even in this case this will prevent
Xen to boot since it will see that the module memory range that
it is going to add in 'add_boot_module' clashes with a /memreserve/
range.
When Xen populate the data structure that tracks the memory ranges,
it also adds a memory type described in 'enum membank_type', so
in order to fix this behavior, allow the 'check_reserved_regions_overlap'
function to check for exact memory range match given a specific memory
type; allowing reserved-memory node ranges and boot modules to have an
exact match with ranges from /memreserve/.
While there, set a type for the memory recorded during ACPI boot.
Fixes: 53dc37829c31 ("xen/arm: Add DT reserve map regions to
bootinfo.reserved_mem")
Reported-by: Shawn Anastasio
Reported-by: Grygorii Strashko
Signed-off-by: Luca Fancellu
---
I tested this patch adding the same range in a /memreserve/ entry and
/reserved-memory node, and by letting u-boot pass a ramdisk.
I've also tested that a configuration running static shared memory still works
fine.
[...]
Thank you, Dom0 started successfully with Initrd.
Tested-by: Grygorii Strashko
Meminfo from boot log is below:
(XEN) Checking for initrd in /chosen
(XEN) Initrd 8440-860864fd
(XEN) RAM: 4800 - bfff
(XEN) RAM: 00048000 - 0004
(XEN) RAM: 0006 - 0006
(XEN)
(XEN) MODULE[0]: 4808 - 481ec000 Xen
(XEN) MODULE[1]: 4800 - 4801e080 Device Tree
(XEN) MODULE[2]: 8440 - 860864fd Ramdisk
(XEN) MODULE[3]: 4830 - 4a30 Kernel
(XEN) MODULE[4]: 4807 - 4808 XSM
(XEN) RESVD[0]: 8440 - 860864fc
(XEN) RESVD[1]: 6000 - 7fff
(XEN) RESVD[2]: b000 - bfff
(XEN) RESVD[3]: a000 - afff
...
BR,
-grygorii
Re: [PATCH] xen/device-tree: Allow exact match for overlapping regions
On 06.11.24 17:16, Luca Fancellu wrote:
Hi Michal,
So we have 2 separate issues. I don't particularly like the concept of
introducing MEMBANK_NONE
and the changes below look a bit too much for me, given that for boot modules
we can only have
/memreserve/ matching initrd.
Shawn patch fixes the first issue. AFAICT the second issue can be fixed by
below simple patch:
diff --git a/xen/common/device-tree/bootfdt.c b/xen/common/device-tree/bootfdt.c
index 927f59c64b0d..d8bd8c44bd35 100644
--- a/xen/common/device-tree/bootfdt.c
+++ b/xen/common/device-tree/bootfdt.c
@@ -586,6 +586,10 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t paddr)
add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), false);
+ret = device_tree_for_each_node(fdt, 0, early_scan_node, NULL);
+if ( ret )
+panic("Early FDT parsing failed (%d)\n", ret);
+
nr_rsvd = fdt_num_mem_rsv(fdt);
if ( nr_rsvd < 0 )
panic("Parsing FDT memory reserve map failed (%d)\n", nr_rsvd);
@@ -594,10 +598,14 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t
paddr)
{
struct membank *bank;
paddr_t s, sz;
+const struct bootmodule *mod =
boot_module_find_by_kind(BOOTMOD_RAMDISK);
if ( fdt_get_mem_rsv_paddr(device_tree_flattened, i, &s, &sz) < 0 )
continue;
+if ( mod && (mod->start == s) && (mod->size == sz) )
+continue;
Ok I see, we skip the /memreserve/ entry if it matches the ramdisk, fair
enough, I don’t have
a strong opinion on how we do that, the important thing is just to unblock the
users experiencing
this issue.
Don't know if my opinion would matter here, but Luca's patch looks more generic
and logically solid for me.
While handling only "ramdisk" somewhere in the middle - looks more like a hack.
Any way, it's up to you.
BR,
-grygorii
Re: [PATCH] xen/device-tree: Allow exact match for overlapping regions
On 14.11.24 15:09, Julien Grall wrote: On 14/11/2024 12:22, Michal Orzel wrote: On 14/11/2024 13:04, Julien Grall wrote: Hi Michal, On 14/11/2024 11:48, Michal Orzel wrote: On 14/11/2024 11:31, Julien Grall wrote: Looking at the code, I think /memreserve/ and /reserved-memory are not mapped in Xen and everything in /reserved-memory is mapped to dom0. Why do we forward /reserved-memory to dom0 fdt but /memreserve/ not? I was wondering the same. The main issue I can think of with /memreserve/ is some of the regions will likely be for Xen own usage. So Can you give example of regions defined as reserved for Xen usage (other than static-mem)? The spin table to bring-up CPUs. we would need to have a way to exclude them from dom0. > From the discussion> we're having it seems like we should treat them equally. Also, looking at Luca patch, we seem to special case /memreserve/ and only allow for overlap /memresrve/ with boot modules and not /reserved-memory/ with boot modules. If we are going to claim that all the boot modules can be marked as reserved by the bootloader, then I think we should treat them equally providing the same experience to dom0. In my mind, /memreserved/ and /reserved-memory/ are different. The former doesn't say what the region is for, but the latter will indicate it. In the context of this patch, I don't agree. We're discussing overlap, and if a region A from /memreserve/ overlaps fully with a module A, we know what is the purpose of it. > Today it's initrd, but as you say we cannot rule out other modules as well. To give a concrete example, the /reserved-region/ can be used to reserve space for the VGA buffer. It would be odd that someone would put the boot module in the middle of the VGA buffer... If Xen ends up to use the VGA buffer (not the case today), then it would be a problem. Xen would need to be reworked to move all boot modules outside of the memory because it can access the VGA (or any other reserved regions). The problem is slightly different for /memreserve/ because we don't exactly know what the regions are for until we parse the rest of the DT. Yes technically, a user could put the initrd in the wrong place. So the problem is the same. But this is a relexation I am more willing to accept over /reserved-region/ right now. So I am not 100% sure how the bootmodule could end up in /reserved-memory/ because they are described as part of the multiboot modules. Do you have a scenario? I don't same as I don't have scenario for /memreserve/ overlapping with sth else than initrd. All of these comes from my validation of u-boot, grub, barebox code. I have a feeling that due to U-Boot trick that is not present in any other *known* bootloader, we are trying to over-engineer the problem :) But as Stefano and you wrote, we should follow the spec and for me we should therefore treat them equally. See above why I don't think we should treat them equally today. We might be able in the future if we can categorize all the regions in /memreserve/. [...] Last thing I wanted to ask (for my clarity) is that if bootloader loads initrd at region A and marks it as reserved (either in /memreserve/ or /reserved-memory/), and later on Xen copies initrd from region A to B, shouldn't the reserved memory region be updated to cover new region for initrd? If we mark the region has a reserved, then we are telling the OS it can't use the region. But I am not sure why it would be needed as Xen Well, in the context of initrd, kernel uses it even though it is reserved. This is because of the second part of the spec where other bindings come into play. doesn't care how the regions is going to be used by the domain. From a domain side, do you see any reason why we would want to mark again the region as reserved? TBH I don't same as I still don't know why U-Boot does that trick. It comes from a very old code and my initial understanding is that it is done purely for U-boot bookkeeping. /memreserve/ (and now) /reserved-regions/ are an easy way to find the regions that should be excluded from the RAM. Without that, you will need to have special case (such as for initrd, and the various xen boot moudles). I suspect that Linux have a special case and hence why it hasn't been a problem that Xen doesn't reserve the region. My be it will help in this discussion - some investigation results. At boot time (only ARM64, but other arches looks similar): - Determines if initrd present from DT : early_init_dt_scan()->setup_machine_fdt() or by checking bootargs "initrd/initrdmem=". - [1] In arm64_memblock_init() it adds initrd in reserved memory https://github.com/torvalds/linux/blob/master/arch/arm64/mm/init.c#L274 therefore Linux doesn't depends on any kind of "DT reserved memory" ranges for initrd. - The Linux processes "reserved-memory" nodes first. https://github.com/torvalds/linux/blob/master/drivers/of/fdt.c#L504 - After this FDT m
Re: [PATCH v2] xen/device-tree: Allow region overlapping with /memreserve/ ranges
On 14.11.24 12:28, Luca Fancellu wrote:
There are some cases where the device tree exposes a memory range
in both /memreserve/ and reserved-memory node, in this case the
current code will stop Xen to boot since it will find that the
latter range is clashing with the already recorded /memreserve/
ranges.
Furthermore, u-boot lists boot modules ranges, such as ramdisk,
in the /memreserve/ part and even in this case this will prevent
Xen to boot since it will see that the module memory range that
it is going to add in 'add_boot_module' clashes with a /memreserve/
range.
When Xen populate the data structure that tracks the memory ranges,
it also adds a memory type described in 'enum membank_type', so
in order to fix this behavior, allow overlapping with the /memreserve/
ranges in the 'check_reserved_regions_overlap' function when a flag
is set.
In order to implement this solution, there is a distinction between
the 'struct membanks *' handled by meminfo_overlap_check(...) that
needs to be done, because the static shared memory banks doesn't have
a usable bank[].type field and so it can't be accessed, hence now
the 'struct membanks_hdr' have a 'enum region_type type' field in order
to be able to identify static shared memory banks in meminfo_overlap_check(...).
While there, set a type for the memory recorded using meminfo_add_bank()
from efi-boot.h.
Fixes: 53dc37829c31 ("xen/arm: Add DT reserve map regions to
bootinfo.reserved_mem")
Reported-by: Shawn Anastasio
Reported-by: Grygorii Strashko
Signed-off-by: Luca Fancellu
---
Changes from v1:
- fixed commit message
- used boolean to allow /memreserve/ overlap or not
- now 'struct membanks *' have a type, while it might seem a waste of space,
it might be used in the future to consolidate also the 'struct bootmodules'
and 'struct bootcmdlines' to use the same 'struct membanks *' interface.
Also the added space is limited to 3/4 enum size.
- The change above allowed this version to remove the "hack" when dealing with
static shared memory banks, that doesn't have a type.
I tested this patch adding the same range in a /memreserve/ entry and
/reserved-memory node, and by letting u-boot pass a ramdisk.
I've also tested that a configuration running static shared memory still works
fine.
---
---
xen/arch/arm/efi/efi-boot.h | 3 ++-
xen/arch/arm/static-shmem.c | 2 +-
xen/common/device-tree/bootfdt.c | 9 ++-
xen/common/device-tree/bootinfo.c | 39 +++
xen/include/xen/bootfdt.h | 20 +---
5 files changed, 57 insertions(+), 16 deletions(-)
[...]
Dom0 started successfully with Initrd.
Tested-by: Grygorii Strashko
Meminfo from boot log is below:
(XEN) Checking for initrd in /chosen
(XEN) Initrd 8440-87bc5831
(XEN) RAM: 4800 - bfff
(XEN) RAM: 00048000 - 0004
(XEN) RAM: 0006 - 0006
(XEN)
(XEN) MODULE[0]: 4808 - 481ec000 Xen
(XEN) MODULE[1]: 4800 - 4801e080 Device Tree
(XEN) MODULE[2]: 8440 - 87bc5831 Ramdisk
(XEN) MODULE[3]: 4830 - 4a30 Kernel
(XEN) MODULE[4]: 4807 - 4808 XSM
(XEN) RESVD[0]: 8440 - 87bc5830
(XEN) RESVD[1]: 6000 - 7fff
(XEN) RESVD[2]: b000 - bfff
(XEN) RESVD[3]: a000 - afff
Thank you,
-grygorii
[PATCH] device-tree: optimize dt_device_for_passthrough()
The dt_device_for_passthrough() is called many times during Xen
initialization and Dom0 creation. On every call it traverses struct
dt_device_node properties list and compares compares properties name with
"xen,passthrough" which is runtime overhead. This can be optimized by
marking dt_device_node as passthrough while unflattening DT.
This patch introduced new struct dt_device_node property "is_passthrough"
which is filled if "xen,passthrough" property is present while unflattening
DT and dt_device_for_passthrough() just return it's value.
Signed-off-by: Grygorii Strashko
---
xen/common/device-tree/device-tree.c | 7 +--
xen/include/xen/device_tree.h| 2 ++
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/xen/common/device-tree/device-tree.c
b/xen/common/device-tree/device-tree.c
index d0528c582565..a329aaf576da 100644
--- a/xen/common/device-tree/device-tree.c
+++ b/xen/common/device-tree/device-tree.c
@@ -1682,8 +1682,7 @@ bool dt_device_is_available(const struct dt_device_node
*device)
bool dt_device_for_passthrough(const struct dt_device_node *device)
{
-return (dt_find_property(device, "xen,passthrough", NULL) != NULL);
-
+return device->is_passthrough;
}
static int __dt_parse_phandle_with_args(const struct dt_device_node *np,
@@ -1913,6 +1912,7 @@ static unsigned long unflatten_dt_node(const void *fdt,
np->used_by = 0;
/* By default the device is not protected */
np->is_protected = false;
+np->is_passthrough = false;
INIT_LIST_HEAD(&np->domain_list);
if ( new_format )
@@ -2001,6 +2001,9 @@ static unsigned long unflatten_dt_node(const void *fdt,
* stuff */
if ( strcmp(pname, "ibm,phandle") == 0 )
np->phandle = be32_to_cpup((__be32 *)*p);
+
+if ( strcmp(pname, "xen,passthrough") == 0 )
+np->is_passthrough = true;
pp->name = pname;
pp->length = sz;
pp->value = (void *)*p;
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 5ff763bb80bb..96001d5b7843 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -94,6 +94,8 @@ struct dt_device_node {
/* IOMMU specific fields */
bool is_protected;
+/* Indicates DT device is for passthrough */
+bool is_passthrough;
#ifdef CONFIG_STATIC_EVTCHN
/* HACK: Remove this if there is a need of space */
--
2.34.1
Re: [PATCH] device-tree: optimize dt_device_for_passthrough()
On 11.02.25 14:32, Julien Grall wrote:
On 11/02/2025 11:57, Orzel, Michal wrote:
On 11/02/2025 12:18, Grygorii Strashko wrote:
The dt_device_for_passthrough() is called many times during Xen
initialization and Dom0 creation. On every call it traverses struct
dt_device_node properties list and compares compares properties name with
double "compares"
10x
"xen,passthrough" which is runtime overhead. This can be optimized by
Are you sure? Looking at the calls, it's almost only used at boot except for dt
overlay.
marking dt_device_node as passthrough while unflattening DT.
This patch introduced new struct dt_device_node property "is_passthrough"
which is filled if "xen,passthrough" property is present while unflattening
DT and dt_device_for_passthrough() just return it's value.
In the past we were skeptical about adding new fields to the dt_device_node
structure for use cases like this one. I would say this optimization is not
worth it. Also, why would you optimize dt_device_for_passthrough but not
e.g. dt_device_is_available.
So we are trading speed with memory usage. It looks like we may be using a
padding, although I didn't check whether the existing structure could be
packed...
Actually I see it consumes nothing due to alignments:
- before sizeof(dt_device_node)=144
- after sizeof(dt_device_node)=144
But it could be made bool is_passthrough:1; together with other bools, and
probably moved at the end of struct dt_device_node.
By the way, below diff can save 8 bytes on arm64 per struct dt_device_node.
You can check with other Arm maintainers.
Before forging an opinion, I'd like to see some numbers showing the performance
improvement. Also, is this impacting only boot?
Sry, indeed only boot, need to be more specific.
My DT: ~700 nodes
Number of calls till the end of create_dom0():
(XEN) === dt_device_for_passthrough=2684
dt_device_is_available=1429.
I've enabled console_timestamps=boot and did 5 boots and calculated average -
it gives ~20 microseconds improvement.
Also, I agree with Michal, if this is a concern for
dt_device_device_for_passthrough(). Then it would be a concern for a few others
calls using dt_find_property(). Are you going to modify all of them?
I follow the rule one patch one functional change. Regarding further optimization - I think only
generic properties can be optimized and personally I see now only "xen,passthrough" and
may be "status".
Ok. 20 microseconds. it's probably more like a measurement error margin.
Please advice if i should continue or drop?
Thank you for you comments.
-grygorii
---
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 96001d5b7843..0448cc297445 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -81,8 +81,8 @@ struct dt_property {
struct dt_device_node {
const char *name;
const char *type;
-dt_phandle phandle;
char *full_name;
+dt_phandle phandle;
domid_t used_by; /* By default it's used by dom0 */
struct dt_property *properties;
Re: Coding Style Review and Automation
Hi On 12.02.25 11:14, Roger Pau Monné wrote: On Tue, Feb 11, 2025 at 02:33:08PM -0800, Stefano Stabellini wrote: Hi Oleksandr, This morning, we had a discussion among maintainers, and the suggested approach moving forward is as follows: - First, it would be helpful to see a sample of the proposed changes applied to a single source file as an example. If you could provide such a patch, it would help advance the discussion. - If the changes are acceptable, we need to properly document the new coding style in xen.git. If not, we will need to iterate again. We may also need to add a "xen" template to clang-format. - Once finalized, we will proceed by making changes to the Xen source code piece by piece, as you suggested, rather than applying a single large patch. No objections, just wandering myself whether it was considered to initially only apply the new style to new chunks of code? Using `git-clang-format` or similar as suggested by Anthony. Is the adjusted style expected to be too different from the current one as such approach would lead to hard to read code due to the mixed styles? Sorry for may be dumb question, but wouldn't it be reasonable to consider adding just .clang-format specification to the Xen code base without automation features? For example, I've downloaded .clang-format from [1] and using it with my editor which supports clang-format integration. So, I can just select chunk of code and do auto-format on it. It speed ups my work very much and results make me happy more than 90% of the times. So, it would be nice to have it out of the box while cloning Xen code instead of searching for it, even if it's not perfect for now. Unhappy: it's probably "known" things - identification of complex defines and static struct/arrays declarations. For example original code: DT_DEVICE_START(ipmmu, "Renesas IPMMU-VMSA", DEVICE_IOMMU) .dt_match = ipmmu_dt_match, .init = ipmmu_init, DT_DEVICE_END And after auto format (me, personally, unhappy): DT_DEVICE_START(ipmmu, "Renesas IPMMU-VMSA", DEVICE_IOMMU) .dt_match = ipmmu_dt_match, .init = ipmmu_init, DT_DEVICE_END Best regards, -grygorii
Re: [PATCH] device-tree: optimize dt_device_for_passthrough()
On 12.02.25 11:04, Julien Grall wrote: Hi, On 11/02/2025 15:14, Grygorii Strashko wrote: On 11.02.25 14:32, Julien Grall wrote: On 11/02/2025 11:57, Orzel, Michal wrote: On 11/02/2025 12:18, Grygorii Strashko wrote: The dt_device_for_passthrough() is called many times during Xen initialization and Dom0 creation. On every call it traverses struct dt_device_node properties list and compares compares properties name with double "compares" 10x "xen,passthrough" which is runtime overhead. This can be optimized by Are you sure? Looking at the calls, it's almost only used at boot except for dt overlay. marking dt_device_node as passthrough while unflattening DT. This patch introduced new struct dt_device_node property "is_passthrough" which is filled if "xen,passthrough" property is present while unflattening DT and dt_device_for_passthrough() just return it's value. In the past we were skeptical about adding new fields to the dt_device_node structure for use cases like this one. I would say this optimization is not worth it. Also, why would you optimize dt_device_for_passthrough but not e.g. dt_device_is_available. So we are trading speed with memory usage. It looks like we may be using a padding, although I didn't check whether the existing structure could be packed... Actually I see it consumes nothing due to alignments: - before sizeof(dt_device_node)=144 - after sizeof(dt_device_node)=144 But it could be made bool is_passthrough:1; together with other bools, and probably moved at the end of struct dt_device_node. By the way, below diff can save 8 bytes on arm64 per struct dt_device_node. You can check with other Arm maintainers. Before forging an opinion, I'd like to see some numbers showing the performance improvement. Also, is this impacting only boot? Sry, indeed only boot, need to be more specific. My DT: ~700 nodes Number of calls till the end of create_dom0(): (XEN) === dt_device_for_passthrough=2684 dt_device_is_available=1429. I've enabled console_timestamps=boot and did 5 boots and calculated average - it gives ~20 microseconds improvement. This doesn't seem to be worth it. But I also don't know what's the normal boot time on your system... I guess we are still in seconds? Yes. in general if exclude SILO timeout. (XEN) [0.433789] *** (XEN) [0.434588] WARNING: SILO mode is not enabled. (XEN) [0.435204] It has implications on the security of the system, (XEN) [0.435992] unless the communications have been forbidden between (XEN) [0.436813] untrusted domains. (XEN) [0.437256] *** (XEN) [0.438055] 3... 2... 1... (XEN) [3.438566] *** Serial input to DOM0 (type 'CTRL-a' three times to switch input) (XEN) [3.439559] Freed 400kB init memory. Also, I agree with Michal, if this is a concern for dt_device_device_for_passthrough(). Then it would be a concern for a few others calls using dt_find_property(). Are you going to modify all of them? I follow the rule one patch one functional change. Regarding further optimization - I think only generic properties can be optimized and personally I see now only "xen,passthrough" and may be "status". Ok. 20 microseconds. it's probably more like a measurement error margin. Please advice if i should continue or drop? See above. Regardless that, would you be able to provide a bit more information of your end goal?Are you intending to be able to boot Xen/dom0/dom0less guest in less than N milliseconds? How far are you from that target? Are those numbers done on the latest Xen? It's more like result of code studying from my side as Xen newbie. I've considered it as kinda "obvious" change - calc some value once is better then do the same calculations many times. Ok, it's not obvious. I'll drop it. Asking because there are probably bigger place where optimization can be done first. Thanks, -grygorii
[PATCH] device-tree: optimize size of struct dt_device_node
From: Michal Orzel
The current placement of fields in struct dt_device_node is not optimal and
introduces holes due to fields alignment.
Checked with "'pahole xen-syms -C dt_device_node"
ARM64 size 144B, 16B holes:
/* size: 144, cachelines: 3, members: 15 */
/* sum members: 128, holes: 3, sum holes: 16 */
/* last cacheline: 16 bytes */
ARM32 size 72B, 4B holes
/* size: 72, cachelines: 2, members: 15 */
/* sum members: 68, holes: 2, sum holes: 4 */
/* last cacheline: 8 bytes */
This patch optimizes size of struct dt_device_node by rearranging its
field, which eliminates holes and reduces structure size by 16B(ARM64) and
4B(ARM32).
After ARM64 size 128B, no holes (-16B):
/* size: 128, cachelines: 2, members: 15 */
After ARM32 size 68B, no holes (-4B)
/* size: 68, cachelines: 2, members: 15 */
/* last cacheline: 4 bytes */
Signed-off-by: Michal Orzel
Signed-off-by: Grygorii Strashko
---
This patch follows discussion in [1]
[1] https://patchwork.kernel.org/comment/26239672/
xen/include/xen/device_tree.h | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 5ff763bb80bb..0ff80fda04da 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -81,17 +81,10 @@ struct dt_property {
struct dt_device_node {
const char *name;
const char *type;
-dt_phandle phandle;
char *full_name;
+dt_phandle phandle;
domid_t used_by; /* By default it's used by dom0 */
-struct dt_property *properties;
-struct dt_device_node *parent;
-struct dt_device_node *child;
-struct dt_device_node *sibling;
-struct dt_device_node *next; /* TODO: Remove it. Only use to know the last
children */
-struct dt_device_node *allnext;
-
/* IOMMU specific fields */
bool is_protected;
@@ -100,6 +93,13 @@ struct dt_device_node {
bool static_evtchn_created;
#endif
+struct dt_property *properties;
+struct dt_device_node *parent;
+struct dt_device_node *child;
+struct dt_device_node *sibling;
+struct dt_device_node *next; /* TODO: Remove it. Only use to know the last
children */
+struct dt_device_node *allnext;
+
/*
* The main purpose of this list is to link the structure in the list
* of devices assigned to domain.
--
2.34.1
Re: [PATCH v2 3/8] xen/arm: Add SCMI over SMC calls handling layer
On 09.12.24 19:37, Andrei Cherechesu wrote:
Hi Julien, Grygorii,
On 11/11/2024 12:33, Julien Grall wrote:
Hi,
On 01/11/2024 15:22, Grygorii Strashko wrote:
Hi
I'd be apprcieated if could consider my comments below.
On 30.09.24 14:47, Andrei Cherechesu (OSS) wrote:
From: Andrei Cherechesu
Introduce the SCMI layer to have some basic degree of awareness
about SMC calls that are based on the ARM System Control and
Management Interface (SCMI) specification (DEN0056E).
The SCMI specification includes various protocols for managing
system-level resources, such as: clocks, pins, reset, system power,
power domains, performance domains, etc. The clients are named
"SCMI agents" and the server is named "SCMI platform".
Only support the shared-memory based transport with SMCs as
the doorbell mechanism for notifying the platform. Also, this
implementation only handles the "arm,scmi-smc" compatible,
requiring the following properties:
- "arm,smc-id" (unique SMC ID)
- "shmem" (one or more phandles pointing to shmem zones
for each channel)
The initialization is done as 'presmp_initcall', since we need
SMCs and PSCI should already probe EL3 FW for supporting SMCCC.
If no "arm,scmi-smc" compatible node is found in Dom0's
DT, the initialization fails silently, as it's not mandatory.
Otherwise, we get the 'arm,smc-id' DT property from the node,
to know the SCMI SMC ID we handle. The 'shmem' memory ranges
are not validated, as the SMC calls are only passed through
to EL3 FW if coming from Dom0 and as if Dom0 would be natively
running.
Signed-off-by: Andrei Cherechesu
Reviewed-by: Stefano Stabellini
---
xen/arch/arm/Kconfig | 10 ++
xen/arch/arm/Makefile | 1 +
xen/arch/arm/include/asm/scmi-smc.h | 52 +
xen/arch/arm/scmi-smc.c | 163
Could it be moved in separate folder - for example "sci" or "firmware"?
There are definitely more SCMI specific code will be added in the future
as this solution is little bit too simplified.
4 files changed, 226 insertions(+)
create mode 100644 xen/arch/arm/include/asm/scmi-smc.h
create mode 100644 xen/arch/arm/scmi-smc.c
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 323c967361..adf53e2de1 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -245,6 +245,16 @@ config PARTIAL_EMULATION
not been emulated to their complete functionality. Enabling this might
result in unwanted/non-spec compliant behavior.
+config SCMI_SMC
Could you please rename it to clearly specify that it is only dom0/hwdom
specific? Like SCMI_SMC_DOM0 or SCMI_SMC_HW_DOM.
I expect this series to be just a stop gap until we support SCMI for the VMs.
Once this is merge, I don't expect we would want to keep a Kconfig to allow
SCMI just for dom0. Therefore, I am not entirely convinced the proposed new
name is a good idea.
AFAIU, Julien, you don't agree with renaming the config, nor with moving the
support to a separate folder since it's something temporary? That's my view
as well.
These changes do not introduce support for a layer of mediators for
interacting with system firmware, but only for one simplified implementation.
So I suppose the patch set that adds that support also creates the folder
(named 'sci' - per Gregorii's proposal - or 'firmware' to align with Linux),
and the required config.
But I'm up for doing whatever you consider more suitable.
+ bool "Enable forwarding SCMI over SMC calls from Dom0 to EL3 firmware"
+ default y
+ help
+ This option enables basic awareness for SCMI calls using SMC as
+ doorbell mechanism and Shared Memory for transport ("arm,scmi-smc"
+ compatible only). The value of "arm,smc-id" DT property from SCMI
+ firmware node is used to trap and forward corresponding SCMI SMCs
+ to firmware running at EL3, if the call comes from Dom0.
+
endmenu
menu "ARM errata workaround via the alternative framework"
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 7792bff597..b85ad9c13f 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -45,6 +45,7 @@ obj-y += platform_hypercall.o
obj-y += physdev.o
obj-y += processor.o
obj-y += psci.o
+obj-$(CONFIG_SCMI_SMC) += scmi-smc.o
obj-y += setup.o
obj-y += shutdown.o
obj-y += smp.o
diff --git a/xen/arch/arm/include/asm/scmi-smc.h b/xen/arch/arm/
include/asm/scmi-smc.h
new file mode 100644
index 00..c6c0079e86
--- /dev/null
+++ b/xen/arch/arm/include/asm/scmi-smc.h
@@ -0,0 +1,52 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * xen/arch/arm/include/asm/scmi-smc.h
+ *
+ * ARM System Control and Management Interface (SCMI) over SMC
+ * Generic
Re: [RFC PATCH] arm: dom0: allow static memory configuration
Hi Stefano, On 13.02.25 00:11, Stefano Stabellini wrote: On Wed, 12 Feb 2025, Grygorii Strashko wrote: The Arm Xen allocates memory to place Dom0 following the logic described in allocate_memory_11() function which is a bit complicated with major requirement to place Dom0 within the first 128MB of RAM and below 4G. But this doesn't guarantee it will be placed at the same physical base address even between two boots with different configuration (changing the Kernel image size or Initrd size may cause Dom0 base address to change). In case of "thin Dom0" use case, when Dom0 implemented with RTOS like Zephyr, which doesn't use dynamic device-tree parsing, such behavior causes a lot of inconvenience as it is required to perform modification and recompiling of Zephyr image to adjust memory layout. It also prevents from using Initrd with Zephyr, for example, as it's expected to be placed at known, fixed address in memory. This RFC patch introduces the possibility to place Dom0 at fixed physical base address, by checking if "chosen" node contains property "xen,static-mem" and places Dom0 exactly at the specified memory chunk. The implementation follows the same approach as for the static, direct-mapped guest domain in case of dom0less boot. Signed-off-by: Grygorii Strashko I fully support this idea and the addition of static memory support to Dom0. However, I would suggest a different approach regarding the device tree binding. Specifically, I would prefer to avoid introducing additional top-level properties for Dom0 under /chosen. That's was major point declaring it RFC. Instead, we should create a domain node for Dom0 under /chosen, like we do for other DomUs. Jason is currently working on adding a capability properties to the Dom0less domain nodes, allowing us to specify whether a domain is the hardware domain, the control domain, or both (effectively making it Dom0). Once this is in place, we can use static-mem for Dom0 in the same way as always. Good to here that, I assume it can wait (a bit) then. But please note that our requirement here to allow static memory for both dom0less and non-dom0less boot, so here is the question - will bindings and dom0/hwdom/control domain setup be generic? Honestly, for ARM, the discrepancies between boot modes and Xen DT definitions (and actually toolstack) are very confusing :( And now there is also hyperlaunch on the horizon :( [...] BR, -grygorii
[RFC PATCH] arm: dom0: allow static memory configuration
The Arm Xen allocates memory to place Dom0 following the logic described in
allocate_memory_11() function which is a bit complicated with major
requirement to place Dom0 within the first 128MB of RAM and below 4G. But
this doesn't guarantee it will be placed at the same physical base address
even between two boots with different configuration (changing the Kernel
image size or Initrd size may cause Dom0 base address to change).
In case of "thin Dom0" use case, when Dom0 implemented with RTOS like
Zephyr, which doesn't use dynamic device-tree parsing, such behavior
causes a lot of inconvenience as it is required to perform modification and
recompiling of Zephyr image to adjust memory layout.
It also prevents from using Initrd with Zephyr, for example, as it's
expected to be placed at known, fixed address in memory.
This RFC patch introduces the possibility to place Dom0 at fixed physical
base address, by checking if "chosen" node contains property
"xen,static-mem" and placs Dom0 exactly at the specified memory chunk.
The implementation follows the same approach as for the static, direct-mapped
guest domain in case of dom0less boot.
Signed-off-by: Grygorii Strashko
---
docs/misc/arm/device-tree/booting.txt | 20
xen/arch/arm/domain_build.c | 12 +---
xen/common/device-tree/bootfdt.c | 14 ++
3 files changed, 43 insertions(+), 3 deletions(-)
diff --git a/docs/misc/arm/device-tree/booting.txt
b/docs/misc/arm/device-tree/booting.txt
index 9c881baccc19..485dcb82de8c 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -448,6 +448,26 @@ device-tree:
This will reserve a 512MB region starting at the host physical address
0x3000 to be exclusively used by DomU1.
+Dom0 Static Allocation
+==
+
+The Memory can be statically allocated to a Dom0 using the property
+"xen,static-mem" defined under the "\chosen" node. This options allows to use
+RTOS as the dom0 kernel, which support only static memory layout.
+
+Below is an DT example:
+
+/ {
+chosen {
+#address-cells = <0x1>;
+#size-cells = <0x1>;
+xen,static-mem = <0x5000 0x800>;
+...
+};
+
+This will reserve a 128MB region starting at the host physical address
+0x5000 to be exclusively used by Dom0.
+
Static Event Channel
The event channel communication will be established statically between two
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 7b47abade196..8ee280614813 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -31,6 +31,7 @@
#include
#include
#include
+#include
#include
#include
@@ -2272,6 +2273,7 @@ int __init construct_domain(struct domain *d, struct
kernel_info *kinfo)
static int __init construct_dom0(struct domain *d)
{
struct kernel_info kinfo = KERNEL_INFO_INIT;
+const struct dt_device_node *chosen = dt_find_node_by_path("/chosen");
int rc;
/* Sanity! */
@@ -2305,10 +2307,14 @@ static int __init construct_dom0(struct domain *d)
d->arch.type = kinfo.type;
#endif
find_gnttab_region(d, &kinfo);
-if ( is_domain_direct_mapped(d) )
-allocate_memory_11(d, &kinfo);
-else
+if ( is_domain_direct_mapped(d) ) {
+if ( !dt_find_property(chosen, "xen,static-mem", NULL) )
+allocate_memory_11(d, &kinfo);
+else
+assign_static_memory_11(d, &kinfo, chosen);
+} else {
allocate_memory(d, &kinfo);
+}
rc = process_shm_chosen(d, &kinfo);
if ( rc < 0 )
diff --git a/xen/common/device-tree/bootfdt.c b/xen/common/device-tree/bootfdt.c
index 529c91e603ab..563a5436fac0 100644
--- a/xen/common/device-tree/bootfdt.c
+++ b/xen/common/device-tree/bootfdt.c
@@ -413,6 +413,20 @@ static int __init process_chosen_node(const void *fdt, int
node,
using_static_heap = true;
}
+if ( fdt_get_property(fdt, node, "xen,static-mem", NULL) )
+{
+int rc;
+
+printk("Checking for static static-mem in /chosen\n");
+
+rc = device_tree_get_meminfo(fdt, node, "xen,static-mem",
+ address_cells, size_cells,
+ bootinfo_get_reserved_mem(),
+ MEMBANK_STATIC_DOMAIN);
+if ( rc )
+return rc;
+}
+
printk("Checking for initrd in /chosen\n");
prop = fdt_get_property(fdt, node, "linux,initrd-start", &len);
--
2.34.1
Re: [PATCH] xen/arm: fix iomem_ranges cfg in map_range_to_domain()
On 14.02.25 15:15, Julien Grall wrote:
Hi,
On 14/02/2025 12:55, Grygorii Strashko wrote:
Now the following code in map_range_to_domain()
res = iomem_permit_access(d, paddr_to_pfn(addr),
paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
and
res = rangeset_add_range(mr_data->iomem_ranges,
paddr_to_pfn(addr),
paddr_to_pfn_aligned(addr + len - 1));
> > will incorrectly calculate end address of the iomem_range by rounding it up
to the next Xen page, which in turn will give Control domain access to
manage incorrect MMIO range.
I think the key point that needs to be spelled out is that both functions are expecting
"end" to be inclusive.
Whereas the code is thinking that the end should be exclusive.
This is a very common error in Xen and why I have been advocating several times
to switch to use
"start, nr" rather than "start, end".
"
Now the following code in map_range_to_domain()
...
calculates iomem_range end address by rounding it up to the next Xen page
with incorrect assumption that iomem_range end address passed to
iomem_permit_access()/rangeset_add_range()
is exclusive, while it is expected to be inclusive.
It gives Control domain (Dom0) access to manage incorrect MMIO range with one
additional page.
"
I can change it as above. Is it ok?
For example, requested range:
00e614 - 00e6141004 should set e6140:e6141 nr=2, but will configure
e6140 e6142 nr=3 instead.
I am not sure what 'nr' is referring to here.
Sorry, will change to "num_pages"?
Also, with the range you provide above, it means that you will still give access to more than necessary.
That's because you give access to the full page but only the first four bytes
are valid. Is this intended?
It's known and expected for Dom0 as MMIO addresses are taken from Host DT and
not all HW is virtualization friendly
(have HW modules at least 4K page aligned). What is not expected - is to get
access to one additional page.
Note. paddr_to_pfn_aligned() has PAGE_ALIGN() inside.
Fix it, by using paddr_to_pfn(addr + len - 1) in both places to get correct
end address of the iomem_range.
> > Signed-off-by: Grygorii Strashko
I think this wants to have a fixes tag and possibly split in two because I
suspect we may need to backport the changes to different versions.
Ok.
For the second chunk it seems obvious:
Fixes: 57d4d7d4e8f3b (arm/asm/setup.h: Update struct map_range_data to add
rangeset.")
For the first one - not sure, seems:
Fixes: 33233c2758345 ("arch/arm: domain build: let dom0 access I/O memory of mapped
devices")
---
Hi All,
I have a question - the paddr_to_pfn_aligned() is not used any more,
should I remove it as part of this patch?
I would keep it. It might be used in the future.
[...]
Thank you for review.
BR,
-grygorii
[PATCH] xen/arm: fix iomem_ranges cfg in map_range_to_domain()
Now the following code in map_range_to_domain()
res = iomem_permit_access(d, paddr_to_pfn(addr),
paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
and
res = rangeset_add_range(mr_data->iomem_ranges,
paddr_to_pfn(addr),
paddr_to_pfn_aligned(addr + len - 1));
will incorrectly calculate end address of the iomem_range by rounding it up
to the next Xen page, which in turn will give Control domain access to
manage incorrect MMIO range.
For example, requested range:
00e614 - 00e6141004 should set e6140:e6141 nr=2, but will configure
e6140 e6142 nr=3 instead.
Note. paddr_to_pfn_aligned() has PAGE_ALIGN() inside.
Fix it, by using paddr_to_pfn(addr + len - 1) in both places to get correct
end address of the iomem_range.
Signed-off-by: Grygorii Strashko
---
Hi All,
I have a question - the paddr_to_pfn_aligned() is not used any more,
should I remove it as part of this patch?
xen/arch/arm/device.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
index 5610cddcba8e..5e1c1cc326ac 100644
--- a/xen/arch/arm/device.c
+++ b/xen/arch/arm/device.c
@@ -71,7 +71,7 @@ int map_range_to_domain(const struct dt_device_node *dev,
strlen("/reserved-memory/")) != 0 )
{
res = iomem_permit_access(d, paddr_to_pfn(addr),
-paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
+ paddr_to_pfn(addr + len - 1));
if ( res )
{
printk(XENLOG_ERR "Unable to permit to dom%d access to"
@@ -107,7 +107,7 @@ int map_range_to_domain(const struct dt_device_node *dev,
{
res = rangeset_add_range(mr_data->iomem_ranges,
paddr_to_pfn(addr),
- paddr_to_pfn_aligned(addr + len - 1));
+ paddr_to_pfn(addr + len - 1));
if ( res )
return res;
}
--
2.34.1
[PATCH] xen/iocap.h: add documentation
Change rangeset parameters to "start, last" as proposed in [1],
and add documentation for public interface.
No functional changes.
[1] https://patchwork.kernel.org/comment/26251962/
Signed-off-by: Grygorii Strashko
---
xen/include/xen/iocap.h | 134 +---
1 file changed, 112 insertions(+), 22 deletions(-)
diff --git a/xen/include/xen/iocap.h b/xen/include/xen/iocap.h
index ffbc48b60fd5..8845949ab885 100644
--- a/xen/include/xen/iocap.h
+++ b/xen/include/xen/iocap.h
@@ -12,11 +12,21 @@
#include
#include
-static inline int iomem_permit_access(struct domain *d, unsigned long s,
- unsigned long e)
+/**
+ * @brief Gives domain permission to access IOMEM range
+ *
+ * @d: Domain to give IOMEM range access
+ * @start: IOMEM range start address, inclusive
+ * @last: IOMEM range last address, inclusive
+ *
+ * @retval 0 Is successful
+ * @retval -ENOMEM if memory allocation failed
+ */
+static inline int iomem_permit_access(struct domain *d, unsigned long start,
+ unsigned long last)
{
bool flush = cache_flush_permitted(d);
-int ret = rangeset_add_range(d->iomem_caps, s, e);
+int ret = rangeset_add_range(d->iomem_caps, start, last);
if ( !ret && !is_iommu_enabled(d) && !flush )
/*
@@ -29,10 +39,20 @@ static inline int iomem_permit_access(struct domain *d,
unsigned long s,
return ret;
}
-static inline int iomem_deny_access(struct domain *d, unsigned long s,
-unsigned long e)
+/**
+ * @brief Denies domain permission to access IOMEM range
+ *
+ * @d: Domain to deny IOMEM range access
+ * @start: IOMEM range start address, inclusive
+ * @last: IOMEM range last address, inclusive
+ *
+ * @retval 0 Is successful
+ * @retval -ENOMEM if memory allocation failed
+ */
+static inline int iomem_deny_access(struct domain *d, unsigned long start,
+unsigned long last)
{
-int ret = rangeset_remove_range(d->iomem_caps, s, e);
+int ret = rangeset_remove_range(d->iomem_caps, start, last);
if ( !ret && !is_iommu_enabled(d) && !cache_flush_permitted(d) )
/*
@@ -45,23 +65,93 @@ static inline int iomem_deny_access(struct domain *d,
unsigned long s,
return ret;
}
-#define iomem_access_permitted(d, s, e) \
-rangeset_contains_range((d)->iomem_caps, s, e)
-
-#define irq_permit_access(d, i) \
-rangeset_add_singleton((d)->irq_caps, i)
-#define irq_deny_access(d, i) \
-rangeset_remove_singleton((d)->irq_caps, i)
-#define irqs_permit_access(d, s, e) \
-rangeset_add_range((d)->irq_caps, s, e)
-#define irqs_deny_access(d, s, e) \
-rangeset_remove_range((d)->irq_caps, s, e)
-#define irq_access_permitted(d, i) \
-rangeset_contains_singleton((d)->irq_caps, i)
-
-#define pirq_access_permitted(d, i) ({ \
+/**
+ * @brief Checks if domain has permissions to access IOMEM range
+ *
+ * @d: Domain to check IOMEM range access
+ * @start: IOMEM range start address, inclusive
+ * @last: IOMEM range last address, inclusive
+ *
+ * @retval true if access permitted
+ * @retval false if access denied
+ */
+#define iomem_access_permitted(d, start, last) \
+rangeset_contains_range((d)->iomem_caps, start, last)
+
+/**
+ * @brief Gives domain permission to access IRQ
+ *
+ * @d: Domain to give IRQ access
+ * @irq: IRQ number
+ *
+ * @retval 0 Is successful
+ * @retval -ENOMEM if memory allocation failed
+ */
+#define irq_permit_access(d, irq) \
+rangeset_add_singleton((d)->irq_caps, irq)
+
+/**
+ * @brief Denies domain permission to access IRQ
+ *
+ * @d: Domain to deny IRQ access
+ * @irq: IRQ number
+ *
+ * @retval 0 Is successful
+ * @retval -ENOMEM if memory allocation failed
+ */
+#define irq_deny_access(d, irq) \
+rangeset_remove_singleton((d)->irq_caps, irq)
+
+/**
+ * @brief Gives domain permission to access IRQ range
+ *
+ * @d: Domain to give IRQ range access
+ * @start_irq: IRQ range start number, inclusive
+ * @last_irq: IRQ range last number, inclusive
+ *
+ * @retval 0 Is successful
+ * @retval -ENOMEM if memory allocation failed
+ */
+#define irqs_permit_access(d, start_irq, last_irq) \
+rangeset_add_range((d)->irq_caps, start_irq, last_irq)
+
+/**
+ * @brief Denies domain permission to access IRQ range
+ *
+ * @d: Domain to deny IRQ range access
+ * @start_irq: IRQ range start number, inclusive
+ * @last_irq: IRQ range last number, inclusive
+ *
+ * @retval 0 Is successful
+ * @retval -ENOMEM if memory allocation failed
+ */
+#define irqs_deny_access(d, start_irq, last_irq)\
+rangeset_remove_range((d)->irq_caps, start_irq, last_irq)
+
+/**
+
[PATCH v2 1/2] xen/arm: fix iomem permissions cfg in map_range_to_domain()
Now the following code in map_range_to_domain()
res = iomem_permit_access(d, paddr_to_pfn(addr),
paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
calculates the iomem range end address by rounding it up to the next Xen
page with incorrect assumption that iomem range end address passed to
iomem_permit_access() is exclusive, while it is expected to be inclusive.
It gives Control domain (Dom0) access to manage incorrect MMIO range with
one additional page.
For example, if requested range is [00e614:00e6141004] then it expected
to add [e6140:e6141] range (num_pages=2) to the domain iomem_caps rangeset,
but will add [e6140:e6142] (num_pages=3) instead.
To fix it, drop PAGE_ALIGN() from the iomem range end address calculation
formula.
Fixes: 33233c2758345 ("arch/arm: domain build: let dom0 access I/O memory
of mapped devices")
Signed-off-by: Grygorii Strashko
---
xen/arch/arm/device.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
index 5610cddcba8e..97e613e06afa 100644
--- a/xen/arch/arm/device.c
+++ b/xen/arch/arm/device.c
@@ -71,7 +71,7 @@ int map_range_to_domain(const struct dt_device_node *dev,
strlen("/reserved-memory/")) != 0 )
{
res = iomem_permit_access(d, paddr_to_pfn(addr),
-paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
+ paddr_to_pfn(addr + len - 1));
if ( res )
{
printk(XENLOG_ERR "Unable to permit to dom%d access to"
--
2.34.1
[PATCH v2 2/2] xen/arm: fix iomem_ranges cfg in map_range_to_domain()
Now the following code in map_range_to_domain()
res = rangeset_add_range(mr_data->iomem_ranges,
paddr_to_pfn(addr),
paddr_to_pfn_aligned(addr + len - 1));
where
paddr_to_pfn_aligned(paddr) defined as paddr_to_pfn(PAGE_ALIGN(paddr))
calculates the iomem range end address by rounding it up to the next Xen
page with incorrect assumption that iomem range end address passed to
rangeset_add_range() is exclusive, while it is expected to be inclusive.
For example, if requested range is [00e614:00e6141004] then it expected
to add [e6140:e6141] range (num_pages=2) to the mr_data->iomem_ranges
rangeset, but will add [e6140:e6142] (num_pages=3) instead.
To fix it, drop PAGE_ALIGN() from the iomem range end address calculation
formula and just use paddr_to_pfn(addr + len - 1).
Fixes: 57d4d7d4e8f3b (arm/asm/setup.h: Update struct map_range_data to add
rangeset.")
Signed-off-by: Grygorii Strashko
---
xen/arch/arm/device.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
index 97e613e06afa..5e1c1cc326ac 100644
--- a/xen/arch/arm/device.c
+++ b/xen/arch/arm/device.c
@@ -107,7 +107,7 @@ int map_range_to_domain(const struct dt_device_node *dev,
{
res = rangeset_add_range(mr_data->iomem_ranges,
paddr_to_pfn(addr),
- paddr_to_pfn_aligned(addr + len - 1));
+ paddr_to_pfn(addr + len - 1));
if ( res )
return res;
}
--
2.34.1
[PATCH v2 0/2] xen/arm: fix iomem_ranges cfg in map_range_to_domain()
Hi
This series fixes incorrect calculation of iomem_range end address in
map_range_to_domain() which is then passed to iomem_permit_access() and
rangeset_add_range().
Those function expect the iomem_range end address to be inclusive
while calculations are made with incorrect assumption it's exclusive.
Output of "key 'q' (ascii '71') => dump domain (and guest debug) info"
before:
(XEN) I/O Memory { 8000-c000, 3-4, 47ff0-47ff8, 7ff00-7ff01,
c-d, e6020-e6021, ...
after:
(XEN) I/O Memory { 8000-bfff, 3-3, 47ff0-47ff7, 7ff00, c-c,
e6020, ...
Changes in v2:
- split on two patches
- add "fixes" tags
- rewording of commit messages
v1:
https://patchew.org/Xen/20250214125505.2862809-1-grygorii._5fstras...@epam.com/
Grygorii Strashko (2):
xen/arm: fix iomem permissions cfg in map_range_to_domain()
xen/arm: fix iomem_ranges cfg in map_range_to_domain()
xen/arch/arm/device.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--
2.34.1
Re: [PATCH] xen/arm: fix iomem_ranges cfg in map_range_to_domain()
Hi Andrew, Julien, On 14.02.25 16:25, Andrew Cooper wrote: On 14/02/2025 2:10 pm, Grygorii Strashko wrote: For example, requested range: 00e614 - 00e6141004 should set e6140:e6141 nr=2, but will configure e6140 e6142 nr=3 instead. I am not sure what 'nr' is referring to here. Sorry, will change to "num_pages"? I agree Xen needs to be better (and by that, I mean consistent and clear), but there are subtle bugs with most approaches like this. Any exclusive bound, as well as counts like this need $n+1 bits of arithmetic when you want to describe the boundaries of the space. There is also a boundary condition for counts. What map_foo(x, 0) mean? Real hardware uses "last" for describing boundaries (x86 specifically calls this "limit" in the ISA, but it's a trick used by other architectures too). Unlike "end", it's clearly an inclusive bound. Personally, I'd like to see Xen switch to "start, last" pairs. It's unambiguous and has fewest boundary cases. I'm preparing for re-sending the changes with applied Julien's comments, but I'd like to ask for clarification, as I'm not fully understand if I need to perform any other actions regarding above comment, or not. Sorry. BR, -grygorii
Re: [PATCH 0/2] code style exercise: Drivers folder samples
On 16.02.25 12:21, Oleksandr Andrushchenko wrote:
Hello, everybody!
As agreed before [1] I am sending a series to show two samples of the
formatting done with clang-format. The clang-format configuration can be
found at [2]. It already has some notes on previous discussions when
Luca presented his version of that configuration and which I used to
start from.
There are two diff files which show what happens in case the same is
applied to the whole xen/drivers directory:
- first one is the result of the "git diff" command, 1.2M [3]
- the second one is for "git diff --ignire-all-space", 600K [4]
This is not to limit the reviewers from seeing a bigger picture and not
only the files in this series.
N.B. xen/drivers uses tabs a lot, so this is no surprise the size
difference is big enough: roughly space conversion is half of the changes.
While reviewing the changes I have collected some of the unexpected things
done by clang-format and some interesting pieces. You can find those
below. For some of those I have filed an issue on clang-format and hope the
community will lead me in resolving those. Of course what I collected is
not the complete list of such changes, so I hope we can discuss missed
ones here.
From this exercise I can definitely tell that clang-format does help a
lot and has potential to be employed as a code formatting tool for Xen.
Of course it cannot be used as is now and will require discussions on the
Xen coding style and possibly submitting patches to clang-format to
satisfy those which cannot be handled by the tool now.
Stay safe,
Oleksandr Andrushchenko
1. Const string arrays reformatting
In case the length of items change we might need to introduce a bigger
change wrt new formatting of unaffected lines
==
--- a/xen/drivers/acpi/tables.c
+++ b/xen/drivers/acpi/tables.c
@@ -38,10 +38,10 @@
-static const char *__initdata
-mps_inti_flags_polarity[] = { "dfl", "high", "res", "low" };
-static const char *__initdata
-mps_inti_flags_trigger[] = { "dfl", "edge", "res", "level" };
+static const char *__initdata mps_inti_flags_polarity[] = { "dfl", "high",
+"res", "low" };
+static const char *__initdata mps_inti_flags_trigger[] = { "dfl", "edge",
"res",
--- a/xen/drivers/acpi/utilities/utglobal.c
+++ b/xen/drivers/acpi/utilities/utglobal.c
static const char *const acpi_gbl_region_types[ACPI_NUM_PREDEFINED_REGIONS] =
{
- "SystemMemory",
- "SystemIO",
- "PCI_Config",
- "EmbeddedControl",
- "SMBus",
- "CMOS",
- "PCIBARTarget",
- "DataTable"
+"SystemMemory", "SystemIO", "PCI_Config", "EmbeddedControl",
+"SMBus","CMOS", "PCIBARTarget", "DataTable"
};
2. Long strings in ptintk violations
I filed an issue on printk format strings [5]
==
@@ -225,9 +231,9 @@ void __init acpi_table_print_madt_entry(struct
acpi_subtable_header *header)
printk(KERN_DEBUG PREFIX
- "GIC Distributor (gic_id[0x%04x] address[0x%"PRIx64"]
gsi_base[%d])\n",
- p->gic_id, p->base_address,
- p->global_irq_base);
+ "GIC Distributor (gic_id[0x%04x] address[0x%" PRIx64
+ "] gsi_base[%d])\n",
+ p->gic_id, p->base_address, p->global_irq_base);
@@ -629,12 +628,14 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header)
- printk(XENLOG_ERR VTDPREFIX
- "Overlapping RMRRs [%"PRIx64",%"PRIx64"] and
[%"PRIx64",%"PRIx64"]\n",
- rmrru->base_address, rmrru->end_address,
- base_addr, end_addr);
+printk(XENLOG_ERR VTDPREFIX "Overlapping RMRRs [%" PRIx64
+",%" PRIx64 "] and [%" PRIx64
+",%" PRIx64 "]\n",
+ rmrru->base_address, rmrru->end_address, base_addr,
+ end_addr);
It doesn't understand properly things like "PRIx64" :(
Most of other items, I've mentioned already,
Unhappy: it's probably "known" things - identification of complex defines and
static struct/arrays declarations.
And for them, probably, no magic automation tools exist which will satisfy
everybody as,
at least static definitions are usually formatted to achieve better readability
which is always subjective.
The tags /* clang-format off/clang-format on */ might be useful.
[..]
Honestly, It looks a bit strange that Xen community is considering batch
automated code formatting,
For example Linux kernel cleanly rejected such approach.
Linux kernel docs "4.1.1. Coding style" section [1].
Another thing, checking the new code and accepting code style violations (if
any) on per-patch basis.
[1] https://docs.kernel.org/process/4.Coding.html
BR,
-grygorii
[RFC PATCH v3 4/7] xen/arm: scmi: introduce SCI SCMI SMC multi-agent driver
fix up shmem
node according to assigned agent_id.
Guest domains enable SCMI SMC:
- xl.cfg: add configuration option as below
arm_sci = "type=scmi_smc_multiagent,agent_id=2"
- xl.cfg: enable access to the "arm,scmi-shmem" which should correspond
assigned agent_id for
the domain, for example:
iomem = [
"47ff2,1@22001",
]
- DT: add SCMI nodes to the Driver domain partial device tree as in the
below example. The "arm,smc-id" should correspond assigned agent_id for the
domain:
passthrough {
scmi_shm_0: sram@22001000 {
compatible = "arm,scmi-shmem";
reg = <0x0 0x22001000 0x0 0x1000>;
};
firmware {
compatible = "simple-bus";
scmi: scmi {
compatible = "arm,scmi-smc";
arm,smc-id = <0x8204>;
shmem = <&scmi_shm_0>;
...
}
}
}
SCMI "4.2.1.1 Device specific access control"
The XEN SCI SCMI SMC multi-agent driver performs "access-controller" provider
function
in case EL3 SCMI FW implements SCMI "4.2.1.1 Device specific access control"
and provides the
BASE_SET_DEVICE_PERMISSIONS command to configure the devices that an agents
have access to.
The DT SCMI node should "#access-controller-cells=<1>" property and DT devices
should be bound
to the Xen SCMI.
&i2c1 {
access-controllers = <&scmi 0>;
};
The Dom0 and dom0less (TBD) domains DT devices will be processed automatically
through
sci_assign_dt_device() call, but to assign SCMI devices from toolstack the
xl.cfg:"dtdev" property
shell be used:
dtdev = [
"/soc/i2c@e6508000",
]
xl.cfg:dtdev will contain all nodes which are under SCMI management (not only
those which are behind IOMMU).
TODO:
- dom0less is not fully supported yet
- move memcpy_fro/tomio in separate patch
[1]
https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
[2]
https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/access-controllers/access-controllers.yaml
Signed-off-by: Oleksii Moisieiev
Signed-off-by: Grygorii Strashko
---
docs/man/xl.cfg.5.pod.in| 15 +
docs/misc/xen-command-line.pandoc | 9 +
tools/libs/light/libxl_arm.c| 4 +
tools/libs/light/libxl_types.idl| 4 +-
tools/xl/xl_parse.c | 17 +
xen/arch/arm/domain_build.c | 3 +-
xen/arch/arm/firmware/Kconfig | 11 +
xen/arch/arm/firmware/Makefile | 1 +
xen/arch/arm/firmware/scmi-proto.h | 164
xen/arch/arm/firmware/scmi-shmem.c | 172
xen/arch/arm/firmware/scmi-shmem.h | 45 +
xen/arch/arm/firmware/scmi-smc-multiagent.c | 856
xen/include/public/arch-arm.h | 3 +
13 files changed, 1302 insertions(+), 2 deletions(-)
create mode 100644 xen/arch/arm/firmware/scmi-proto.h
create mode 100644 xen/arch/arm/firmware/scmi-shmem.c
create mode 100644 xen/arch/arm/firmware/scmi-shmem.h
create mode 100644 xen/arch/arm/firmware/scmi-smc-multiagent.c
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 7edf272386e3..fc6041724a13 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -3126,6 +3126,21 @@ single SCMI OSPM agent support.
Should be used together with B Xen command line
option.
+=item B
+
+Enables ARM SCMI SMC multi-agent support for the guest by enabling SCMI over
+SMC calls forwarding from domain to the EL3 firmware (like Trusted Firmware-A)
+with a multi SCMI OSPM agent support. The SCMI B should be
+specified for the guest.
+
+=back
+
+=item B
+
+Specifies a non-zero ARM SCI agent id for the guest. This option is mandatory
+if the SCMI SMC support is enabled for the guest. The agent ids of domains
+existing on a single host must be unique.
+
=back
=back
diff --git a/docs/misc/xen-command-line.pandoc
b/docs/misc/xen-command-line.pandoc
index 8e50f6b7c7ac..bc3c64d6ec90 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1091,6 +1091,15 @@ which serves as Driver domain. The SCMI will be disabled
for Dom0/hwdom and
SCMI nodes removed from Dom0/hwdom device tree.
(for example, thin Dom0 with Driver domain use-case).
+### dom0_scmi_agent_id (ARM)
+> `= `
+
+The option is available when `CONFIG_SCMI_SMC_MA` is compiled in, and allows to
+enable SCMI functionality for Dom0 by specifying a non-zero ARM SCMI agent id.
+The SCMI will be disabled for Dom0 if this option is not specified
+(for example, thin Dom0 or dom0less use-cases).
+The agent ids of domains existing on a single host must be unique.
+
### dtuart (ARM)
> `= path [:options]`
diff --git a/tools/libs/light/libxl_
Re: [PATCH v2 1/7] xen/arm: allow PCI host bridge to have private data
On 11.03.25 12:24, Mykyta Poturai wrote:
From: Oleksandr Andrushchenko
Some of the PCI host bridges require private data. Create a generic
approach for that, so such bridges may request the private data to
be allocated during initialization.
Signed-off-by: Oleksandr Andrushchenko
Signed-off-by: Mykyta Poturai
---
v1->v2:
* no change
---
xen/arch/arm/include/asm/pci.h | 4 +++-
xen/arch/arm/pci/pci-host-common.c | 18 +-
xen/arch/arm/pci/pci-host-generic.c | 2 +-
xen/arch/arm/pci/pci-host-zynqmp.c | 2 +-
4 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
index 7f77226c9b..44344ac8c1 100644
--- a/xen/arch/arm/include/asm/pci.h
+++ b/xen/arch/arm/include/asm/pci.h
@@ -66,6 +66,7 @@ struct pci_host_bridge {
uint16_t segment;/* Segment number */
struct pci_config_window* cfg; /* Pointer to the bridge config window */
const struct pci_ops *ops;
+void *priv; /* Private data of the bridge. */
};
struct pci_ops {
@@ -95,7 +96,8 @@ struct pci_ecam_ops {
extern const struct pci_ecam_ops pci_generic_ecam_ops;
int pci_host_common_probe(struct dt_device_node *dev,
- const struct pci_ecam_ops *ops);
+ const struct pci_ecam_ops *ops,
+ size_t priv_sz);
int pci_generic_config_read(struct pci_host_bridge *bridge, pci_sbdf_t sbdf,
uint32_t reg, uint32_t len, uint32_t *value);
int pci_generic_config_write(struct pci_host_bridge *bridge, pci_sbdf_t sbdf,
diff --git a/xen/arch/arm/pci/pci-host-common.c
b/xen/arch/arm/pci/pci-host-common.c
index c0faf0f436..be7e6c3510 100644
--- a/xen/arch/arm/pci/pci-host-common.c
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -209,7 +209,8 @@ static int pci_bus_find_domain_nr(struct dt_device_node
*dev)
}
int pci_host_common_probe(struct dt_device_node *dev,
- const struct pci_ecam_ops *ops)
+ const struct pci_ecam_ops *ops,
+ size_t priv_sz)
{
struct pci_host_bridge *bridge;
struct pci_config_window *cfg;
@@ -241,11 +242,26 @@ int pci_host_common_probe(struct dt_device_node *dev,
printk(XENLOG_ERR "Inconsistent \"linux,pci-domain\" property in
DT\n");
BUG();
}
+
bridge->segment = domain;
+
+if ( priv_sz )
+{
+bridge->priv = xzalloc_bytes(priv_sz);
+if ( !bridge->priv )
+{
+err = -ENOMEM;
+goto err_priv;
+}
+}
+
I'd like to propose to move allocation into pci_alloc_host_bridge()
to keep mallocs in one place and do it earlier, before proceeding
with other initialization steps.
Also the pci_alloc_host_bridge() could be made static, seems.
pci_add_host_bridge(bridge);
return 0;
+err_priv:
+xfree(bridge->priv);
+
err_exit:
xfree(bridge);
[...]
-grygorii
[RFC PATCH v3 2/7] xen/arm: scmi-smc: update to be used under sci subsystem
The introduced SCI (System Control Interface) subsystem provides unified
interface to integrate in Xen SCI drivers which adds support for ARM
firmware (EL3, SCP) based software interfaces (like SCMI) that are used in
system management. The SCI subsystem allows to add drivers for different FW
interfaces or have different drivers for the same FW interface (for example,
SCMI with different transports).
This patch updates SCMI over SMC calls handling layer, introduced by
commit 3e322bef8bc0 ("xen/arm: firmware: Add SCMI over SMC calls handling
layer"), to be SCI driver:
- convert to DT device;
- convert to SCI Xen interface.
Signed-off-by: Grygorii Strashko
---
xen/arch/arm/firmware/Kconfig| 13 ++-
xen/arch/arm/firmware/scmi-smc.c | 93 +++-
xen/arch/arm/include/asm/firmware/scmi-smc.h | 41 -
xen/arch/arm/vsmc.c | 5 +-
xen/include/public/arch-arm.h| 1 +
5 files changed, 64 insertions(+), 89 deletions(-)
delete mode 100644 xen/arch/arm/include/asm/firmware/scmi-smc.h
diff --git a/xen/arch/arm/firmware/Kconfig b/xen/arch/arm/firmware/Kconfig
index fc7918c7fc56..02d7b600317f 100644
--- a/xen/arch/arm/firmware/Kconfig
+++ b/xen/arch/arm/firmware/Kconfig
@@ -8,9 +8,18 @@ config ARM_SCI
menu "Firmware Drivers"
+choice
+ prompt "ARM SCI driver type"
+ default ARM_SCI_NONE
+ help
+ Choose which ARM SCI driver to enable.
+
+config ARM_SCI_NONE
+ bool "none"
+
config SCMI_SMC
bool "Forward SCMI over SMC calls from hwdom to EL3 firmware"
- default y
+ select ARM_SCI
help
This option enables basic awareness for SCMI calls using SMC as
doorbell mechanism and Shared Memory for transport ("arm,scmi-smc"
@@ -18,4 +27,6 @@ config SCMI_SMC
firmware node is used to trap and forward corresponding SCMI SMCs
to firmware running at EL3, for calls coming from the hardware domain.
+endchoice
+
endmenu
diff --git a/xen/arch/arm/firmware/scmi-smc.c b/xen/arch/arm/firmware/scmi-smc.c
index 33473c04b181..188bd659513b 100644
--- a/xen/arch/arm/firmware/scmi-smc.c
+++ b/xen/arch/arm/firmware/scmi-smc.c
@@ -9,6 +9,7 @@
* Copyright 2024 NXP
*/
+#include
#include
#include
#include
@@ -16,12 +17,11 @@
#include
#include
+#include
#include
-#include
#define SCMI_SMC_ID_PROP "arm,smc-id"
-static bool __ro_after_init scmi_enabled;
static uint32_t __ro_after_init scmi_smc_id;
/*
@@ -41,14 +41,11 @@ static bool scmi_is_valid_smc_id(uint32_t fid)
*
* Returns true if SMC was handled (regardless of response), false otherwise.
*/
-bool scmi_handle_smc(struct cpu_user_regs *regs)
+static bool scmi_handle_smc(struct cpu_user_regs *regs)
{
uint32_t fid = (uint32_t)get_user_reg(regs, 0);
struct arm_smccc_res res;
-if ( !scmi_enabled )
-return false;
-
if ( !scmi_is_valid_smc_id(fid) )
return false;
@@ -78,49 +75,45 @@ bool scmi_handle_smc(struct cpu_user_regs *regs)
return true;
}
-static int __init scmi_check_smccc_ver(void)
+static int scmi_smc_domain_init(struct domain *d,
+struct xen_domctl_createdomain *config)
{
-if ( smccc_ver < ARM_SMCCC_VERSION_1_1 )
-{
-printk(XENLOG_WARNING
- "SCMI: No SMCCC 1.1 support, SCMI calls forwarding disabled\n");
-return -ENOSYS;
-}
+if ( !is_hardware_domain(d) )
+return 0;
+d->arch.sci_enabled = true;
+printk(XENLOG_DEBUG "SCMI: %pd init\n", d);
return 0;
}
-static int __init scmi_dt_init_smccc(void)
+static void scmi_smc_domain_destroy(struct domain *d)
{
-static const struct dt_device_match scmi_ids[] __initconst =
-{
-/* We only support "arm,scmi-smc" binding for now */
-DT_MATCH_COMPATIBLE("arm,scmi-smc"),
-{ /* sentinel */ },
-};
-const struct dt_device_node *scmi_node;
-int ret;
+if ( !is_hardware_domain(d) )
+return;
-/* If no SCMI firmware node found, fail silently as it's not mandatory */
-scmi_node = dt_find_matching_node(NULL, scmi_ids);
-if ( !scmi_node )
-return -EOPNOTSUPP;
+printk(XENLOG_DEBUG "SCMI: %pd destroy\n", d);
+}
-ret = dt_property_read_u32(scmi_node, SCMI_SMC_ID_PROP, &scmi_smc_id);
-if ( !ret )
+static int __init scmi_check_smccc_ver(void)
+{
+if ( smccc_ver < ARM_SMCCC_VERSION_1_1 )
{
-printk(XENLOG_ERR "SCMI: No valid \"%s\" property in \"%s\" DT node\n",
- SCMI_SMC_ID_PROP, scmi_node->full_name);
-return -ENOENT;
+printk(XENLOG_WARNING
+ "SCMI: No SMCCC 1.1 support, SCMI calls forwarding disabled\n");
+return -ENOSYS;
}
-scmi_enabled = t
[RFC PATCH v3 0/7] xen/arm: scmi: introduce SCI SCMI SMC multi-agent support
Hi,
This is respin of RFCv2 series from Oleksii Moisieiev [1] which was send pretty
long time ago (2022),
with the main intention is to resume this discussion in public and gather more
opinions.
Hence the code was previously sent long time ago there are pretty high number
of changes,
including rebase on newer Xen version 4.20.0-rc2, which already contains some
basic SCMI SMC driver
introduced by Andrei Cherechesu, commit 3e322bef8bc0 ("xen/arm: firmware: Add
SCMI over SMC calls
handling layer").
Patch 1 "xen/arm: add generic SCI subsystem"
- rebased and refactored
- introduced DEVICE_ARM_SCI DT device class and used for SCI drivers probing
instead of custom,
linker sections based implementation.
- added SCI API for Dom0 DT handling, instead of manipulating with ARM arch
dom0 code directly.
- TODO: RFC changes in XEN_DOMCTL_assign_device OP processing
Patch 2 "xen/arm: scmi-smc: update to be used under sci subsystem"
- update driver introduced by commit 3e322bef8bc0 ("xen/arm: firmware: Add SCMI
over SMC calls
handling layer") be used under sci subsystem.
- no functional changes in general
Patch 3 "xen/arm: scmi-smc: passthrough SCMI SMC to guest domain
This is new change which allows passthrough SCMI SMC, single agent interface to
guest domain
cover use case "thin Dom0 with guest domain, which serves as Driver domain".
See patch commit message for full description.
Patch 4 - xen/arm: scmi: introduce SCI SCMI SMC multi-agent driver
- added "xen,scmi-secondary-agents" property in "chosen" to inform SCI SCMI
multi-agent driver
about available agents and their configuration. It defines to
map.
This option is Xen specific as Xen is the only one entry in the system which
need to know
about SCMI multi-agent support and configuration.
- each guest using SCMI should be configured with SCMI agent_id, so SCMI
FW can implement Agent-specific permission policy.
-- dom0: dom0_scmi_agent_id= in Xen command line option
-- toolstack: arm_sci = "type=scmi_smc_multiagent,agent_id="
-- dom0less: todo: "xen,sci_type", "xen,sci_agent_id" properties in
"xen,domain" nodes.
- factored out SCMI generic definitions (re-usable)
- factored out SCMI shmem code (re-usable)
- the SCMI passthrough configuration for guest domains is similar to any other
HW passthrough cfg.
Patches 5-7
- no major changes, except to follow rebase and changes in previous patches
Regarding patches 5-7 I'd like to rise a question and I, personally, feel very
skeptical doing any
kind of SCMI DT nodes generation as from toolstack as from Xen.
1) SCMI is no differ as any other HW MFD device, and HW passthrough
configuration works for it in
the same way.
2) if toolstack generates DT then dom0less case might need it also, but this
means more code in Xen,
so, with certification in mind, it means more overhead requirements, docs
and testing.
In my opinion if something can be done outside "kernel" - it should.
So better invest in tools (imagebuilder, lopper, etc.) instead.
3) Hence SCMI DT bindings are pretty complex the corresponding guest DT nodes
can't be generated
from scratch - the user still need to add scmi node, protocols and protocols
subnodes in the
partial device tree, at least empty. But stop, not exactly empty - the
properties like
"#clock-cells" need to be added to avoid DTC warnings. Such behavior is
rather confusing than
helpful.
4) Exposing the Host Device tree in Dom0 is another questionable thing for
toolstack SCMI DT
generation. It consumes 128K of memory on Renesas r8a779g0-whitehawk.
5) No needs for additional public API (XEN_DOMCTL_get_sci_info,
GUEST_SCI_SHMEM_BASE..) if dropped
Code can be found at:
https://github.com/GrygiriiS/xen/commits/master_v4h_sci_v13_dt_gen
[1] RFC v2:
https://patchwork.kernel.org/project/xen-devel/cover/cover.1644341635.git.oleksii_moisie...@epam.com/
SCMI spec:
https://developer.arm.com/documentation/den0056/e/?lang=en
SCMI bindings:
https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/access-controllers/access-controllers.yaml
Reference EL3 FW:
RPI5: https://github.com/xen-troops/arm-trusted-firmware/commits/rpi5_dev/
Renesas v4h:
https://github.com/GrygiriiS/arm-trusted-firmware/commits/rcar_gen4_v2.7_v4x-scmi_upd/
base-commit: c3f5d1bb40b5 ("automation/cirrus-ci: introduce FreeBSD randconfig
builds")
Grygorii Strashko (2):
xen/arm: scmi-smc: update to be used under sci subsystem
xen/arm: scmi-smc: passthrough SCMI SMC to domain, single agent
Oleksii Moisieiev (5):
xen/arm: add generic SCI subsystem
xen/arm: scmi: introduce SCI SCMI SMC multi-agent dri
[RFC PATCH v3 5/7] libs: libxenhypfs - handle blob properties
From: Oleksii Moisieiev libxenhypfs will return blob properties as is. This output can be used to retrieve information from the hypfs. Caller is responsible for parsing property value. Signed-off-by: Oleksii Moisieiev Reviewed-by: Volodymyr Babchuk --- tools/libs/hypfs/core.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/tools/libs/hypfs/core.c b/tools/libs/hypfs/core.c index 52b30db8d777..d09bba7d8c86 100644 --- a/tools/libs/hypfs/core.c +++ b/tools/libs/hypfs/core.c @@ -307,8 +307,6 @@ char *xenhypfs_read(xenhypfs_handle *fshdl, const char *path) errno = EISDIR; break; case xenhypfs_type_blob: -errno = EDOM; -break; case xenhypfs_type_string: ret_buf = buf; buf = NULL; -- 2.34.1
Re: [PATCH v2 1/2] xen/arm: fix iomem permissions cfg in map_range_to_domain()
Hi
On 19.02.25 13:25, Julien Grall wrote:
Hi Grygorii,
On 18/02/2025 11:22, Grygorii Strashko wrote:
Now the following code in map_range_to_domain()
res = iomem_permit_access(d, paddr_to_pfn(addr),
paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
calculates the iomem range end address by rounding it up to the next Xen
page with incorrect assumption that iomem range end address passed to
iomem_permit_access() is exclusive, while it is expected to be inclusive.
It gives Control domain (Dom0) access to manage incorrect MMIO range with
one additional page.
For example, if requested range is [00e614:00e6141004] then it expected
to add [e6140:e6141] range (num_pages=2) to the domain iomem_caps rangeset,
but will add [e6140:e6142] (num_pages=3) instead.
To fix it, drop PAGE_ALIGN() from the iomem range end address calculation
formula.
Fixes: 33233c2758345 ("arch/arm: domain build: let dom0 access I/O memory
of mapped devices")
Signed-off-by: Grygorii Strashko
Reviewed-by: Julien Grall
Sorry, that I'm disturbing you, but do i need to perform any additional actions
here?
Cheers,
---
xen/arch/arm/device.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
index 5610cddcba8e..97e613e06afa 100644
--- a/xen/arch/arm/device.c
+++ b/xen/arch/arm/device.c
@@ -71,7 +71,7 @@ int map_range_to_domain(const struct dt_device_node *dev,
strlen("/reserved-memory/")) != 0 )
{
res = iomem_permit_access(d, paddr_to_pfn(addr),
- paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
+ paddr_to_pfn(addr + len - 1));
if ( res )
{
printk(XENLOG_ERR "Unable to permit to dom%d access to"
Best regards,
-grygorii
[RFC PATCH v3 1/7] xen/arm: add generic SCI subsystem
From: Oleksii Moisieiev
This patch adds the basic framework for ARM SCI mediator. SCI is System
Control Interface, which is designed to redirect requests from the Domains
to ARM specific Firmware (for example SCMI). This will allow the devices,
passed-through to the different Domains, to access to the System resources
(such as clocks/resets etc) by sending requests to the firmware.
ARM SCI subsystem allows to implement different SCI drivers to handle
specific ARM firmware interfaces (like ARM SCMI) and mediate requests
between the Domains and the Firmware. Also it allows SCI drivers to perform
proper action during Domain creation/destruction which is vital for
handling use cases like Domain reboot.
This patch introduces new DEVICE_ARM_SCI device subclass for probing SCI
drivers basing on device tree, SCI drivers register itself with
DT_DEVICE_START/END macro. On init - the SCI drivers should register its
SCI ops with sci_register(). Only one SCI driver can be supported.
At run-time, the following SCI API calls are introduced:
- sci_domain_sanitise_config() called from arch_sanitise_domain_config()
- sci_domain_init() called from arch_domain_create()
- sci_relinquish_resources() called from domain_relinquish_resources()
- sci_domain_destroy() called from arch_domain_destroy()
- sci_handle_call() called from vsmccc_handle_call()
- sci_dt_handle_node()
sci_dt_finalize() called from handle_node() (Dom0 DT)
Signed-off-by: Oleksii Moisieiev
Signed-off-by: Grygorii Strashko
---
MAINTAINERS | 6 +
xen/arch/arm/device.c | 5 +
xen/arch/arm/dom0less-build.c | 13 ++
xen/arch/arm/domain.c | 12 +-
xen/arch/arm/domain_build.c | 8 +
xen/arch/arm/firmware/Kconfig | 8 +
xen/arch/arm/firmware/Makefile | 1 +
xen/arch/arm/firmware/sci.c | 187 +
xen/arch/arm/include/asm/domain.h | 5 +
xen/arch/arm/include/asm/firmware/sci.h | 214
xen/arch/arm/vsmc.c | 3 +
xen/common/domctl.c | 13 ++
xen/drivers/passthrough/device_tree.c | 7 +
xen/include/asm-generic/device.h| 1 +
xen/include/public/arch-arm.h | 4 +
15 files changed, 486 insertions(+), 1 deletion(-)
create mode 100644 xen/arch/arm/firmware/sci.c
create mode 100644 xen/arch/arm/include/asm/firmware/sci.h
diff --git a/MAINTAINERS b/MAINTAINERS
index c11b82eca98f..c0e8143dca63 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -526,6 +526,12 @@ S: Supported
F: xen/arch/arm/include/asm/tee/
F: xen/arch/arm/tee/
+SCI MEDIATORS
+M: Oleksii Moisieiev
+S: Supported
+F: xen/arch/arm/sci
+F: xen/include/asm-arm/sci
+
TOOLSTACK
M: Anthony PERARD
S: Supported
diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
index 5610cddcba8e..bdab96a408c4 100644
--- a/xen/arch/arm/device.c
+++ b/xen/arch/arm/device.c
@@ -13,6 +13,7 @@
#include
#include
+#include
#include
int map_irq_to_domain(struct domain *d, unsigned int irq,
@@ -303,6 +304,10 @@ int handle_device(struct domain *d, struct dt_device_node
*dev, p2m_type_t p2mt,
return res;
}
}
+
+res = sci_assign_dt_device(d, dev);
+if ( res )
+return res;
}
res = map_device_irqs_to_domain(d, dev, own_device, irq_ranges);
diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
index 49d1f14d659b..c463ab3eaca5 100644
--- a/xen/arch/arm/dom0less-build.c
+++ b/xen/arch/arm/dom0less-build.c
@@ -14,6 +14,7 @@
#include
#include
#include
+#include
#include
#include
@@ -321,6 +322,10 @@ static int __init handle_passthrough_prop(struct
kernel_info *kinfo,
return -EINVAL;
}
+res = sci_assign_dt_device(kinfo->d, node);
+if ( res )
+return res;
+
res = map_device_irqs_to_domain(kinfo->d, node, true, NULL);
if ( res < 0 )
return res;
@@ -970,6 +975,14 @@ void __init create_domUs(void)
if ( !llc_coloring_enabled && llc_colors_str )
panic("'llc-colors' found, but LLC coloring is disabled\n");
+/*
+ * TODO: enable ARM SCI for dom0less case
+ * The configuration need to be retrieved from DT
+ * - arch.arm_sci_type, like "xen,sci_type"
+ * - arch.arm_sci_agent_id, like "xen,sci_agent_id"
+ */
+d_cfg.arch.arm_sci_type = XEN_DOMCTL_CONFIG_ARM_SCI_NONE;
+
/*
* The variable max_init_domid is initialized with zero, so here it's
* very important to use the pre-increment operator to call
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 3ba959f86633..652aeb7a55de 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -25,6 +25,7 @@
#include
#include
#include
+#inc
[RFC PATCH v3 6/7] xen/arm: Export host device-tree to hypfs
From: Oleksii Moisieiev
If enabled, host device-tree will be exported to hypfs and can be
accessed through /devicetree path.
Exported device-tree has the same format, as the device-tree
exported to the sysfs by the Linux kernel.
This is useful when XEN toolstack needs an access to the host device-tree.
Signed-off-by: Oleksii Moisieiev
---
xen/arch/arm/Kconfig | 16
xen/arch/arm/Makefile | 1 +
xen/arch/arm/host_dtb_export.c | 28
3 files changed, 45 insertions(+)
create mode 100644 xen/arch/arm/host_dtb_export.c
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index a26d3e11827c..fa51244e2706 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -125,6 +125,22 @@ config DOM0LESS_BOOT
Xen boot without the need of a control domain (Dom0), which could be
present anyway.
+config HOST_DTB_EXPORT
+ bool "Export host device tree to hypfs if enabled"
+ depends on ARM && HYPFS && !ACPI
+ help
+
+ Export host device-tree to hypfs so toolstack can have an access for
the
+ host device tree from Dom0. If you unsure say N.
+
+config HOST_DTB_MAX_SIZE
+ int "Max host dtb export size"
+ depends on HOST_DTB_EXPORT
+ default 8192
+ help
+
+ Maximum size of the host device-tree exported to hypfs.
+
config GICV3
bool "GICv3 driver"
depends on !NEW_VGIC
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 43ab5e8f2550..1518592deb64 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_DOM0LESS_BOOT) += dom0less-build.init.o
obj-y += domain.o
obj-y += domain_build.init.o
obj-y += domctl.o
+obj-$(CONFIG_HOST_DTB_EXPORT) += host_dtb_export.o
obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
obj-y += efi/
obj-y += gic.o
diff --git a/xen/arch/arm/host_dtb_export.c b/xen/arch/arm/host_dtb_export.c
new file mode 100644
index ..c9beb2803883
--- /dev/null
+++ b/xen/arch/arm/host_dtb_export.c
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Export host FDT to the hypfs
+ *
+ * Copyright (C) 2024 EPAM Systems
+ */
+
+#include
+#include
+#include
+#include
+
+static HYPFS_VARSIZE_INIT(dt_prop, XEN_HYPFS_TYPE_BLOB,
+"devicetree", CONFIG_HOST_DTB_MAX_SIZE,
+&hypfs_leaf_ro_funcs);
+
+static int __init host_dtb_export_init(void)
+{
+ASSERT(dt_host && (dt_host->sibling == NULL));
+
+dt_prop.u.content = device_tree_flattened;
+dt_prop.e.size = fdt_totalsize(device_tree_flattened);
+hypfs_add_leaf(&hypfs_root, &dt_prop, true);
+
+return 0;
+}
+
+__initcall(host_dtb_export_init);
--
2.34.1
Re: [PATCH] xen/iocap.h: add documentation
Hi Jan, On 11.03.25 17:35, Jan Beulich wrote: On 11.03.2025 15:53, Grygorii Strashko wrote: On 05.03.25 12:37, Jan Beulich wrote: On 24.02.2025 12:38, Grygorii Strashko wrote: Change rangeset parameters to "start, last" as proposed in [1], and add documentation for public interface. No functional changes. [1] https://patchwork.kernel.org/comment/26251962/ Signed-off-by: Grygorii Strashko To be honest, this is getting too verbose for my taste. I also don't think title and description fit together: One says the main thing the patch does is add doc, the other says the main thing is the parameter renaming. When then there's at least one further parameter which is also renamed, despite not fitting the description. I can update subject and commit message and resend. This would address the latter part of my comment, but ... Or you want me to drop some parts? ... only this would address the former part. I'm very sorry, but I feel very much confused about your above comment :( So I'd be appreciated if You can provide some clarification here. 1) you do not want API documentation at all? 2) you want API documentation, but only for some API? 3) you are not satisfied with documentation style? In case 3, how do you want it to be look like? Could you point on any .h or function in Xen, to inherit the doc style? Here I've followed doxygen style (like in xen/include/xen/vmap.h for example) Before proceeding I've checked CODING_STYLE and other headers as well and saw that there is no generic style for code documentation. Best regards, -grygorii
Re: [PATCH] xen/iocap.h: add documentation
Hi Jan, On 05.03.25 12:37, Jan Beulich wrote: On 24.02.2025 12:38, Grygorii Strashko wrote: Change rangeset parameters to "start, last" as proposed in [1], and add documentation for public interface. No functional changes. [1] https://patchwork.kernel.org/comment/26251962/ Signed-off-by: Grygorii Strashko To be honest, this is getting too verbose for my taste. I also don't think title and description fit together: One says the main thing the patch does is add doc, the other says the main thing is the parameter renaming. When then there's at least one further parameter which is also renamed, despite not fitting the description. I can update subject and commit message and resend. Or you want me to drop some parts? [...] Thank you for your review. BR, -grygorii
[RFC PATCH v3 7/7] xen/arm: scmi: generate scmi dt node for DomUs
From: Oleksii Moisieiev
This feature introduces SCMI support for DomU domains with partial SCMI DT
node generation.
During domain creation the following prerequisites are expected:
- SCMI node template in partial device-tree, which should contain all
subnodes used by DomU:
/ {
firmware {
scmi {
scmi_reset: protocol@19 {
\#reset-cells = <1>;
};
scmi_clk: protocol@14 {
\#clock-cells = <1>;
};
scmi_pinctrl: protocol@19 {
sdio_mux: mux {
};
mux1: mux1 {
};
};
};
};
passthrough {
sdio {
pinctrl-0 = <&sdio_mux>;
resets = <&scmi_reset 0x1>;
clocks = <&scmi_clk 0x1>;
};
dev1 {
resets = <&scmi_reset 2>;
pinctrl-0 = <&mux1>;
};
};
};
- properly defined "arm_sci" property in domain xl.cfg:
arm_sci = "type=scmi_smc_multiagent,agent_id=2"
- Platform/Xen DT exposed to Dom0 through Xen hypfs.
The Xen toolstack:
- obtains from Xen information about phys address of the SCMI shmem and
SMC/HVC id used by specified SCMI agent_id (domctl
XEN_DOMCTL_get_sci_info)
- creates the SCMI shmem node in domain DT using predefined guest MMIO
mappings and DT phandle
GUEST_SCI_SHMEM_BASE xen_mk_ullong(0x22001000)
GUEST_SCI_SHMEM_SIZE xen_mk_ullong(0x01000)
GUEST_PHANDLE_SCMI (GUEST_PHANDLE_IOMMU + 1)
- creates SCMI node in domain DT with:
- "shmem" phandle sets to GUEST_PHANDLE_SCMI
- "arm,smc-id" sets to SMC/HVC id obtained from Xen
- parses partial device tree and creates corresponding SCMI subnodes in
domain DT. All SCMI subnodes properties are copied from Xen DT except
phandles, which are taken from partial DT.
- maps the SCMI shmem into DomU GUEST_SCI_SHMEM_BASE address.
Signed-off-by: Oleksii Moisieiev
Signed-off-by: Grygorii Strashko
---
tools/include/xenctrl.h | 3 +
tools/libs/ctrl/xc_domain.c | 18 ++
tools/libs/light/libxl_arm.c| 294 +++-
tools/libs/light/libxl_create.c | 12 +
tools/libs/light/libxl_internal.h | 3 +
xen/arch/arm/domctl.c | 22 ++
xen/arch/arm/firmware/scmi-smc-multiagent.c | 2 +
xen/arch/arm/include/asm/domain.h | 6 +
xen/include/public/arch-arm.h | 4 +
xen/include/public/device_tree_defs.h | 1 +
xen/include/public/domctl.h | 11 +
11 files changed, 365 insertions(+), 11 deletions(-)
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 495598123133..54a93431641a 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -1205,6 +1205,9 @@ int xc_domain_getvnuma(xc_interface *xch,
int xc_domain_soft_reset(xc_interface *xch,
uint32_t domid);
+int xc_domain_get_sci_info(xc_interface *xch, uint32_t domid,
+ uint64_t *paddr, uint32_t *func_id);
+
#if defined(__i386__) || defined(__x86_64__)
/*
* PC BIOS standard E820 types and structure.
diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
index 2ddc3f4f426d..f4ffab2021cd 100644
--- a/tools/libs/ctrl/xc_domain.c
+++ b/tools/libs/ctrl/xc_domain.c
@@ -2229,6 +2229,24 @@ out:
return ret;
}
+
+int xc_domain_get_sci_info(xc_interface *xch, uint32_t domid,
+uint64_t *paddr, uint32_t *func_id)
+{
+struct xen_domctl domctl = {};
+
+memset(&domctl, 0, sizeof(domctl));
+domctl.cmd = XEN_DOMCTL_get_sci_info;
+domctl.domain = domid;
+
+if ( do_domctl(xch, &domctl) != 0 )
+return 1;
+
+*paddr = domctl.u.sci_info.paddr;
+*func_id = domctl.u.sci_info.func_id;
+return 0;
+}
+
/*
* Local variables:
* mode: C
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index cdf5edb299af..cc54abc1ea79 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -9,6 +9,7 @@
#include
#include
#include
+#include
/*
* There is no clear requirements for the total size of Virtio MMIO region.
@@ -640,9 +641,6 @@ static int make_optee_node(libxl__gc *gc, void *fdt)
int res;
LOG(DEBUG, "Creating OP-TEE node in dtb");
-res = fdt_begin_node(fdt, "firmware");
-if (res) return res;
-
res = fdt_begin_node(fdt, "optee");
if (res) return res;
@@ -655,9 +653,6 @@ static int make_optee_node(libxl__gc *gc, void *fdt)
res = fdt_end_node(fdt);
if (res) return res;
-res = fdt_end_node(fdt);
-if (res) retu
Re: [PATCH v2 6/7] xen/arm: rcar4: add simple optimization to avoid ATU reprogramming
On 11.03.25 12:24, Mykyta Poturai wrote:
From: Volodymyr Babchuk
There are high chances that there will be a number of a consecutive
accesses to configuration space of one device. To speed things up,
we can program ATU only during first access.
This is mostly beneficial taking into account the previous patch that
adds 1ms delay after ATU reprogramming.
Signed-off-by: Volodymyr Babchuk
Signed-off-by: Mykyta Poturai
---
v1->v2:
* rebased
---
xen/arch/arm/pci/pci-designware.c | 8
1 file changed, 8 insertions(+)
diff --git a/xen/arch/arm/pci/pci-designware.c
b/xen/arch/arm/pci/pci-designware.c
index def2c12d63..cec52cf81a 100644
--- a/xen/arch/arm/pci/pci-designware.c
+++ b/xen/arch/arm/pci/pci-designware.c
@@ -272,6 +272,14 @@ static void dw_pcie_prog_outbound_atu(struct
pci_host_bridge *pci, int index,
int type, uint64_t cpu_addr,
uint64_t pci_addr, uint64_t size)
{
First, using (hiding) static var inside the function deserve big, fat comment
on why and what.
+static uint64_t prev_addr = ~0;
Second, from an experience, static vars in functions is source of potential
problems,
it's kinda land mines in code. For example, lets assume there are two
pci_host_bridge
instances in Xen and they are accessed concurrently.
Best to get rid of static var here - may be move in pci_host_bridge, like
cached_pci_addr? not sure
+
+/* Simple optimization to not-program ATU for every transaction */
+if (prev_addr == pci_addr)
+return;
+
+prev_addr = pci_addr;
+
__dw_pcie_prog_outbound_atu(pci, 0, index, type,
cpu_addr, pci_addr, size);
}
BR,
-grygorii
Re: [PATCH v2 5/7] xen/arm: rcar4: add delay after programming ATU
On 11.03.25 12:24, Mykyta Poturai wrote: From: Volodymyr Babchuk For some reason, we need a delay before accessing ATU region after we programmed it. Otherwise, we'll get erroneous TLP. There is a code below, which should do this in proper way, by polling CTRL2 register, but according to documentation, hardware does not change this ATU_ENABLE bit at all. Signed-off-by: Volodymyr Babchuk Signed-off-by: Mykyta Poturai --- v1->v2: * rebased --- xen/arch/arm/pci/pci-designware.c | 5 + 1 file changed, 5 insertions(+) diff --git a/xen/arch/arm/pci/pci-designware.c b/xen/arch/arm/pci/pci-designware.c index 6ab03cf9b0..def2c12d63 100644 --- a/xen/arch/arm/pci/pci-designware.c +++ b/xen/arch/arm/pci/pci-designware.c @@ -194,6 +194,11 @@ static void dw_pcie_prog_outbound_atu_unroll(struct pci_host_bridge *pci, dw_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL2, PCIE_ATU_ENABLE); +/* + * HACK: We need to delay there, because the next code does not + * work as expected on S4 + */ +mdelay(1); It seems like a HACK to WA some platform issue, but in its current form it will affect all DW PCI compatible platforms. So, it is required a proper solution for the affected platform to be found, or some sort of DW PCI "quirk"s processing code be introduced. I'd recommend to drop this patch for now from this series. /* * Make sure ATU enable takes effect before any subsequent config * and I/O accesses. BR, -grygorii
