[libvirt] [PATCH 1/2] Fix default USB controller for ppc64
From: Dipankar Sarma <dipankar@in.ibm.com> Fix the default usb controller for pseries systems if none specified. Signed-off-by: Dipankar Sarma <dipankar@in.ibm.com> --- src/qemu/qemu_command.c | 13 +++++++++---- 1 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a34c707..bd4f96a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2605,7 +2605,8 @@ qemuControllerModelUSBToCaps(int model) static int -qemuBuildUSBControllerDevStr(virDomainControllerDefPtr def, +qemuBuildUSBControllerDevStr(virDomainDefPtr domainDef, + virDomainControllerDefPtr def, virBitmapPtr qemuCaps, virBuffer *buf) { @@ -2614,8 +2615,12 @@ qemuBuildUSBControllerDevStr(virDomainControllerDefPtr def, model = def->model; - if (model == -1) - model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI; + if (model == -1) { + if (STREQ(domainDef->os.arch, "ppc64")) + model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI; + else + model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI; + } smodel = qemuControllerModelUSBTypeToString(model); caps = qemuControllerModelUSBToCaps(model); @@ -2701,7 +2706,7 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, break; case VIR_DOMAIN_CONTROLLER_TYPE_USB: - if (qemuBuildUSBControllerDevStr(def, qemuCaps, &buf) == -1) + if (qemuBuildUSBControllerDevStr(domainDef, def, qemuCaps, &buf) == -1) goto error; if (nusbcontroller)
From: Dipankar Sarma <dipankar@in.ibm.com> Fix emumeration of scsi disks to use scsi id instead of lun. Without this, pseries partition firmware (OF) breaks. Also, as per PAPR, one controler per device for ibmvscsi. Signed-off-by: Dipankar Sarma <dipankar@in.ibm.com> --- src/conf/domain_conf.c | 40 +++++++++++++++++++++++++++++++++------- src/conf/domain_conf.h | 2 +- src/qemu/qemu_command.c | 9 +++++---- src/vmx/vmx.c | 28 ++++++++++++++++------------ src/vmx/vmx.h | 6 +++--- 5 files changed, 58 insertions(+), 27 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5ea264f..7ee0f52 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2994,8 +2994,26 @@ virDomainDiskFindControllerModel(virDomainDefPtr def, return model; } +static int +virDomainFindControllerModel(virDomainDefPtr def, int idx) +{ + int model = -1; + int i; + + for (i = 0; i < def->ncontrollers; i++) { + if (def->controllers[i]->idx == idx) { + model = def->controllers[i]->model; + break; + } + } + + return model; +} + int -virDomainDiskDefAssignAddress(virCapsPtr caps, virDomainDiskDefPtr def) +virDomainDiskDefAssignAddress(virDomainDefPtr ddef, + virCapsPtr caps, + virDomainDiskDefPtr def) { int idx = virDiskNameToIndex(def->dst); if (idx < 0) @@ -3004,8 +3022,15 @@ virDomainDiskDefAssignAddress(virCapsPtr caps, virDomainDiskDefPtr def) switch (def->bus) { case VIR_DOMAIN_DISK_BUS_SCSI: def->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; - - if (caps->hasWideScsiBus) { + if (STREQ(ddef->os.arch, "ppc64") && + virDomainFindControllerModel(ddef, idx) == + VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI) { + /* ppc64 PAPR we put one drive per (virtual) controller */ + def->info.addr.drive.controller = idx; + def->info.addr.drive.bus = 0; + def->info.addr.drive.target = 0; + def->info.addr.drive.unit = 0; + } else if (caps->hasWideScsiBus) { /* For a wide SCSI bus we define the default mapping to be * 16 units per bus, 1 bus per controller, many controllers. * Unit 7 is the SCSI controller itself. Therefore unit 7 @@ -3321,7 +3346,8 @@ cleanup: * @param node XML nodeset to parse for disk definition */ static virDomainDiskDefPtr -virDomainDiskDefParseXML(virCapsPtr caps, +virDomainDiskDefParseXML(virDomainDefPtr ddef, + virCapsPtr caps, xmlNodePtr node, xmlXPathContextPtr ctxt, virBitmapPtr bootMap, @@ -3945,7 +3971,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, goto no_memory; if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE - && virDomainDiskDefAssignAddress(caps, def) < 0) + && virDomainDiskDefAssignAddress(ddef, caps, def) < 0) goto error; cleanup: @@ -7083,7 +7109,7 @@ virDomainDeviceDefPtr virDomainDeviceDefParse(virCapsPtr caps, if (xmlStrEqual(node->name, BAD_CAST "disk")) { dev->type = VIR_DOMAIN_DEVICE_DISK; - if (!(dev->data.disk = virDomainDiskDefParseXML(caps, node, ctxt, + if (!(dev->data.disk = virDomainDiskDefParseXML(def, caps, node, ctxt, NULL, &def->seclabel, flags))) goto error; } else if (xmlStrEqual(node->name, BAD_CAST "lease")) { @@ -8578,7 +8604,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, if (n && VIR_ALLOC_N(def->disks, n) < 0) goto no_memory; for (i = 0 ; i < n ; i++) { - virDomainDiskDefPtr disk = virDomainDiskDefParseXML(caps, + virDomainDiskDefPtr disk = virDomainDiskDefParseXML(def, caps, nodes[i], ctxt, bootMap, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 86c1e63..2b20664 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1982,7 +1982,7 @@ int virDomainDiskInsert(virDomainDefPtr def, virDomainDiskDefPtr disk); void virDomainDiskInsertPreAlloced(virDomainDefPtr def, virDomainDiskDefPtr disk); -int virDomainDiskDefAssignAddress(virCapsPtr caps, virDomainDiskDefPtr def); +int virDomainDiskDefAssignAddress(virDomainDefPtr ddef, virCapsPtr caps, virDomainDiskDefPtr def); virDomainDiskDefPtr virDomainDiskRemove(virDomainDefPtr def, size_t i); diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index bd4f96a..c4bff16 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6467,7 +6467,8 @@ error: * Will fail if not using the 'index' keyword */ static virDomainDiskDefPtr -qemuParseCommandLineDisk(virCapsPtr caps, +qemuParseCommandLineDisk(virDomainDefPtr ddef, + virCapsPtr caps, const char *val, int nvirtiodisk, bool old_style_ceph_args) @@ -6754,7 +6755,7 @@ qemuParseCommandLineDisk(virCapsPtr caps, else def->dst[2] = 'a' + idx; - if (virDomainDiskDefAssignAddress(caps, def) < 0) { + if (virDomainDiskDefAssignAddress(ddef, caps, def) < 0) { qemuReportError(VIR_ERR_INTERNAL_ERROR, _("invalid device name '%s'"), def->dst); virDomainDiskDefFree(def); @@ -7769,7 +7770,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, !disk->dst) goto no_memory; - if (virDomainDiskDefAssignAddress(caps, disk) < 0) + if (virDomainDiskDefAssignAddress(def, caps, disk) < 0) goto error; if (VIR_REALLOC_N(def->disks, def->ndisks+1) < 0) @@ -7940,7 +7941,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, } } else if (STREQ(arg, "-drive")) { WANT_VALUE(); - if (!(disk = qemuParseCommandLineDisk(caps, val, nvirtiodisk, + if (!(disk = qemuParseCommandLineDisk(def, caps, val, nvirtiodisk, ceph_args != NULL))) goto error; if (VIR_REALLOC_N(def->disks, def->ndisks+1) < 0) diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 3de7062..c8f26d0 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -915,7 +915,7 @@ virVMXFloppyDiskNameToUnit(const char *name, int *unit) static int -virVMXVerifyDiskAddress(virCapsPtr caps, virDomainDiskDefPtr disk) +virVMXVerifyDiskAddress(virDomainDefPtr ddef, virCapsPtr caps, virDomainDiskDefPtr disk) { virDomainDiskDef def; virDomainDeviceDriveAddressPtr drive; @@ -934,7 +934,7 @@ virVMXVerifyDiskAddress(virCapsPtr caps, virDomainDiskDefPtr disk) def.dst = disk->dst; def.bus = disk->bus; - if (virDomainDiskDefAssignAddress(caps, &def) < 0) { + if (virDomainDiskDefAssignAddress(ddef, caps, &def) < 0) { VMX_ERROR(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not verify disk address")); return -1; @@ -1569,7 +1569,8 @@ virVMXParseConfig(virVMXContext *ctx, virCapsPtr caps, const char *vmx) continue; } - if (virVMXParseDisk(ctx, caps, conf, VIR_DOMAIN_DISK_DEVICE_DISK, + if (virVMXParseDisk(def, ctx, caps, conf, + VIR_DOMAIN_DISK_DEVICE_DISK, VIR_DOMAIN_DISK_BUS_SCSI, controller, unit, &def->disks[def->ndisks]) < 0) { goto cleanup; @@ -1580,7 +1581,8 @@ virVMXParseConfig(virVMXContext *ctx, virCapsPtr caps, const char *vmx) continue; } - if (virVMXParseDisk(ctx, caps, conf, VIR_DOMAIN_DISK_DEVICE_CDROM, + if (virVMXParseDisk(def, ctx, caps, conf, + VIR_DOMAIN_DISK_DEVICE_CDROM, VIR_DOMAIN_DISK_BUS_SCSI, controller, unit, &def->disks[def->ndisks]) < 0) { goto cleanup; @@ -1595,7 +1597,8 @@ virVMXParseConfig(virVMXContext *ctx, virCapsPtr caps, const char *vmx) /* def:disks (ide) */ for (bus = 0; bus < 2; ++bus) { for (unit = 0; unit < 2; ++unit) { - if (virVMXParseDisk(ctx, caps, conf, VIR_DOMAIN_DISK_DEVICE_DISK, + if (virVMXParseDisk(def, ctx, caps, conf, + VIR_DOMAIN_DISK_DEVICE_DISK, VIR_DOMAIN_DISK_BUS_IDE, bus, unit, &def->disks[def->ndisks]) < 0) { goto cleanup; @@ -1606,7 +1609,8 @@ virVMXParseConfig(virVMXContext *ctx, virCapsPtr caps, const char *vmx) continue; } - if (virVMXParseDisk(ctx, caps, conf, VIR_DOMAIN_DISK_DEVICE_CDROM, + if (virVMXParseDisk(def, ctx, caps, conf, + VIR_DOMAIN_DISK_DEVICE_CDROM, VIR_DOMAIN_DISK_BUS_IDE, bus, unit, &def->disks[def->ndisks]) < 0) { goto cleanup; @@ -1620,7 +1624,7 @@ virVMXParseConfig(virVMXContext *ctx, virCapsPtr caps, const char *vmx) /* def:disks (floppy) */ for (unit = 0; unit < 2; ++unit) { - if (virVMXParseDisk(ctx, caps, conf, VIR_DOMAIN_DISK_DEVICE_FLOPPY, + if (virVMXParseDisk(def, ctx, caps, conf, VIR_DOMAIN_DISK_DEVICE_FLOPPY, VIR_DOMAIN_DISK_BUS_FDC, 0, unit, &def->disks[def->ndisks]) < 0) { goto cleanup; @@ -1899,9 +1903,9 @@ virVMXParseSCSIController(virConfPtr conf, int controller, bool *present, int -virVMXParseDisk(virVMXContext *ctx, virCapsPtr caps, virConfPtr conf, - int device, int busType, int controllerOrBus, int unit, - virDomainDiskDefPtr *def) +virVMXParseDisk(virDomainDefPtr ddef, virVMXContext *ctx, virCapsPtr caps, + virConfPtr conf, int device, int busType, int controllerOrBus, + int unit, virDomainDiskDefPtr *def) { /* * device = {VIR_DOMAIN_DISK_DEVICE_DISK, VIR_DOMAIN_DISK_DEVICE_CDROM} @@ -2233,7 +2237,7 @@ virVMXParseDisk(virVMXContext *ctx, virCapsPtr caps, virConfPtr conf, goto cleanup; } - if (virDomainDiskDefAssignAddress(caps, *def) < 0) { + if (virDomainDiskDefAssignAddress(ddef, caps, *def) < 0) { VMX_ERROR(VIR_ERR_INTERNAL_ERROR, _("Could not assign address to disk '%s'"), (*def)->src); goto cleanup; @@ -3079,7 +3083,7 @@ virVMXFormatConfig(virVMXContext *ctx, virCapsPtr caps, virDomainDefPtr def, /* def:disks */ for (i = 0; i < def->ndisks; ++i) { - if (virVMXVerifyDiskAddress(caps, def->disks[i]) < 0 || + if (virVMXVerifyDiskAddress(def, caps, def->disks[i]) < 0 || virVMXHandleLegacySCSIDiskDriverName(def, def->disks[i]) < 0) { goto cleanup; } diff --git a/src/vmx/vmx.h b/src/vmx/vmx.h index 4d54660..79788c1 100644 --- a/src/vmx/vmx.h +++ b/src/vmx/vmx.h @@ -86,9 +86,9 @@ int virVMXParseVNC(virConfPtr conf, virDomainGraphicsDefPtr *def); int virVMXParseSCSIController(virConfPtr conf, int controller, bool *present, int *virtualDev); -int virVMXParseDisk(virVMXContext *ctx, virCapsPtr caps, virConfPtr conf, - int device, int busType, int controllerOrBus, int unit, - virDomainDiskDefPtr *def); +int virVMXParseDisk(virDomainDefPtr ddef, virVMXContext *ctx, virCapsPtr caps, + virConfPtr conf, int device, int busType, + int controllerOrBus, int unit, virDomainDiskDefPtr *def); int virVMXParseEthernet(virConfPtr conf, int controller, virDomainNetDefPtr *def);
On 06/19/2012 04:26 AM, Dipankar Sarma wrote:
From: Dipankar Sarma <dipankar@in.ibm.com>
Fix emumeration of scsi disks to use scsi id instead of lun. Without
s/emumeration/enumeration/
this, pseries partition firmware (OF) breaks. Also, as per PAPR, one controler per device for ibmvscsi.
s/controler/controller/
Signed-off-by: Dipankar Sarma <dipankar@in.ibm.com> ---
src/conf/domain_conf.c | 40 +++++++++++++++++++++++++++++++++------- src/conf/domain_conf.h | 2 +- src/qemu/qemu_command.c | 9 +++++---- src/vmx/vmx.c | 28 ++++++++++++++++------------ src/vmx/vmx.h | 6 +++--- 5 files changed, 58 insertions(+), 27 deletions(-)
This change is a bit bigger than the last; is there any way you can add to tests/qemuxml2argvtest.c (and by implication, to tests/qemuxml2argvdata) to cover the new code paths?
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
@@ -3004,8 +3022,15 @@ virDomainDiskDefAssignAddress(virCapsPtr caps, virDomainDiskDefPtr def) switch (def->bus) { case VIR_DOMAIN_DISK_BUS_SCSI: def->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; - - if (caps->hasWideScsiBus) { + if (STREQ(ddef->os.arch, "ppc64") &&
This doesn't feel right. We shouldn't be hard-coding strings into the generic domain_conf code, so much as letting specific hypervisor drivers be making decisions. Remember, running LXC on ppc64 does not necessarily have the same default bus handling as running a ppc64 guest via qemu on an x86 machine. We either need a new field in caps (similar to caps->hasWideScsiBus) or even a callback function, so that the hypervisor driver code can answer questions about what defaults to use as part of the existing caps mechanism. The fact that you even had to touch vmx and qemu code in the same patch for a feature that you are really only trying to add to qemu support is further evidence that this is not quite right. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Jun 19, 2012 at 03:55:47PM -0600, Eric Blake wrote:
On 06/19/2012 04:26 AM, Dipankar Sarma wrote:
src/conf/domain_conf.c | 40 +++++++++++++++++++++++++++++++++------- src/conf/domain_conf.h | 2 +- src/qemu/qemu_command.c | 9 +++++---- src/vmx/vmx.c | 28 ++++++++++++++++------------ src/vmx/vmx.h | 6 +++--- 5 files changed, 58 insertions(+), 27 deletions(-)
This change is a bit bigger than the last; is there any way you can add to tests/qemuxml2argvtest.c (and by implication, to tests/qemuxml2argvdata) to cover the new code paths?
Will do.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
@@ -3004,8 +3022,15 @@ virDomainDiskDefAssignAddress(virCapsPtr caps, virDomainDiskDefPtr def) switch (def->bus) { case VIR_DOMAIN_DISK_BUS_SCSI: def->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; - - if (caps->hasWideScsiBus) { + if (STREQ(ddef->os.arch, "ppc64") &&
This doesn't feel right. We shouldn't be hard-coding strings into the generic domain_conf code, so much as letting specific hypervisor drivers be making decisions. Remember, running LXC on ppc64 does not necessarily have the same default bus handling as running a ppc64 guest via qemu on an x86 machine. We either need a new field in caps (similar to caps->hasWideScsiBus) or even a callback function, so that the hypervisor driver code can answer questions about what defaults to use as part of the existing caps mechanism. The fact that you even had to touch vmx and qemu code in the same patch for a feature that you are really only trying to add to qemu support is further evidence that this is not quite right.
Thanks for the hint. That seems cleaner. It should be useful for some other changes for ppc64 that I am working on. I will rework this patch. Thanks Dipankar
On 06/19/2012 04:21 AM, Dipankar Sarma wrote:
From: Dipankar Sarma <dipankar@in.ibm.com>
Fix the default usb controller for pseries systems if none specified.
Signed-off-by: Dipankar Sarma <dipankar@in.ibm.com> ---
src/qemu/qemu_command.c | 13 +++++++++---- 1 files changed, 9 insertions(+), 4 deletions(-)
Pretty straight-forward. ACK and pushed, after adding you to AUTHORS. Let me know if you prefer an alternate spelling. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Dipankar Sarma -
Eric Blake