Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "cpu-kvm: Support perf counters on hybrid host architectures" #1127

Merged
merged 1 commit into from
May 21, 2024

Conversation

Harshil2107
Copy link
Contributor

Reverts #1065

Reverting this change because this PR breaks X86 kvm as mentioned in the issue #1126.

@Harshil2107
Copy link
Contributor Author

@nmosier @andysan ping to inform you of this.

@andysan
Copy link
Contributor

andysan commented May 14, 2024

Wouldn't it be better to just disable this feature by default instead? That way, we can treat the feature as experimental and we don't end up reverting a large amount of code (less disruptive). It's presumably working on some systems when enabled.

@ivanaamit ivanaamit added the cpu-kvm gem5's KVM CPU label May 14, 2024
@powerjg
Copy link
Contributor

powerjg commented May 15, 2024

Related: #1138

@ivanaamit
Copy link
Contributor

@nmosier , @andysan : We will merge this on Monday if there is no new patch that fixes this issue.

Change-Id: Ieff05ae342cdc9e3b23718e365ff86b83784d41f
@ivanaamit ivanaamit merged commit 0824d7f into develop May 21, 2024
37 checks passed
BobbyRBruce pushed a commit to BobbyRBruce/gem5 that referenced this pull request May 25, 2024
…em5#1127)

Reverts gem5#1065

Reverting this change because this PR breaks X86 kvm as mentioned in the
issue gem5#1126.
@nmosier
Copy link
Contributor

nmosier commented May 26, 2024

Sorry for the late reply.

I agree with @andysan — I think it would be better to simply disable hybrid performance counters by default and treat them as an experimental feature rather than just revert the entire patch. Reverting this patch has once again made the KVM CPU non-functional on the E-cores of my Alder Lake system.

Can I just resubmit this patch with this line changed from True to False, disabling hybrid performance counters by default? Unfortunately, I don’t think I’ll have time in the forseeable future to debug issue #1126. However, in the meantime, it would be nice to be able to use the KVM CPU on my Alder Lake E-cores.

@powerjg
Copy link
Contributor

powerjg commented May 27, 2024

I agree that it would be good to have support for hybrid performance counters. However, we were not able to get #1065 to work on our systems (see discussion here: #1126 (comment) apologies for the information being spread around).

I am positive that there are some problems with the current KVM support. We are also having issues here: #1119 (comment)

I'm also wary of adding parameters to SimObjects that are required for different hosts. I don't like the idea of tying simulation parameters to a host.

That said, if we can 1) document the problems and which hosts are causing the issue and 2) explain the needed configurations for different hosts, then we can resubmit the patch. However, we need to do a lot more testing this time before committing 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cpu-kvm gem5's KVM CPU
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants