Re: [PATCH v4 19/24] xen/domctl: wrap hvm_save{,load} with CONFIG_MGMT_HYPERCALLS

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.11.2025 17:30, Grygorii Strashko wrote:
> On 25.11.25 17:59, Jan Beulich wrote:
>> On 21.11.2025 11:57, Penny Zheng wrote:
>>> The following functions have been referenced in places which is either 
>>> guarded
>>> with CONFIG_MGMT_HYPERCALLS or CONFIG_MEM_SHARING:
>>> - arch_hvm_save
>>> - arch_hvm_check
>>> - arch_hvm_load
>>> - hvm_save_size
>>> - hvm_save
>>> - hvm_load
>>> While CONFIG_MEM_SHARING is also dependent on CONFIG_MGMT_HYPERCALLS.
>>> So they shall be wrapped under MGMT_HYPERCALLS, otherwise they will become
>>> unreachable codes when MGMT_HYPERCALLS=n, and hence violating Misra rule 
>>> 2.1.
>>> We move arch_hvm_save(), arch_hvm_check(), arch_hvm_load() and 
>>> hvm_save_size()
>>> nearer to the left functions, to avoid scattered #ifdef-wrapping.
>>
>> Why would you move things? What is in this source file that is of any use 
>> when
>> MGMT_HYPERCALLS=n? The only caller of hvm_save_one() lives in x86/domctl.c. 
>> With
>> that also removed, hvm_sr_handlers[] is only ever written to afaict, which 
>> means
>> that's an effectively dead array then too.
>>
>> The final few functions ...
>>
>>> @@ -390,6 +391,7 @@ int hvm_load(struct domain *d, bool real, 
>>> hvm_domain_context_t *h)
>>>   
>>>       /* Not reached */
>>>   }
>>> +#endif /* CONFIG_MGMT_HYPERCALLS */
>>>   
>>>   int _hvm_init_entry(struct hvm_domain_context *h, uint16_t tc, uint16_t 
>>> inst,
>>>                       uint32_t len)
>>
>> ... here and below are only helpers for the save/restore machinery, i.e. that
>> _all_ is also usable only when MGMT_HYPERCALLS=y. Yes, that's a lot of 
>> further
>> work, but what do you do? You'll then have quite a bit more code removed from
>> the set that as per coverage analysis is never reached.
> 
> I have a local patch which allows to disable all HVM save/load code at once 
> by using
> separated Kconfig option SAVE_RESTORE.
> 
> +++ b/xen/arch/x86/hvm/Kconfig
> @@ -127,4 +127,8 @@ config VHPET
> 
> +config SAVE_RESTORE
> +    depends on MGMT_HYPERCALLS
> +    def_bool y
> 
> SAVE_RESTORE - annotates all HVM save/load code and, in general, could made a 
> feature by
> allowing it to be selectable.
> Of course, It all can be done by just using MGMT_HYPERCALLS.
> 
> So, I'd be appreciated for you opinion - does it make sense to have separate 
> SAVE_RESTORE?

Yes, why not? The granularity of MGMT_HYPERCALLS is getting a little unwieldy
anyway, so why not leverage what you have to split it up at least some. (Of
course much depends on how intrusive that change is. Then again the same
intrusiveness would have to be expected if it all went under MGMT_HYPERCALLS.)

Jan