[libvirt] [PATCH 1/1] Assign spapr-vio address value to VIO devices without hardcode
Hardcode address will cause conflicts when there are a lot of VIO devices. This patch is to remove the harcode of the address, and assign a variable to it, which is cnt * 0x1000UL. And assign spapr-vio address to VIO devices, such as spapr-vlan and spapr-vty. Several test cases are modified. Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- src/qemu/qemu_command.c | 38 +++++++++++++++----- .../qemuxml2argv-disk-scsi-vscsi.args | 2 +- .../qemuxml2argv-pseries-basic.args | 2 +- .../qemuxml2argv-pseries-vio-user-assigned.args | 4 +-- .../qemuxml2argvdata/qemuxml2argv-pseries-vio.args | 8 ++--- 5 files changed, 37 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3a92c70..fbff73e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -780,14 +780,25 @@ int qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def) { int i, rc; int model; + int cnt = 0; /* Default values match QEMU. See spapr_(llan|vscsi|vty).c */ for (i = 0 ; i < def->nnets; i++) { - rc = qemuAssignSpaprVIOAddress(def, &def->nets[i]->info, - 0x1000ul); - if (rc) - return rc; + if (!def->nets[i]->model && + STREQ(def->os.arch, "ppc64") && + STREQ(def->os.machine, "pseries")) + strcpy(def->nets[i]->model, "spapr-vlan"); + if (def->nets[i]->model && + STREQ(def->nets[i]->model, "spapr-vlan")) { + cnt ++; + def->nets[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO; + rc = qemuAssignSpaprVIOAddress(def, &def->nets[i]->info, + cnt * 0x1000ul); + if (rc) + return rc; + } + } for (i = 0 ; i < def->ncontrollers; i++) { @@ -797,19 +808,28 @@ int qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def) model = qemuDefaultScsiControllerModel(def); if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI && def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { + cnt ++; def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO; rc = qemuAssignSpaprVIOAddress(def, &def->controllers[i]->info, - 0x2000ul); + cnt * 0x1000ul); if (rc) return rc; } } for (i = 0 ; i < def->nserials; i++) { - rc = qemuAssignSpaprVIOAddress(def, &def->serials[i]->info, - 0x30000000ul); - if (rc) - return rc; + if (STREQ(def->os.arch, "ppc64") && + STREQ(def->os.machine, "pseries")) + def->serials[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO; + + if (def->serials[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) { + cnt ++; + rc = qemuAssignSpaprVIOAddress(def, &def->serials[i]->info, + cnt * 0x1000ul); + if (rc) + return rc; + } + } /* No other devices are currently supported on spapr-vio */ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-vscsi.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-vscsi.args index d57159f..1feab49 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-vscsi.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-vscsi.args @@ -1,7 +1,7 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults -monitor \ unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -device spapr-vscsi,id=scsi0,\ -reg=0x2000 -drive file=/dev/HostVG/QEMUGuest1,if=none,\ +reg=0x1000 -drive file=/dev/HostVG/QEMUGuest1,if=none,\ id=drive-ide0-0-0 -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ -drive file=/tmp/scsidisk.img,if=none,id=drive-scsi0-0-3-0 \ -device scsi-disk,bus=scsi0.0,channel=0,scsi-id=3,lun=0,drive=drive-scsi0-0-3-0,id=scsi0-0-3-0 \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-basic.args b/tests/qemuxml2argvdata/qemuxml2argv-pseries-basic.args index f9aec92..927e014 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pseries-basic.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-basic.args @@ -1 +1 @@ -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu-system-ppc64 -S -M pseries -m 512 -smp 1 -nographic -nodefconfig -nodefaults -chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait -mon chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c -chardev pty,id=charserial0 -device spapr-vty,chardev=charserial0,reg=0x30000000 -usb +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu-system-ppc64 -S -M pseries -m 512 -smp 1 -nographic -nodefconfig -nodefaults -chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait -mon chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c -chardev pty,id=charserial0 -device spapr-vty,chardev=charserial0,reg=0x1000 -usb diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-vio-user-assigned.args b/tests/qemuxml2argvdata/qemuxml2argv-pseries-vio-user-assigned.args index fad4346..8072864 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pseries-vio-user-assigned.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-vio-user-assigned.args @@ -2,11 +2,11 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu-system-p -S -M pseries -m 512 -smp 1 -nographic -nodefconfig -nodefaults \ -chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait \ -mon chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c \ --device spapr-vscsi,id=scsi0,reg=0x2000 \ +-device spapr-vscsi,id=scsi0,reg=0x1000 \ -device spapr-vscsi,id=scsi1,reg=0x30000000 \ -drive file=/tmp/scsidisk.img,if=none,id=drive-scsi1-0-0-0 \ -device scsi-disk,bus=scsi1.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi1-0-0-0,id=scsi1-0-0-0 \ -chardev pty,id=charserial0 \ -device spapr-vty,chardev=charserial0,reg=0x20000000 \ -chardev pty,id=charserial1 \ --device spapr-vty,chardev=charserial1,reg=0x30001000 -usb +-device spapr-vty,chardev=charserial1,reg=0x4000 -usb diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-vio.args b/tests/qemuxml2argvdata/qemuxml2argv-pseries-vio.args index a75b428..458a1fe 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pseries-vio.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-vio.args @@ -2,11 +2,11 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu-system-p -S -M pseries -m 512 -smp 1 -nographic -nodefconfig -nodefaults \ -chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait \ -mon chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c \ --device spapr-vscsi,id=scsi0,reg=0x2000 \ --device spapr-vscsi,id=scsi1,reg=0x3000 \ +-device spapr-vscsi,id=scsi0,reg=0x1000 \ +-device spapr-vscsi,id=scsi1,reg=0x2000 \ -drive file=/tmp/scsidisk.img,if=none,id=drive-scsi1-0-0-0 \ -device scsi-disk,bus=scsi1.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi1-0-0-0,id=scsi1-0-0-0 \ -chardev pty,id=charserial0 \ --device spapr-vty,chardev=charserial0,reg=0x30000000 \ +-device spapr-vty,chardev=charserial0,reg=0x3000 \ -chardev pty,id=charserial1 \ --device spapr-vty,chardev=charserial1,reg=0x30001000 -usb +-device spapr-vty,chardev=charserial1,reg=0x4000 -usb -- 1.7.9.5
On 05/17/2012 12:16 AM, Li Zhang wrote:
Hardcode address will cause conflicts when there are a lot of VIO devices.
This patch is to remove the harcode of the address, and assign a variable to it, which is cnt * 0x1000UL. And assign spapr-vio address to VIO devices, such as spapr-vlan and spapr-vty. Several test cases are modified.
/* Default values match QEMU. See spapr_(llan|vscsi|vty).c */
for (i = 0 ; i < def->nnets; i++) { - rc = qemuAssignSpaprVIOAddress(def, &def->nets[i]->info, - 0x1000ul);
Isn't the point of qemuAssignSpaprVIOAddress to start at 0x1000 and increment until it finds a gap? If so, then the caller shouldn't also be incrementing.
- if (rc) - return rc; + if (!def->nets[i]->model &&
This says def->nets[i]->model is NULL...
+ STREQ(def->os.arch, "ppc64") && + STREQ(def->os.machine, "pseries")) + strcpy(def->nets[i]->model, "spapr-vlan");
and this tells strcpy() to dereference it. You can't have possibly tested this line of code, since it will crash. Did you mean to do a strdup() instead?
+ if (def->nets[i]->model && + STREQ(def->nets[i]->model, "spapr-vlan")) { + cnt ++;
Style nit - no spacing between ++ or -- operator and the variable it modifies.
+ def->nets[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO; + rc = qemuAssignSpaprVIOAddress(def, &def->nets[i]->info, + cnt * 0x1000ul);
Indentation is off.
+ if (rc) + return rc; + } + }
for (i = 0 ; i < def->ncontrollers; i++) { @@ -797,19 +808,28 @@ int qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def) model = qemuDefaultScsiControllerModel(def); if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI && def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { + cnt ++; def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO; rc = qemuAssignSpaprVIOAddress(def, &def->controllers[i]->info, - 0x2000ul); + cnt * 0x1000ul);
About the only difference I can see is that if you skipped the first loop entirely, this would let the second loop start at 0x1000 instead of 0x2000. But again, since qemuAssignSpaprVIOAddress is supposed to auto-increment until it finds a gap, wouldn't that mean you can just change this to start at 0x1000 without needing 'cnt'?
if (rc) return rc; } }
for (i = 0 ; i < def->nserials; i++) { - rc = qemuAssignSpaprVIOAddress(def, &def->serials[i]->info, - 0x30000000ul); - if (rc) - return rc; + if (STREQ(def->os.arch, "ppc64") && + STREQ(def->os.machine, "pseries")) + def->serials[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO; + + if (def->serials[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) { + cnt ++; + rc = qemuAssignSpaprVIOAddress(def, &def->serials[i]->info, + cnt * 0x1000ul);
This changes the default value from 0x30000000 to the first gap after 0x1000. Can you please justify this change by pointing to the qemu commit that shows how qemu assigns addresses, to make sure libvirt is picking the same defaults as qemu? -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/18/2012 06:41 AM, Eric Blake wrote:
On 05/17/2012 12:16 AM, Li Zhang wrote:
Hardcode address will cause conflicts when there are a lot of VIO devices.
This patch is to remove the harcode of the address, and assign a variable to it, which is cnt * 0x1000UL. And assign spapr-vio address to VIO devices, such as spapr-vlan and spapr-vty. Several test cases are modified.
/* Default values match QEMU. See spapr_(llan|vscsi|vty).c */
for (i = 0 ; i< def->nnets; i++) { - rc = qemuAssignSpaprVIOAddress(def,&def->nets[i]->info, - 0x1000ul);
Isn't the point of qemuAssignSpaprVIOAddress to start at 0x1000 and increment until it finds a gap? If so, then the caller shouldn't also be incrementing. Yes, I didn't realize that. Sorry. My way is to resolve the same problem with that. Now it is not necessary to do this. :)
- if (rc) - return rc; + if (!def->nets[i]->model&&
This says def->nets[i]->model is NULL...
+ STREQ(def->os.arch, "ppc64")&& + STREQ(def->os.machine, "pseries")) + strcpy(def->nets[i]->model, "spapr-vlan");
and this tells strcpy() to dereference it. You can't have possibly tested this line of code, since it will crash. Did you mean to do a strdup() instead? Sorry for my stupid mistake. It needs to malloc memory to the string. or use strdup() to do that.
+ if (def->nets[i]->model&& + STREQ(def->nets[i]->model, "spapr-vlan")) { + cnt ++;
Style nit - no spacing between ++ or -- operator and the variable it modifies.
Got it. Thanks for your comments.
+ def->nets[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO; + rc = qemuAssignSpaprVIOAddress(def,&def->nets[i]->info, + cnt * 0x1000ul);
Indentation is off.
+ if (rc) + return rc; + } + }
for (i = 0 ; i< def->ncontrollers; i++) { @@ -797,19 +808,28 @@ int qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def) model = qemuDefaultScsiControllerModel(def); if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI&& def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { + cnt ++; def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO; rc = qemuAssignSpaprVIOAddress(def,&def->controllers[i]->info, - 0x2000ul); + cnt * 0x1000ul);
About the only difference I can see is that if you skipped the first loop entirely, this would let the second loop start at 0x1000 instead of 0x2000. But again, since qemuAssignSpaprVIOAddress is supposed to auto-increment until it finds a gap, wouldn't that mean you can just change this to start at 0x1000 without needing 'cnt'?
You are right.
if (rc) return rc; } }
for (i = 0 ; i< def->nserials; i++) { - rc = qemuAssignSpaprVIOAddress(def,&def->serials[i]->info, - 0x30000000ul); - if (rc) - return rc; + if (STREQ(def->os.arch, "ppc64")&& + STREQ(def->os.machine, "pseries")) + def->serials[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO; + + if (def->serials[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) { + cnt ++; + rc = qemuAssignSpaprVIOAddress(def,&def->serials[i]->info, + cnt * 0x1000ul);
This changes the default value from 0x30000000 to the first gap after 0x1000. Can you please justify this change by pointing to the qemu commit that shows how qemu assigns addresses, to make sure libvirt is picking the same defaults as qemu?
I will check this in Qemu. Thanks. :) -- Best Regards Li IBM LTC, China System&Technology Lab, Beijing
participants (2)
-
Eric Blake -
Li Zhang