5:33 a.m.
On Tue, Mar 10, 2020 at 10:18 AM Christian Ehrhardt <
christian.ehrhardt(a)canonical.com> wrote:
...
>
> Running make check would reveal all these issues because every single
> test which involves parsing the cpu_map was failing due to multiple
> definitions of the same CPU model.
>
> I'd not have expected that the tests will exercise the new XMLs in any
way.
Good to know and thanks for your feedback.
> And since this patch is adding several CPU models which are already
> supported by QEMU since 4.2.0, you need to update several existing test
> files for domaincapstest too. You can use
>
> VIR_TEST_REGENERATE_OUTPUT=1 tests/domaincapstest
>
> to regenerate the files. Just make sure you review the changes before
> adding them to this commit.
>
Without regenerating these I see as expected
FAIL: domaincapstest
FAIL: cputest
Adding these 5 types to the qemu 4.2 and qemu 5.0
tests/domaincapsdata worked.
I've squashed that into the same patch - let me know if you'd prefer the
domaincapsdata change as an individual patch instead.
Regenerating the test files will also be needed for cputest because I
> just pushed the "cputest: Add data for Intel(R) Core(TM) i7-8550U CPU
> without TSX". Doing so will nicely show that the computed host CPU model
> (in x86_64-cpuid-Core-i7-8550U-host.xml file) is
> Skylake-Client-noTSX-IBRS rather than Broadwell-noTSX-IBRS.
>
Indeed:
1007 In
'/home/paelzer/work/libvirt/libvirt-ubuntu-git/build/../tests/cputestdata/x86_64-cpuid-Core-i7-8550U-host.xml':
...
1009 Expect [Broadwell-noTSX-IBRS</model>
...
1043 Actual [Skylake-Client-noTSX-IBRS</model>
> However, the CPU used for host-model (and reported in domain
> capabilities) as shown in x86_64-cpuid-Core-i7-8550U-guest.xml and
> x86_64-cpuid-Core-i7-8550U-json.xml will change from Skylake-Client-IBRS
> to Skylake-Client-noTSX-IBRS. As I said in my previous reply to this
> patch, I think these two CPU definitions should keep using the old
> Skylake-Client-IBRS model to make sure any domain with host-model CPU
> will always use the CPU models without noTSX for better compatibility
> between current and future version of libvirt.
I see three changes:
x86_64-cpuid-Core-i7-8550U-host.xml: Broadwell-noTSX-IBRS ->
Skylake-Client-noTSX-IBRS
x86_64-cpuid-Core-i7-8550U-guest.xml: Skylake-Client-IBRS ->
Skylake-Client-noTSX-IBRS
This shows up twice in:
238) cpuTestGuestCPUID(x86_64): Core-i7-8550U
240) cpuTestGuestCPUID(x86_64): Core-i7-8550U
x86_64-cpuid-Core-i7-8550U-json.xml: Skylake-Client-IBRS ->
Skylake-Client-noTSX-IBRS
So far so good and as expected.
But I have to beg your pardon and need to ask where such an override to
continue to use
"Skylake-Client-IBRS + policy='disable' name='hle'
policy='disable'
name='rtm'" instead of just "Skylake-Client-noTSX-IBRS" would have
to go?
Do I get it right that this request would break the current definition of
cpuDecode:
...
185 * when decoding the data. In general, this function will select the
model
186 * closest to the CPU specified by @data.
187 *
188 * For VIR_ARCH_I686 and VIR_ARCH_X86_64 architectures this means the
computed
189 * CPU definition will have the shortest possible list of additional
features.
The actual decode is arch specific via x86DecodeCPUData, here it evaluates
the length of the feature list and thereby prefers the -noTSX type.
Skylake-Client-noTSX-IBRS:
(gdb) p cpuCandidate->nfeatures
$30 = 25
And since it needs to disable hle/rtm the type Skylake-Client-IBRS will have
(gdb) p cpuCandidate->nfeatures
$33 = 27
I don't see an obvious non-hacky way yet to get these detected as non
-noTSX types.
And in general I think we'd not want to map just -noTSX-IBRS to the -IBRS
types.
With the versioned CPUs it is more like "Skylake-Client-*
=> Skylake-Client" which is like selecting the moving base version.
I really beg your pardon, but I don't see this as part of this patch set
just trying to add these new -noTSX types, but some later work to add
versioned CPU support.
Please tell if I'm missing something here and guide me to where such an
override of the preferred type should be done.
Until then I'll prep a v2 that also adapts the cputests to match ...
Holding back v2 submission until this is solved ...
This change should be in
> a separate patch, but in single series with the current patch.
>
> Jirka
>
>
--
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd
--
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd