Skip to content
This repository has been archived by the owner on Feb 29, 2020. It is now read-only.

Add cpuid hypervisor support to work with virtualization #17

Merged
merged 10 commits into from
Oct 30, 2019

Conversation

Leoyzen
Copy link
Contributor

@Leoyzen Leoyzen commented Oct 24, 2019

acidanthera/bugtracker#531

For some reason that some msr not impletement in such hypervisor (QEMU/KVM/VMWare), so some logic may not work fine in a VM.
This commit will add cpuid hypervisor support to indicate whether we are
under virtualization, then we do some tricks to make it work.

According to these links:
1. CPUID usage for interaction between Hypervisors and Linux.
2. [Qemu-devel] [PATCH v2 0/3] x86-kvm: Fix Mac guest timekeeping by exposi

There is a "VMWare Timing" Node located at 0x40000010 where we can get
TSC/FSB Frequency.It is more sensable than geting those freq from MSR which
most hypervisors not implemented yet (KVM/VMWare).

@Leoyzen Leoyzen changed the title Add cpuid hypervisor support to work with visualization Add cpuid hypervisor support to work with visualization https://github.com/acidanthera/bugtracker/issues/531 Oct 24, 2019
@Leoyzen Leoyzen changed the title Add cpuid hypervisor support to work with visualization https://github.com/acidanthera/bugtracker/issues/531 Add cpuid hypervisor support to work with visualization Oct 24, 2019
Copy link
Member

@mhaeuser mhaeuser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm the Ryzen bug described, but cannot comment on the Intel portion. As for the Ryzen changes, I'm not convinced this is the right approach. I don't have the references at hand, but there are semi-standardised and conventional HV CPUID leafs providing things like FSB Frequency (which might be labeled differently), even supported by XNU directly to bypass things like FSB/ARTFrequency property evaluation. This route should be investigated before deciding on the workaround.

You seem to have referenced the incorrect bugreport in your commit message. I think it was supposed to refer to: acidanthera/bugtracker#531

Library/OcCpuLib/OcCpuLib.c Outdated Show resolved Hide resolved
Library/OcCpuLib/OcCpuLib.c Outdated Show resolved Hide resolved
Library/OcCpuLib/OcCpuLib.c Outdated Show resolved Hide resolved
Library/OcCpuLib/OcCpuLib.c Outdated Show resolved Hide resolved
Library/OcCpuLib/OcCpuLib.c Outdated Show resolved Hide resolved
@Leoyzen
Copy link
Contributor Author

Leoyzen commented Oct 24, 2019

@Download-Fritz You are right, I found some imformation from here:

Definition of the Generic CPUID space.
Leaf 0x40000010, Timing Information.

   VMware has defined the first generic leaf to provide timing
   information.  This leaf returns the current TSC frequency and
  current Bus frequency in kHz.

   # EAX: (Virtual) TSC frequency in kHz.
   # EBX: (Virtual) Bus (local apic timer) frequency in kHz.
   # ECX, EDX: RESERVED (Per above, reserved fields are set to zero).

So we can just get tsc and bus frequency from 0x40000010 and caculate the max-bus-ratio.

@Leoyzen
Copy link
Contributor Author

Leoyzen commented Oct 24, 2019

I successfully get correct TSCFrequence and FSBFrequency from VMWare Timing node (0x40000010), although kvm hardcode the FSBFrequency with 1GHz.

    if (cpu->vmware_cpuid_freq
        /* Guests depend on 0x40000000 to detect this feature, so only expose
         * it if KVM exposes leaf 0x40000000. (Conflicts with Hyper-V) */
        && cpu->expose_kvm
        && kvm_base == KVM_CPUID_SIGNATURE
        /* TSC clock must be stable and known for this feature. */
        && tsc_is_stable_and_known(env)) {

        c = &cpuid_data.entries[cpuid_i++];
        c->function = KVM_CPUID_SIGNATURE | 0x10;
        c->eax = env->tsc_khz;
        /* LAPIC resolution of 1ns (freq: 1GHz) is hardcoded in KVM's
         * APIC_BUS_CYCLE_NS */
        c->ebx = 1000000;
        c->ecx = c->edx = 0;

        c = cpuid_find_entry(&cpuid_data.cpuid, kvm_base, 0);
        c->eax = MAX(c->eax, KVM_CPUID_SIGNATURE | 0x10);
    }

image

I will submit another commit to make it happend.

Issue: acidanthera/bugtracker#531
For some reason that some msr not impletement in such hypervisor (QEMU/KVM/VMWare), so some logic may not work fine in a VM.
This commit will add cpuid hypervisor support to indicate whether we are
under visualization, then we do some tricks to make it work.
@mhaeuser
Copy link
Member

although kvm hardcode the FSBFrequency with 1GHz
@Leoyzen And is this a problem? For virtualised OSes, we do not care for the system's frequencies afterall. Not to mention that there has not been something like a "FSB Frequency" for several hardware generations and it's merely "some base frequency" at this point. Is there any technical difficulty arising from this?

@Leoyzen
Copy link
Contributor Author

Leoyzen commented Oct 24, 2019

@Download-Fritz No, I just say that it "hardcore" so maybe not much sense to get it from 0x40000010.
Besides I don't know much that how much performance impact of different 'FSBFrequence' and 'Busratio' in a VM.

I know there are many other supervisor like VMWare or Xen so we definately should get if from 0x4000010 instead of hard code it. I'm just saying, not quite a problem for me.

Include/Library/OcCpuLib.h Outdated Show resolved Hide resolved
@vit9696
Copy link
Contributor

vit9696 commented Oct 24, 2019

I agree with the timing node suggestion, please do that. Given the scope of the change we can handle it within this pull request for simplicity.

According to these links:
    1. [CPUID usage for interaction between Hypervisors and Linux.](https://lwn.net/Articles/301888/)
    2. [[Qemu-devel] [PATCH v2 0/3] x86-kvm: Fix Mac guest timekeeping by exposi](https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg04344.html)

There is a "VMWare Timing" Node located at 0x40000010 where we can get
TSC/FSB Frequency.It is more sensable than get those freq in MSR which
most hypervisor not implemented yet (KVM/VMWare).
@Leoyzen
Copy link
Contributor Author

Leoyzen commented Oct 25, 2019

Commit have updated to support VMWare Timing node.

@Leoyzen
Copy link
Contributor Author

Leoyzen commented Oct 25, 2019

maybe we should check 0x4000000 first to see if kvm exposed.
Sometimes the hypversior is enable but kvm didn't exposed then the vmware timing is not work.

@Leoyzen
Copy link
Contributor Author

Leoyzen commented Oct 26, 2019

BTW, is there any difference between different bus ratio for MacOS in a VM?
Like my cpu frequency is 3.6GHz, the FSBFrequency of kvm is 1GHz, so what is the difference between 3 and 4 bus ratio?

Copy link
Contributor

@vit9696 vit9696 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I checked the code, and I do not believe I have issues code wise. However, there are several issues style wise and English-wise:

  • Virtualization (not visualization)
  • Comment style
  • Indentation should be 2 spaces (not 4 spaces)

I commented on most of the issues inline. Please handle that so we can check once again and merge. Thanks for working on the matter.

Include/Library/OcCpuLib.h Outdated Show resolved Hide resolved
Library/OcCpuLib/OcCpuLib.c Outdated Show resolved Hide resolved
Library/OcCpuLib/OcCpuLib.c Outdated Show resolved Hide resolved
Library/OcCpuLib/OcCpuLib.c Outdated Show resolved Hide resolved
Library/OcCpuLib/OcCpuLib.c Outdated Show resolved Hide resolved
Library/OcCpuLib/OcCpuLib.c Outdated Show resolved Hide resolved
Library/OcCpuLib/OcCpuLib.c Outdated Show resolved Hide resolved
Library/OcCpuLib/OcCpuLib.c Show resolved Hide resolved
Library/OcCpuLib/OcCpuLib.c Outdated Show resolved Hide resolved
Library/OcCpuLib/OcCpuLib.c Outdated Show resolved Hide resolved
@vit9696
Copy link
Contributor

vit9696 commented Oct 26, 2019

BTW, is there any difference between different bus ratio for MacOS in a VM?
Like my cpu frequency is 3.6GHz, the FSBFrequency of kvm is 1GHz, so what is the difference between 3 and 4 bus ratio?

I do not know of anything about it. Probably nothing?

@Leoyzen Leoyzen force-pushed the qemu-kvm-support branch 3 times, most recently from 786f48b to 4b06fed Compare October 26, 2019 17:06
@Leoyzen
Copy link
Contributor Author

Leoyzen commented Oct 26, 2019

Typo updates. We should squash these commits.

Also I'm sorry for the coding style and typos.

Copy link
Contributor

@vit9696 vit9696 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fixes, I will squash the commits upon merge at no issue. There still are more issues left, I marked some inline but unlikely all.

Library/OcCpuLib/OcCpuLib.c Show resolved Hide resolved
Library/OcCpuLib/OcCpuLib.c Show resolved Hide resolved
Library/OcCpuLib/OcCpuLib.c Outdated Show resolved Hide resolved
Library/OcCpuLib/OcCpuLib.c Outdated Show resolved Hide resolved
Copy link
Contributor Author

@Leoyzen Leoyzen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some improvements should be made, but I need some advises.

Library/OcCpuLib/OcCpuLib.c Outdated Show resolved Hide resolved
Library/OcCpuLib/OcCpuLib.c Outdated Show resolved Hide resolved
Copy link
Contributor

@vit9696 vit9696 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We definitely need some linter not to trouble the authors with such stuff, but I spotted more.

Library/OcCpuLib/OcCpuLib.c Outdated Show resolved Hide resolved
Library/OcCpuLib/OcCpuLib.c Outdated Show resolved Hide resolved
Library/OcCpuLib/OcCpuLib.c Outdated Show resolved Hide resolved
Library/OcCpuLib/OcCpuLib.c Outdated Show resolved Hide resolved
Library/OcCpuLib/OcCpuLib.c Show resolved Hide resolved
Library/OcCpuLib/OcCpuLib.c Outdated Show resolved Hide resolved
Library/OcCpuLib/OcCpuLib.c Outdated Show resolved Hide resolved
@vit9696 vit9696 changed the title Add cpuid hypervisor support to work with visualization Add cpuid hypervisor support to work with virtualization Oct 27, 2019
@Leoyzen
Copy link
Contributor Author

Leoyzen commented Oct 27, 2019

@vit9696 I'm so sorry about those stupid mistakes, but I'm not quite familar with these type programming and my IDE is quite simple so I'm not seeing it out.
I will do more check ensure that do not bother you any more.

Include/Library/OcCpuLib.h Outdated Show resolved Hide resolved
Library/OcCpuLib/OcCpuLib.c Outdated Show resolved Hide resolved
Library/OcCpuLib/OcCpuLib.c Outdated Show resolved Hide resolved
Library/OcCpuLib/OcCpuLib.c Outdated Show resolved Hide resolved
Copy link
Contributor Author

@Leoyzen Leoyzen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After I add this more accurate procedure to determine whether we've got the frequency already, it still have chance failed some way (like hypervisor didn't expose the VMWare Timing Leaf), then we fallback to the old procedure, and intel part is ok but amd part will crash because of some unsafe dividing.Should we change them in this PR?

@vit9696
Copy link
Contributor

vit9696 commented Oct 28, 2019

This looks good enough to me now, @Download-Fritz do you have any objections to merge it? If not, I would like to do that tomorrow or so.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants