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....@citrix.com>
>>
>> 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
