On Wed, 2023-01-04 at 18:02 +0000, Sean Christopherson wrote: > On Thu, Dec 29, 2022, mlevitsk@xxxxxxxxxx wrote: > > On Fri, 2022-12-09 at 00:00 +0200, Maxim Levitsky wrote: > > > On Sat, 2022-10-01 at 00:59 +0000, Sean Christopherson wrote: > > > > + } else { > > > > + cluster = (ldr >> 4) << 2; > > > > + if (cluster >= 0xf) > > > > return NULL; > > > > - } else { /* cluster */ > > > > - int cluster = (dlid & 0xf0) >> 4; > > > > - int apic = ffs(dlid & 0x0f) - 1; > > > > - > > > > - if ((apic < 0) || (apic > 7) || > > > > - (cluster >= 0xf)) > > > > - return NULL; > > > > - index = (cluster << 2) + apic; > > > > + ldr &= 0xf; > > > > } > > > > + if (!ldr || !is_power_of_2(ldr)) > > > > + return NULL; > > > > + > > > > + index = __ffs(ldr); > > > > + if (WARN_ON_ONCE(index > 7)) > > > > + return NULL; > > > > + index += (cluster << 2); > > > > > > > > logical_apic_id_table = (u32 *) page_address(kvm_svm->avic_logical_id_table_page); > > > > > > > > > > Looks good. > > > > I hate to say it but this patch has a bug: > > > > We have both 'cluster = (ldr >> 4) << 2' and then 'index += (cluster << 2)' > > > > One of the shifts has to go. > > The first shift is wrong. The "cluster >= 0xf" check needs to be done on the actual > cluster. The "<< 2", a.k.a. "* 4", is specific to indexing the AVIC table. > > Thanks! > Yep, agree. (I mean technically you can remove the second shift and do the check before doing the first shift, that is why I told you that one of the shifts has to go) Best regards, Maxim Levitsky