[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1f157cca-a71e-cdee-4427-e1f4bb1b1115@redhat.com>
Date: Tue, 27 Dec 2022 12:30:18 +0100
From: Paolo Bonzini <pbonzini@...hat.com>
To: Sean Christopherson <seanjc@...gle.com>,
Maxim Levitsky <mlevitsk@...hat.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
Alejandro Jimenez <alejandro.j.jimenez@...cle.com>,
Suravee Suthikulpanit <suravee.suthikulpanit@....com>,
Li RongQing <lirongqing@...du.com>
Subject: Re: [PATCH v4 19/32] KVM: x86: Explicitly track all possibilities for
APIC map's logical modes
On 12/17/22 00:34, Sean Christopherson wrote:
> On Fri, Dec 16, 2022, Sean Christopherson wrote:
>> On Thu, Dec 08, 2022, Maxim Levitsky wrote:
>
> ...
>
>>>> @@ -282,7 +291,8 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
>>>> if (!apic_x2apic_mode(apic) && !new->phys_map[xapic_id])
>>>> new->phys_map[xapic_id] = apic;
>>>>
>>>> - if (!kvm_apic_sw_enabled(apic))
>>>> + if (new->logical_mode == KVM_APIC_MODE_MAP_DISABLED ||
>>>> + !kvm_apic_sw_enabled(apic))
>>>> continue;
>>> Very minor nitpick: it feels to me that code that updates the logical mode of the
>>> map, might be better to be in a function, or in 'if', like
>>
>> An if-statement would be rough due to the indentation. A function works well
>> though, especially if both the physical and logical chunks are put into helpers.
>> E.g. the patch at the bottom (with other fixup for this patch) yields:
>>
>> new->max_apic_id = max_id;
>> new->logical_mode = KVM_APIC_MODE_SW_DISABLED;
>>
>> kvm_for_each_vcpu(i, vcpu, kvm) {
>> if (!kvm_apic_present(vcpu))
>> continue;
>>
>> if (kvm_recalculate_phys_map(new, vcpu, &xapic_id_mismatch)) {
>> kvfree(new);
>> new = NULL;
>> goto out;
>> }
>>
>> kvm_recalculate_logical_map(new, vcpu);
>> }
>>
>> I'll tack that patch on at the end of the series if it looks ok.
Yes, please send as a follow up.
Paolo
Powered by blists - more mailing lists