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.

Oh, one more thing: SAVE_RESTORE, simply by its name, promises to cover PV as 
well.
That either wants to be the case, or the name may want adjusting.

Jan