This will break the ABI. I think you should consider adding a new one called Virtio9PGeneric

There is no need to change this one as there is already a generic version.

Again I think you'll need to a VirtioSerialGeneric

Rather than add -ccw version add a generic version.

for vhost-vsock generic version does not exist. But both vhost-vsock-ccw and vhost-vsock-pci exist hence I created a new device

Can you add generic versions of this and those below rather than renaming.

there is not alias in this case, add a variable as for vfio-pci

I understand why you split it in 2 switches, but I personally don't like it. If the maintainers agree, I would rather put a variable virtioNet and vfio and and a if statement at the beginning. Something like this

var virtioNet = "virtio-net-pci"
var vfio = "vfio-pci"

if runtime.GOARCH == "s390x" {
   virtioNet = "virtio-net-ccw"
   vfio = "vfio-ccw"
}

and substitue the string in the original switch with the variable

You can save some lines by adding a generic VhostVSOCK and assign the ccw value if the architecture is s390x

Generic vhost-vsock does not exist but to avoid unnecessary duplication, we can set VhostVSOCKPCI to "vhost-vsock-ccw" on Z. Not sure if the maintainers want this.

I'd use the generic variable and remove this if-else statement

see comment above

Let's wait for @markdryan or @rbradford to comment here, but I'd be tempted to remove all these 390-specific if tests and create qemu_s390x.go like we do in the runtime:

• [WIP] Runtime : Add support for Z runtime#667 (comment)

To minimise code duplication, you can create generic functions and variables that all other arches can call.

I think adding a new specific file for s390x is not necessary, there are not many points where arch specific if tests are used. But if you still insist I am okay with it.

I'd explicitly use -ccw

Maybe also a short comment that on s390x the -pci devices don't properly work. That's why you introduce the init function

The same as for the other init function. Add a short comment

please add this variable to the list of variables at the beginning of the file

I think this if..else condition is not needed since you can use the variable vfio, right?

No vfio is not an available device

sorry I don't understand why you can't do something like this

deviceParam = fmt.Sprintf("%s,host=%s", vfio, vfioDev.BDF)

might be I missed something, please help me to understand

Other devices like virtio-net are alias to virtio-net-ccw on Z and virtio-net-pci on others. However, a similar alias for vfio does not exist. Hence this needs to be kept as is.

So just writing 'vfio' would result in an error

However, a similar alias for vfio does not exist

what do you mean?

At the beginning of the file you are declaring Vfio = "vfio-pci" but you change its value at init() if runtime.GOARCH == "s390x", I wonder if is possible to use theVfio variable or declare a new variable (like you did with VirtioSCSI) in order to rid of this if...else condition.

let me update my example

deviceParam = fmt.Sprintf("%s,host=%s", Vfio, vfioDev.BDF)

yup that's possible... I thought you were asking me to change vfio-pci to generic vfio

@devimc Updated

you can declare a variable for virtio-scsi-pci and change it value to virtio-scsi-ccw at at init()

you don't need the if-else you use the same variable. Actually, the two lines are the same

Oops oversight

now I think this condition is not required, since VirtioSCSI changes its values at init() if runtime.GOARCH == "s390x", right?

sorry oversight changes done

I think this is a very bad idea. We should use the generic versions for all those that have aliases in QEMU. Those that don't should be managed explicitly.

Actually the QEMU team is planning on deprecating those alias in favour of the -ccw alternatives. Hence this was done.

@rbradford Keeping them to -ccw would also allow us to have better ABI compatibility

@rbradford @devimc Do you still want me to make this change?

Let's wait for @markdryan or @rbradford to comment here, but I'd be tempted to remove all these 390-specific if tests and create qemu_s390x.go like we do in the runtime:

• [WIP] Runtime : Add support for Z runtime#667 (comment)

To minimise code duplication, you can create generic functions and variables that all other arches can call.

Please don't remove the const here 😄

I am setting the value of these variables at runtime. So making them const is not really possible.

However, since you and others have raised concerns with this and the arch specific if-else, let me just refactor this into a separate file.

This is the only difference between this function and the one in qemu_s390x.go so you could make this a generic function and define something like the following:

• qemu_amd64.go

  const vsockDevice = VhostVSOCKPCI
  

• qemu_s390x.go

  const vsockDevice = VhostVSOCKCCW
  

... however, that would require you to create qemu_amd64.go of course :)

@jodh-intel PCI is also used on ARM so this probably not you want.

Why the "double comments"? I'd remove the C-style comments from this file and qemu_s390x.go as they are redundant.

For some mysterious reasons, the go build tool fails to build without this on my instances hence I have kept them

Again, for consistency with the qemu*.go files, I'd rework this as follows:

• Create qmp_amd64.go and qmp_s390x.go containing their own implementations of:

  var archPCIVFIODeviceAddMap = map[string]interface{}{ ... }
  

• Make qmp.go reference the right implementation. Something like:

  var pciVFIODeviceAddMap = archPCIVFIODeviceAddMap
  
  func (q *QMP) ExecutePCIVFIODeviceAdd(...) {
        arg := archPCIVFIODeviceAddMap
        return q.executeCommand(ctx, "device_add", args, nil)
  }
  

@jodh-intel PCI is also used on ARM so this probably not you want.

This grates me. But I understand why you did it. Maybe we could transition all the users to a more generic name and eventually get rid of this?

Add a new variable VhostVSOCK...where do I include the deprecation notice?

please remove this function, declare these variables

var deviceFSString = "-device virtio-9p-ccw,disable-modern=true,fsdev=workload9p,mount_tag=rootfs -fsdev local,id=workload9p,path=/var/lib/docker/devicemapper/mnt/e31ebda2,security_model=none"
var deviceNetworkString = "-netdev tap,id=tap0,vhost=on,ifname=ceth0,downscript=no,script=no -device driver=virtio-net,netdev=tap0,mac=01:02:de:ad:be:ef,disable-modern=true"
....

and move the declaration of these variables from qemu_test.go to qemu_generic_test.go

please remove this function