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

[Laine Stump]

On 11/06/2013 06:13 PM, Vitor de Lima wrote:
> From: Vitor de Lima <vitor.lima at 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.