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

[Laine Stump]

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 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.
>>
> 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.