| Message ID | 1487588434-4359-9-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 20.02.17 at 12:00, <andrew.cooper3@citrix.com> wrote: > Leaf 0xb is reserved by AMD, and uniformly hidden from guests by the toolstack > logic and hypervisor PV logic. > > The previous dynamic logic filled in the x2APIC ID for all HVM guests. This > is modified to respect the entire leaf being reserved by AMD, but is altered > to include PV Intel guests, so they get more sensible values in their emulated > and faulted view of CPUID. Don't we expose x2APIC to HVM guests even on AMD systems? In which case we surely will want to also expose the x2APIC ID. > @@ -959,6 +950,14 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf, > } > break; > > + case 0xb: > + if ( p->x86_vendor == X86_VENDOR_INTEL ) > + { > + /* Fix the x2APIC identifier. */ > + res->d = v->vcpu_id * 2; > + } > + break; Irrespective of the comment above, wouldn't the if() here better look at the x2APIC feature flag of the domain? Jan
On 22/02/17 09:16, Jan Beulich wrote: >>>> On 20.02.17 at 12:00, <andrew.cooper3@citrix.com> wrote: >> Leaf 0xb is reserved by AMD, and uniformly hidden from guests by the toolstack >> logic and hypervisor PV logic. >> >> The previous dynamic logic filled in the x2APIC ID for all HVM guests. This >> is modified to respect the entire leaf being reserved by AMD, but is altered >> to include PV Intel guests, so they get more sensible values in their emulated >> and faulted view of CPUID. > Don't we expose x2APIC to HVM guests even on AMD systems? In > which case we surely will want to also expose the x2APIC ID. The x2apic feature bit is still listed as reserved in the latest AMD SDM, and I haven't seen a piece of hardware which supports it. Also, x2apic or not, this leaf is documented as fully reserved, so we shouldn't be filling it in anyway. > >> @@ -959,6 +950,14 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf, >> } >> break; >> >> + case 0xb: >> + if ( p->x86_vendor == X86_VENDOR_INTEL ) >> + { >> + /* Fix the x2APIC identifier. */ >> + res->d = v->vcpu_id * 2; >> + } >> + break; > Irrespective of the comment above, wouldn't the if() here better > look at the x2APIC feature flag of the domain? Perhaps. There isn't a formal link called out, but looking at the content, I am guessing that real hardware is consistent with their support of x2apic and the availability of this leaf? ~Andrew
>>> On 22.02.17 at 11:22, <andrew.cooper3@citrix.com> wrote: > On 22/02/17 09:16, Jan Beulich wrote: >>>>> On 20.02.17 at 12:00, <andrew.cooper3@citrix.com> wrote: >>> Leaf 0xb is reserved by AMD, and uniformly hidden from guests by the > toolstack >>> logic and hypervisor PV logic. >>> >>> The previous dynamic logic filled in the x2APIC ID for all HVM guests. This >>> is modified to respect the entire leaf being reserved by AMD, but is altered >>> to include PV Intel guests, so they get more sensible values in their emulated >>> and faulted view of CPUID. >> Don't we expose x2APIC to HVM guests even on AMD systems? In >> which case we surely will want to also expose the x2APIC ID. > > The x2apic feature bit is still listed as reserved in the latest AMD > SDM, and I haven't seen a piece of hardware which supports it. This doesn't seem to answer my question, as hardware behavior is of no interest here (our emulation works on old Intel hardware too). Just to be sure I've checked a HVM guest on an AMD box, and this is what Linux reports: <6>Enabling x2apic <6>Enabled x2apic <6>Switched APIC routing to physical x2apic. > Also, x2apic or not, this leaf is documented as fully reserved, so we > shouldn't be filling it in anyway. While guests appear to do fine without, I think this is inconsistent with providing x2APIC support. But as current behavior is the same, we can as well leave this for the future. >>> @@ -959,6 +950,14 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf, >>> } >>> break; >>> >>> + case 0xb: >>> + if ( p->x86_vendor == X86_VENDOR_INTEL ) >>> + { >>> + /* Fix the x2APIC identifier. */ >>> + res->d = v->vcpu_id * 2; >>> + } >>> + break; >> Irrespective of the comment above, wouldn't the if() here better >> look at the x2APIC feature flag of the domain? > > Perhaps. There isn't a formal link called out, but looking at the > content, I am guessing that real hardware is consistent with their > support of x2apic and the availability of this leaf? I suppose so, at least they've got introduced together iirc. Jan
On 22/02/17 10:37, Jan Beulich wrote: >>>> On 22.02.17 at 11:22, <andrew.cooper3@citrix.com> wrote: >> On 22/02/17 09:16, Jan Beulich wrote: >>>>>> On 20.02.17 at 12:00, <andrew.cooper3@citrix.com> wrote: >>>> Leaf 0xb is reserved by AMD, and uniformly hidden from guests by the >> toolstack >>>> logic and hypervisor PV logic. >>>> >>>> The previous dynamic logic filled in the x2APIC ID for all HVM guests. This >>>> is modified to respect the entire leaf being reserved by AMD, but is altered >>>> to include PV Intel guests, so they get more sensible values in their emulated >>>> and faulted view of CPUID. >>> Don't we expose x2APIC to HVM guests even on AMD systems? In >>> which case we surely will want to also expose the x2APIC ID. >> The x2apic feature bit is still listed as reserved in the latest AMD >> SDM, and I haven't seen a piece of hardware which supports it. > This doesn't seem to answer my question, as hardware behavior is > of no interest here (our emulation works on old Intel hardware too). > Just to be sure I've checked a HVM guest on an AMD box, and this > is what Linux reports: > > <6>Enabling x2apic > <6>Enabled x2apic > <6>Switched APIC routing to physical x2apic. Hmm - that is a good point - we do offer x2apic even to AMD HVM guests. However, this is problematic and highlights another can of worms. We cannot turn on APICV/AVIC for guests if our cpuid features claim a greater featureset than hardware can support. (Looking at c/s 3e0c8272f, TSC_DEADLINE is a feature in a similar position.) I think the only feasible way of making this work is to ensure that the default policy exactly matches hardware, and the max policy contains all features we are able to emulate. Determination of whether to use APICV/AVIC or not must be on whether the chosen featureset is compatible with hardware or not. > >> Also, x2apic or not, this leaf is documented as fully reserved, so we >> shouldn't be filling it in anyway. > While guests appear to do fine without, I think this is inconsistent > with providing x2APIC support. But as current behavior is the same, > we can as well leave this for the future. > >>>> @@ -959,6 +950,14 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf, >>>> } >>>> break; >>>> >>>> + case 0xb: >>>> + if ( p->x86_vendor == X86_VENDOR_INTEL ) >>>> + { >>>> + /* Fix the x2APIC identifier. */ >>>> + res->d = v->vcpu_id * 2; >>>> + } >>>> + break; >>> Irrespective of the comment above, wouldn't the if() here better >>> look at the x2APIC feature flag of the domain? >> Perhaps. There isn't a formal link called out, but looking at the >> content, I am guessing that real hardware is consistent with their >> support of x2apic and the availability of this leaf? > I suppose so, at least they've got introduced together iirc. Ok. I will change the determination to be based exclusively on x2apic being advertised, and leave a comment explaining why. ~Andrew
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c index c8dadab..f1bcd7d 100644 --- a/xen/arch/x86/cpuid.c +++ b/xen/arch/x86/cpuid.c @@ -189,6 +189,9 @@ static void recalculate_misc(struct cpuid_policy *p) p->basic.raw[0x3] = EMPTY_LEAF; /* PSN - always hidden. */ p->basic.raw[0x9] = EMPTY_LEAF; /* DCA - always hidden. */ + /* TODO: Rework topology logic. */ + p->basic.raw[0xb] = EMPTY_LEAF; + p->extd.vendor_ebx = 0; p->extd.vendor_ecx = 0; p->extd.vendor_edx = 0; @@ -208,7 +211,7 @@ static void recalculate_misc(struct cpuid_policy *p) zero_leaves(p->basic.raw, 0x2, 0x3); memset(p->cache.raw, 0, sizeof(p->cache.raw)); - zero_leaves(p->basic.raw, 0x9, 0xa); + zero_leaves(p->basic.raw, 0x9, 0xb); p->extd.vendor_ebx = p->basic.vendor_ebx; p->extd.vendor_ecx = p->basic.vendor_ecx; @@ -634,12 +637,7 @@ static void pv_cpuid(uint32_t leaf, uint32_t subleaf, struct cpuid_leaf *res) switch ( leaf ) { - case 0x0000000b: /* Extended Topology Enumeration */ - *res = EMPTY_LEAF; - break; - - case 0x0 ... 0xa: - case 0xc ... XSTATE_CPUID: + case 0x0 ... XSTATE_CPUID: case 0x80000000 ... 0xffffffff: ASSERT_UNREACHABLE(); /* Now handled in guest_cpuid(). */ @@ -655,13 +653,7 @@ static void hvm_cpuid(uint32_t leaf, uint32_t subleaf, struct cpuid_leaf *res) switch ( leaf ) { - case 0xb: - /* Fix the x2APIC identifier. */ - res->d = v->vcpu_id * 2; - break; - - case 0x0 ... 0xa: - case 0xc ... XSTATE_CPUID: + case 0x0 ... XSTATE_CPUID: case 0x80000000 ... 0xffffffff: ASSERT_UNREACHABLE(); /* Now handled in guest_cpuid(). */ @@ -721,8 +713,7 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf, case 0x0 ... 0x3: case 0x5 ... 0x6: - case 0x8 ... 0xa: - case 0xc: + case 0x8 ... 0xc: *res = p->basic.raw[leaf]; break; } @@ -959,6 +950,14 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf, } break; + case 0xb: + if ( p->x86_vendor == X86_VENDOR_INTEL ) + { + /* Fix the x2APIC identifier. */ + res->d = v->vcpu_id * 2; + } + break; + case XSTATE_CPUID: switch ( subleaf ) {
Leaf 0xb is reserved by AMD, and uniformly hidden from guests by the toolstack logic and hypervisor PV logic. The previous dynamic logic filled in the x2APIC ID for all HVM guests. This is modified to respect the entire leaf being reserved by AMD, but is altered to include PV Intel guests, so they get more sensible values in their emulated and faulted view of CPUID. Sensibly exposing the rest of the leaf requires further topology infrastructure. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> --- xen/arch/x86/cpuid.c | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-)