Messages in this thread | | | Subject | Re: [PATCH v5 2/7] iommu/vt-d: Duplicate iommu_resv_region objects per device list | From | Auger Eric <> | Date | Wed, 29 May 2019 17:40:51 +0200 |
| |
Hi Lu,
On 5/29/19 4:04 AM, Lu Baolu wrote: > Hi Eric, > > On 5/28/19 7:50 PM, Eric Auger wrote: >> intel_iommu_get_resv_regions() aims to return the list of >> reserved regions accessible by a given @device. However several >> devices can access the same reserved memory region and when >> building the list it is not safe to use a single iommu_resv_region >> object, whose container is the RMRR. This iommu_resv_region must >> be duplicated per device reserved region list. >> >> Let's remove the struct iommu_resv_region from the RMRR unit >> and allocate the iommu_resv_region directly in >> intel_iommu_get_resv_regions(). We hold the dmar_global_lock instead >> of the rcu-lock to allow sleeping. >> >> Fixes: 0659b8dc45a6 ("iommu/vt-d: Implement reserved region get/put >> callbacks") >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> >> --- >> >> v4 -> v5 >> - replace rcu-lock by the dmar_global_lock >> --- >> drivers/iommu/intel-iommu.c | 34 +++++++++++++++++----------------- >> 1 file changed, 17 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c >> index a209199f3af6..5ec8b5bd308f 100644 >> --- a/drivers/iommu/intel-iommu.c >> +++ b/drivers/iommu/intel-iommu.c >> @@ -322,7 +322,6 @@ struct dmar_rmrr_unit { >> u64 end_address; /* reserved end address */ >> struct dmar_dev_scope *devices; /* target devices */ >> int devices_cnt; /* target device count */ >> - struct iommu_resv_region *resv; /* reserved region handle */ >> }; >> struct dmar_atsr_unit { >> @@ -4205,7 +4204,6 @@ static inline void init_iommu_pm_ops(void) {} >> int __init dmar_parse_one_rmrr(struct acpi_dmar_header *header, void >> *arg) >> { >> struct acpi_dmar_reserved_memory *rmrr; >> - int prot = DMA_PTE_READ|DMA_PTE_WRITE; >> struct dmar_rmrr_unit *rmrru; >> size_t length; >> @@ -4219,22 +4217,16 @@ int __init dmar_parse_one_rmrr(struct >> acpi_dmar_header *header, void *arg) >> rmrru->end_address = rmrr->end_address; >> length = rmrr->end_address - rmrr->base_address + 1; >> - rmrru->resv = iommu_alloc_resv_region(rmrr->base_address, length, >> prot, >> - IOMMU_RESV_DIRECT); >> - if (!rmrru->resv) >> - goto free_rmrru; >> rmrru->devices = dmar_alloc_dev_scope((void *)(rmrr + 1), >> ((void *)rmrr) + rmrr->header.length, >> &rmrru->devices_cnt); >> if (rmrru->devices_cnt && rmrru->devices == NULL) >> - goto free_all; >> + goto free_rmrru; >> list_add(&rmrru->list, &dmar_rmrr_units); >> return 0; >> -free_all: >> - kfree(rmrru->resv); >> free_rmrru: >> kfree(rmrru); >> out: >> @@ -4452,7 +4444,6 @@ static void intel_iommu_free_dmars(void) >> list_for_each_entry_safe(rmrru, rmrr_n, &dmar_rmrr_units, list) { >> list_del(&rmrru->list); >> dmar_free_dev_scope(&rmrru->devices, &rmrru->devices_cnt); >> - kfree(rmrru->resv); >> kfree(rmrru); >> } >> @@ -5470,22 +5461,33 @@ static void intel_iommu_remove_device(struct >> device *dev) >> static void intel_iommu_get_resv_regions(struct device *device, >> struct list_head *head) >> { >> + int prot = DMA_PTE_READ|DMA_PTE_WRITE; > > I know this is moved from above. How about adding spaces around the '|'? sure > >> struct iommu_resv_region *reg; >> struct dmar_rmrr_unit *rmrr; >> struct device *i_dev; >> int i; >> - rcu_read_lock(); >> + down_write(&dmar_global_lock); > > Just out of curiosity, why not down_read()? We don't change the rmrr > list here, right? you're right, my mistake. > >> for_each_rmrr_units(rmrr) { >> for_each_active_dev_scope(rmrr->devices, rmrr->devices_cnt, >> i, i_dev) { >> + struct iommu_resv_region *resv; >> + size_t length; >> + >> if (i_dev != device) >> continue; >> - list_add_tail(&rmrr->resv->list, head); >> + length = rmrr->end_address - rmrr->base_address + 1; >> + resv = iommu_alloc_resv_region(rmrr->base_address, >> + length, prot, >> + IOMMU_RESV_DIRECT); >> + if (!resv) >> + break; >> + >> + list_add_tail(&resv->list, head); >> } >> } >> - rcu_read_unlock(); >> + up_write(&dmar_global_lock); >> reg = iommu_alloc_resv_region(IOAPIC_RANGE_START, >> IOAPIC_RANGE_END - IOAPIC_RANGE_START + 1, >> @@ -5500,10 +5502,8 @@ static void intel_iommu_put_resv_regions(struct >> device *dev, >> { >> struct iommu_resv_region *entry, *next; >> - list_for_each_entry_safe(entry, next, head, list) { >> - if (entry->type == IOMMU_RESV_MSI) >> - kfree(entry); >> - } >> + list_for_each_entry_safe(entry, next, head, list) >> + kfree(entry); >> } >> int intel_iommu_enable_pasid(struct intel_iommu *iommu, struct >> device *dev) >> > > Other looks good to me. > > Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com> Thanks!
Eric > > Best regards, > Baolu
| |