Re: [PATCH] xen/x86: move domctl.o out of PV_SHIM_EXCLUSIVE

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 28 Aug 2025, Jan Beulich wrote:
> On 28.08.2025 02:52, Stefano Stabellini wrote:
> > 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.
> 
> Except that, as said, conceptually "true" is the wrong value to use in
> such a stub.
> 
> >> 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.
> 
> If that's your view, then why did you not comment on the SYSCTL series,
> when I asked the prompt to appear last?

I am not trying to be obtuse, but I am not sure what you mean by this.
In any case, I do not recall reading a specific email on this topic. I
try my best to follow other review comments, but I may have overlooked
this one.


> > An extreme example is that I could write:
> > 
> > static inline bool domctl_lock_acquire(void)
> > {
> >     obviously broken
> > }
> > 
> > and no tests would catch it.
> 
> Tests would catch it at the point the prompt is added. Much like it was
> with the SYSCTL series (and why, with the prompt removed, the rest of
> the series can stay in).

In my opinion, this is a no-go. The code must function correctly, and
that is my top priority, certainly above conceptual issues, such as the
return value of domctl_lock_acquire. With your suggestion, there is no
guarantee the code works and there is no way to test it.


> >> 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.
> 
> If "this patch" is the one outlined here, then with the reverts we wouldn't
> need it at all. The reverts alone will fix the build issue, according to my
> understanding.

The reverts you are suggesting do not fix the issue; they only hide it.
The Kconfig option can no longer be disabled, which renders the entire
patch series ineffective. In fact, the #ifdef's could be completely
incorrect, and the code would still build, as in my example:

static inline bool domctl_lock_acquire(void)
{
    obviously broken
}

It looks like we'll need a third opinion to make forward progress.