Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Date:   Wed, 13 Dec 2023 14:00:21 -0800
From:   Jacob Pan <jacob.jun.pan@linux.intel.com>
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     LKML <linux-kernel@vger.kernel.org>, X86 Kernel <x86@kernel.org>,
        iommu@lists.linux.dev, Lu Baolu <baolu.lu@linux.intel.com>,
        kvm@vger.kernel.org, Dave Hansen <dave.hansen@intel.com>,
        Joerg Roedel <joro@8bytes.org>,
        "H. Peter Anvin" <hpa@zytor.com>, Borislav Petkov <bp@alien8.de>,
        Ingo Molnar <mingo@redhat.com>,
        Raj Ashok <ashok.raj@intel.com>,
        "Tian, Kevin" <kevin.tian@intel.com>, maz@kernel.org,
        peterz@infradead.org, seanjc@google.com,
        Robin Murphy <robin.murphy@arm.com>,
        jacob.jun.pan@linux.intel.com
Subject: Re: [PATCH RFC 13/13] iommu/vt-d: Enable posted mode for device
 MSIs
Message-ID: <20231213140021.4cc84bb2@jacob-builder>
In-Reply-To: <87zfynt6uo.ffs@tglx>
References: <20231112041643.2868316-1-jacob.jun.pan@linux.intel.com>
        <20231112041643.2868316-14-jacob.jun.pan@linux.intel.com>
        <87zfynt6uo.ffs@tglx>
Organization: OTC
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Thomas,

On Wed, 06 Dec 2023 21:26:55 +0100, Thomas Gleixner <tglx@linutronix.de>
wrote:

> On Sat, Nov 11 2023 at 20:16, Jacob Pan wrote:
> >  #ifdef CONFIG_X86_POSTED_MSI
> >  
> >  static u64 get_pi_desc_addr(struct irq_data *irqd)
> > @@ -1133,6 +1144,29 @@ static u64 get_pi_desc_addr(struct irq_data
> > *irqd) 
> >  	return __pa(per_cpu_ptr(&posted_interrupt_desc, cpu));
> >  }
> > +
> > +static void intel_ir_reconfigure_irte_posted(struct irq_data *irqd)
> > +{
> > +	struct intel_ir_data *ir_data = irqd->chip_data;
> > +	struct irte *irte = &ir_data->irte_entry;
> > +	struct irte irte_pi;
> > +	u64 pid_addr;
> > +
> > +	pid_addr = get_pi_desc_addr(irqd);
> > +
> > +	memset(&irte_pi, 0, sizeof(irte_pi));
> > +
> > +	/* The shared IRTE already be set up as posted during
> > alloc_irte */  
> 
> -ENOPARSE
Will delete this. What I meant was that the shared IRTE has already been
setup as posted mode instead of remappable mode. So when we make a copy,
there is no need to change the mode.

> > +	dmar_copy_shared_irte(&irte_pi, irte);
> > +
> > +	irte_pi.pda_l = (pid_addr >> (32 - PDA_LOW_BIT)) & ~(-1UL <<
> > PDA_LOW_BIT);
> > +	irte_pi.pda_h = (pid_addr >> 32) & ~(-1UL << PDA_HIGH_BIT);
> > +
> > +	modify_irte(&ir_data->irq_2_iommu, &irte_pi);
> > +}
> > +
> > +#else
> > +static inline void intel_ir_reconfigure_irte_posted(struct irq_data
> > *irqd) {} #endif
> >  
> >  static void intel_ir_reconfigure_irte(struct irq_data *irqd, bool
> > force) @@ -1148,8 +1182,9 @@ static void
> > intel_ir_reconfigure_irte(struct irq_data *irqd, bool force)
> > irte->vector = cfg->vector; irte->dest_id = IRTE_DEST(cfg->dest_apicid);
> >  
> > -	/* Update the hardware only if the interrupt is in remapped
> > mode. */
> > -	if (force || ir_data->irq_2_iommu.mode == IRQ_REMAPPING)
> > +	if (ir_data->irq_2_iommu.posted_msi)
> > +		intel_ir_reconfigure_irte_posted(irqd);
> > +	else if (force || ir_data->irq_2_iommu.mode == IRQ_REMAPPING)
> >  		modify_irte(&ir_data->irq_2_iommu, irte);
> >  }
> >  
> > @@ -1203,7 +1238,7 @@ static int intel_ir_set_vcpu_affinity(struct
> > irq_data *data, void *info) struct intel_ir_data *ir_data =
> > data->chip_data; struct vcpu_data *vcpu_pi_info = info;
> >  
> > -	/* stop posting interrupts, back to remapping mode */
> > +	/* stop posting interrupts, back to the default mode */
> >  	if (!vcpu_pi_info) {
> >  		modify_irte(&ir_data->irq_2_iommu,
> > &ir_data->irte_entry); } else {
> > @@ -1300,10 +1335,14 @@ static void
> > intel_irq_remapping_prepare_irte(struct intel_ir_data *data, {
> >  	struct irte *irte = &data->irte_entry;
> >  
> > -	prepare_irte(irte, irq_cfg->vector, irq_cfg->dest_apicid);
> > +	if (data->irq_2_iommu.mode == IRQ_POSTING)
> > +		prepare_irte_posted(irte);
> > +	else
> > +		prepare_irte(irte, irq_cfg->vector,
> > irq_cfg->dest_apicid); 
> >  	switch (info->type) {
> >  	case X86_IRQ_ALLOC_TYPE_IOAPIC:
> > +		prepare_irte(irte, irq_cfg->vector,
> > irq_cfg->dest_apicid);  
> 
> What? This is just wrong. Above you have:
> 
> > +	if (data->irq_2_iommu.mode == IRQ_POSTING)
> > +		prepare_irte_posted(irte);
> > +	else
> > +		prepare_irte(irte, irq_cfg->vector,
> > irq_cfg->dest_apicid);  
> 
> Can you spot the fail?
My bad, I forgot to delete this.

It is probably easier just override the IRTE for the posted MSI case.
@@ -1274,6 +1354,11 @@ static void intel_irq_remapping_prepare_irte(struct intel_ir_data 
*data,
                break;
        case X86_IRQ_ALLOC_TYPE_PCI_MSI:
        case X86_IRQ_ALLOC_TYPE_PCI_MSIX:
+               if (posted_msi_supported()) {
+                       prepare_irte_posted(irte);
+                       data->irq_2_iommu.posted_msi = 1;
+               }
+

> 
> >  		/* Set source-id of interrupt request */
> >  		set_ioapic_sid(irte, info->devid);
> >  		apic_printk(APIC_VERBOSE, KERN_DEBUG "IOAPIC[%d]: Set
> > IRTE entry (P:%d FPD:%d Dst_Mode:%d Redir_hint:%d Trig_Mode:%d
> > Dlvry_Mode:%X Avail:%X Vector:%02X Dest:%08X SID:%04X SQ:%X SVT:%X)\n",
> > @@ -1315,10 +1354,18 @@ static void
> > intel_irq_remapping_prepare_irte(struct intel_ir_data *data, sub_handle
> > = info->ioapic.pin; break; case X86_IRQ_ALLOC_TYPE_HPET:
> > +		prepare_irte(irte, irq_cfg->vector,
> > irq_cfg->dest_apicid); set_hpet_sid(irte, info->devid);
> >  		break;
> >  	case X86_IRQ_ALLOC_TYPE_PCI_MSI:
> >  	case X86_IRQ_ALLOC_TYPE_PCI_MSIX:
> > +		if (posted_msi_supported()) {
> > +			prepare_irte_posted(irte);
> > +			data->irq_2_iommu.posted_msi = 1;
> > +		} else {
> > +			prepare_irte(irte, irq_cfg->vector,
> > irq_cfg->dest_apicid);
> > +		}  
> 
> Here it gets even more hilarious.


Thanks,

Jacob