Re: [PATCH 1/3] x86/mem_sharing: option to skip populating special pages during fork

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Fri, Mar 25, 2022, 6:25 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:

On Thu, Mar 24, 2022 at 12:27:02PM -0400, Tamas K Lengyel wrote:
> On Thu, Mar 24, 2022 at 11:51 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> >
> > On Thu, Mar 24, 2022 at 11:15:08AM -0400, Tamas K Lengyel wrote:
> > > On Thu, Mar 24, 2022 at 10:50 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> > > >
> > > > On Tue, Mar 22, 2022 at 01:41:37PM -0400, Tamas K Lengyel wrote:
> > > > > +    {
> > > > > +        cd->arch.hvm.mem_sharing.block_interrupts = block_interrupts;
> > > > > +        cd->arch.hvm.mem_sharing.skip_special_pages = skip_special_pages;
> > > > > +        /* skip mapping the vAPIC page on unpause if skipping special pages */
> > > > > +        cd->creation_finished = skip_special_pages;
> > > >
> > > > I think this is dangerous. While it might be true at the moment that
> > > > the arch_domain_creation_finished only maps the vAPIC page, there's no
> > > > guarantee it couldn't do other stuff in the future that could be
> > > > required for the VM to be started.
> > >
> > > I understand this domain_creation_finished route could be expanded in
> > > the future to include other stuff, but IMHO we can evaluate what to do
> > > about that when and if it becomes necessary. This is all experimental
> > > to begin with.
> >
> > Maybe you could check the skip_special_pages field from
> > domain_creation_finished in order to decide whether the vAPIC page
> > needs to be mapped?
>
> Could certainly do that but it means adding experimental code in an
> #ifdef to the vAPIC mapping logic. Technically nothing wrong with that
> but I would prefer keeping all this code in a single-place if
> possible.

I see, while I agree it's best to keep the code contained when
possible, I think in this case it's better to modify the hook,
specially because further changes to domain_creation_finished will
easily spot that this path is special cases for VM forks.

While the code is experimental it doesn't mean it shouldn't be
properly integrated with the rest of the code base.

Sure, I'm fine with moving it there.

Tamas