SCMI-Mediator add support for DomUs #214

oleksiimoisieiev · 2024-09-30T18:04:25Z

This feature is adding SCMI mediator support for DomU. Since toolstack is not using in current rpi5 setup - there is no posibility to test it on rpi5. But this approach was tested on R-Car Gen3 board which is using toolstack to start the Domains.
This pr was made to make SCMI-Mediator support generic.

Config parameters needed to enable scmi feature:

CONFIG_ARM_SCI=y                                                                
CONFIG_SCMI_SMC=y                                                               
CONFIG_HOST_DTB_EXPORT=y                                                        
CONFIG_ACCESS_CONTROLLER=y

DomU configuration:

arm_sci="scmi_smc"

DomU partial dtb:

/{
      firmware {
            scmi {
                  scmi_reset:protocol@16 {
                  };
                  scmi_pinctrl:protocol@19 {
                               mux1: mux1 {
                               };
                               mux2: mux2 {
                               };
                  };
      };
      passthrough {
         dev1 {
               resets = <&scmi_reset 1>;
               pinctrl-0 = <&mux1>;
         };
         dev2 {
               pinctrl-0 = <&mux2>;
         };
      };
};

TODO:

after review make a PR to xen-4.19-xt
Postponed:
add new syscall to flask.

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 <[email protected]> Reviewed-by: Volodymyr Babchuk <[email protected]>

lorc · 2024-09-30T20:29:15Z

xen/arch/arm/domain.c

@@ -698,6 +702,27 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
    return 0;
 }

+#ifdef CONFIG_HOST_DTB_EXPORT
+#define HYPFS_PROPERTY_MAX_SIZE 4096


I believe that DTB is much bigger than 4k. Also, naming is quite ambiguous. It just says "hypfs property". Which property exactly?

I believe, this is the leftover from your previous version. But in this version it makes no sense

Sorry. it's leftover. I've made HOST_DTB_MAX_SIZE config parameter

with default as 8192. but probably 4096 is enough because current dtb in rcar is 2150

lorc · 2024-09-30T20:31:43Z

xen/arch/arm/domain.c

@@ -698,6 +702,27 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
    return 0;
 }

+#ifdef CONFIG_HOST_DTB_EXPORT


Why it is living in the middle of domain.c? This file is for low-level domain state manipulation

Couldn't find a better place. What do you suggest?

bootfdt.c maybe?

lorc · 2024-09-30T20:33:28Z

xen/arch/arm/domain_build.c

@@ -1741,6 +1755,12 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
         */
        evtchn_allocate(d);

+#ifdef CONFIG_SCMI_SMC
+        res = mem_permit_access(kinfo->d, kinfo->d->arch.sci_channel.paddr,
+                0x1000);


Please no magic values

There is still constant 0x1000

lorc · 2024-09-30T20:34:12Z

xen/arch/arm/domain_build.c

+    if ( res )
+        return res;
+
+    return map_regions_p2mt(d, gaddr_to_gfn(addr), PFN_DOWN(len),


Should you remove iomem access permission if this call is failed?

I don't see where it was fixed.

Merged these changes to wrong commit. I'm sorry... now fixec

lorc · 2024-09-30T20:36:07Z

xen/include/xen/scmi_dt_maker.h

@@ -16,10 +16,13 @@ int __init scmi_dt_make_shmem_node(struct kernel_info *kinfo);
 int __init scmi_dt_create_node(struct kernel_info *kinfo);
 int __init scmi_dt_scan_node(struct kernel_info *kinfo, void *pfdt,
                             int nodeoff);
+int __init scmi_dt_set_phandle(struct kernel_info *kinfo,
+        const char *name);


formatting is not consistent with the previous lines.

lorc · 2024-09-30T20:50:24Z

tools/libs/light/libxl_arm.c

+    int r;
+
+    if (fdt_magic(fdt) != FDT_MAGIC) {
+         LOG(ERROR, "Partial FDT is not a valid Flat Device Tree");


This is not the partial device tree

lorc · 2024-09-30T20:50:28Z

tools/libs/light/libxl_arm.c

+
+     r = fdt_check_header(fdt);
+     if (r) {
+         LOG(ERROR, "Failed to check the partial FDT (%d)", r);


This is not the partial device tree

lorc · 2024-09-30T20:51:24Z

tools/libs/light/libxl_arm.c

+        goto out;
+    }
+
+    nodeoff = get_path_nodeoff("/scmi", pfdt);


Should it be /firmware/scmi ? I believe we discussed this at the daily call

yup. sorry. it's leftover

lorc · 2024-09-30T20:52:06Z

tools/libs/light/libxl_arm.c

+    }
+
+    nodeoff = get_path_nodeoff("/scmi", pfdt);
+    if ( nodeoff <= 0 ) {


coding style

lorc · 2024-09-30T20:54:58Z

xen/arch/arm/domctl.c

+    sci_info->paddr = d->arch.sci_channel.paddr;
+    sci_info->func_id = d->arch.sci_channel.guest_func_id;
+#endif /* CONFIG_ARM_SCI */
+    return 0;


I believe you need to return -ENODEV if CONFIG_ARM_SCI is not enabled or SCI is not enabled for the domain

What about the second part of my comment? What if SCI is not enabled for that particular domain? It will have no valid paddr and func id in that case

oleksiimoisieiev · 2024-10-01T09:34:04Z

xen/arch/arm/domctl.c

+    return 0;
+}
+
+


remove extra enter

oleksiimoisieiev · 2024-10-01T09:34:23Z

xen/arch/arm/domctl.c

        return subarch_do_domctl(domctl, d, u_domctl);
    }
+    }


remove leftover

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 <[email protected]> Reviewed-by: Volodymyr Babchuk <[email protected]>

This enumeration sets SCI type for the domain. Currently there is two possible options: either 'none' or 'scmi_smc'. 'none' is the default value and it disables SCI support at all. 'scmi_smc' enables access to the Firmware from the domains using SCMI protocol and SMC as transport. Signed-off-by: Oleksii Moisieiev <[email protected]> Reviewed-by: Volodymyr Babchuk <[email protected]>

oleksiimoisieiev · 2024-10-01T17:42:13Z

@lorc @GrygiriiS updated. ready to stage 2

lorc · 2024-10-02T15:40:27Z

I see no new changes

oleksiimoisieiev · 2024-10-02T15:50:19Z

I'm really sorry... pushed to another branch... Updated @lorc

lorc

Looks like you are missing some fixes, although you said that you fixed it

lorc · 2024-10-02T20:14:42Z

xen/arch/arm/domain_build.c

+    if ( res )
+        return res;
+
+    return map_regions_p2mt(d, gaddr_to_gfn(addr), PFN_DOWN(len),


I don't see where it was fixed.

lorc · 2024-10-02T20:14:55Z

xen/arch/arm/domain_build.c

@@ -1741,6 +1755,12 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
         */
        evtchn_allocate(d);

+#ifdef CONFIG_SCMI_SMC
+        res = mem_permit_access(kinfo->d, kinfo->d->arch.sci_channel.paddr,
+                0x1000);


There is still constant 0x1000

Mapping for the hardware domain should be done during domain construciton as it is done for other domains. Signed-off-by: Oleksii Moisieiev <[email protected]> Reviewed-by: Volodymyr Babchuk <[email protected]>

Integration of the SCMI-Mediator feature to the Domain-0 construction process. It includes shared memory node creation and mapping with phandle update on the scmi node. When creating hardware domain there is a need to set correct phandle to the "shmem" property on the scmi node. Shared memory node is generated while scmi node is copied as is from Xen device-tree. Correct shmem phandle is set during domain device-tree processing. Signed-off-by: Oleksii Moisieiev <[email protected]> Reviewed-by: Volodymyr Babchuk <[email protected]>

Assgin devices to the access-controller during Domain-0 build. This registers devices in the SCMI server for the Domain-0. Signed-off-by: Oleksii Moisieiev <[email protected]> Reviewed-by: Volodymyr Babchuk <[email protected]>

Remove space before function definition. Signed-off-by: Oleksii Moisieiev <[email protected]> Reviewed-by: Volodymyr Babchuk <[email protected]>

oleksiimoisieiev · 2024-10-03T07:12:06Z

Fixed all. Merged changes to the wrong commit. I think once Review is finished. I will make a pr to 4.19-xt0.1 with already reviewed code

lorc

With the last two comments resolved:

Reviewed-by: Volodymyr Babchuk <[email protected]>

lorc · 2024-10-03T12:36:50Z

xen/arch/arm/domctl.c

+    sci_info->paddr = d->arch.sci_channel.paddr;
+    sci_info->func_id = d->arch.sci_channel.guest_func_id;
+#endif /* CONFIG_ARM_SCI */
+    return 0;


What about the second part of my comment? What if SCI is not enabled for that particular domain? It will have no valid paddr and func id in that case

lorc · 2024-10-03T12:44:39Z

tools/libs/light/libxl_arm.c

+
+        phandle = fdt_get_phandle(pfdt, node_next);
+
+        rc = fdt_get_path(pfdt, node_next, full_name, 128);


At least use sizeof(full_name) here. No need for magic numbers.

oleksiimoisieiev · 2024-10-04T19:46:32Z

In reply to comment: #214 (comment)
I'm returning -ENODEV in this case.

lorc · 2024-10-04T19:52:23Z

I'm returning -ENODEV in this case.

Where?

Imagine you have purely virtual DomU without access to hardware. It does not need access to SCMI and SCMI should be disabled for this domain. In this case this domctl should return EINVAL or ENODEV or some other error.

oleksiimoisieiev · 2024-10-04T19:57:10Z

xen/arch/arm/domctl.c

+    sci_info->func_id = d->arch.sci_channel.guest_func_id;
+    return 0;
+#else
+    return -ENODEV;


This is some weird github twist... probably.

Integration of the SCMI mediator with xen libs: - add hypercalls to aquire SCI channel and set device permissions for DomUs; - add SCMI_SMC nodes to DomUs device-tree based on partial device-tree; - SCI requests redirection from DomUs to Firmware. Signed-off-by: Oleksii Moisieiev <[email protected]> Reviewed-by: Volodymyr Babchuk <[email protected]>

oleksiimoisieiev · 2024-10-04T20:03:03Z

#214 (comment)

fixed.
Adding rb

lorc reviewed Sep 30, 2024

View reviewed changes

oleksiimoisieiev commented Oct 1, 2024

View reviewed changes

oleksiimoisieiev added 2 commits October 1, 2024 20:36

oleksiimoisieiev force-pushed the scmi_rcar_rpi5 branch from 2eb8c90 to 418a1bc Compare October 2, 2024 15:49

lorc reviewed Oct 2, 2024

View reviewed changes

oleksiimoisieiev added 4 commits October 3, 2024 09:51

xen: arch: scmi_smc: remove memory mapping for hardware domain

8b4c6d1

Mapping for the hardware domain should be done during domain construciton as it is done for other domains. Signed-off-by: Oleksii Moisieiev <[email protected]> Reviewed-by: Volodymyr Babchuk <[email protected]>

xen: scmi_dt_maker: Fix code style minor issue

d627fd4

Remove space before function definition. Signed-off-by: Oleksii Moisieiev <[email protected]> Reviewed-by: Volodymyr Babchuk <[email protected]>

oleksiimoisieiev force-pushed the scmi_rcar_rpi5 branch from 418a1bc to 2e8d596 Compare October 3, 2024 06:52

lorc reviewed Oct 3, 2024

View reviewed changes

oleksiimoisieiev commented Oct 4, 2024

View reviewed changes

oleksiimoisieiev force-pushed the scmi_rcar_rpi5 branch from 2e8d596 to 7226c43 Compare October 4, 2024 20:02

oleksiimoisieiev force-pushed the scmi_rcar_rpi5 branch from 7226c43 to f5007bb Compare October 4, 2024 20:03


		phandle = fdt_get_phandle(pfdt, node_next);

		rc = fdt_get_path(pfdt, node_next, full_name, 128);

SCMI-Mediator add support for DomUs #214

Are you sure you want to change the base?

SCMI-Mediator add support for DomUs #214

Conversation

oleksiimoisieiev commented Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleksiimoisieiev commented Oct 1, 2024

lorc commented Oct 2, 2024

oleksiimoisieiev commented Oct 2, 2024

lorc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleksiimoisieiev commented Oct 3, 2024

lorc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleksiimoisieiev commented Oct 4, 2024

lorc commented Oct 4, 2024

Choose a reason for hiding this comment

oleksiimoisieiev commented Oct 4, 2024

oleksiimoisieiev commented Sep 30, 2024 •

edited

Loading