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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 18 Jun 2019 19:55:57 -0700
From:   Ankur Arora <ankur.a.arora@...cle.com>
To:     Juergen Gross <jgross@...e.com>, linux-kernel@...r.kernel.org,
        xen-devel@...ts.xenproject.org
Cc:     pbonzini@...hat.com, boris.ostrovsky@...cle.com,
        konrad.wilk@...cle.com, sstabellini@...nel.org,
        joao.m.martins@...cle.com
Subject: Re: [RFC PATCH 13/16] drivers/xen: gnttab, evtchn, xenbus API changes

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@...cle.com>
>> Signed-off-by: Ankur Arora <ankur.a.arora@...cle.com>
>> ---
>>   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