Re: [PART2 PATCH v5 12/12] svm: Implements update_pi_irte hook to setup posted interrupt

[Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>]

Hi Radim,


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)
>>  	WARN_ON(is_run == !!(entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK));
>>
>>  	entry &= ~AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK;
>> -	if (is_run)
>> +	if (is_run) {
>>  		entry |= AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK;
>> -	WRITE_ONCE(*(svm->avic_physical_id_cache), entry);
>> +		WRITE_ONCE(*(svm->avic_physical_id_cache), entry);
>> +		avic_update_iommu(vcpu, h_physical_id,
>> +				  page_to_phys(svm->avic_backing_page), 1);
>> +	} else {
>> +		avic_update_iommu(vcpu, h_physical_id,
>> +				  page_to_phys(svm->avic_backing_page), 0);
>> +		WRITE_ONCE(*(svm->avic_physical_id_cache), entry);
>> +	}
>
> You need to do the same change twice ... I guess it is time to factor
> the code. :)
>
> Wouldn't the following be an improvement in the !is_run path too?
>
>   static void avic_set_running(struct kvm_vcpu *vcpu, bool is_run)
>   {
>   	svm->avic_is_running = is_run;
>
>   	if (is_run)
>   		avic_vcpu_load(vcpu, vcpu->cpu);
>   	else
>   		avic_vcpu_put(vcpu);
>   }
>

I like this change. Thanks.

>> +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.

> 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.

>> [....]
>> +
>> +			/* Try to enable guest_mode in IRTE */
>> +			pi_data->ga_tag = AVIC_GATAG(kvm->arch.avic_vm_id,
>> +						     vcpu->vcpu_id);
>> +			pi_data->vcpu_data = &vcpu_info;
>> +			pi_data->is_guest_mode = true;
>> +			ret = irq_set_vcpu_affinity(host_irq, pi_data);
>> +
>> +			/**
>> +			 * We save the pointer to pi_data in the struct
>> +			 * vcpu_svm so that we can reference to them directly
>> +			 * when we update vcpu scheduling information in IOMMU
>> +			 * irte.
>> +			 */
>> +			if (!ret && pi_data->is_guest_mode)
>> +				svm_pi_list_add(svm, pi_data);
>
> pi_data leaks in the else case.
>
> (Allocating the element in svm_pi_list_add() would solve this.)

Ahh .. good catch.

Thanks,
Suravee