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

[Julien Grall <julien@xxxxxxx>]

On 14/06/2024 08:54, Luca Fancellu wrote:
> Hi Julien,
Hi Luca,

> > On 13 Jun 2024, at 12:31, Julien Grall <julien@xxxxxxx> wrote:
> >
> > 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?
> So at first we tested the code, with/without the static shared memory feature 
> enabled, creating/destroying guest from Dom0 and checking that everything
> was ok.
Just to details a bit more for the other. In a normal setup, guest would 
be using PV block/network which are based on grant table. To exercise 
the foreing mapping path, you would need a backend that map using the 
GFN directly.
This is the case for virtio MMIO. Other users are IIRC XenFB, IOREQ (if 
you emulate a device).
> After a chat on Matrix with Julien he suggested that using a virtio-mmio disk 
> was better to stress out the foreign mapping looking for
> regressions.
>
> Luckily I’ve found this slide deck from @Oleksandr: 
> https://static.linaro.org/connect/lvc21/presentations/lvc21-314.pdf
>
> So I did a setup using fvp-base, having a disk with two partitions containing 
> Dom0 rootfs and DomU rootfs, Dom0 sees
> this disk using VirtIO block.
Thanks for the testing! I am satisfied with the result. I have committed 
the series.
Cheers,

--
Julien Grall