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