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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.01.2025 17:25, Roger Pau Monné wrote:
> 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@xxxxxxxxx/
>>
>> 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.

I think we simply should not add MADT entries with wrapped (truncated)
APIC IDs. Which can be done when they truly are at risk of wrapping, or
right here.

Jan