Re: [PATCH v3 6/7] xen/arm: introduce xen-evtchn dom0less property
Hi Julien,
> On 1 Sep 2022, at 7:07 pm, Julien Grall 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
>> ---
>> 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 >;
>> +};
>> +
>
> 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 0x4b00 0xff>;
>> };
>> +
>> +/* one sub-node per local event channel */
>> +ec2: evtchn@2 {
>> +compatible = "xen,evtchn-v1";
>> +/* local-evtchn link-to-foreign-evtchn */
>> +xen,evtchn = <0xa >;
>> +};
>> +
>> +ec3: evtchn@3 {
>> +compatible = "xen,evtchn-v1";
>> +xen,evtchn = <0xb >;
>> +};
>> +
>> +ec4: evtchn@4 {
>> +compatible = "xen,evtchn-v1";
>> +xen,evtchn = <0xc >;
>> +};
>> };
>>domU2 {
>> @@ -296,6 +346,18 @@ chosen {
>> compatible = "multiboot,ramdisk", "multiboot,module";
>> reg = <0x0 0x4d00 0xff>;
>> };
>> +
>> +/* one sub-node per local event channel */
>> +ec5: evtchn@5 {
>> +compatible = "xen,evtchn-v1";
>> +/* local-evtchn link-to-foreign-evtchn */
>> +xen,evtchn = <0xb >;
>> +};
>> +
>> +ec6: evtchn@6 {
>> +compatible = "xen,evtchn-v1";
>> +xen,evtchn = <0xd >;
>> +};
>> };
>> };
>> 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
>> #include
>> +#define STATIC_EVTCHN_NODE_SIZE_CELLS 2
>> +
>> static unsigned int __initdata
Re: [PATCH v3 6/7] xen/arm: introduce xen-evtchn dom0less property
Hi Stefano,
On 02/09/2022 03:20, Stefano Stabellini wrote:
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.
I would move static_evtchn_created just above (or below) "bool
is_protected". It would still re-use the padding and it would be
closer to another more similar field of the struct.
The only other option that I can think of would be to use port_is_valid,
instead of static_evtchn_created, to check that the port has already
been allocated, but we wouldn't be able to tell if it is a static evtchn
or simply unavailable for other reasons
You don't need to know the event channel was statically allocated. If
you have access to the event channel, then you can easily find out what
is the remote port.
and it would require more device
tree parsing.
The parsing is indeed the big cons. Hence, why I hadn't suggest it.
Cheers,
--
Julien Grall
Re: [PATCH v3 6/7] xen/arm: introduce xen-evtchn dom0less property
On Thu, 1 Sep 2022, Julien Grall wrote:
> 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
> > ---
> > 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.
>
> > +
> > +/* one sub-node per local event channel */
> > +ec1: evtchn@1 {
> > + compatible = "xen,evtchn-v1";
> > + /* local-evtchn link-to-foreign-evtchn */
> > + xen,evtchn = <0xa >;
> > +};
> > +
>
> 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.
>
> > domU1 {
> > compatible = "xen,domain";
> > #address-cells = <0x2>;
> > @@ -277,6 +310,23 @@ chosen {
> > compatible = "multiboot,ramdisk", "multiboot,module";
> > reg = <0x0 0x4b00 0xff>;
> > };
> > +
> > +/* one sub-node per local event channel */
> > +ec2: evtchn@2 {
> > +compatible = "xen,evtchn-v1";
> > +/* local-evtchn link-to-foreign-evtchn */
> > +xen,evtchn = <0xa >;
> > +};
> > +
> > +ec3: evtchn@3 {
> > +compatible = "xen,evtchn-v1";
> > +xen,evtchn = <0xb >;
> > +};
> > +
> > +ec4: evtchn@4 {
> > +compatible = "xen,evtchn-v1";
> > +xen,evtchn = <0xc >;
> > +};
> > };
> > domU2 {
> > @@ -296,6 +346,18 @@ chosen {
> > compatible = "multiboot,ramdisk", "multiboot,module";
> > reg = <0x0 0x4d00 0xff>;
> > };
> > +
> > +/* one sub-node per local event channel */
> > +ec5: evtchn@5 {
> > +compatible = "xen,evtchn-v1";
> > +/* local-evtchn link-to-foreign-evtchn */
> > +xen,evtchn = <0xb >;
> > +};
> > +
> > +ec6: evtchn@6 {
> > +compatible = "xen,evtchn-v1";
> > +xen,evtchn = <0xd >;
> > +};
> > };
> > };
> > 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
> > #include
Re: [PATCH v3 6/7] xen/arm: introduce xen-evtchn dom0less property
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
---
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.
+
+/* one sub-node per local event channel */
+ec1: evtchn@1 {
+ compatible = "xen,evtchn-v1";
+ /* local-evtchn link-to-foreign-evtchn */
+ xen,evtchn = <0xa >;
+};
+
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.
domU1 {
compatible = "xen,domain";
#address-cells = <0x2>;
@@ -277,6 +310,23 @@ chosen {
compatible = "multiboot,ramdisk", "multiboot,module";
reg = <0x0 0x4b00 0xff>;
};
+
+/* one sub-node per local event channel */
+ec2: evtchn@2 {
+compatible = "xen,evtchn-v1";
+/* local-evtchn link-to-foreign-evtchn */
+xen,evtchn = <0xa >;
+};
+
+ec3: evtchn@3 {
+compatible = "xen,evtchn-v1";
+xen,evtchn = <0xb >;
+};
+
+ec4: evtchn@4 {
+compatible = "xen,evtchn-v1";
+xen,evtchn = <0xc >;
+};
};
domU2 {
@@ -296,6 +346,18 @@ chosen {
compatible = "multiboot,ramdisk", "multiboot,module";
reg = <0x0 0x4d00 0xff>;
};
+
+/* one sub-node per local event channel */
+ec5: evtchn@5 {
+compatible = "xen,evtchn-v1";
+/* local-evtchn link-to-foreign-evtchn */
+xen,evtchn = <0xb >;
+};
+
+ec6: evtchn@6 {
+compatible = "xen,evtchn-v1";
+xen,evtchn = <0xd >;
+};
};
};
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
#include
+#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 =
[PATCH v3 6/7] xen/arm: introduce xen-evtchn dom0less property
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
---
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 = "...";
+
+};
+
+/* one sub-node per local event channel */
+ec1: evtchn@1 {
+ compatible = "xen,evtchn-v1";
+ /* local-evtchn link-to-foreign-evtchn */
+ xen,evtchn = <0xa >;
+};
+
domU1 {
compatible = "xen,domain";
#address-cells = <0x2>;
@@ -277,6 +310,23 @@ chosen {
compatible = "multiboot,ramdisk", "multiboot,module";
reg = <0x0 0x4b00 0xff>;
};
+
+/* one sub-node per local event channel */
+ec2: evtchn@2 {
+compatible = "xen,evtchn-v1";
+/* local-evtchn link-to-foreign-evtchn */
+xen,evtchn = <0xa >;
+};
+
+ec3: evtchn@3 {
+compatible = "xen,evtchn-v1";
+xen,evtchn = <0xb >;
+};
+
+ec4: evtchn@4 {
+compatible = "xen,evtchn-v1";
+xen,evtchn = <0xc >;
+};
};
domU2 {
@@ -296,6 +346,18 @@ chosen {
compatible = "multiboot,ramdisk", "multiboot,module";
reg = <0x0 0x4d00 0xff>;
};
+
+/* one sub-node per local event channel */
+ec5: evtchn@5 {
+compatible = "xen,evtchn-v1";
+/* local-evtchn link-to-foreign-evtchn */
+xen,evtchn = <0xb >;
+};
+
+ec6: evtchn@6 {
+compatible = "xen,evtchn-v1";
+xen,evtchn = <0xd >;
+};
};
};
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
#include
+#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", );
+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, );
+*phandle = dt_next_cell(1, );
+
+return 0;
+}
+
+static int __init
