+Stefan I have a question about ratelimit_set_speed() below:
On Fri, Apr 30, 2021 at 06:33:05PM +0800, Chenyi Qiang wrote: > A bus lock is acquired through either split locked access to writeback > (WB) memory or any locked access to non-WB memory. It is typically >1000 > cycles slower than an atomic operation within a cache and can also > disrupts performance on other cores. > > Virtual Machines can exploit bus locks to degrade the performance of > system. To address this kind of performance DOS attack coming from the > VMs, bus lock VM exit is introduced in KVM and it can report the bus > locks detected in guest. If enabled in KVM, it would exit to the > userspace to let the user enforce throttling policies once bus locks > acquired in VMs. > > The availability of bus lock VM exit can be detected through the > KVM_CAP_X86_BUS_LOCK_EXIT. The returned bitmap contains the potential > policies supported by KVM. The field KVM_BUS_LOCK_DETECTION_EXIT in > bitmap is the only supported strategy at present. It indicates that KVM > will exit to userspace to handle the bus locks. > > This patch adds a ratelimit on the bus locks acquired in guest as a > mitigation policy. > > Introduce a new field "bus_lock_ratelimit" to record the limited speed > of bus locks in the target VM. The user can specify it through the > "bus-lock-ratelimit" as a machine property. In current implementation, > the default value of the speed is 0 per second, which means no > restrictions on the bus locks > > As for ratelimit on detected bus locks, simply set the ratelimit > interval to 1s and restrict the quota of bus lock occurence to the value > of "bus_lock_ratelimit". A potential alternative is to introduce the > time slice as a property which can help the user achieve more precise > control. > > The detail of Bus lock VM exit can be found in spec: > https://software.intel.com/content/www/us/en/develop/download/intel-architecture-instruction-set-extensions-programming-reference.html > > Signed-off-by: Chenyi Qiang <chenyi.qi...@intel.com> > > --- > Changes from v2: > - do some rename work (bus-lock-ratelimit and BUS_LOCK_TIME_SLICE). > (Eduardo) > - change to register a class property at the x86_machine_class_init() > and write the gettter/setter for the bus_lock_ratelimit property. > (Eduardo) > - add the lock to access the Ratelimit instance to avoid vcpu thread > race condition. (Eduardo) > - v2: > https://lore.kernel.org/qemu-devel/20210420093736.17613-1-chenyi.qi...@intel.com/ > > Changes from RFC v1: > - Remove the rip info output, as the rip can't reflect the bus lock > position correctly. (Xiaoyao) > - RFC v1: > https://lore.kernel.org/qemu-devel/20210317084709.15605-1-chenyi.qi...@intel.com/ [...] > diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h > index c09b648dff..49b130a649 100644 > --- a/include/hw/i386/x86.h > +++ b/include/hw/i386/x86.h > @@ -74,12 +74,21 @@ struct X86MachineState { > * will be translated to MSI messages in the address space. > */ > AddressSpace *ioapic_as; > + > + /* > + * Ratelimit enforced on detected bus locks in guest. > + * The default value of the bus_lock_ratelimit is 0 per second, > + * which means no limitation on the guest's bus locks. > + */ > + uint64_t bus_lock_ratelimit; > + RateLimit bus_lock_ratelimit_ctrl; > }; > > #define X86_MACHINE_SMM "smm" > #define X86_MACHINE_ACPI "acpi" > #define X86_MACHINE_OEM_ID "x-oem-id" > #define X86_MACHINE_OEM_TABLE_ID "x-oem-table-id" > +#define X86_MACHINE_BUS_LOCK_RATELIMIT "bus-lock-ratelimit" > > #define TYPE_X86_MACHINE MACHINE_TYPE_NAME("x86") > OBJECT_DECLARE_TYPE(X86MachineState, X86MachineClass, X86_MACHINE) > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c > index 7fe9f52710..19b6c4a7e8 100644 > --- a/target/i386/kvm/kvm.c > +++ b/target/i386/kvm/kvm.c > @@ -130,6 +130,9 @@ static bool has_msr_mcg_ext_ctl; > static struct kvm_cpuid2 *cpuid_cache; > static struct kvm_msr_list *kvm_feature_msrs; > > +#define BUS_LOCK_SLICE_TIME 1000000000ULL /* ns */ > +static QemuMutex bus_lock_ratelimit_lock; > + > int kvm_has_pit_state2(void) > { > return has_pit_state2; > @@ -2267,6 +2270,28 @@ int kvm_arch_init(MachineState *ms, KVMState *s) > } > } > > + if (object_dynamic_cast(OBJECT(ms), TYPE_X86_MACHINE)) { > + X86MachineState *x86ms = X86_MACHINE(ms); > + > + if (x86ms->bus_lock_ratelimit > 0) { > + ret = kvm_check_extension(s, KVM_CAP_X86_BUS_LOCK_EXIT); > + if (!(ret & KVM_BUS_LOCK_DETECTION_EXIT)) { > + error_report("kvm: bus lock detection unsupported"); > + return -ENOTSUP; > + } > + ret = kvm_vm_enable_cap(s, KVM_CAP_X86_BUS_LOCK_EXIT, 0, > + KVM_BUS_LOCK_DETECTION_EXIT); > + if (ret < 0) { > + error_report("kvm: Failed to enable bus lock detection cap: > %s", > + strerror(-ret)); > + return ret; > + } > + qemu_mutex_init(&bus_lock_ratelimit_lock); > + ratelimit_set_speed(&x86ms->bus_lock_ratelimit_ctrl, > x86ms->bus_lock_ratelimit, > + BUS_LOCK_SLICE_TIME); > + } > + } > + > return 0; > } > > @@ -4221,6 +4246,20 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run > *run) > } > } > > +static void kvm_rate_limit_on_bus_lock(void) > +{ > + MachineState *ms = MACHINE(qdev_get_machine()); qdev_get_machine() seems thread safe except for the first call, but it's not documented as such. Until it is documented as thread safe (which could take a while, considering that there are ongoing attempts to clean it up), I would avoid calling without the BQL, just in case. > + X86MachineState *x86ms = X86_MACHINE(ms); > + > + qemu_mutex_lock(&bus_lock_ratelimit_lock); > + uint64_t delay_ns = > ratelimit_calculate_delay(&x86ms->bus_lock_ratelimit_ctrl, 1); > + qemu_mutex_unlock(&bus_lock_ratelimit_lock); Stefan, ratelimit_calculate_delay() is supposed to be thread safe, correct? In that case, bus_lock_ratelimit_lock would be completely unnecessary. I normally prefer to avoid static variables, but in this case a static RateLimit bus_lock_ratelimit_ctrl; variable could be the simplest solution here. > + > + if (delay_ns) { > + g_usleep(delay_ns / SCALE_US); > + } > +} > + > MemTxAttrs kvm_arch_post_run(CPUState *cpu, struct kvm_run *run) > { > X86CPU *x86_cpu = X86_CPU(cpu); > @@ -4236,6 +4275,9 @@ MemTxAttrs kvm_arch_post_run(CPUState *cpu, struct > kvm_run *run) > } else { > env->eflags &= ~IF_MASK; > } > + if (run->flags & KVM_RUN_X86_BUS_LOCK) { > + kvm_rate_limit_on_bus_lock(); > + } > > /* We need to protect the apic state against concurrent accesses from > * different threads in case the userspace irqchip is used. */ > @@ -4594,6 +4636,10 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run > *run) > ioapic_eoi_broadcast(run->eoi.vector); > ret = 0; > break; > + case KVM_EXIT_X86_BUS_LOCK: > + /* already handled in kvm_arch_post_run */ > + ret = 0; > + break; > default: > fprintf(stderr, "KVM: unknown exit reason %d\n", run->exit_reason); > ret = -1; > -- > 2.17.1 > -- Eduardo
