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

[Stefano Stabellini <sstabellini@kernel.org>]

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