linux-kernel - Re: [PART2 PATCH v5 12/12] svm: Implements update_pi

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:	Tue, 16 Aug 2016 18:33:29 +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 v5 12/12] svm: Implements update_pi_irte hook to
 setup posted interrupt

2016-08-16 22:19+0700, Suravee Suthikulpanit:
> On 8/13/16 19:03, Radim Krčmář wrote:
>> 2016-07-25 04:32-0500, Suravee Suthikulpanit:
>> > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> > @@ -1485,9 +1521,16 @@ static void avic_set_running(struct kvm_vcpu *vcpu, bool is_run)
>> > +static void svm_pi_list_add(struct vcpu_svm *svm, struct amd_iommu_pi_data *pi)
>> > +{
>> > +	bool found = false;
>> > +	unsigned long flags;
>> > +	struct amd_iommu_pi_data *cur;
>> > +
>> > +	spin_lock_irqsave(&svm->pi_list_lock, flags);
>> > +	list_for_each_entry(cur, &svm->pi_list, node) {
>> > +		if (cur->ir_data != pi->ir_data)
>> > +			continue;
>> > +		found = true;
>> 
>> This optimization turned out to be ugly ... sorry.
> 
> That's okay. It makes sense to avoid using the hash table if we can.
> 
>> Manipulation with pi_list is hard to understand, IMO, so a comment
>> explaining why we couldn't do that without traversing a list and
>> comparing pi->ir_data would be nice.
> 
> I'll add more comment here.

Thanks.

>> Maybe I was a bit confused by reusing amd_iommu_pi_data when all we care
>> about is a list of cur->ir_data -- can't we have a list of just ir_data?
> 
> Actually, in SVM, we care about posted-interrupt information, which is
> generated from the SVM side, and stored in the amd_iommu_pi_data. This is
> also communicated to IOMMU via the irq_set_vcpu_affinity().
> 
> Here, I only use ir_data to differentiate amd_iommu_pi_data.

I'm still confused then. :)

struct amd_iommu_pi_data is a throwaway structure for I/O with
irq_set_vcpu_affinity(), but we keep it afterwards ...
is it more than a wrapper that allows us to put ir_data into a list,
because we don't want to add list_head directly to ir_data?