On Tue, 6 May 2025, Jason Andryuk wrote:
> On 2025-05-05 16:44, Stefano Stabellini wrote:
> > On Sat, 3 May 2025, Jason Andryuk wrote:
> > > Make xenbus_init() allow a non-local xenstore for a PVH dom0 - it is
> > > currently forced to XS_LOCAL. With Hyperlaunch booting dom0 and a
> > > xenstore stubdom, dom0 can be handled as a regular XS_HVM following the
> > > late init path.
> > >
> > > Drop the use of xen_initial_domain() and just check for the event
> > > channel instead. This matches the PV case where there is no check for
> > > initial domain.
> > >
> > > Check the full 64bit HVM_PARAM_STORE_EVTCHN value to catch the off
> > > chance that high bits are set for the 32bit event channel.
> > >
> > > Signed-off-by: Jason Andryuk <jason.andr...@amd.com>
> >
> > Reviewed-by: Stefano Stabellini <sstabell...@kernel.org>
>
> Thanks, Stefano. But I'm wondering if this might break ARM enhanced
> no-xenstore.
>
> >
> > > ---
> > > drivers/xen/xenbus/xenbus_probe.c | 14 ++++++++------
> > > 1 file changed, 8 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/xen/xenbus/xenbus_probe.c
> > > b/drivers/xen/xenbus/xenbus_probe.c
> > > index 6d32ffb01136..7604f70ee108 100644
> > > --- a/drivers/xen/xenbus/xenbus_probe.c
> > > +++ b/drivers/xen/xenbus/xenbus_probe.c
> > > @@ -966,9 +966,15 @@ static int __init xenbus_init(void)
> > > if (xen_pv_domain())
> > > xen_store_domain_type = XS_PV;
> > > if (xen_hvm_domain())
> > > + {
> > > xen_store_domain_type = XS_HVM;
>
> ARM would have everything set to XS_HVM...
>
> > > - if (xen_hvm_domain() && xen_initial_domain())
> > > - xen_store_domain_type = XS_LOCAL;
>
> ...and only dom0 set to XS_LOCAL.
>
> > > + err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v);
> > > + if (err)
> > > + goto out_error;
> > > + xen_store_evtchn = (int)v;
> > > + if (!v)
> > > + xen_store_domain_type = XS_LOCAL;
> > > + }
> > > if (xen_pv_domain() && !xen_start_info->store_evtchn)
> > > xen_store_domain_type = XS_LOCAL;
> > > if (xen_pv_domain() && xen_start_info->store_evtchn)
> > > @@ -987,10 +993,6 @@ static int __init xenbus_init(void)
> > > xen_store_interface = gfn_to_virt(xen_store_gfn);
> > > break;
> > > case XS_HVM:
> > > - err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v);
> > > - if (err)
> > > - goto out_error;
> > > - xen_store_evtchn = (int)v;
> > > err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v);
> > > if (err)
> > > goto out_error;
> /*
> * Uninitialized hvm_params are zero and return no error.
> * Although it is theoretically possible to have
> * HVM_PARAM_STORE_PFN set to zero on purpose, in reality it
> is
> * not zero when valid. If zero, it means that Xenstore hasn't
> * been properly initialized. Instead of attempting to map a
> * wrong guest physical address return error.
> *
> * Also recognize all bits set as an invalid/uninitialized
> value.
> */
> if (!v) {
> err = -ENOENT;
> goto out_error;
> }
>
> IIUC, this !v check is for enhanced no-xenstore to end up in XS_UNKNOWN. I'll
> have to re-work to handle that case.
I was wondering about that when reviewing this patch, because the
removal of this check was the main change introduced I wasn't sure
about. Looking around I (wrongly) convinced myself that removing the
check was OK. Thank you for spotting this on your own.
