| Message ID | 2fb69ff7cf9a36dd1294da4f9f4b968ff7076d42.1660902588.git.rahul.singh@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/evtchn: implement static event channel signaling | expand |
Hi, On 19/08/2022 11:02, Rahul Singh wrote: > Introduce a new "xen,enhanced" dom0less property value "evtchn" to > enable/disable event-channel interfaces for dom0less guests. The documentation in docs/misc/arm/device-tree/booting.txt is missing. Also, you probably wants to update docs/feature/dom0less.pandoc because the section "PV drivers" suggests that if the property "xen,enhanced" is specified, then we would end up to allocate information for PV drivers. AFAIU, this is not the case when "evtchn" is specified. > > The configurable option is for domUs only. For dom0 we always set the > corresponding property in the Xen code to true. > > Signed-off-by: Rahul Singh <rahul.singh@arm.com> > --- > Changes in v2: > - no change > --- > --- > xen/arch/arm/domain_build.c | 149 ++++++++++++++++-------------- > xen/arch/arm/include/asm/kernel.h | 3 + > 2 files changed, 82 insertions(+), 70 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 5101bca979..bd8b8475b7 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -1396,85 +1396,92 @@ static int __init make_hypervisor_node(struct domain *d, > if ( res ) > return res; > The diff below is quite difficult to read. I have applied to have a look. You seem to have simply indented the code and now some of the lines are over the 80 characters mark. Ideally, I would like to avoid large 'if'. So I would suggest to either re-ordering the code or split in multiple functions. However, reading the binding of "xen,xen", the property "reg" and "interrupts" are not optional. I also don't think can make them optional because some OSes may not boot if it can't find one of the property. In any case, at minimum you should explain why this is fine to make them optional. [...] > - /* > - * Interrupt event channel upcall: > - * - Active-low level-sensitive > - * - All CPUs > - * TODO: Handle properly the cpumask; > - */ > - set_interrupt(intr, d->arch.evtchn_irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW); > - res = fdt_property_interrupts(kinfo, &intr, 1); > - if ( res ) > - return res; > + if ( kinfo->dom0less_evtchn ) So I understand why you want to make the first part optional. But this is not clear why this one become conditional to "dom0less_evtchn". Do you have any plan to only present the node "xen,xen" where neither event channels nor PV interfaces would be used? > + { > + BUG_ON(d->arch.evtchn_irq == 0); > + > + /* > + * Interrupt event channel upcall: > + * - Active-low level-sensitive > + * - All CPUs > + * TODO: Handle properly the cpumask; > + */ > + set_interrupt(intr, d->arch.evtchn_irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW); > + res = fdt_property_interrupts(kinfo, &intr, 1); > + if ( res ) > + return res; > + } > > res = fdt_end_node(fdt); > > @@ -2891,7 +2898,7 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo) > goto err; > } > > - if ( kinfo->dom0less_enhanced ) > + if ( kinfo->dom0less_enhanced || kinfo->dom0less_evtchn ) I think the first part of the if can be removed because you can't do without event channel. > { > ret = make_hypervisor_node(d, kinfo, addrcells, sizecells); > if ( ret ) > @@ -3343,11 +3350,11 @@ static int __init construct_domU(struct domain *d, > rc == -ENODATA || > (rc == 0 && !strcmp(dom0less_enhanced, "enabled")) ) > { > - if ( hardware_domain ) > - kinfo.dom0less_enhanced = true; > - else > - panic("Tried to use xen,enhanced without dom0\n"); > + kinfo.dom0less_enhanced = true; > + kinfo.dom0less_evtchn = true; > } > + else if ( rc == 0 && !strcmp(dom0less_enhanced, "evtchn") ) > + kinfo.dom0less_evtchn = true; > > if ( vcpu_create(d, 0) == NULL ) > return -ENOMEM; > @@ -3526,6 +3533,8 @@ static int __init construct_dom0(struct domain *d) > > kinfo.unassigned_mem = dom0_mem; > kinfo.d = d; > + kinfo.dom0less_enhanced = true; > + kinfo.dom0less_evtchn = true; > > rc = kernel_probe(&kinfo, NULL); > if ( rc < 0 ) > diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h > index c4dc039b54..7cff19b997 100644 > --- a/xen/arch/arm/include/asm/kernel.h > +++ b/xen/arch/arm/include/asm/kernel.h > @@ -39,6 +39,9 @@ struct kernel_info { > /* Enable PV drivers */ > bool dom0less_enhanced; > > + /* Enable event-channel interface */ > + bool dom0less_evtchn; So technically, the event channel interface is still exposed even if this is false. This is because we are still allocate the PPI and set the number of events to a non-zero value. IMHO, if dom0less_evtchn is false, then we should properly disable the event channels interface not just hide it. Cheers,
Hi Julien, > On 23 Aug 2022, at 11:05 am, Julien Grall <julien@xen.org> wrote: > > Hi, > > On 19/08/2022 11:02, Rahul Singh wrote: >> Introduce a new "xen,enhanced" dom0less property value "evtchn" to >> enable/disable event-channel interfaces for dom0less guests. > > The documentation in docs/misc/arm/device-tree/booting.txt is missing. Also, you probably wants to update docs/feature/dom0less.pandoc because the section "PV drivers" suggests that if the property "xen,enhanced" is specified, then we would end up to allocate information for PV drivers. > > AFAIU, this is not the case when "evtchn" is specified. > >> The configurable option is for domUs only. For dom0 we always set the >> corresponding property in the Xen code to true. >> Signed-off-by: Rahul Singh <rahul.singh@arm.com> >> --- >> Changes in v2: >> - no change >> --- >> --- >> xen/arch/arm/domain_build.c | 149 ++++++++++++++++-------------- >> xen/arch/arm/include/asm/kernel.h | 3 + >> 2 files changed, 82 insertions(+), 70 deletions(-) >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >> index 5101bca979..bd8b8475b7 100644 >> --- a/xen/arch/arm/domain_build.c >> +++ b/xen/arch/arm/domain_build.c >> @@ -1396,85 +1396,92 @@ static int __init make_hypervisor_node(struct domain *d, >> if ( res ) >> return res; >> > > > The diff below is quite difficult to read. I have applied to have a look. You seem to have simply indented the code and now some of the > lines are over the 80 characters mark. > > Ideally, I would like to avoid large 'if'. So I would suggest to either > re-ordering the code or split in multiple functions. > > However, reading the binding of "xen,xen", the property "reg" and "interrupts" are not optional. > > I also don't think can make them optional because some OSes may not boot if it can't find one of the property. > > In any case, at minimum you should explain why this is fine to make them optional. > > [...] If we want to expose the "reg” and “interrupts” property always to guests and these properties are not optional then we can discard this patch and add support for "xen,enhanced” property for domUs for static evtchn to work for domUs Please let me know your view on this. diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index bfe7bc6b36..a1e23eee59 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -3562,12 +3562,7 @@ static int __init construct_domU(struct domain *d, if ( rc == -EILSEQ || rc == -ENODATA || (rc == 0 && !strcmp(dom0less_enhanced, “enabled”)) ) - { - if ( hardware_domain ) kinfo.dom0less_enhanced = true; - else - panic(“Tried to use xen,enhanced without dom0\n”); - } if ( vcpu_create(d, 0) == NULL ) return -ENOMEM; Regards, Rahul
On 24/08/2022 13:15, Rahul Singh wrote: > Hi Julien, Hi Rahul, > Please let me know your view on this. > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index bfe7bc6b36..a1e23eee59 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -3562,12 +3562,7 @@ static int __init construct_domU(struct domain *d, > if ( rc == -EILSEQ || > rc == -ENODATA || > (rc == 0 && !strcmp(dom0less_enhanced, “enabled”)) ) > - { > - if ( hardware_domain ) > kinfo.dom0less_enhanced = true; > - else > - panic(“Tried to use xen,enhanced without dom0\n”); > - } You can't use "xen,enhanced" without dom0. In fact, you will end up to dereference NULL in alloc_xenstore_evtchn(). That's because "xen,enhanced" means the domain will be able to use Xenstored. Now if you want to support your feature without a dom0. Then I think we want to introduce an option which would be the same as "xen,enhanced" but doesn't expose Xenstored. Cheers,
Hi Julien, > On 24 Aug 2022, at 1:59 pm, Julien Grall <julien@xen.org> wrote: > > > > On 24/08/2022 13:15, Rahul Singh wrote: >> Hi Julien, > > Hi Rahul, > >> Please let me know your view on this. >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >> index bfe7bc6b36..a1e23eee59 100644 >> --- a/xen/arch/arm/domain_build.c >> +++ b/xen/arch/arm/domain_build.c >> @@ -3562,12 +3562,7 @@ static int __init construct_domU(struct domain *d, >> if ( rc == -EILSEQ || >> rc == -ENODATA || >> (rc == 0 && !strcmp(dom0less_enhanced, “enabled”)) ) >> - { >> - if ( hardware_domain ) >> kinfo.dom0less_enhanced = true; >> - else >> - panic(“Tried to use xen,enhanced without dom0\n”); >> - } > > You can't use "xen,enhanced" without dom0. In fact, you will end up to dereference NULL in alloc_xenstore_evtchn(). That's because "xen,enhanced" means the domain will be able to use Xenstored. > > Now if you want to support your feature without a dom0. Then I think we want to introduce an option which would be the same as "xen,enhanced" but doesn't expose Xenstored. If we modify the patch as below we can use the "xen,enhanced" for domUs without dom0. I tested the patch and its works fine. Do you see any issue with this approach? diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index cffd508af2..870846b742 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -3568,12 +3568,7 @@ static int __init construct_domU(struct domain *d, if ( rc == -EILSEQ || rc == -ENODATA || (rc == 0 && !strcmp(dom0less_enhanced, "enabled")) ) - { - if ( hardware_domain ) kinfo.dom0less_enhanced = true; - else - panic("Tried to use xen,enhanced without dom0\n"); - } if ( vcpu_create(d, 0) == NULL ) return -ENOMEM; @@ -3613,9 +3608,8 @@ static int __init construct_domU(struct domain *d, if ( rc < 0 ) return rc; - if ( kinfo.dom0less_enhanced ) + if ( kinfo.dom0less_enhanced && hardware_domain ) { - ASSERT(hardware_domain); rc = alloc_xenstore_evtchn(d); if ( rc < 0 ) return rc; Regards, Rahul
On 24/08/2022 15:42, Rahul Singh wrote: >> On 24 Aug 2022, at 1:59 pm, Julien Grall <julien@xen.org> wrote: >> >> >> >> On 24/08/2022 13:15, Rahul Singh wrote: >>> Hi Julien, >> >> Hi Rahul, >> >>> Please let me know your view on this. >>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>> index bfe7bc6b36..a1e23eee59 100644 >>> --- a/xen/arch/arm/domain_build.c >>> +++ b/xen/arch/arm/domain_build.c >>> @@ -3562,12 +3562,7 @@ static int __init construct_domU(struct domain *d, >>> if ( rc == -EILSEQ || >>> rc == -ENODATA || >>> (rc == 0 && !strcmp(dom0less_enhanced, “enabled”)) ) >>> - { >>> - if ( hardware_domain ) >>> kinfo.dom0less_enhanced = true; >>> - else >>> - panic(“Tried to use xen,enhanced without dom0\n”); >>> - } >> >> You can't use "xen,enhanced" without dom0. In fact, you will end up to dereference NULL in alloc_xenstore_evtchn(). That's because "xen,enhanced" means the domain will be able to use Xenstored. >> >> Now if you want to support your feature without a dom0. Then I think we want to introduce an option which would be the same as "xen,enhanced" but doesn't expose Xenstored. > > If we modify the patch as below we can use the "xen,enhanced" for domUs without dom0. > I tested the patch and its works fine. Do you see any issue with this approach? Yes. For two reasons: 1) It is muddying the meaning of "xen,enhanced". In particular a user may not realize that Xenstore is not available if dom0 is not present. 2) It would be more complicated to handle the case where Xenstored lives in a non-dom0 domain. I am not aware of anyone wanting this on Arm yet, but I don't want to close the door. So if you want to support create "xen,xen" without all the rest. Then I think we need a different property value. I don't have a good suggestion for the name. Cheers,
Hi Julien > On 24 Aug 2022, at 4:36 pm, Julien Grall <julien@xen.org> wrote: > > On 24/08/2022 15:42, Rahul Singh wrote: >>> On 24 Aug 2022, at 1:59 pm, Julien Grall <julien@xen.org> wrote: >>> >>> >>> >>> On 24/08/2022 13:15, Rahul Singh wrote: >>>> Hi Julien, >>> >>> Hi Rahul, >>> >>>> Please let me know your view on this. >>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>>> index bfe7bc6b36..a1e23eee59 100644 >>>> --- a/xen/arch/arm/domain_build.c >>>> +++ b/xen/arch/arm/domain_build.c >>>> @@ -3562,12 +3562,7 @@ static int __init construct_domU(struct domain *d, >>>> if ( rc == -EILSEQ || >>>> rc == -ENODATA || >>>> (rc == 0 && !strcmp(dom0less_enhanced, “enabled”)) ) >>>> - { >>>> - if ( hardware_domain ) >>>> kinfo.dom0less_enhanced = true; >>>> - else >>>> - panic(“Tried to use xen,enhanced without dom0\n”); >>>> - } >>> >>> You can't use "xen,enhanced" without dom0. In fact, you will end up to dereference NULL in alloc_xenstore_evtchn(). That's because "xen,enhanced" means the domain will be able to use Xenstored. >>> >>> Now if you want to support your feature without a dom0. Then I think we want to introduce an option which would be the same as "xen,enhanced" but doesn't expose Xenstored. >> If we modify the patch as below we can use the "xen,enhanced" for domUs without dom0. >> I tested the patch and its works fine. Do you see any issue with this approach? > > Yes. For two reasons: > 1) It is muddying the meaning of "xen,enhanced". In particular a user may not realize that Xenstore is not available if dom0 is not present. > 2) It would be more complicated to handle the case where Xenstored lives in a non-dom0 domain. I am not aware of anyone wanting this on Arm yet, but I don't want to close the door. > > So if you want to support create "xen,xen" without all the rest. Then I think we need a different property value. I don't have a good suggestion for the name. Is that okay if we use the earlier approach, when user set "xen,enhanced = evtchn” we will not call alloc_xenstore_evtchn() but we create hypervisor node with all fields. Regards, Rahul
On Wed, 24 Aug 2022, Rahul Singh wrote: > > On 24 Aug 2022, at 4:36 pm, Julien Grall <julien@xen.org> wrote: > > On 24/08/2022 15:42, Rahul Singh wrote: > >>> On 24 Aug 2022, at 1:59 pm, Julien Grall <julien@xen.org> wrote: > >>> > >>> > >>> > >>> On 24/08/2022 13:15, Rahul Singh wrote: > >>>> Hi Julien, > >>> > >>> Hi Rahul, > >>> > >>>> Please let me know your view on this. > >>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > >>>> index bfe7bc6b36..a1e23eee59 100644 > >>>> --- a/xen/arch/arm/domain_build.c > >>>> +++ b/xen/arch/arm/domain_build.c > >>>> @@ -3562,12 +3562,7 @@ static int __init construct_domU(struct domain *d, > >>>> if ( rc == -EILSEQ || > >>>> rc == -ENODATA || > >>>> (rc == 0 && !strcmp(dom0less_enhanced, “enabled”)) ) > >>>> - { > >>>> - if ( hardware_domain ) > >>>> kinfo.dom0less_enhanced = true; > >>>> - else > >>>> - panic(“Tried to use xen,enhanced without dom0\n”); > >>>> - } > >>> > >>> You can't use "xen,enhanced" without dom0. In fact, you will end up to dereference NULL in alloc_xenstore_evtchn(). That's because "xen,enhanced" means the domain will be able to use Xenstored. > >>> > >>> Now if you want to support your feature without a dom0. Then I think we want to introduce an option which would be the same as "xen,enhanced" but doesn't expose Xenstored. > >> If we modify the patch as below we can use the "xen,enhanced" for domUs without dom0. > >> I tested the patch and its works fine. Do you see any issue with this approach? > > > > Yes. For two reasons: > > 1) It is muddying the meaning of "xen,enhanced". In particular a user may not realize that Xenstore is not available if dom0 is not present. > > 2) It would be more complicated to handle the case where Xenstored lives in a non-dom0 domain. I am not aware of anyone wanting this on Arm yet, but I don't want to close the door. > > > > So if you want to support create "xen,xen" without all the rest. Then I think we need a different property value. I don't have a good suggestion for the name. > > Is that okay if we use the earlier approach, when user set "xen,enhanced = evtchn” we will not call alloc_xenstore_evtchn() > but we create hypervisor node with all fields. Thinking more about this, today xen,enhanced has the implication that: - the guest will get a regular and complete "xen,xen" node in device tree - xenstore and PV drivers will be available (full Xen interfaces support) We don't necessarely imply that dom0 is required (from a domU point of view) but we do imply that xenstore+evtchn+gnttab will be available to the domU. Now, static event channels are different. They don't require xenstore and they don't require gnttab. It is as if the current xen,enhanced node actually meant: xen,enhanced = "xenstore,gnttab,evtchn"; and now we are only enabling a subset: xen,enhanced = "evtchn"; Is that a correct understanding? If so, we can clarify that: xen,enhanced; it is a convenient shortend for: xen,enhanced = "xenstore,gnttab,evtchn"; and that other combinations are also acceptable, e.g.: xen,enhanced = "gnttab"; xen,enhanced = "evtchn"; xen,enhanced = "evtchn,gnttab"; It is OK to panic if the user specifies an option that is currently unsupported (e.g. "gnttab"). In practice xenstore requires both gnttab and evtchn, I don't know if we want to write that down in the device tree bindings. We could panic if the user specifies: xen,enhanced = "xenstore,evtchn"; What do you guys think?
Hi Stefano, On 24/08/2022 22:59, Stefano Stabellini wrote: > On Wed, 24 Aug 2022, Rahul Singh wrote: >>> On 24 Aug 2022, at 4:36 pm, Julien Grall <julien@xen.org> wrote: >>> On 24/08/2022 15:42, Rahul Singh wrote: >>>>> On 24 Aug 2022, at 1:59 pm, Julien Grall <julien@xen.org> wrote: >>>>> >>>>> >>>>> >>>>> On 24/08/2022 13:15, Rahul Singh wrote: >>>>>> Hi Julien, >>>>> >>>>> Hi Rahul, >>>>> >>>>>> Please let me know your view on this. >>>>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>>>>> index bfe7bc6b36..a1e23eee59 100644 >>>>>> --- a/xen/arch/arm/domain_build.c >>>>>> +++ b/xen/arch/arm/domain_build.c >>>>>> @@ -3562,12 +3562,7 @@ static int __init construct_domU(struct domain *d, >>>>>> if ( rc == -EILSEQ || >>>>>> rc == -ENODATA || >>>>>> (rc == 0 && !strcmp(dom0less_enhanced, “enabled”)) ) >>>>>> - { >>>>>> - if ( hardware_domain ) >>>>>> kinfo.dom0less_enhanced = true; >>>>>> - else >>>>>> - panic(“Tried to use xen,enhanced without dom0\n”); >>>>>> - } >>>>> >>>>> You can't use "xen,enhanced" without dom0. In fact, you will end up to dereference NULL in alloc_xenstore_evtchn(). That's because "xen,enhanced" means the domain will be able to use Xenstored. >>>>> >>>>> Now if you want to support your feature without a dom0. Then I think we want to introduce an option which would be the same as "xen,enhanced" but doesn't expose Xenstored. >>>> If we modify the patch as below we can use the "xen,enhanced" for domUs without dom0. >>>> I tested the patch and its works fine. Do you see any issue with this approach? >>> >>> Yes. For two reasons: >>> 1) It is muddying the meaning of "xen,enhanced". In particular a user may not realize that Xenstore is not available if dom0 is not present. >>> 2) It would be more complicated to handle the case where Xenstored lives in a non-dom0 domain. I am not aware of anyone wanting this on Arm yet, but I don't want to close the door. >>> >>> So if you want to support create "xen,xen" without all the rest. Then I think we need a different property value. I don't have a good suggestion for the name. >> >> Is that okay if we use the earlier approach, when user set "xen,enhanced = evtchn” we will not call alloc_xenstore_evtchn() >> but we create hypervisor node with all fields. > > Thinking more about this, today xen,enhanced has the implication that: > > - the guest will get a regular and complete "xen,xen" node in device tree > - xenstore and PV drivers will be available (full Xen interfaces support) > > We don't necessarely imply that dom0 is required (from a domU point of > view) but we do imply that xenstore+evtchn+gnttab will be available to > the domU. > > Now, static event channels are different. They don't require xenstore > and they don't require gnttab. > > It is as if the current xen,enhanced node actually meant: > > xen,enhanced = "xenstore,gnttab,evtchn"; Correct. > > and now we are only enabling a subset: > > xen,enhanced = "evtchn"; > > Is that a correct understanding? Yes with some cavears (see below). > > > If so, we can clarify that: > > xen,enhanced; > > it is a convenient shortend for: > > xen,enhanced = "xenstore,gnttab,evtchn"; > > and that other combinations are also acceptable, e.g.: > > xen,enhanced = "gnttab"; > xen,enhanced = "evtchn"; > xen,enhanced = "evtchn,gnttab"; > > It is OK to panic if the user specifies an option that is currently > unsupported (e.g. "gnttab"). So today, if you create the node "xen,xen", the guest will expect to be able to use both grant-table and event channel. Therefore, in the list above, the only configuration we can sensibly support without any major rework is "evtchn,gnttab". If we want to support "evtchn" or "gnttab" only. Then we likely need to define a new binding (or new version) because neither "regs" nor "interrupts" are optional (although a guest OS is free to ignore them). > > In practice xenstore requires both gnttab and evtchn, I don't know if we > want to write that down in the device tree bindings. We could panic if > the user specifies: xen,enhanced = "xenstore,evtchn"; I think the interface for dom0less domUs is quite messy at the moment. Even if we don't advertise the support for event channel and grant-table, hypercalls. They are still accessible if the guest wish to do so. If we decide to introduce "evtchn", "gnttab" & co then we should also make sure that if "evtchn" is not specified then we are not allowing the guest to allocate any event channel (or map the grant-table). Otherwise, this is pointless if we try to tell the user "evtchn", "gnttab"... And just to be clear, I would be perfectly happy to break anyone trying to use event channel without "xen,enanced" because we didn't advertise the feature. So they should not use it. Cheers,
On Wed, 24 Aug 2022, Julien Grall wrote: > On 24/08/2022 22:59, Stefano Stabellini wrote: > > On Wed, 24 Aug 2022, Rahul Singh wrote: > > > > On 24 Aug 2022, at 4:36 pm, Julien Grall <julien@xen.org> wrote: > > > > On 24/08/2022 15:42, Rahul Singh wrote: > > > > > > On 24 Aug 2022, at 1:59 pm, Julien Grall <julien@xen.org> wrote: > > > > > > > > > > > > > > > > > > > > > > > > On 24/08/2022 13:15, Rahul Singh wrote: > > > > > > > Hi Julien, > > > > > > > > > > > > Hi Rahul, > > > > > > > > > > > > > Please let me know your view on this. > > > > > > > diff --git a/xen/arch/arm/domain_build.c > > > > > > > b/xen/arch/arm/domain_build.c > > > > > > > index bfe7bc6b36..a1e23eee59 100644 > > > > > > > --- a/xen/arch/arm/domain_build.c > > > > > > > +++ b/xen/arch/arm/domain_build.c > > > > > > > @@ -3562,12 +3562,7 @@ static int __init construct_domU(struct > > > > > > > domain *d, > > > > > > > if ( rc == -EILSEQ || > > > > > > > rc == -ENODATA || > > > > > > > (rc == 0 && !strcmp(dom0less_enhanced, “enabled”)) ) > > > > > > > - { > > > > > > > - if ( hardware_domain ) > > > > > > > kinfo.dom0less_enhanced = true; > > > > > > > - else > > > > > > > - panic(“Tried to use xen,enhanced without dom0\n”); > > > > > > > - } > > > > > > > > > > > > You can't use "xen,enhanced" without dom0. In fact, you will end up > > > > > > to dereference NULL in alloc_xenstore_evtchn(). That's because > > > > > > "xen,enhanced" means the domain will be able to use Xenstored. > > > > > > > > > > > > Now if you want to support your feature without a dom0. Then I think > > > > > > we want to introduce an option which would be the same as > > > > > > "xen,enhanced" but doesn't expose Xenstored. > > > > > If we modify the patch as below we can use the "xen,enhanced" for > > > > > domUs without dom0. > > > > > I tested the patch and its works fine. Do you see any issue with this > > > > > approach? > > > > > > > > Yes. For two reasons: > > > > 1) It is muddying the meaning of "xen,enhanced". In particular a user > > > > may not realize that Xenstore is not available if dom0 is not present. > > > > 2) It would be more complicated to handle the case where Xenstored lives > > > > in a non-dom0 domain. I am not aware of anyone wanting this on Arm yet, > > > > but I don't want to close the door. > > > > > > > > So if you want to support create "xen,xen" without all the rest. Then I > > > > think we need a different property value. I don't have a good suggestion > > > > for the name. > > > > > > Is that okay if we use the earlier approach, when user set "xen,enhanced > > > = evtchn” we will not call alloc_xenstore_evtchn() > > > but we create hypervisor node with all fields. > > > > Thinking more about this, today xen,enhanced has the implication that: > > > > - the guest will get a regular and complete "xen,xen" node in device tree > > - xenstore and PV drivers will be available (full Xen interfaces support) > > > > We don't necessarely imply that dom0 is required (from a domU point of > > view) but we do imply that xenstore+evtchn+gnttab will be available to > > the domU. > > > > Now, static event channels are different. They don't require xenstore > > and they don't require gnttab. > > > > It is as if the current xen,enhanced node actually meant: > > > > xen,enhanced = "xenstore,gnttab,evtchn"; > > Correct. > > > > > and now we are only enabling a subset: > > > > xen,enhanced = "evtchn"; > > > > Is that a correct understanding? > > Yes with some cavears (see below). > > > > > > > If so, we can clarify that: > > > > xen,enhanced; > > > > it is a convenient shortend for: > > > > xen,enhanced = "xenstore,gnttab,evtchn"; > > > > and that other combinations are also acceptable, e.g.: > > > > xen,enhanced = "gnttab"; > > xen,enhanced = "evtchn"; > > xen,enhanced = "evtchn,gnttab"; > > > > It is OK to panic if the user specifies an option that is currently > > unsupported (e.g. "gnttab"). > > So today, if you create the node "xen,xen", the guest will expect to be able > to use both grant-table and event channel. > > Therefore, in the list above, the only configuration we can sensibly support > without any major rework is "evtchn,gnttab". > > If we want to support "evtchn" or "gnttab" only. Then we likely need to define > a new binding (or new version) because neither "regs" nor "interrupts" are > optional (although a guest OS is free to ignore them). Yes I think you are right. I also broadly agree with the rest of your reply. Thinking about it and given the above, we only need 2 "levels" of enhancement: 1) everything: xenstore, gnttab, evtchn 2) gnttab, evtchn, but not xenstore Nothing else is really possible because, as Julien pointed out, "xen,enhanced" implies the xen,xen node in the domU device tree and in turn that node implies both evtchn and gnttab. xenstore is separate and is detected using HVM_PARAM_STORE_EVTCHN and HVM_PARAM_STORE_PFN anyway. So I think we just need to add a way to express 2). We could do something like: xen,enhanced = "evtchn,gnttab"; Or we could use a new separate option like Julien initially suggested, e.g.: xen,enhanced-no-xenstore; "xen,enhanced-no-xenstore" is a terrible name actually, but just to explain what I am thinking :-)
Hi, > On 25 Aug 2022, at 02:10, Stefano Stabellini <sstabellini@kernel.org> wrote: > > On Wed, 24 Aug 2022, Julien Grall wrote: >> On 24/08/2022 22:59, Stefano Stabellini wrote: >>> On Wed, 24 Aug 2022, Rahul Singh wrote: >>>>> On 24 Aug 2022, at 4:36 pm, Julien Grall <julien@xen.org> wrote: >>>>> On 24/08/2022 15:42, Rahul Singh wrote: >>>>>>> On 24 Aug 2022, at 1:59 pm, Julien Grall <julien@xen.org> wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> On 24/08/2022 13:15, Rahul Singh wrote: >>>>>>>> Hi Julien, >>>>>>> >>>>>>> Hi Rahul, >>>>>>> >>>>>>>> Please let me know your view on this. >>>>>>>> diff --git a/xen/arch/arm/domain_build.c >>>>>>>> b/xen/arch/arm/domain_build.c >>>>>>>> index bfe7bc6b36..a1e23eee59 100644 >>>>>>>> --- a/xen/arch/arm/domain_build.c >>>>>>>> +++ b/xen/arch/arm/domain_build.c >>>>>>>> @@ -3562,12 +3562,7 @@ static int __init construct_domU(struct >>>>>>>> domain *d, >>>>>>>> if ( rc == -EILSEQ || >>>>>>>> rc == -ENODATA || >>>>>>>> (rc == 0 && !strcmp(dom0less_enhanced, “enabled”)) ) >>>>>>>> - { >>>>>>>> - if ( hardware_domain ) >>>>>>>> kinfo.dom0less_enhanced = true; >>>>>>>> - else >>>>>>>> - panic(“Tried to use xen,enhanced without dom0\n”); >>>>>>>> - } >>>>>>> >>>>>>> You can't use "xen,enhanced" without dom0. In fact, you will end up >>>>>>> to dereference NULL in alloc_xenstore_evtchn(). That's because >>>>>>> "xen,enhanced" means the domain will be able to use Xenstored. >>>>>>> >>>>>>> Now if you want to support your feature without a dom0. Then I think >>>>>>> we want to introduce an option which would be the same as >>>>>>> "xen,enhanced" but doesn't expose Xenstored. >>>>>> If we modify the patch as below we can use the "xen,enhanced" for >>>>>> domUs without dom0. >>>>>> I tested the patch and its works fine. Do you see any issue with this >>>>>> approach? >>>>> >>>>> Yes. For two reasons: >>>>> 1) It is muddying the meaning of "xen,enhanced". In particular a user >>>>> may not realize that Xenstore is not available if dom0 is not present. >>>>> 2) It would be more complicated to handle the case where Xenstored lives >>>>> in a non-dom0 domain. I am not aware of anyone wanting this on Arm yet, >>>>> but I don't want to close the door. >>>>> >>>>> So if you want to support create "xen,xen" without all the rest. Then I >>>>> think we need a different property value. I don't have a good suggestion >>>>> for the name. >>>> >>>> Is that okay if we use the earlier approach, when user set "xen,enhanced >>>> = evtchn” we will not call alloc_xenstore_evtchn() >>>> but we create hypervisor node with all fields. >>> >>> Thinking more about this, today xen,enhanced has the implication that: >>> >>> - the guest will get a regular and complete "xen,xen" node in device tree >>> - xenstore and PV drivers will be available (full Xen interfaces support) >>> >>> We don't necessarely imply that dom0 is required (from a domU point of >>> view) but we do imply that xenstore+evtchn+gnttab will be available to >>> the domU. >>> >>> Now, static event channels are different. They don't require xenstore >>> and they don't require gnttab. >>> >>> It is as if the current xen,enhanced node actually meant: >>> >>> xen,enhanced = "xenstore,gnttab,evtchn"; >> >> Correct. >> >>> >>> and now we are only enabling a subset: >>> >>> xen,enhanced = "evtchn"; >>> >>> Is that a correct understanding? >> >> Yes with some cavears (see below). >> >>> >>> >>> If so, we can clarify that: >>> >>> xen,enhanced; >>> >>> it is a convenient shortend for: >>> >>> xen,enhanced = "xenstore,gnttab,evtchn"; >>> >>> and that other combinations are also acceptable, e.g.: >>> >>> xen,enhanced = "gnttab"; >>> xen,enhanced = "evtchn"; >>> xen,enhanced = "evtchn,gnttab"; >>> >>> It is OK to panic if the user specifies an option that is currently >>> unsupported (e.g. "gnttab"). >> >> So today, if you create the node "xen,xen", the guest will expect to be able >> to use both grant-table and event channel. >> >> Therefore, in the list above, the only configuration we can sensibly support >> without any major rework is "evtchn,gnttab". >> >> If we want to support "evtchn" or "gnttab" only. Then we likely need to define >> a new binding (or new version) because neither "regs" nor "interrupts" are >> optional (although a guest OS is free to ignore them). > > Yes I think you are right. I also broadly agree with the rest of your > reply. > > Thinking about it and given the above, we only need 2 "levels" of > enhancement: > > 1) everything: xenstore, gnttab, evtchn > 2) gnttab, evtchn, but not xenstore > > Nothing else is really possible because, as Julien pointed out, > "xen,enhanced" implies the xen,xen node in the domU device tree and in > turn that node implies both evtchn and gnttab. So we could say that xen,enhanced always includes gnttab and Xenstore is optional. > > xenstore is separate and is detected using HVM_PARAM_STORE_EVTCHN and > HVM_PARAM_STORE_PFN anyway. Ack, not having Xenstore should be handled properly using the params. > > So I think we just need to add a way to express 2). We could do > something like: > > xen,enhanced = "evtchn,gnttab"; I am a bit puzzled here as gnttab is always there. > > Or we could use a new separate option like Julien initially suggested, > e.g.: > > xen,enhanced-no-xenstore; > > "xen,enhanced-no-xenstore" is a terrible name actually, but just to > explain what I am thinking :-) I think most common use case will be to have all, so make sense to allow to disable Xenstore. How about: xen,enhanced = “no-xenstore” ? An other solution is to keep xen,enhanced as it is and introduce a new option: Xen,no-xenstore At the end Xenstore cannot be used if there is no Dom0 and that we can detect easily. Also there is no solution at this stage to have an other domain then Dom0 providing Xenstore (maybe in the long term someone will want to introduce that and we will need a way to specify which domain is handling it). So I still think that we could just say that Xenstore can only be active if there is a Dom0 and just disable Xenstore automatically if it is not the case. If there is a dom0 and someone wants a guest without Xenstore, then we would need to have the no-xenstore support. But is it a use case ? All in all, enhance dom0less was not supported before 4.17 so we will not create any backward compatibility issue. What do you guys think ? Cheers Bertrand
On 25/08/2022 08:39, Bertrand Marquis wrote: > Hi, > >> On 25 Aug 2022, at 02:10, Stefano Stabellini <sstabellini@kernel.org> wrote: >> >> On Wed, 24 Aug 2022, Julien Grall wrote: >>> On 24/08/2022 22:59, Stefano Stabellini wrote: >>>> On Wed, 24 Aug 2022, Rahul Singh wrote: >>>>>> On 24 Aug 2022, at 4:36 pm, Julien Grall <julien@xen.org> wrote: >>>>>> On 24/08/2022 15:42, Rahul Singh wrote: >>>>>>>> On 24 Aug 2022, at 1:59 pm, Julien Grall <julien@xen.org> wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On 24/08/2022 13:15, Rahul Singh wrote: >>>>>>>>> Hi Julien, >>>>>>>> >>>>>>>> Hi Rahul, >>>>>>>> >>>>>>>>> Please let me know your view on this. >>>>>>>>> diff --git a/xen/arch/arm/domain_build.c >>>>>>>>> b/xen/arch/arm/domain_build.c >>>>>>>>> index bfe7bc6b36..a1e23eee59 100644 >>>>>>>>> --- a/xen/arch/arm/domain_build.c >>>>>>>>> +++ b/xen/arch/arm/domain_build.c >>>>>>>>> @@ -3562,12 +3562,7 @@ static int __init construct_domU(struct >>>>>>>>> domain *d, >>>>>>>>> if ( rc == -EILSEQ || >>>>>>>>> rc == -ENODATA || >>>>>>>>> (rc == 0 && !strcmp(dom0less_enhanced, “enabled”)) ) >>>>>>>>> - { >>>>>>>>> - if ( hardware_domain ) >>>>>>>>> kinfo.dom0less_enhanced = true; >>>>>>>>> - else >>>>>>>>> - panic(“Tried to use xen,enhanced without dom0\n”); >>>>>>>>> - } >>>>>>>> >>>>>>>> You can't use "xen,enhanced" without dom0. In fact, you will end up >>>>>>>> to dereference NULL in alloc_xenstore_evtchn(). That's because >>>>>>>> "xen,enhanced" means the domain will be able to use Xenstored. >>>>>>>> >>>>>>>> Now if you want to support your feature without a dom0. Then I think >>>>>>>> we want to introduce an option which would be the same as >>>>>>>> "xen,enhanced" but doesn't expose Xenstored. >>>>>>> If we modify the patch as below we can use the "xen,enhanced" for >>>>>>> domUs without dom0. >>>>>>> I tested the patch and its works fine. Do you see any issue with this >>>>>>> approach? >>>>>> >>>>>> Yes. For two reasons: >>>>>> 1) It is muddying the meaning of "xen,enhanced". In particular a user >>>>>> may not realize that Xenstore is not available if dom0 is not present. >>>>>> 2) It would be more complicated to handle the case where Xenstored lives >>>>>> in a non-dom0 domain. I am not aware of anyone wanting this on Arm yet, >>>>>> but I don't want to close the door. >>>>>> >>>>>> So if you want to support create "xen,xen" without all the rest. Then I >>>>>> think we need a different property value. I don't have a good suggestion >>>>>> for the name. >>>>> >>>>> Is that okay if we use the earlier approach, when user set "xen,enhanced >>>>> = evtchn” we will not call alloc_xenstore_evtchn() >>>>> but we create hypervisor node with all fields. >>>> >>>> Thinking more about this, today xen,enhanced has the implication that: >>>> >>>> - the guest will get a regular and complete "xen,xen" node in device tree >>>> - xenstore and PV drivers will be available (full Xen interfaces support) >>>> >>>> We don't necessarely imply that dom0 is required (from a domU point of >>>> view) but we do imply that xenstore+evtchn+gnttab will be available to >>>> the domU. >>>> >>>> Now, static event channels are different. They don't require xenstore >>>> and they don't require gnttab. >>>> >>>> It is as if the current xen,enhanced node actually meant: >>>> >>>> xen,enhanced = "xenstore,gnttab,evtchn"; >>> >>> Correct. >>> >>>> >>>> and now we are only enabling a subset: >>>> >>>> xen,enhanced = "evtchn"; >>>> >>>> Is that a correct understanding? >>> >>> Yes with some cavears (see below). >>> >>>> >>>> >>>> If so, we can clarify that: >>>> >>>> xen,enhanced; >>>> >>>> it is a convenient shortend for: >>>> >>>> xen,enhanced = "xenstore,gnttab,evtchn"; >>>> >>>> and that other combinations are also acceptable, e.g.: >>>> >>>> xen,enhanced = "gnttab"; >>>> xen,enhanced = "evtchn"; >>>> xen,enhanced = "evtchn,gnttab"; >>>> >>>> It is OK to panic if the user specifies an option that is currently >>>> unsupported (e.g. "gnttab"). >>> >>> So today, if you create the node "xen,xen", the guest will expect to be able >>> to use both grant-table and event channel. >>> >>> Therefore, in the list above, the only configuration we can sensibly support >>> without any major rework is "evtchn,gnttab". >>> >>> If we want to support "evtchn" or "gnttab" only. Then we likely need to define >>> a new binding (or new version) because neither "regs" nor "interrupts" are >>> optional (although a guest OS is free to ignore them). >> >> Yes I think you are right. I also broadly agree with the rest of your >> reply. >> >> Thinking about it and given the above, we only need 2 "levels" of >> enhancement: >> >> 1) everything: xenstore, gnttab, evtchn >> 2) gnttab, evtchn, but not xenstore >> >> Nothing else is really possible because, as Julien pointed out, >> "xen,enhanced" implies the xen,xen node in the domU device tree and in >> turn that node implies both evtchn and gnttab. > > So we could say that xen,enhanced always includes gnttab and Xenstore is optional. Not really, Xenstore has always been part of the story in Xen. So I think making it optional for "xen,enhanced" is going to make more difficult for user to understand what the meaning of the option (in particular that in the future we may want to support Xenstored in a separate domain). >> So I think we just need to add a way to express 2). We could do >> something like: >> >> xen,enhanced = "evtchn,gnttab"; > > I am a bit puzzled here as gnttab is always there. What do you mean? > >> >> Or we could use a new separate option like Julien initially suggested, >> e.g.: >> >> xen,enhanced-no-xenstore; >> >> "xen,enhanced-no-xenstore" is a terrible name actually, but just to >> explain what I am thinking :-) > > I think most common use case will be to have all, so make sense to allow to disable Xenstore. > > How about: > xen,enhanced = “no-xenstore” ? I would be fine with it. > > An other solution is to keep xen,enhanced as it is and introduce a new option: > Xen,no-xenstore I don't like the idea of introducing yet another option. > > At the end Xenstore cannot be used if there is no Dom0 and that we can detect easily. > Also there is no solution at this stage to have an other domain then Dom0 providing > Xenstore (maybe in the long term someone will want to introduce that and we will need > a way to specify which domain is handling it). > > So I still think that we could just say that Xenstore can only be active if there is a Dom0 > and just disable Xenstore automatically if it is not the case. See above about disabling Xenstore automatically. > If there is a dom0 and someone wants a guest without Xenstore, then we would need to > have the no-xenstore support. > But is it a use case ? Do you mean when "xen,enhanced" is specified? If yes, this could be useful if one want to limit the interface exposed to the guest. > > All in all, enhance dom0less was not supported before 4.17 so we will not create any > backward compatibility issue. I agree we still have the flexibility to change. Cheers,
Hi Julien, > On 25 Aug 2022, at 10:37, Julien Grall <julien@xen.org> wrote: > > > > On 25/08/2022 08:39, Bertrand Marquis wrote: >> Hi, >>> On 25 Aug 2022, at 02:10, Stefano Stabellini <sstabellini@kernel.org> wrote: >>> >>> On Wed, 24 Aug 2022, Julien Grall wrote: >>>> On 24/08/2022 22:59, Stefano Stabellini wrote: >>>>> On Wed, 24 Aug 2022, Rahul Singh wrote: >>>>>>> On 24 Aug 2022, at 4:36 pm, Julien Grall <julien@xen.org> wrote: >>>>>>> On 24/08/2022 15:42, Rahul Singh wrote: >>>>>>>>> On 24 Aug 2022, at 1:59 pm, Julien Grall <julien@xen.org> wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On 24/08/2022 13:15, Rahul Singh wrote: >>>>>>>>>> Hi Julien, >>>>>>>>> >>>>>>>>> Hi Rahul, >>>>>>>>> >>>>>>>>>> Please let me know your view on this. >>>>>>>>>> diff --git a/xen/arch/arm/domain_build.c >>>>>>>>>> b/xen/arch/arm/domain_build.c >>>>>>>>>> index bfe7bc6b36..a1e23eee59 100644 >>>>>>>>>> --- a/xen/arch/arm/domain_build.c >>>>>>>>>> +++ b/xen/arch/arm/domain_build.c >>>>>>>>>> @@ -3562,12 +3562,7 @@ static int __init construct_domU(struct >>>>>>>>>> domain *d, >>>>>>>>>> if ( rc == -EILSEQ || >>>>>>>>>> rc == -ENODATA || >>>>>>>>>> (rc == 0 && !strcmp(dom0less_enhanced, “enabled”)) ) >>>>>>>>>> - { >>>>>>>>>> - if ( hardware_domain ) >>>>>>>>>> kinfo.dom0less_enhanced = true; >>>>>>>>>> - else >>>>>>>>>> - panic(“Tried to use xen,enhanced without dom0\n”); >>>>>>>>>> - } >>>>>>>>> >>>>>>>>> You can't use "xen,enhanced" without dom0. In fact, you will end up >>>>>>>>> to dereference NULL in alloc_xenstore_evtchn(). That's because >>>>>>>>> "xen,enhanced" means the domain will be able to use Xenstored. >>>>>>>>> >>>>>>>>> Now if you want to support your feature without a dom0. Then I think >>>>>>>>> we want to introduce an option which would be the same as >>>>>>>>> "xen,enhanced" but doesn't expose Xenstored. >>>>>>>> If we modify the patch as below we can use the "xen,enhanced" for >>>>>>>> domUs without dom0. >>>>>>>> I tested the patch and its works fine. Do you see any issue with this >>>>>>>> approach? >>>>>>> >>>>>>> Yes. For two reasons: >>>>>>> 1) It is muddying the meaning of "xen,enhanced". In particular a user >>>>>>> may not realize that Xenstore is not available if dom0 is not present. >>>>>>> 2) It would be more complicated to handle the case where Xenstored lives >>>>>>> in a non-dom0 domain. I am not aware of anyone wanting this on Arm yet, >>>>>>> but I don't want to close the door. >>>>>>> >>>>>>> So if you want to support create "xen,xen" without all the rest. Then I >>>>>>> think we need a different property value. I don't have a good suggestion >>>>>>> for the name. >>>>>> >>>>>> Is that okay if we use the earlier approach, when user set "xen,enhanced >>>>>> = evtchn” we will not call alloc_xenstore_evtchn() >>>>>> but we create hypervisor node with all fields. >>>>> >>>>> Thinking more about this, today xen,enhanced has the implication that: >>>>> >>>>> - the guest will get a regular and complete "xen,xen" node in device tree >>>>> - xenstore and PV drivers will be available (full Xen interfaces support) >>>>> >>>>> We don't necessarely imply that dom0 is required (from a domU point of >>>>> view) but we do imply that xenstore+evtchn+gnttab will be available to >>>>> the domU. >>>>> >>>>> Now, static event channels are different. They don't require xenstore >>>>> and they don't require gnttab. >>>>> >>>>> It is as if the current xen,enhanced node actually meant: >>>>> >>>>> xen,enhanced = "xenstore,gnttab,evtchn"; >>>> >>>> Correct. >>>> >>>>> >>>>> and now we are only enabling a subset: >>>>> >>>>> xen,enhanced = "evtchn"; >>>>> >>>>> Is that a correct understanding? >>>> >>>> Yes with some cavears (see below). >>>> >>>>> >>>>> >>>>> If so, we can clarify that: >>>>> >>>>> xen,enhanced; >>>>> >>>>> it is a convenient shortend for: >>>>> >>>>> xen,enhanced = "xenstore,gnttab,evtchn"; >>>>> >>>>> and that other combinations are also acceptable, e.g.: >>>>> >>>>> xen,enhanced = "gnttab"; >>>>> xen,enhanced = "evtchn"; >>>>> xen,enhanced = "evtchn,gnttab"; >>>>> >>>>> It is OK to panic if the user specifies an option that is currently >>>>> unsupported (e.g. "gnttab"). >>>> >>>> So today, if you create the node "xen,xen", the guest will expect to be able >>>> to use both grant-table and event channel. >>>> >>>> Therefore, in the list above, the only configuration we can sensibly support >>>> without any major rework is "evtchn,gnttab". >>>> >>>> If we want to support "evtchn" or "gnttab" only. Then we likely need to define >>>> a new binding (or new version) because neither "regs" nor "interrupts" are >>>> optional (although a guest OS is free to ignore them). >>> >>> Yes I think you are right. I also broadly agree with the rest of your >>> reply. >>> >>> Thinking about it and given the above, we only need 2 "levels" of >>> enhancement: >>> >>> 1) everything: xenstore, gnttab, evtchn >>> 2) gnttab, evtchn, but not xenstore >>> >>> Nothing else is really possible because, as Julien pointed out, >>> "xen,enhanced" implies the xen,xen node in the domU device tree and in >>> turn that node implies both evtchn and gnttab. >> So we could say that xen,enhanced always includes gnttab and Xenstore is optional. > > Not really, Xenstore has always been part of the story in Xen. So I think making it optional for "xen,enhanced" is going to make more difficult for user to understand what the meaning of the option (in particular that in the future we may want to support Xenstored in a separate domain). Sorry wrong formulation, here I was meaning that we just need a solution to disable Xenstore (should still be here by default when supported). > >>> So I think we just need to add a way to express 2). We could do >>> something like: >>> >>> xen,enhanced = "evtchn,gnttab"; >> I am a bit puzzled here as gnttab is always there. > > What do you mean? Asking the user to specify gnttab in the list even though it is not supported to not have it in the list. > >>> >>> Or we could use a new separate option like Julien initially suggested, >>> e.g.: >>> >>> xen,enhanced-no-xenstore; >>> >>> "xen,enhanced-no-xenstore" is a terrible name actually, but just to >>> explain what I am thinking :-) >> I think most common use case will be to have all, so make sense to allow to disable Xenstore. >> How about: >> xen,enhanced = “no-xenstore” ? > > I would be fine with it. > >> An other solution is to keep xen,enhanced as it is and introduce a new option: >> Xen,no-xenstore > > I don't like the idea of introducing yet another option. > >> At the end Xenstore cannot be used if there is no Dom0 and that we can detect easily. >> Also there is no solution at this stage to have an other domain then Dom0 providing >> Xenstore (maybe in the long term someone will want to introduce that and we will need >> a way to specify which domain is handling it). >> So I still think that we could just say that Xenstore can only be active if there is a Dom0 >> and just disable Xenstore automatically if it is not the case. > > See above about disabling Xenstore automatically. Right now Xenstore can only work with a dom0 and if someone wants to have an other domain to provide it we would need a way to specify which one in the configuration. So in a configuration without dom0, I still think that not enabling Xenstore automatically is ok. > >> If there is a dom0 and someone wants a guest without Xenstore, then we would need to >> have the no-xenstore support. >> But is it a use case ? > > Do you mean when "xen,enhanced" is specified? If yes, this could be useful if one want to limit the interface exposed to the guest. How about the following: Xen,enhanced: gnttab, events and Xenstore if there is a dom0 Xen,enhanced = “[no-]xenstore,[no-]evtchn,[no-]gnttab” for when the user wants to explicitly specify what he wants (and Xen stopping on unsupported configuration). In this I would allow to provide any combinations of the 3 Bertrand > >> All in all, enhance dom0less was not supported before 4.17 so we will not create any >> backward compatibility issue. > > I agree we still have the flexibility to change. > > Cheers, > > -- > Julien Grall
On Thu, 25 Aug 2022, Bertrand Marquis wrote: > > On 25 Aug 2022, at 10:37, Julien Grall <julien@xen.org> wrote: > > On 25/08/2022 08:39, Bertrand Marquis wrote: > >> Hi, > >>> On 25 Aug 2022, at 02:10, Stefano Stabellini <sstabellini@kernel.org> wrote: > >>> > >>> On Wed, 24 Aug 2022, Julien Grall wrote: > >>>> On 24/08/2022 22:59, Stefano Stabellini wrote: > >>>>> On Wed, 24 Aug 2022, Rahul Singh wrote: > >>>>>>> On 24 Aug 2022, at 4:36 pm, Julien Grall <julien@xen.org> wrote: > >>>>>>> On 24/08/2022 15:42, Rahul Singh wrote: > >>>>>>>>> On 24 Aug 2022, at 1:59 pm, Julien Grall <julien@xen.org> wrote: > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> On 24/08/2022 13:15, Rahul Singh wrote: > >>>>>>>>>> Hi Julien, > >>>>>>>>> > >>>>>>>>> Hi Rahul, > >>>>>>>>> > >>>>>>>>>> Please let me know your view on this. > >>>>>>>>>> diff --git a/xen/arch/arm/domain_build.c > >>>>>>>>>> b/xen/arch/arm/domain_build.c > >>>>>>>>>> index bfe7bc6b36..a1e23eee59 100644 > >>>>>>>>>> --- a/xen/arch/arm/domain_build.c > >>>>>>>>>> +++ b/xen/arch/arm/domain_build.c > >>>>>>>>>> @@ -3562,12 +3562,7 @@ static int __init construct_domU(struct > >>>>>>>>>> domain *d, > >>>>>>>>>> if ( rc == -EILSEQ || > >>>>>>>>>> rc == -ENODATA || > >>>>>>>>>> (rc == 0 && !strcmp(dom0less_enhanced, “enabled”)) ) > >>>>>>>>>> - { > >>>>>>>>>> - if ( hardware_domain ) > >>>>>>>>>> kinfo.dom0less_enhanced = true; > >>>>>>>>>> - else > >>>>>>>>>> - panic(“Tried to use xen,enhanced without dom0\n”); > >>>>>>>>>> - } > >>>>>>>>> > >>>>>>>>> You can't use "xen,enhanced" without dom0. In fact, you will end up > >>>>>>>>> to dereference NULL in alloc_xenstore_evtchn(). That's because > >>>>>>>>> "xen,enhanced" means the domain will be able to use Xenstored. > >>>>>>>>> > >>>>>>>>> Now if you want to support your feature without a dom0. Then I think > >>>>>>>>> we want to introduce an option which would be the same as > >>>>>>>>> "xen,enhanced" but doesn't expose Xenstored. > >>>>>>>> If we modify the patch as below we can use the "xen,enhanced" for > >>>>>>>> domUs without dom0. > >>>>>>>> I tested the patch and its works fine. Do you see any issue with this > >>>>>>>> approach? > >>>>>>> > >>>>>>> Yes. For two reasons: > >>>>>>> 1) It is muddying the meaning of "xen,enhanced". In particular a user > >>>>>>> may not realize that Xenstore is not available if dom0 is not present. > >>>>>>> 2) It would be more complicated to handle the case where Xenstored lives > >>>>>>> in a non-dom0 domain. I am not aware of anyone wanting this on Arm yet, > >>>>>>> but I don't want to close the door. > >>>>>>> > >>>>>>> So if you want to support create "xen,xen" without all the rest. Then I > >>>>>>> think we need a different property value. I don't have a good suggestion > >>>>>>> for the name. > >>>>>> > >>>>>> Is that okay if we use the earlier approach, when user set "xen,enhanced > >>>>>> = evtchn” we will not call alloc_xenstore_evtchn() > >>>>>> but we create hypervisor node with all fields. > >>>>> > >>>>> Thinking more about this, today xen,enhanced has the implication that: > >>>>> > >>>>> - the guest will get a regular and complete "xen,xen" node in device tree > >>>>> - xenstore and PV drivers will be available (full Xen interfaces support) > >>>>> > >>>>> We don't necessarely imply that dom0 is required (from a domU point of > >>>>> view) but we do imply that xenstore+evtchn+gnttab will be available to > >>>>> the domU. > >>>>> > >>>>> Now, static event channels are different. They don't require xenstore > >>>>> and they don't require gnttab. > >>>>> > >>>>> It is as if the current xen,enhanced node actually meant: > >>>>> > >>>>> xen,enhanced = "xenstore,gnttab,evtchn"; > >>>> > >>>> Correct. > >>>> > >>>>> > >>>>> and now we are only enabling a subset: > >>>>> > >>>>> xen,enhanced = "evtchn"; > >>>>> > >>>>> Is that a correct understanding? > >>>> > >>>> Yes with some cavears (see below). > >>>> > >>>>> > >>>>> > >>>>> If so, we can clarify that: > >>>>> > >>>>> xen,enhanced; > >>>>> > >>>>> it is a convenient shortend for: > >>>>> > >>>>> xen,enhanced = "xenstore,gnttab,evtchn"; > >>>>> > >>>>> and that other combinations are also acceptable, e.g.: > >>>>> > >>>>> xen,enhanced = "gnttab"; > >>>>> xen,enhanced = "evtchn"; > >>>>> xen,enhanced = "evtchn,gnttab"; > >>>>> > >>>>> It is OK to panic if the user specifies an option that is currently > >>>>> unsupported (e.g. "gnttab"). > >>>> > >>>> So today, if you create the node "xen,xen", the guest will expect to be able > >>>> to use both grant-table and event channel. > >>>> > >>>> Therefore, in the list above, the only configuration we can sensibly support > >>>> without any major rework is "evtchn,gnttab". > >>>> > >>>> If we want to support "evtchn" or "gnttab" only. Then we likely need to define > >>>> a new binding (or new version) because neither "regs" nor "interrupts" are > >>>> optional (although a guest OS is free to ignore them). > >>> > >>> Yes I think you are right. I also broadly agree with the rest of your > >>> reply. > >>> > >>> Thinking about it and given the above, we only need 2 "levels" of > >>> enhancement: > >>> > >>> 1) everything: xenstore, gnttab, evtchn > >>> 2) gnttab, evtchn, but not xenstore > >>> > >>> Nothing else is really possible because, as Julien pointed out, > >>> "xen,enhanced" implies the xen,xen node in the domU device tree and in > >>> turn that node implies both evtchn and gnttab. > >> So we could say that xen,enhanced always includes gnttab and Xenstore is optional. > > > > Not really, Xenstore has always been part of the story in Xen. So I think making it optional for "xen,enhanced" is going to make more difficult for user to understand what the meaning of the option (in particular that in the future we may want to support Xenstored in a separate domain). > > Sorry wrong formulation, here I was meaning that we just need a solution to disable Xenstore (should still be here by default when supported). > > > > >>> So I think we just need to add a way to express 2). We could do > >>> something like: > >>> > >>> xen,enhanced = "evtchn,gnttab"; > >> I am a bit puzzled here as gnttab is always there. > > > > What do you mean? > > Asking the user to specify gnttab in the list even though it is not supported to not have it in the list. > > > > >>> > >>> Or we could use a new separate option like Julien initially suggested, > >>> e.g.: > >>> > >>> xen,enhanced-no-xenstore; > >>> > >>> "xen,enhanced-no-xenstore" is a terrible name actually, but just to > >>> explain what I am thinking :-) > >> I think most common use case will be to have all, so make sense to allow to disable Xenstore. > >> How about: > >> xen,enhanced = “no-xenstore” ? > > > > I would be fine with it. We have agreement on this, so I would say let's keep it simple and go with this option. > >> An other solution is to keep xen,enhanced as it is and introduce a new option: > >> Xen,no-xenstore > > > > I don't like the idea of introducing yet another option. > > > >> At the end Xenstore cannot be used if there is no Dom0 and that we can detect easily. > >> Also there is no solution at this stage to have an other domain then Dom0 providing > >> Xenstore (maybe in the long term someone will want to introduce that and we will need > >> a way to specify which domain is handling it). > >> So I still think that we could just say that Xenstore can only be active if there is a Dom0 > >> and just disable Xenstore automatically if it is not the case. > > > > See above about disabling Xenstore automatically. > > Right now Xenstore can only work with a dom0 and if someone wants to have an other domain to provide it we would need a way to specify which one in the configuration. > So in a configuration without dom0, I still think that not enabling Xenstore automatically is ok. > > > > >> If there is a dom0 and someone wants a guest without Xenstore, then we would need to > >> have the no-xenstore support. > >> But is it a use case ? > > > > Do you mean when "xen,enhanced" is specified? If yes, this could be useful if one want to limit the interface exposed to the guest. > > How about the following: > Xen,enhanced: gnttab, events and Xenstore if there is a dom0 > Xen,enhanced = “[no-]xenstore,[no-]evtchn,[no-]gnttab” for when the user wants to explicitly specify what he wants (and Xen stopping on unsupported configuration). > In this I would allow to provide any combinations of the 3 I am OK with what you wrote as well, but considering the additional complexity that no-gnttab and no-evtchn entail given that they cannot be actually disabled today, I suggest to keep it simple and go with: xen,enhanced = "no-xenstore"
Hi Stefano, > On 25 Aug 2022, at 18:44, Stefano Stabellini <sstabellini@kernel.org> wrote: > > On Thu, 25 Aug 2022, Bertrand Marquis wrote: >>> On 25 Aug 2022, at 10:37, Julien Grall <julien@xen.org> wrote: >>> On 25/08/2022 08:39, Bertrand Marquis wrote: >>>> Hi, >>>>> On 25 Aug 2022, at 02:10, Stefano Stabellini <sstabellini@kernel.org> wrote: >>>>> >>>>> On Wed, 24 Aug 2022, Julien Grall wrote: >>>>>> On 24/08/2022 22:59, Stefano Stabellini wrote: >>>>>>> On Wed, 24 Aug 2022, Rahul Singh wrote: >>>>>>>>> On 24 Aug 2022, at 4:36 pm, Julien Grall <julien@xen.org> wrote: >>>>>>>>> On 24/08/2022 15:42, Rahul Singh wrote: >>>>>>>>>>> On 24 Aug 2022, at 1:59 pm, Julien Grall <julien@xen.org> wrote: >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On 24/08/2022 13:15, Rahul Singh wrote: >>>>>>>>>>>> Hi Julien, >>>>>>>>>>> >>>>>>>>>>> Hi Rahul, >>>>>>>>>>> >>>>>>>>>>>> Please let me know your view on this. >>>>>>>>>>>> diff --git a/xen/arch/arm/domain_build.c >>>>>>>>>>>> b/xen/arch/arm/domain_build.c >>>>>>>>>>>> index bfe7bc6b36..a1e23eee59 100644 >>>>>>>>>>>> --- a/xen/arch/arm/domain_build.c >>>>>>>>>>>> +++ b/xen/arch/arm/domain_build.c >>>>>>>>>>>> @@ -3562,12 +3562,7 @@ static int __init construct_domU(struct >>>>>>>>>>>> domain *d, >>>>>>>>>>>> if ( rc == -EILSEQ || >>>>>>>>>>>> rc == -ENODATA || >>>>>>>>>>>> (rc == 0 && !strcmp(dom0less_enhanced, “enabled”)) ) >>>>>>>>>>>> - { >>>>>>>>>>>> - if ( hardware_domain ) >>>>>>>>>>>> kinfo.dom0less_enhanced = true; >>>>>>>>>>>> - else >>>>>>>>>>>> - panic(“Tried to use xen,enhanced without dom0\n”); >>>>>>>>>>>> - } >>>>>>>>>>> >>>>>>>>>>> You can't use "xen,enhanced" without dom0. In fact, you will end up >>>>>>>>>>> to dereference NULL in alloc_xenstore_evtchn(). That's because >>>>>>>>>>> "xen,enhanced" means the domain will be able to use Xenstored. >>>>>>>>>>> >>>>>>>>>>> Now if you want to support your feature without a dom0. Then I think >>>>>>>>>>> we want to introduce an option which would be the same as >>>>>>>>>>> "xen,enhanced" but doesn't expose Xenstored. >>>>>>>>>> If we modify the patch as below we can use the "xen,enhanced" for >>>>>>>>>> domUs without dom0. >>>>>>>>>> I tested the patch and its works fine. Do you see any issue with this >>>>>>>>>> approach? >>>>>>>>> >>>>>>>>> Yes. For two reasons: >>>>>>>>> 1) It is muddying the meaning of "xen,enhanced". In particular a user >>>>>>>>> may not realize that Xenstore is not available if dom0 is not present. >>>>>>>>> 2) It would be more complicated to handle the case where Xenstored lives >>>>>>>>> in a non-dom0 domain. I am not aware of anyone wanting this on Arm yet, >>>>>>>>> but I don't want to close the door. >>>>>>>>> >>>>>>>>> So if you want to support create "xen,xen" without all the rest. Then I >>>>>>>>> think we need a different property value. I don't have a good suggestion >>>>>>>>> for the name. >>>>>>>> >>>>>>>> Is that okay if we use the earlier approach, when user set "xen,enhanced >>>>>>>> = evtchn” we will not call alloc_xenstore_evtchn() >>>>>>>> but we create hypervisor node with all fields. >>>>>>> >>>>>>> Thinking more about this, today xen,enhanced has the implication that: >>>>>>> >>>>>>> - the guest will get a regular and complete "xen,xen" node in device tree >>>>>>> - xenstore and PV drivers will be available (full Xen interfaces support) >>>>>>> >>>>>>> We don't necessarely imply that dom0 is required (from a domU point of >>>>>>> view) but we do imply that xenstore+evtchn+gnttab will be available to >>>>>>> the domU. >>>>>>> >>>>>>> Now, static event channels are different. They don't require xenstore >>>>>>> and they don't require gnttab. >>>>>>> >>>>>>> It is as if the current xen,enhanced node actually meant: >>>>>>> >>>>>>> xen,enhanced = "xenstore,gnttab,evtchn"; >>>>>> >>>>>> Correct. >>>>>> >>>>>>> >>>>>>> and now we are only enabling a subset: >>>>>>> >>>>>>> xen,enhanced = "evtchn"; >>>>>>> >>>>>>> Is that a correct understanding? >>>>>> >>>>>> Yes with some cavears (see below). >>>>>> >>>>>>> >>>>>>> >>>>>>> If so, we can clarify that: >>>>>>> >>>>>>> xen,enhanced; >>>>>>> >>>>>>> it is a convenient shortend for: >>>>>>> >>>>>>> xen,enhanced = "xenstore,gnttab,evtchn"; >>>>>>> >>>>>>> and that other combinations are also acceptable, e.g.: >>>>>>> >>>>>>> xen,enhanced = "gnttab"; >>>>>>> xen,enhanced = "evtchn"; >>>>>>> xen,enhanced = "evtchn,gnttab"; >>>>>>> >>>>>>> It is OK to panic if the user specifies an option that is currently >>>>>>> unsupported (e.g. "gnttab"). >>>>>> >>>>>> So today, if you create the node "xen,xen", the guest will expect to be able >>>>>> to use both grant-table and event channel. >>>>>> >>>>>> Therefore, in the list above, the only configuration we can sensibly support >>>>>> without any major rework is "evtchn,gnttab". >>>>>> >>>>>> If we want to support "evtchn" or "gnttab" only. Then we likely need to define >>>>>> a new binding (or new version) because neither "regs" nor "interrupts" are >>>>>> optional (although a guest OS is free to ignore them). >>>>> >>>>> Yes I think you are right. I also broadly agree with the rest of your >>>>> reply. >>>>> >>>>> Thinking about it and given the above, we only need 2 "levels" of >>>>> enhancement: >>>>> >>>>> 1) everything: xenstore, gnttab, evtchn >>>>> 2) gnttab, evtchn, but not xenstore >>>>> >>>>> Nothing else is really possible because, as Julien pointed out, >>>>> "xen,enhanced" implies the xen,xen node in the domU device tree and in >>>>> turn that node implies both evtchn and gnttab. >>>> So we could say that xen,enhanced always includes gnttab and Xenstore is optional. >>> >>> Not really, Xenstore has always been part of the story in Xen. So I think making it optional for "xen,enhanced" is going to make more difficult for user to understand what the meaning of the option (in particular that in the future we may want to support Xenstored in a separate domain). >> >> Sorry wrong formulation, here I was meaning that we just need a solution to disable Xenstore (should still be here by default when supported). >> >>> >>>>> So I think we just need to add a way to express 2). We could do >>>>> something like: >>>>> >>>>> xen,enhanced = "evtchn,gnttab"; >>>> I am a bit puzzled here as gnttab is always there. >>> >>> What do you mean? >> >> Asking the user to specify gnttab in the list even though it is not supported to not have it in the list. >> >>> >>>>> >>>>> Or we could use a new separate option like Julien initially suggested, >>>>> e.g.: >>>>> >>>>> xen,enhanced-no-xenstore; >>>>> >>>>> "xen,enhanced-no-xenstore" is a terrible name actually, but just to >>>>> explain what I am thinking :-) >>>> I think most common use case will be to have all, so make sense to allow to disable Xenstore. >>>> How about: >>>> xen,enhanced = “no-xenstore” ? >>> >>> I would be fine with it. > > We have agreement on this, so I would say let's keep it simple and go > with this option. > > >>>> An other solution is to keep xen,enhanced as it is and introduce a new option: >>>> Xen,no-xenstore >>> >>> I don't like the idea of introducing yet another option. >>> >>>> At the end Xenstore cannot be used if there is no Dom0 and that we can detect easily. >>>> Also there is no solution at this stage to have an other domain then Dom0 providing >>>> Xenstore (maybe in the long term someone will want to introduce that and we will need >>>> a way to specify which domain is handling it). >>>> So I still think that we could just say that Xenstore can only be active if there is a Dom0 >>>> and just disable Xenstore automatically if it is not the case. >>> >>> See above about disabling Xenstore automatically. >> >> Right now Xenstore can only work with a dom0 and if someone wants to have an other domain to provide it we would need a way to specify which one in the configuration. >> So in a configuration without dom0, I still think that not enabling Xenstore automatically is ok. >> >>> >>>> If there is a dom0 and someone wants a guest without Xenstore, then we would need to >>>> have the no-xenstore support. >>>> But is it a use case ? >>> >>> Do you mean when "xen,enhanced" is specified? If yes, this could be useful if one want to limit the interface exposed to the guest. >> >> How about the following: >> Xen,enhanced: gnttab, events and Xenstore if there is a dom0 >> Xen,enhanced = “[no-]xenstore,[no-]evtchn,[no-]gnttab” for when the user wants to explicitly specify what he wants (and Xen stopping on unsupported configuration). >> In this I would allow to provide any combinations of the 3 > > I am OK with what you wrote as well, but considering the additional > complexity that no-gnttab and no-evtchn entail given that they cannot be > actually disabled today, I suggest to keep it simple and go with: > > xen,enhanced = "no-xenstore" I agree with that. Cheers Bertrand
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 5101bca979..bd8b8475b7 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1396,85 +1396,92 @@ static int __init make_hypervisor_node(struct domain *d, if ( res ) return res; - if ( !opt_ext_regions ) - { - printk(XENLOG_INFO "%pd: extended regions support is disabled\n", d); - nr_ext_regions = 0; - } - else if ( is_32bit_domain(d) ) - { - printk(XENLOG_WARNING - "%pd: extended regions not supported for 32-bit guests\n", d); - nr_ext_regions = 0; - } - else + if ( kinfo->dom0less_enhanced ) { - ext_regions = xzalloc(struct meminfo); - if ( !ext_regions ) - return -ENOMEM; - - if ( is_domain_direct_mapped(d) ) + if ( !opt_ext_regions ) { - if ( !is_iommu_enabled(d) ) - res = find_unallocated_memory(kinfo, ext_regions); - else - res = find_memory_holes(kinfo, ext_regions); + printk(XENLOG_INFO + "%pd: extended regions support is disabled\n", d); + nr_ext_regions = 0; } - else + else if ( is_32bit_domain(d) ) { - res = find_domU_holes(kinfo, ext_regions); + printk(XENLOG_WARNING + "%pd: extended regions not supported for 32-bit guests\n", d); + nr_ext_regions = 0; } + else + { + ext_regions = xzalloc(struct meminfo); + if ( !ext_regions ) + return -ENOMEM; - if ( res ) - printk(XENLOG_WARNING "%pd: failed to allocate extended regions\n", - d); - nr_ext_regions = ext_regions->nr_banks; - } + if ( is_domain_direct_mapped(d) ) + { + if ( !is_iommu_enabled(d) ) + res = find_unallocated_memory(kinfo, ext_regions); + else + res = find_memory_holes(kinfo, ext_regions); + } + else + { + res = find_domU_holes(kinfo, ext_regions); + } - reg = xzalloc_array(__be32, (nr_ext_regions + 1) * (addrcells + sizecells)); - if ( !reg ) - { - xfree(ext_regions); - return -ENOMEM; - } + if ( res ) + printk(XENLOG_WARNING + "%pd: failed to allocate extended regions\n", d); + nr_ext_regions = ext_regions->nr_banks; + } - /* reg 0 is grant table space */ - cells = ®[0]; - dt_child_set_range(&cells, addrcells, sizecells, - kinfo->gnttab_start, kinfo->gnttab_size); - /* reg 1...N are extended regions */ - for ( i = 0; i < nr_ext_regions; i++ ) - { - u64 start = ext_regions->bank[i].start; - u64 size = ext_regions->bank[i].size; + reg = xzalloc_array(__be32, (nr_ext_regions + 1) * (addrcells + sizecells)); + if ( !reg ) + { + xfree(ext_regions); + return -ENOMEM; + } - printk("%pd: extended region %d: %#"PRIx64"->%#"PRIx64"\n", - d, i, start, start + size); + /* reg 0 is grant table space */ + cells = ®[0]; + dt_child_set_range(&cells, addrcells, sizecells, + kinfo->gnttab_start, kinfo->gnttab_size); + /* reg 1...N are extended regions */ + for ( i = 0; i < nr_ext_regions; i++ ) + { + u64 start = ext_regions->bank[i].start; + u64 size = ext_regions->bank[i].size; - dt_child_set_range(&cells, addrcells, sizecells, start, size); - } + printk("%pd: extended region %d: %#"PRIx64"->%#"PRIx64"\n", + d, i, start, start + size); - res = fdt_property(fdt, "reg", reg, - dt_cells_to_size(addrcells + sizecells) * - (nr_ext_regions + 1)); - xfree(ext_regions); - xfree(reg); + dt_child_set_range(&cells, addrcells, sizecells, start, size); + } - if ( res ) - return res; + res = fdt_property(fdt, "reg", reg, + dt_cells_to_size(addrcells + sizecells) * + (nr_ext_regions + 1)); + xfree(ext_regions); + xfree(reg); - BUG_ON(d->arch.evtchn_irq == 0); + if ( res ) + return res; + } - /* - * Interrupt event channel upcall: - * - Active-low level-sensitive - * - All CPUs - * TODO: Handle properly the cpumask; - */ - set_interrupt(intr, d->arch.evtchn_irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW); - res = fdt_property_interrupts(kinfo, &intr, 1); - if ( res ) - return res; + if ( kinfo->dom0less_evtchn ) + { + BUG_ON(d->arch.evtchn_irq == 0); + + /* + * Interrupt event channel upcall: + * - Active-low level-sensitive + * - All CPUs + * TODO: Handle properly the cpumask; + */ + set_interrupt(intr, d->arch.evtchn_irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW); + res = fdt_property_interrupts(kinfo, &intr, 1); + if ( res ) + return res; + } res = fdt_end_node(fdt); @@ -2891,7 +2898,7 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo) goto err; } - if ( kinfo->dom0less_enhanced ) + if ( kinfo->dom0less_enhanced || kinfo->dom0less_evtchn ) { ret = make_hypervisor_node(d, kinfo, addrcells, sizecells); if ( ret ) @@ -3343,11 +3350,11 @@ static int __init construct_domU(struct domain *d, rc == -ENODATA || (rc == 0 && !strcmp(dom0less_enhanced, "enabled")) ) { - if ( hardware_domain ) - kinfo.dom0less_enhanced = true; - else - panic("Tried to use xen,enhanced without dom0\n"); + kinfo.dom0less_enhanced = true; + kinfo.dom0less_evtchn = true; } + else if ( rc == 0 && !strcmp(dom0less_enhanced, "evtchn") ) + kinfo.dom0less_evtchn = true; if ( vcpu_create(d, 0) == NULL ) return -ENOMEM; @@ -3526,6 +3533,8 @@ static int __init construct_dom0(struct domain *d) kinfo.unassigned_mem = dom0_mem; kinfo.d = d; + kinfo.dom0less_enhanced = true; + kinfo.dom0less_evtchn = true; rc = kernel_probe(&kinfo, NULL); if ( rc < 0 ) diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h index c4dc039b54..7cff19b997 100644 --- a/xen/arch/arm/include/asm/kernel.h +++ b/xen/arch/arm/include/asm/kernel.h @@ -39,6 +39,9 @@ struct kernel_info { /* Enable PV drivers */ bool dom0less_enhanced; + /* Enable event-channel interface */ + bool dom0less_evtchn; + /* GIC phandle */ uint32_t phandle_gic;
Introduce a new "xen,enhanced" dom0less property value "evtchn" to enable/disable event-channel interfaces for dom0less guests. The configurable option is for domUs only. For dom0 we always set the corresponding property in the Xen code to true. Signed-off-by: Rahul Singh <rahul.singh@arm.com> --- Changes in v2: - no change --- --- xen/arch/arm/domain_build.c | 149 ++++++++++++++++-------------- xen/arch/arm/include/asm/kernel.h | 3 + 2 files changed, 82 insertions(+), 70 deletions(-)