Re: [PATCH v2 3/6] KVM: VMX: Handle vectoring error in check_emulate_instruction

[Ivan Orlov <ivan.orlov0322@gmail.com>]

On 12/11/24 18:15, Sean Christopherson wrote:
> On Mon, Nov 11, 2024, Ivan Orlov wrote:
>> Move unhandleable vmexit due to MMIO during vectoring error detection
>> into check_emulate_instruction. Implement a function which checks if
>> emul_type indicates MMIO so it can be used for both VMX and SVM.
>>
>> Fix the comment about EMULTYPE_PF as this flag doesn't necessarily
>> mean MMIO anymore: it can also be set due to the write protection
>> violation.
>>
>> Signed-off-by: Ivan Orlov <iorlov@amazon.com>
>> ---
>> V1 -> V2:
>> - Detect the unhandleable vectoring error in vmx_check_emulate_instruction
>> instead of handling it in the common MMU code (which is specific for
>> cached MMIO)
>>
>>   arch/x86/include/asm/kvm_host.h | 10 ++++++++--
>>   arch/x86/kvm/vmx/vmx.c          | 25 ++++++++++++-------------
>>   2 files changed, 20 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index eb413079b7c6..3de9702a9135 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -2017,8 +2017,8 @@ u64 vcpu_tsc_khz(struct kvm_vcpu *vcpu);
>>    *			VMware backdoor emulation handles select instructions
>>    *			and reinjects the #GP for all other cases.
>>    *
>> - * EMULTYPE_PF - Set when emulating MMIO by way of an intercepted #PF, in which
>> - *		 case the CR2/GPA value pass on the stack is valid.
>> + * EMULTYPE_PF - Set when an intercepted #PF triggers the emulation, in which case
>> + *		 the CR2/GPA value pass on the stack is valid.
>>    *
>>    * EMULTYPE_COMPLETE_USER_EXIT - Set when the emulator should update interruptibility
>>    *				 state and inject single-step #DBs after skipping
>> @@ -2053,6 +2053,12 @@ u64 vcpu_tsc_khz(struct kvm_vcpu *vcpu);
>>   #define EMULTYPE_COMPLETE_USER_EXIT (1 << 7)
>>   #define EMULTYPE_WRITE_PF_TO_SP	    (1 << 8)
>>   
>> +static inline bool kvm_is_emul_type_mmio(int emul_type)
> 
> Hmm, this should probably be "pf_mmio", not just "mmio".  E.g. if KVM is emulating
> large swaths of guest code because unrestricted guest is disabled, then can end up
> emulating an MMIO access for "normal" emulation.
> 
> Hmm, actually, what if we go with this?
> 
>    static inline bool kvm_can_emulate_event_vectoring(int emul_type)
>    {
> 	return !(emul_type & EMULTYPE_PF) ||
> 	       (emul_type & EMULTYPE_WRITE_PF_TO_SP);
>    }
> 

I don't mind using either option here, in fact both of them would 
require an update if there is a new emulated page fault type; Let's use 
more generic one (which is kvm_can_emulate_event_vectoring) :)

I'm thinking about a static assert we could add to it, to be sure the 
condition gets updated if such an EMUL_TYPE is introduced, but I can't 
think of something neat... Anyway, it can be done in a separate patch I 
guess (if we really need it).

--
Kind regards,
Ivan Orlov