| Message ID | 20250128163342.1491-1-alejandro.vallejo@cloud.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | tools/hvmloader: Decouple APIC IDs from vCPU IDs | expand |
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). 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.
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. A second approach is to gate things differently by preventing legacy BIOS domains from having APs with APIC IDs >= 255 at all. OVMF already has MP and $PIR tables disabled, from what I can see in hvmloader. Cheers, Alejandro
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.
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@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. 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
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. This short series is extracted from v7 of the much longer "Expose consistent topology to guests". https://lore.kernel.org/xen-devel/20241021154600.11745-5-alejandro.vallejo@cloud.com/ Changes with respect to the original patch on each individual patch. Alejandro Vallejo (3): tools/hvmloader: Retrieve (x2)APIC IDs from the APs themselves tools/hvmloader: Replace LAPIC_ID() with cpu_to_apicid[] tools/hvmloader: Skip writing MP tables if any CPU has an APIC ID >= 255 tools/firmware/hvmloader/config.h | 4 ++- tools/firmware/hvmloader/hvmloader.c | 12 ++++--- tools/firmware/hvmloader/mp_tables.c | 2 +- tools/firmware/hvmloader/smp.c | 47 +++++++++++++++++++++++++++- tools/firmware/hvmloader/util.c | 2 +- 5 files changed, 59 insertions(+), 8 deletions(-)