| Message ID | 1468416032-7692-8-git-send-email-suravee.suthikulpanit@amd.com |
|---|---|
| State | New |
| Headers | show |
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@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 >> @@ -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
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
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c index fe9b005..4a337dc 100644 --- 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) 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 vm_id, + 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->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; + + 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->gatag_ir_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 2ed353b..52160c8 100644 --- a/drivers/iommu/amd_iommu_types.h +++ b/drivers/iommu/amd_iommu_types.h @@ -844,6 +844,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 940fdd8..a6fc022 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)); +extern int +amd_iommu_update_ga(u32 vcpu_id, u32 cpu, u32 vm_id, 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)) return 0; } +static inline int +amd_iommu_update_ga(u32 vcpu_id, u32 cpu, u32 vm_id, u64 base, bool is_run) +{ + return 0; +} + #endif /* defined(CONFIG_AMD_IOMMU) && defined(CONFIG_IRQ_REMAP) */ #endif /* _ASM_X86_AMD_IOMMU_H */