Re: [XEN PATCH v2 12/15] x86/vmx: guard access to cpu_has_vmx_* in common code

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.05.2024 12:58, Sergiy Kibrik wrote:
> 16.05.24 10:32, Jan Beulich:
>> On 16.05.2024 02:50, Stefano Stabellini wrote:
>>> On Wed, 15 May 2024, Sergiy Kibrik wrote:
>>>> There're several places in common code, outside of arch/x86/hvm/vmx,
>>>> where cpu_has_vmx_* get accessed without checking if VMX present first.
>>>> We may want to guard these macros, as they read global variables defined
>>>> inside vmx-specific files -- so VMX can be made optional later on.
>>>>
>>>> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@xxxxxxxx>
>>>> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>> CC: Jan Beulich <jbeulich@xxxxxxxx>
>>>> ---
>>>> Here I've tried a different approach from prev.patches [1,2] -- instead of
>>>> modifying whole set of cpu_has_{svm/vmx}_* macros, we can:
>>>>   1) do not touch SVM part at all, because just as Andrew pointed out 
>>>> they're
>>>> used inside arch/x86/hvm/svm only.
>>>>   2) track several places in common code where cpu_has_vmx_* features are
>>>> checked out and guard them using cpu_has_vmx condition
>>>>   3) two of cpu_has_vmx_* macros being used in common code are checked in 
>>>> a bit
>>>> more tricky way, so instead of making complex conditionals even more 
>>>> complicated,
>>>> we can instead integrate cpu_has_vmx condition inside these two macros.
>>>>
>>>> This patch aims to replace [1,2] from v1 series by doing steps above.
>>>>
> [..]
>>>
>>> I am missing some of the previous discussions but why can't we just fix
>>> all of the cpu_has_vmx_* #defines in vmcs.h to also check for
>>> cpu_has_vmx?
>>>
>>> That seems easier and simpler than to add add-hoc checks at the invocations?
>>
>> I'd like to take the question on step further: Following 0b5f149338e3
>> ("x86/HVM: hide SVM/VMX when their enabling is prohibited by firmware"),
>> is this change needed at all? IOW is there a path left where cpu_has_vmx
>> may be false, by any cpu_has_vmx_* may still yield true?
>>
> 
> This change is about exec control variables (vmx_secondary_exec_control, 
> vmx_pin_based_exec_control etc) not been built, because they're in vmx 
> code, but accessed in common code. The description is probably unclear 
> about that.
> Also build issues related to VMX can be solved differently, without 
> touching cpu_has_vmx_* macros and related logic at all.
> I can move exec control variables from vmcs.c to common hvm.c, this 
> would be simpler change and directly related to problem that I'm having.

That would be moving them one layer too high. Proper disentangling then
will need to wait until that data is actually part of the (host) CPU
policy. For the time being your change may thus be acceptable, assuming
that we won't be very quick in doing said move.

Jan