On Mon, Oct 10, 2022 at 10:39:52AM -0700, David Woodhouse wrote:
> On Mon, 2022-10-10 at 13:30 -0400, Michael S. Tsirkin wrote:
> > From: Peter Xu <
> > pet...@redhat.com
> > >
> >
> > It's true that when vcpus<=255 we don't require the length of 32bit APIC
> > IDs. However here since we already have EIM=ON it means the hypervisor
> > will declare the VM as x2apic supported (e.g. VT-d ECAP register will have
> > EIM bit 4 set), so the guest should assume the APIC IDs are 32bits width
> > even if vcpus<=255. In short, commit 77250171bdc breaks any simple cmdline
> > that wants to boot a VM with >=9 but <=255 vcpus with:
>
> I find that paragraph really hard to parse. What does it even mean that
> "guest should assume the APIC IDs are 32bits"?
Quotting EIM definition:
0: On Intel® 64 platforms, hardware supports only 8-bit APIC-IDs (xAPIC
Mode).
1: On Intel® 64 platforms, hardware supports 32-bit APIC- IDs (x2APIC
mode). Hardware implementation reporting Interrupt Remapping support
(IR) field as Clear also report this field as Clear.
I hope the statement was matching the spec. Please let me know if you have
better way to reword it.
>
> In practice, all the EIM bit does is *allow* 32 bits of APIC ID in the
> tables. Which is perfectly fine if there are only 254 CPUs anyway, and
> we never need to use a higher value.
>
> I *think* the actual problem here is when logical addressing is used,
> which puts the APIC cluster ID into higher bits? But it's kind of weird
> that the message doesn't mention that at all?
The commit message actually doesn't even need to contain a lot of
information in this case, IMO.
Literally it can be seen as a revert of a commit which breaks guest with
>8vcpu from boot. I kept the other lines because that still make sense, or
it can be a full revert with "something broke with commit xxx, revert it to
fix" and anything else could be reworked. AFAICT that's how it normally
works with QEMU or Linux.
I am not 100% familiar with the original purpose of the patch, would
eim=off work for you even after patch applied? Anything severely wrong
with this patch?
>
> That's fixable by just setting the X2APIC_PHYSICAL bit in the ACPI
> FADT, isn't it? Then the only values that a guest may put into those
> fields — 32-bit fields or not — are lower than 0xff anyway.
It's still not clear to me why we need to make it inconsistent between the
EIM we declare to the guest and the KVM behavior on understanding EIM bit.
Even if enforced physical mode will work we loose the possibility of
cluster mode, and I also don't see what's the major benefit since EIM=off
will just work, afaiu, meanwhile make everything aligned.
Are you fine if we proceed with this pull request first and revisit later?
Follow up patches will always be fine, and we're unbreaking something. I
have copied you since the 1st patch I posted and the small patch was there
for weeks, it'll be appreciated if either you could comment earlier next
time, or even propose a better fix then we can discuss what's the best way
to fix. Thanks.
--
Peter Xu
