Re: [PATCH] x86/apic: reset LDR in clear_local_APIC

[Bandan Das]

Thomas Gleixner <tglx@xxxxxxxxxxxxx> writes:

> Bandan,
>
> On Wed, 21 Aug 2019, Bandan Das wrote:
>> Thomas Gleixner <tglx@xxxxxxxxxxxxx> writes:
>> So, in KVM: if we make sure that the logical destination map isn't filled up if the virtual
>> apic is not enabled by software, it really doesn't matter whether the LDR for an inactive CPU
>> has a stale value.
>>
>> In x86/apic: if we make sure that the LDR is 0 or reset,
>> recalculate_apic_map() will never consider including this cpu in the
>> logical map.
> ?
>> In short, as I mentioned in the patch description, this is really a KVM
>> bug but it doesn't hurt to clear out the LDR in the guest and then, it
>> wouldn't need a hypervisor fix.
>
> I still needs a hypervisor fix. Taking disabled APICs into account is a bug
> which has also other consequeces than that particular one. So please don't
> claim that. It's wrong.
>
> If that prevents the APIC bug from triggering on unfixed hypervisors, then
> this is a nice side effect, but not a solution.
>
Agreed and fwiw, the kvm fix has been queued already.

>> Is this better ?
>
> That's way better.
>
> So can you please create two patches:
>
>    1) Make that bogus bigsmp ldr init empty
>
>       That one wants a changelog along these lines:
>
>       - Setting LDR for physical destination mode is pointless
>       - Setting multiple bits in the LDR is wrong
>
>       Mention how this was discovered and caused the KVM APIC bug to be
>       triggered. Also mention that the change is not there to paper over
>       the KVM APIC bug. The change fixes a bug in the bigsmp APIC code.
>
>    2) Clear LDR in in that apic reset function
>
>       That one wants a changelog along these lines:
>
>       - Except for x2apic the LDR should be cleared as any other APIC
>       	register
>
>       Mention how this was discovered. Again the change is not there to
>       paper over the KVM APIC bug. It's for correctness sake and valid on
>       its own.
>
> Thanks,
>
Will do as you suggested. Thank you for the review.

Bandan
> 	tglx
>
>