1:16 a.m.
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(a)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
5:41 p.m.
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
1:05 a.m.
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
4571
days inactive
4572
days old
2 comments
2 participants
participants (2)
-
Eric Blake -
Li Zhang