Hi Thomas,
On Wed, 06 Dec 2023 21:26:55 +0100, Thomas Gleixner <tglx@xxxxxxxxxxxxx>
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
