Automated cherry pick of #25313: Fix/device add params2 (#25315)

* fix(host): device add params use interface

* fix(host): ide device addr

pkg/hostman/guestman/guesttasks.go

@@ -294,7 +294,7 @@ func (d *SGuestDiskSyncTask) changeCdrom(cdrom *desc.SGuestCdrom) {
 				}
 				cb := func(res string) {
 					cdrom.Scsi.Options["drive"] = cdrom.Id
-					d.guest.Monitor.DeviceAdd(cdrom.Scsi.DevType, cdrom.Scsi.Options, cb2)
+					d.guest.Monitor.DeviceAdd(cdrom.Scsi.DevType, monitor.StringParams(cdrom.Scsi.Options), cb2)
 				}
 				params := map[string]string{}
 				for k, v := range cdrom.DriveOptions {
@@ -471,7 +471,7 @@ func (d *SGuestDiskSyncTask) checkDiskDriver(disk *desc.SGuestDisk) {
 			d.checkDrivers = append(d.checkDrivers, DISK_DRIVER_SCSI)
 			d.startAddDisk(disk)
 		}
-		params := map[string]string{
+		params := map[string]interface{}{
 			"id":   d.guest.Desc.VirtioScsi.Id,
 			"bus":  d.guest.Desc.VirtioScsi.BusStr(),
 			"addr": d.guest.Desc.VirtioScsi.SlotFunc(),
@@ -504,7 +504,7 @@ func (d *SGuestDiskSyncTask) checkDiskDriver(disk *desc.SGuestDisk) {
 			d.checkDrivers = append(d.checkDrivers, DISK_DRIVER_SATA)
 			d.startAddDisk(disk)
 		}
-		params := map[string]string{
+		params := map[string]interface{}{
 			"id":   d.guest.Desc.SataController.Id,
 			"bus":  d.guest.Desc.SataController.BusStr(),
 			"addr": d.guest.Desc.SataController.SlotFunc(),
@@ -564,7 +564,11 @@ func (d *SGuestDiskSyncTask) startAddDisk(disk *desc.SGuestDisk) {
 		}
 		bus = d.guest.GetPciBus()
 	case DISK_DRIVER_IDE:
-		bus = fmt.Sprintf("ide.%d", diskIndex/2)
+		var busNum = diskIndex / 2
+		if d.guest.Desc.Machine == api.VM_MACHINE_TYPE_Q35 {
+			busNum = diskIndex
+		}
+		bus = fmt.Sprintf("ide.%d", busNum)
 	case DISK_DRIVER_SATA:
 		bus = fmt.Sprintf("ahci0.%d", diskIndex)
 	}
@@ -604,7 +608,7 @@ func (d *SGuestDiskSyncTask) onAddDiskSucc(disk *desc.SGuestDisk, results string
 	}
 
 	d.guest.Desc.Disks = append(d.guest.Desc.Disks, disk)
-	var params = map[string]string{
+	var params = map[string]interface{}{
 		"drive": fmt.Sprintf("drive_%d", diskIndex),
 		"id":    fmt.Sprintf("drive_%d", diskIndex),
 	}
@@ -882,7 +886,7 @@ func (n *SGuestNetworkSyncTask) onNetdevAdd(nic *desc.SGuestNetwork, cType *desc
 		return
 	}
 
-	params := map[string]string{
+	params := map[string]interface{}{
 		"id":     fmt.Sprintf("netdev-%s", nic.Ifname),
 		"netdev": nic.Ifname,
 		"mac":    nic.Mac,
@@ -2862,7 +2866,7 @@ func (task *SGuestHotplugCpuMemTask) onAddMemObject(reason string, memSlot *desc
 		task.onAddMemFailed(reason)
 		return
 	}
-	params := map[string]string{
+	params := map[string]interface{}{
 		"id":     fmt.Sprintf("dimm%d", *task.memSlotNewIndex),
 		"memdev": fmt.Sprintf("mem%d", *task.memSlotNewIndex),
 	}

pkg/hostman/guestman/qemu/generate.go

@@ -273,7 +273,7 @@ func generateKickstartBootOptions(drvOpt QemuOptions, kickstartBoot *KickstartBo
 	return opts
 }
 
-func generateDisksOptions(drvOpt QemuOptions, disks []*desc.SGuestDisk, isEncrypt, isMaster bool, osName string) []string {
+func generateDisksOptions(drvOpt QemuOptions, disks []*desc.SGuestDisk, isEncrypt, isMaster bool, osName, machineType string) []string {
 	opts := make([]string, 0)
 	for _, disk := range disks {
 		if disk.Driver == api.DISK_DRIVER_VFIO {
@@ -285,7 +285,7 @@ func generateDisksOptions(drvOpt QemuOptions, disks []*desc.SGuestDisk, isEncryp
 		} else {
 			opts = append(opts, getDiskDriveOption(drvOpt, disk, isEncrypt))
 		}
-		opts = append(opts, getDiskDeviceOption(drvOpt, disk, osName))
+		opts = append(opts, getDiskDeviceOption(drvOpt, disk, osName, machineType))
 	}
 	return opts
 }
@@ -352,7 +352,7 @@ func isLocalStorage(disk *desc.SGuestDisk) bool {
 	}
 }
 
-func getDiskDeviceOption(optDrv QemuOptions, disk *desc.SGuestDisk, osName string) string {
+func getDiskDeviceOption(optDrv QemuOptions, disk *desc.SGuestDisk, osName, machineType string) string {
 	diskIndex := disk.Index
 	diskDriver := disk.Driver
 	numQueues := disk.NumQueues
@@ -379,7 +379,11 @@ func getDiskDeviceOption(optDrv QemuOptions, disk *desc.SGuestDisk, osName strin
 	} else if utils.IsInStringArray(diskDriver, []string{DISK_DRIVER_SCSI, DISK_DRIVER_PVSCSI}) {
 		opt += ",bus=scsi.0"
 	} else if diskDriver == DISK_DRIVER_IDE {
-		opt += fmt.Sprintf(",bus=ide.%d,unit=%d", diskIndex/2, diskIndex%2)
+		if machineType == api.VM_MACHINE_TYPE_Q35 {
+			opt += fmt.Sprintf(",bus=ide.%d,unit=%d", diskIndex, 0)
+		} else {
+			opt += fmt.Sprintf(",bus=ide.%d,unit=%d", diskIndex/2, diskIndex%2)
+		}
 	} else if diskDriver == DISK_DRIVER_SATA {
 		opt += fmt.Sprintf(",bus=ahci0.%d", diskIndex)
 	}
@@ -395,10 +399,16 @@ func getDiskDeviceOption(optDrv QemuOptions, disk *desc.SGuestDisk, osName strin
 	return optDrv.Device(opt)
 }
 
-func generateCdromOptions(optDrv QemuOptions, cdroms []*desc.SGuestCdrom) []string {
+func generateCdromOptions(optDrv QemuOptions, cdroms []*desc.SGuestCdrom, disks []*desc.SGuestDisk, machine string) []string {
 	opts := make([]string, 0)
+	ideDisksCnt := 0
+	for _, disk := range disks {
+		if disk.Driver == DISK_DRIVER_IDE {
+			ideDisksCnt += 1
+		}
+	}
 
-	for _, cdrom := range cdroms {
+	for idx, cdrom := range cdroms {
 		//cdromDriveId := cdrom
 		driveOpt := fmt.Sprintf("id=%s", cdrom.Id)
 		driveOpt += desc.OptionsToString(cdrom.DriveOptions)
@@ -409,9 +419,21 @@ func generateCdromOptions(optDrv QemuOptions, cdroms []*desc.SGuestCdrom) []stri
 		}
 
 		if cdrom.Ide != nil {
+			var devOpt string
 			opts = append(opts, optDrv.Drive(driveOpt))
-			devOpt := fmt.Sprintf("%s,drive=%s,bus=ide.1",
-				cdrom.Ide.DevType, cdrom.Id)
+			if machine == api.VM_MACHINE_TYPE_Q35 {
+				busNum := idx + ideDisksCnt
+				if busNum == 0 {
+					busNum = 1
+				}
+				devOpt = fmt.Sprintf("%s,drive=%s,bus=ide.%d",
+					cdrom.Ide.DevType, cdrom.Id, busNum)
+			} else {
+				devOpt = fmt.Sprintf("%s,drive=%s,bus=ide.1",
+					cdrom.Ide.DevType, cdrom.Id)
+
+			}
+
 			if len(cdromPath) > 0 {
 				if cdrom.BootIndex != nil && *cdrom.BootIndex >= 0 {
 					devOpt += fmt.Sprintf(",bootindex=%d", *cdrom.BootIndex)
@@ -875,10 +897,10 @@ func GenerateStartOptions(
 
 	// generate disk options
 	opts = append(opts, generateDisksOptions(
-		drvOpt, input.GuestDesc.Disks, isEncrypt, input.GuestDesc.IsMaster, input.OsName)...)
+		drvOpt, input.GuestDesc.Disks, isEncrypt, input.GuestDesc.IsMaster, input.OsName, input.GuestDesc.Machine)...)
 
 	// cdrom
-	opts = append(opts, generateCdromOptions(drvOpt, input.GuestDesc.Cdroms)...)
+	opts = append(opts, generateCdromOptions(drvOpt, input.GuestDesc.Cdroms, input.GuestDesc.Disks, input.GuestDesc.Machine)...)
 
 	//floppy
 	opts = append(opts, generateFloppyOptions(drvOpt, input.GuestDesc.Floppys)...)

pkg/hostman/isolated_device/isolated_device.go

@@ -95,7 +95,7 @@ type IHost interface {
 
 type HotPlugOption struct {
 	Device  string
-	Options map[string]string
+	Options map[string]interface{}
 }
 
 type HotUnplugOption struct {
@@ -1112,7 +1112,7 @@ func (dev *SBaseDevice) GetHotPlugOptions(isolatedDev *desc.SGuestIsolatedDevice
 
 	var masterDevOpt *HotPlugOption
 	for i := 0; i < len(isolatedDev.VfioDevs); i++ {
-		opts := map[string]string{
+		opts := map[string]interface{}{
 			"host": isolatedDev.VfioDevs[i].HostAddr,
 			"bus":  isolatedDev.VfioDevs[i].BusStr(),
 			"addr": isolatedDev.VfioDevs[i].SlotFunc(),
@@ -1120,13 +1120,13 @@ func (dev *SBaseDevice) GetHotPlugOptions(isolatedDev *desc.SGuestIsolatedDevice
 		}
 		if isolatedDev.VfioDevs[i].Multi != nil {
 			if *isolatedDev.VfioDevs[i].Multi {
-				opts["multifunction"] = "on"
+				opts["multifunction"] = true
 			} else {
-				opts["multifunction"] = "off"
+				opts["multifunction"] = false
 			}
 		}
 		if isolatedDev.VfioDevs[i].XVga {
-			opts["x-vga"] = "on"
+			opts["x-vga"] = true
 		}
 		devOpt := &HotPlugOption{
 			Device:  isolatedDev.VfioDevs[i].DevType,

pkg/hostman/isolated_device/nvidia_vgpu.go

@@ -232,7 +232,7 @@ func (dev *sNVIDIAVgpuDevice) GetHotPlugOptions(isolatedDev *desc.SGuestIsolated
 	var masterDevOpt *HotPlugOption
 	for i := 0; i < len(isolatedDev.VfioDevs); i++ {
 		sysfsdev := path.Join("/sys/bus/mdev/devices", isolatedDev.MdevId)
-		opts := map[string]string{
+		opts := map[string]interface{}{
 			"sysfsdev": sysfsdev,
 			"bus":      isolatedDev.VfioDevs[i].BusStr(),
 			"addr":     isolatedDev.VfioDevs[i].SlotFunc(),
@@ -240,9 +240,9 @@ func (dev *sNVIDIAVgpuDevice) GetHotPlugOptions(isolatedDev *desc.SGuestIsolated
 		}
 		if isolatedDev.VfioDevs[i].Multi != nil {
 			if *isolatedDev.VfioDevs[i].Multi {
-				opts["multifunction"] = "on"
+				opts["multifunction"] = true
 			} else {
-				opts["multifunction"] = "off"
+				opts["multifunction"] = false
 			}
 		}
 

pkg/hostman/isolated_device/usb.go

@@ -68,7 +68,7 @@ func GetUSBDevId(vendorId, devId, bus, addr string) string {
 	return fmt.Sprintf("dev_%s_%s-%s_%s", vendorId, devId, bus, addr)
 }
 
-func getUSBDevQemuOptions(vendorId, deviceId string, bus, addr, port string) (map[string]string, error) {
+func getUSBDevQemuOptions(vendorId, deviceId string, bus, addr, port string) (map[string]interface{}, error) {
 	// id := GetUSBDevId(vendorId, deviceId, bus, addr)
 	busI, err := strconv.Atoi(bus)
 	if err != nil {
@@ -79,23 +79,31 @@ func getUSBDevQemuOptions(vendorId, deviceId string, bus, addr, port string) (ma
 		return nil, errors.Wrapf(err, "parse addr to int %q", bus)
 	}
 	if len(port) > 0 {
-		return map[string]string{
-			"hostbus":  fmt.Sprintf("%d", busI),
+		return map[string]interface{}{
+			"hostbus":  uint64(busI),
 			"hostport": port,
 		}, nil
 	}
 
-	return map[string]string{
+	vendorIDI, err := strconv.ParseUint(vendorId, 16, 32)
+	if err != nil {
+		return nil, errors.Wrapf(err, "parse vendor ID %q", vendorId)
+	}
+	productIDI, err := strconv.ParseUint(deviceId, 16, 32)
+	if err != nil {
+		return nil, errors.Wrapf(err, "parse product ID %q", deviceId)
+	}
+	return map[string]interface{}{
 		// "id": id,
 		// "bus":       "usb.0",
-		"vendorid":  fmt.Sprintf("0x%s", vendorId),
-		"productid": fmt.Sprintf("0x%s", deviceId),
-		"hostbus":   fmt.Sprintf("%d", busI),
-		"hostaddr":  fmt.Sprintf("%d", addrI),
+		"vendorid":  uint32(vendorIDI),
+		"productid": uint32(productIDI),
+		"hostbus":   uint64(busI),
+		"hostaddr":  uint64(addrI),
 	}, nil
 }
 
-func GetUSBDevQemuOptions(vendorDevId string, addr string) (map[string]string, error) {
+func GetUSBDevQemuOptions(vendorDevId string, addr string) (map[string]interface{}, error) {
 	parts := strings.Split(vendorDevId, ":")
 	if len(parts) != 2 {
 		return nil, fmt.Errorf("invalid vendor_device_id %q", vendorDevId)
@@ -128,7 +136,15 @@ func (dev *sUSBDevice) GetQemuId() string {
 
 func (dev *sUSBDevice) GetPassthroughOptions() map[string]string {
 	opts, _ := GetUSBDevQemuOptions(dev.dev.GetVendorDeviceId(), dev.dev.Addr)
-	return opts
+	ret := make(map[string]string, len(opts))
+	for k, v := range opts {
+		if k == "vendorid" || k == "productid" {
+			ret[k] = fmt.Sprintf("0x%x", v)
+			continue
+		}
+		ret[k] = fmt.Sprint(v)
+	}
+	return ret
 }
 
 func (dev *sUSBDevice) GetPassthroughCmd(index int) string {

pkg/hostman/isolated_device/usb_test.go

@@ -158,7 +158,7 @@ func Test_getUSBDevQemuOptions(t *testing.T) {
 	tests := []struct {
 		name string
 		args args
-		want map[string]string
+		want map[string]interface{}
 	}{
 		{
 			name: "",
@@ -168,11 +168,11 @@ func Test_getUSBDevQemuOptions(t *testing.T) {
 				bus:      "001",
 				addr:     "009",
 			},
-			want: map[string]string{
-				"vendorid":  "0x1d6b",
-				"productid": "0x0001",
-				"hostbus":   "1",
-				"hostaddr":  "9",
+			want: map[string]interface{}{
+				"vendorid":  uint32(0x1d6b),
+				"productid": uint32(0x0001),
+				"hostbus":   uint64(1),
+				"hostaddr":  uint64(9),
 			},
 		},
 	}

pkg/hostman/monitor/hmp.go

@@ -293,14 +293,24 @@ func (m *HmpMonitor) DriveAdd(bus, node string, params map[string]string, callba
 	m.Query(fmt.Sprintf("%s %s %s", cmd, bus, strings.Join(paramsKvs, ",")), callback)
 }
 
-func (m *HmpMonitor) DeviceAdd(dev string, params map[string]string, callback StringCallback) {
+func (m *HmpMonitor) DeviceAdd(dev string, params map[string]interface{}, callback StringCallback) {
 	var paramsKvs = []string{}
 	for k, v := range params {
-		paramsKvs = append(paramsKvs, fmt.Sprintf("%s=%s", k, v))
+		paramsKvs = append(paramsKvs, fmt.Sprintf("%s=%s", k, hmpDeviceValue(v)))
 	}
 	m.Query(fmt.Sprintf("device_add %s,%s", dev, strings.Join(paramsKvs, ",")), callback)
 }
 
+func hmpDeviceValue(value interface{}) interface{} {
+	if value, ok := value.(bool); ok {
+		if value {
+			return "on"
+		}
+		return "off"
+	}
+	return value
+}
+
 func (m *HmpMonitor) MigrateSetDowntime(dtSec float64, callback StringCallback) {
 	m.Query(fmt.Sprintf("migrate_set_downtime %f", dtSec), callback)
 }

pkg/hostman/monitor/monitor.go

@@ -219,7 +219,7 @@ type Monitor interface {
 
 	ObjectAdd(objectType string, params map[string]string, callback StringCallback)
 	DriveAdd(bus, node string, params map[string]string, callback StringCallback)
-	DeviceAdd(dev string, params map[string]string, callback StringCallback)
+	DeviceAdd(dev string, params map[string]interface{}, callback StringCallback)
 	DeviceAddCpu(dev string, params map[string]interface{}, callback StringCallback)
 
 	XBlockdevChange(parent, node, child string, callback StringCallback)
@@ -280,6 +280,15 @@ type SBaseMonitor struct {
 	reading bool
 }
 
+// StringParams preserves string-valued device properties while callers migrate to typed QMP arguments.
+func StringParams(params map[string]string) map[string]interface{} {
+	ret := make(map[string]interface{}, len(params))
+	for k, v := range params {
+		ret[k] = v
+	}
+	return ret
+}
+
 func NewBaseMonitor(server, sid string, OnMonitorConnected MonitorSuccFunc, OnMonitorDisConnect, OnMonitorTimeout MonitorErrorFunc) *SBaseMonitor {
 	return &SBaseMonitor{
 		OnMonitorConnected:  OnMonitorConnected,

pkg/hostman/monitor/qmp.go

@@ -532,25 +532,10 @@ func (m *QmpMonitor) DriveAdd(bus, node string, params map[string]string, callba
 	m.HumanMonitorCommand(cmd, callback)
 }
 
-func (m *QmpMonitor) DeviceAdd(dev string, params map[string]string, callback StringCallback) {
-	args := map[string]interface{}{
-		"driver": dev,
-	}
-
-	for k, v := range params {
-		var iv interface{}
-		iv = v
-		if v == "on" || v == "true" {
-			iv = true
-		} else if v == "off" || v == "false" {
-			iv = false
-		}
-		args[k] = iv
-	}
-
+func (m *QmpMonitor) DeviceAdd(dev string, params map[string]interface{}, callback StringCallback) {
 	cmd := &Command{
 		Execute: "device_add",
-		Args:    args,
+		Args:    deviceAddArgs(dev, params),
 	}
 
 	cb := func(res *Response) {
@@ -560,6 +545,14 @@ func (m *QmpMonitor) DeviceAdd(dev string, params map[string]string, callback St
 	m.Query(cmd, cb)
 }
 
+func deviceAddArgs(dev string, params map[string]interface{}) map[string]interface{} {
+	args := map[string]interface{}{"driver": dev}
+	for k, v := range params {
+		args[k] = v
+	}
+	return args
+}
+
 func (m *QmpMonitor) DeviceAddCpu(dev string, params map[string]interface{}, callback StringCallback) {
 	args := map[string]interface{}{
 		"driver": dev,

pkg/hostman/monitor/qmp_test.go

@@ -15,12 +15,33 @@
 package monitor
 
 import (
+	"encoding/json"
+	"strings"
 	"testing"
 	"time"
 
 	"yunion.io/x/log"
 )
 
+func TestDeviceAddArgsPreservesQmpTypes(t *testing.T) {
+	cmd := Command{Execute: "device_add", Args: deviceAddArgs("usb-host", map[string]interface{}{
+		"hostbus":  uint64(1),
+		"hostaddr": uint64(9),
+		"hostport": "1.2",
+	})}
+
+	raw, err := json.Marshal(cmd)
+	if err != nil {
+		t.Fatal(err)
+	}
+	got := string(raw)
+	for _, want := range []string{`"hostbus":1`, `"hostaddr":9`, `"hostport":"1.2"`} {
+		if !strings.Contains(got, want) {
+			t.Errorf("QMP command %s does not contain %s", got, want)
+		}
+	}
+}
+
 func TestQmpMonitor_Connect(t *testing.T) {
 	onConnected := func() { log.Infof("Monitor Connected") }
 	onDisConnect := func(error) { log.Infof("Monitor DisConnect") }