From: Wei Liu <wei.liu@kernel.org> Date: 2021-07-09 11:43:48
Hi all
Device passthrough is a critial feature for a virtualization stack. When
designing this feature for MSHV support on Linux, one important
considration is to not deviate from Linux's default VFIO stack. VFIO
relies on an IOMMU or IOMMUs in the system to manipulate DMA mappings.
In this series an IOMMU driver is implemented using a set of hypercall
interfaces provided by the Microsoft Hypervisor. At this stage only DMA
remapping is implemented. Interrupt remapping will come later.
With this series I'm able to passthrough an NVMe drive to a guest with VFIO on
a modified version of Cloud Hypervisor. From users' point of view, nothing
needs changing. Cloud Hypervisor and Rust-VMM changes, which depend on the new
kernel UAPIs from this series, will be upstreamed too.
This series is built on top of Nuno and Vineeth's patches [0][1].
The meat is in the patch named "mshv: add paravirtualized IOMMU
support".
The in-kernel device framework and the VFIO bridge device are heavily
inspired by KVM's code. I pondered whether it would be worth refactoring
the code in KVM but decided against that route for two reasons: 1. it
allowed faster prototyping and 2. I was not sure if that's something KVM
community would agree to.
For the VT-D changes, what we're after is to build the RMRR regions list
so that reserved regions are respected. Instead of doing a bad job
myself, I decided to piggy-back on Intel's own code. AMD support is to
be added until we have an AMD system.
Comments are welcome.
Thanks,
Wei.
[0] https://lore.kernel.org/linux-hyperv/1622241819-21155-1-git-send-email-nunodasneves@linux.microsoft.com/
[1] https://lore.kernel.org/linux-hyperv/cover.1622654100.git.viremana@linux.microsoft.com/
Wei Liu (8):
x86/hyperv: export hv_build_pci_dev_id
asm-generic/hyperv: add device domain definitions
intel/vt-d: make DMAR table parsing code more flexible
intel/vt-d: export intel_iommu_get_resv_regions
mshv: add paravirtualized IOMMU support
mshv: command line option to skip devices in PV-IOMMU
mshv: implement in-kernel device framework
mshv: add vfio bridge device
Documentation/virt/mshv/api.rst | 12 +
arch/x86/hyperv/irqdomain.c | 3 +-
arch/x86/include/asm/mshyperv.h | 1 +
drivers/hv/Kconfig | 4 +
drivers/hv/Makefile | 2 +-
drivers/hv/mshv_main.c | 186 ++++++++
drivers/hv/vfio.c | 244 ++++++++++
drivers/hv/vfio.h | 18 +
drivers/iommu/Kconfig | 14 +
drivers/iommu/hyperv-iommu.c | 673 ++++++++++++++++++++++++++++
drivers/iommu/intel/dmar.c | 38 +-
drivers/iommu/intel/iommu.c | 7 +-
drivers/iommu/intel/irq_remapping.c | 2 +-
include/asm-generic/hyperv-tlfs.h | 144 ++++++
include/linux/dmar.h | 2 +-
include/linux/intel-iommu.h | 4 +
include/linux/mshv.h | 57 +++
include/uapi/linux/mshv.h | 36 ++
18 files changed, 1429 insertions(+), 18 deletions(-)
create mode 100644 drivers/hv/vfio.c
create mode 100644 drivers/hv/vfio.h
--
2.30.2
From: Wei Liu <wei.liu@kernel.org> Date: 2021-07-09 11:43:50
These definitions are for the IOMMU device domain interfaces exposed by
Microsoft Hyperivsor. We will use them to implement DMA remapping.
Signed-off-by: Wei Liu <wei.liu@kernel.org>
---
include/asm-generic/hyperv-tlfs.h | 144 ++++++++++++++++++++++++++++++
1 file changed, 144 insertions(+)
From: Wei Liu <wei.liu@kernel.org> Date: 2021-07-09 11:43:54
When Microsoft Hypervisor runs on Intel platforms it needs to know the
reserved regions to program devices correctly. There is no reason to
duplicate intel_iommu_get_resv_regions. Export it.
Signed-off-by: Wei Liu <wei.liu@kernel.org>
---
drivers/iommu/intel/iommu.c | 5 +++--
include/linux/intel-iommu.h | 4 ++++
2 files changed, 7 insertions(+), 2 deletions(-)
From: Wei Liu <wei.liu@kernel.org> Date: 2021-07-09 11:43:55
Microsoft Hypervisor provides a set of hypercalls to manage device
domains. The root kernel should parse the DMAR so that it can program
the IOMMU (with hypercalls) correctly.
The DMAR code was designed to work with Intel IOMMU only. Add two more
parameters to make it useful to Microsoft Hypervisor. Microsoft
Hypervisor does not need the DMAR parsing code to allocate an Intel
IOMMU structure; it also wishes to always reparse the DMAR table even
after it has been parsed before.
Adjust Intel IOMMU code to use the new dmar_table_init. There should be
no functional change to Intel IOMMU code.
Signed-off-by: Wei Liu <wei.liu@kernel.org>
---
We may be able to combine alloc and force_parse?
---
drivers/iommu/intel/dmar.c | 38 ++++++++++++++++++++---------
drivers/iommu/intel/iommu.c | 2 +-
drivers/iommu/intel/irq_remapping.c | 2 +-
include/linux/dmar.h | 2 +-
4 files changed, 30 insertions(+), 14 deletions(-)
From: Wei Liu <wei.liu@kernel.org> Date: 2021-07-09 11:43:56
Microsoft Hypervisor provides a set of hypercalls to manage device
domains. Implement a type-1 IOMMU using those hypercalls.
Implement DMA remapping as the first step for this driver. Interrupt
remapping will come in a later stage.
Signed-off-by: Wei Liu <wei.liu@kernel.org>
---
drivers/iommu/Kconfig | 14 +
drivers/iommu/hyperv-iommu.c | 628 +++++++++++++++++++++++++++++++++++
2 files changed, 642 insertions(+)
@@ -338,3 +346,623 @@ static const struct irq_domain_ops hyperv_root_ir_domain_ops = {};#endif++#ifdef CONFIG_HYPERV_ROOT_PVIOMMU++/* DMA remapping support */+structhv_iommu_domain{+structiommu_domaindomain;+structhv_iommu_dev*hv_iommu;++structhv_input_device_domaindevice_domain;++spinlock_tmappings_lock;+structrb_root_cachedmappings;++u32map_flags;+u64pgsize_bitmap;+};++/* What hardware IOMMU? */+enum{+INVALID=0,+INTEL,/* Only Intel for now */+}hw_iommu_type={INVALID};++staticstructhv_iommu_domainhv_identity_domain,hv_null_domain;++#define to_hv_iommu_domain(d) \+container_of(d,structhv_iommu_domain,domain)++structhv_iommu_mapping{+phys_addr_tpaddr;+structinterval_tree_nodeiova;+u32flags;+};++structhv_iommu_dev{+structiommu_deviceiommu;++structidadomain_ids;++/* Device configuration */+structiommu_domain_geometrygeometry;+u64first_domain;+u64last_domain;++u32map_flags;+u64pgsize_bitmap;+};++staticstructhv_iommu_dev*hv_iommu_device;++structhv_iommu_endpoint{+structdevice*dev;+structhv_iommu_dev*hv_iommu;+structhv_iommu_domain*domain;+};++staticvoid__inithv_initialize_special_domains(void)+{+structhv_iommu_domain*domain;++/* Default passthrough domain */+domain=&hv_identity_domain;++memset(domain,0,sizeof(*domain));++domain->device_domain.partition_id=HV_PARTITION_ID_SELF;+domain->device_domain.domain_id.type=HV_DEVICE_DOMAIN_TYPE_S2;+domain->device_domain.domain_id.id=HV_DEVICE_DOMAIN_ID_S2_DEFAULT;++domain->domain.geometry=hv_iommu_device->geometry;++/* NULL domain that blocks all DMA transactions */+domain=&hv_null_domain;++memset(domain,0,sizeof(*domain));++domain->device_domain.partition_id=HV_PARTITION_ID_SELF;+domain->device_domain.domain_id.type=HV_DEVICE_DOMAIN_TYPE_S2;+domain->device_domain.domain_id.id=HV_DEVICE_DOMAIN_ID_S2_NULL;++domain->domain.geometry=hv_iommu_device->geometry;+}++staticboolis_identity_domain(structhv_iommu_domain*d)+{+returnd->device_domain.domain_id.id==HV_DEVICE_DOMAIN_ID_S2_DEFAULT;+}++staticboolis_null_domain(structhv_iommu_domain*d)+{+returnd->device_domain.domain_id.id==HV_DEVICE_DOMAIN_ID_S2_NULL;+}++staticstructiommu_domain*hv_iommu_domain_alloc(unsignedtype)+{+structhv_iommu_domain*domain;+intret;+u64status;+unsignedlongflags;+structhv_input_create_device_domain*input;++if(type==IOMMU_DOMAIN_IDENTITY)+return&hv_identity_domain.domain;++if(type==IOMMU_DOMAIN_BLOCKED)+return&hv_null_domain.domain;++domain=kzalloc(sizeof(*domain),GFP_KERNEL);+if(!domain)+gotoout;++spin_lock_init(&domain->mappings_lock);+domain->mappings=RB_ROOT_CACHED;++if(type==IOMMU_DOMAIN_DMA&&+iommu_get_dma_cookie(&domain->domain)){+gotoout_free;+}++ret=ida_alloc_range(&hv_iommu_device->domain_ids,+hv_iommu_device->first_domain,hv_iommu_device->last_domain,+GFP_KERNEL);+if(ret<0)+gotoout_put_cookie;++domain->device_domain.partition_id=HV_PARTITION_ID_SELF;+domain->device_domain.domain_id.type=HV_DEVICE_DOMAIN_TYPE_S2;+domain->device_domain.domain_id.id=ret;++domain->hv_iommu=hv_iommu_device;+domain->map_flags=hv_iommu_device->map_flags;++local_irq_save(flags);++input=*this_cpu_ptr(hyperv_pcpu_input_arg);+memset(input,0,sizeof(*input));++input->device_domain=domain->device_domain;++input->create_device_domain_flags.forward_progress_required=1;+input->create_device_domain_flags.inherit_owning_vtl=0;++status=hv_do_hypercall(HVCALL_CREATE_DEVICE_DOMAIN,input,NULL);++local_irq_restore(flags);++if(!hv_result_success(status)){+pr_err("%s: hypercall failed, status %lld\n",__func__,status);+gotoout_free_id;+}++domain->domain.pgsize_bitmap=hv_iommu_device->pgsize_bitmap;+domain->domain.geometry=hv_iommu_device->geometry;++return&domain->domain;++out_free_id:+ida_free(&hv_iommu_device->domain_ids,domain->device_domain.domain_id.id);+out_put_cookie:+iommu_put_dma_cookie(&domain->domain);+out_free:+kfree(domain);+out:+returnNULL;+}++staticvoidhv_iommu_domain_free(structiommu_domain*d)+{+structhv_iommu_domain*domain=to_hv_iommu_domain(d);+unsignedlongflags;+u64status;+structhv_input_delete_device_domain*input;++if(is_identity_domain(domain)||is_null_domain(domain))+return;++local_irq_save(flags);+input=*this_cpu_ptr(hyperv_pcpu_input_arg);+memset(input,0,sizeof(*input));++input->device_domain=domain->device_domain;++status=hv_do_hypercall(HVCALL_DELETE_DEVICE_DOMAIN,input,NULL);++local_irq_restore(flags);++if(!hv_result_success(status))+pr_err("%s: hypercall failed, status %lld\n",__func__,status);++ida_free(&domain->hv_iommu->domain_ids,domain->device_domain.domain_id.id);++iommu_put_dma_cookie(d);++kfree(domain);+}++staticinthv_iommu_attach_dev(structiommu_domain*d,structdevice*dev)+{+structhv_iommu_domain*domain=to_hv_iommu_domain(d);+u64status;+unsignedlongflags;+structhv_input_attach_device_domain*input;+structpci_dev*pdev;+structhv_iommu_endpoint*vdev=dev_iommu_priv_get(dev);++/* Only allow PCI devices for now */+if(!dev_is_pci(dev))+return-EINVAL;++pdev=to_pci_dev(dev);++dev_dbg(dev,"Attaching (%strusted) to %d\n",pdev->untrusted?"un":"",+domain->device_domain.domain_id.id);++local_irq_save(flags);+input=*this_cpu_ptr(hyperv_pcpu_input_arg);+memset(input,0,sizeof(*input));++input->device_domain=domain->device_domain;+input->device_id=hv_build_pci_dev_id(pdev);++status=hv_do_hypercall(HVCALL_ATTACH_DEVICE_DOMAIN,input,NULL);+local_irq_restore(flags);++if(!hv_result_success(status))+pr_err("%s: hypercall failed, status %lld\n",__func__,status);+else+vdev->domain=domain;++returnhv_status_to_errno(status);+}++staticvoidhv_iommu_detach_dev(structiommu_domain*d,structdevice*dev)+{+u64status;+unsignedlongflags;+structhv_input_detach_device_domain*input;+structpci_dev*pdev;+structhv_iommu_domain*domain=to_hv_iommu_domain(d);+structhv_iommu_endpoint*vdev=dev_iommu_priv_get(dev);++/* See the attach function, only PCI devices for now */+if(!dev_is_pci(dev))+return;++pdev=to_pci_dev(dev);++dev_dbg(dev,"Detaching from %d\n",domain->device_domain.domain_id.id);++local_irq_save(flags);+input=*this_cpu_ptr(hyperv_pcpu_input_arg);+memset(input,0,sizeof(*input));++input->partition_id=HV_PARTITION_ID_SELF;+input->device_id=hv_build_pci_dev_id(pdev);++status=hv_do_hypercall(HVCALL_DETACH_DEVICE_DOMAIN,input,NULL);+local_irq_restore(flags);++if(!hv_result_success(status))+pr_err("%s: hypercall failed, status %lld\n",__func__,status);++vdev->domain=NULL;+}++staticinthv_iommu_add_mapping(structhv_iommu_domain*domain,unsignedlongiova,+phys_addr_tpaddr,size_tsize,u32flags)+{+unsignedlongirqflags;+structhv_iommu_mapping*mapping;++mapping=kzalloc(sizeof(*mapping),GFP_ATOMIC);+if(!mapping)+return-ENOMEM;++mapping->paddr=paddr;+mapping->iova.start=iova;+mapping->iova.last=iova+size-1;+mapping->flags=flags;++spin_lock_irqsave(&domain->mappings_lock,irqflags);+interval_tree_insert(&mapping->iova,&domain->mappings);+spin_unlock_irqrestore(&domain->mappings_lock,irqflags);++return0;+}++staticsize_thv_iommu_del_mappings(structhv_iommu_domain*domain,+unsignedlongiova,size_tsize)+{+unsignedlongflags;+size_tunmapped=0;+unsignedlonglast=iova+size-1;+structhv_iommu_mapping*mapping=NULL;+structinterval_tree_node*node,*next;++spin_lock_irqsave(&domain->mappings_lock,flags);+next=interval_tree_iter_first(&domain->mappings,iova,last);+while(next){+node=next;+mapping=container_of(node,structhv_iommu_mapping,iova);+next=interval_tree_iter_next(node,iova,last);++/* Trying to split a mapping? Not supported for now. */+if(mapping->iova.start<iova)+break;++unmapped+=mapping->iova.last-mapping->iova.start+1;++interval_tree_remove(node,&domain->mappings);+kfree(mapping);+}+spin_unlock_irqrestore(&domain->mappings_lock,flags);++returnunmapped;+}++staticinthv_iommu_map(structiommu_domain*d,unsignedlongiova,+phys_addr_tpaddr,size_tsize,intprot,gfp_tgfp)+{+u32map_flags;+unsignedlongflags,pfn,npages;+intret,i;+structhv_iommu_domain*domain=to_hv_iommu_domain(d);+structhv_input_map_device_gpa_pages*input;+u64status;++/* Reject size that's not a whole page */+if(size&~HV_HYP_PAGE_MASK)+return-EINVAL;++map_flags=HV_MAP_GPA_READABLE;/* Always required */+map_flags|=prot&IOMMU_WRITE?HV_MAP_GPA_WRITABLE:0;++ret=hv_iommu_add_mapping(domain,iova,paddr,size,flags);+if(ret)+returnret;++npages=size>>HV_HYP_PAGE_SHIFT;++local_irq_save(flags);+input=*this_cpu_ptr(hyperv_pcpu_input_arg);+memset(input,0,sizeof(*input));++input->device_domain=domain->device_domain;+input->map_flags=map_flags;+input->target_device_va_base=iova;++pfn=paddr>>HV_HYP_PAGE_SHIFT;+for(i=0;i<npages;i++){+input->gpa_page_list[i]=pfn;+pfn+=1;+}++status=hv_do_rep_hypercall(HVCALL_MAP_DEVICE_GPA_PAGES,npages,0,+input,NULL);++local_irq_restore(flags);++if(!hv_result_success(status)){+pr_err("%s: hypercall failed, status %lld\n",__func__,status);+hv_iommu_del_mappings(domain,iova,size);+}++returnhv_status_to_errno(status);+}++staticsize_thv_iommu_unmap(structiommu_domain*d,unsignedlongiova,+size_tsize,structiommu_iotlb_gather*gather)+{+size_tunmapped;+structhv_iommu_domain*domain=to_hv_iommu_domain(d);+unsignedlongflags,npages;+structhv_input_unmap_device_gpa_pages*input;+u64status;++unmapped=hv_iommu_del_mappings(domain,iova,size);+if(unmapped<size)+return0;++npages=size>>HV_HYP_PAGE_SHIFT;++local_irq_save(flags);+input=*this_cpu_ptr(hyperv_pcpu_input_arg);+memset(input,0,sizeof(*input));++input->device_domain=domain->device_domain;+input->target_device_va_base=iova;++/* Unmap `npages` pages starting from VA base */+status=hv_do_rep_hypercall(HVCALL_UNMAP_DEVICE_GPA_PAGES,npages,+0,input,NULL);++local_irq_restore(flags);++if(!hv_result_success(status))+pr_err("%s: hypercall failed, status %lld\n",__func__,status);++returnhv_result_success(status)?unmapped:0;+}++staticphys_addr_thv_iommu_iova_to_phys(structiommu_domain*d,+dma_addr_tiova)+{+u64paddr=0;+unsignedlongflags;+structhv_iommu_mapping*mapping;+structinterval_tree_node*node;+structhv_iommu_domain*domain=to_hv_iommu_domain(d);++spin_lock_irqsave(&domain->mappings_lock,flags);+node=interval_tree_iter_first(&domain->mappings,iova,iova);+if(node){+mapping=container_of(node,structhv_iommu_mapping,iova);+paddr=mapping->paddr+(iova-mapping->iova.start);+}+spin_unlock_irqrestore(&domain->mappings_lock,flags);++returnpaddr;+}++staticstructiommu_device*hv_iommu_probe_device(structdevice*dev)+{+structhv_iommu_endpoint*vdev;++if(!dev_is_pci(dev))+returnERR_PTR(-ENODEV);++vdev=kzalloc(sizeof(*vdev),GFP_KERNEL);+if(!vdev)+returnERR_PTR(-ENOMEM);++vdev->dev=dev;+vdev->hv_iommu=hv_iommu_device;+dev_iommu_priv_set(dev,vdev);++return&vdev->hv_iommu->iommu;+}++staticvoidhv_iommu_probe_finalize(structdevice*dev)+{+structiommu_domain*d=iommu_get_domain_for_dev(dev);++if(d&&d->type==IOMMU_DOMAIN_DMA)+iommu_setup_dma_ops(dev,1<<PAGE_SHIFT,0);+else+set_dma_ops(dev,NULL);+}++staticvoidhv_iommu_release_device(structdevice*dev)+{+structhv_iommu_endpoint*vdev=dev_iommu_priv_get(dev);++/* Need to detach device from device domain if necessary. */+if(vdev->domain)+hv_iommu_detach_dev(&vdev->domain->domain,dev);++dev_iommu_priv_set(dev,NULL);+set_dma_ops(dev,NULL);++kfree(vdev);+}++staticstructiommu_group*hv_iommu_device_group(structdevice*dev)+{+if(dev_is_pci(dev))+returnpci_device_group(dev);+else+returngeneric_device_group(dev);+}++/*+*Thisbitmapisusedtoadvertisethepagesizesourhardwaresupporttothe+*IOMMUcore,whichwillthenusethisinformationtosplitphysically+*contiguousmemoryregionsitismappingintopagesizesthatwesupport.+*+*GiventhesizeofthePFNarraywecanaccommodatelessthan5124KiBpages.+*/+#define HV_IOMMU_PGSIZES (SZ_4K | SZ_1M)++staticvoidhv_iommu_get_resv_regions(structdevice*dev,+structlist_head*head)+{+switch(hw_iommu_type){+caseINTEL:+intel_iommu_get_resv_regions(dev,head);+break;+default:+/* Do nothing */;+}+}++staticinthv_iommu_def_domain_type(structdevice*dev)+{+/* The hypervisor has created a default passthrough domain */+returnIOMMU_DOMAIN_IDENTITY;+}++staticstructiommu_opshv_iommu_ops={+.domain_alloc=hv_iommu_domain_alloc,+.domain_free=hv_iommu_domain_free,+.attach_dev=hv_iommu_attach_dev,+.detach_dev=hv_iommu_detach_dev,+.map=hv_iommu_map,+.unmap=hv_iommu_unmap,+.iova_to_phys=hv_iommu_iova_to_phys,+.probe_device=hv_iommu_probe_device,+.probe_finalize=hv_iommu_probe_finalize,+.release_device=hv_iommu_release_device,+.def_domain_type=hv_iommu_def_domain_type,+.device_group=hv_iommu_device_group,+.get_resv_regions=hv_iommu_get_resv_regions,+.put_resv_regions=generic_iommu_put_resv_regions,+.pgsize_bitmap=HV_IOMMU_PGSIZES,+.owner=THIS_MODULE,+};++staticvoid__inithv_initalize_resv_regions_intel(void)+{+intret;++down_write(&dmar_global_lock);+if(dmar_table_init(false,true)){+pr_err("Failed to initialize DMAR table\n");+up_write(&dmar_global_lock);+return;+}++ret=dmar_dev_scope_init();+if(ret)+pr_err("Failed to initialize device scope\n");++up_write(&dmar_global_lock);++hw_iommu_type=INTEL;+}++staticvoid__inithv_initialize_resv_regions(void)+{+hv_initalize_resv_regions_intel();+}++int__inithv_iommu_init(void)+{+intret=0;+structhv_iommu_dev*hv_iommu=NULL;++if(!hv_is_hyperv_initialized())+return-ENODEV;++hv_initialize_resv_regions();++hv_iommu=kzalloc(sizeof(*hv_iommu),GFP_KERNEL);+if(!hv_iommu)+return-ENOMEM;++ida_init(&hv_iommu->domain_ids);+hv_iommu->first_domain=HV_DEVICE_DOMAIN_ID_S2_DEFAULT+1;+hv_iommu->last_domain=HV_DEVICE_DOMAIN_ID_S2_NULL-1;++hv_iommu->geometry=(structiommu_domain_geometry){+.aperture_start=0,+.aperture_end=-1UL,+.force_aperture=true,+};++hv_iommu->map_flags=IOMMU_READ|IOMMU_WRITE;+hv_iommu->pgsize_bitmap=HV_IOMMU_PGSIZES;++ret=iommu_device_sysfs_add(&hv_iommu->iommu,NULL,NULL,"%s","hv-iommu");+if(ret){+pr_err("iommu_device_sysfs_add failed: %d\n",ret);+gotoerr_free;+}++ret=iommu_device_register(&hv_iommu->iommu,&hv_iommu_ops,NULL);+if(ret){+pr_err("iommu_device_register failed: %d\n",ret);+gotoerr_sysfs_remove;+}++/* This must come before bus_set_iommu because it calls into the hooks. */+hv_iommu_device=hv_iommu;+hv_initialize_special_domains();++#ifdef CONFIG_PCI+ret=bus_set_iommu(&pci_bus_type,&hv_iommu_ops);+if(ret){+pr_err("bus_set_iommu failed: %d\n",ret);+gotoerr_unregister;+}+#endif+pr_info("Microsoft Hypervisor IOMMU initialized\n");++return0;++#ifdef CONFIG_PCI+err_unregister:+iommu_device_unregister(&hv_iommu->iommu);+#endif+err_sysfs_remove:+iommu_device_sysfs_remove(&hv_iommu->iommu);+err_free:+kfree(hv_iommu);+returnret;+}++int__inithv_iommu_detect(void)+{+if(!(ms_hyperv.misc_features&HV_DEVICE_DOMAIN_AVAILABLE))+return-ENODEV;++iommu_detected=1;+x86_init.iommu.iommu_init=hv_iommu_init;++return1;+}+IOMMU_INIT_POST(hv_iommu_detect);++#endif /* CONFIG_HYPERV_ROOT_PVIOMMU */
From: Wei Liu <wei.liu@kernel.org> Date: 2021-07-09 11:43:57
Some devices may have been claimed by the hypervisor already. One such
example is a user can assign a NIC for debugging purpose.
Ideally Linux should be able to tell retrieve that information, but
there is no way to do that yet. And designing that new mechanism is
going to take time.
Provide a command line option for skipping devices. This is a stopgap
solution, so it is intentionally undocumented. Hopefully we can retire
it in the future.
Signed-off-by: Wei Liu <wei.liu@kernel.org>
---
drivers/iommu/hyperv-iommu.c | 45 ++++++++++++++++++++++++++++++++++++
1 file changed, 45 insertions(+)
From: Wei Liu <wei.liu@kernel.org> Date: 2021-07-09 11:44:05
This is basically the same code adopted from KVM. The main user case is
the future MSHV-VFIO bridge device. We don't have any plan to support
in-kernel device emulation yet, but it wouldn't hurt to make the code
more flexible.
Signed-off-by: Wei Liu <wei.liu@kernel.org>
---
Documentation/virt/mshv/api.rst | 12 +++
drivers/hv/mshv_main.c | 181 ++++++++++++++++++++++++++++++++
include/linux/mshv.h | 57 ++++++++++
include/uapi/linux/mshv.h | 36 +++++++
4 files changed, 286 insertions(+)
@@ -170,4 +170,16 @@ Can be used to get/set various properties of a partition. Some properties can only be set at partition creation. For these, there are parameters in MSHV_CREATE_PARTITION.+3.12 MSHV_CREATE_DEVICE+-----------------------+:Type: partition ioctl+:Parameters: struct mshv_create_device+:Returns: 0 on success++Can be used to create an in-kernel device.++If the MSHV_CREATE_DEVICE_TEST flag is set, only test whether the+device type is supported (not necessarily whether it can be created+in the current vm).+Currently only supports VFIO type device.
@@ -121,4 +123,59 @@ struct mshv {}partitions;};+structmshv_device{+conststructmshv_device_ops*ops;+structmshv_partition*partition;+void*private;+structlist_headpartition_node;++};++/* create, destroy, and name are mandatory */+structmshv_device_ops{+constchar*name;++/*+*createiscalledholdingpartition->mutexandanyoperationsnotsuitable+*todowhileholdingthelockshouldbedeferredtoinit(see+*below).+*/+int(*create)(structmshv_device*dev,u32type);++/*+*initiscalledaftercreateifcreateissuccessfulandiscalled+*outsideofholdingpartition->mutex.+*/+void(*init)(structmshv_device*dev);++/*+*Destroyisresponsibleforfreeingdev.+*+*Destroymaybecalledbeforeorafterdestructorsarecalled+*onemulatedI/Oregions,dependingonwhetherareferenceis+*heldbyavcpuorothermshvcomponentthatgetsdestroyed+*aftertheemulatedI/O.+*/+void(*destroy)(structmshv_device*dev);++/*+*Releaseisanalternativemethodtofreethedevice.Itis+*calledwhenthedevicefiledescriptorisclosed.Once+*releaseiscalled,thedestroymethodwillnotbecalled+*anymoreasthedeviceisremovedfromthedevicelistof+*theVM.partition->mutexisheld.+*/+void(*release)(structmshv_device*dev);++int(*set_attr)(structmshv_device*dev,structmshv_device_attr*attr);+int(*get_attr)(structmshv_device*dev,structmshv_device_attr*attr);+int(*has_attr)(structmshv_device*dev,structmshv_device_attr*attr);+long(*ioctl)(structmshv_device*dev,unsignedintioctl,+unsignedlongarg);+int(*mmap)(structmshv_device*dev,structvm_area_struct*vma);+};++intmshv_register_device_ops(conststructmshv_device_ops*ops,u32type);+voidmshv_unregister_device_ops(u32type);+#endif
From: Wei Liu <wei.liu@kernel.org> Date: 2021-07-09 11:44:07
The main purpose of this device at this stage is to hold a reference to
the vfio_group to avoid it disappearing under our feet.
Signed-off-by: Wei Liu <wei.liu@kernel.org>
---
Is there value in unifying with KVM?
---
drivers/hv/Kconfig | 4 +
drivers/hv/Makefile | 2 +-
drivers/hv/mshv_main.c | 5 +
drivers/hv/vfio.c | 244 +++++++++++++++++++++++++++++++++++++++++
drivers/hv/vfio.h | 18 +++
5 files changed, 272 insertions(+), 1 deletion(-)
create mode 100644 drivers/hv/vfio.c
create mode 100644 drivers/hv/vfio.h
@@ -0,0 +1,244 @@+// SPDX-License-Identifier: GPL-2.0-only+/*+*VFIO-MSHVbridgepseudodevice+*+*HeavilyinspiredbytheVFIO-KVMbridgepseudodevice.+*Copyright(C)2013RedHat,Inc.Allrightsreserved.+*Author:AlexWilliamson<alex.williamson@redhat.com>+*/++#include<linux/errno.h>+#include<linux/file.h>+#include<linux/list.h>+#include<linux/module.h>+#include<linux/mutex.h>+#include<linux/slab.h>+#include<linux/uaccess.h>+#include<linux/vfio.h>+#include<linux/mshv.h>++#include"vfio.h"+++structmshv_vfio_group{+structlist_headnode;+structvfio_group*vfio_group;+};++structmshv_vfio{+structlist_headgroup_list;+structmutexlock;+};++staticstructvfio_group*mshv_vfio_group_get_external_user(structfile*filep)+{+structvfio_group*vfio_group;+structvfio_group*(*fn)(structfile*);++fn=symbol_get(vfio_group_get_external_user);+if(!fn)+returnERR_PTR(-EINVAL);++vfio_group=fn(filep);++symbol_put(vfio_group_get_external_user);++returnvfio_group;+}++staticboolmshv_vfio_external_group_match_file(structvfio_group*group,+structfile*filep)+{+boolret,(*fn)(structvfio_group*,structfile*);++fn=symbol_get(vfio_external_group_match_file);+if(!fn)+returnfalse;++ret=fn(group,filep);++symbol_put(vfio_external_group_match_file);++returnret;+}++staticvoidmshv_vfio_group_put_external_user(structvfio_group*vfio_group)+{+void(*fn)(structvfio_group*);++fn=symbol_get(vfio_group_put_external_user);+if(!fn)+return;++fn(vfio_group);++symbol_put(vfio_group_put_external_user);+}++staticintmshv_vfio_set_group(structmshv_device*dev,longattr,u64arg)+{+structmshv_vfio*mv=dev->private;+structvfio_group*vfio_group;+structmshv_vfio_group*mvg;+int32_t__user*argp=(int32_t__user*)(unsignedlong)arg;+structfdf;+int32_tfd;+intret;++switch(attr){+caseMSHV_DEV_VFIO_GROUP_ADD:+if(get_user(fd,argp))+return-EFAULT;++f=fdget(fd);+if(!f.file)+return-EBADF;++vfio_group=mshv_vfio_group_get_external_user(f.file);+fdput(f);++if(IS_ERR(vfio_group))+returnPTR_ERR(vfio_group);++mutex_lock(&mv->lock);++list_for_each_entry(mvg,&mv->group_list,node){+if(mvg->vfio_group==vfio_group){+mutex_unlock(&mv->lock);+mshv_vfio_group_put_external_user(vfio_group);+return-EEXIST;+}+}++mvg=kzalloc(sizeof(*mvg),GFP_KERNEL_ACCOUNT);+if(!mvg){+mutex_unlock(&mv->lock);+mshv_vfio_group_put_external_user(vfio_group);+return-ENOMEM;+}++list_add_tail(&mvg->node,&mv->group_list);+mvg->vfio_group=vfio_group;++mutex_unlock(&mv->lock);++return0;++caseMSHV_DEV_VFIO_GROUP_DEL:+if(get_user(fd,argp))+return-EFAULT;++f=fdget(fd);+if(!f.file)+return-EBADF;++ret=-ENOENT;++mutex_lock(&mv->lock);++list_for_each_entry(mvg,&mv->group_list,node){+if(!mshv_vfio_external_group_match_file(mvg->vfio_group,+f.file))+continue;++list_del(&mvg->node);+mshv_vfio_group_put_external_user(mvg->vfio_group);+kfree(mvg);+ret=0;+break;+}++mutex_unlock(&mv->lock);++fdput(f);++returnret;+}++return-ENXIO;+}++staticintmshv_vfio_set_attr(structmshv_device*dev,+structmshv_device_attr*attr)+{+switch(attr->group){+caseMSHV_DEV_VFIO_GROUP:+returnmshv_vfio_set_group(dev,attr->attr,attr->addr);+}++return-ENXIO;+}++staticintmshv_vfio_has_attr(structmshv_device*dev,+structmshv_device_attr*attr)+{+switch(attr->group){+caseMSHV_DEV_VFIO_GROUP:+switch(attr->attr){+caseMSHV_DEV_VFIO_GROUP_ADD:+caseMSHV_DEV_VFIO_GROUP_DEL:+return0;+}++break;+}++return-ENXIO;+}++staticvoidmshv_vfio_destroy(structmshv_device*dev)+{+structmshv_vfio*mv=dev->private;+structmshv_vfio_group*mvg,*tmp;++list_for_each_entry_safe(mvg,tmp,&mv->group_list,node){+mshv_vfio_group_put_external_user(mvg->vfio_group);+list_del(&mvg->node);+kfree(mvg);+}++kfree(mv);+kfree(dev);+}++staticintmshv_vfio_create(structmshv_device*dev,u32type);++staticstructmshv_device_opsmshv_vfio_ops={+.name="mshv-vfio",+.create=mshv_vfio_create,+.destroy=mshv_vfio_destroy,+.set_attr=mshv_vfio_set_attr,+.has_attr=mshv_vfio_has_attr,+};++staticintmshv_vfio_create(structmshv_device*dev,u32type)+{+structmshv_device*tmp;+structmshv_vfio*mv;++/* Only one VFIO "device" per VM */+list_for_each_entry(tmp,&dev->partition->devices,partition_node)+if(tmp->ops==&mshv_vfio_ops)+return-EBUSY;++mv=kzalloc(sizeof(*mv),GFP_KERNEL_ACCOUNT);+if(!mv)+return-ENOMEM;++INIT_LIST_HEAD(&mv->group_list);+mutex_init(&mv->lock);++dev->private=mv;++return0;+}++intmshv_vfio_ops_init(void)+{+returnmshv_register_device_ops(&mshv_vfio_ops,MSHV_DEV_TYPE_VFIO);+}++voidmshv_vfio_ops_exit(void)+{+mshv_unregister_device_ops(MSHV_DEV_TYPE_VFIO);+}
From: Robin Murphy <robin.murphy@arm.com> Date: 2021-07-09 12:46:29
On 2021-07-09 12:43, Wei Liu wrote:
Some devices may have been claimed by the hypervisor already. One such
example is a user can assign a NIC for debugging purpose.
Ideally Linux should be able to tell retrieve that information, but
there is no way to do that yet. And designing that new mechanism is
going to take time.
Provide a command line option for skipping devices. This is a stopgap
solution, so it is intentionally undocumented. Hopefully we can retire
it in the future.
Huh? If the host is using a device, why the heck is it exposing any
knowledge of that device to the guest at all, let alone allowing the
guest to do anything that could affect its operation!?
Robin.
From: Robin Murphy <robin.murphy@arm.com> Date: 2021-07-09 12:56:54
On 2021-07-09 12:43, Wei Liu wrote:
Microsoft Hypervisor provides a set of hypercalls to manage device
domains. The root kernel should parse the DMAR so that it can program
the IOMMU (with hypercalls) correctly.
The DMAR code was designed to work with Intel IOMMU only. Add two more
parameters to make it useful to Microsoft Hypervisor. Microsoft
Hypervisor does not need the DMAR parsing code to allocate an Intel
IOMMU structure; it also wishes to always reparse the DMAR table even
after it has been parsed before.
We've recently defined the VIOT table for describing paravirtualised
IOMMUs - would it make more sense to extend that to support the
Microsoft implementation than to abuse a hardware-specific table? Am I
right in assuming said hypervisor isn't intended to only ever run on
Intel hardware?
Robin.
quoted hunk
Adjust Intel IOMMU code to use the new dmar_table_init. There should be
no functional change to Intel IOMMU code.
Signed-off-by: Wei Liu <wei.liu@kernel.org>
---
We may be able to combine alloc and force_parse?
---
drivers/iommu/intel/dmar.c | 38 ++++++++++++++++++++---------
drivers/iommu/intel/iommu.c | 2 +-
drivers/iommu/intel/irq_remapping.c | 2 +-
include/linux/dmar.h | 2 +-
4 files changed, 30 insertions(+), 14 deletions(-)
From: Wei Liu <wei.liu@kernel.org> Date: 2021-07-09 13:35:02
On Fri, Jul 09, 2021 at 01:46:19PM +0100, Robin Murphy wrote:
On 2021-07-09 12:43, Wei Liu wrote:
quoted
Some devices may have been claimed by the hypervisor already. One such
example is a user can assign a NIC for debugging purpose.
Ideally Linux should be able to tell retrieve that information, but
there is no way to do that yet. And designing that new mechanism is
going to take time.
Provide a command line option for skipping devices. This is a stopgap
solution, so it is intentionally undocumented. Hopefully we can retire
it in the future.
Huh? If the host is using a device, why the heck is it exposing any
knowledge of that device to the guest at all, let alone allowing the guest
to do anything that could affect its operation!?
The host in this setup consists of the hypervisor, the root kernel and a
bunch of user space programs.
Root is not an ordinary guest. It does need to know all the hardware to
manage the platform. Hypervisor does not claim more devices than it
needs to, nor does it try to hide hardware details from the root.
The hypervisor can protect itself just fine. Any attempt to use the
already claimed devices will be blocked or rejected, so are the attempts
to attach them to device domains.
That, however, leads to some interesting interactions between the
hypervisor and Linux kernel. When kernel initializes IOMMU during boot,
it will try to attach all devices in one go. Any failure there will
cause kernel to detach the already attached devices. That's not fatal to
kernel, and is only a minor annoyance to our current use case, because
the default domain is a passthrough domain anyway. It will become
problematic once we switch the default domain to a DMA domain to further
tighten security during Linux boot.
Wei.
From: Wei Liu <wei.liu@kernel.org> Date: 2021-07-09 13:43:02
On Fri, Jul 09, 2021 at 01:56:46PM +0100, Robin Murphy wrote:
On 2021-07-09 12:43, Wei Liu wrote:
quoted
Microsoft Hypervisor provides a set of hypercalls to manage device
domains. The root kernel should parse the DMAR so that it can program
the IOMMU (with hypercalls) correctly.
The DMAR code was designed to work with Intel IOMMU only. Add two more
parameters to make it useful to Microsoft Hypervisor. Microsoft
Hypervisor does not need the DMAR parsing code to allocate an Intel
IOMMU structure; it also wishes to always reparse the DMAR table even
after it has been parsed before.
We've recently defined the VIOT table for describing paravirtualised IOMMUs
- would it make more sense to extend that to support the Microsoft
implementation than to abuse a hardware-specific table? Am I right in
I searched for VIOT and believed I found the correct link
https://lwn.net/Articles/859291/. My understanding is based on the
reading of that series.
VIOT is useful. I think it solves the problem for guests.
It does not solve the problem we have though. The DMAR tables are not
conjured up by some backend software running on the host side. They are
the real tables provided by the firmware. The kernel here is part of the
host setup, dealing with physical hardware.
No matter how much I wish all vendors unified their tables, I don't see
how that's going to happen for readily available servers. :-(
assuming said hypervisor isn't intended to only ever run on Intel hardware?
Yes, that's correct. We also plan to add support AMD and ARM64.
Wei.
From: Lu Baolu <baolu.lu@linux.intel.com> Date: 2021-07-09 14:17:37
On 2021/7/9 19:43, Wei Liu wrote:
When Microsoft Hypervisor runs on Intel platforms it needs to know the
reserved regions to program devices correctly. There is no reason to
duplicate intel_iommu_get_resv_regions. Export it.
Why not using iommu_get_resv_regions()?
Best regards,
baolu
From: Wei Liu <wei.liu@kernel.org> Date: 2021-07-09 14:21:20
On Fri, Jul 09, 2021 at 10:17:25PM +0800, Lu Baolu wrote:
On 2021/7/9 19:43, Wei Liu wrote:
quoted
When Microsoft Hypervisor runs on Intel platforms it needs to know the
reserved regions to program devices correctly. There is no reason to
duplicate intel_iommu_get_resv_regions. Export it.
Why not using iommu_get_resv_regions()?
That calls into ops->get_resv_regions.
In this patch series, get_resv_regions is hv_iommu_resv_regions, which
wants to use intel_iommu_get_resv_regions when it detects the underlying
hardware platform is from Intel.
Wei.
From: Wei Liu <wei.liu@kernel.org> Date: 2021-07-09 19:14:14
specifically On Fri, Jul 09, 2021 at 05:38:24PM +0100, Matthew Wilcox wrote:
On Fri, Jul 09, 2021 at 04:27:32PM +0000, Wei Liu wrote:
quoted
quoted
Then don't define your own structure. Use theirs.
I specifically mentioned in the cover letter I didn't do it because I
was not sure if that would be acceptable. I guess I will find out.
I only got patch 7/8. You can't blame me for not reading 0/8 if you
didn't send me 0/8.
To be clear I did not blame you.
You were not CC'ed on this patch, so presumably you got it via one of
the mailing lists. I'm not sure why you only got this one patch. Perhaps
if you wait a bit you will get the rest.
But it is all good. I appreciate the fact that you took the time to go
over this patch and voiced your opinion.
Wei.
From: Matthew Wilcox <willy@infradead.org> Date: 2021-07-09 19:48:50
On Fri, Jul 09, 2021 at 07:14:05PM +0000, Wei Liu wrote:
You were not CC'ed on this patch, so presumably you got it via one of
the mailing lists. I'm not sure why you only got this one patch. Perhaps
if you wait a bit you will get the rest.
No, I won't. You only cc'd linux-doc on this one patch and not on any
of the others.
From: Wei Liu <wei.liu@kernel.org> Date: 2021-07-09 20:11:11
On Fri, Jul 09, 2021 at 08:48:36PM +0100, Matthew Wilcox wrote:
On Fri, Jul 09, 2021 at 07:14:05PM +0000, Wei Liu wrote:
quoted
You were not CC'ed on this patch, so presumably you got it via one of
the mailing lists. I'm not sure why you only got this one patch. Perhaps
if you wait a bit you will get the rest.
No, I won't. You only cc'd linux-doc on this one patch and not on any
of the others.
Some devices may have been claimed by the hypervisor already. One such
example is a user can assign a NIC for debugging purpose.
Ideally Linux should be able to tell retrieve that information, but
there is no way to do that yet. And designing that new mechanism is
going to take time.
Provide a command line option for skipping devices. This is a stopgap
solution, so it is intentionally undocumented. Hopefully we can retire
it in the future.
Signed-off-by: Wei Liu <wei.liu@kernel.org>
---
drivers/iommu/hyperv-iommu.c | 45 ++++++++++++++++++++++++++++++++++++
1 file changed, 45 insertions(+)
Is there a possibility of pci_devs_to_skip + pos > sizeof(pci_devs_to_skip) and also a valid memory ?
I would recommend to have a check of size as well before accessing the array content, just to be safer accessing any memory.
This is basically the same code adopted from KVM. The main user case is
the future MSHV-VFIO bridge device. We don't have any plan to support
in-kernel device emulation yet, but it wouldn't hurt to make the code
more flexible.
Signed-off-by: Wei Liu <wei.liu@kernel.org>
---
Documentation/virt/mshv/api.rst | 12 +++
drivers/hv/mshv_main.c | 181 ++++++++++++++++++++++++++++++++
include/linux/mshv.h | 57 ++++++++++
include/uapi/linux/mshv.h | 36 +++++++
4 files changed, 286 insertions(+)
@@ -170,4 +170,16 @@ Can be used to get/set various properties of a partition. Some properties can only be set at partition creation. For these, there are parameters in MSHV_CREATE_PARTITION.+3.12 MSHV_CREATE_DEVICE+-----------------------+:Type: partition ioctl+:Parameters: struct mshv_create_device+:Returns: 0 on success++Can be used to create an in-kernel device.++If the MSHV_CREATE_DEVICE_TEST flag is set, only test whether the+device type is supported (not necessarily whether it can be created+in the current vm).+Currently only supports VFIO type device.
Have seen some static analyzer tool cribbing here of not returning any error.
If you feel OK, please move the 'return -ENOTTY' down after switch block. Thanks.
Is it OK to deallocate the resources, if hypercall has failed ?
It should be fine. We leak some resources in the hypervisor, but Linux
is in a rather wedged state anyway. Refusing to free up resources in
Linux does not much good.
Do we have any specific error code EBUSY (kind of) which we need to wait upon ?
From: Wei Liu <wei.liu@kernel.org> Date: 2021-08-03 21:56:24
On Wed, Aug 04, 2021 at 12:20:42AM +0530, Praveen Kumar wrote:
On 09-07-2021 17:13, Wei Liu wrote:
quoted
Some devices may have been claimed by the hypervisor already. One such
example is a user can assign a NIC for debugging purpose.
Ideally Linux should be able to tell retrieve that information, but
there is no way to do that yet. And designing that new mechanism is
going to take time.
Provide a command line option for skipping devices. This is a stopgap
solution, so it is intentionally undocumented. Hopefully we can retire
it in the future.
Signed-off-by: Wei Liu <wei.liu@kernel.org>
---
drivers/iommu/hyperv-iommu.c | 45 ++++++++++++++++++++++++++++++++++++
1 file changed, 45 insertions(+)
Is there a possibility of pci_devs_to_skip + pos > sizeof(pci_devs_to_skip)
and also a valid memory ?
pos should point to the last parsed position. If parsing fails pos does
not get updated and the code breaks out of the loop. If parsing is
success pos should point to either the start of next element of '\0'
(end of string). To me this is good enough.
I would recommend to have a check of size as well before accessing the
array content, just to be safer accessing any memory.
From: Wei Liu <wei.liu@kernel.org> Date: 2021-08-03 22:04:26
On Wed, Aug 04, 2021 at 12:42:22AM +0530, Praveen Kumar wrote:
On 09-07-2021 17:13, Wei Liu wrote:
[...]
quoted
+static long mshv_device_ioctl(struct file *filp, unsigned int ioctl,
+ unsigned long arg)
+{
+ struct mshv_device *dev = filp->private_data;
+
+ switch (ioctl) {
+ case MSHV_SET_DEVICE_ATTR:
+ return mshv_device_ioctl_attr(dev, dev->ops->set_attr, arg);
+ case MSHV_GET_DEVICE_ATTR:
+ return mshv_device_ioctl_attr(dev, dev->ops->get_attr, arg);
+ case MSHV_HAS_DEVICE_ATTR:
+ return mshv_device_ioctl_attr(dev, dev->ops->has_attr, arg);
+ default:
+ if (dev->ops->ioctl)
+ return dev->ops->ioctl(dev, ioctl, arg);
+
+ return -ENOTTY;
+ }
Have seen some static analyzer tool cribbing here of not returning any error.
If you feel OK, please move the 'return -ENOTTY' down after switch block. Thanks.
Fair point. I will make the change.
quoted
+}
+
[...]
quoted
+static long
+mshv_partition_ioctl_create_device(struct mshv_partition *partition,
+ void __user *user_args)
+{
+ long r;
+ struct mshv_create_device tmp, *cd;
+ struct mshv_device *dev;
+ const struct mshv_device_ops *ops;
+ int type;
+
+ if (copy_from_user(&tmp, user_args, sizeof(tmp))) {
+ r = -EFAULT;
+ goto out;
+ }
+
+ cd = &tmp;
+
+ if (cd->type >= ARRAY_SIZE(mshv_device_ops_table)) {
+ r = -ENODEV;
+ goto out;
+ }
+
+ type = array_index_nospec(cd->type, ARRAY_SIZE(mshv_device_ops_table));
+ ops = mshv_device_ops_table[type];
+ if (ops == NULL) {
+ r = -ENODEV;
+ goto out;
+ }
+
+ if (cd->flags & MSHV_CREATE_DEVICE_TEST) {
+ r = 0;
+ goto out;
+ }
+
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL_ACCOUNT);
+ if (!dev) {
+ r = -ENOMEM;
+ goto out;
+ }
+
+ dev->ops = ops;
+ dev->partition = partition;
+
+ r = ops->create(dev, type);
+ if (r < 0) {
+ kfree(dev);
+ goto out;
+ }
+
+ list_add(&dev->partition_node, &partition->devices);
+
+ if (ops->init)
+ ops->init(dev);
+
+ mshv_partition_get(partition);
+ r = anon_inode_getfd(ops->name, &mshv_device_fops, dev, O_RDWR | O_CLOEXEC);
+ if (r < 0) {
+ mshv_partition_put_no_destroy(partition);
+ list_del(&dev->partition_node);
+ ops->destroy(dev);
I hope ops->destroy will free dev as well ?
Yes. It is clearly written in the preceding comment of that hook. I hope
that's prominent enough.
quoted
+ goto out;
+ }
+
+ cd->fd = r;
+ r = 0;
+
+ if (copy_to_user(user_args, &tmp, sizeof(tmp))) {
+ r = -EFAULT;
I don't think we will be cleaning up anything ? Or do we need to?
No need. Whatever residuals left will be cleaned up once the VM is
destroyed.
Wei.
Is there a case where unmapped > 0 && unmapped < size ?
There could be such a case -- hv_iommu_del_mappings' return value is >= 0.
Is there a problem with this predicate?
What I understand, if we are unmapping and return 0, means nothing was unmapped, and will that not cause any corruption or illegal access of unmapped memory later?
From __iommu_unmap
...
13 while (unmapped < size) {
12 size_t pgsize = iommu_pgsize(domain, iova, size - unmapped);
11
10 unmapped_page = ops->unmap(domain, iova, pgsize, iotlb_gather);
9 if (!unmapped_page)
8 break; <<< we just break here, thinking there is nothing unmapped, but actually hv_iommu_del_mappings has removed some pages.
7
6 pr_debug("unmapped: iova 0x%lx size 0x%zx\n",
5 ¦iova, unmapped_page);
4
3 iova += unmapped_page;
2 unmapped += unmapped_page;
1 }
...
Am I missing something ?
Regards,
~Praveen.
@@ -774,6 +784,41 @@ static struct iommu_device *hv_iommu_probe_device(struct device *dev) if (!dev_is_pci(dev)) return ERR_PTR(-ENODEV);+ /*+ * Skip the PCI device specified in `pci_devs_to_skip`. This is a+ * temporary solution until we figure out a way to extract information+ * from the hypervisor what devices it is already using.+ */+ if (pci_devs_to_skip && *pci_devs_to_skip) {+ int pos = 0;+ int parsed;+ int segment, bus, slot, func;+ struct pci_dev *pdev = to_pci_dev(dev);++ do {+ parsed = 0;++ sscanf(pci_devs_to_skip + pos,+ " (%x:%x:%x.%x) %n",+ &segment, &bus, &slot, &func, &parsed);++ if (parsed <= 0)+ break;++ if (pci_domain_nr(pdev->bus) == segment &&+ pdev->bus->number == bus &&+ PCI_SLOT(pdev->devfn) == slot &&+ PCI_FUNC(pdev->devfn) == func)+ {+ dev_info(dev, "skipped by MSHV IOMMU\n");+ return ERR_PTR(-ENODEV);+ }++ pos += parsed;++ } while (pci_devs_to_skip[pos]);
Is there a possibility of pci_devs_to_skip + pos > sizeof(pci_devs_to_skip)
and also a valid memory ?
pos should point to the last parsed position. If parsing fails pos does
not get updated and the code breaks out of the loop. If parsing is
success pos should point to either the start of next element of '\0'
(end of string). To me this is good enough.
The point is, hypothetically the address to pci_devs_to_skip + pos can be valid address (later to '\0'), and thus there is a possibility, that parsing may not fail.
Another, there is also a possibility of sscanf faulting accessing the illegal address, if pci_devs_to_skip[pos] turns out to be not NULL or valid address.
quoted
I would recommend to have a check of size as well before accessing the
array content, just to be safer accessing any memory.
What check do you have in mind?
Something like,
size_t len = strlen(pci_devs_to_skip);
do {
len -= parsed;
} while (len);
OR
do {
...
pos += parsed;
} while (pos < len);
Further, I'm also fine with the existing code, if you think this won't break and already been taken care. Thanks.
Regards,
~Praveen.
From: Wei Liu <wei.liu@kernel.org> Date: 2021-08-10 10:04:27
On Wed, Aug 04, 2021 at 12:33:54PM +0530, Praveen Kumar wrote:
On 04-08-2021 03:26, Wei Liu wrote:
quoted
quoted
quoted
struct iommu_domain domain;
@@ -774,6 +784,41 @@ static struct iommu_device *hv_iommu_probe_device(struct device *dev) if (!dev_is_pci(dev)) return ERR_PTR(-ENODEV);+ /*+ * Skip the PCI device specified in `pci_devs_to_skip`. This is a+ * temporary solution until we figure out a way to extract information+ * from the hypervisor what devices it is already using.+ */+ if (pci_devs_to_skip && *pci_devs_to_skip) {+ int pos = 0;+ int parsed;+ int segment, bus, slot, func;+ struct pci_dev *pdev = to_pci_dev(dev);++ do {+ parsed = 0;++ sscanf(pci_devs_to_skip + pos,+ " (%x:%x:%x.%x) %n",+ &segment, &bus, &slot, &func, &parsed);++ if (parsed <= 0)+ break;++ if (pci_domain_nr(pdev->bus) == segment &&+ pdev->bus->number == bus &&+ PCI_SLOT(pdev->devfn) == slot &&+ PCI_FUNC(pdev->devfn) == func)+ {+ dev_info(dev, "skipped by MSHV IOMMU\n");+ return ERR_PTR(-ENODEV);+ }++ pos += parsed;++ } while (pci_devs_to_skip[pos]);
Is there a possibility of pci_devs_to_skip + pos > sizeof(pci_devs_to_skip)
and also a valid memory ?
pos should point to the last parsed position. If parsing fails pos does
not get updated and the code breaks out of the loop. If parsing is
success pos should point to either the start of next element of '\0'
(end of string). To me this is good enough.
The point is, hypothetically the address to pci_devs_to_skip + pos can
be valid address (later to '\0'), and thus there is a possibility,
that parsing may not fail.
Have you found an example how at any given point in time
pci_devs_to_skip + pos can point outside of user provided string?
Another, there is also a possibility of sscanf faulting accessing the
illegal address, if pci_devs_to_skip[pos] turns out to be not NULL or
valid address.
That depends on pci_devs_to_skip + pos can point to an invalid address
in the first place, so that goes back to the question above.
quoted
quoted
I would recommend to have a check of size as well before accessing the
array content, just to be safer accessing any memory.
What check do you have in mind?
Something like,
size_t len = strlen(pci_devs_to_skip);
do {
len -= parsed;
} while (len);
OR
do {
...
pos += parsed;
} while (pos < len);
Further, I'm also fine with the existing code, if you think this won't
break and already been taken care. Thanks.
But in the loop somewhere you will still need to parse pci_devs_to_skip
+ some_offset. The new code structure does not remove that, right?
Given this is for debugging and is supposed to be temporary, I think the
code is good enough. But I want to make sure if there is anything I
missed.
Wei.
Is there a case where unmapped > 0 && unmapped < size ?
There could be such a case -- hv_iommu_del_mappings' return value is >= 0.
Is there a problem with this predicate?
What I understand, if we are unmapping and return 0, means nothing was
unmapped, and will that not cause any corruption or illegal access of
unmapped memory later? From __iommu_unmap
Those pages are not really unmapped. The hypercall is skipped.
...
13 while (unmapped < size) {
12 size_t pgsize = iommu_pgsize(domain, iova, size - unmapped);
11
10 unmapped_page = ops->unmap(domain, iova, pgsize, iotlb_gather);
9 if (!unmapped_page)
8 break; <<< we just break here, thinking there is nothing unmapped, but actually hv_iommu_del_mappings has removed some pages.
7
6 pr_debug("unmapped: iova 0x%lx size 0x%zx\n",
5 ¦iova, unmapped_page);
4
3 iova += unmapped_page;
2 unmapped += unmapped_page;
1 }
...
Am I missing something ?
Regards,
~Praveen.
Can we move these both checks above switch statement and do fdput
accordingly under both case statement accordingly?
Fair point. This can be done, albeit at the cost of having a rather
different code structure.
I was waiting to see if we should somehow merge this with KVM's
implementation so the code was deliberately kept close. If there is no
further comment I can of course make the change you suggested.