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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 7 Sep 2022, Stefano Stabellini wrote:
> 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? :-)


I should also add that I don't think we should change the compatible
string for xen,static-mem. If the feature is tech preview, maybe we
can consider the device tree bindings tech preview as well? I think that
would be OK.

If you don't think is OK, then I suggest to:
- first check "#xen,static-mem-address-cells" and
  "#xen,static-mem-size-cells" for compatibility
- if missing, use the regular #address-cells and #size-cells

This way we retain compatibility. Either way, I wouldn't change the
compatible string for xen,static-mem.