| Message ID | 1468231899-6987-8-git-send-email-suravee.suthikulpanit@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2016-07-11 05:11-0500, Suravee Suthikulpanit: > From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> > > 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@amd.com> > --- > 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? > +{ > + 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.) > + 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.) > + iommu_flush_irt(iommu, ir_data->irq_2_irte.devid); > + iommu_completion_wait(iommu); > + } > + > + spin_unlock_irqrestore(&iommu->ga_hash_lock, flags); -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@amd.com> >> >> 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@amd.com> >> --- >> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c index 939ebb8..95f106a 100644 --- 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) return 0; } + +static int +update_irte_ga(struct irte_ga *irte, unsigned int devid, + u64 base, int cpu, bool is_run) +{ + struct irq_remap_table *irt = get_irq_table(devid, false); + unsigned long flags; + + if (!irt) + return -ENODEV; + + spin_lock_irqsave(&irt->lock, flags); + + if (irte->lo.fields_vapic.guest_mode) { + irte->hi.fields.ga_root_ptr = (base >> 12); + if (cpu >= 0) + irte->lo.fields_vapic.destination = cpu; + irte->lo.fields_vapic.is_run = is_run; + barrier(); + } + + spin_unlock_irqrestore(&irt->lock, flags); + + return 0; +} + +int amd_iommu_update_ga(u32 vcpu_id, u32 cpu, u32 ga_tag, + u64 base, bool is_run) +{ + 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; + + 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); + iommu_flush_irt(iommu, ir_data->irq_2_irte.devid); + iommu_completion_wait(iommu); + } + + spin_unlock_irqrestore(&iommu->ga_hash_lock, flags); + } + + return 0; +} +EXPORT_SYMBOL(amd_iommu_update_ga); #endif diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h index 926ef6e..81c5def 100644 --- a/drivers/iommu/amd_iommu_types.h +++ b/drivers/iommu/amd_iommu_types.h @@ -838,6 +838,7 @@ struct amd_ir_data { union { struct msi_msg msi_entry; }; + void *ref; /* Pointer to the actual irte */ }; #ifdef CONFIG_IRQ_REMAP diff --git a/include/linux/amd-iommu.h b/include/linux/amd-iommu.h index 8f7a469..9897da8 100644 --- a/include/linux/amd-iommu.h +++ b/include/linux/amd-iommu.h @@ -179,6 +179,9 @@ static inline int amd_iommu_detect(void) { return -ENODEV; } /* IOMMU AVIC Function */ extern int amd_iommu_register_ga_log_notifier(int (*notifier)(int, int, int)); +extern int +amd_iommu_update_ga(u32 vcpu_id, u32 cpu, u32 ga_tag, u64 base, bool is_run); + #else /* defined(CONFIG_AMD_IOMMU) && defined(CONFIG_IRQ_REMAP) */ static inline int @@ -187,6 +190,12 @@ amd_iommu_register_ga_log_notifier(int (*notifier)(int, int, int)) return 0; } +static inline int +amd_iommu_update_ga(u32 vcpu_id, u32 cpu, u32 ga_tag, u64 base, bool is_run) +{ + return 0; +} + #endif /* defined(CONFIG_AMD_IOMMU) && defined(CONFIG_IRQ_REMAP) */ #endif /* _ASM_X86_AMD_IOMMU_H */