Hi Radim,
I have a feeling that there might be some confusion in the use of
parameter ga_tag here in various places. My apology. I am in the process
of cleaning up this, and will send out the V4.
In the meantime, let me try to clarify a couple design detail that might
be missed here.
On 07/12/2016 01:59 AM, Radim Krčmář wrote:
2016-07-11 05:11-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
@@ -4481,4 +4481,67 @@ int amd_iommu_create_irq_domain(struct amd_iommu *iommu)
+int amd_iommu_update_ga(u32 vcpu_id, u32 cpu, u32 ga_tag,
+ u64 base, bool is_run)
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?
Basically, the amd_iommu_update_ga() function is designed to achieve two
things:
1. Communicate from SVM to AMD IOMMU driver the interrupt routing
information (e.g. the physical CPU to route the guest interrupt to) in
case the vcpu is running.
2. In case of vcpu is not running, the IOMMU driver should decode the
GATAG field of the GA log entry to find out which VCPU of which VM need
to be scheduled in, and notify KVM/SVM. The GATAG is encode as ((VM_ID
<< 8) | VCPU_ID).
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.
+{
+ 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->ga_hash_lock, flags);
+
+ /* Note: Update all possible ir_data for a particular
+ * vcpu in a particular vm.
+ */
+ hash_for_each_possible(iommu->ga_hash, ir_data, hnode,
+ AMD_IOMMU_GATAG(ga_tag, 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).
+ 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 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.
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
