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

[Suravee Suthikulpanit]

Hi Radim

On 08/19/2016 09:49 PM, Radim KrÄmÃÅ wrote:
> 2016-08-18 14:42-0500, Suravee Suthikulpanit:
> > This patch implements update_pi_irte function hook to allow SVM
> > communicate to IOMMU driver regarding how to set up IRTE for handling
> > posted interrupt.
> >
> > In case AVIC is enabled, during vcpu_load/unload, SVM needs to update
> > IOMMU IRTE with appropriate host physical APIC ID. Also, when
> > vcpu_blocking/unblocking, SVM needs to update the is-running bit in
> > the IOMMU IRTE. Both are achieved via calling amd_iommu_update_ga().
> >
> > However, if GA mode is not enabled for the pass-through device,
> > IOMMU driver will simply just return when calling amd_iommu_update_ga.
> >
> > Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@xxxxxxx>
> > ---
> > diff --git a/include/linux/amd-iommu.h b/include/linux/amd-iommu.h
> > @@ -34,6 +34,7 @@ struct amd_ir_data {
> >   	struct msi_msg msi_entry;
> >   	void *entry;    /* Pointer to union irte or struct irte_ga */
> >   	void *ref;      /* Pointer to the actual irte */
> > +	struct list_head node;	/* Used by SVM for per-vcpu ir_list */
>
> Putting a list_head here requires all users of amd-iommu to cooperate,
> which is dangerous, but it allows simpler SVM code.  The alternative is
> to force wrappers in SVM, which would also allow IOMMU to keep struct
> amd_ir_data as incomplete in public headers.
>
>   struct struct amd_ir_data_wrapper {
>   	struct list_head node;
>   	struct amd_ir_data *ir_data;
>   }
>
> (The rant continues below.)

The struct amd_ir_data is only used by AMD IOMMU driver (and now SVM 
driver). I don't think it would be that bad since they both would have 
to already coordinate.  Could you please clarify the point you mention 
that it could be dangerous?  Are you thinking of the case where the 
struct amd_ir_data could be free (by AMD IOMMU driver), while still in 
the vcpu list (in SVM)?

> > +static int svm_update_pi_irte(struct kvm *kvm, unsigned int host_irq,
> > +			      uint32_t guest_irq, bool set)
> > +{
> > +	struct kvm_kernel_irq_routing_entry *e;
> > +	struct kvm_irq_routing_table *irq_rt;
> [...]
> > +	hlist_for_each_entry(e, &irq_rt->map[guest_irq], link) {
> > +		struct kvm_lapic_irq irq;
> > +		struct vcpu_data vcpu_info;
> [...]
> > +		kvm_set_msi_irq(e, &irq);
> > +		if (kvm_intr_is_single_vcpu(kvm, &irq, &vcpu)) {
> > +			svm = to_svm(vcpu);
> > +			vcpu_info.pi_desc_addr = page_to_phys(svm->avic_backing_page);
> > +			vcpu_info.vector = irq.vector;
> [...]
> > +			struct amd_iommu_pi_data pi;
> > +
> > +			/* Try to enable guest_mode in IRTE */
> > +			pi.ga_tag = AVIC_GATAG(kvm->arch.avic_vm_id,
> > +						     vcpu->vcpu_id);
> > +			pi.is_guest_mode = true;
> > +			pi.vcpu_data = &vcpu_info;
> > +			ret = irq_set_vcpu_affinity(host_irq, &pi);
> > +			if (!ret && pi.is_guest_mode)
> > +				svm_ir_list_add(svm, pi.ir_data);
>
> I missed a bug here the last time:
>
> If ir_data is already inside some VCPU list and the VCPU changes, then
> we don't remove ir_data from the previous list and just add it to a new
> one.  This was not as bad when we only had wrappers (only resulted in
> duplication), but now we break the removed list ...

Good point here...

> The problem with wrappers is that we don't know what list we should
> remove the "struct amd_ir_data" from;  we would need to add another
> tracking structure or go through all VCPUs.
>
> Having "struct list_head" in "struct amd_ir_data" would allow us to know
> the current list and remove it from there:
> One "struct amd_ir_data" should never be used by more than one caller of
> amd_iommu_update_ga(), because they would have to be cooperating anyway,
> which would mean a single mediator, so we can add a "struct list_head"
> into "struct amd_ir_data".
>
>    Minor design note:
>    To make the usage of "struct amd_ir_data" safer, we could pass "struct
>    list_head" into irq_set_vcpu_affinity(), instead of returning "struct
>    amd_ir_data *".
>
>    irq_set_vcpu_affinity() would add "struct amd_ir_data" to the list only
>    if ir_data was not already in some list and report whether the list
>    was modified.
>
> I think that adding "struct list_head" into "struct amd_ir_data" is
> nicer than having wrappers.
>
> Joerg, Paolo, what do you think?

I think modifying irq_set_vcpu_affinity() to also pass struct list_head 
seems a bit redundant since it is currently design to allow passing in 
void *, which leaves the other option where we might just need to pass 
in a wrapper (e.g. going back to the previous design where we pass in 
struct amd_iommu_pi_data) and also add a pointer to the ir_list in the 
wrapper as well. Then, IOMMU is responsible for adding/deleting ir_data 
to/from this list instead of SVM. This should be fine since we only need 
to coordinate b/w SVM and AMD-IOMMU.

Thanks,
Suravee