Re: [PATCH 3/4] xen/arm, libxl: Revert XEN_DOMCTL_shadow_op; use p2m mempool hypercalls

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 16 Nov 2022, Julien Grall wrote:
> On 16/11/2022 23:44, Stefano Stabellini wrote:
> > On Wed, 16 Nov 2022, Andrew Cooper wrote:
> > > On 16/11/2022 08:30, Jan Beulich wrote:
> > > > On 16.11.2022 03:00, Stefano Stabellini wrote:
> > > > > On Wed, 16 Nov 2022, Andrew Cooper wrote:
> > > > > > On 16/11/2022 01:37, Stefano Stabellini wrote:
> > > > > > > On Wed, 26 Oct 2022, Andrew Cooper wrote:
> > > > > > > > This reverts most of commit
> > > > > > > > cf2a68d2ffbc3ce95e01449d46180bddb10d24a0, and bits
> > > > > > > > of cbea5a1149ca7fd4b7cdbfa3ec2e4f109b601ff7.
> > > > > > > > 
> > > > > > > > First of all, with ARM borrowing x86's implementation, the logic
> > > > > > > > to set the
> > > > > > > > pool size should have been common, not duplicated.  Introduce
> > > > > > > > libxl__domain_set_p2m_pool_size() as a shared implementation,
> > > > > > > > and use it from
> > > > > > > > the ARM and x86 paths.  It is left as an exercise to the reader
> > > > > > > > to judge how
> > > > > > > > libxl/xl can reasonably function without the ability to query
> > > > > > > > the pool size...
> > > > > > > > 
> > > > > > > > Remove ARM's p2m_domctl() infrastructure now the functioanlity
> > > > > > > > has been
> > > > > > > > replaced with a working and unit tested interface.
> > > > > > > > 
> > > > > > > > This is part of XSA-409 / CVE-2022-33747.
> > > > > > > Genuine question: I can see this patch removes the implementation
> > > > > > > of
> > > > > > > XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION on ARM. It also switches libxl
> > > > > > > (both
> > > > > > > ARM and x86) to the new hypercall.
> > > > > > > 
> > > > > > > Why keep the old hypercall (XEN_DOMCTL_shadow_op and
> > > > > > > XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION) implementation on x86 (not on
> > > > > > > ARM)?
> > > > > > > 
> > > > > > > Is that because it was only recently implemented? And not actually
> > > > > > > present in any past Xen release?
> > > > > > > 
> > > > > > > If so, please add a note about this in the commit message. Also,
> > > > > > > if that
> > > > > > > is the case, I think this patch series should go in 4.17. If it is
> > > > > > > too
> > > > > > > late to get it in before the release, then we should backport it
> > > > > > > to 4.17
> > > > > > > as soon as possible. That's because ideally we want to keep the
> > > > > > > hypercall interface changes down to a minimum.
> > > > > > On ARM, the hypercall has existed for a little over 4 weeks, and
> > > > > > isn't
> > > > > > in any released version of Xen (yet).
> > > > > > 
> > > > > > On x86, the hypercall has existed for more than a decade, and has
> > > > > > known
> > > > > > out-of-tree users.  It needs to be deprecated properly, which in
> > > > > > this
> > > > > > case means "phased out in the 4.18 cycle once known callers have
> > > > > > been
> > > > > > adapted to the new hypercall".
> > > > > Understoon. Then I am in favor of getting all 4 patches in 4.17,
> > > > > either
> > > > > before the release or via backports.
> > > > Removing something from the domctl interface generally requires bumping
> > > > the interface version, so some extra care may need applying if such an
> > > > interface change was to be backported to any stable branch.
> > > 
> > > To be clear, I have no plans to remove the x86 "older" interface in this
> > > patch series.  It will definitely break out of tree users.
> > > 
> > > In the 4.18 timeframe, we can see about retiring the older hypercalls,
> > > but as a non-backportable change.
> > 
> > For ARM, given that XEN_DOMCTL_shadow_op has not been enabled for long,
> > maybe we can get away without bumping the interface version?
> 
> IMHO how long it was out doesn't matter. Once we do a release, we should avoid
> changing the interface in minor version.
> 
> This is because a user may start to rely on it and we don't want to break them
> for minor releases.

We haven't released 4.17 yet, so I take you are referring to a stable
minor release, right?