Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

qmp: add checks for the CPU toplogy #103

Merged
merged 2 commits into from
Jul 26, 2019

Conversation

alicefr
Copy link
Member

@alicefr alicefr commented Jul 22, 2019

Support for function isSocketIDSupported, isThreadIDSupported and isDieIDSupported.
The functions check if the cpu driver and the qemu version support the
id parameter.

Fixes: #102

The support table for various param looks like:

Arch cpu driver socket id thread id die id cpu hotplug
x86_64 host-x86_64-cpu y y y ( >= Qemu 4.1) y
s390x host-s390x-cpu n n n y
arm64 host-arm-cpu y y n n
ppc64le host-powerpc64-cpu n n n y

taken from #101 (comment)

Signed-off-by: Alice Frosi afrosi@de.ibm.com

@alicefr
Copy link
Member Author

alicefr commented Jul 22, 2019

The PR needs to be rebased on top of #101

@coveralls
Copy link

coveralls commented Jul 22, 2019

Coverage Status

Coverage increased (+0.2%) to 79.923% when pulling 68cdf64 on alicefr:cpu_topology into e894e7a on intel:master.

@devimc
Copy link

devimc commented Jul 22, 2019

l g t m. @alicefr could you add some unit tests?

@devimc
Copy link

devimc commented Jul 24, 2019

Hi @alicefr any update on this? also looks like the CI is not happy

qemu/qmp.go:1177:15:warning: func (*QMP).isDieIDSupported is unused (U1000) (staticcheck)

@alicefr
Copy link
Member Author

alicefr commented Jul 25, 2019

@devmic I can add some tests, and it's normal that the CI complains. The function isDieIDSupported is not used because I'm waiting for the #101 and I need to rebase mine on top and remove the if that I move inside the function isDieIDSupported. Is it ok the flow for you?

@devimc
Copy link

devimc commented Jul 25, 2019

@alicefr sounds good to me 😃

@alicefr alicefr force-pushed the cpu_topology branch 2 times, most recently from 3e47a12 to 7afdaf0 Compare July 25, 2019 13:31
@alicefr
Copy link
Member Author

alicefr commented Jul 25, 2019

@devimc I rebased the PR and added the tests

qemu/qmp.go Outdated

// isDieIDSupported returns if the cpu driver and the qemu version support the die id option
func (q *QMP) isDieIDSupported(driver string) bool {
if q.version.Major > 4 || (q.version.Major == 4 && q.version.Minor >= 1) && driver == "qemu64-x86_64-cpu" {
Copy link

Choose a reason for hiding this comment

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

in my system the driver is host-x86_64-cpu 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I had that previously but then I saw the test and here

Copy link

Choose a reason for hiding this comment

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

heh - I think qemu64-x86_64-cpu was used just for testing 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

@sboeuf Any idea what the correct cpu driver to use here is? Perhaps we need to check for both. Basically, we only want die-id to be specified on x86 systems.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok changed back to host-x86_64-cpu

Copy link

Choose a reason for hiding this comment

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

@markdryan host-x86_64-cpu is correct

$ qemu-system-x86_64 -device host-x86_64-cpu,help
host-x86_64-cpu options:
  min-xlevel2=<uint32>
  amd-ssbd=<bool>
  vendor=<string>
...

Copy link

Choose a reason for hiding this comment

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

and the qmp command query-hotpluggable-cpus returns this:

{"props": {"core-id": 0, "thread-id": 0, "node-id": 0, "socket-id": 7}, "vcpus-count": 1, "type": "host-x86_64-cpu"}

Copy link

@devimc devimc left a comment

Choose a reason for hiding this comment

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

thanks @alicefr

@devimc
Copy link

devimc commented Jul 26, 2019

@markdryan can we merge this?

qemu/qmp.go Outdated

// isDieIDSupported returns if the cpu driver and the qemu version support the die id option
func (q *QMP) isDieIDSupported(driver string) bool {
if q.version.Major > 4 || (q.version.Major == 4 && q.version.Minor >= 1) && driver == "host-x86_64-cpu" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this test correct? Won't it always enable die-id for qemu version 5 and above regardless of the driver?

Copy link

Choose a reason for hiding this comment

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

good catch!

Copy link
Member Author

Choose a reason for hiding this comment

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

I added the parenthesis, sorry

Alice Frosi added 2 commits July 26, 2019 14:27
Support for function isSocketIDSupported, isThreadIDSupported and isDieIDSupported.
The functions check if the cpu driver and the qemu version support the
id parameter.

Fixes: kata-containers#102

Signed-off-by: Alice Frosi <afrosi@de.ibm.com>
Add cpu driver types in TestQMPCPUDeviceAdd

Signed-off-by: Alice Frosi <afrosi@de.ibm.com>
Copy link
Contributor

@markdryan markdryan left a comment

Choose a reason for hiding this comment

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

LGTM

@devimc devimc merged commit e050524 into kata-containers:master Jul 26, 2019
@Shunnnnnnnn
Copy link

Hi!Does arm64 support cpu hotplug?

@devimc
Copy link

devimc commented Oct 21, 2019

@Shunnnnnnnn no,
cc @Pennyzct

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

Successfully merging this pull request may close these issues.

s390x: add checks for the CPU toplogy
5 participants