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

[Julien Grall <julien@xen.org>]

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