lkml.org 
[lkml]   [2024]   [May]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] drivers/xen: Improve the late XenStore init protocol
On Thu, 16 May 2024, Henry Wang wrote:
> Hi Stefano,
>
> On 5/16/2024 6:30 AM, Stefano Stabellini wrote:
> > On Wed, 15 May 2024, Henry Wang wrote:
> > > Currently, the late XenStore init protocol is only triggered properly
> > > for the case that HVM_PARAM_STORE_PFN is ~0ULL (invalid). For the
> > > case that XenStore interface is allocated but not ready (the connection
> > > status is not XENSTORE_CONNECTED), Linux should also wait until the
> > > XenStore is set up properly.
> > >
> > > Introduce a macro to describe the XenStore interface is ready, use
> > > it in xenbus_probe_initcall() and xenbus_probe() to select the code
> > > path of doing the late XenStore init protocol or not.
> > >
> > > Take the opportunity to enhance the check of the allocated XenStore
> > > interface can be properly mapped, and return error early if the
> > > memremap() fails.
> > >
> > > Signed-off-by: Henry Wang <xin.wang2@amd.com>
> > > Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> > Please add a Fixes: tag
>
> Sure. Will do.
>
> > > ---
> > > drivers/xen/xenbus/xenbus_probe.c | 21 ++++++++++++++++-----
> > > 1 file changed, 16 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/xen/xenbus/xenbus_probe.c
> > > b/drivers/xen/xenbus/xenbus_probe.c
> > > index 3205e5d724c8..8aec0ed1d047 100644
> > > --- a/drivers/xen/xenbus/xenbus_probe.c
> > > +++ b/drivers/xen/xenbus/xenbus_probe.c
> > > @@ -72,6 +72,10 @@ EXPORT_SYMBOL_GPL(xen_store_evtchn);
> > > struct xenstore_domain_interface *xen_store_interface;
> > > EXPORT_SYMBOL_GPL(xen_store_interface);
> > > +#define XS_INTERFACE_READY \
> > > + ((xen_store_interface != NULL) && \
> > > + (xen_store_interface->connection == XENSTORE_CONNECTED))
> > > +
> > > enum xenstore_init xen_store_domain_type;
> > > EXPORT_SYMBOL_GPL(xen_store_domain_type);
> > > @@ -751,9 +755,10 @@ static void xenbus_probe(void)
> > > {
> > > xenstored_ready = 1;
> > > - if (!xen_store_interface) {
> > > - xen_store_interface = memremap(xen_store_gfn <<
> > > XEN_PAGE_SHIFT,
> > > - XEN_PAGE_SIZE, MEMREMAP_WB);
> > > + if (!xen_store_interface || XS_INTERFACE_READY) {
> > > + if (!xen_store_interface)
> > These two nested if's don't make sense to me. If XS_INTERFACE_READY
> > succeeds, it means that ((xen_store_interface != NULL) &&
> > (xen_store_interface->connection == XENSTORE_CONNECTED)).
> >
> > So it is not possible that xen_store_interface == NULL immediately
> > after. Right?
>
> I think this is because we want to free the irq for the late init case,
> otherwise the init-dom0less will fail. For the xenstore PFN allocated case,
> the connection is already set to CONNECTED when we execute init-dom0less. But
> I agree with you, would below diff makes more sense to you?
>
> diff --git a/drivers/xen/xenbus/xenbus_probe.c
> b/drivers/xen/xenbus/xenbus_probe.c
> index 8aec0ed1d047..b8005b651a29 100644
> --- a/drivers/xen/xenbus/xenbus_probe.c
> +++ b/drivers/xen/xenbus/xenbus_probe.c
> @@ -76,6 +76,8 @@ EXPORT_SYMBOL_GPL(xen_store_interface);
>         ((xen_store_interface != NULL) && \
>          (xen_store_interface->connection == XENSTORE_CONNECTED))
>
> +static bool xs_late_init = false;
> +
>  enum xenstore_init xen_store_domain_type;
>  EXPORT_SYMBOL_GPL(xen_store_domain_type);
>
> @@ -755,7 +757,7 @@ static void xenbus_probe(void)
>  {
>         xenstored_ready = 1;
>
> -       if (!xen_store_interface || XS_INTERFACE_READY) {
> +       if (xs_late_init) {
>                 if (!xen_store_interface)
>                         xen_store_interface = memremap(xen_store_gfn <<


I would just remove the outer 'if' and do this:


if (!xen_store_interface)
xen_store_interface = memremap(xen_store_gfn << XEN_PAGE_SHIFT,
XEN_PAGE_SIZE, MEMREMAP_WB);
/*
* Now it is safe to free the IRQ used for xenstore late
* initialization. No need to unbind: it is about to be
* bound again from xb_init_comms. Note that calling
* unbind_from_irqhandler now would result in xen_evtchn_close()
* being called and the event channel not being enabled again
* afterwards, resulting in missed event notifications.
*/
if (xs_init_irq > 0)
free_irq(xs_init_irq, &xb_waitq);


I think this should work fine in all cases. I am unsure if
xs_init_irq==0 is possible valid value for xs_init_irq. If it is not,
then we are fine. If 0 is a possible valid irq number, then we should
initialize xs_init_irq to -1, and here check for xs_init_irq >= 0.
\
 
 \ /
  Last update: 2024-05-27 18:30    [W:0.093 / U:0.136 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site