On 2013-03-04 15:28, Paolo Bonzini wrote:
> Il 03/03/2013 21:21, Jan Kiszka ha scritto:
>> From: Jan Kiszka <jan.kiszka@xxxxxxxxxxx>
>>
>> A VCPU sending INIT or SIPI to some other VCPU races for setting the
>> remote VCPU's mp_state. When we were unlucky, KVM_MP_STATE_INIT_RECEIVED
>> was overwritten by kvm_emulate_halt and, thus, got lost.
>>
>> Fix this by raising requests on the sender side that will then be
>> handled synchronously over the target VCPU context.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@xxxxxxxxxxx>
>> ---
>>
>> Turned out to be simpler than expected. I'm no longer able to reproduce
>> the race I saw before.
>>
>> arch/x86/kvm/lapic.c | 9 ++++-----
>> arch/x86/kvm/x86.c | 16 +++++++++++++++-
>> include/linux/kvm_host.h | 2 ++
>> 3 files changed, 21 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index 02b51dd..be1e37a 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -731,8 +731,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
>> case APIC_DM_INIT:
>> if (!trig_mode || level) {
>> result = 1;
>> - vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
>> - kvm_make_request(KVM_REQ_EVENT, vcpu);
>> + kvm_make_request(KVM_REQ_INIT, vcpu);
>> kvm_vcpu_kick(vcpu);
>> } else {
>> apic_debug("Ignoring de-assert INIT to vcpu %d\n",
>> @@ -743,11 +742,11 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
>> case APIC_DM_STARTUP:
>> apic_debug("SIPI to vcpu %d vector 0x%02x\n",
>> vcpu->vcpu_id, vector);
>> - if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
>> + if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED ||
>> + test_bit(KVM_REQ_INIT, &vcpu->requests)) {
>> result = 1;
>> vcpu->arch.sipi_vector = vector;
>> - vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
>> - kvm_make_request(KVM_REQ_EVENT, vcpu);
>> + kvm_make_request(KVM_REQ_SIPI, vcpu);
>> kvm_vcpu_kick(vcpu);
>> }
>> break;
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index d0cf737..8c8843c 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -5641,6 +5641,18 @@ static void update_eoi_exitmap(struct kvm_vcpu *vcpu)
>> kvm_x86_ops->load_eoi_exitmap(vcpu, eoi_exit_bitmap);
>> }
>>
>> +static bool kvm_check_init_and_sipi(struct kvm_vcpu *vcpu)
>> +{
>> + if (kvm_check_request(KVM_REQ_INIT, vcpu))
>> + vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
>> + if (kvm_check_request(KVM_REQ_SIPI, vcpu) &&
>> + vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
>> + vcpu->arch.mp_state = KVM_MP_STATE_SIPI_RECEIVED;
>
> Do you need KVM_MP_STATE_SIPI_RECEIVED at all anymore? Perhaps you can
> call kvm_check_init_and_sipi from __vcpu_run, before the call to
> kvm_vcpu_block (and move the reset from __vcpu_run to
> kvm_check_init_and_sipi too)? Then you do not even need to touch
> kvm_arch_vcpu_runnable.
Haven't thought about this in details yet as I first wanted to fix
within the existing infrastructure. But maybe the change below requires
more refactoring anyway. Let's see.
>
>> + return true;
>> + }
>> + return false;
>> +}
>> +
>> static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>> {
>> int r;
>> @@ -5649,6 +5661,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>> bool req_immediate_exit = 0;
>>
>> if (vcpu->requests) {
>> + kvm_check_init_and_sipi(vcpu);
>
> Does this need to return 1 if kvm_check_init_and_sipi returns 1?
> Otherwise the guest is entered in INIT state. I think.
Hmm, true... Need to refactor things a bit more as
kvm_check_init_and_sipi is designed to return true only for
wait-on-sipi->runnable transition.
Thanks,
Jan
--
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
