Date:   Wed, 2 Feb 2022 14:32:00 +0700
From:   "Suthikulpanit, Suravee" <suravee.suthikulpanit@....com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org, x86@...nel.org,
        pbonzini@...hat.com, joro@...tes.org, mlevitsk@...hat.com,
        tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
        peterz@...radead.org, hpa@...or.com, thomas.lendacky@....com,
        jon.grimm@....com
Subject: Re: [PATCH v3 3/3] KVM: SVM: Extend host physical APIC ID field to
 support more than 8-bit

Sean,

On 2/2/2022 4:57 AM, Sean Christopherson wrote:
> On Tue, Feb 01, 2022, Suthikulpanit, Suravee wrote:
>>> 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.
> 
> But x2APIC IDs are 32-bit values that, from the APM, are model specific:
> 
>    The x2APIC_ID is a concatenation of several fields such as socket ID, core ID
>    and thread ID.
> 
>    Because the number of sockets, cores and threads may differ for each SOC, the
>    format of x2APIC ID is model-dependent.
> 
> In other words, there's nothing that _architecturally_ guarantees 8 bits are
> sufficient to hold the x2APIC ID.

Agree that there is nothing architecturally guarantee. Let's discuss this below....

>>> 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.
> 
> Ah.  If you have the ear of the APM writers, can you ask that they insert a "guest",
> e.g. so that it reads:
> 
>    Since a destination of FFh is used to specify a broadcast, guest physical APIC ID FFh is reserved.

I'll let them know :)

>>> 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.
> 
> I don't get this.  The APM literally says bits 11:8 are:
> 
>    Reserved/SBZ for legacy APIC; extension of Host Physical APIC ID when
>    x2APIC is enabled.
> 
> so we absolutely should be able to key off x2APIC mode. IMO, defining the mask
> based on apic_get_max_phys_apicid() is pointless and misleading.  The only thing
> it really protects is passing in a completely bogus value, e.g. -1.  If for some
> reason bits 11:8 are ignored/reserved by older CPUs even in x2APIC, and the CPU
> assigns an x2APIC ID with bits 11:8!=0, then KVM is hosed no matter what as the
> dynamic calculation will also allow the "bad" ID.

.... here

As I mentioned, the APM will be corrected to remove the word "x2APIC".
Essentially, it will be changed to:

  * 7:0  - For systems w/ max APIC ID upto 255 (a.k.a old system)
  * 11:8 - For systems w/ max APIC ID 256 and above (a.k.a new system). Otherwise, reserved and should be zero.

As for the required number of bits, there is no good way to tell what's the max
APIC ID would be on a particular system. Hence, we utilize the apic_get_max_phys_apicid()
to figure out how to properly program the table (which is leaving the reserved field
alone when making change to the table).

The avic_host_physical_id_mask is not just for protecting APIC ID larger than
the allowed fields. It is also currently used for clearing the old physical APIC ID table entry
before programing it with the new APIC ID.

So, What if we use the following logic:

+	u32 count = get_count_order(apic_get_max_phys_apicid());
+
+	/*
+	 * Depending on the maximum host physical APIC ID available
+	 * on the system, AVIC can support upto 8-bit or 12-bit host
+	 * physical APIC ID.
+	 */
+	if (count <= 8)
+		avic_host_physical_id_mask = GENMASK(7, 0);
+	else if (count <= 12)
+		avic_host_physical_id_mask = GENMASK(11, 0);
+	else
+		/* Warn and Disable AVIC here due to unable to satisfy APIC ID requirement */

Regards,
Suravee

Powered by blists - more mailing lists