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

[Daniel Henrique Barboza]

On 4/2/19 4:46 AM, Ján Tomko wrote:
> 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.


Thanks for this tip. I've used it in both patches to fix this leak inside
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
>