Mark McLoughlin wrote:
> On Tue, 2008-12-02 at 22:22 +0800, Han, Weidong wrote:
>
>> Separate add/remove domain device info functions for virtual machine
>> VT-d from natvie VT-d.
>>
>> Signed-off-by: Weidong Han <[EMAIL PROTECTED]>
>> ---
>> drivers/pci/intel-iommu.c | 164
>> +++++++++++++++++++++++++++++++++++++++-
>> include/linux/dma_remapping.h | 1 + 2 files changed, 160
>> insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
>> index 09a5150..429aff4 100644
>> --- a/drivers/pci/intel-iommu.c
>> +++ b/drivers/pci/intel-iommu.c
>> @@ -200,6 +200,27 @@ static struct intel_iommu
>> *domain_get_iommu(struct dmar_domain *domain) return NULL; }
>>
>> +static struct intel_iommu *device_find_matched_iommu(u8 bus, u8
>> devfn)
>
> That's quite an unwieldy name, how about device_to_iommu() ?
will rename it.
>
>> +{
>> + struct dmar_drhd_unit *drhd = NULL;
>> + int i;
>> +
>> + for_each_drhd_unit(drhd) {
>> + if (drhd->ignored)
>> + continue;
>> +
>> + for (i = 0; i < drhd->devices_cnt; i++)
>> + if (drhd->devices[i]->bus->number == bus &&
>> + drhd->devices[i]->devfn == devfn)
>> + return drhd->iommu;
>> +
>> + if (drhd->include_all)
>> + return drhd->iommu;
>> + }
>> +
>> + return NULL;
>> +}
> ...
>> @@ -1269,9 +1292,12 @@ domain_page_mapping(struct dmar_domain
>> *domain, dma_addr_t iova, return 0; }
>>
>> -static void detach_domain_for_dev(struct dmar_domain *domain, u8
>> bus, u8 devfn) +static void iommu_detach_dev(u8 bus, u8 devfn)
>
> Would be nicer if this function took a struct intel_iommu pointer
> rather than bus/devfn.
intel_iommu can be got by bus and devfn, so I didn't add the parameter. But
your suggestion is reasonable.
>
>> {
>> - struct intel_iommu *iommu = domain_get_iommu(domain);
>> + struct intel_iommu *iommu = device_find_matched_iommu(bus, devfn);
>> + + if (!iommu)
>> + return;
>>
>> clear_context_table(iommu, bus, devfn);
>> iommu->flush.flush_context(iommu, 0, 0, 0, ...
>> +/* "Coherency" capability may be different across iommus */
>> +static void domain_update_iommu_coherency(struct dmar_domain
>> *domain) +{ + struct dmar_drhd_unit *drhd;
>> +
>> + domain->iommu_coherency = 1;
>> +
>> + for_each_drhd_unit(drhd) {
>> + if (drhd->ignored)
>> + continue;
>> + if (test_bit(drhd->iommu->seq_id, &domain->iommu_bmp)) {
>> + if (!ecap_coherent(drhd->iommu->ecap)) {
>> + domain->iommu_coherency = 0;
>> + break;
>> + }
>> + }
>> + }
>> +}
>
> As I said, this belongs in the patch where you added the
> iommu_coherency
> flag.
>
>> +
>> +static int vm_domain_add_dev_info(struct dmar_domain *domain, +
>>
>> struct pci_dev *pdev) +{
>> + struct device_domain_info *info;
>> + unsigned long flags;
>> +
>> + info = alloc_devinfo_mem();
>> + if (!info)
>> + return -ENOMEM;
>> +
>> + info->bus = pdev->bus->number;
>> + info->devfn = pdev->devfn;
>> + info->dev = pdev;
>> + info->domain = domain;
>> +
>> + spin_lock_irqsave(&device_domain_lock, flags);
>> + list_add(&info->link, &domain->devices);
>> + list_add(&info->global, &device_domain_list);
>> + pdev->dev.archdata.iommu = info;
>> + spin_unlock_irqrestore(&device_domain_lock, flags); +
>> + return 0;
>> +}
>> +
>> +static void vm_domain_remove_one_dev_info(struct dmar_domain
>> *domain, + struct pci_dev *pdev) +{
>> + struct device_domain_info *info;
>> + struct intel_iommu *iommu;
>> + unsigned long flags;
>> + int found = 0;
>> +
>> + iommu = device_find_matched_iommu(pdev->bus->number, pdev->devfn);
>> + + spin_lock_irqsave(&device_domain_lock, flags);
>> + while (!list_empty(&domain->devices)) {
>> + info = list_entry(domain->devices.next,
>> + struct device_domain_info, link);
>> + if (info->bus == pdev->bus->number &&
>> + info->devfn == pdev->devfn) {
>> + list_del(&info->link);
>> + list_del(&info->global);
>> + if (info->dev)
>> + info->dev->dev.archdata.iommu = NULL;
>> + spin_unlock_irqrestore(&device_domain_lock, flags); +
>> + iommu_detach_dev(info->bus, info->devfn);
>> + free_devinfo_mem(info);
>> +
>> + spin_lock_irqsave(&device_domain_lock, flags);
>> +
>> + if (found)
>> + break;
>> + else
>> + continue;
>> + }
>> +
>> + /* if there is no other devices under the same iommu
>> + * owned by this domain, clear this iommu in iommu_bmp +
>> */
>> + if (device_find_matched_iommu(info->bus, info->devfn) == iommu)
>> + found = 1; + }
>> +
>> + if (found == 0) {
>> + spin_lock_irqsave(&iommu->lock, flags);
>> + clear_bit(iommu->seq_id, &domain->iommu_bmp);
>> + domain->iommu_count--;
>> + domain_update_iommu_coherency(domain);
>> + spin_unlock_irqrestore(&iommu->lock, flags);
>> + }
>> +
>> + spin_unlock_irqrestore(&device_domain_lock, flags); +}
>> +
>> +static void vm_domain_remove_all_dev_info(struct dmar_domain
>> *domain) +{ + struct device_domain_info *info;
>> + struct intel_iommu *iommu;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&device_domain_lock, flags);
>> + while (!list_empty(&domain->devices)) {
>> + info = list_entry(domain->devices.next,
>> + struct device_domain_info, link);
>> + list_del(&info->link);
>> + list_del(&info->global);
>> + if (info->dev)
>> + info->dev->dev.archdata.iommu = NULL;
>> +
>> + spin_unlock_irqrestore(&device_domain_lock, flags);
>> + iommu_detach_dev(info->bus, info->devfn);
>> +
>> + /* clear this iommu in iommu_bmp */
>> + iommu = device_find_matched_iommu(info->bus, info->devfn);
>> + spin_lock_irqsave(&iommu->lock, flags);
>> + if (test_and_clear_bit(iommu->seq_id,
>> + &domain->iommu_bmp)) {
>> + domain->iommu_count--;
>> + domain_update_iommu_coherency(domain);
>> + }
>> + spin_unlock_irqrestore(&iommu->lock, flags);
>> +
>> + free_devinfo_mem(info);
>> + spin_lock_irqsave(&device_domain_lock, flags);
>> + }
>> + spin_unlock_irqrestore(&device_domain_lock, flags);
>> +}
>
> You're adding three functions here which are essentially copies of
> code
> that already exists, with some minor changes. You need to refactor the
> existing code and modify it to handle these cases.
Adding these functions is to remove domain flag judgment in many functions.
domain flag judgment seems brittle. So it's a balance. I will clean up it again.
Regards,
Weidong
>
> Cheers,
> Mark.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
