Re: [libvirt] [PATCH v3 3/4] qemu_domain: NVLink2 device tree functions for PPC64

On 3/12/19 1:05 AM, Alexey Kardashevskiy wrote: > > On 12/03/2019 09:15, Daniel Henrique Barboza wrote: >> >> On 3/7/19 10:29 PM, Alexey Kardashevskiy wrote: >>> On 08/03/2019 04:51, Piotr Jaroszynski wrote: >>>> On 3/7/19 7:39 AM, Daniel Henrique Barboza wrote: >>>>> On 3/7/19 12:15 PM, Erik Skultety wrote: >>>>>> On Tue, Mar 05, 2019 at 09:46:08AM -0300, Daniel Henrique Barboza >>>>>> wrote: >>>>>>> The NVLink2 support in QEMU implements the detection of NVLink2 >>>>>>> capable devices by verfying 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. >>>>>>> >>>>>>> An alternative is presented in this patch. Given a PCI device, >>>>>>> we'll traverse the device tree at /proc/device-tree to check if >>>>>>> the device has a NPU bridge, retrieve the node of the NVLink2 bus, >>>>>>> find the memory-node that is related to the bus and see if it's a >>>>>>> NVLink2 bus by inspecting its 'reg' value. This logic is contained >>>>>>> inside the 'device_is_nvlink2_capable' function, which uses other >>>>>>> new helper functions to navigate and fetch values from the device >>>>>>> tree nodes. >>>>>>> >>>>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; >>>>>>> --- >>>>>> This fails with a bunch of compilation errors, please make sure you >>>>>> run make >>>>>> check and make syntax-check on each patch of the series. >>>>> Ooops, forgot to follow up make syntax-check with 'make' before >>>>> submitting ... my bad. >>>>> >>>>>>>     src/qemu/qemu_domain.c | 194 >>>>>>> +++++++++++++++++++++++++++++++++++++++++ >>>>>>>     1 file changed, 194 insertions(+) >>>>>>> >>>>>>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c >>>>>>> index 77548c224c..97de5793e2 100644 >>>>>>> --- a/src/qemu/qemu_domain.c >>>>>>> +++ b/src/qemu/qemu_domain.c >>>>>>> @@ -10343,6 +10343,200 @@ >>>>>>> qemuDomainUpdateCurrentMemorySize(virDomainObjPtr vm) >>>>>>>     } >>>>>>> >>>>>>> >>>>>>> +/** >>>>>>> + * Reads a phandle file and returns the phandle value. >>>>>>> + */ >>>>>>> +static int >>>>>>> +read_dt_phandle(const char* file) >>>>>> ..we prefer camelCase naming of functions...all functions should >>>>>> have a prefix, >>>>>> e.g. vir,<driver>, etc. >>>>>> >>>>>>> +{ >>>>>>> +    unsigned int buf[1]; >>>>>>> +    size_t read; >>>>>>> +    FILE *f; >>>>>>> + >>>>>>> +    f = fopen(file, "r"); >>>>>>> +    if (!f) >>>>>>> +        return -1; >>>>>>> + >>>>>>> +    read = fread(buf, sizeof(unsigned int), 1, f); >>>>>> We already have safe helpers that take care of such operations. >>>>>> >>>>>>> + >>>>>>> +    if (!read) { >>>>>>> +        VIR_CLOSE(f); >>>>>> You could use VIR_AUTOCLOSE for this >>>>>> >>>>>>> +        return 0; >>>>>>> +    } >>>>>>> + >>>>>>> +    VIR_CLOSE(f); >>>>>>> +    return be32toh(buf[0]); >>>>>> Is this part of gnulib? We need to make sure it's portable >>>>>> otherwise we'd need >>>>>> to make the conversion ourselves, just like for le64toh >>>>>> >>>>>>> +} >>>>>>> + >>>>>>> + >>>>>>> +/** >>>>>>> + * Reads a memory reg file and returns the first 4 int values. >>>>>>> + * >>>>>>> + * The caller is responsible for freeing the returned array. >>>>>>> + */ >>>>>>> +static unsigned int * >>>>>>> +read_dt_memory_reg(const char *file) >>>>>>> +{ >>>>>>> +    unsigned int *buf; >>>>>>> +    size_t read, i; >>>>>>> +    FILE *f; >>>>>>> + >>>>>>> +    f = fopen(file, "r"); >>>>>>> +    if (!f) >>>>>>> +        return NULL; >>>>>>> + >>>>>>> +    if (VIR_ALLOC_N(buf, 4) < 0) >>>>>>> +        return NULL; >>>>>>> + >>>>>>> +    read = fread(buf, sizeof(unsigned int), 4, f); >>>>>>> + >>>>>>> +    if (!read && read < 4) >>>>>>> +        /* shouldn't happen */ >>>>>>> +        VIR_FREE(buf); >>>>>>> +    else for (i = 0; i < 4; i++) >>>>>>> +        buf[i] = be32toh(buf[i]); >>>>>>> + >>>>>>> +    VIR_CLOSE(f); >>>>>>> +    return buf; >>>>>> Same comments as above... >>>>>> >>>>>>> +} >>>>>>> + >>>>>>> + >>>>>>> +/** >>>>>>> + * This wrapper function receives arguments to be used in a >>>>>>> + * 'find' call to retrieve the file names that matches >>>>>>> + * the criteria inside the /proc/device-tree dir. >>>>>>> + * >>>>>>> + * A 'find' call with '-iname phandle' inside /proc/device-tree >>>>>>> + * provides more than a thousand matches. Adding '-path' to >>>>>>> + * narrow it down further is necessary to keep the file >>>>>>> + * listing sane. >>>>>>> + * >>>>>>> + * The caller is responsible to free the buffer returned by >>>>>>> + * this function. >>>>>>> + */ >>>>>>> +static char * >>>>>>> +retrieve_dt_files_pattern(const char *path_pattern, const char >>>>>>> *file_pattern) >>>>>>> +{ >>>>>>> +    virCommandPtr cmd = NULL; >>>>>>> +    char *output = NULL; >>>>>>> + >>>>>>> +    cmd = virCommandNew("find"); >>>>>>> +    virCommandAddArgList(cmd, "/proc/device-tree/", "-path", >>>>>>> path_pattern, >>>>>>> +                         "-iname", file_pattern, NULL); >>>>>>> +    virCommandSetOutputBuffer(cmd, &output); >>>>>>> + >>>>>>> +    if (virCommandRun(cmd, NULL) < 0) >>>>>>> +        VIR_FREE(output); >>>>>>> + >>>>>>> +    virCommandFree(cmd); >>>>>>> +    return output; >>>>>>> +} >>>>>>> + >>>>>>> + >>>>>>> +/** >>>>>>> + * Helper function that receives a listing of file names and >>>>>>> + * calls read_dt_phandle() on each one finding for a match >>>>>>> + * with the given phandle argument. Returns the file name if a >>>>>>> + * match is found, NULL otherwise. >>>>>>> + */ >>>>>>> +static char * >>>>>>> +find_dt_file_with_phandle(char *files, int phandle) >>>>>>> +{ >>>>>>> +    char *line, *tmp; >>>>>>> +    int ret; >>>>>>> + >>>>>>> +    line = strtok_r(files, "\n", &tmp); >>>>>>> +    do { >>>>>>> +       ret = read_dt_phandle(line); >>>>>>> +       if (ret == phandle) >>>>>>> +           break; >>>>>>> +    } while ((line = strtok_r(NULL, "\n", &tmp)) != NULL); >>>>>>> + >>>>>>> +    return line; >>>>>>> +} >>>>>>> + >>>>>>> + >>>>>>> +/** >>>>>>> + * This function receives a string that represents a PCI device, >>>>>>> + * such as '0004:04:00.0', and tells if the device is NVLink2 >>>>>>> capable. >>>>>>> + * >>>>>>> + * The logic goes as follows: >>>>>>> + * >>>>>>> + * 1 - get the phandle of a nvlink of the device, reading the >>>>>>> 'ibm,npu' >>>>>>> + * attribute; >>>>>>> + * 2 - find the device tree node of the nvlink bus using the >>>>>>> phandle >>>>>>> + * found in (1) >>>>>>> + * 3 - get the phandle of the memory region of the nvlink bus >>>>>>> + * 4 - find the device tree node of the memory region using the >>>>>>> + * phandle found in (3) >>>>>>> + * 5 - read the 'reg' value of the memory region. If the value of >>>>>>> + * the second 64 bit value is 0x02 0x00, the device is attached >>>>>>> + * to a NVLink2 bus. >>>>>>> + * >>>>>>> + * If any of these steps fails, the function returns false. >>>>>>> + */ >>>>>>> +static bool >>>>>>> +device_is_nvlink2_capable(const char *device) >>>>>>> +{ >>>>>>> +    char *file, *files, *tmp; >>>>>>> +    unsigned int *reg; >>>>>>> +    int phandle; >>>>>>> + >>>>>>> +    if ((virAsprintf(&file, >>>>>>> "/sys/bus/pci/devices/%s/of_node/ibm,npu", >>>>>>> +                    device)) < 0) >>>>>>> +        return false; >>>>>>> + >>>>>>> +    /* Find phandles of nvlinks:  */ >>>>>>> +    if ((phandle = read_dt_phandle(file)) == -1) >>>>>>> +        return false; >>>>>>> + >>>>>>> +    /* Find a DT node for the phandle found */ >>>>>>> +    files = retrieve_dt_files_pattern("*device-tree/pci*", >>>>>>> "phandle"); >>>>>>> +    if (!files) >>>>>>> +        return false; >>>>>>> + >>>>>>> +    if ((file = find_dt_file_with_phandle(files, phandle)) == >>>>>>> NULL) >>>>>>> +        goto fail; >>>>>>> + >>>>>>> +    /* Find a phandle of the GPU memory region of the device. The >>>>>>> +     * file found above ends with '/phandle' - the memory region >>>>>>> +     * of the GPU ends with '/memory-region */ >>>>>>> +    tmp = strrchr(file, '/'); >>>>>>> +    *tmp = '\0'; >>>>>>> +    file = strcat(file, "/memory-region"); >>>>>>> + >>>>>>> +    if ((phandle = read_dt_phandle(file)) == -1) >>>>>>> +        goto fail; >>>>>>> + >>>>>>> +    file = NULL; >>>>>>> +    VIR_FREE(files); >>>>>>> + >>>>>>> +    /* Find the memory node for the phandle found above */ >>>>>>> +    files = retrieve_dt_files_pattern("*device-tree/memory*", >>>>>>> "phandle"); >>>>>>> +    if (!files) >>>>>>> +        return false; >>>>>>> + >>>>>>> +    if ((file = find_dt_file_with_phandle(files, phandle)) == >>>>>>> NULL) >>>>>>> +        goto fail; >>>>>>> + >>>>>>> +    /* And see its size in the second 64bit value of 'reg'. First, >>>>>>> +     * the end of the file needs to be changed from '/phandle' to >>>>>>> +     * '/reg' */ >>>>>>> +    tmp = strrchr(file, '/'); >>>>>>> +    *tmp = '\0'; >>>>>>> +    file = strcat(file, "/reg"); >>>>>>> + >>>>>>> +    reg = read_dt_memory_reg(file); >>>>>>> +    if (reg && reg[2] == 0x20 && reg[3] == 0x00) >>>>>>> +        return true; >>>>>> This function is very complex just to find out whether a PCI device >>>>>> is capable >>>>>> of NVLink or not. Feels wrong to do it this way, I believe it would >>>>>> be much >>>>>> easier if NVIDIA exposed a sysfs attribute saying whether a PCI >>>>>> device supports >>>>>> NVLink so that our node-device driver would take care of this >>>>>> during libvirtd >>>>>> startup and then you'd only call a single API from the PPC64 helper >>>>>> you're >>>>>> introducing in the next patch to find out whether you need the >>>>>> alternative >>>>>> formula or not. >>>>>> >>>>>> Honestly, apart from the coding style issues, I don't like this >>>>>> approach and >>>>>> unless there's a really compelling reason for libvirt to do it in a >>>>>> way which >>>>>> involves spawning a 'find' process because of a complex pattern and >>>>>> a bunch of >>>>>> data necessary to filter out, I'd really suggest contacting NVIDIA >>>>>> about this. >>>>>> It's the same for mdevs, NVIDIA exposes a bunch of attributes in >>>>>> sysfs which >>>>>> we're able to read. >>>>> I'll contact NVIDIA and see if there is an easier way (a sysfs >>>>> attribute, for >>>>> example) and, if doesn't, try to provide a plausibe reason to >>>>> justify this >>>>> detection code. >>>> Sorry for the delay in responding. The problem is that all the V100 >>>> GPUs >>>> support NVLink, but it may or may not be connected up. This is >>>> detected >>>> at runtime during GPU initialization, which seems like much too >>>> heavy of >>>> an operation to perform as part of passthrough initialization. And >>>> that's >>>> why vfio-pci pieces rely on device tree information to figure it out. >>>> >>>> Alexey, would it be possible for vfio-pci to export the information >>>> in a >>>> way more friendly to libvirt? >>> The only information needed here is whether a specific GPU has RAM or >>> not. This can easily be found from the device-tree, imho quite friendly >>> already. VFIO gets to know about these new capabilities when the VFIO >>> PCI device is opened, and we rather want to avoid going that far in >>> libvirt (open a VFIO container, attach a group, get a vfio-pci fd from >>> it, enumerate regions - 2 PCI resets on the way, delays, meh). >>> >>> btw the first "find" for "ibm,npu" can be skipped - NVLinks have to be >>> passed too or the entire RAM thing won't work. "find" for the memory >>> node can also be dropped really - if NVLink bridge OF node has >>> "memory-region", then VFIO will most likely expose RAM and QEMU will >>> try >>> using it anyway. >> >> Hmmm I am not not sure I understood it completely ..... seeing the >> of_nodes exposed in sysfs of one of the NVLink buses we have: >> >> >> /sys/bus/pci/devices/0007:00:00.0/of_node$ ls >> class-code           ibm,gpu       ibm,nvlink-speed memory-region  reg >> device-id            ibm,loc-code  ibm,pci-config-space-type >> name           revision-id >> ibm,device-tgt-addr  ibm,nvlink    interrupts phandle        vendor-id >> >> >> We can make a safe assumption that the V100 GPU will always be passed >> through with at least one NVLink2 bus. > Assumption #1: if we want GPU RAM to be available in the guest, an > NVLink bridge must be passed through. No bridge - no RAM - no rlimit > adjustment. > > >> How many hops do we need to assert >> that a given device is a NVLink2 bus from its of_node info? >> >> For example, can we say something like: >> >> "The device node of this device has ibm,gpu and ibm,nvlink and >> ibm,nvlink-speed >> and ibm,nvlink-speed is 0x8, so this is a NVLink2 bus. Since it is not > > ibm,nvlink-speed can be different (I saw 9), you cannot rely on this. > > >> possible to pass >> through the bus alone, there is a V100 GPU in this same IOMMU group. So >> this is >> a NVLink2 passthrough scenario" >> >> >> Or perhaps: >> >> "It has ibm,gpu and ibm,nvlink and the of_node of its memory-region has >> a reg >> value of 0x20 , thus this is a nvlink2 bus and ....." > > 0x20 is a size of the GPU RAM window which might change in different > revisions of the hw as well. > >> Both alternatives are way simpler than what I'm doing in this patch. I >> am not sure >> if they are valid though. > > Assumption #2: if the nvlink2 bridge's DT node has memory-region, then > the GPU will try to use its RAM. The adjustment you'll have to make > won't depend on anything as the existing GPU RAM placement is so that > whatever you can possibly throw at QEMU will fit 128TiB window and we > cannot make the window smaller anyway (next size would lower power of > two - 64TiB - and GPU RAM lives just above that). Ok, so, from the of_node of an unknown device that has been passed through to a VM, if the DT node has: ibm,gpu ibm,nvlink memory-region Is that enough to justify the rlimit adjustment for NVLink2?

> > I could do better of course and allow adjusting the limit to cover let's > say 66TiB instead of 128TiB, it just seems to be unnecessary > complication (we already allocate only required amount of TCE entries on > demand). > > > >>>>>> Thinking about it a bit more, since this is NVIDIA-specific, >>>>>> having an >>>>>> NVIDIA-only sysfs attribute doesn't help the node-device driver I >>>>>> mentioned >>>>>> above. In general, we try to avoid introducing vendor-specific >>>>>> code, so nodedev >>>>>> might not be the right place to check for the attribute (because >>>>>> it's not >>>>>> universal and would require vendor-specific elements), but I think >>>>>> we could >>>>>> have an NVLink helper reading this sysfs attribute(s) elsewhere in >>>>>> libvirt >>>>>> codebase. >>>>>> >>>>>> Erik > >