Received-SPF: pass (google.com: domain of linux-kernel+bounces-181662-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 16 May 2024 17:52:51 -0700 (PDT)
From: Stefano Stabellini <sstabellini@kernel.org>
To: Henry Wang <xin.wang2@amd.com>
cc: Stefano Stabellini <sstabellini@kernel.org>, 
    xen-devel@lists.xenproject.org, linux-kernel@vger.kernel.org, 
    Juergen Gross <jgross@suse.com>, 
    Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>, 
    Michal Orzel <michal.orzel@amd.com>
Subject: Re: [PATCH] drivers/xen: Improve the late XenStore init protocol
In-Reply-To: <028f29be-0393-4a57-83e2-ea27fe0320d5@amd.com>
Message-ID: <alpine.DEB.2.22.394.2405161743170.2544314@ubuntu-linux-20-04-desktop>
References: <20240515014330.1044617-1-xin.wang2@amd.com> <alpine.DEB.2.22.394.2405151524270.2544314@ubuntu-linux-20-04-desktop> <028f29be-0393-4a57-83e2-ea27fe0320d5@amd.com>
User-Agent: Alpine 2.22 (DEB 394 2020-01-19)
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="8323329-217339045-1715907173=:2544314"

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323329-217339045-1715907173=:2544314
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT

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.
--8323329-217339045-1715907173=:2544314--