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

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(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. > If the 'make check' doesn't regress then it's fine for known ARM needs.