Re: [PATCH] iommu/riscv: prefer WSI on IGS=BOTH when wired IRQs are described

[fangyu.yu@linux.alibaba.com]

>>> Won't it change the DT behavior as it doesn't check msi-parent
>>> anymore?  IOW, should this be made specific to ACPI
>>> by checking whether the device node is acpi node?
>>>
>> 
>> Thanks for the review, you're right that the current condition would
>> affect DT as well.
>> 
>> My assumption was that DT would describe either "interrupts" for WSI
>> or "msi-parent" for MSI, but not both, so platform_irq_count() > 0
>> would effectively imply "no msi-parent" in practice.
>> 
>> That said, I agree that this should be limited to the ACPI path. That
>> matches the actual bug scope — only ACPI was falling back to MSI when
>> wired IRQs were described — and leaves DT unchanged.
>> 
>> I'll respin a v2 to scope the fix to ACPI only, and tighten the commit
>> message accordingly.
>
>Why would this be specific to ACPI? AFAICS if DT specifies an 
>"msi-parent" property such that an MSI domain exists, then MSIs will be 
>preferred as well, since the entire function is structured to prefer 
>MSIs if available, and fall back to wired if not. If the system does 
>support both options then DT should describe that, same as ACPI; what 
>Linux chooses to use is entirely Linux's own policy.
>
>But if you do want to change the Linux driver's policy for some reason 
>that the commit message isn't really explaining, then rearrange the 
>whole switch statement; don't just add a weird hack to try to defeat the 
>existing logic _in the same function_...
>
>In general though, I would have thought preferring MSIs makes the most 
>sense, since MSI vectors are generally cheaper than wires, so there's a 
>better chance of being able to have unique IRQs per interrupt source, 
>whereas with wires you may be stuck with a combined IRQ.

Thanks, Robin.

To clarify, the goal here is not to change the general MSI-vs-WSI
preference policy. The bug is that, when the hardware advertises
IGS=BOTH, DT already provides a way to select WSI by describing wired
IRQs without an msi-parent, but the ACPI path does not currently have
an equivalent way to steer the driver to WSI.

    - DT: if "msi-parent" is omitted, of_msi_get_domain() returns NULL,
      IGS=BOTH then falls through to the WSI path, so DT has a real knob
      ("don't describe MSI") to select WSI.
    - ACPI: there is no per-device equivalent. On systems with IMSIC, the
      IGS=BOTH IOMMU always enters the MSI branch and resolves a non-NULL
      msi_domain, regardless of what its own _CRS describes. The MSI fallback
      only triggers when the system has no IMSIC at all, which is a
      firmware-wide decision rather than a per-device one.

As a result, with ACPI and IGS=BOTH, the driver always uses MSI even when
firmware describes wired IRQs, so the platform cannot express the same
intent as it can with DT.

So this patch is meant to fix that ACPI gap for IGS=BOTH, not to make
WSI generally preferred over MSI. I agree the commit message should
make that bug scope explicit.

>
>(As a side note, is there a reason this is calling of_msi_configure() on 
>a platform device when of_platform_device_create_pdata() will have done 
>that already?)

Good catch, agreed.

Thanks,
Fangyu

>
>Thanks,
>Robin.