> On May 25, 2022, at 1:04 PM, Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > On Fri, May 20, 2022, Jon Kohler wrote: >> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c >> index 610355b9ccce..1c725d17d984 100644 >> --- a/arch/x86/kvm/vmx/vmx.c >> +++ b/arch/x86/kvm/vmx/vmx.c >> @@ -2057,20 +2057,32 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) >> return 1; >> >> vmx->spec_ctrl = data; >> - if (!data) >> + >> + /* >> + * Disable interception on the first non-zero write, unless the >> + * guest is hosted on an eIBRS system and setting only > > The "unless guest is hosted on an eIBRS system" blurb is wrong and doesn't match Ah right, thanks for catching that > the code. Again, it's all about whether eIBRS is advertised to the guest. With > some other minor tweaking to wrangle the comment to 80 chars... RE 80 chars - quick question (and forgive the silly question here), but how are you counting that? I’ve got my editor cutting at 79 cols, where tab size is accounted for as 4 cols, so the longest line on my side for this patch is 72-73 or so. These also pass the checkpatch.pl script as well, so I just want to make sure going forward I’m formatting them appropriately. Let me know and I’ll incorporate all of this into a v4 after I hear back :) > > /* > * Disable interception on the first non-zero write, except if > * eIBRS is advertised to the guest and the guest is enabling > * _only_ IBRS. On eIBRS systems, kernels typically set IBRS > * once at boot and never touch it post-boot. All other bits, > * and IBRS on non-eIBRS systems, are often set on a per-task > * basis, i.e. change frequently, so the benefit of avoiding > * VM-exits during guest context switches outweighs the cost of > * RDMSR on every VM-Exit to save the guest's value. > */ Thanks for the suggestion, this text works for me > >> + * SPEC_CTRL_IBRS, which is typically set once at boot and never > > Uber nit, when it makes sense, avoid regurgitating the code verbatim, e.g. refer > to "setting SPEC_CTRL_IBRS" as "enabling IBRS". That little bit of abstraction > can sometimes help unfamiliar readers understand the effect of the code, whereas > copy+pasting bits of the code doesn't provide any additional context. Ok thanks for this as well, I appreciate the feedback! > >> + * touched again. All other bits are often set on a per-task >> + * basis, i.e. may change frequently, so the benefit of avoiding >> + * VM-exits during guest context switches outweighs the cost of >> + * RDMSR on every VM-Exit to save the guest's value. >> + */ >> + if (!data || >> + (data == SPEC_CTRL_IBRS && >> + (vcpu->arch.arch_capabilities & ARCH_CAP_IBRS_ALL))) > > Align the two halves of the logical-OR, i.e. > > if (!data || > (data == SPEC_CTRL_IBRS && > (vcpu->arch.arch_capabilities & ARCH_CAP_IBRS_ALL))) > break; Ack on this
