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

[Suravee Suthikulpanit]

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