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

[Suravee Suthikulpanit]

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