On Sun, Nov 03, 2024, Manali Shukla wrote:
> On 10/15/2024 11:19 PM, Sean Christopherson wrote:
> > On Fri, Oct 04, 2024, Manali Shukla wrote:
> ...
> >>
> >> +static int bus_lock_exit(struct kvm_vcpu *vcpu)
> >> +{
> >> + struct vcpu_svm *svm = to_svm(vcpu);
> >> +
> >> + vcpu->run->exit_reason = KVM_EXIT_X86_BUS_LOCK;
> >> + vcpu->run->flags |= KVM_RUN_X86_BUS_LOCK;
> >> +
> >> + /*
> >> + * Reload the counter with value greater than '0'.
> >
> > The value quite obviously must be exactly '1', not simply greater than '0. I also
> > think this is the wrong place to set the counter. Rather than set the counter at
> > the time of exit, KVM should implement a vcpu->arch.complete_userspace_io callback
> > and set the counter to '1' if and only if RIP (or LIP, but I have no objection to
> > keeping things simple) is unchanged. It's a bit of extra complexity, but it will
> > make it super obvious why KVM is setting the counter to '1'. And, if userspace
> > wants to stuff state and move past the instruction, e.g. by emulating the guilty
> > instruction, then KVM won't unnecessarily allow a bus lock in the guest.
> >
> > And then the comment can be:
> >
> > /*
> > * If userspace has NOT change RIP, then KVM's ABI is to let the guest
> > * execute the bus-locking instruction. Set the bus lock counter to '1'
> > * to effectively step past the bus lock.
> > */
> >
>
> The bus lock threshold intercept feature is available for SEV-ES and SEV-SNP
> guests too. The rip where the bus lock exit occurred, is not available in
> bus_lock_exit handler for SEV-ES and SEV-SNP guests, so the above-mentioned
> solution won't work with SEV-ES and SEV-SNP guests.
>
> I would propose to add the above-mentioned solution only for normal and SEV guests
> and unconditionally reloading of bus_lock_counter to 1 in complete_userspace_io
> for SEV-ES and SEV-SNP guests.
Yeah, that works. Though I would condition the check on guest_state_protected.
Actually, and this is going to seem really stupid, but everything will Just Work
if you use kvm_get_linear_rip() and kvm_is_linear_rip(), because kvm_get_linear_rip()
returns '0' for vCPUs with protected state. I.e. KVM will do a rather superfluous
cui() callback, but otherwise it's fine. Silly, but in many ways preferable to
special casing ES and SNP guests.
On a related topic, can you add a refacotring prep patch to move linear_rip out
of kvm_pio_request and place it next to complete_userspace_io? There's nothing
port I/O specific about that field, it just so happens to that port I/O is the
only case where KVM's ABI is to let userspace stuff state (to emulate RESET)
without first completing the I/O instruction.
I.e.
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 8e8ca6dab2b2..8617b15096a6 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -406,7 +406,6 @@ struct kvm_rmap_head {
};
struct kvm_pio_request {
- unsigned long linear_rip;
unsigned long count;
int in;
int port;
@@ -884,6 +883,7 @@ struct kvm_vcpu_arch {
bool emulate_regs_need_sync_to_vcpu;
bool emulate_regs_need_sync_from_vcpu;
int (*complete_userspace_io)(struct kvm_vcpu *vcpu);
+ unsigned long cui_linear_rip;
gpa_t time;
struct pvclock_vcpu_time_info hv_clock;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 425a301911a6..7704d3901481 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9308,7 +9308,7 @@ static int complete_fast_pio_out(struct kvm_vcpu *vcpu)
{
vcpu->arch.pio.count = 0;
- if (unlikely(!kvm_is_linear_rip(vcpu, vcpu->arch.pio.linear_rip)))
+ if (unlikely(!kvm_is_linear_rip(vcpu, vcpu->arch.cui_linear_rip)))
return 1;
return kvm_skip_emulated_instruction(vcpu);
@@ -9333,7 +9333,7 @@ static int kvm_fast_pio_out(struct kvm_vcpu *vcpu, int size,
complete_fast_pio_out_port_0x7e;
kvm_skip_emulated_instruction(vcpu);
} else {
- vcpu->arch.pio.linear_rip = kvm_get_linear_rip(vcpu);
+ vcpu->arch.cui_linear_rip = kvm_get_linear_rip(vcpu);
vcpu->arch.complete_userspace_io = complete_fast_pio_out;
}
return 0;
@@ -9346,7 +9346,7 @@ static int complete_fast_pio_in(struct kvm_vcpu *vcpu)
/* We should only ever be called with arch.pio.count equal to 1 */
BUG_ON(vcpu->arch.pio.count != 1);
- if (unlikely(!kvm_is_linear_rip(vcpu, vcpu->arch.pio.linear_rip))) {
+ if (unlikely(!kvm_is_linear_rip(vcpu, vcpu->arch.cui_linear_rip))) {
vcpu->arch.pio.count = 0;
return 1;
}
@@ -9375,7 +9375,7 @@ static int kvm_fast_pio_in(struct kvm_vcpu *vcpu, int size,
return ret;
}
- vcpu->arch.pio.linear_rip = kvm_get_linear_rip(vcpu);
+ vcpu->arch.cui_linear_rip = kvm_get_linear_rip(vcpu);
vcpu->arch.complete_userspace_io = complete_fast_pio_in;
return 0;
