On 12/10/19 9:16 PM, Cole Robinson wrote:
On 12/2/19 9:26 AM, Michal Privoznik wrote:If a domain has an NVMe disk configured, then we need to create /dev/vfio/* paths in domain's namespace so that qemu can open them. Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx> --- src/qemu/qemu_domain.c | 67 ++++++++++++++++++++++++++++++++---------- 1 file changed, 51 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 7b515b9520..70c4ee8e65 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -13430,16 +13430,29 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg G_GNUC_UNUSED, { virStorageSourcePtr next; char *dst = NULL; + bool hasNVMe = false; int ret = -1;for (next = disk->src; virStorageSourceIsBacking(next); next = next->backingStore) {- if (!next->path || !virStorageSourceIsLocalStorage(next)) { - /* Not creating device. Just continue. */ - continue; - } + if (next->type == VIR_STORAGE_TYPE_NVME) { + g_autofree char *nvmePath = NULL;- if (qemuDomainCreateDevice(next->path, data, false) < 0)- goto cleanup; + hasNVMe = true; + + if (!(nvmePath = virPCIDeviceAddressGetIOMMUGroupDev(&next->nvme->pciAddr))) + goto cleanup; + + if (qemuDomainCreateDevice(nvmePath, data, false) < 0) + goto cleanup; + } else { + if (!next->path || !virStorageSourceIsLocalStorage(next)) { + /* Not creating device. Just continue. */ + continue; + } + + if (qemuDomainCreateDevice(next->path, data, false) < 0) + goto cleanup; + } }I don't see anything wrong with this patch. In the interest of getting this series into git, here is my: Reviewed-by: Cole Robinson <crobinso@xxxxxxxxxx> But, I'm finding a lot of the 'next->type == VIR_STORAGE_TYPE_NVME' sprinkled around unfortunate, here and in later patches. It seems like there's two StorageSource cases here involving local filesystem paths: 1) Does src->path contain the <source> storage location? Answered by virStorageSourceIsLocalStorage 2) Does src have a local path that the VM needs to access? Until now this was always the same as #1 && src->path, but not any more. So maybe add helpers like: bool virStorageSourceHasLocalResource(virStorageSourcePtr src) { return next->path || next->type == VIR_STORAGE_TYPE_NVME; } const char * virStorageSourceGetLocalResource(virStorageSourcePtr src) { if (!virStorageSourceHasLocalResource(src)) <error> if (next->type == NVME) <return VFIO path or error> return strdup(next->path); } I think virStorageSourceIsLocalStorage should be renamed too because now it is ambiguous. Maybe something more explicit like SourceUsesPath, or SourceIsLocalPath. Naming sucks obviously so suggestions welcome With those funtions, the loop above becomes: for (...) { g_autofree char *path = NULL; if (!HasLocalResource) continue if (!(path = GetLocalResource(...)) error <do stuff with path> } The hasNVMe bit can then be replaced with virStorageSourceChainHasNVMe security driver usage will be simpler too I think. But maybe I'm missing some complication
I think you're right. I will save that to a follow up series. Michal -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
