[libvirt] [PATCH 1/2] Assign PCI address to primary video card in pseries guests
From: Vitor de Lima <vitor.lima@eldorado.org.br> This patch assings a PCI address to the primary video card if it does not have any kind of address. This fixes issues with pseries guests. --- src/qemu/qemu_command.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e48c9c2..159d920 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2959,6 +2959,15 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, goto error; } + /* Assign a PCI slot to the primary video card if there is not an + * assigned address. */ + if (def->nvideos >=1 && + def->videos[0]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + if (qemuDomainPCIAddressReserveNextSlot(addrs, &def->videos[0]->info, + flags) < 0) + goto error; + } + /* Further non-primary video cards which have to be qxl type */ for (i = 1; i < def->nvideos; i++) { if (def->videos[i]->type != VIR_DOMAIN_VIDEO_TYPE_QXL) { -- 1.8.1.4
From: Vitor de Lima <vitor.lima@eldorado.org.br> This patch moves some code in the qemuDomainAttachSCSIDisk function. The check for the existence of a PCI address assigned to the SCSI controller was moved in order to be executed only when needed. The PCI address of a controller is not necessary if QEMU_CAPS_DEVICE is not supported. This fixes issues with the hotplug of SCSI disks on pseries guests. --- src/qemu/qemu_hotplug.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e9424d9..6eb483c 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -518,12 +518,6 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, and hence the above loop must iterate at least once. */ sa_assert(cont); - if (cont->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("SCSI controller %d was missing its PCI address"), cont->idx); - goto error; - } - if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0) goto error; @@ -540,6 +534,13 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, } } } else { + if (cont->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("SCSI controller %d was missing its PCI address"), + cont->idx); + goto error; + } + virDomainDeviceDriveAddress driveAddr; ret = qemuMonitorAttachDrive(priv->mon, drivestr, -- 1.8.1.4
Il 06/11/2013 17:13, Vitor de Lima ha scritto:
From: Vitor de Lima <vitor.lima@eldorado.org.br>
This patch moves some code in the qemuDomainAttachSCSIDisk function. The check for the existence of a PCI address assigned to the SCSI controller was moved in order to be executed only when needed. The PCI address of a controller is not necessary if QEMU_CAPS_DEVICE is not supported.
... if QEMU_CAPS_DEVICE *is* supported. Looks good apart from this typo. Paolo
This fixes issues with the hotplug of SCSI disks on pseries guests.
On 11/06/2013 08:01 PM, Paolo Bonzini wrote:
Il 06/11/2013 17:13, Vitor de Lima ha scritto:
From: Vitor de Lima <vitor.lima@eldorado.org.br>
This patch moves some code in the qemuDomainAttachSCSIDisk function. The check for the existence of a PCI address assigned to the SCSI controller was moved in order to be executed only when needed. The PCI address of a controller is not necessary if QEMU_CAPS_DEVICE is not supported. ... if QEMU_CAPS_DEVICE *is* supported.
Looks good apart from this typo.
Okay, I've fixed the typo in the log message and pushed this.
On 11/06/2013 06:13 PM, Vitor de Lima wrote:
From: Vitor de Lima <vitor.lima@eldorado.org.br>
I would remove reference to "pseries" from the subject, as it's assigning a pci address on any arch that hasn't already assigned one (and the only ones that have are x86 Q35 and i440fx/PIIX3
This patch assings
s/assings/assigns/
a PCI address to the primary video card if it does not have any kind of address. This fixes issues with pseries guests.
It's probably worth adding in a statement that this was broken when adding support for Q35, which moved the assignment of a PCI address for the primary video card out to arch-specific functions for Q35 and i440fx, but forgot to do anything for all the other arches. I'm now nervous about assigning PCI addresses to devices due to the lack of PCI bus on many arches. Cole - does this particular change cause a problem for ARM? (I don't see anywhere that the address type is set to != NONE on video devices other than where it's set to PCI specifically for Q35 or i440fx, so if ARM arches support video, this patch will try to allocate a PCI slot, which will fail).
--- src/qemu/qemu_command.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e48c9c2..159d920 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2959,6 +2959,15 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, goto error; }
+ /* Assign a PCI slot to the primary video card if there is not an + * assigned address. */ + if (def->nvideos >=1 &&
You missed a space, and other comparisons of nvideos use "> 0" instead of ">= 1".
+ def->videos[0]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + if (qemuDomainPCIAddressReserveNextSlot(addrs, &def->videos[0]->info, + flags) < 0) + goto error; + } + /* Further non-primary video cards which have to be qxl type */ for (i = 1; i < def->nvideos; i++) { if (def->videos[i]->type != VIR_DOMAIN_VIDEO_TYPE_QXL) {
ACK to this patch if Cole says that it's okay for ARM. I'll make the small changes and push it if so.
On 11/07/2013 05:40 AM, Laine Stump wrote:
On 11/06/2013 06:13 PM, Vitor de Lima wrote:
From: Vitor de Lima <vitor.lima@eldorado.org.br>
I would remove reference to "pseries" from the subject, as it's assigning a pci address on any arch that hasn't already assigned one (and the only ones that have are x86 Q35 and i440fx/PIIX3
This patch assings
s/assings/assigns/
a PCI address to the primary video card if it does not have any kind of address. This fixes issues with pseries guests.
It's probably worth adding in a statement that this was broken when adding support for Q35, which moved the assignment of a PCI address for the primary video card out to arch-specific functions for Q35 and i440fx, but forgot to do anything for all the other arches.
I'm now nervous about assigning PCI addresses to devices due to the lack of PCI bus on many arches. Cole - does this particular change cause a problem for ARM? (I don't see anywhere that the address type is set to != NONE on video devices other than where it's set to PCI specifically for Q35 or i440fx, so if ARM arches support video, this patch will try to allocate a PCI slot, which will fail).
--- src/qemu/qemu_command.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e48c9c2..159d920 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2959,6 +2959,15 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, goto error; }
+ /* Assign a PCI slot to the primary video card if there is not an + * assigned address. */ + if (def->nvideos >=1 &&
You missed a space, and other comparisons of nvideos use "> 0" instead of ">= 1".
+ def->videos[0]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + if (qemuDomainPCIAddressReserveNextSlot(addrs, &def->videos[0]->info, + flags) < 0) + goto error; + } + /* Further non-primary video cards which have to be qxl type */ for (i = 1; i < def->nvideos; i++) { if (def->videos[i]->type != VIR_DOMAIN_VIDEO_TYPE_QXL) {
ACK to this patch if Cole says that it's okay for ARM. I'll make the small changes and push it if so.
If the 'make check' doesn't regress then it's fine for known ARM needs. - Cole
On 11/07/2013 03:13 PM, Cole Robinson wrote:
On 11/07/2013 05:40 AM, Laine Stump wrote:
On 11/06/2013 06:13 PM, Vitor de Lima wrote:
From: Vitor de Lima <vitor.lima@eldorado.org.br> I would remove reference to "pseries" from the subject, as it's assigning a pci address on any arch that hasn't already assigned one (and the only ones that have are x86 Q35 and i440fx/PIIX3 This patch assings s/assings/assigns/
a PCI address to the primary video card if it does not have any kind of address. This fixes issues with pseries guests. It's probably worth adding in a statement that this was broken when adding support for Q35, which moved the assignment of a PCI address for the primary video card out to arch-specific functions for Q35 and i440fx, but forgot to do anything for all the other arches.
I'm now nervous about assigning PCI addresses to devices due to the lack of PCI bus on many arches. Cole - does this particular change cause a problem for ARM? (I don't see anywhere that the address type is set to != NONE on video devices other than where it's set to PCI specifically for Q35 or i440fx, so if ARM arches support video, this patch will try to allocate a PCI slot, which will fail).
--- src/qemu/qemu_command.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e48c9c2..159d920 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2959,6 +2959,15 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, goto error; }
+ /* Assign a PCI slot to the primary video card if there is not an + * assigned address. */ + if (def->nvideos >=1 && You missed a space, and other comparisons of nvideos use "> 0" instead of ">= 1".
+ def->videos[0]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + if (qemuDomainPCIAddressReserveNextSlot(addrs, &def->videos[0]->info, + flags) < 0) + goto error; + } + /* Further non-primary video cards which have to be qxl type */ for (i = 1; i < def->nvideos; i++) { if (def->videos[i]->type != VIR_DOMAIN_VIDEO_TYPE_QXL) { ACK to this patch if Cole says that it's okay for ARM. I'll make the small changes and push it if so.
If the 'make check' doesn't regress then it's fine for known ARM needs.
Okay. It still passes make check, so I made the small changes and pushed it. I haven't done anything with the SCSI patch in this series yet though, as I don't know as much about that part of the code.
participants (4)
-
Cole Robinson -
Laine Stump -
Paolo Bonzini -
Vitor de Lima