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

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

On 08/08/2012 01:19 PM, Stefano Stabellini wrote:
> On Wed, 8 Aug 2012, Daniel De Graaf wrote:
>> On 08/08/2012 12:51 PM, Stefano Stabellini wrote:
>>> 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@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.
>>>
>>
>> That looks correct, but you'd need to split up the switch statement in
>> order to populate xen_store_evtchn before that last condition, which
>> ends up pretty much eliminating the usage variable.
> 
> Going back to what you wrote in the previous email, in what way this
> patch breaks the case when an initial domain is started after a Xenstore
> stub domain?
> 
> Assuming that we are talking about a PV initial domain on x86, the
> following check
> 
> if (xen_pv_domain() && !xen_start_info->store_evtchn)
>     usage = LOCAL;
> 
> will return false (because store_evtchn is set), therefore usage will
> remain set to PV.
> And the check:
> 
> if (xen_pv_domain() && xen_start_info->store_evtchn)
> 	xenstored_ready = 1;
> 
> will return true so xenstored_ready is going to be set to 1.
> 

Right, the original patch didn't break anything with PV domains. The case
it doesn't handle is an HVM initial domain with an already-running
Xenstore domain; I think this applies both to ARM and hybrid/PVH on x86.
In that case, usage would be set to LOCAL instead of HVM.

As a side note: the value of xen_initial_domain() shouldn't be connected to
determining if xenstore is running locally or not.

-- 
Daniel De Graaf
National Security Agency