linux-kernel - Re: [PART2 PATCH v4 07/11] iommu/amd: Introduce amd_iommu_update

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 14 Jul 2016 15:45:18 +0200
From:	Radim Krčmář <rkrcmar@...hat.com>
To:	Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>
Cc:	joro@...tes.org, pbonzini@...hat.com, alex.williamson@...hat.com,
	kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
	sherry.hurwitz@....com
Subject: Re: [PART2 PATCH v4 07/11] iommu/amd: Introduce amd_iommu_update_ga()

2016-07-14 16:33+0700, 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.

I agree, it could enable doorbell to an unscheduled VCPU and therefore
lose the notification.

The per-vcpu list of IRTEs would solve it as well, but making sure that
no two VMs have the same id might be easier and 2^22 active VMs should
be more than enough. :)