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

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 2249 bytes --]

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.

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.

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

[Julien Grall]

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.

Cheers,

-- 
Julien Grall

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

[Julien Grall]

On 06/08/2021 23:00, 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 [...]

I actually forgot to reply on this one and only remembered now. There 
are some funny things happening in Xen on Arm when mapping the grant 
table. At the moment, we rely on the grant table to always be mapped for 
all the components (toolstack, OS, firmware...) at the same place.

If the region end up to be re-used by something else, then it will end 
up to unmap it... We would need to fix it before we can fully re-use the 
region.

>>
>>
>> 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.
> 
> Cheers,
> 

-- 
Julien Grall

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

[Stefano Stabellini]

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.

Although I think it would be OK to have a new compatible string, am I
not sure we need it.

In any case, we'll have to be able to recognize and handle the case
where we run out of the space in the provided region. If the region is
too small (16MB) then it just means we'll run out of space immediately
after mapping the grant table. Then, we'll have to use other techniques.

Or perhaps you think that if we had a new compatible string to say "Xen
binding with a larger region" then we could get away with a simpler
implementation in Linux, one that doesn't handle the case where we run
out of space in the region? If that was the case, then I agree that it
would be worthwhile adding a new compatible.

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

[Julien Grall]

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.
> 
> Although I think it would be OK to have a new compatible string, am I
> not sure we need it.

Let's assume we don't add a new compatible string, property... How do 
would you prevent the following two issues?
   1) XSA-300: A frontend can DoS the backend
   2) Existing Xen expects the grant-table to be mapped at the exact 
same place.

2# could potentially be solved by reserved the first range for the grant 
table. For 1#, I think we need a compatible string (or property).

What else do you have in mind?

FAOD, relying on the region to always be big enough would not be an 
acceptable solution to me :). A frontend may find a new way for a 
frontend to exhaust the region (*hint* virtio *hint*).

> 
> In any case, we'll have to be able to recognize and handle the case
> where we run out of the space in the provided region. If the region is
> too small (16MB) then it just means we'll run out of space immediately
> after mapping the grant table. Then, we'll have to use other techniques.

Right, one of the other techniques is likely to steal RAM page. Which 
means that a frontend could potentially DoS the backend. This will be a 
lot easier to trigger with virtio as the DM tends to cache the mappings.

So I think we ought to prevent stealing the RAM if a new kernel is 
running on a new Xen.

> 
> Or perhaps you think that if we had a new compatible string to say "Xen
> binding with a larger region" then we could get away with a simpler
> implementation in Linux, one that doesn't handle the case where we run
> out of space in the region? If that was the case, then I agree that it
> would be worthwhile adding a new compatible.

We will need to keep the code to steal RAM for the forseeable future 
because a newer Linux may run on an older Xen setup. So simplicity is 
not the reason here.

I have provided the reason above.

Cheers,

-- 
Julien Grall

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

[Oleksandr]

Hello, all.

Gentle reminder.


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?


>>
>> Although I think it would be OK to have a new compatible string, am I
>> not sure we need it.
>
> Let's assume we don't add a new compatible string, property... How do 
> would you prevent the following two issues?
>   1) XSA-300: A frontend can DoS the backend
>   2) Existing Xen expects the grant-table to be mapped at the exact 
> same place.
>
> 2# could potentially be solved by reserved the first range for the 
> grant table. For 1#, I think we need a compatible string (or property).
>
> What else do you have in mind?
>
> FAOD, relying on the region to always be big enough would not be an 
> acceptable solution to me :). A frontend may find a new way for a 
> frontend to exhaust the region (*hint* virtio *hint*).
>
>>
>> In any case, we'll have to be able to recognize and handle the case
>> where we run out of the space in the provided region. If the region is
>> too small (16MB) then it just means we'll run out of space immediately
>> after mapping the grant table. Then, we'll have to use other techniques.
>
> Right, one of the other techniques is likely to steal RAM page. Which 
> means that a frontend could potentially DoS the backend. This will be 
> a lot easier to trigger with virtio as the DM tends to cache the 
> mappings.
>
> So I think we ought to prevent stealing the RAM if a new kernel is 
> running on a new Xen.
>
>>
>> Or perhaps you think that if we had a new compatible string to say "Xen
>> binding with a larger region" then we could get away with a simpler
>> implementation in Linux, one that doesn't handle the case where we run
>> out of space in the region? If that was the case, then I agree that it
>> would be worthwhile adding a new compatible.
>
> We will need to keep the code to steal RAM for the forseeable future 
> because a newer Linux may run on an older Xen setup. So simplicity is 
> not the reason here.
>
> I have provided the reason above.


-- 
Regards,

Oleksandr Tyshchenko

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 5380 bytes --]

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>;
};


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

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

[Oleksandr]

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

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

[Julien Grall]

Hi Stefano,

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".

> 
> So one option is to rely on a check based on the Xen version. Example:
> 
>    version >= xen,xen-4.16

This option would prevent a stakeholder to backport the work to older Xen.

> 
> 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>;
> };
> 
> 
> Julien, do you have a better suggestion for the property name?

I am a bit confused with the name you suggest. To me it looks this would 
be used to indicate whether "reg" has one or more regions.

But I don't think we need a property for that. What we need to a 
property for is to indicate whether the first region is safe to use (see 
the discussion about the grant table).

So can you clarify what you expect to convey with this property? 
Although, at the moment, I don't have a good name to suggest.

Cheers,

-- 
Julien Grall

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 8199 bytes --]

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. 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".


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.



> > So one option is to rely on a check based on the Xen version. Example:
> > 
> >    version >= xen,xen-4.16
> 
> This option would prevent a stakeholder to backport the work to older Xen.
> 
> > 
> > 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>;
> > };
> > 
> > 
> > Julien, do you have a better suggestion for the property name?
> 
> I am a bit confused with the name you suggest. To me it looks this would be
> used to indicate whether "reg" has one or more regions.
> 
> But I don't think we need a property for that. What we need to a property for
> is to indicate whether the first region is safe to use (see the discussion
> about the grant table).
> 
> So can you clarify what you expect to convey with this property? Although, at
> the moment, I don't have a good name to suggest.

Yeah extended-region is a bad name. It meant to convey that the reg
property will cover a larger (extended) address range.

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

[Julien Grall]

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