[v1] xenbus: Allow PVH dom0 a non-local xenstore

[PATCH] xenbus: Allow PVH dom0 a non-local xenstore

Posted by Jason Andryuk 6 months ago

Make xenbus_init() allow a non-local xenstore for a PVH dom0 - it is
currently forced to XS_LOCAL.  With Hyperlaunch booting dom0 and a
xenstore stubdom, dom0 can be handled as a regular XS_HVM following the
late init path.

Drop the use of xen_initial_domain() and just check for the event
channel instead.  This matches the PV case where there is no check for
initial domain.

Check the full 64bit HVM_PARAM_STORE_EVTCHN value to catch the off
chance that high bits are set for the 32bit event channel.

Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
---
 drivers/xen/xenbus/xenbus_probe.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
index 6d32ffb01136..7604f70ee108 100644
--- a/drivers/xen/xenbus/xenbus_probe.c
+++ b/drivers/xen/xenbus/xenbus_probe.c
@@ -966,9 +966,15 @@ static int __init xenbus_init(void)
 	if (xen_pv_domain())
 		xen_store_domain_type = XS_PV;
 	if (xen_hvm_domain())
+	{
 		xen_store_domain_type = XS_HVM;
-	if (xen_hvm_domain() && xen_initial_domain())
-		xen_store_domain_type = XS_LOCAL;
+		err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v);
+		if (err)
+			goto out_error;
+		xen_store_evtchn = (int)v;
+		if (!v)
+			xen_store_domain_type = XS_LOCAL;
+	}
 	if (xen_pv_domain() && !xen_start_info->store_evtchn)
 		xen_store_domain_type = XS_LOCAL;
 	if (xen_pv_domain() && xen_start_info->store_evtchn)
@@ -987,10 +993,6 @@ static int __init xenbus_init(void)
 		xen_store_interface = gfn_to_virt(xen_store_gfn);
 		break;
 	case XS_HVM:
-		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;
-- 
2.34.1

Re: [PATCH] xenbus: Allow PVH dom0 a non-local xenstore

Posted by Stefano Stabellini 5 months, 4 weeks ago

On Sat, 3 May 2025, Jason Andryuk wrote:
> Make xenbus_init() allow a non-local xenstore for a PVH dom0 - it is
> currently forced to XS_LOCAL.  With Hyperlaunch booting dom0 and a
> xenstore stubdom, dom0 can be handled as a regular XS_HVM following the
> late init path.
> 
> Drop the use of xen_initial_domain() and just check for the event
> channel instead.  This matches the PV case where there is no check for
> initial domain.
> 
> Check the full 64bit HVM_PARAM_STORE_EVTCHN value to catch the off
> chance that high bits are set for the 32bit event channel.
> 
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  drivers/xen/xenbus/xenbus_probe.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
> index 6d32ffb01136..7604f70ee108 100644
> --- a/drivers/xen/xenbus/xenbus_probe.c
> +++ b/drivers/xen/xenbus/xenbus_probe.c
> @@ -966,9 +966,15 @@ static int __init xenbus_init(void)
>  	if (xen_pv_domain())
>  		xen_store_domain_type = XS_PV;
>  	if (xen_hvm_domain())
> +	{
>  		xen_store_domain_type = XS_HVM;
> -	if (xen_hvm_domain() && xen_initial_domain())
> -		xen_store_domain_type = XS_LOCAL;
> +		err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v);
> +		if (err)
> +			goto out_error;
> +		xen_store_evtchn = (int)v;
> +		if (!v)
> +			xen_store_domain_type = XS_LOCAL;
> +	}
>  	if (xen_pv_domain() && !xen_start_info->store_evtchn)
>  		xen_store_domain_type = XS_LOCAL;
>  	if (xen_pv_domain() && xen_start_info->store_evtchn)
> @@ -987,10 +993,6 @@ static int __init xenbus_init(void)
>  		xen_store_interface = gfn_to_virt(xen_store_gfn);
>  		break;
>  	case XS_HVM:
> -		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;
> -- 
> 2.34.1
>

Re: [PATCH] xenbus: Allow PVH dom0 a non-local xenstore

Posted by Jason Andryuk 5 months, 3 weeks ago

On 2025-05-05 16:44, Stefano Stabellini wrote:
> On Sat, 3 May 2025, Jason Andryuk wrote:
>> Make xenbus_init() allow a non-local xenstore for a PVH dom0 - it is
>> currently forced to XS_LOCAL.  With Hyperlaunch booting dom0 and a
>> xenstore stubdom, dom0 can be handled as a regular XS_HVM following the
>> late init path.
>>
>> Drop the use of xen_initial_domain() and just check for the event
>> channel instead.  This matches the PV case where there is no check for
>> initial domain.
>>
>> Check the full 64bit HVM_PARAM_STORE_EVTCHN value to catch the off
>> chance that high bits are set for the 32bit event channel.
>>
>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Thanks, Stefano.  But I'm wondering if this might break ARM enhanced 
no-xenstore.

> 
>> ---
>>   drivers/xen/xenbus/xenbus_probe.c | 14 ++++++++------
>>   1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
>> index 6d32ffb01136..7604f70ee108 100644
>> --- a/drivers/xen/xenbus/xenbus_probe.c
>> +++ b/drivers/xen/xenbus/xenbus_probe.c
>> @@ -966,9 +966,15 @@ static int __init xenbus_init(void)
>>   	if (xen_pv_domain())
>>   		xen_store_domain_type = XS_PV;
>>   	if (xen_hvm_domain())
>> +	{
>>   		xen_store_domain_type = XS_HVM;

ARM would have everything set to XS_HVM...

>> -	if (xen_hvm_domain() && xen_initial_domain())
>> -		xen_store_domain_type = XS_LOCAL;

...and only dom0 set to XS_LOCAL.

>> +		err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v);
>> +		if (err)
>> +			goto out_error;
>> +		xen_store_evtchn = (int)v;
>> +		if (!v)
>> +			xen_store_domain_type = XS_LOCAL;
>> +	}
>>   	if (xen_pv_domain() && !xen_start_info->store_evtchn)
>>   		xen_store_domain_type = XS_LOCAL;
>>   	if (xen_pv_domain() && xen_start_info->store_evtchn)
>> @@ -987,10 +993,6 @@ static int __init xenbus_init(void)
>>   		xen_store_interface = gfn_to_virt(xen_store_gfn);
>>   		break;
>>   	case XS_HVM:
>> -		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;
                 /*
                  * Uninitialized hvm_params are zero and return no error.
                  * Although it is theoretically possible to have
                  * HVM_PARAM_STORE_PFN set to zero on purpose, in 
reality it is
                  * not zero when valid. If zero, it means that Xenstore 
hasn't
                  * been properly initialized. Instead of attempting to 
map a
                  * wrong guest physical address return error.
                  *
                  * Also recognize all bits set as an 
invalid/uninitialized value.
                  */
                 if (!v) {
                         err = -ENOENT;
                         goto out_error;
                 }

IIUC, this !v check is for enhanced no-xenstore to end up in XS_UNKNOWN. 
  I'll have to re-work to handle that case.

Regards,
Jason


>> -- 
>> 2.34.1
>>

Re: [PATCH] xenbus: Allow PVH dom0 a non-local xenstore

Posted by Stefano Stabellini 5 months, 3 weeks ago

On Tue, 6 May 2025, Jason Andryuk wrote:
> On 2025-05-05 16:44, Stefano Stabellini wrote:
> > On Sat, 3 May 2025, Jason Andryuk wrote:
> > > Make xenbus_init() allow a non-local xenstore for a PVH dom0 - it is
> > > currently forced to XS_LOCAL.  With Hyperlaunch booting dom0 and a
> > > xenstore stubdom, dom0 can be handled as a regular XS_HVM following the
> > > late init path.
> > > 
> > > Drop the use of xen_initial_domain() and just check for the event
> > > channel instead.  This matches the PV case where there is no check for
> > > initial domain.
> > > 
> > > Check the full 64bit HVM_PARAM_STORE_EVTCHN value to catch the off
> > > chance that high bits are set for the 32bit event channel.
> > > 
> > > Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> > 
> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> Thanks, Stefano.  But I'm wondering if this might break ARM enhanced
> no-xenstore.
> 
> > 
> > > ---
> > >   drivers/xen/xenbus/xenbus_probe.c | 14 ++++++++------
> > >   1 file changed, 8 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/xen/xenbus/xenbus_probe.c
> > > b/drivers/xen/xenbus/xenbus_probe.c
> > > index 6d32ffb01136..7604f70ee108 100644
> > > --- a/drivers/xen/xenbus/xenbus_probe.c
> > > +++ b/drivers/xen/xenbus/xenbus_probe.c
> > > @@ -966,9 +966,15 @@ static int __init xenbus_init(void)
> > >   	if (xen_pv_domain())
> > >   		xen_store_domain_type = XS_PV;
> > >   	if (xen_hvm_domain())
> > > +	{
> > >   		xen_store_domain_type = XS_HVM;
> 
> ARM would have everything set to XS_HVM...
> 
> > > -	if (xen_hvm_domain() && xen_initial_domain())
> > > -		xen_store_domain_type = XS_LOCAL;
> 
> ...and only dom0 set to XS_LOCAL.
> 
> > > +		err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v);
> > > +		if (err)
> > > +			goto out_error;
> > > +		xen_store_evtchn = (int)v;
> > > +		if (!v)
> > > +			xen_store_domain_type = XS_LOCAL;
> > > +	}
> > >   	if (xen_pv_domain() && !xen_start_info->store_evtchn)
> > >   		xen_store_domain_type = XS_LOCAL;
> > >   	if (xen_pv_domain() && xen_start_info->store_evtchn)
> > > @@ -987,10 +993,6 @@ static int __init xenbus_init(void)
> > >   		xen_store_interface = gfn_to_virt(xen_store_gfn);
> > >   		break;
> > >   	case XS_HVM:
> > > -		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;
>                 /*
>                  * Uninitialized hvm_params are zero and return no error.
>                  * Although it is theoretically possible to have
>                  * HVM_PARAM_STORE_PFN set to zero on purpose, in reality it
> is
>                  * not zero when valid. If zero, it means that Xenstore hasn't
>                  * been properly initialized. Instead of attempting to map a
>                  * wrong guest physical address return error.
>                  *
>                  * Also recognize all bits set as an invalid/uninitialized
> value.
>                  */
>                 if (!v) {
>                         err = -ENOENT;
>                         goto out_error;
>                 }
> 
> IIUC, this !v check is for enhanced no-xenstore to end up in XS_UNKNOWN.  I'll
> have to re-work to handle that case.

I was wondering about that when reviewing this patch, because the
removal of this check was the main change introduced I wasn't sure
about. Looking around I (wrongly) convinced myself that removing the
check was OK. Thank you for spotting this on your own.