Re: [PATCH v5] x86/apic: Use EILVT register count from APIC_EFEAT

[Borislav Petkov]

On Mon, Sep 28, 2026 at 12:20:04PM +0530, Naveen N Rao wrote:
> On Fri, Sep 25, 2026 at 05:38:33PM -0700, Borislav Petkov wrote:
> > On Fri, Sep 25, 2026 at 09:47:06PM +0530, Naveen N Rao (AMD) wrote:
> > > Future AMD processors will be increasing the number of EILVT registers.
> > > Rather than hardcoding the maximum EILVT register count and using that
> > > everywhere, introduce a variable in 'struct apic' to track the EILVT
> > > register count.
> > > 
> > > The number of EILVT registers is exposed through the extended APIC
> > > Feature Register (APIC_EFEAT) bits 23:16 on platforms that support the
> > > AMD Extended APIC Register space (X86_FEATURE_EXTAPIC).  Use this to
> > > initialize the count and fall back to the current default from AMD
> > > family 0x10 (APIC_EILVT_NR_AMD_10H, which is 4) otherwise. Since this
> > > value is no longer a compile-time constant, update eilvt_offsets to be
> > > dynamically allocated.
> > > 
> > > Drop the now-redundant APIC_EILVT_NR_MAX macro. Other than during EILVT
> > > register offset allocation (which now uses apic->eilvt_regs_count), that
> > > macro was being used in the IBS driver for determining the EILVT offset
> > > for AMD family 0x10 since the EILVT offsets were not assigned by the
> > > BIOS. Switch that to use APIC_EILVT_NR_AMD_10H, which reflects the
> > > correct EILVT register count for that family.
> > > 
> > > Note: because the EILVT register count is now derived from APIC_EFEAT,
> > > it is possible that the register count is less than 4 (1 or 0 even) on
> > > some AMD K8 parts (rather than the previous default of 4), which should
> > > more accurately reflect the correct EILVT register count on those parts.
> > 
> > Please, do not talk about *what* the patch is doing in the commit message
> > - that should be obvious from the diff itself. Rather, concentrate on the
> > *why* it needs to be done and why your patch exists.
> > 
> > It is perfectly fine to explain non-trivial aspects of the code the patch is
> > touching but do not regurgitate what it does.
> 
> I thought I have explained the why. What am I missing?
> 
> Perhaps you are saying paragraph 2 is not needed? Though I fail to see 
> how it can hurt (it does clarify why the fallback is the value that it 
> is).

I went and rewrote your commit message without the code explanations:

"Future AMD processors will increase the number of EILVT registers. Track the
EILVT register max count in a variable. Use the current default of 4 EILVT max
count from F10h times as the fallback.

Use the APIC_EILVT_NR_AMD_10H macro only in force_ibs_eilvt_setup() which is
F10 specific anyway."

That's it. Everything else can be read out from the patch itself and you don't
need to spell it again in the commit message.

> > Also, that second note about clamping it:
> > 
> > https://sashiko.dev/#/patchset/20260925161706.1619042-1-naveen%40kernel.org
> > 
> > does sound relevant.
> 
> I had addressed this in the cover note on the previous version and 
> didn't repeat it here:
> https://lore.kernel.org/all/cover.1788425679.git.naveen@xxxxxxxxxx/
> 
>   3. Need to clamp the maximum EILVT register count to prevent incorrect 
>      MMIO accesses: this is not an issue since all offsets being 
>      programmed are appropriately clamped at the source.

"Table 16-2. APIC Registers

...

500-570h	Extended Interrupt [7:0] Local Vector Table Registers	00000000h"

I'm reading this as, EILVT max cannot be more than 8 regs. Right?

So doing a  if > 8 at read time would be a good sanity-check, no?

> Oh yeah, and no amount of me calling this out explicitly in the commit 
> log seemed to help. See patch 2 here, where I added that it is the 
> actual count:
> https://lore.kernel.org/all/39d39c3b91d174f4090fb954c6690edae9ed8295.1784619898.git.naveen@xxxxxxxxxx/
> 
> Sashiko review of that:
> https://sashiko.dev/#/patchset/cover.1784619898.git.naveen@xxxxxxxxxx

Recent experience tells me that I cannot trust AI one bit.
 
> No, and I never claimed they did. If anything, I have called this out 
> explicitly as part of the changelog:
>   - Pick up Bharata's Tested-by, and retain Manali's tag since the change
>     is minimal
> 
> Retaining their tags was a deliberate decision on my part knowing the 
> tests they are doing and that they would not be exercizing the fallback 
> path, which this version changes.
> 
> I'm absolutely ok to drop that, but no, I didn't claim they tested this 
> version.

Just drop the Tested-by tags. They haven't tested the patch and that's it.
It's not like you fixed comments or something else immaterial to code.
A Tested-by should mean what it says, not the person tested some old version
of the patch.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette