On Fri, Mar 25, 2022, 6:25 AM Roger Pau Monné <roger....@citrix.com> 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....@citrix.com>
> 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....@citrix.com> 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
>
