Re: Clarification regarding updating "Xen hypervisor device tree bindings on Arm"

[Julien Grall <julien@xen.org>]

Hi Stefano,

On 31/08/2021 22:19, Stefano Stabellini wrote:
> On Tue, 31 Aug 2021, Julien Grall wrote:
>> On 28/08/2021 01:05, Stefano Stabellini wrote:
>>> On Fri, 27 Aug 2021, Oleksandr wrote:
>>>> On 07.08.21 01:57, Julien Grall wrote:
>>>>> Hi Stefano,
>>>>>
>>>>> On 06/08/2021 23:26, Stefano Stabellini wrote:
>>>>>> On Fri, 6 Aug 2021, Julien Grall wrote:
>>>>>>> Hi Stefano,
>>>>>>>
>>>>>>> On 06/08/2021 21:15, Stefano Stabellini wrote:
>>>>>>>> On Fri, 6 Aug 2021, Oleksandr Tyshchenko wrote:
>>>>>>>>> Hello, all.
>>>>>>>>>
>>>>>>>>> I would like to clarify some bits regarding a possible update
>>>>>>>>> for
>>>>>>>>> "Xen
>>>>>>>>> device tree bindings for the guest" [1].
>>>>>>>>>
>>>>>>>>> A bit of context:
>>>>>>>>> We are considering extending "reg" property under the hypervisor
>>>>>>>>> node and
>>>>>>>>> we would like to avoid breaking backward compatibility.
>>>>>>>>> So far, the "reg" was used to carry a single region for the
>>>>>>>>> grant
>>>>>>>>> table
>>>>>>>>> mapping only and it's size is quite small for the new
>>>>>>>>> improvement
>>>>>>>>> we are currently working on.
>>>>>>>>>
>>>>>>>>> What we want to do is to extend the current region [reg: 0] and
>>>>>>>>> add
>>>>>>>>> an
>>>>>>>>> extra regions [reg: 1-N] to be used as a safe address space for
>>>>>>>>> any
>>>>>>>>> Xen specific mappings. But, we need to be careful about running
>>>>>>>>> "new"
>>>>>>>>> guests (with the improvement being built-in already) on "old"
>>>>>>>>> Xen
>>>>>>>>> which is not aware of the extended regions, so we need the
>>>>>>>>> binding
>>>>>>>>> to be
>>>>>>>>> extended in a backward compatible way. In order to detect
>>>>>>>>> whether
>>>>>>>>> we are running on top of the "new" Xen (and it provides us
>>>>>>>>> enough
>>>>>>>>> space to
>>>>>>>>> be used for improvement), we definitely need some sign to
>>>>>>>>> indicate that.
>>>>>>>>>
>>>>>>>>> Could you please clarify, how do you expect the binding to be
>>>>>>>>> changed in
>>>>>>>>> the backward compatible way?
>>>>>>>>> - by adding an extra compatible (as it is a change of the
>>>>>>>>> binding
>>>>>>>>> technically)
>>>>>>>>> - by just adding new property (xen,***) to indicate that "reg"
>>>>>>>>> contains
>>>>>>>>> enough space
>>>>>>>>> - other option
>>>>>>>>      The current description is:
>>>>>>>>
>>>>>>>> - reg: specifies the base physical address and size of a region in
>>>>>>>>       memory where the grant table should be mapped to, using an
>>>>>>>>       HYPERVISOR_memory_op hypercall [...]
>>>>>>>>
>>>>>>>>
>>>>>>>> Although it says "a region" I think that adding multiple ranges
>>>>>>>> would
>>>>>>>> be
>>>>>>>> fine and shouldn't break backward compatibility.
>>>>>>>>
>>>>>>>> In addition, the purpose of the region was described as "where the
>>>>>>>> grant
>>>>>>>> table should be mapped". In other words, it is a safe address
>>>>>>>> range
>>>>>>>> where the OS can map Xen special pages.
>>>>>>>>
>>>>>>>> Your proposal is to extend the region to be bigger to allow the OS
>>>>>>>> to
>>>>>>>> map more Xen special pages. I think it is a natural extension to
>>>>>>>> the
>>>>>>>> binding, which should be backward compatible.
>>>>>>>
>>>>>>> I agree that extending the reg (or even adding a second region)
>>>>>>> should
>>>>>>> be fine
>>>>>>> for older OS.
>>>>>>>
>>>>>>>>
>>>>>>>> Rob, I am not sure what is commonly done in these cases. Maybe we
>>>>>>>> just
>>>>>>>> need an update to the description of the binding? I am also fine
>>>>>>>> with
>>>>>>>> adding a new compatible string if needed.
>>>>>>>
>>>>>>> So the trouble is how a newer Linux version knows that the region is
>>>>>>> big
>>>>>>> enough to deal with all the foreign/grant mapping?
>>>>>>>
>>>>>>> If you run on older Xen, then the region will only be 16MB. This
>>>>>>> means
>>>>>>> the
>>>>>>> Linux will have to fallback on stealing RAM as it is today.
>>>>>>>
>>>>>>> IOW, XSA-300 will still be a thing. On newer Xen (or toolstack), we
>>>>>>> ideally
>>>>>>> want the OS to not fallback on stealing RAM (and close XSA-300).
>>>>>>> This is
>>>>>>> where
>>>>>>> we need a way to advertise it.
>>>>>>>
>>>>>>> The question here is whether we want to use a property or a
>>>>>>> compatible
>>>>>>> for
>>>>>>> this.
>>>>>>>
>>>>>>> I am leaning towards the latter because this is an extension of the
>>>>>>> bindings.
>>>>>>> However, I wasn't entirely whether this was a normal way to do it.
>>>>
>>>>
>>>> May I please ask for the clarification how to properly advertise that we
>>>> have
>>>> extended region? By new compatible or property?
>>>
>>> The current compatible string is defined as:
>>>
>>> - compatible:
>>> 	compatible = "xen,xen-<version>", "xen,xen";
>>>     where <version> is the version of the Xen ABI of the platform.
>>>
>>>
>>> On the Xen side it is implemented as:
>>>
>>> "xen,xen-"__stringify(XEN_VERSION)"."__stringify(XEN_SUBVERSION)"\0"
>>>
>>>
>>> So in a way we already have the version in the compatible string but it
>>> is just the Xen version, not the version of the Device Tree binding.
>>>
>>>
>>> Looking at the way the compatible string is parsed in Linux, I think we
>>> cannot easily change/add a different string format because it would
>>> cause older Linux to stop initializing the Xen subsystem.
>>
>> AFAICT, Linux doesn't care about extra compatible after "xen,xen". So in
>> theory we could write:
>>
>> "xen,xen-<version>", "xen,xen", "xen,xen-v2".
> 
> Keep in mind that the generic compatible string ("xen,xen") is typically
> last.

Ok. Even if it is written "xen,xen-<version>", "xen,xen-v2", "xen,xen". 
I still don't see the problem (see more below).

  Also we shouldn't rely on their ordering. Considering the definition
> of hyper_node:
> 
> 
> static __initdata struct {
> 	const char *compat;
> 	const char *prefix;
> 	const char *version;
> 	bool found;
> } hyper_node = {"xen,xen", "xen,xen-", NULL, false};
> 
> 
> And the following code:
> 
> 
> 	s = of_get_flat_dt_prop(node, "compatible", &len);
> 	if (strlen(hyper_node.prefix) + 3  < len &&
> 	    !strncmp(hyper_node.prefix, s, strlen(hyper_node.prefix)))
> 		hyper_node.version = s + strlen(hyper_node.prefix);
> 
> 
> It looks like there is potential for breakage. For instance, It looks
> like that hyper_node.version could end up being set to "xen,xen-v2"
> depending on the success or failure of the first < len check. If not
> "xen,xen-v2", I would guess that "xen,xen-version-2" would cause
> hyper_node.version to be set to "version-2".

How so? s points to the beginning of the property. So if the if 
"xen,xen-4.16" is always first, there there is no way 
"hyper_node.version" can be set to "version-2".

> 
> 
> If we were to introduce a new compatible we would need to make it so the
> prefix "xen,xen-" does not match it. Something like "xen,api-v2" might
> be possible.

I don't particularly care about the compatible name. This was an example 
to show that there is no issue with adding an extra compatible. I 
thought the compatible way was better because at least we don't have to 
add a new property every single time we extend the bindings.

However, the point of this conversation was to figure out whether the 
common way to extend the Device-Tree is to add a compatible or a new 
property. Not what we (Xen Project) prefer.

Cheers,

-- 
Julien Grall