Re: [PATCH v5 7/8] KVM: VMX: Update PID-pointer table entry when APIC ID is changed

[Maxim Levitsky]

On Fri, 2022-01-07 at 16:05 +0800, Zeng Guang wrote:
> On 1/6/2022 10:06 PM, Tom Lendacky wrote:
> > On 1/5/22 7:44 PM, Zeng Guang wrote:
> > > On 1/6/2022 3:13 AM, Tom Lendacky wrote:
> > > > On 12/31/21 8:28 AM, Zeng Guang wrote:
> > > > Won't this blow up on AMD since there is no corresponding SVM op?
> > > > 
> > > > Thanks,
> > > > Tom
> > > Right, need check ops validness to avoid ruining AMD system. Same
> > > consideration on ops "update_ipiv_pid_table" in patch8.
> > Not necessarily for patch8. That is "protected" by the
> > kvm_check_request(KVM_REQ_PID_TABLE_UPDATE, vcpu) test, but it couldn't hurt.
> 
> OK, make sense. Thanks.

I haven't fully reviewed this patch series yet,
and I will soon.

I just want to point out few things:

1. AMD's AVIC also has a PID table (its calle AVIC physical ID table). 
It stores addressses of vCPUs apic backing pages,
and thier real APIC IDs.

avic_init_backing_page initializes the entry (assuming apic_id == vcpu_id) 
(which is double confusing)

2. For some reason KVM supports writable APIC IDs. Does anyone use these?
Even Intel's PRM strongly discourages users from using them and in X2APIC mode,
the APIC ID is read only.

Because of this we have quite some bookkeeping in lapic.c, 
(things like kvm_recalculate_apic_map and such)

Also AVIC has its own handling for writes to APIC_ID,APIC_LDR,APIC_DFR
which tries to update its physical and logical ID tables.

(it used also to handle apic base and I removed this as apic base otherwise
was always hardcoded to the default vaule)

Note that avic_handle_apic_id_update is broken - it always copies the entry
from the default (apicid == vcpu_id) location to new location and zeros
the old location, which will fail in many cases, like even if the guest
were to swap few apic ids.

Also writable apic ID means that two vCPUs can have same apic ID. No way
we handle this correclty, and no way APICv/AVIC does.

Best regards,
	Maxim Levitsky

> 
> > Thanks,
> > Tom
> > 
> > > I will revise in next version. Thanks.
> > > > > +        } else
> > > > >                 ret = 1;
> > > > >             break;
> > > > >