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@xxxxxxxxxxxxxxx>
>
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list