On Wed, 27 Aug 2025, Jan Beulich wrote:
> On 27.08.2025 02:33, Stefano Stabellini wrote:
> > So I ran a test and the appended change, which is based on [1] and
> > renaming CONFIG_DOMCTL to CONFIG_SYSCTL, is sufficient to resolve the
> > build issue.
> >
> > For 4.21, I suggest we go with two patches:
> > 1) global rename of CONFIG_SYSCTL to CONFIG_MGMT_HYPERCALLS
> > 2) stub domctl_lock_acquire/release based on CONFIG_MGMT_HYPERCALLS
> >
> > Jan, are you OK with this?
>
> Naming if the option aside, no, I fear I dislike the stubbing. What's
> worse though, ...
>
> > --- a/xen/include/xen/domain.h
> > +++ b/xen/include/xen/domain.h
> > @@ -148,8 +148,17 @@ void arch_dump_domain_info(struct domain *d);
> >
> > int arch_vcpu_reset(struct vcpu *v);
> >
> > +#ifdef CONFIG_SYSCTL
> > bool domctl_lock_acquire(void);
> > void domctl_lock_release(void);
> > +#else
> > +static inline bool domctl_lock_acquire(void)
> > +{
> > + return false;
>
> ... this will break x86'es HVM_PARAM_IDENT_PT handling. That is, in
> principle I would agree that returning false here is appropriate. But
> for the specific case there it's wrong.
Uhm, that is a good point actually. And while in principle "false"
sounds appropriate, in practice there is no domctl.c to worry about
concurrency so "true" is what we want.
> As said on the call yesterday, until what you call MGMT_HYPERCALLS is
> completely done, the option needs to be prompt-less, always-on.
I do not think this is a good idea, because we would be unable to test
the configuration. Although we have been accepting code without tests,
that is not a good principle. At least with the current approach we can
run manual tests if automated tests are not available. If we make it
silent, we risk introducing broken code, or code soon-to-become broken.
In my view, we need to make gradual progress toward the goal. In this
case, we should move incrementally toward compiling out all the
"management" hypercalls. Also the alternative of waiting until all
patches are ready before committing them is not feasible. An incremental
approach reduces risk, preserves testability, and makes regressions
easier to identify.
An extreme example is that I could write:
static inline bool domctl_lock_acquire(void)
{
obviously broken
}
and no tests would catch it.
So in short, I think we need to keep the prompt.
> Adding
> a prompt was necessary to be the last thing on the SYSCTL series, and
> it'll need to be last on the follow-on one masking out further
> hypercalls. IOW my take is that 34317c508294 and 568f806cba4c will
> need reverting (the latter being what caused the regression, and the
> former depending on the latter), to allow to cleanly continue that
> work after the rename. If we don't do the reverts now (and take either
> Penny's patch or what you propose), imo we'll need to do them later.
> Else we're risking to introduce new randconfig breakages while the
> further conversion work is ongoing.
My suggestion remains to go forward with 2 patches:
0) keep both 568f806cba4c and 34317c508294
1) rename CONFIG_SYSCTL to CONFIG_MGMT_HYPERCALLS
2) this patch with return true from domctl_lock_acquire
I am open to reverting 568f806cba4c but I don't think it would improve
things. I definitely don't think we should revert 34317c508294. We need
34317c508294 otherwise this patch doesn't fix the build.
This is why I think we need the prompt: otherwise we would not discover
even very basic important build breakages.
