qemu: support x86 SMP die

[devimc]

In QEMU 4.1 the CPU topology for x86 will change to:
socket > die > core > thread.
Add die-id field to CPUProperties and include it in CPU hotplugging

Signed-off-by: Julio Montes julio.montes@intel.com

[coveralls]

Coverage increased (+0.05%) to 79.699% when pulling a5c1190 on devimc:topic/supportQemu41 into 52b2309 on intel:master.

[markdryan]

@devimc What happens if you pass the dieid parameter to an earlier version of qemu? Will it just be ignored? Will everything work as expected?

[devimc]

@markdryan no, it's not ignored, {"error": {"class": "GenericError", "desc": "Property '.die-id' not found"}}. QEMU 4.1 will fail with a similar error if this property is not specified 😟

[markdryan]

@devimc We've encountered these sort of issues before. In these cases an explicit version check has been placed in the method, e.g.,

https://github.com/intel/govmm/blob/master/qemu/qmp.go#L764

Could we do something like that here? We also need to make sure that we don't break s390 on qemu 4.1 and above.

[devimc]

@markdryan changes applied, thanks

[markdryan]

@alicefr Is this change likely to cause problems for s390?

[alicefr]

@manowared I need to check, I'll do that today

[alicefr]

@manowared I got the error qemu-system-s390x: Property '.dies' not found.

[markdryan]

@alicefr Are you testing on qemu 4.1 with this patch applied on s390x?

[markdryan]

I guess what I'm trying to figure out is that if you are getting this error on qemu 4.1 on s490 with this patch, we'd need to find a way to make to disable this change on s390x.

@devimc When does kata call the ExecuteCPUDeviceAdd method? Does this method get called on the s390x builds as well?

[alicefr]

I simply tried to boot qemu (4.1) with -smp ,dies=1, not with kata + govmm and this change. I'm also looking at the code to find a way how to avoid that

[markdryan]

I simply tried to boot qemu (4.1) with -smp ,dies=1, not with kata + govmm and this change.

@alicefr Thanks. I think that's enough information though. It looks like we probably do need to find a way to avoid this new code being run on the s390 builds.

[alicefr]

So, yes you need to avoid that for s390x, not sure about power /cc @nitkon . I guess it's the same for the socket and thread options. If you look at the kata-runtime code (https://github.com/kata-containers/runtime/blob/bc15e442459c95b7221d5f4097ff85798d6f0fc8/virtcontainers/qemu.go#L1252), those param are not set. The problem is that you're checking if dieID is empty or not and the trick to leave that empty for other arch won't work. I think either we decide if the param is empty than we don't set this option (I don't like it) or we can find a way to identify the architecture and avoid to set that option (a little bit more trick to implement but much more clean)

[markdryan]

So we could merge this and simply extend

(https://github.com/kata-containers/runtime/blob/bc15e442459c95b7221d5f4097ff85798d6f0fc8/virtcontainers/qemu.go#L1252)

to check for die-id when the kata code is modified. It's not great but this seems to be the way things are done at the moment and there isn't currently any architecture specific checks in qmp.go. @devimc are you planning to update the virtcontainers code once this is merged?

[alicefr]

@devimc I think you could simply check the driver type here. Something like && driver=="x86_64" . Do you know if dieID is valid for power and arm too?

[markdryan]

Whatever we do, we probably want to have one mechanism for handling socket, thread and die. Either we handle all three here or we handle them in virtcontainers.

[alicefr]

Ok, agree. If you want I can open a small PR to do that. Something like:

bool isSocketSupported(driver string)  {
        if driver == "host-s390x-cpu" && driver "host-ppc64le-cpu" {
                return false
        }
        return true
}

and then in "ExecuteCPUDeviceAdd"

        if isSocketSupported(driver) {
                        args["socket-id"] = socketID

        }

we could do that for all 3 param

[markdryan]

@devimc, @sboeuf What do you guys think? Should we move the platform specific checks in virtcontainers into govmm?

[devimc]

@markdryan sounds good, I will update this pr

[Pennyzct]

Hi~ @alicefr @justin-he is working on this filed~ ;)

[justin-he]

ok then, in the future will be enough to update the function. For me the support table looks like

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

Hi @alicefr
The cpu driver of arm64 in qemu (virt machine type) should be "driver=host-arm-cpu"
But it is worth to note that the cpu hotplug (what kata required) has not been supported yet.

[devimc]

@justin-he thanks, @alicefr @markdryan can we merge this?

[alicefr]

@devimc for me yes, I'll open a PR for the threadID, socketID and dieID

[alicefr]

I open a PR #103 with the changes with the check for the 3 params

[devimc]

@markdryan @alicefr can we merge this?

[markdryan]

@devimc Yes, we can I think. We then need to rebase and update #103 right?

[markdryan]

at which point the build error should disappear.

[markdryan]

LGTM