Re: [PATCH v8 7/7] xen/sysctl: wrap around sysctl hypercall

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.07.2025 08:59, Penny, Zheng wrote:
> [Public]
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Tuesday, July 22, 2025 1:33 PM
>> To: Penny, Zheng <penny.zheng@xxxxxxx>
>> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Stefano Stabellini
>> <sstabellini@xxxxxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; Roger
>> Pau Monné <roger.pau@xxxxxxxxxx>; Anthony PERARD
>> <anthony.perard@xxxxxxxxxx>; Orzel, Michal <Michal.Orzel@xxxxxxx>; Julien
>> Grall <julien@xxxxxxx>; Sergiy Kibrik <Sergiy_Kibrik@xxxxxxxx>; xen-
>> devel@xxxxxxxxxxxxxxxxxxxx; Stabellini, Stefano <stefano.stabellini@xxxxxxx>
>> Subject: Re: [PATCH v8 7/7] xen/sysctl: wrap around sysctl hypercall
>>
>> On 22.07.2025 07:05, Penny, Zheng wrote:
>>> [Public]
>>>
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@xxxxxxxx>
>>>> Sent: Thursday, July 17, 2025 4:55 PM
>>>> To: Penny, Zheng <penny.zheng@xxxxxxx>; Stabellini, Stefano
>>>> <stefano.stabellini@xxxxxxx>
>>>> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Stefano Stabellini
>>>> <sstabellini@xxxxxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>;
>>>> Roger Pau Monné <roger.pau@xxxxxxxxxx>; Anthony PERARD
>>>> <anthony.perard@xxxxxxxxxx>; Orzel, Michal <Michal.Orzel@xxxxxxx>;
>>>> Julien Grall <julien@xxxxxxx>; Sergiy Kibrik
>>>> <Sergiy_Kibrik@xxxxxxxx>; xen- devel@xxxxxxxxxxxxxxxxxxxx
>>>> Subject: Re: [PATCH v8 7/7] xen/sysctl: wrap around sysctl hypercall
>>>>
>>>> On 11.07.2025 06:31, Penny Zheng wrote:
>>>>> --- a/xen/common/Makefile
>>>>> +++ b/xen/common/Makefile
>>>>> @@ -49,6 +49,7 @@ obj-y += spinlock.o
>>>>>  obj-$(CONFIG_STACK_PROTECTOR) += stack-protector.o  obj-y +=
>>>>> stop_machine.o  obj-y += symbols.o
>>>>> +obj-$(CONFIG_SYSCTL) += sysctl.o
>>>>>  obj-y += tasklet.o
>>>>>  obj-y += time.o
>>>>>  obj-y += timer.o
>>>>> @@ -70,7 +71,6 @@ obj-$(CONFIG_COMPAT) += $(addprefix
>>>>> compat/,domain.o memory.o multicall.o xlat.o  ifneq
>>>>> ($(CONFIG_PV_SHIM_EXCLUSIVE),y) obj-y += domctl.o
>>>>>  obj-$(CONFIG_VM_EVENT) += monitor.o -obj-y += sysctl.o  endif
>>>>>
>>>>>  extra-y := symbols-dummy.o
>>>>
>>>> CI demonstrates that this combination of changes is wrong. The job
>>>> that failed
>>>> (debian-12-x86_64-gcc-ibt) is a randconfig one, and ended up picking
>>>> both SYSCTL=y and PV_SHIM_EXCLUSIVE=y. Which results in sysctl.c
>>>> being built, but domctl.c not being built. Which leaves
>>>> getdomaininfo() undefined, causing linking to fail. In case the next
>>>> pipeline also ends up failing, I'll simply revert that change. In
>>>> case it succeeds, not reverting may be an option, as long as a proper fix 
>>>> shows
>> up pretty quickly.
>>>
>>> I've push commit of " xen: move getdomaininfo() to domain.c " to try to fix 
>>> the
>> error.
>>
>> And you're reasonably certain that's the only issue? I ask because it is the 
>> nature of
>> randconfig to pick random combinations of settings; on a later pipeline I 
>> had seen a
>> different failure. I didn't look at that in detail (it may have had to do 
>> with the domctl
>> lock or something vaguely similar), which perhaps was a mistake.
> 
> I turned on HVM, HYPERV_GUEST, and VGA when PV_SHIM_EXCLUSIVE is on to track 
> down more linking issues
> All undefined link failure is due to removing PV_SHIM_EXCLUSIVE dependency 
> for CONFIG_HVM, like monitor_traps, domctl_lock_acquire/domctl_lock_release, 
> etc
> I suggest to move domctl_lock_acquire/domctl_lock_release out of domctl.c 
> too, and also "obj-$(CONFIG_VM_EVENT) += monitor.o" out of PV_SHIM_EXCLUSIVE 
> guard

I'm not convinced of that approach; I'm curious what others think. One 
alternative
would appear to be to have SYSCTL depend on DOMCTL (the latter yet to be 
introduced).
As it stands, my vote would go towards reverting the final one or two patches. 
It's
not entirely clear to me though why you say ...

> And above change, fwit, is to fix commit of " xen/x86: remove "depends on 
> !PV_SHIM_EXCLUSIVE"  "

... this. My understanding is that the issue became manifest with "xen/sysctl: 
wrap
around sysctl hypercall", i.e. reverting just that one would suffice at least 
for
the getdomaininfo() issue. Yet then "above" is ambiguous here, and hence may 
mean
the domctl lock issue. Which then would indeed suggest we need to revert both.

Jan