Re: [PATCH 0/3] tools/hvmloader: Decouple APIC IDs from vCPU IDs

["Roger Pau Monné" <roger.pau@citrix.com>]

On Tue, Jan 28, 2025 at 06:42:38PM +0000, Alejandro Vallejo wrote:
> On Tue Jan 28, 2025 at 5:45 PM GMT, Roger Pau Monné wrote:
> > On Tue, Jan 28, 2025 at 04:33:39PM +0000, Alejandro Vallejo wrote:
> > > The hypervisor, hvmloader and the toolstack currently engage in a shared
> > > assumption that for every vCPU apicid == 2 * vcpuid. This series removes such
> > > assumption from hvmloader, by making it read the APIC ID of each vCPU and
> > > storing it for later use.
> > > 
> > > The last patch prevents writing an MP Tables should we have vCPUs that can not
> > > be represented there. That's at the moment dead code because all vCPUs are
> > > currently representable in 8 bits. This will inavitably stop being true in the
> > > future after we increase the maximum number of guest vCPUs.
> >
> > While I'm fine with the MP Table change, should it also come together
> > with a patch that introduces the code to create x2APIC entries in
> > libacpi construct_madt() helper? (and bumping the MADT revision, as
> > I'm quite sure version 2 didn't have x2APIC entries in the
> > specification).
> 
> That's a lot more involved though. Matt started something in that direction
> last year, but testing it was (and still is) effectively impossible until
> HVM_MAX_VCPUS increases.
> 
>   https://lore.kernel.org/xen-devel/cd1a3ce14790af8c1bb4372ef0be5a6cbbb50b1c.1710338145.git.matthew.barnes@cloud.com/
> 
> The rest of the topo series can be used to test that (with a hack to
> artificially bump the width of thread_id space), I'd rather not test a patch
> with a long and still uncommitted series.
> 
> >
> > Otherwise the MP Table change seems like a red herring, because the
> > MADT created by libacpi will also be incorrect and APIC IDs will wrap in
> > local APIC entries, just like it would on MP Tables.
> >
> > Thanks, Roger.
> 
> My take is that this is strictly better than what we have today by virtue of
> going down from 2 latent bugs to just 1. That said, I don't strictly need it
> for the topo series to advance, so it is (in a sense) optional.

I'm fine with the patch, but it probably wants to mention in the
commit message that MADT tables will still wrap when using APIC IDs >
255, as otherwise it seems MADT is not taken into consideration.

Thanks, Roger.