Re: [PATCH v3 2/4] docs, xen/arm: Introduce static heap memory

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 7 Sep 2022, Julien Grall wrote:
> On 07/09/2022 14:49, Henry Wang wrote:
> > > -----Original Message-----
> > > From: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
> > > > > > > But in any case we should only add one pair here for sure, as you
> > > > > > > say
> > > the
> > > > > > > only implication is to add a couple of 0 in the worst case.
> > > > > > I agree. The only drawback is the need to modify the already
> > > > > > introduced
> > > properties
> > > > > > to be coherent.
> > > > > 
> > > > > Agree, someone will need to do a pass on the whole doc which might be
> > > easier with all things in.
> > > > > 
> > > > Well, not only docs. If we decide to use a single pair of #address-cells
> > > > and
> > > #size-cells, then
> > > > we need to modify the code that expects different properties e.g.
> > > xen,static-mem-{address/size}-cells.
> > > 
> > > Right I forgot that some parts are already in.
> > > So we will need an extra patch to handle those.
> > 
> > I think I've addressed all comments from Julien regarding my series,
> 
> If it is not too late for you would you be able to resend your series without
> the 'address-cells'/'size-cells' change? This will give me the opportunity to
> have an other review today.
> 
> > so I think I've got some bandwidth to do the clean-up patch tomorrow
> > after the agreement, unless someone would like to do it himself?
> 
> Renaming "xen,static-mem-..." is a bit tricky because they have been defined
> in Xen 4.16.
> 
> I couldn't find any support statement specific to the static memory feature.
> So it would technically fall under the "dom0less" section which is security
> supported.
> 
> That said, I don't think we can consider that the static memory feature is
> even supported because, until yesterday, the code wasn't properly handling
> request to balloon in/out. So I would view this is a tech preview (Could
> someone send a patch to clarify SUPPORT.MD)?
> 
> This would mean that would be that we could consider the binding unstable and
> we could do a straight renaming. That said, I can understand this may be
> undesirable.
> 
> If that's the case then we would need to keep the current binding as-is. So we
> would have two options:
>   1) Provide a new compatible so #address-cells #size-cells can be used. The
> current binding can be deprecated
>   2) Leave as-is and accept the difference
> 
> I don't have a strong opinion on which way to go. Whichever, it would be good
> to write down the rationale in the commit message of the "future" patch.
> 
> I would not block this series on the renaming for existing property (what
> matter is the new ones are consistent with the discussion). The renaming could
> be done afterwards. I would even say post the feature freeze on Friday because
> this could be considered as a bug fix (assuming you agree as the release
> manager :)).

I very much agree that we should be consistent. Consistency aside, I
would prefer *not* to introduce #xen,static-heap-address-cells and
#xen,static-heap-size-cells and instead reuse the regular #address-cells
and #size-cells. I think there is no reason why we shouldn't.

I was about to write something about it a couple of days ago but then I
noticed that we had already introduced #xen,static-mem-address-cells and
#xen,static-mem-size-cells. In order to be consistent I didn't say
anything and gave my ack.

But actually I think it is better to get rid of them all. I think we
should:

1) do not introduce #xen,static-heap-address-cells and
#xen,static-heap-size-cells in this series, instead rely on
#address-cells and #size-cells. Please write in the binding that the
number of address cells and size cells of xen,static-heap is determined
by the parent #address-cells and #size-cells. (It has to be the parent
because that is how #address-cells and #size-cells are defined.)

2) Also remove "#xen,static-mem-address-cells" and
"#xen,static-mem-size-cells", and also use #address-cells and
#size-cells for xen,static-mem as well. I think we should do that in
this release for consistency. Any volunteers? :-)

It is not going to break anything because, not only static-mem is tech
preview, but also it is very likely that if someone was using
#xen,static-heap-address-cells it would be setting it to the same value
as #address-cells. So in the vast majority of cases it would continue to
work as expected (not that we couldn't change it anyway, given that it
is a tech preview.)

So I am aligned with Julien on this.