[libvirt] [PATCH v4 1/2] qemu_domain: NVLink2 bridge detection function for PPC64

[Ján Tomko]

On Tue, Apr 02, 2019 at 09:21:53AM +0200, Peter Krempa wrote:
>On Tue, Mar 12, 2019 at 18:55:49 -0300, Daniel Henrique Barboza wrote:
>> The NVLink2 support in QEMU implements the detection of NVLink2
>> capable devices by verifying the attributes of the VFIO mem region
>> QEMU allocates for the NVIDIA GPUs. To properly allocate an
>> adequate amount of memLock, Libvirt needs this information before
>> a QEMU instance is even created, thus querying QEMU is not
>> possible and opening a VFIO window is too much.
>>
>> An alternative is presented in this patch. Making the following
>> assumptions:
>>
>> - if we want GPU RAM to be available in the guest, an NVLink2 bridge
>> must be passed through;
>>
>> - an unknown PCI device can be classified as a NVLink2 bridge
>> if its device tree node has 'ibm,gpu', 'ibm,nvlink',
>> 'ibm,nvlink-speed' and 'memory-region'.
>>
>> This patch introduces a helper called @ppc64VFIODeviceIsNV2Bridge
>> that checks the device tree node of a given PCI device and
>> check if it meets the criteria to be a NVLink2 bridge. This
>> new function will be used in a follow-up patch that, using the
>> first assumption, will set up the rlimits of the guest
>> accordingly.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413 at gmail.com>
>> ---
>>  src/qemu/qemu_domain.c | 29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>
>disclaimer: This is a code review. I don't have much knowledge about
>what you are trying to implement.
>
>
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 1659e88478..dcc92d253c 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -10398,6 +10398,35 @@ qemuDomainUpdateCurrentMemorySize(virDomainObjPtr vm)
>>  }
>>
>>
>> +/**
>> + * ppc64VFIODeviceIsNV2Bridge:
>> + * @device: string with the PCI device address
>> + *
>> + * This function receives a string that represents a PCI device,
>> + * such as '0004:04:00.0', and tells if the device is a NVLink2
>> + * bridge.
>> + */
>> +static bool
>> +ppc64VFIODeviceIsNV2Bridge(const char *device)
>> +{
>> +    const char *nvlink2Files[] = {"ibm,gpu", "ibm,nvlink",
>> +                                  "ibm,nvlink-speed", "memory-region"};
>> +    char *file;
>> +    size_t i;
>> +
>> +    for (i = 0; i < 4; i++) {
>
>Use ARRAY_CARDINALITY(nvlink2Files) instead of '4'.
>
>> +        if ((virAsprintf(&file, "/sys/bus/pci/devices/%s/of_node/%s",
>> +                         device, nvlink2Files[i])) < 0)
>> +            return false;
>> +
>> +        if (!virFileExists(file))
>> +            return false;
>
>memory allocated to 'file' is leaked in every loop and also at exit. You
>can use VIR_AUTOFREE(char *) file above, but you need to free the
>pointer also in every single loop.

Or just declare the file variable inside the loop, then it will be freed
after going out of scope every loop.

Jano

>
>> +    }
>> +
>> +    return true;
>> +}
>
>This function is unused and static, this means the build will fail after
>this patch.
>
>> +
>> +
>>  /**
>>   * getPPC64MemLockLimitBytes:
>>   * @def: domain definition
>> --
>> 2.20.1
>>
>> --
>> libvir-list mailing list
>> libvir-list at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list



>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190402/51184ff8/attachment-0001.sig>