On 2/21/24 07:43, Sean Christopherson wrote: > On Tue, Feb 20, 2024, Dongli Zhang wrote: >> Hi Sean, >> >> On 2/19/24 14:33, Sean Christopherson wrote: >>> On Fri, Feb 16, 2024, Dongli Zhang wrote: >>>> --- >>>> arch/x86/kvm/vmx/vmx.c | 55 +++++++++++++++++++++--------------------- >>>> 1 file changed, 28 insertions(+), 27 deletions(-) >>>> >>>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c >>>> index 5a866d3c2bc8..76dff0e7d8bd 100644 >>>> --- a/arch/x86/kvm/vmx/vmx.c >>>> +++ b/arch/x86/kvm/vmx/vmx.c >>>> @@ -669,14 +669,18 @@ static int possible_passthrough_msr_slot(u32 msr) >>>> return -ENOENT; >>>> } >>>> >>>> -static bool is_valid_passthrough_msr(u32 msr) >>>> +#define VMX_POSSIBLE_PASSTHROUGH 1 >>>> +#define VMX_OTHER_PASSTHROUGH 2 >>>> +/* >>>> + * Vefify if the msr is the passthrough MSRs. >>>> + * Return the index in *possible_idx if it is a possible passthrough MSR. >>>> + */ >>>> +static int validate_passthrough_msr(u32 msr, int *possible_idx) >>> >>> There's no need for a custom tri-state return value or an out-param, just return >>> the slot/-ENOENT. Not fully tested yet, but this should do the trick. >> >> The new patch looks good to me, from functionality's perspective. >> >> Just that the new patched function looks confusing. That's why I was adding the >> out-param initially to differentiate from different cases. >> >> The new vmx_get_passthrough_msr_slot() is just doing the trick by combining many >> jobs together: >> >> 1. Get the possible passthrough msr slot index. >> >> 2. For x2APIC/PT/LBR msr, return -ENOENT. >> >> 3. For other msr, return the same -ENOENT, with a WARN. >> >> The semantics of the function look confusing. >> >> If the objective is to return passthrough msr slot, why return -ENOENT for >> x2APIC/PT/LBR. > > Because there is no "slot" for them in vmx_possible_passthrough_msrs, and the > main purpose of the helpers is to get that slot in order to efficiently update > the MSR bitmaps in response to userspace MSR filter changes. The WARN is an extra > sanity check to ensure that KVM doesn't start passing through an MSR without > adding the MSR to vmx_possible_passthrough_msrs (or special casing it a la XAPIC, > PT, and LBR MSRS). > >> Why both x2APIC/PT/LBR and other MSRs return the same -ENOENT, while the other >> MSRs may trigger WARN. (I know this is because the other MSRs do not belong to >> any passthrough MSRs). > > The x2APIC/PT/LBR MSRs are given special treatment: KVM may pass them through to > the guest, but unlike the "regular" passthrough MSRs, userspace is NOT allowed to > override that behavior via MSR filters. > > And so as mentioned above, they don't have a slot in vmx_possible_passthrough_msrs. Thank you very much for the explanation! This looks good to me. Dongli Zhang
