qemu: Fix iommu_platform for CCW

[Jakob-Naucke]

• Remove extra comma when device parameters are already joined with
  commas
• Enable iommu_platform for vhost user devices

Fixes: #178

Signed-off-by: Jakob Naucke jakob.naucke@ibm.com

[jodh-intel]

Thanks @Jakob-Naucke. ooi, did you spot this problem in testing or by code inspection alone?

Looking over this file, there are a lot of functions (88, 18 of which are QemuConfig() ones ;) and each seems to handle building up these params differently (either appending with a comma, then joining with a null string; or appending without a separator, then joining with a comma).

It's not required for this PR, but I do think we need to make the handling more consistent generally.

Maybe you could add a unit test for the functions you've changed to ensure we don't end up with either a missing comma-separator or a double-comma though?

[Jakob-Naucke]

Thanks @Jakob-Naucke. ooi, did you spot this problem in testing or by code inspection alone?

@jodh-intel this was in testing, by enabling enable_iommu_platform (kata-containers/kata-containers#636). I thought I'd need it explicitly for running with a Secure Execution protected image, but this is done implicitly by kata-containers/kata-containers#1772, and all I needed was kata-containers/kata-containers#2107.

Looking over this file, there are a lot of functions (88, 18 of which are QemuConfig() ones ;) and each seems to handle building up these params differently (either appending with a comma, then joining with a null string; or appending without a separator, then joining with a comma).

It's not required for this PR, but I do think we need to make the handling more consistent generally.

I noticed :) I might get into it when I find the time.

Maybe you could add a unit test for the functions you've changed to ensure we don't end up with either a missing comma-separator or a double-comma though?

Sure, let me get back to you.

[Jakob-Naucke]

Hmmm. There is one test that tests IOMMUPlatform for FSDevice:

govmm/qemu/qemu_s390x_test.go

Line 84 in 263136e

func TestAppendDeviceFSCCWIOMMU(t *testing.T) {

but that's 25 lines for that tiny flag, and that times all the devices... the ideal thing would be to refactor testing to allow for these flags... that's a lot though. Hmmm.

[jodh-intel]

@Jakob-Naucke - ack. Let's just raise an issue to track this as you can't be expected to rewrite most of the tests for a relatively small change like this.

[Jakob-Naucke]

See #180, #181

[jodh-intel]

Thanks @Jakob-Naucke.