Hi Julien,
> On 1 Sep 2022, at 7:07 pm, Julien Grall <jul...@xen.org> wrote:
>
> Hi Rahul,
>
> On 01/09/2022 10:13, Rahul Singh wrote:
>> Introduce a new sub-node under /chosen node to establish static event
>> channel communication between domains on dom0less systems.
>> An event channel will be created beforehand to allow the domains to
>> send notifications to each other.
>> Signed-off-by: Rahul Singh <rahul.si...@arm.com>
>> ---
>> Changes in v3:
>> - use device-tree used_by to find the domain id of the evtchn node.
>> - add new static_evtchn_create variable in struct dt_device_node to
>> hold the information if evtchn is already created.
>> - fix minor comments
>> Changes in v2:
>> - no change
>> ---
>> docs/misc/arm/device-tree/booting.txt | 64 ++++++++++++-
>> xen/arch/arm/domain_build.c | 128 ++++++++++++++++++++++++++
>> xen/arch/arm/include/asm/setup.h | 1 +
>> xen/arch/arm/setup.c | 2 +
>> xen/include/xen/device_tree.h | 13 +++
>> 5 files changed, 207 insertions(+), 1 deletion(-)
>> diff --git a/docs/misc/arm/device-tree/booting.txt
>> b/docs/misc/arm/device-tree/booting.txt
>> index 98253414b8..edef98e6d1 100644
>> --- a/docs/misc/arm/device-tree/booting.txt
>> +++ b/docs/misc/arm/device-tree/booting.txt
>> @@ -212,7 +212,7 @@ with the following properties:
>> enable only selected interfaces.
>> Under the "xen,domain" compatible node, one or more sub-nodes are present
>> -for the DomU kernel and ramdisk.
>> +for the DomU kernel, ramdisk and static event channel.
>> The kernel sub-node has the following properties:
>> @@ -254,11 +254,44 @@ The ramdisk sub-node has the following properties:
>> property because it will be created by the UEFI stub on boot.
>> This option is needed only when UEFI boot is used.
>> +The static event channel sub-node has the following properties:
>> +
>> +- compatible
>> +
>> + "xen,evtchn"
>> +
>> +- xen,evtchn
>> +
>> + The property is tuples of two numbers
>> + (local-evtchn link-to-foreign-evtchn) where:
>> +
>> + local-evtchn is an integer value that will be used to allocate local
>> port
>> + for a domain to send and receive event notifications to/from the remote
>> + domain. Maximum supported value is 2^17 for FIFO ABI and 4096 for 2L
>> ABI.
>> + It is recommended to use low event channel IDs.
>> +
>> + link-to-foreign-evtchn is a single phandle to a remote evtchn to which
>> + local-evtchn will be connected.
>> Example
>> =======
>> chosen {
>> +
>> + module@0 {
>> + compatible = "multiboot,kernel", "multiboot,module";
>> + xen,uefi-binary = "...";
>> + bootargs = "...";
>> +
>> + };
>
> NIT: Describing this node in the example seems unnecessary.
Ack. I will remove this.
>
>> +
>> + /* one sub-node per local event channel */
>> + ec1: evtchn@1 {
>> + compatible = "xen,evtchn-v1";
>> + /* local-evtchn link-to-foreign-evtchn */
>> + xen,evtchn = <0xa &ec2>;
>> + };
>> +
>
> Here you provide an example for dom0. But the position where you describe the
> binding suggests this binding only exists for domUs.
>
> Either we duplicate the binding or we re-order the documentation to have
> common binding in a single place. My preference would be the latter.
>
Ack.
>> domU1 {
>> compatible = "xen,domain";
>> #address-cells = <0x2>;
>> @@ -277,6 +310,23 @@ chosen {
>> compatible = "multiboot,ramdisk", "multiboot,module";
>> reg = <0x0 0x4b000000 0xffffff>;
>> };
>> +
>> + /* one sub-node per local event channel */
>> + ec2: evtchn@2 {
>> + compatible = "xen,evtchn-v1";
>> + /* local-evtchn link-to-foreign-evtchn */
>> + xen,evtchn = <0xa &ec1>;
>> + };
>> +
>> + ec3: evtchn@3 {
>> + compatible = "xen,evtchn-v1";
>> + xen,evtchn = <0xb &ec5>;
>> + };
>> +
>> + ec4: evtchn@4 {
>> + compatible = "xen,evtchn-v1";
>> + xen,evtchn = <0xc &ec6>;
>> + };
>> };
>> domU2 {
>> @@ -296,6 +346,18 @@ chosen {
>> compatible = "multiboot,ramdisk", "multiboot,module";
>> reg = <0x0 0x4d000000 0xffffff>;
>> };
>> +
>> + /* one sub-node per local event channel */
>> + ec5: evtchn@5 {
>> + compatible = "xen,evtchn-v1";
>> + /* local-evtchn link-to-foreign-evtchn */
>> + xen,evtchn = <0xb &ec3>;
>> + };
>> +
>> + ec6: evtchn@6 {
>> + compatible = "xen,evtchn-v1";
>> + xen,evtchn = <0xd &ec4>;
>> + };
>> };
>> };
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 707e247f6a..4b24261825 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -33,6 +33,8 @@
>> #include <xen/grant_table.h>
>> #include <xen/serial.h>
>> +#define STATIC_EVTCHN_NODE_SIZE_CELLS 2
>> +
>> static unsigned int __initdata opt_dom0_max_vcpus;
>> integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
>> @@ -3052,6 +3054,131 @@ void __init evtchn_allocate(struct domain *d)
>> d->arch.hvm.params[HVM_PARAM_CALLBACK_IRQ] = val;
>> }
>> +static int __init get_evtchn_dt_property(const struct dt_device_node *np,
>> + uint32_t *port, uint32_t *phandle)
>> +{
>> + const __be32 *prop = NULL;
>> + uint32_t len;
>> +
>> + prop = dt_get_property(np, "xen,evtchn", &len);
>> + if ( !prop )
>> + {
>> + printk(XENLOG_ERR "xen,evtchn property should not be empty.\n");
>> + return -EINVAL;
>> + }
>> +
>> + if ( !len || len < dt_cells_to_size(STATIC_EVTCHN_NODE_SIZE_CELLS) )
>> + {
>> + printk(XENLOG_ERR "xen,evtchn property value is not valid.\n");
>> + return -EINVAL;
>> + }
>> +
>> + *port = dt_next_cell(1, &prop);
>> + *phandle = dt_next_cell(1, &prop);
>> +
>> + return 0;
>> +}
>> +
>> +static int __init alloc_domain_evtchn(struct dt_device_node *node)
>> +{
>> + int rc;
>> + uint32_t domU1_port, domU2_port, remote_phandle;
>> + struct dt_device_node *remote_node;
>> + struct evtchn_alloc_unbound alloc_unbound;
>> + struct evtchn_bind_interdomain bind_interdomain;
>> + struct domain *d1 = NULL, *d2 = NULL;
>> +
>> + if ( dt_device_static_evtchn_created(node) )
>
> I think this deserve a comment explain why the node would be created. I.e it
> would happen if the other side was created first. I will comment about
> dt_device_static_evtchn_created() futher down.
I will modify as below:
/*
* Event channel is already created while parsing the other side of
* evtchn node.
*/
if ( dt_device_static_evtchn_created(node) )
return 0;
>
>> + return 0;
>> +
>> + rc = get_evtchn_dt_property(node, &domU1_port, &remote_phandle);
>> + if ( rc )
>> + return rc;
>> +
>> + remote_node = dt_find_node_by_phandle(remote_phandle);
>> + if ( !remote_node )
>> + {
>> + printk(XENLOG_ERR
>> + "evtchn: could not find remote evtchn phandle\n");
>> + return -EINVAL;
>> + }
>> +
>> + rc = get_evtchn_dt_property(remote_node, &domU2_port, &remote_phandle);
>> + if ( rc )
>> + return rc;
>> +
>> + if ( node->phandle != remote_phandle )
>> + {
>> + printk(XENLOG_ERR "xen,evtchn property is not setup correctly.\n");
>> + return -EINVAL;
>> + }
>> +
>> + d1 = get_domain_by_id(dt_get_parent(node)->used_by);
>> + d2 = get_domain_by_id(dt_get_parent(remote_node)->used_by);
>
> I think dt_get_parent() could return NULL (i.e. for the root). So I think you
> want to check that at least remote_node() has a parent. For convenience, this
> check could be done in
Ack.
>
>> +
>> + if ( !d1 || !d2 )
>> + {
>> + printk(XENLOG_ERR "evtchn: could not find domains\n" );
>> + return -EINVAL;
>> + }
>> +
>> + alloc_unbound.dom = d1->domain_id;
>> + alloc_unbound.remote_dom = d2->domain_id;
>> +
>> + rc = evtchn_alloc_unbound(&alloc_unbound, domU1_port);
>> + if ( rc < 0 )
>> + {
>> + printk(XENLOG_ERR
>> + "evtchn_alloc_unbound() failure (Error %d) \n", rc);
>> + return rc;
>> + }
>> +
>> + bind_interdomain.remote_dom = d1->domain_id;
>> + bind_interdomain.remote_port = domU1_port;
>> +
>> + rc = evtchn_bind_interdomain(&bind_interdomain, d2, domU2_port);
>> + if ( rc < 0 )
>> + {
>> + printk(XENLOG_ERR
>> + "evtchn_bind_interdomain() failure (Error %d) \n", rc);
>> + return rc;
>> + }
>> +
>> + dt_device_set_static_evtchn_created(node);
>> + dt_device_set_static_evtchn_created(remote_node);
>> +
>> + return 0;
>> +}
>> +
>> +void __init process_static_evtchn_node(struct dt_device_node *node)
>
> This is missing a prototype. So I guess this wants to be static?
>
> That said, I think it would make more sense to fold
> process_static_evtchn_node() in alloc_domain_evtchn() or
> alloc_static-evtchn().
Ack.
>
>> +{
>> + if ( dt_device_is_compatible(node, "xen,evtchn-v1") )
>> + {
>> + if ( alloc_domain_evtchn(node) != 0 )
>> + panic("Could not set up domains evtchn\n");
>> + }
>> +}
>> +
>> +void __init alloc_static_evtchn(void)
>> +{
>> + struct dt_device_node *node, *evtchn_node;
>> + struct dt_device_node *chosen = dt_find_node_by_path("/chosen");
>> +
>> + BUG_ON(chosen == NULL);
>> +
>> + if ( hardware_domain )
>> + dt_device_set_used_by(chosen, hardware_domain->domain_id);
>> +
>> + dt_for_each_child_node(chosen, node)
>> + {
>> + if ( hardware_domain )
>> + process_static_evtchn_node(node);
>> +
>> + dt_for_each_child_node(node, evtchn_node)
>> + process_static_evtchn_node(evtchn_node);
>> + }
>> +}
>> +
>> static void __init find_gnttab_region(struct domain *d,
>> struct kernel_info *kinfo)
>> {
>> @@ -3364,6 +3491,7 @@ void __init create_domUs(void)
>> panic("Error creating domain %s\n", dt_node_name(node));
>> d->is_console = true;
>> + dt_device_set_used_by(node, d->domain_id);
>> if ( construct_domU(d, node) != 0 )
>> panic("Could not set up domain %s\n", dt_node_name(node));
>> diff --git a/xen/arch/arm/include/asm/setup.h
>> b/xen/arch/arm/include/asm/setup.h
>> index 5815ccf8c5..5ee28b270f 100644
>> --- a/xen/arch/arm/include/asm/setup.h
>> +++ b/xen/arch/arm/include/asm/setup.h
>> @@ -106,6 +106,7 @@ int acpi_make_efi_nodes(void *fdt, struct membank
>> tbl_add[]);
>> void create_domUs(void);
>> void create_dom0(void);
>> +void alloc_static_evtchn(void);
>> void discard_initial_modules(void);
>> void fw_unreserved_regions(paddr_t s, paddr_t e,
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index 6e0398f3f6..cf15d359d2 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -1077,6 +1077,8 @@ void __init start_xen(unsigned long boot_phys_offset,
>> if ( acpi_disabled )
>> create_domUs();
>> + alloc_static_evtchn();
>> +
>> /*
>> * This needs to be called **before** heap_init_late() so modules
>> * will be scrubbed (unless suppressed).
>> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
>> index 430a1ef445..5579c875d2 100644
>> --- a/xen/include/xen/device_tree.h
>> +++ b/xen/include/xen/device_tree.h
>> @@ -82,6 +82,7 @@ struct dt_device_node {
>> dt_phandle phandle;
>> char *full_name;
>> domid_t used_by; /* By default it's used by dom0 */
>> + bool_t static_evtchn_created;
>
> I can see why you want to add the boolean in dt_device_node. However, I
> dislike this approach because this feels an abuse of dt_device_node and the
> field is only used at boot.
>
> So this seems to be a bit of a waste to include it in the structure (even if
> we are re-using padding today).
>
> I don't have a solution that is has trivial as this approach. However, at
> minimum we should document this is a HACK and should be remove if we need
> space in the structure.
Ok. I will add the comment that this is HACK.
….
/* IOMMU specific fields */
bool is_protected;
+
+ /* HACK: Remove this if there is a need of space */
+ bool_t static_evtchn_created;
+
/*
* The main purpose of this list is to link the structure in the list
* of devices assigned to domain.
….
Regards,
Rahul
