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

Ivan Orlov
2024-12-11T22:05:45Z

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