lkml.org 
[lkml]   [2022]   [Aug]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/2] KVM: x86: Return emulator error if RDMSR/WRMSR emulation failed
    On Thu, Jul 28, 2022, Hou Wenlong wrote:
    > The return value of emulator_{get|set}_mst_with_filter()
    > is confused, since msr access error and emulator error
    > are mixed. Although, KVM_MSR_RET_* doesn't conflict with
    > X86EMUL_IO_NEEDED at present, it is better to convert
    > msr access error to emulator error if error value is
    > needed.
    >
    > Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
    > ---
    > arch/x86/kvm/x86.c | 22 ++++++++++++----------
    > 1 file changed, 12 insertions(+), 10 deletions(-)
    >
    > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
    > index 5366f884e9a7..8df89b9c212f 100644
    > --- a/arch/x86/kvm/x86.c
    > +++ b/arch/x86/kvm/x86.c
    > @@ -7908,11 +7908,12 @@ static int emulator_get_msr_with_filter(struct x86_emulate_ctxt *ctxt,
    > int r;
    >
    > r = kvm_get_msr_with_filter(vcpu, msr_index, pdata);
    > -
    > - if (r && kvm_msr_user_space(vcpu, msr_index, KVM_EXIT_X86_RDMSR, 0,
    > - complete_emulated_rdmsr, r)) {
    > - /* Bounce to user space */
    > - return X86EMUL_IO_NEEDED;
    > + if (r) {
    > + if (kvm_msr_user_space(vcpu, msr_index, KVM_EXIT_X86_RDMSR, 0,
    > + complete_emulated_rdmsr, r))
    > + r = X86EMUL_IO_NEEDED;
    > + else
    > + r = X86EMUL_UNHANDLEABLE;

    This should be X86EMUL_PROPAGATE_FAULT, X86EMUL_UNHANDLEABLE is used to indicate
    that KVM needs to bail all the way to userspace.

    I definitely like the idea of converting to X86EMUL_* here instead of spreading
    it across these helpers and the emulator, but in that case should convert _all_
    types.

    And I think it makes sense to opportunistically handle "r < 0" in the get helper.
    KVM may not return -errno today, but assuming that will always hold true is
    unnecessarily risking.

    E.g. what about:


    static int emulator_get_msr_with_filter(struct x86_emulate_ctxt *ctxt,
    u32 msr_index, u64 *pdata)
    {
    struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
    int r;

    r = kvm_get_msr_with_filter(vcpu, msr_index, pdata);
    if (r < 0)
    return X86EMUL_UNHANDLEABLE;

    if (r) {
    if (kvm_msr_user_space(vcpu, msr_index, KVM_EXIT_X86_RDMSR, 0,
    complete_emulated_rdmsr, r))
    return X86EMUL_IO_NEEDED;
    else
    return X86EMUL_PROPAGATE_FAULT;
    }

    return X86EMUL_CONTINUE;
    }

    static int emulator_set_msr_with_filter(struct x86_emulate_ctxt *ctxt,
    u32 msr_index, u64 data)
    {
    struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
    int r;

    r = kvm_set_msr_with_filter(vcpu, msr_index, data);
    if (r < 0)
    return X86EMUL_UNHANDLEABLE;

    if (r) {
    if (kvm_msr_user_space(vcpu, msr_index, KVM_EXIT_X86_WRMSR, data,
    complete_emulated_msr_access, r))
    return X86EMUL_IO_NEEDED;
    else
    return X86EMUL_PROPAGATE_FAULT;
    }

    return X86EMUL_CONTINUE;
    }


    Or maybe even add a helper to do the translation? Can't tell if this is a net
    positive or not. It's a bit gratuitous, but it does ensure consistent behavior
    for RDMSR vs. WRMSR.

    static int emulator_handle_msr_return(struct kvm_vcpu *vcpu *, int r,
    u32 msr, u64 data, u32 exit_reason,
    int (*comp)(struct kvm_vcpu *vcpu))
    {
    if (r < 0)
    return X86EMUL_UNHANDLEABLE;

    if (r) {
    if (kvm_msr_user_space(vcpu, msr, exit_reason, data, comp, r))
    return X86EMUL_IO_NEEDED;
    else
    return X86EMUL_UNHANDLEABLE;
    }

    return X86EMUL_CONTINUE;
    }

    static int emulator_get_msr_with_filter(struct x86_emulate_ctxt *ctxt,
    u32 msr_index, u64 *pdata)
    {
    struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
    int r;

    r = kvm_get_msr_with_filter(vcpu, msr_index, pdata);
    return emulator_handle_msr_return(vcpu, r, msr_index, 0,
    KVM_EXIT_X86_RDMSR,
    complete_emulated_rdmsr);
    }

    static int emulator_set_msr_with_filter(struct x86_emulate_ctxt *ctxt,
    u32 msr_index, u64 data)
    {
    struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
    int r;

    r = kvm_set_msr_with_filter(vcpu, msr_index, data);
    return emulator_handle_msr_return(vcpu, r, msr_index, data,
    KVM_EXIT_X86_WRMSR,
    complete_emulated_msr_access);
    }


    And then the emulator side of things can be:

    static int em_wrmsr(struct x86_emulate_ctxt *ctxt)
    {
    u64 msr_index = reg_read(ctxt, VCPU_REGS_RCX);
    u64 msr_data;
    int r;

    msr_data = (u32)reg_read(ctxt, VCPU_REGS_RAX)
    | ((u64)reg_read(ctxt, VCPU_REGS_RDX) << 32);
    r = ctxt->ops->set_msr_with_filter(ctxt, msr_index, msr_data);

    if (r == X86EMUL_PROPAGATE_FAULT)
    return emulate_gp(ctxt, 0);

    return r;
    }

    static int em_rdmsr(struct x86_emulate_ctxt *ctxt)
    {
    u64 msr_index = reg_read(ctxt, VCPU_REGS_RCX);
    u64 msr_data;
    int r;

    r = ctxt->ops->get_msr_with_filter(ctxt, msr_index, &msr_data);

    if (r == X86EMUL_PROPAGATE_FAULT)
    return emulate_gp(ctxt, 0);

    if (r == X86EMUL_CONTINUE) {
    *reg_write(ctxt, VCPU_REGS_RAX) = (u32)msr_data;
    *reg_write(ctxt, VCPU_REGS_RDX) = msr_data >> 32;
    }
    return r;
    }

    \
     
     \ /
      Last update: 2022-08-30 20:44    [W:3.060 / U:0.156 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site