Re: [Xen-devel] [PATCH RESEND] xenbus: Add support for xenbus backend in stub domain

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

On 05/08/2012 12:34 PM, Konrad Rzeszutek Wilk wrote:
> On Tue, May 08, 2012 at 09:46:57AM -0400, Daniel De Graaf wrote:
>> Add an ioctl to the /dev/xen/xenbus_backend device allowing the xenbus
>> backend to be started after the kernel has booted. This allows xenstore
>> to run in a different domain from the dom0.
>>
>> Signed-off-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
>> ---
>>  drivers/xen/xenbus/xenbus_comms.c       |    6 ++++
>>  drivers/xen/xenbus/xenbus_comms.h       |    1 +
>>  drivers/xen/xenbus/xenbus_dev_backend.c |   51 
>> +++++++++++++++++++++++++++++++
>>  include/xen/grant_table.h               |    2 +
>>  include/xen/xenbus_dev.h                |    3 ++
>>  5 files changed, 63 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/xen/xenbus/xenbus_comms.c 
>> b/drivers/xen/xenbus/xenbus_comms.c
>> index 2eff7a6..52fe7ad 100644
>> --- a/drivers/xen/xenbus/xenbus_comms.c
>> +++ b/drivers/xen/xenbus/xenbus_comms.c
>> @@ -234,3 +234,9 @@ int xb_init_comms(void)
>>  
>>      return 0;
>>  }
>> +
>> +void xb_deinit_comms(void)
>> +{
>> +    unbind_from_irqhandler(xenbus_irq, &xb_waitq);
>> +    xenbus_irq = 0;
>> +}
>> diff --git a/drivers/xen/xenbus/xenbus_comms.h 
>> b/drivers/xen/xenbus/xenbus_comms.h
>> index 6e42800..c8abd3b 100644
>> --- a/drivers/xen/xenbus/xenbus_comms.h
>> +++ b/drivers/xen/xenbus/xenbus_comms.h
>> @@ -35,6 +35,7 @@
>>  
>>  int xs_init(void);
>>  int xb_init_comms(void);
>> +void xb_deinit_comms(void);
>>  
>>  /* Low level routines. */
>>  int xb_write(const void *data, unsigned len);
>> diff --git a/drivers/xen/xenbus/xenbus_dev_backend.c 
>> b/drivers/xen/xenbus/xenbus_dev_backend.c
>> index 3d3be78..be738c4 100644
>> --- a/drivers/xen/xenbus/xenbus_dev_backend.c
>> +++ b/drivers/xen/xenbus/xenbus_dev_backend.c
>> @@ -8,7 +8,11 @@
>>  
>>  #include <xen/xen.h>
>>  #include <xen/page.h>
>> +#include <xen/xenbus.h>
>>  #include <xen/xenbus_dev.h>
>> +#include <xen/grant_table.h>
>> +#include <xen/events.h>
>> +#include <asm/xen/hypervisor.h>
>>  
>>  #include "xenbus_comms.h"
>>  
>> @@ -22,6 +26,50 @@ static int xenbus_backend_open(struct inode *inode, 
>> struct file *filp)
>>      return nonseekable_open(inode, filp);
>>  }
>>  
>> +static long xenbus_alloc(domid_t domid)
>> +{
>> +    struct evtchn_alloc_unbound arg;
>> +    int err = -EEXIST;
>> +
>> +    xs_suspend();
>> +
>> +    /* If xenstored_ready is nonzero, that means we have already talked to
>> +     * xenstore and set up watches. These watches will be restored by
>> +     * xs_resume, but that requires communication over the port established
>> +     * below that is not visible to anyone until the ioctl returns.
>> +     *
>> +     * This can be resolved by splitting the ioctl into two parts
>> +     * (postponing the resume until xenstored is active) but this is
>> +     * unnecessarily complex for the intended use where xenstored is only
>> +     * started once - so return -EEXIST if it's already running.
>> +     */
>> +    if (xenstored_ready)
>> +            goto out_err;
>> +
>> +    gnttab_grant_foreign_access_ref(GNTTAB_RESERVED_XENSTORE, domid,
>> +                    virt_to_mfn(xen_store_interface), 0 /* writable */);
>> +
>> +    arg.dom = DOMID_SELF;
>> +    arg.remote_dom = domid;
> 
> If I specify as domid = DOMID_SELF what will happen? Should we filter some
> of the obvious no-no values? Like DOMID_IO, DOMID_SELF?

For DOMID_SELF, this will just be a convoluted way of getting a xenstore
event channel bound locally - the same as from the existing ioctl. This ends up
being a useless re-allocation of the event channel since it closes the existing
one.

Specifying an invalid domain ID will leave xenstore in the uninitialized state
because no domain will ever connect and send the notify to make xenstored_ready
nonzero. Until this happens, you can call the ioctl again, so there's no need to
filter out the invalid values.

>> +
>> +    err = HYPERVISOR_event_channel_op(EVTCHNOP_alloc_unbound, &arg);
>> +    if (err)
>> +            goto out_err;
>> +
>> +    if (xen_store_evtchn > 0)
>> +            xb_deinit_comms();
>> +
>> +    xen_store_evtchn = arg.port;
>> +
>> +    xs_resume();
>> +
>> +    return arg.port;
>> +
>> + out_err:
>> +    xs_suspend_cancel();
>> +    return err;
>> +}
>> +
>>  static long xenbus_backend_ioctl(struct file *file, unsigned int cmd, 
>> unsigned long data)
>>  {
>>      if (!capable(CAP_SYS_ADMIN))
>> @@ -33,6 +81,9 @@ static long xenbus_backend_ioctl(struct file *file, 
>> unsigned int cmd, unsigned l
>>                              return xen_store_evtchn;
>>                      return -ENODEV;
>>  
>> +            case IOCTL_XENBUS_BACKEND_SETUP:
>> +                    return xenbus_alloc(data);
>> +
>>              default:
>>                      return -ENOTTY;
>>      }
>> diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
>> index 15f8a00..11e27c3 100644
>> --- a/include/xen/grant_table.h
>> +++ b/include/xen/grant_table.h
>> @@ -46,6 +46,8 @@
>>  
>>  #include <xen/features.h>
>>  
>> +#define GNTTAB_RESERVED_XENSTORE 1
>> +
>>  /* NR_GRANT_FRAMES must be less than or equal to that configured in Xen */
>>  #define NR_GRANT_FRAMES 4
>>  
>> diff --git a/include/xen/xenbus_dev.h b/include/xen/xenbus_dev.h
>> index ac5f0fe..bbee8c6 100644
>> --- a/include/xen/xenbus_dev.h
>> +++ b/include/xen/xenbus_dev.h
>> @@ -38,4 +38,7 @@
>>  #define IOCTL_XENBUS_BACKEND_EVTCHN                 \
>>      _IOC(_IOC_NONE, 'B', 0, 0)
>>  
>> +#define IOCTL_XENBUS_BACKEND_SETUP                  \
>> +    _IOC(_IOC_NONE, 'B', 1, 0)
>> +
>>  #endif /* __LINUX_XEN_XENBUS_DEV_H__ */
>> -- 
>> 1.7.7.6


-- 
Daniel De Graaf
National Security Agency

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel