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

Suravee Suthikulpanit
2016-08-16T15:18:47Z

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