Re: [PATCH v3 6/7] xen/arm: introduce xen-evtchn dom0less property

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Julien,

> On 1 Sep 2022, at 7:07 pm, Julien Grall <julien@xxxxxxx> 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.singh@xxxxxxx>
>> ---
>> 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