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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Julien,

> On 23 Aug 2022, at 10:32 am, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Rahul,
> 
> On 19/08/2022 11:02, 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 v2:
>>  - no change
>> ---
>> ---
>>  docs/misc/arm/device-tree/booting.txt |  63 +++++++++++-
>>  xen/arch/arm/domain_build.c           | 136 ++++++++++++++++++++++++++
>>  xen/arch/arm/include/asm/domain.h     |   1 +
>>  xen/arch/arm/include/asm/setup.h      |   1 +
>>  xen/arch/arm/setup.c                  |   2 +
>>  5 files changed, 202 insertions(+), 1 deletion(-)
>> diff --git a/docs/misc/arm/device-tree/booting.txt 
>> b/docs/misc/arm/device-tree/booting.txt
>> index 98253414b8..ec7dbcaf8f 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,43 @@ 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 ID.
> 
> I think you are either missing a 'a' or 'ID' should be 'IDs'

Ack. 
> 
>> +
>> +    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 = "...";
>> +
>> +        /* one sub-node per local event channel */
>> +        ec1: evtchn@1 {
>> +            compatible = "xen,evtchn-v1";
>> +            /* local-evtchn link-to-foreign-evtchn */
>> +            xen,evtchn = <0xa &ec2>;
>> +        };
> 
> AFAIU, this is meant to describe the static event channels for dom0. I can't 
> find the documentation for it. Do they always need to be a subnode the node 
> "multiboot,kernel"?
> 
> The reason I am asking is it feels strange to define them below that subnode 
> when for domUs, both nodes have the same parent. So I think it would make 
> more sense to define them in chosen.

Ok. I will move the dom0 evtchn node under chosen done.
> 
>> +    };
>> +
>>      domU1 {
>>          compatible = "xen,domain";
>>          #address-cells = <0x2>;
>> @@ -277,6 +309,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 +345,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 11a8c6b8b5..5101bca979 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -3052,6 +3052,141 @@ void __init evtchn_allocate(struct domain *d)
>>      d->arch.hvm.params[HVM_PARAM_CALLBACK_IRQ] = val;
>>  }
>>  +static const void *__init get_evtchn_dt_property(
>> +        const struct dt_device_node *np)
>> +{
>> +    const void *prop = NULL;
>> +    uint32_t len;
>> +
>> +    prop = dt_get_property(np, "xen,evtchn", &len);
>> +    if ( !prop )
>> +        return NULL;
>> +
>> +    if ( !len )
>> +    {
>> +        printk(XENLOG_ERR "xen,evtchn property cannot be empty.\n")
> 
> Looking at the callers, they all assume that there is enough cells in the 
> property. So I think you should check the size as well.

Ack, 
> 
>> +        return ERR_PTR(-EINVAL);
>> +    }
>> +
>> +    return prop;
>> +}
>> +
>> +static int __init allocate_domain_evtchn(const struct dt_device_node *node)
>> +{
>> +    const void *prop = NULL;
>> +    const __be32 *cell;
>> +    uint32_t domU1_port, domU2_port, remote_phandle;
>> +    const struct dt_device_node *evtchn_node, *remote_node;
>> +    struct evtchn_alloc_unbound alloc_unbound;
>> +    struct evtchn_bind_interdomain bind_interdomain;
>> +    int rc;
>> +
>> +    dt_for_each_child_node(node, evtchn_node)
>> +    {
>> +        struct domain *d, *d1 = NULL, *d2 = NULL;
>> +
>> +        if ( !dt_device_is_compatible(evtchn_node, "xen,evtchn-v1") )
>> +            continue;
>> +
>> +        prop = get_evtchn_dt_property(evtchn_node);
>> +        /* If the property is not found, return without errors */
> 
> From the binding description, the property is not optional. So do we want to 
> ignore the error? If you treat it as an error, then ...

Ok. I will treat it as en error.
> 
>> +        if ( !prop || IS_ERR(prop) )
>> +            return IS_ERR(prop) ? PTR_ERR(prop) : 0;
> 
> ... you could return ERR_PTR(-ENOMEM) instead of NULL and then simplify this 
> code with:

Ack. 
> 
>> +
>> +        cell = (const __be32 *)prop;
> 
> prop is a void pointer. So the cast is unnecessary.

Ack. 
> 
>> +        domU1_port = dt_next_cell(1, &cell);
>> +        remote_phandle = dt_next_cell(1, &cell);
> The code is also duplicated below for the remote port. I think it would be 
> better if this is part of your helper get_evtchn_dt_property().

Ack .
> 
>> +
>> +        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;
>> +        }
>> +
>> +        prop = get_evtchn_dt_property(remote_node);
>> +        /* If the property is not found, return without errors */
>> +        if ( !prop || IS_ERR(prop) )
>> +            return IS_ERR(prop) ? PTR_ERR(prop) : 0;
>> +
>> +        cell = (const __be32 *)prop;
>> +        domU2_port = dt_next_cell(1, &cell);
>> +        remote_phandle = dt_next_cell(1, &cell);
>> +
>> +        if ( evtchn_node->phandle != remote_phandle )
>> +        {
>> +            printk(XENLOG_ERR "xen,evtchn property is not setup 
>> correctly.\n");
>> +            return -EINVAL;
>> +        }
>> +
>> +        for_each_domain ( d )
>> +        {
>> +            if ( d->arch.node == node )
>> +            {
>> +                d1 = d;
>> +                continue;
>> +            }
>> +            if ( d->arch.node == dt_get_parent(remote_node) )
>> +                d2 = d;
>> +        }
> 
> The loop could be avoided if you stash the domid in the field 'used_by' of 
> the device-tree node when the domain is created.

Ok. I will used the “used_by” to store the domid.
> 
>> +
>> +        if ( !d1 && dt_device_is_compatible(node, "multiboot,kernel") )
>> +            d1 = hardware_domain;
>> +
>> +        if ( !d2 && dt_device_is_compatible(dt_get_parent(remote_node),
>> +                                            "multiboot,kernel") )
>> +            d2 = hardware_domain;
> 
> Any particular reason to handle the hardware domain differently?

As I have not set the "d->arch.node” for hwdom that why there is differently 
handling for hwdom.
I will try to use the “used_by” and will avoid this handling.

> 
>> +
>> +        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 && rc != -EBUSY )
> 
> Please explain in a comment why you want to handle -EBUSY differently.

-EBUSY is used to check if evtchn is not already created while we scanning
the evtchn dt nodes.

> 
>> +        {
>> +            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 && rc != -EBUSY )
> 
> AFAIU, EBUSY only tells you the port is been used. It doesn't tell you the 
> link is the same. So I think you want to also confirm that to avoid to 
> continuing with the wrong setup.

Ack. 
> 
>> +        {
>> +            printk(XENLOG_ERR
>> +                   "evtchn_bind_interdomain() failure (Error %d) \n", rc);
>> +            return rc;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +void __init allocate_static_evtchn(void)
>> +{
>> +    struct dt_device_node *node;
> 
> AFAICT, all the users below can deal with constisfied node. So I think you 
> want to add 'const' here.

Ack.
> 
>> +    const struct dt_device_node *chosen = dt_find_node_by_path("/chosen");
>> +
>> +    BUG_ON(chosen == NULL);
>> +    dt_for_each_child_node(chosen, node)
>> +    {
>> +        if ( dt_device_is_compatible(node, "xen,domain") ||
>> +             dt_device_is_compatible(node, "multiboot,kernel") )
>> +        {
>> +            if ( allocate_domain_evtchn(node) != 0 )
>> +                panic("Could not set up domains evtchn\n");
>> +        }
>> +    }
>> +}
>> +
>>  static void __init find_gnttab_region(struct domain *d,
>>                                        struct kernel_info *kinfo)
>>  {
>> @@ -3358,6 +3493,7 @@ void __init create_domUs(void)
>>              panic("Error creating domain %s\n", dt_node_name(node));
>>            d->is_console = true;
>> +        d->arch.node = node;
> 
> If you follow my suggestion above, this should not be necessary. However, if 
> this is still needed for some reason, then I think we should also set 
> d->arch.node for the Hardware Domain and ...

Ack. 
> 
>>            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/domain.h 
>> b/xen/arch/arm/include/asm/domain.h
>> index cd9ce19b4b..51192b28ee 100644
>> --- a/xen/arch/arm/include/asm/domain.h
>> +++ b/xen/arch/arm/include/asm/domain.h
>> @@ -105,6 +105,7 @@ struct arch_domain
>>  #endif
>>        bool directmap;
>> +    struct dt_device_node *node;
> 
> ... this should be const as the node shouldn't be modifiable.

Ack. 

Regards,
Rahul