Re: [PATCH v3 3/3] KVM: SVM: Extend host physical APIC ID field to support more than 8-bit

[Suthikulpanit, Suravee]

Hi Sean,

On 12/31/2021 12:21 AM, Sean Christopherson wrote:
> On Mon, Dec 13, 2021, Suravee Suthikulpanit wrote:
> > .....
> > +	} else {
> > +		u32 count = get_count_order(apic_get_max_phys_apicid());
> > +
> > +		avic_host_physical_id_mask = BIT_ULL(count) - 1;
> > +	}
>
> Why is the "legal" mask dynamically calculated?  That's way more complicated and
> convoluted then this needs to be.
>
> The cover letter says
>
>    However, newer AMD systems can have physical APIC ID larger than 255,
>    and AVIC hardware has been extended to support upto the maximum physical
>    APIC ID available in the system.
>
> and newer versions of the APM have bits
>
>    11:8 - Reserved/SBZ for legacy APIC; extension of Host Physical APIC ID when
>           x2APIC is enabled.
>    7:0  - Host Physical APIC ID Physical APIC ID of the physical core allocated by
>           the VMM to host the guest virtual processor. This field is not valid
> 	 unless the IsRunning bit is set.
>
> whereas older versions have
>
>    11:8 - Reserved, SBZ. Should always be set to zero.

I have checked with the hardware and documentation team. The statement regarding "x2APIC"
is not accurate and will be corrected. Sorry for confusion.

> That implies that an APIC ID > 255 on older hardware what ignores bits 11:8 even
> in x2APIC will silently fail, and the whole point of this mask is to avoid exactly
> that.

On current AMD system w/ x2APIC and 256 cpus (e.g. max APIC ID is 255), it would only
need 8 bits in the physical APIC ID table entry, and the bit 11:9 are reserved.
For newer system, it could take upto 12 bits to represent APIC ID.

> To further confuse things, the APM was only partially updated and needs to be fixed,
> e.g. "Figure 15-19. Physical APIC Table in Memory" and the following blurb wasn't
> updated to account for the new x2APIC behavior.

Noted. I'll inform the team.

> But at least one APM blurb appears to have been wrong (or the architecture is broken)
> prior to the larger AVIC width:
>
>    Since a destination of FFh is used to specify a broadcast, physical APIC ID FFh
>    is reserved.
>
> We have Rome systems with 256 CPUs and thus an x2APIC ID for a CPU of FFh.  So
> either the APM is wrong or AVIC is broken on older large systems.

Actually, the statement is referred to the guest physical APIC ID, which is used to
index the per-vm physical APIC table in the host. So, it should be correct in the case
of AVIC, which only support APIC mode in the guest.

> Anyways, for the new larger mask, IMO dynamically computing the mask based on what
> APIC IDs were enumerated to the kernel is pointless.  If the AVIC doesn't support
> using bits 11:0 to address APIC IDs then KVM is silently hosed no matter what if
> any APIC ID is >255.

The reason for dynamic mask is to protect the reserved bits, which varies between
the current platform (i.e 11:8) vs. newer platform (i.e. 11:10), in which
there is no good way to tell except to check the max_physical_apicid (see below).

> Ideally, there would be a feature flag enumerating the larger AVIC support so we
> could do:
>
> 	if (!x2apic_mode || !boot_cpu_has(X86_FEATURE_FANCY_NEW_AVIC))
> 		avic_host_physical_id_mask = GENMASK(7:0);
> 	else
> 		avic_host_physical_id_mask = GENMASK(11:0);
>
> but since it sounds like that's not the case, and presumably hardware is smart
> enough not to assign APIC IDs it can't address, this can simply be
>
> 	if (!x2apic_mode)
> 		avic_host_physical_id_mask = GENMASK(7:0);
> 	else
> 		avic_host_physical_id_mask = GENMASK(11:0);
>
> and patch 01 to add+export apic_get_max_phys_apicid() goes away.

Unfortunately, we do not have the "X86_FEATURE_FANCY_NEW_AVIC" CPUID bit :(

Also, based on the previous comment, we can't use the x2APIC mode in the host
to determine such condition. Hence, the need for dynamic mask based on
the max_physical_apicid.

> > +	pr_debug("Using AVIC host physical APIC ID mask %#0llx\n",
> > +		 avic_host_physical_id_mask);
> > +}
> > +
> >   int avic_vm_init(struct kvm *kvm)
> >   {
> >   	unsigned long flags;
> > @@ -943,22 +959,17 @@ avic_update_iommu_vcpu_affinity(struct kvm_vcpu *vcpu, int cpu, bool r)
> >   void avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> >   {
> >   	u64 entry;
> > -	/* ID = 0xff (broadcast), ID > 0xff (reserved) */
> >   	int h_physical_id = kvm_cpu_get_apicid(cpu);
> >   	struct vcpu_svm *svm = to_svm(vcpu);
> >   
> > -	/*
> > -	 * Since the host physical APIC id is 8 bits,
> > -	 * we can support host APIC ID upto 255.
> > -	 */
> > -	if (WARN_ON(h_physical_id > AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK))
> > +	if (WARN_ON(h_physical_id > avic_host_physical_id_mask))
>
> Not really your code, but this should really be
>
> 	if (WARN_ON((h_physical_id & avic_host_physical_id_mask) != h_physical_id))
> 		return;
>
> otherwise a negative value will get a false negative.

I can do this in v4.

Regards,
Suravee