| Message ID | 20220505001656.395419-4-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dom0less PV drivers | expand |
On 05.05.2022 02:16, Stefano Stabellini wrote: > From: Luca Miccio <lucmiccio@gmail.com> > > Export evtchn_alloc_unbound and make it __must_check. > > If "xen,enhanced" is enabled, then add to dom0less domains: > > - the hypervisor node in device tree > - the xenstore event channel > > The xenstore event channel is also used for the first notification to > let the guest know that xenstore has become available. > > Signed-off-by: Luca Miccio <lucmiccio@gmail.com> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> > CC: Julien Grall <julien@xen.org> > CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > CC: Bertrand Marquis <bertrand.marquis@arm.com> > CC: jbeulich@suse.com Somehow my ack given for v5 was lost? > --- > Changes in v5: > - merge with "xen: export evtchn_alloc_unbound" > - __must_check > > Changes in v3: > - use evtchn_alloc_unbound > > Changes in v2: > - set HVM_PARAM_STORE_PFN to ~0ULL at domain creation > - in alloc_xenstore_evtchn do not call _evtchn_alloc_unbound > > xen: export evtchn_alloc_unbound > > It will be used during dom0less domains construction. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> This is somewhat confusing to find in a post-commit-message remark. Jan
On Thu, 5 May 2022, Jan Beulich wrote: > On 05.05.2022 02:16, Stefano Stabellini wrote: > > From: Luca Miccio <lucmiccio@gmail.com> > > > > Export evtchn_alloc_unbound and make it __must_check. > > > > If "xen,enhanced" is enabled, then add to dom0less domains: > > > > - the hypervisor node in device tree > > - the xenstore event channel > > > > The xenstore event channel is also used for the first notification to > > let the guest know that xenstore has become available. > > > > Signed-off-by: Luca Miccio <lucmiccio@gmail.com> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > > Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> > > CC: Julien Grall <julien@xen.org> > > CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > > CC: Bertrand Marquis <bertrand.marquis@arm.com> > > CC: jbeulich@suse.com > > Somehow my ack given for v5 was lost? I'll put it back > > --- > > Changes in v5: > > - merge with "xen: export evtchn_alloc_unbound" > > - __must_check > > > > Changes in v3: > > - use evtchn_alloc_unbound > > > > Changes in v2: > > - set HVM_PARAM_STORE_PFN to ~0ULL at domain creation > > - in alloc_xenstore_evtchn do not call _evtchn_alloc_unbound > > > > xen: export evtchn_alloc_unbound > > > > It will be used during dom0less domains construction. > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > > This is somewhat confusing to find in a post-commit-message remark. Sorry I don't know what happened there
Hi Stefano, > On 5 May 2022, at 1:16 am, Stefano Stabellini <sstabellini@kernel.org> wrote: > > From: Luca Miccio <lucmiccio@gmail.com> > > Export evtchn_alloc_unbound and make it __must_check. > > If "xen,enhanced" is enabled, then add to dom0less domains: > > - the hypervisor node in device tree > - the xenstore event channel > > The xenstore event channel is also used for the first notification to > let the guest know that xenstore has become available. > > Signed-off-by: Luca Miccio <lucmiccio@gmail.com> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> > CC: Julien Grall <julien@xen.org> > CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > CC: Bertrand Marquis <bertrand.marquis@arm.com> > CC: jbeulich@suse.com > > --- > Changes in v5: > - merge with "xen: export evtchn_alloc_unbound" > - __must_check > > Changes in v3: > - use evtchn_alloc_unbound > > Changes in v2: > - set HVM_PARAM_STORE_PFN to ~0ULL at domain creation > - in alloc_xenstore_evtchn do not call _evtchn_alloc_unbound > > xen: export evtchn_alloc_unbound > > It will be used during dom0less domains construction. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > --- > xen/arch/arm/domain_build.c | 37 +++++++++++++++++++++++++++++++++++++ > xen/common/event_channel.c | 2 +- > xen/include/xen/event.h | 3 +++ > 3 files changed, 41 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 016f56a99f..bb430f2189 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -27,6 +27,7 @@ > #include <asm/setup.h> > #include <asm/cpufeature.h> > #include <asm/domain_build.h> > +#include <xen/event.h> > > #include <xen/irq.h> > #include <xen/grant_table.h> > @@ -2810,6 +2811,8 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo) > int ret; > > kinfo->phandle_gic = GUEST_PHANDLE_GIC; > + kinfo->gnttab_start = GUEST_GNTTAB_BASE; > + kinfo->gnttab_size = GUEST_GNTTAB_SIZE; > > addrcells = GUEST_ROOT_ADDRESS_CELLS; > sizecells = GUEST_ROOT_SIZE_CELLS; > @@ -2884,6 +2887,13 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo) > goto err; > } > > + if ( kinfo->dom0less_enhanced ) > + { > + ret = make_hypervisor_node(d, kinfo, addrcells, sizecells); > + if ( ret ) > + goto err; > + } > + > ret = fdt_end_node(kinfo->fdt); > if ( ret < 0 ) > goto err; > @@ -3150,6 +3160,25 @@ static int __init construct_domain(struct domain *d, struct kernel_info *kinfo) > return 0; > } > > +static int __init alloc_xenstore_evtchn(struct domain *d) > +{ > + evtchn_alloc_unbound_t alloc; > + int rc; > + > + alloc.dom = d->domain_id; > + alloc.remote_dom = hardware_domain->domain_id; I tried to test the patch series with two dom0less domUs without dom0 and oberved the below error. This error is because there is no hardware_domain in that case. (XEN) Data Abort Trap. Syndrome=0x6 (XEN) Walking Hypervisor VA 0x0 on CPU0 via TTBR 0x00000000f91f5000 (XEN) 0TH[0x0] = 0x00000000f91f4f7f (XEN) 1ST[0x0] = 0x00000000f91f1f7f (XEN) 2ND[0x0] = 0x0000000000000000 (XEN) CPU0: Unexpected Trap: Data Abort (XEN) ----[ Xen-4.17-unstable arm64 debug=y Not tainted ]---- (XEN) CPU: 0 (XEN) PC: 00000000002e4180 domain_build.c#construct_domU+0xc90/0xd0c (XEN) LR: 00000000002e4178 (XEN) SP: 000000000031e450 (XEN) CPSR: 0000000060000249 MODE:64-bit EL2h (Hypervisor, handler) (XEN) X0: 0000000000000000 X1: 0000000000000000 X2: 0000000000000000 (XEN) X3: 0000000000000005 X4: 0000000000000000 X5: 0000000000000028 (XEN) X6: 0000000000000080 X7: fefefefefefeff09 X8: 7f7f7f7f7f7f7f7f (XEN) X9: ff6f606c2c68726c X10: 7f7f7f7f7f7f7f7f X11: 0101010101010101 (XEN) X12: 0000000000000008 X13: 00000000002ca000 X14: 00000000002cb000 (XEN) X15: 6db6db6db6db6db7 X16: fffffff800000000 X17: 0000000000000001 (XEN) X18: 0180000000000000 X19: 0000800763a34000 X20: 0000000000000000 (XEN) X21: 000000000031e4c0 X22: 000000000031e4d0 X23: 0000000000000003 (XEN) X24: 000000000031fdfc X25: 0000008400000000 X26: 0000000000000021 (XEN) X27: 0000000000300d08 X28: 00000000003fe97f FP: 000000000031e450 (XEN) (XEN) VTCR_EL2: 00000000800d3590 (XEN) VTTBR_EL2: 00000083e3a67000 (XEN) (XEN) SCTLR_EL2: 0000000030cd183d (XEN) HCR_EL2: 0000000080000039 (XEN) TTBR0_EL2: 00000000f91f5000 (XEN) (XEN) ESR_EL2: 0000000096000006 (XEN) HPFAR_EL2: 0000000000000000 (XEN) FAR_EL2: 0000000000000000 (XEN) (XEN) Xen stack trace from sp=000000000031e450: (XEN) 000000000031fda0 00000000002e5484 0000800763ff2390 00000000002f1ae0 (XEN) 00000000002c98c8 000000000031fde0 0000000000000003 000000000031fdfc (XEN) 0000008400000000 0000000000000021 0000000000300d08 00000000003fe97f (XEN) 00c2010000000000 000000000031e4e0 0000000000000000 00000000040f0000 (XEN) 0000002200000001 0010000000000000 0000020300000000 0000000100000000 (XEN) 00000000000c0000 0000000000000000 0000000000000001 0000800763a34000 (XEN) 0000800763a0c000 0000000000000000 0000000000000001 00000000a0000000 (XEN) 0000000030000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) 0000000000000000 0000000000000000 0000000000000000 0000000000000000 (XEN) Xen call trace: (XEN) [<00000000002e4180>] domain_build.c#construct_domU+0xc90/0xd0c (PC) (XEN) [<00000000002e4178>] domain_build.c#construct_domU+0xc88/0xd0c (LR) (XEN) [<00000000002e5484>] create_domUs+0xb4/0x1e8 (XEN) [<00000000002e999c>] start_xen+0xaf0/0xbe8 (XEN) [<00000000002001a0>] arm64/head.o#primary_switched+0xc/0x1c (XEN) (XEN) (XEN) **************************************** (XEN) Panic on CPU 0: (XEN) CPU0: Unexpected Trap: Data Abort (XEN) **************************************** > + rc = evtchn_alloc_unbound(&alloc); > + if ( rc ) > + { > + printk("Failed allocating event channel for domain\n"); > + return rc; > + } > + > + d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = alloc.port; > + > + return 0; > +} > + > static int __init construct_domU(struct domain *d, > const struct dt_device_node *node) > { > @@ -3214,6 +3243,14 @@ static int __init construct_domU(struct domain *d, > if ( rc < 0 ) > return rc; > > + if ( kinfo.dom0less_enhanced ) I think we need to do something like this to fix the error. if ( hardware_domain && kinfo.dom0less_enhanced ) { } > + { > + rc = alloc_xenstore_evtchn(d); > + if ( rc < 0 ) > + return rc; > + d->arch.hvm.params[HVM_PARAM_STORE_PFN] = ~0ULL; > + } > + > return rc; > } Regards, Rahul
Hi Rahul, On 10/05/2022 17:30, Rahul Singh wrote: >> + rc = evtchn_alloc_unbound(&alloc); >> + if ( rc ) >> + { >> + printk("Failed allocating event channel for domain\n"); >> + return rc; >> + } >> + >> + d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = alloc.port; >> + >> + return 0; >> +} >> + >> static int __init construct_domU(struct domain *d, >> const struct dt_device_node *node) >> { >> @@ -3214,6 +3243,14 @@ static int __init construct_domU(struct domain *d, >> if ( rc < 0 ) >> return rc; >> >> + if ( kinfo.dom0less_enhanced ) > > I think we need to do something like this to fix the error. > if ( hardware_domain && kinfo.dom0less_enhanced ) > { > > } Is there any use case to use "dom0less_enhanced" without dom0 (or a domain servicing Xenstored)? If not, then I would consider to forbid this case and return an error. Cheers,
Hi Julien, > On 10 May 2022, at 17:35, Julien Grall <julien@xen.org> wrote: > > Hi Rahul, > > On 10/05/2022 17:30, Rahul Singh wrote: >>> + rc = evtchn_alloc_unbound(&alloc); >>> + if ( rc ) >>> + { >>> + printk("Failed allocating event channel for domain\n"); >>> + return rc; >>> + } >>> + >>> + d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = alloc.port; >>> + >>> + return 0; >>> +} >>> + >>> static int __init construct_domU(struct domain *d, >>> const struct dt_device_node *node) >>> { >>> @@ -3214,6 +3243,14 @@ static int __init construct_domU(struct domain *d, >>> if ( rc < 0 ) >>> return rc; >>> >>> + if ( kinfo.dom0less_enhanced ) >> I think we need to do something like this to fix the error. >> if ( hardware_domain && kinfo.dom0less_enhanced ) >> { >> } > > Is there any use case to use "dom0less_enhanced" without dom0 (or a domain servicing Xenstored)? > Just being curious here but would it even be possible to have non dom0 domain servicing xenstored ? > If not, then I would consider to forbid this case and return an error. One way or an other we need to solve the crash but if it is forbidden we must prevent coming to this step earlier as it means the configuration is wrong. Cheers Bertrand > > Cheers, > > -- > Julien Grall
Hi Bertrand, On 11/05/2022 08:46, Bertrand Marquis wrote: >> On 10 May 2022, at 17:35, Julien Grall <julien@xen.org> wrote: >> >> Hi Rahul, >> >> On 10/05/2022 17:30, Rahul Singh wrote: >>>> + rc = evtchn_alloc_unbound(&alloc); >>>> + if ( rc ) >>>> + { >>>> + printk("Failed allocating event channel for domain\n"); >>>> + return rc; >>>> + } >>>> + >>>> + d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = alloc.port; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> static int __init construct_domU(struct domain *d, >>>> const struct dt_device_node *node) >>>> { >>>> @@ -3214,6 +3243,14 @@ static int __init construct_domU(struct domain *d, >>>> if ( rc < 0 ) >>>> return rc; >>>> >>>> + if ( kinfo.dom0less_enhanced ) >>> I think we need to do something like this to fix the error. >>> if ( hardware_domain && kinfo.dom0less_enhanced ) >>> { >>> } >> >> Is there any use case to use "dom0less_enhanced" without dom0 (or a domain servicing Xenstored)? >> > > Just being curious here but would it even be possible to have non dom0 domain servicing xenstored ? You can build Xenstored against mini-os and configure the init script to launch xenstored as a domain. > >> If not, then I would consider to forbid this case and return an error. > > One way or an other we need to solve the crash but if it is forbidden we must prevent coming to this step earlier as it means the configuration is wrong. I think this should be checked when parsing the configuration. Cheers,
> On 11 May 2022, at 09:38, Julien Grall <julien@xen.org> wrote: > > Hi Bertrand, > > On 11/05/2022 08:46, Bertrand Marquis wrote: >>> On 10 May 2022, at 17:35, Julien Grall <julien@xen.org> wrote: >>> >>> Hi Rahul, >>> >>> On 10/05/2022 17:30, Rahul Singh wrote: >>>>> + rc = evtchn_alloc_unbound(&alloc); >>>>> + if ( rc ) >>>>> + { >>>>> + printk("Failed allocating event channel for domain\n"); >>>>> + return rc; >>>>> + } >>>>> + >>>>> + d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = alloc.port; >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> static int __init construct_domU(struct domain *d, >>>>> const struct dt_device_node *node) >>>>> { >>>>> @@ -3214,6 +3243,14 @@ static int __init construct_domU(struct domain *d, >>>>> if ( rc < 0 ) >>>>> return rc; >>>>> >>>>> + if ( kinfo.dom0less_enhanced ) >>>> I think we need to do something like this to fix the error. >>>> if ( hardware_domain && kinfo.dom0less_enhanced ) >>>> { >>>> } >>> >>> Is there any use case to use "dom0less_enhanced" without dom0 (or a domain servicing Xenstored)? >>> >> Just being curious here but would it even be possible to have non dom0 domain servicing xenstored ? > > You can build Xenstored against mini-os and configure the init script to launch xenstored as a domain. So dom0 is not mandatory or should mini-os be started as Dom0 for this to work ? > >>> If not, then I would consider to forbid this case and return an error. >> One way or an other we need to solve the crash but if it is forbidden we must prevent coming to this step earlier as it means the configuration is wrong. > > I think this should be checked when parsing the configuration. If dom0 is mandatory yes, we should still make sure that this code cannot be reached so an ASSERT would be nice here at least in case someone tries to activate this code without dom0 (which might happen when we will push the serie for static event channels). Cheers Bertrand > > Cheers, > > -- > Julien Grall
Hi Bertrand, On 11/05/2022 09:46, Bertrand Marquis wrote: > > >> On 11 May 2022, at 09:38, Julien Grall <julien@xen.org> wrote: >> >> Hi Bertrand, >> >> On 11/05/2022 08:46, Bertrand Marquis wrote: >>>> On 10 May 2022, at 17:35, Julien Grall <julien@xen.org> wrote: >>>> >>>> Hi Rahul, >>>> >>>> On 10/05/2022 17:30, Rahul Singh wrote: >>>>>> + rc = evtchn_alloc_unbound(&alloc); >>>>>> + if ( rc ) >>>>>> + { >>>>>> + printk("Failed allocating event channel for domain\n"); >>>>>> + return rc; >>>>>> + } >>>>>> + >>>>>> + d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = alloc.port; >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> static int __init construct_domU(struct domain *d, >>>>>> const struct dt_device_node *node) >>>>>> { >>>>>> @@ -3214,6 +3243,14 @@ static int __init construct_domU(struct domain *d, >>>>>> if ( rc < 0 ) >>>>>> return rc; >>>>>> >>>>>> + if ( kinfo.dom0less_enhanced ) >>>>> I think we need to do something like this to fix the error. >>>>> if ( hardware_domain && kinfo.dom0less_enhanced ) >>>>> { >>>>> } >>>> >>>> Is there any use case to use "dom0less_enhanced" without dom0 (or a domain servicing Xenstored)? >>>> >>> Just being curious here but would it even be possible to have non dom0 domain servicing xenstored ? >> >> You can build Xenstored against mini-os and configure the init script to launch xenstored as a domain. > > So dom0 is not mandatory or should mini-os be started as Dom0 for this to work ? In order to allocate the event channel, you need to know the ID of the domain where Xenstored will run. Stefano's patch is relying on Xenstored to be run in Domain 0. This would need to be updated if we want to run it in a separate domain. > >> >>>> If not, then I would consider to forbid this case and return an error. >>> One way or an other we need to solve the crash but if it is forbidden we must prevent coming to this step earlier as it means the configuration is wrong. >> >> I think this should be checked when parsing the configuration. > > If dom0 is mandatory yes, we should still make sure that this code cannot be reached so an ASSERT would be nice here at least in case someone tries to activate this code without dom0 (which might happen when we will push the serie for static event channels). I am fine with an ASSERT(). Are you saying that dom0less_enhanced will be set to true for the static event channel series? If yes, then I think dom0less_enhanced will need to be an enum so we know what part of Xen is exposed. Cheers,
Hi Julien, > On 11 May 2022, at 10:10, Julien Grall <julien@xen.org> wrote: > > Hi Bertrand, > > On 11/05/2022 09:46, Bertrand Marquis wrote: >>> On 11 May 2022, at 09:38, Julien Grall <julien@xen.org> wrote: >>> >>> Hi Bertrand, >>> >>> On 11/05/2022 08:46, Bertrand Marquis wrote: >>>>> On 10 May 2022, at 17:35, Julien Grall <julien@xen.org> wrote: >>>>> >>>>> Hi Rahul, >>>>> >>>>> On 10/05/2022 17:30, Rahul Singh wrote: >>>>>>> + rc = evtchn_alloc_unbound(&alloc); >>>>>>> + if ( rc ) >>>>>>> + { >>>>>>> + printk("Failed allocating event channel for domain\n"); >>>>>>> + return rc; >>>>>>> + } >>>>>>> + >>>>>>> + d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = alloc.port; >>>>>>> + >>>>>>> + return 0; >>>>>>> +} >>>>>>> + >>>>>>> static int __init construct_domU(struct domain *d, >>>>>>> const struct dt_device_node *node) >>>>>>> { >>>>>>> @@ -3214,6 +3243,14 @@ static int __init construct_domU(struct domain *d, >>>>>>> if ( rc < 0 ) >>>>>>> return rc; >>>>>>> >>>>>>> + if ( kinfo.dom0less_enhanced ) >>>>>> I think we need to do something like this to fix the error. >>>>>> if ( hardware_domain && kinfo.dom0less_enhanced ) >>>>>> { >>>>>> } >>>>> >>>>> Is there any use case to use "dom0less_enhanced" without dom0 (or a domain servicing Xenstored)? >>>>> >>>> Just being curious here but would it even be possible to have non dom0 domain servicing xenstored ? >>> >>> You can build Xenstored against mini-os and configure the init script to launch xenstored as a domain. >> So dom0 is not mandatory or should mini-os be started as Dom0 for this to work ? > > In order to allocate the event channel, you need to know the ID of the domain where Xenstored will run. Stefano's patch is relying on Xenstored to be run in Domain 0. > > This would need to be updated if we want to run it in a separate domain. Ok then Dom0 is mandatory at the moment, I am ok with that. > >>> >>>>> If not, then I would consider to forbid this case and return an error. >>>> One way or an other we need to solve the crash but if it is forbidden we must prevent coming to this step earlier as it means the configuration is wrong. >>> >>> I think this should be checked when parsing the configuration. >> If dom0 is mandatory yes, we should still make sure that this code cannot be reached so an ASSERT would be nice here at least in case someone tries to activate this code without dom0 (which might happen when we will push the serie for static event channels). > > I am fine with an ASSERT(). > > Are you saying that dom0less_enhanced will be set to true for the static event channel series? > > If yes, then I think dom0less_enhanced will need to be an enum so we know what part of Xen is exposed. No it won’t, we just need some of the changes done but without setting dom0less_enhanced. @Rahul: can you confirm. Cheers Bertrand > > Cheers, > > -- > Julien Grall
Hi Julien > On 11 May 2022, at 10:18 am, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote: > > Hi Julien, > >> On 11 May 2022, at 10:10, Julien Grall <julien@xen.org> wrote: >> >> Hi Bertrand, >> >> On 11/05/2022 09:46, Bertrand Marquis wrote: >>>> On 11 May 2022, at 09:38, Julien Grall <julien@xen.org> wrote: >>>> >>>> Hi Bertrand, >>>> >>>> On 11/05/2022 08:46, Bertrand Marquis wrote: >>>>>> On 10 May 2022, at 17:35, Julien Grall <julien@xen.org> wrote: >>>>>> >>>>>> Hi Rahul, >>>>>> >>>>>> On 10/05/2022 17:30, Rahul Singh wrote: >>>>>>>> + rc = evtchn_alloc_unbound(&alloc); >>>>>>>> + if ( rc ) >>>>>>>> + { >>>>>>>> + printk("Failed allocating event channel for domain\n"); >>>>>>>> + return rc; >>>>>>>> + } >>>>>>>> + >>>>>>>> + d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = alloc.port; >>>>>>>> + >>>>>>>> + return 0; >>>>>>>> +} >>>>>>>> + >>>>>>>> static int __init construct_domU(struct domain *d, >>>>>>>> const struct dt_device_node *node) >>>>>>>> { >>>>>>>> @@ -3214,6 +3243,14 @@ static int __init construct_domU(struct domain *d, >>>>>>>> if ( rc < 0 ) >>>>>>>> return rc; >>>>>>>> >>>>>>>> + if ( kinfo.dom0less_enhanced ) >>>>>>> I think we need to do something like this to fix the error. >>>>>>> if ( hardware_domain && kinfo.dom0less_enhanced ) >>>>>>> { >>>>>>> } >>>>>> >>>>>> Is there any use case to use "dom0less_enhanced" without dom0 (or a domain servicing Xenstored)? >>>>>> >>>>> Just being curious here but would it even be possible to have non dom0 domain servicing xenstored ? >>>> >>>> You can build Xenstored against mini-os and configure the init script to launch xenstored as a domain. >>> So dom0 is not mandatory or should mini-os be started as Dom0 for this to work ? >> >> In order to allocate the event channel, you need to know the ID of the domain where Xenstored will run. Stefano's patch is relying on Xenstored to be run in Domain 0. >> >> This would need to be updated if we want to run it in a separate domain. > > Ok then Dom0 is mandatory at the moment, I am ok with that. > >> >>>> >>>>>> If not, then I would consider to forbid this case and return an error. >>>>> One way or an other we need to solve the crash but if it is forbidden we must prevent coming to this step earlier as it means the configuration is wrong. >>>> >>>> I think this should be checked when parsing the configuration. >>> If dom0 is mandatory yes, we should still make sure that this code cannot be reached so an ASSERT would be nice here at least in case someone tries to activate this code without dom0 (which might happen when we will push the serie for static event channels). >> >> I am fine with an ASSERT(). >> >> Are you saying that dom0less_enhanced will be set to true for the static event channel series? >> >> If yes, then I think dom0less_enhanced will need to be an enum so we know what part of Xen is exposed. > > No it won’t, we just need some of the changes done but without setting dom0less_enhanced. > @Rahul: can you confirm. > We need to set the "xen,enhanced” enabled for dom0less domU to enable the event-channel interface in dom0less guest. If we did not set this property we can’t use the event-channel interface in dom0less domUs guests. Regards, Rahul > Cheers > Bertrand > >> >> Cheers, >> >> -- >> Julien Grall
Hi Rahul, On 11/05/2022 11:53, Rahul Singh wrote: >> On 11 May 2022, at 10:18 am, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote: >> >> Hi Julien, >> >>> On 11 May 2022, at 10:10, Julien Grall <julien@xen.org> wrote: >>> >>> Hi Bertrand, >>> >>> On 11/05/2022 09:46, Bertrand Marquis wrote: >>>>> On 11 May 2022, at 09:38, Julien Grall <julien@xen.org> wrote: >>>>> >>>>> Hi Bertrand, >>>>> >>>>> On 11/05/2022 08:46, Bertrand Marquis wrote: >>>>>>> On 10 May 2022, at 17:35, Julien Grall <julien@xen.org> wrote: >>>>>>> >>>>>>> Hi Rahul, >>>>>>> >>>>>>> On 10/05/2022 17:30, Rahul Singh wrote: >>>>>>>>> + rc = evtchn_alloc_unbound(&alloc); >>>>>>>>> + if ( rc ) >>>>>>>>> + { >>>>>>>>> + printk("Failed allocating event channel for domain\n"); >>>>>>>>> + return rc; >>>>>>>>> + } >>>>>>>>> + >>>>>>>>> + d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = alloc.port; >>>>>>>>> + >>>>>>>>> + return 0; >>>>>>>>> +} >>>>>>>>> + >>>>>>>>> static int __init construct_domU(struct domain *d, >>>>>>>>> const struct dt_device_node *node) >>>>>>>>> { >>>>>>>>> @@ -3214,6 +3243,14 @@ static int __init construct_domU(struct domain *d, >>>>>>>>> if ( rc < 0 ) >>>>>>>>> return rc; >>>>>>>>> >>>>>>>>> + if ( kinfo.dom0less_enhanced ) >>>>>>>> I think we need to do something like this to fix the error. >>>>>>>> if ( hardware_domain && kinfo.dom0less_enhanced ) >>>>>>>> { >>>>>>>> } >>>>>>> >>>>>>> Is there any use case to use "dom0less_enhanced" without dom0 (or a domain servicing Xenstored)? >>>>>>> >>>>>> Just being curious here but would it even be possible to have non dom0 domain servicing xenstored ? >>>>> >>>>> You can build Xenstored against mini-os and configure the init script to launch xenstored as a domain. >>>> So dom0 is not mandatory or should mini-os be started as Dom0 for this to work ? >>> >>> In order to allocate the event channel, you need to know the ID of the domain where Xenstored will run. Stefano's patch is relying on Xenstored to be run in Domain 0. >>> >>> This would need to be updated if we want to run it in a separate domain. >> >> Ok then Dom0 is mandatory at the moment, I am ok with that. >> >>> >>>>> >>>>>>> If not, then I would consider to forbid this case and return an error. >>>>>> One way or an other we need to solve the crash but if it is forbidden we must prevent coming to this step earlier as it means the configuration is wrong. >>>>> >>>>> I think this should be checked when parsing the configuration. >>>> If dom0 is mandatory yes, we should still make sure that this code cannot be reached so an ASSERT would be nice here at least in case someone tries to activate this code without dom0 (which might happen when we will push the serie for static event channels). >>> >>> I am fine with an ASSERT(). >>> >>> Are you saying that dom0less_enhanced will be set to true for the static event channel series? >>> >>> If yes, then I think dom0less_enhanced will need to be an enum so we know what part of Xen is exposed. >> >> No it won’t, we just need some of the changes done but without setting dom0less_enhanced. >> @Rahul: can you confirm. >> > > We need to set the "xen,enhanced” enabled for dom0less domU to enable > the event-channel interface in dom0less guest. If we did not set this property we can’t > use the event-channel interface in dom0less domUs guests. Is this because the domU will not know which PPI will be used for notification? The property "xen,enhanced" with an empty string (or with the value "enabled") is meant to indicate that PV drivers will be usable in the domain. AFAIU, you are suggesting to change the meaning based on dom0 whether has been created. I don't particularly like that because a user may spent a while to understand why Xenstored doesn't work. The current proposal for xen,enhanced allows us to define new values if we wanted to only enabled selected interfaces. AFAIU, in your case, you only want to expose the event channel interface, so I would create a new value to indicate that the event channel interface is exposed. Xen would then create only the part for the event channel (i.e. no extended regions, grant tables...). Cheers,
Hi Julien, > On 11 May 2022, at 2:11 pm, Julien Grall <julien@xen.org> wrote: > > Hi Rahul, > > On 11/05/2022 11:53, Rahul Singh wrote: >>> On 11 May 2022, at 10:18 am, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote: >>> >>> Hi Julien, >>> >>>> On 11 May 2022, at 10:10, Julien Grall <julien@xen.org> wrote: >>>> >>>> Hi Bertrand, >>>> >>>> On 11/05/2022 09:46, Bertrand Marquis wrote: >>>>>> On 11 May 2022, at 09:38, Julien Grall <julien@xen.org> wrote: >>>>>> >>>>>> Hi Bertrand, >>>>>> >>>>>> On 11/05/2022 08:46, Bertrand Marquis wrote: >>>>>>>> On 10 May 2022, at 17:35, Julien Grall <julien@xen.org> wrote: >>>>>>>> >>>>>>>> Hi Rahul, >>>>>>>> >>>>>>>> On 10/05/2022 17:30, Rahul Singh wrote: >>>>>>>>>> + rc = evtchn_alloc_unbound(&alloc); >>>>>>>>>> + if ( rc ) >>>>>>>>>> + { >>>>>>>>>> + printk("Failed allocating event channel for domain\n"); >>>>>>>>>> + return rc; >>>>>>>>>> + } >>>>>>>>>> + >>>>>>>>>> + d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = alloc.port; >>>>>>>>>> + >>>>>>>>>> + return 0; >>>>>>>>>> +} >>>>>>>>>> + >>>>>>>>>> static int __init construct_domU(struct domain *d, >>>>>>>>>> const struct dt_device_node *node) >>>>>>>>>> { >>>>>>>>>> @@ -3214,6 +3243,14 @@ static int __init construct_domU(struct domain *d, >>>>>>>>>> if ( rc < 0 ) >>>>>>>>>> return rc; >>>>>>>>>> >>>>>>>>>> + if ( kinfo.dom0less_enhanced ) >>>>>>>>> I think we need to do something like this to fix the error. >>>>>>>>> if ( hardware_domain && kinfo.dom0less_enhanced ) >>>>>>>>> { >>>>>>>>> } >>>>>>>> >>>>>>>> Is there any use case to use "dom0less_enhanced" without dom0 (or a domain servicing Xenstored)? >>>>>>>> >>>>>>> Just being curious here but would it even be possible to have non dom0 domain servicing xenstored ? >>>>>> >>>>>> You can build Xenstored against mini-os and configure the init script to launch xenstored as a domain. >>>>> So dom0 is not mandatory or should mini-os be started as Dom0 for this to work ? >>>> >>>> In order to allocate the event channel, you need to know the ID of the domain where Xenstored will run. Stefano's patch is relying on Xenstored to be run in Domain 0. >>>> >>>> This would need to be updated if we want to run it in a separate domain. >>> >>> Ok then Dom0 is mandatory at the moment, I am ok with that. >>> >>>> >>>>>> >>>>>>>> If not, then I would consider to forbid this case and return an error. >>>>>>> One way or an other we need to solve the crash but if it is forbidden we must prevent coming to this step earlier as it means the configuration is wrong. >>>>>> >>>>>> I think this should be checked when parsing the configuration. >>>>> If dom0 is mandatory yes, we should still make sure that this code cannot be reached so an ASSERT would be nice here at least in case someone tries to activate this code without dom0 (which might happen when we will push the serie for static event channels). >>>> >>>> I am fine with an ASSERT(). >>>> >>>> Are you saying that dom0less_enhanced will be set to true for the static event channel series? >>>> >>>> If yes, then I think dom0less_enhanced will need to be an enum so we know what part of Xen is exposed. >>> >>> No it won’t, we just need some of the changes done but without setting dom0less_enhanced. >>> @Rahul: can you confirm. >>> >> We need to set the "xen,enhanced” enabled for dom0less domU to enable >> the event-channel interface in dom0less guest. If we did not set this property we can’t >> use the event-channel interface in dom0less domUs guests. > > Is this because the domU will not know which PPI will be used for notification? Yes you are right if we don’t use "xen,enhanced” for domUs, domUs will not know which PPI will be used. Also if we don’t use "xen,enhanced” there is no hypervisor node created for Linux and if there is no hypervisor node that means no xen support detected. > > The property "xen,enhanced" with an empty string (or with the value "enabled") is meant to indicate that PV drivers will be usable in the domain. > > AFAIU, you are suggesting to change the meaning based on dom0 whether has been created. I don't particularly like that because a user may spent a while to understand why Xenstored doesn't work. > > The current proposal for xen,enhanced allows us to define new values if we wanted to only enabled selected interfaces. AFAIU, in your case, you only want to expose the event channel interface, so I would create a new value to indicate that the event channel interface is exposed. Xen would then create only the part for the event channel (i.e. no extended regions, grant tables...). Ok. I will create the new property something like “xen,evtchn” to enable the event-channel for dom0less guests. Based on “xen,evtchn” property I will create the hypervisor node with only PPI interrupt property. Regards, Rahul
Hi Julien, > On 11 May 2022, at 3:57 pm, Rahul Singh <Rahul.Singh@arm.com> wrote: > > Hi Julien, > >> On 11 May 2022, at 2:11 pm, Julien Grall <julien@xen.org> wrote: >> >> Hi Rahul, >> >> On 11/05/2022 11:53, Rahul Singh wrote: >>>> On 11 May 2022, at 10:18 am, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote: >>>> >>>> Hi Julien, >>>> >>>>> On 11 May 2022, at 10:10, Julien Grall <julien@xen.org> wrote: >>>>> >>>>> Hi Bertrand, >>>>> >>>>> On 11/05/2022 09:46, Bertrand Marquis wrote: >>>>>>> On 11 May 2022, at 09:38, Julien Grall <julien@xen.org> wrote: >>>>>>> >>>>>>> Hi Bertrand, >>>>>>> >>>>>>> On 11/05/2022 08:46, Bertrand Marquis wrote: >>>>>>>>> On 10 May 2022, at 17:35, Julien Grall <julien@xen.org> wrote: >>>>>>>>> >>>>>>>>> Hi Rahul, >>>>>>>>> >>>>>>>>> On 10/05/2022 17:30, Rahul Singh wrote: >>>>>>>>>>> + rc = evtchn_alloc_unbound(&alloc); >>>>>>>>>>> + if ( rc ) >>>>>>>>>>> + { >>>>>>>>>>> + printk("Failed allocating event channel for domain\n"); >>>>>>>>>>> + return rc; >>>>>>>>>>> + } >>>>>>>>>>> + >>>>>>>>>>> + d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = alloc.port; >>>>>>>>>>> + >>>>>>>>>>> + return 0; >>>>>>>>>>> +} >>>>>>>>>>> + >>>>>>>>>>> static int __init construct_domU(struct domain *d, >>>>>>>>>>> const struct dt_device_node *node) >>>>>>>>>>> { >>>>>>>>>>> @@ -3214,6 +3243,14 @@ static int __init construct_domU(struct domain *d, >>>>>>>>>>> if ( rc < 0 ) >>>>>>>>>>> return rc; >>>>>>>>>>> >>>>>>>>>>> + if ( kinfo.dom0less_enhanced ) >>>>>>>>>> I think we need to do something like this to fix the error. >>>>>>>>>> if ( hardware_domain && kinfo.dom0less_enhanced ) >>>>>>>>>> { >>>>>>>>>> } >>>>>>>>> >>>>>>>>> Is there any use case to use "dom0less_enhanced" without dom0 (or a domain servicing Xenstored)? >>>>>>>>> >>>>>>>> Just being curious here but would it even be possible to have non dom0 domain servicing xenstored ? >>>>>>> >>>>>>> You can build Xenstored against mini-os and configure the init script to launch xenstored as a domain. >>>>>> So dom0 is not mandatory or should mini-os be started as Dom0 for this to work ? >>>>> >>>>> In order to allocate the event channel, you need to know the ID of the domain where Xenstored will run. Stefano's patch is relying on Xenstored to be run in Domain 0. >>>>> >>>>> This would need to be updated if we want to run it in a separate domain. >>>> >>>> Ok then Dom0 is mandatory at the moment, I am ok with that. >>>> >>>>> >>>>>>> >>>>>>>>> If not, then I would consider to forbid this case and return an error. >>>>>>>> One way or an other we need to solve the crash but if it is forbidden we must prevent coming to this step earlier as it means the configuration is wrong. >>>>>>> >>>>>>> I think this should be checked when parsing the configuration. >>>>>> If dom0 is mandatory yes, we should still make sure that this code cannot be reached so an ASSERT would be nice here at least in case someone tries to activate this code without dom0 (which might happen when we will push the serie for static event channels). >>>>> >>>>> I am fine with an ASSERT(). >>>>> >>>>> Are you saying that dom0less_enhanced will be set to true for the static event channel series? >>>>> >>>>> If yes, then I think dom0less_enhanced will need to be an enum so we know what part of Xen is exposed. >>>> >>>> No it won’t, we just need some of the changes done but without setting dom0less_enhanced. >>>> @Rahul: can you confirm. >>>> >>> We need to set the "xen,enhanced” enabled for dom0less domU to enable >>> the event-channel interface in dom0less guest. If we did not set this property we can’t >>> use the event-channel interface in dom0less domUs guests. >> >> Is this because the domU will not know which PPI will be used for notification? > > Yes you are right if we don’t use "xen,enhanced” for domUs, domUs will not know which PPI will be used. > Also if we don’t use "xen,enhanced” there is no hypervisor node created for Linux and if there is > no hypervisor node that means no xen support detected. > >> >> The property "xen,enhanced" with an empty string (or with the value "enabled") is meant to indicate that PV drivers will be usable in the domain. >> >> AFAIU, you are suggesting to change the meaning based on dom0 whether has been created. I don't particularly like that because a user may spent a while to understand why Xenstored doesn't work. >> >> The current proposal for xen,enhanced allows us to define new values if we wanted to only enabled selected interfaces. AFAIU, in your case, you only want to expose the event channel interface, so I would create a new value to indicate that the event channel interface is exposed. Xen would then create only the part for the event channel (i.e. no extended regions, grant tables...). > > Ok. I will create the new property something like “xen,evtchn” to enable the event-channel for dom0less guests. > Based on “xen,evtchn” property I will create the hypervisor node with only PPI interrupt property. If we don’t want to create the new property we can use "xen,enhanced = evtchn” to enable the event-channel interfacefor dom0less guests. Regards, Rahul
On 11/05/2022 16:05, Rahul Singh wrote: >>> The property "xen,enhanced" with an empty string (or with the value "enabled") is meant to indicate that PV drivers will be usable in the domain. >>> >>> AFAIU, you are suggesting to change the meaning based on dom0 whether has been created. I don't particularly like that because a user may spent a while to understand why Xenstored doesn't work. >>> >>> The current proposal for xen,enhanced allows us to define new values if we wanted to only enabled selected interfaces. AFAIU, in your case, you only want to expose the event channel interface, so I would create a new value to indicate that the event channel interface is exposed. Xen would then create only the part for the event channel (i.e. no extended regions, grant tables...). >> >> Ok. I will create the new property something like “xen,evtchn” to enable the event-channel for dom0less guests. >> Based on “xen,evtchn” property I will create the hypervisor node with only PPI interrupt property. > > If we don’t want to create the new property we can use "xen,enhanced = evtchn” to > enable the event-channel interfacefor dom0less guests. I would prefer the "xen,enhanced = evtchn" because to avoid corner cases such as xen,enhanced without xen,evtchn Cheers,
Hi Stefano, On 05/05/2022 01:16, Stefano Stabellini wrote: > From: Luca Miccio <lucmiccio@gmail.com> > > Export evtchn_alloc_unbound and make it __must_check. > > If "xen,enhanced" is enabled, then add to dom0less domains: > > - the hypervisor node in device tree > - the xenstore event channel > > The xenstore event channel is also used for the first notification to > let the guest know that xenstore has become available. > > Signed-off-by: Luca Miccio <lucmiccio@gmail.com> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> > CC: Julien Grall <julien@xen.org> > CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > CC: Bertrand Marquis <bertrand.marquis@arm.com> > CC: jbeulich@suse.com > > --- > Changes in v5: > - merge with "xen: export evtchn_alloc_unbound" > - __must_check > > Changes in v3: > - use evtchn_alloc_unbound > > Changes in v2: > - set HVM_PARAM_STORE_PFN to ~0ULL at domain creation > - in alloc_xenstore_evtchn do not call _evtchn_alloc_unbound > > xen: export evtchn_alloc_unbound > > It will be used during dom0less domains construction. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > --- > xen/arch/arm/domain_build.c | 37 +++++++++++++++++++++++++++++++++++++ > xen/common/event_channel.c | 2 +- > xen/include/xen/event.h | 3 +++ > 3 files changed, 41 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 016f56a99f..bb430f2189 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -27,6 +27,7 @@ > #include <asm/setup.h> > #include <asm/cpufeature.h> > #include <asm/domain_build.h> > +#include <xen/event.h> > > #include <xen/irq.h> > #include <xen/grant_table.h> > @@ -2810,6 +2811,8 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo) > int ret; > > kinfo->phandle_gic = GUEST_PHANDLE_GIC; > + kinfo->gnttab_start = GUEST_GNTTAB_BASE; > + kinfo->gnttab_size = GUEST_GNTTAB_SIZE; > > addrcells = GUEST_ROOT_ADDRESS_CELLS; > sizecells = GUEST_ROOT_SIZE_CELLS; > @@ -2884,6 +2887,13 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo) > goto err; > } > > + if ( kinfo->dom0less_enhanced ) > + { > + ret = make_hypervisor_node(d, kinfo, addrcells, sizecells); > + if ( ret ) > + goto err; > + } > + > ret = fdt_end_node(kinfo->fdt); > if ( ret < 0 ) > goto err; > @@ -3150,6 +3160,25 @@ static int __init construct_domain(struct domain *d, struct kernel_info *kinfo) > return 0; > } > > +static int __init alloc_xenstore_evtchn(struct domain *d) > +{ > + evtchn_alloc_unbound_t alloc; > + int rc; > + > + alloc.dom = d->domain_id; > + alloc.remote_dom = hardware_domain->domain_id; > + rc = evtchn_alloc_unbound(&alloc); > + if ( rc ) > + { > + printk("Failed allocating event channel for domain\n"); > + return rc; > + } > + > + d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = alloc.port; > + > + return 0; > +} > + > static int __init construct_domU(struct domain *d, > const struct dt_device_node *node) > { > @@ -3214,6 +3243,14 @@ static int __init construct_domU(struct domain *d, > if ( rc < 0 ) > return rc; > > + if ( kinfo.dom0less_enhanced ) > + { > + rc = alloc_xenstore_evtchn(d); > + if ( rc < 0 ) > + return rc; > + d->arch.hvm.params[HVM_PARAM_STORE_PFN] = ~0ULL; This sounds a little bit odd that we set the value to ~0ULL with dom0less_enhanced but keep it to 0 outside of dom0less. If there are any rationale for that, then I would suggest to add a comment. (Can be done on commit). For the common part: Acked-by: Julien Grall <jgrall@amazon.com> # common Cheers,
On Wed, 11 May 2022, Julien Grall wrote: > > If dom0 is mandatory yes, we should still make sure that this code cannot be > > reached so an ASSERT would be nice here at least in case someone tries to > > activate this code without dom0 (which might happen when we will push the > > serie for static event channels). > > I am fine with an ASSERT(). I added an ASSERT(hardware_domain).
Hi Stefano, On 13/05/2022 02:23, Stefano Stabellini wrote: > On Wed, 11 May 2022, Julien Grall wrote: >>> If dom0 is mandatory yes, we should still make sure that this code cannot be >>> reached so an ASSERT would be nice here at least in case someone tries to >>> activate this code without dom0 (which might happen when we will push the >>> serie for static event channels). >> >> I am fine with an ASSERT(). > > I added an ASSERT(hardware_domain). Just to clarify and avoid a round trip. The ASSERT() is not sufficient here. We also need to forbid the user to set xen,enhanced when the HW domain is not NULL, at least until Rahul's pre-allocated event channel series. This check would have to be done when parsing the configuration. The ASSERT() would be here to ensure that if someone is reworking the parsing, it would get caught nicely rather than a NULL dereference. Cheers,
On Fri, 13 May 2022, Julien Grall wrote: > On 13/05/2022 02:23, Stefano Stabellini wrote: > > On Wed, 11 May 2022, Julien Grall wrote: > > > > If dom0 is mandatory yes, we should still make sure that this code > > > > cannot be > > > > reached so an ASSERT would be nice here at least in case someone tries > > > > to > > > > activate this code without dom0 (which might happen when we will push > > > > the > > > > serie for static event channels). > > > > > > I am fine with an ASSERT(). > > > > I added an ASSERT(hardware_domain). > > Just to clarify and avoid a round trip. The ASSERT() is not sufficient here. > We also need to forbid the user to set xen,enhanced when the HW domain is not > NULL, at least until Rahul's pre-allocated event channel series. > > This check would have to be done when parsing the configuration. The ASSERT() > would be here to ensure that if someone is reworking the parsing, it would get > caught nicely rather than a NULL dereference. Thanks for avoiding a roundtrip. I added a check when parsing device tree if xen,enhanced is specified but dom0 is missing. Initially I wrote it as a "panic" but then I changed it as a regular printk. I am OK either way in case you prefer otherwise.
Hi Stefano, On 13/05/2022 21:52, Stefano Stabellini wrote: > On Fri, 13 May 2022, Julien Grall wrote: >> On 13/05/2022 02:23, Stefano Stabellini wrote: >>> On Wed, 11 May 2022, Julien Grall wrote: >>>>> If dom0 is mandatory yes, we should still make sure that this code >>>>> cannot be >>>>> reached so an ASSERT would be nice here at least in case someone tries >>>>> to >>>>> activate this code without dom0 (which might happen when we will push >>>>> the >>>>> serie for static event channels). >>>> >>>> I am fine with an ASSERT(). >>> >>> I added an ASSERT(hardware_domain). >> >> Just to clarify and avoid a round trip. The ASSERT() is not sufficient here. >> We also need to forbid the user to set xen,enhanced when the HW domain is not >> NULL, at least until Rahul's pre-allocated event channel series. >> >> This check would have to be done when parsing the configuration. The ASSERT() >> would be here to ensure that if someone is reworking the parsing, it would get >> caught nicely rather than a NULL dereference. > > Thanks for avoiding a roundtrip. I added a check when parsing device > tree if xen,enhanced is specified but dom0 is missing. Initially I wrote > it as a "panic" but then I changed it as a regular printk. I am OK > either way in case you prefer otherwise. This is a configuration issue from the user. So I think we shouldn't continue (i.e panic()) if we can't honor what the user requested. This make the problem a lot more obvious to the user (printk() can be easily overlooked). Cheers,
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 016f56a99f..bb430f2189 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -27,6 +27,7 @@ #include <asm/setup.h> #include <asm/cpufeature.h> #include <asm/domain_build.h> +#include <xen/event.h> #include <xen/irq.h> #include <xen/grant_table.h> @@ -2810,6 +2811,8 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo) int ret; kinfo->phandle_gic = GUEST_PHANDLE_GIC; + kinfo->gnttab_start = GUEST_GNTTAB_BASE; + kinfo->gnttab_size = GUEST_GNTTAB_SIZE; addrcells = GUEST_ROOT_ADDRESS_CELLS; sizecells = GUEST_ROOT_SIZE_CELLS; @@ -2884,6 +2887,13 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo) goto err; } + if ( kinfo->dom0less_enhanced ) + { + ret = make_hypervisor_node(d, kinfo, addrcells, sizecells); + if ( ret ) + goto err; + } + ret = fdt_end_node(kinfo->fdt); if ( ret < 0 ) goto err; @@ -3150,6 +3160,25 @@ static int __init construct_domain(struct domain *d, struct kernel_info *kinfo) return 0; } +static int __init alloc_xenstore_evtchn(struct domain *d) +{ + evtchn_alloc_unbound_t alloc; + int rc; + + alloc.dom = d->domain_id; + alloc.remote_dom = hardware_domain->domain_id; + rc = evtchn_alloc_unbound(&alloc); + if ( rc ) + { + printk("Failed allocating event channel for domain\n"); + return rc; + } + + d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = alloc.port; + + return 0; +} + static int __init construct_domU(struct domain *d, const struct dt_device_node *node) { @@ -3214,6 +3243,14 @@ static int __init construct_domU(struct domain *d, if ( rc < 0 ) return rc; + if ( kinfo.dom0less_enhanced ) + { + rc = alloc_xenstore_evtchn(d); + if ( rc < 0 ) + return rc; + d->arch.hvm.params[HVM_PARAM_STORE_PFN] = ~0ULL; + } + return rc; } diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c index 0a82eb3ac2..e60cd98d75 100644 --- a/xen/common/event_channel.c +++ b/xen/common/event_channel.c @@ -290,7 +290,7 @@ void evtchn_free(struct domain *d, struct evtchn *chn) xsm_evtchn_close_post(chn); } -static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc) +int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc) { struct evtchn *chn; struct domain *d; diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h index 21c95e14fd..f3021fe304 100644 --- a/xen/include/xen/event.h +++ b/xen/include/xen/event.h @@ -71,6 +71,9 @@ void evtchn_free(struct domain *d, struct evtchn *chn); /* Allocate a specific event channel port. */ int evtchn_allocate_port(struct domain *d, unsigned int port); +/* Allocate a new event channel */ +int __must_check evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc); + /* Unmask a local event-channel port. */ int evtchn_unmask(unsigned int port);