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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.08.2025 01:41, Stefano Stabellini wrote:
> 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:
>>>>> --- 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.

Originally Penny introduced the option with prompt, very early in the
series. It became clear very quickly that this way she introduced
randconfig issues, for the case where randconfig could have chosen to
turn the option off. Hence why I asked that the option be introduced
prompt-less, then all #ifdef-ary be added, and then the option would
gain a prompt.

>>> 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.

The code working correctly will be tested at the point the option gains
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.
>>
>> 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.

Yes, hence why we wouldn't need a revert of the entire series.

Jan