Re: [RFC PATCH 13/16] drivers/xen: gnttab, evtchn, xenbus API changes

[Ankur Arora]

On 6/17/19 3:07 AM, Juergen Gross wrote:
> On 09.05.19 19:25, Ankur Arora wrote:
> > Mechanical changes, now most of these calls take xenhost_t *
> > as parameter.
> >
> > Co-developed-by: Joao Martins <joao.m.martins@xxxxxxxxxx>
> > Signed-off-by: Ankur Arora <ankur.a.arora@xxxxxxxxxx>
> > ---
> > Â drivers/xen/cpu_hotplug.cÂÂÂÂ | 14 ++++++-------
> > Â drivers/xen/gntalloc.cÂÂÂÂÂÂÂ | 13 ++++++++----
> > Â drivers/xen/gntdev.cÂÂÂÂÂÂÂÂÂ | 16 +++++++++++----
> > Â drivers/xen/manage.cÂÂÂÂÂÂÂÂÂ | 37 ++++++++++++++++++-----------------
> > Â drivers/xen/platform-pci.cÂÂÂ | 12 +++++++-----
> >  drivers/xen/sys-hypervisor.c | 12 ++++++++----
> > Â drivers/xen/xen-balloon.cÂÂÂÂ | 10 +++++++---
> > Â drivers/xen/xenfs/xenstored.c |Â 7 ++++---
> > Â 8 files changed, 73 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/xen/cpu_hotplug.c b/drivers/xen/cpu_hotplug.c
> > index afeb94446d34..4a05bc028956 100644
> > --- a/drivers/xen/cpu_hotplug.c
> > +++ b/drivers/xen/cpu_hotplug.c
> > @@ -31,13 +31,13 @@ static void disable_hotplug_cpu(int cpu)
> > ÂÂÂÂÂ unlock_device_hotplug();
> > Â }
> > -static int vcpu_online(unsigned int cpu)
> > +static int vcpu_online(xenhost_t *xh, unsigned int cpu)
>
> Do we really need xenhost for cpu on/offlinig?
I was in two minds about this. We only need it for the xenbus
interfaces which could very well have been just xh_default.

However, the xenhost is part of the xenbus_watch state, so
I thought it is easier to percolate that down instead of
adding xh_default all over the place.

> > diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
> > index 9a69d955dd5c..1655d0a039fd 100644
> > --- a/drivers/xen/manage.c
> > +++ b/drivers/xen/manage.c
> > @@ -227,14 +227,14 @@ static void shutdown_handler(struct xenbus_watch 
> > *watch,
> > ÂÂÂÂÂÂÂÂÂ return;
> > ÂÂ again:
> > -ÂÂÂ err = xenbus_transaction_start(xh_default, &xbt);
> > +ÂÂÂ err = xenbus_transaction_start(watch->xh, &xbt);
> > ÂÂÂÂÂ if (err)
> > ÂÂÂÂÂÂÂÂÂ return;
> > -ÂÂÂ str = (char *)xenbus_read(xh_default, xbt, "control", "shutdown", 
> > NULL);
> > +ÂÂÂ str = (char *)xenbus_read(watch->xh, xbt, "control", "shutdown", 
> > NULL);
> > ÂÂÂÂÂ /* Ignore read errors and empty reads. */
> > ÂÂÂÂÂ if (XENBUS_IS_ERR_READ(str)) {
> > -ÂÂÂÂÂÂÂ xenbus_transaction_end(xh_default, xbt, 1);
> > +ÂÂÂÂÂÂÂ xenbus_transaction_end(watch->xh, xbt, 1);
> > ÂÂÂÂÂÂÂÂÂ return;
> > ÂÂÂÂÂ }
> > @@ -245,9 +245,9 @@ static void shutdown_handler(struct xenbus_watch 
> > *watch,
> > ÂÂÂÂÂ /* Only acknowledge commands which we are prepared to handle. */
> > ÂÂÂÂÂ if (idx < ARRAY_SIZE(shutdown_handlers))
> > -ÂÂÂÂÂÂÂ xenbus_write(xh_default, xbt, "control", "shutdown", "");
> > +ÂÂÂÂÂÂÂ xenbus_write(watch->xh, xbt, "control", "shutdown", "");
> > -ÂÂÂ err = xenbus_transaction_end(xh_default, xbt, 0);
> > +ÂÂÂ err = xenbus_transaction_end(watch->xh, xbt, 0);
> > ÂÂÂÂÂ if (err == -EAGAIN) {
> > ÂÂÂÂÂÂÂÂÂ kfree(str);
> > ÂÂÂÂÂÂÂÂÂ goto again;
> > @@ -272,10 +272,10 @@ static void sysrq_handler(struct xenbus_watch 
> > *watch, const char *path,
> > ÂÂÂÂÂ int err;
> > ÂÂ again:
> > -ÂÂÂ err = xenbus_transaction_start(xh_default, &xbt);
> > +ÂÂÂ err = xenbus_transaction_start(watch->xh, &xbt);
> > ÂÂÂÂÂ if (err)
> > ÂÂÂÂÂÂÂÂÂ return;
> > -ÂÂÂ err = xenbus_scanf(xh_default, xbt, "control", "sysrq", "%c", 
> > &sysrq_key);
> > +ÂÂÂ err = xenbus_scanf(watch->xh, xbt, "control", "sysrq", "%c", 
> > &sysrq_key);
> > ÂÂÂÂÂ if (err < 0) {
> > ÂÂÂÂÂÂÂÂÂ /*
> > ÂÂÂÂÂÂÂÂÂÂ * The Xenstore watch fires directly after registering it and
> > @@ -287,21 +287,21 @@ static void sysrq_handler(struct xenbus_watch 
> > *watch, const char *path,
> > ÂÂÂÂÂÂÂÂÂ if (err != -ENOENT && err != -ERANGE)
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂ pr_err("Error %d reading sysrq code in control/sysrq\n",
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ err);
> > -ÂÂÂÂÂÂÂ xenbus_transaction_end(xh_default, xbt, 1);
> > +ÂÂÂÂÂÂÂ xenbus_transaction_end(watch->xh, xbt, 1);
> > ÂÂÂÂÂÂÂÂÂ return;
> > ÂÂÂÂÂ }
> > ÂÂÂÂÂ if (sysrq_key != '\0') {
> > -ÂÂÂÂÂÂÂ err = xenbus_printf(xh_default, xbt, "control", "sysrq", 
> > "%c", '\0');
> > +ÂÂÂÂÂÂÂ err = xenbus_printf(watch->xh, xbt, "control", "sysrq", "%c", 
> > '\0');
> > ÂÂÂÂÂÂÂÂÂ if (err) {
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂ pr_err("%s: Error %d writing sysrq in control/sysrq\n",
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ __func__, err);
> > -ÂÂÂÂÂÂÂÂÂÂÂ xenbus_transaction_end(xh_default, xbt, 1);
> > +ÂÂÂÂÂÂÂÂÂÂÂ xenbus_transaction_end(watch->xh, xbt, 1);
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂ return;
> > ÂÂÂÂÂÂÂÂÂ }
> > ÂÂÂÂÂ }
> > -ÂÂÂ err = xenbus_transaction_end(xh_default, xbt, 0);
> > +ÂÂÂ err = xenbus_transaction_end(watch->xh, xbt, 0);
> > ÂÂÂÂÂ if (err == -EAGAIN)
> > ÂÂÂÂÂÂÂÂÂ goto again;
> > @@ -324,14 +324,14 @@ static struct notifier_block xen_reboot_nb = {
> > ÂÂÂÂÂ .notifier_call = poweroff_nb,
> > Â };
> > -static int setup_shutdown_watcher(void)
> > +static int setup_shutdown_watcher(xenhost_t *xh)
>
> I think shutdown is purely local, too.
Yes, I introduced xenhost for the same reason as above.

I agree that either of these cases (and similar others) have no use
for the concept of xenhost. Do you think it makes sense for these
to pass NULL instead and the underlying interface would just assume
xh_default.

Ankur

> Juergen