Re: [XEN PATCH v2 1/2] x86/hvm: introduce config option for ACPI PM timer

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.11.2024 12:01, Sergiy Kibrik wrote:
> 11.11.24 11:51, Jan Beulich:
>> On 06.11.2024 11:14, Sergiy Kibrik wrote:
>>> Introduce config option X86_HVM_PMTIMER and make pmtimer emulation driver
>>> configurable and possible to disable on systems that don't need it.
>>> Option X86_X86_HVM_PMTIMER depends on HVM option, because this driver is 
>>> part
>>> of HVM support code.
>>>
>>> Introduced additional check of domain's emulation flags, to cover the case
>>> when user explicitly states the requirement of emulated devices that are
>>> disabled in the build. HVM always require these devices to be present so 
>>> domains
>>> of this type can't be created when pmtimer or any other emulated device are
>>> disabled.
>>>
>>> Suggested-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>
>> What exactly was it that Roger suggested? I don't think it was what the patch
>> does overall, but just _how_ it is being done? That makes quite a bit of a
>> difference, as the former could be read as kind of an implicit ack to what is
>> being done here (and also in the other patch). Issue is: I remain unconvinced
>> that this conditionalizing is actually something we really want/need.
> 
> about a half of this patch is what Roger suggested. These changes were 
> in a separate patch, which Roger suggested to be merged into other 
> patches. What tag should be put in this case then?

The tag itself is fine, but could do with clarifying by way of attaching
"# <brief>", like we also permit for R-b and A-b. Alternatively a post-
commit-message remark would help during review (but notably not once the
change would have been committed, e.g. for archaeologists).

>>> --- a/xen/arch/x86/Kconfig
>>> +++ b/xen/arch/x86/Kconfig
>>> @@ -144,6 +144,19 @@ config INTEL_VMX
>>>       If your system includes a processor with Intel VT-x support, say Y.
>>>       If in doubt, say Y.
>>>   
>>> +menu "Emulated HVM devices support"
>>> +       visible if EXPERT
>>> +       depends on HVM
>>> +
>>> +config X86_HVM_PMTIMER
>>> +   bool "ACPI PM timer emulation support"
>>> +   default y
>>> +   help
>>> +     Build pmtimer driver that emulates ACPI PM timer for HVM/PVH guests.
>>
>> Does this really affect PVH guests? Isn't the whole point of the change
>> that in a PVH-only environment this wouldn't be needed in Xen?
> 
> PVH guest may (depending on its configuration) still use PM timer, so 
> I'd say the point is in a PVH-only environment this driver becomes optional.

Hmm, the way I look at emulation_flags_ok() it doesn't look to permit this
as optional. The PVH case is "emflags == X86_EMU_LAPIC".

>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -742,11 +742,16 @@ int arch_sanitise_domain_config(struct 
>>> xen_domctl_createdomain *config)
>>>   
>>>   static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
>>>   {
>>> -#ifdef CONFIG_HVM
>>> +    const uint32_t disabled_emu_mask = X86_EMU_PM;
>>> +
>>> +#if defined(CONFIG_X86_HVM_PMTIMER)
>>>       /* This doesn't catch !CONFIG_HVM case but it is better than nothing 
>>> */
>>>       BUILD_BUG_ON(X86_EMU_ALL != XEN_X86_EMU_ALL);
>>>   #endif
>>>   
>>> +    if ( emflags & disabled_emu_mask )
>>> +        return false;
>>> +
>>>       if ( is_hvm_domain(d) )
>>>       {
>>>           if ( is_hardware_domain(d) &&
>>
>> While you commented on this hunk, it didn't become clear what exactly the
>> resulting new hunk would be. I question in particular the change to the
>> #ifdef: If that's changed and the BUILD_BUG_ON() kept as is, the comment
>> also needs adjusting. Yet it would perhaps be better of the BUILD_BUG_ON()
>> was split accordingly.
>>
> 
> This #ifdef definitely wants nicer change. How would you suggest 
> BUILD_BUG_ON() be split?

#ifdef CONFIG_HVM
       BUILD_BUG_ON(X86_EMU_ALL & ~X86_EMU_PM !=
                    XEN_X86_EMU_ALL & ~XEN_X86_EMU_PM);
#endif
#ifdef CONFIG_X86_HVM_PMTIMER
       BUILD_BUG_ON(X86_EMU_PM != XEN_X86_EMU_PM);
#endif

Jan