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

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(a)gmail.com&gt; >> --- >>  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(a)redhat.com >> https://www.redhat.com/mailman/listinfo/libvir-list > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list