Re: [libvirt] [PATCH 1/2] Assign PCI address to primary video card in pseries guests

On 11/06/2013 06:13 PM, Vitor de Lima wrote: > From: Vitor de Lima <vitor.lima(a)eldorado.org.br&gt; 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.