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

[Suravee Suthikulpanit]

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