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

[Oleksandr <olekstysh@gmail.com>]

On 28.08.21 03:05, Stefano Stabellini wrote:

Hi Stefano

> 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.
>
> So one option is to rely on a check based on the Xen version. Example:
>
>    version >= xen,xen-4.16
>
>
> Or we need to go with a property. This seems safer and more solid. The
> property could be as simple as "extended-region":
>
> hypervisor {
> 	compatible = "xen,xen-4.16", "xen,xen";
>      extended-region;
> 	reg = <0 0xb0000000 0 0x20000 0xc 0x0 0x1 0x0>;
> 	interrupts = <1 15 0xf08>;
> };

Thank you for the detailed analysis, I think, it makes sense.


>
>
> Julien, do you have a better suggestion for the property name?

-- 
Regards,

Oleksandr Tyshchenko