Do not modify AVIC's logical ID table if the logical ID portion of the
LDR is not a power-of-2, i.e. if the LDR has multiple bits set. Taking
only the first bit means that KVM will fail to match MDAs that intersect
with "higher" bits in the "ID"
The "ID" acts as a bitmap, but is referred to as an ID because theres an
implicit, unenforced "requirement" that software only set one bit. This
edge case is arguably out-of-spec behavior, but KVM cleanly handles it
in all other cases, e.g. the optimized logical map (and AVIC!) is also
disabled in this scenario.
Refactor the code to consolidate the checks, and so that the code looks
more like avic_kick_target_vcpus_fast().
Fixes: 18f40c53e10f ("svm: Add VMEXIT handlers for AVIC")
Cc: Suravee Suthikulpanit <[email protected]>
Cc: Maxim Levitsky <[email protected]>
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/svm/avic.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 4b6fc9d64f4d..a9e4e09f83fc 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -513,26 +513,26 @@ unsigned long avic_vcpu_get_apicv_inhibit_reasons(struct kvm_vcpu *vcpu)
static u32 *avic_get_logical_id_entry(struct kvm_vcpu *vcpu, u32 ldr, bool flat)
{
struct kvm_svm *kvm_svm = to_kvm_svm(vcpu->kvm);
- int index;
u32 *logical_apic_id_table;
- int dlid = GET_APIC_LOGICAL_ID(ldr);
+ u32 cluster, index;
- if (!dlid)
- return NULL;
+ ldr = GET_APIC_LOGICAL_ID(ldr);
- if (flat) { /* flat */
- index = ffs(dlid) - 1;
- if (index > 7)
+ if (flat) {
+ cluster = 0;
+ } 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);
--
2.38.0.rc1.362.ged0d419d3c-goog
On Sat, 2022-10-01 at 00:59 +0000, Sean Christopherson wrote:
> Do not modify AVIC's logical ID table if the logical ID portion of the
> LDR is not a power-of-2, i.e. if the LDR has multiple bits set. Taking
> only the first bit means that KVM will fail to match MDAs that intersect
> with "higher" bits in the "ID"
>
> The "ID" acts as a bitmap, but is referred to as an ID because theres an
> implicit, unenforced "requirement" that software only set one bit. This
> edge case is arguably out-of-spec behavior, but KVM cleanly handles it
> in all other cases, e.g. the optimized logical map (and AVIC!) is also
> disabled in this scenario.
>
> Refactor the code to consolidate the checks, and so that the code looks
> more like avic_kick_target_vcpus_fast().
>
> Fixes: 18f40c53e10f ("svm: Add VMEXIT handlers for AVIC")
> Cc: Suravee Suthikulpanit <[email protected]>
> Cc: Maxim Levitsky <[email protected]>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> arch/x86/kvm/svm/avic.c | 30 +++++++++++++++---------------
> 1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 4b6fc9d64f4d..a9e4e09f83fc 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -513,26 +513,26 @@ unsigned long avic_vcpu_get_apicv_inhibit_reasons(struct kvm_vcpu *vcpu)
> static u32 *avic_get_logical_id_entry(struct kvm_vcpu *vcpu, u32 ldr, bool flat)
> {
> struct kvm_svm *kvm_svm = to_kvm_svm(vcpu->kvm);
> - int index;
> u32 *logical_apic_id_table;
> - int dlid = GET_APIC_LOGICAL_ID(ldr);
> + u32 cluster, index;
>
> - if (!dlid)
> - return NULL;
> + ldr = GET_APIC_LOGICAL_ID(ldr);
>
> - if (flat) { /* flat */
> - index = ffs(dlid) - 1;
> - if (index > 7)
> + if (flat) {
> + cluster = 0;
> + } 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.
For future refactoring, I also suggest to rename this function to 'avic_get_logical_id_table_entry'
to stress the fact that it gets a pointer to the AVIC's data structure.
Same for 'avic_get_physical_id_entry'
And also while at it : the 'svm->avic_physical_id_cache', is a very misleading name,
It should be svm->avic_physical_id_table_entry_ptr with a comment explaining that
is is the pointer to physid table entry.
Reviewed-by: Maxim Levitsky <[email protected]>
Best regards,
Maxim Levitsky
On Fri, 2022-12-09 at 00:00 +0200, Maxim Levitsky wrote:
> On Sat, 2022-10-01 at 00:59 +0000, Sean Christopherson wrote:
> > Do not modify AVIC's logical ID table if the logical ID portion of the
> > LDR is not a power-of-2, i.e. if the LDR has multiple bits set. Taking
> > only the first bit means that KVM will fail to match MDAs that intersect
> > with "higher" bits in the "ID"
> >
> > The "ID" acts as a bitmap, but is referred to as an ID because theres an
> > implicit, unenforced "requirement" that software only set one bit. This
> > edge case is arguably out-of-spec behavior, but KVM cleanly handles it
> > in all other cases, e.g. the optimized logical map (and AVIC!) is also
> > disabled in this scenario.
> >
> > Refactor the code to consolidate the checks, and so that the code looks
> > more like avic_kick_target_vcpus_fast().
> >
> > Fixes: 18f40c53e10f ("svm: Add VMEXIT handlers for AVIC")
> > Cc: Suravee Suthikulpanit <[email protected]>
> > Cc: Maxim Levitsky <[email protected]>
> > Signed-off-by: Sean Christopherson <[email protected]>
> > ---
> > arch/x86/kvm/svm/avic.c | 30 +++++++++++++++---------------
> > 1 file changed, 15 insertions(+), 15 deletions(-)
> >
> > diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> > index 4b6fc9d64f4d..a9e4e09f83fc 100644
> > --- a/arch/x86/kvm/svm/avic.c
> > +++ b/arch/x86/kvm/svm/avic.c
> > @@ -513,26 +513,26 @@ unsigned long avic_vcpu_get_apicv_inhibit_reasons(struct kvm_vcpu *vcpu)
> > static u32 *avic_get_logical_id_entry(struct kvm_vcpu *vcpu, u32 ldr, bool flat)
> > {
> > struct kvm_svm *kvm_svm = to_kvm_svm(vcpu->kvm);
> > - int index;
> > u32 *logical_apic_id_table;
> > - int dlid = GET_APIC_LOGICAL_ID(ldr);
> > + u32 cluster, index;
> >
> > - if (!dlid)
> > - return NULL;
> > + ldr = GET_APIC_LOGICAL_ID(ldr);
> >
> > - if (flat) { /* flat */
> > - index = ffs(dlid) - 1;
> > - if (index > 7)
> > + if (flat) {
> > + cluster = 0;
> > + } 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.
Best regards,
Maxim Levitsky
>
> For future refactoring, I also suggest to rename this function to 'avic_get_logical_id_table_entry'
> to stress the fact that it gets a pointer to the AVIC's data structure.
>
> Same for 'avic_get_physical_id_entry'
>
> And also while at it : the 'svm->avic_physical_id_cache', is a very misleading name,
>
> It should be svm->avic_physical_id_table_entry_ptr with a comment explaining that
> is is the pointer to physid table entry.
>
>
> Reviewed-by: Maxim Levitsky <[email protected]>
>
>
>
> Best regards,
> Maxim Levitsky
On Thu, 2022-12-29 at 10:27 +0200, [email protected] 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:
> > > Do not modify AVIC's logical ID table if the logical ID portion of the
> > > LDR is not a power-of-2, i.e. if the LDR has multiple bits set. Taking
> > > only the first bit means that KVM will fail to match MDAs that intersect
> > > with "higher" bits in the "ID"
> > >
> > > The "ID" acts as a bitmap, but is referred to as an ID because theres an
> > > implicit, unenforced "requirement" that software only set one bit. This
> > > edge case is arguably out-of-spec behavior, but KVM cleanly handles it
> > > in all other cases, e.g. the optimized logical map (and AVIC!) is also
> > > disabled in this scenario.
> > >
> > > Refactor the code to consolidate the checks, and so that the code looks
> > > more like avic_kick_target_vcpus_fast().
> > >
> > > Fixes: 18f40c53e10f ("svm: Add VMEXIT handlers for AVIC")
> > > Cc: Suravee Suthikulpanit <[email protected]>
> > > Cc: Maxim Levitsky <[email protected]>
> > > Signed-off-by: Sean Christopherson <[email protected]>
> > > ---
> > > arch/x86/kvm/svm/avic.c | 30 +++++++++++++++---------------
> > > 1 file changed, 15 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> > > index 4b6fc9d64f4d..a9e4e09f83fc 100644
> > > --- a/arch/x86/kvm/svm/avic.c
> > > +++ b/arch/x86/kvm/svm/avic.c
> > > @@ -513,26 +513,26 @@ unsigned long avic_vcpu_get_apicv_inhibit_reasons(struct kvm_vcpu *vcpu)
> > > static u32 *avic_get_logical_id_entry(struct kvm_vcpu *vcpu, u32 ldr, bool flat)
> > > {
> > > struct kvm_svm *kvm_svm = to_kvm_svm(vcpu->kvm);
> > > - int index;
> > > u32 *logical_apic_id_table;
> > > - int dlid = GET_APIC_LOGICAL_ID(ldr);
> > > + u32 cluster, index;
> > >
> > > - if (!dlid)
> > > - return NULL;
> > > + ldr = GET_APIC_LOGICAL_ID(ldr);
> > >
> > > - if (flat) { /* flat */
> > > - index = ffs(dlid) - 1;
> > > - if (index > 7)
> > > + if (flat) {
> > > + cluster = 0;
> > > + } 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.
Sean, please don't forget to fix this isssue in the next patch series.
Best regards,
Maxim Levitsky
>
> Best regards,
> Maxim Levitsky
>
>
> > For future refactoring, I also suggest to rename this function to 'avic_get_logical_id_table_entry'
> > to stress the fact that it gets a pointer to the AVIC's data structure.
> >
> > Same for 'avic_get_physical_id_entry'
> >
> > And also while at it : the 'svm->avic_physical_id_cache', is a very misleading name,
> >
> > It should be svm->avic_physical_id_table_entry_ptr with a comment explaining that
> > is is the pointer to physid table entry.
> >
> >
> > Reviewed-by: Maxim Levitsky <[email protected]>
> >
> >
> >
> > Best regards,
> > Maxim Levitsky
>
>
On Thu, Dec 29, 2022, [email protected] 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!
On Wed, 2023-01-04 at 18:02 +0000, Sean Christopherson wrote:
> On Thu, Dec 29, 2022, [email protected] 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