[PATCH v2 10/23] xen/arm: compile and run xenbus

[Stefano Stabellini]

On Tue, 7 Aug 2012, Daniel De Graaf wrote:
> On 08/07/2012 02:21 PM, Konrad Rzeszutek Wilk wrote:
> > On Mon, Aug 06, 2012 at 03:27:13PM +0100, Stefano Stabellini wrote:
> >> bind_evtchn_to_irqhandler can legitimately return 0 (irq 0): it is not
> >> an error.
> >>
> >> If Linux is running as an HVM domain and is running as Dom0, use
> >> xenstored_local_init to initialize the xenstore page and event channel.
> >>
> >> Changes in v2:
> >>
> >> - refactor xenbus_init.
> > 
> > Thank you. Lets also CC our friend at NSA who has been doing some work
> > in that area. Daniel are you OK with this change - will it still make
> > PV initial domain with with the MiniOS XenBus driver?
> > 
> > Thanks.
> 
> That case will work, but what this will break is launching the initial domain
> with a Xenstore stub domain already running (see below).
> 
> >>
> >> Signed-off-by: Stefano Stabellini <stefano.stabellini at eu.citrix.com>
> >> ---
> >>  drivers/xen/xenbus/xenbus_comms.c |    2 +-
> >>  drivers/xen/xenbus/xenbus_probe.c |   62 +++++++++++++++++++++++++-----------
> >>  drivers/xen/xenbus/xenbus_xs.c    |    1 +
> >>  3 files changed, 45 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/drivers/xen/xenbus/xenbus_comms.c b/drivers/xen/xenbus/xenbus_comms.c
> >> index 52fe7ad..c5aa55c 100644
> >> --- a/drivers/xen/xenbus/xenbus_comms.c
> >> +++ b/drivers/xen/xenbus/xenbus_comms.c
> >> @@ -224,7 +224,7 @@ int xb_init_comms(void)
> >>  		int err;
> >>  		err = bind_evtchn_to_irqhandler(xen_store_evtchn, wake_waiting,
> >>  						0, "xenbus", &xb_waitq);
> >> -		if (err <= 0) {
> >> +		if (err < 0) {
> >>  			printk(KERN_ERR "XENBUS request irq failed %i\n", err);
> >>  			return err;
> >>  		}
> > 
> >> diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
> >> index b793723..a67ccc0 100644
> >> --- a/drivers/xen/xenbus/xenbus_probe.c
> >> +++ b/drivers/xen/xenbus/xenbus_probe.c
> >> @@ -719,37 +719,61 @@ static int __init xenstored_local_init(void)
> >>  	return err;
> >>  }
> >>  
> >> +enum xenstore_init {
> >> +	UNKNOWN,
> >> +	PV,
> >> +	HVM,
> >> +	LOCAL,
> >> +};
> >>  static int __init xenbus_init(void)
> >>  {
> >>  	int err = 0;
> >> +	enum xenstore_init usage = UNKNOWN;
> >> +	uint64_t v = 0;
> >>  
> >>  	if (!xen_domain())
> >>  		return -ENODEV;
> >>  
> >>  	xenbus_ring_ops_init();
> >>  
> >> -	if (xen_hvm_domain()) {
> >> -		uint64_t v = 0;
> >> -		err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v);
> >> -		if (err)
> >> -			goto out_error;
> >> -		xen_store_evtchn = (int)v;
> >> -		err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v);
> >> -		if (err)
> >> -			goto out_error;
> >> -		xen_store_mfn = (unsigned long)v;
> >> -		xen_store_interface = ioremap(xen_store_mfn << PAGE_SHIFT, PAGE_SIZE);
> >> -	} else {
> >> -		xen_store_evtchn = xen_start_info->store_evtchn;
> >> -		xen_store_mfn = xen_start_info->store_mfn;
> >> -		if (xen_store_evtchn)
> >> -			xenstored_ready = 1;
> >> -		else {
> >> +	if (xen_pv_domain())
> >> +		usage = PV;
> >> +	if (xen_hvm_domain())
> >> +		usage = HVM;
> 
> The above is correct for domUs, and is overridden for dom0s:
>
> >> +	if (xen_hvm_domain() && xen_initial_domain())
> >> +		usage = LOCAL;
> >> +	if (xen_pv_domain() && !xen_start_info->store_evtchn)
> >> +		usage = LOCAL;
> 
> Instead of these checks, I think it should just be:
> 
> if (!xen_start_info->store_evtchn)
> 	usage = LOCAL;
> 
> Any domain started after xenstore will have store_evtchn set, so if you don't
> have this set, you are either going to be running xenstore locally, or will
> use the ioctl to change it later (and so should still set up everything as if
> it will be running locally).

That would be wrong for an HVM dom0 domain (at least on ARM), because
we don't have a start_info page at all.


> >> +	if (xen_pv_domain() && xen_start_info->store_evtchn)
> >> +		xenstored_ready = 1;
> 
> This part can now just be moved unconditionally into case PV.

What about:

if (xen_pv_domain())
    usage = PV;
if (xen_hvm_domain())
    usage = HVM;
if (!xen_store_evtchn)
    usage = LOCAL;

and moving xenstored_ready in case PV, like you suggested.