Re: [PATCH v2 7/7] xen/arm: introduce new xen,enhanced property value

From: Rahul Singh <Rahul.Singh@arm.com>
To: Julien Grall <julien@xen.org>
Cc: xen-devel <xen-devel@lists.xenproject.org>,
	Bertrand Marquis <Bertrand.Marquis@arm.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [PATCH v2 7/7] xen/arm: introduce new xen,enhanced property value
Date: Wed, 24 Aug 2022 16:35:48 +0000	[thread overview]
Message-ID: <474E0887-7EFC-4A0C-9815-DFBDAE9A93A9@arm.com> (raw)
In-Reply-To: <685e53cc-1b83-8d66-7086-dad4135c0587@xen.org>

Hi Julien

> On 24 Aug 2022, at 4:36 pm, Julien Grall <julien@xen.org> wrote:
> 
> On 24/08/2022 15:42, Rahul Singh wrote:
>>> On 24 Aug 2022, at 1:59 pm, Julien Grall <julien@xen.org> wrote:
>>> 
>>> 
>>> 
>>> On 24/08/2022 13:15, Rahul Singh wrote:
>>>> Hi Julien,
>>> 
>>> Hi Rahul,
>>> 
>>>> Please let me know your view on this.
>>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>>> index bfe7bc6b36..a1e23eee59 100644
>>>> --- a/xen/arch/arm/domain_build.c
>>>> +++ b/xen/arch/arm/domain_build.c
>>>> @@ -3562,12 +3562,7 @@ static int __init construct_domU(struct domain *d,
>>>>    if ( rc == -EILSEQ ||
>>>>      rc == -ENODATA ||
>>>>      (rc == 0 && !strcmp(dom0less_enhanced, “enabled”)) )
>>>> -  {
>>>> -    if ( hardware_domain )
>>>>        kinfo.dom0less_enhanced = true;
>>>> -    else
>>>> -      panic(“Tried to use xen,enhanced without dom0\n”);
>>>> -  }
>>> 
>>> You can't use "xen,enhanced" without dom0. In fact, you will end up to dereference NULL in alloc_xenstore_evtchn(). That's because "xen,enhanced" means the domain will be able to use Xenstored.
>>> 
>>> Now if you want to support your feature without a dom0. Then I think we want to introduce an option which would be the same as "xen,enhanced" but doesn't expose Xenstored.
>> If we modify the patch as below we can use the "xen,enhanced" for domUs without dom0.
>> I tested the patch and its works fine. Do you see any issue with this approach?
> 
> Yes. For two reasons:
> 1) It is muddying the meaning of "xen,enhanced". In particular a user may not realize that Xenstore is not available if dom0 is not present.
> 2) It would be more complicated to handle the case where Xenstored lives in a non-dom0 domain. I am not aware of anyone wanting this on Arm yet, but I don't want to close the door.
> 
> So if you want to support create "xen,xen" without all the rest. Then I think we need a different property value. I don't have a good suggestion for the name.

Is that okay if we use the earlier approach, when user set  "xen,enhanced = evtchn” we will not call alloc_xenstore_evtchn()  
but we create hypervisor node with all fields.

Regards,
Rahul