Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 1 Feb 2022 21:57:57 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     "Suthikulpanit, Suravee" <suravee.suthikulpanit@amd.com>
Cc:     linux-kernel@vger.kernel.org, kvm@vger.kernel.org, x86@kernel.org,
        pbonzini@redhat.com, joro@8bytes.org, mlevitsk@redhat.com,
        tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
        peterz@infradead.org, hpa@zytor.com, thomas.lendacky@amd.com,
        jon.grimm@amd.com
Subject: Re: [PATCH v3 3/3] KVM: SVM: Extend host physical APIC ID field to
 support more than 8-bit
Message-ID: <Yfms5evHbN8JVbVX@google.com>
References: <20211213113110.12143-1-suravee.suthikulpanit@amd.com>
 <20211213113110.12143-4-suravee.suthikulpanit@amd.com>
 <Yc3qt/x1YPYKe4G0@google.com>
 <34a47847-d80d-e93d-a3fe-c22382977c1c@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <34a47847-d80d-e93d-a3fe-c22382977c1c@amd.com>
Precedence: bulk

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.

> > 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.
 
> > 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.