Hi Radim,
On 7/13/16 21:14, Radim Krčmář wrote:
[I pasted v3 reviews prefixed with a pipe where I think they still apply.]
2016-07-13 08:20-0500, Suravee Suthikulpanit:
From: Suravee Suthikulpanit <Suravee.Suthikulpanit@xxxxxxx>
Introduces a new IOMMU API, amd_iommu_update_ga(), which allows
KVM (SVM) to update existing posted interrupt IOMMU IRTE when
load/unload vcpu.
Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@xxxxxxx>
---
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
@@ -4461,4 +4461,69 @@ int amd_iommu_create_irq_domain(struct amd_iommu *iommu)
+int amd_iommu_update_ga(u32 vcpu_id, u32 cpu, u32 vm_id,
+ u64 base, bool is_run)
|2016-07-13 15:49+0700, Suravee Suthikulpanit:
|> On 07/12/2016 01:59 AM, Radim Krčmář wrote:
|>> Not just in this function does the interface between svm and iommu split
|>> ga_tag into its two components (vcpu_id and ga_tag), but it seems that
|>> the combined value could always be used instead ...
|>> Is there an advantage to passing two values?
|>
|> Here, the amd_iommu_update_ga() takes the two separate value for input
|> parameters. Mainly the ga_tag (which is really the vm_id) and vcpu_id. This
|> allow IOMMU driver to decide how to encode the GATAG to be programmed into
|> the IRTE. Currently, the actual GATAG is a 16-bit value, <vm_id><vcpu_id>.
|> This keeps the interface independent from how we encode the GATAG.
I was thinking about making the IOMMU unaware how SVM or other Linux
hypervisors use the ga_tag, i.e. passing the final u32 ga_tag.
For example 32 bit hypervisor doesn't need to use lookup, because any
pointer can used as the ga_tag directly.
Ahh....... (w/ a big light bulb)
I get your point now. Let's just have SVM (or other hypervisor) define
what the tag should be and just pass-on the value to IOMMU. IOMMU can
just simply use this w/o knowing what it is. Sorry, I'm slow :)
And there are other viable algoritms for assigning the ga_tag --
> why isn't the vm_id 24 bits?
Good point! Actually, I am somehow limited to 30-bit hash value. So, the
VM_ID can be 22 bits, I'll make that change.
+ 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?
Also, since we will not be passing the vmid and vcpuid as separate
value, and just passing the (u32)ga_tag, we would not be able to do the
check you suggested here.
+
+ if (!irte->lo.fields_vapic.guest_mode)
+ continue;
+
+ update_irte_ga((struct irte_ga *)ir_data->ref,
+ ir_data->irq_2_irte.devid,
+ base, cpu, is_run);
|>> (The lookup leading up to here is avoidable -- svm, the caller, has the
|>> ability to map ga_tag into irte/ir_data directly with a pointer.
I'm not sure about this optimization to avoid look up.
The struct amd_ir_data is part of the IOMMU driver, and the SVM knows
nothing about it. I don't think it would be able to find out the pointer
to amd_ir_data/irte.
Also, with the current design, each ga_tag can be mapped to different
irte since there could be multiple interrupts targeting a particular
cpu. Here, we would want to update all of the IRTEs with the same ga_tag.
|>> I'm not sure if the lookup is slow enough to pardon optimization, but
|>> it might make the code simpler as well.)
|>
|> I might have mislead you up to this point. Not sure if the assumption here
|> still hold with my explanation above. Sorry for confusion.
SVM configures IOMMU with ga_tag, so IOMMU could return the pointer to
ir_data/irte that was just configured.
Also, IIUC, you want to use the pointer to ir_data/irte as the ga_tag
value. The issue would be ga_tag is a 32-bit value, and this would not
work with 64-bit address.
SVM would couple it with a VCPU
(and hence a ga_tag) and when amd_iommu_update_ga() was needed, SVM
would pass the ir_data/irte pointer directly, instead of looking it up
though a ga_tag.
Please let me know if I am still missing any points.
Thanks,
Suravee
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
