RFR: 8219241: Provide basic virtualization related info in the hs_error file on linux/windows x86_64

[David Holmes]

On 4/04/2019 8:54 pm, Baesken, Matthias wrote:
>>
>> My remaining query is why we need the OS specific checks and code in
>> os-cpu files? Isn't this just cpu specific? Even if a particular OS
>> doesn't support virtualization, won't the cpuid query simply report "no
>> virtualization"?
>>
> 
> Hi David,  if you are referring to  these  os-cpu files  :
> 
> src/hotspot/os_cpu/linux_x86/vm_version_linux_x86.cpp
> src/hotspot/os_cpu/windows_x86/vm_version_windows_x86.cpp
> 
> we have different coding available  because  I am not aware  of a cross OS  API/coding to get   the cpu id info.
> That’s why different coding for LINUX/Windows.

?? But there's nothing in those added functions that is Linux specific 
or Windows specific! The only difference is the mechanism for doing 
"inline assembly" and that's compiler specific. So this should just use 
compiler based ifdefs in vm_version_x86.cpp.

Cheers,
David

> 
> Best regards, Matthias
> 
> 
>> -----Original Message-----
>> From: David Holmes <david.holmes at oracle.com>
>> Sent: Donnerstag, 4. April 2019 03:16
>> To: Baesken, Matthias <matthias.baesken at sap.com>; 'hotspot-
>> dev at openjdk.java.net' <hotspot-dev at openjdk.java.net>
>> Cc: Doerr, Martin <martin.doerr at sap.com>
>> Subject: Re: RFR: 8219241: Provide basic virtualization related info in the
>> hs_error file on linux/windows x86_64
>>
>> Hi Matthias,
>>
>> This is looking much better/simpler - thanks!
>>
>> My remaining query is why we need the OS specific checks and code in
>> os-cpu files? Isn't this just cpu specific? Even if a particular OS
>> doesn't support virtualization, won't the cpuid query simply report "no
>> virtualization"?
>>
>> Thanks,
>> David
>>
>> On 4/04/2019 12:43 am, Baesken, Matthias wrote:
>>>> 1. Why do we have/need os::<os>::print_virtualization_info when we also
>>>> have VM_Version::print_platform_virtualization_info?
>>>>
>>>
>>> Hi David,  I have to agree  -  we do not really need both .
>>> So I removed   os::<os>::print_virtualization_info    and   call
>> VM_Version::print_platform_virtualization_info(st);     at  the places where
>> the output is done.
>>>
>>> Regarding point  2. :
>>>
>>> I now  just use   print_platform_virtualization_info,
>> print_detected_virtualization  has been removed .
>>> print_platform_virtualization_info   is doing  the output   (differs  for
>> vm_version_  s390 /  ppc / x86_64 ) .
>>>
>>> The  new webrev   is simpler, with less methods  :
>>>
>>>
>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8219241.4/
>>>
>>> Best regards, Matthias
>>>
>>>
>>>> -----Original Message-----
>>>> From: David Holmes <david.holmes at oracle.com>
>>>> Sent: Freitag, 29. März 2019 01:16
>>>> To: Baesken, Matthias <matthias.baesken at sap.com>; 'hotspot-
>>>> dev at openjdk.java.net' <hotspot-dev at openjdk.java.net>
>>>> Cc: Doerr, Martin <martin.doerr at sap.com>
>>>> Subject: Re: RFR: 8219241: Provide basic virtualization related info in the
>>>> hs_error file on linux/windows x86_64
>>>>
>>>> Hi Matthias,
>>>>
>>>> I have two issues with this API:
>>>>
>>>> 1. Why do we have/need os::<os>::print_virtualization_info when we also
>>>> have VM_Version::print_platform_virtualization_info?
>>>>
>>>> 2. I don't like the fact that the there are two ways to define the
>>>> platform specific information:
>>>>      a) override VM_Version::print_platform_virtualization_info; or
>>>>      b) hook into the get_detected_virtualization switch
>>>>
>>>> And IIUC code that uses (a) relies on the switch doing nothing
>>>> (NoVirtualization) and code that uses (b) relies on
>>>> print_platform_virtualization_info doing nothing!
>>>>
>>>> Why doesn't the default implementation of
>>>> VM_Version::print_platform_virtualization_info define the
>>>> get_detected_virtualization() switch logic, and do away with
>>>> Abstract_VM_Version::print_detected_virtualization? Code that wants to
>>>> print this info can just call
>>>> VM_Version::print_platform_virtualization_info().
>>>>
>>>> Sorry but I just find the current proposal has too many methods and an
>>>> unclear structure.
>>>>
>>>> David
>>>