Hi David, thanks for your good questions. I had also started to look at it and had similar thoughts.
If we want to reduce overhead, inline assembler / intrisics will be better than creating a stub in the code cache. I think the existing inline assembler implementation for linux should also work on 32 bit linux. Maybe also on BSD (not sure). And on Windows, there's a __cpuid intrinsic available. (I have never tried it.) These implementations have no significant overhead. Maybe we can use them instead? Unfortunately, I'm not familiar with the code in vm_version_windows_x86.cpp, either. And I don't know if it's always fine to 0x40000000 as input to CPUID. Seems like Intel doesn't support that in the real instruction (returns all zero). I this value caught by all virtualization environments? Maybe we can find a little more precise name than "is_in_VM" because I usually think about the Java Thread state "_thread_in_vm". Nevertheless, I appreciate improvements in virtualization detection. Sometimes, problems or crashes are related to the virtualization layer, so having precise information is helpful. Best regards, Martin > -----Original Message----- > From: David Holmes <david.hol...@oracle.com> > Sent: Mittwoch, 12. August 2020 15:05 > To: Yasumasa Suenaga <suen...@oss.nttdata.com>; Baesken, Matthias > <matthias.baes...@sap.com>; hotspot-runtime-...@openjdk.java.net; > build-dev@openjdk.java.net > Cc: Doerr, Martin <martin.do...@sap.com> > Subject: Re: PING: RFR: 8250598: Hyper-V is detected in spite of running on > host OS > > Hi Yasumasa, > > On 12/08/2020 10:56 pm, Yasumasa Suenaga wrote: > > Hi Matthias, David, > > > > On 2020/08/12 21:12, David Holmes wrote: > >> On 12/08/2020 8:51 pm, Baesken, Matthias wrote: > >>> Hi Yasumasa , I'm more or less fine with the change . > >>> But still not fully convinced that removing the iteration is a good > >>> thing . > >>> > >>> http://cr.openjdk.java.net/~ysuenaga/JDK- > 8250598/webrev.01/src/hotspot/cpu/x86/vm_version_x86.cpp.frames.html > >>> > >>> > >>> 1827 for (base = 0x40000000; base < 0x40010000; base += 0x100) { > >>> 1828 check_virt_cpuid(base, registers); > >>> > >>> I think just checking "0x40000000" should work in most cases but if > >>> I remember correctly sometimes it was not enough . > >>> See also some references about Xen/KVM : > >>> > >>> https://lists.linuxfoundation.org/pipermail/virtualization/2012- > May/019974.html > >>> > >>> > >>> "If compat mode for another h/v is enabled then those leaves will > appear > >>> at 0x40000000 and Xen's will be bumped up, so a fully Xen aware set of > >>> drivers (or detection routine, etc) should check at 0x100 intervals > >>> until 0x40010000 " > >>> > >>> and > >>> > >>> https://lore.kernel.org/patchwork/patch/394371/ > > > > I understand that if the process runs on Xen on other hypervisor (e.g. > > KVM), information for Xen would be set between 0x40000100 and > 0x40010000. > > Ok, I will not remove the loop in new webrev, and will add comment about > > it. > > > > > >>> And not so happy about the WMI usage (called in early JVM startup) : > >>> > >>> http://cr.openjdk.java.net/~ysuenaga/JDK- > 8250598/webrev.01/src/hotspot/os_cpu/windows_x86/vm_version_windo > ws_x86.cpp.frames.html > >>> > >>> > >>> bool VM_Version::is_in_VM() { ... } > >>> ... > >>> } > >>> > >>> But if noone else complains about it, I guess it's okay . > >> > >> I haven't reviewed this because I don't understand any of the code. > >> But all that extra stuff during VM initialization doesn't look good to > >> me. We do not want to pay a startup penalty just because we want a > >> more accurate hypervisor listing in an error report! > >> > >> Can we not determine this only if needed? (Granted doing all this > >> stuff during crash reporting may also be an issue.) > > > > This value would be used in hs_err and VM.info dcmd, so we can > determine > > it when it is needed. > > However the change would be bigger, and it is not the original scope. > > Can we fix them at the same time? > > Are you able to run startup benchmarks to see if there is any regression? > > David > ----- > > > > > Thanks, > > > > Yasumasa > > > > > >> David > >> ----- > >> > >>> > >>> Best regards, Matthias > >>> > >>> > >>>> PING: Could you review this change? > >>>> > >>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8250598 > >>>> webrev: http://cr.openjdk.java.net/~ysuenaga/JDK- > 8250598/webrev.01/ > >>>> > >>>> Build change has been reviewed by Erik. > >>>> > >>>
