Re: [PATCH v4 0/7] Static shared memory followup v2 - pt2

[Julien Grall <julien@xxxxxxx>]

Hi,

On 11/06/2024 13:42, Michal Orzel wrote:
> > We would like this serie to be in Xen 4.19, there was a misunderstanding on our 
> > side because we thought
> > that since the serie was sent before the last posting date, it could have been 
> > a candidate for merging in the
> > new release, instead after speaking with Julien and Oleksii we are now aware 
> > that we need to provide a
> > justification for its presence.
> >
> > Pros to this serie is that we are closing the circle for static shared memory, 
> > allowing it to use memory from
> > the host or from Xen, it is also a feature that is not enabled by default, so 
> > it should not cause too much
> > disruption in case of any bugs that escaped the review, however we’ve tested 
> > many configurations for that
> > with/without enabling the feature if that can be an additional value.
> >
> > Cons: we are touching some common code related to p2m, but also there the 
> > impact should be minimal because
> > the new code is subject to l2 foreign mapping (to be confirmed maybe from a p2m 
> > expert like Julien).
> >
> > The comments on patch 3 of this serie are addressed by this patch:
> > https://patchwork.kernel.org/project/xen-devel/patch/20240528125603.2467640-1-luca.fancellu@xxxxxxx/
> > And the serie is fully reviewed.
> >
> > So our request is to allow this serie in 4.19, Oleksii, ARM maintainers, do you 
> > agree on that?
> As a main reviewer of this series I'm ok to have this series in. It is nicely 
> encapsulated and the feature itself
> is still in unsupported state. I don't foresee any issues with it.
There are changes in the p2m code and the memory allocation for boot 
domains. So is it really encapsulated?
For me there are two risks:
 * p2m (already mentioned by Luca): We modify the code to put foreign 
mapping. The worse that can happen if we don't release the foreign 
mapping. This would mean the page will not be freed. AFAIK, we don't 
exercise this path in the CI.
 * domain allocation: This mainly look like refactoring. And the path 
is exercised in the CI.
So I am not concerned with the domain allocation one. @Luca, would it be 
possible to detail how did you test the foreign pages were properly removed?
Cheers,

--
Julien Grall