Re: [PART2 PATCH v4 07/11] iommu/amd: Introduce amd_iommu_update_ga()

From: Suravee Suthikulpanit
Date: Thu Jul 14 2016 - 05:33:44 EST




On 7/14/16 16:13, Suravee Suthikulpanit wrote:
unsigned long flags;
+ struct amd_iommu *iommu;
+
+ if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir))
+ return 0;
+
+ for_each_iommu(iommu) {
+ struct amd_ir_data *ir_data;
+
+ spin_lock_irqsave(&iommu->gatag_ir_hash_lock, flags);
+
+ /* Note:
+ * We need to update all interrupt remapping table entries
+ * for targeting the specified vcpu. Here, we use gatag
+ * as a hash key and iterate through all entries in the bucket.
+ */
+ hash_for_each_possible(iommu->gatag_ir_hash, ir_data, hnode,
+ AMD_IOMMU_GATAG(vm_id, vcpu_id)) {
+ struct irte_ga *irte = (struct irte_ga *) ir_data->entry;

|>> (The ga_tag check is missing here too.)
|>
|> Here, the intention is to update all interrupt remapping entries in
the
|> bucket w/ the same GATAG (i.e. vm_id + vcpu_id), where GATAG =
|> AMD_IOMMU_GATAG(vm_id, vcpu_id).

Which is why you need to check that
AMD_IOMMU_GATAG(vm_id, vcpu_id) == entry->fields_vapic.ga_tag

The hashing function can map two different vm_id + vcpu_id to the same
bucket and hash_for_each_possible() would return both of them, but only
one belongs to the VCPU that we want to update.

(And shouldn't there be only one match?)

Actually, with your suggestion above, the hask key would be (vm_id &
0x3FFFFF << 8)| (vcpu_id & 0xFF). So, it should be unique for each vcpu
of each vm, or am I still missing something?

Ok, one scenario would be when SVM run out of the VM_ID and having to start re-using them. Since we want SVM to generate ga_tag and just pass into IOMMU driver for it to program the IRTE, we probably can make an assumption that SVM would make sure that ga_tag would not conflict for each vm_id/vcpu_id.

Thanks,
Suravee

Thanks,
Suravee