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,
>
>And my point is that that sounds wrong - if the system can support both 
>then DT should have both properties, because DT describes the system, 
>not Linux policy. Of course it is presumably possible to have a reusable 
>IOMMU IP that itself supports IGS=BOTH, integrated into systems which 
>either don't have an MSI controller, or haven't connected the wires, in 
>which case only one or other property is valid to describe anyway, but 
>that's fine - IGS describes what the IOMMU is capable of sending, the 
>firmware properties describe what the rest of the system is capable of 
>receiving, and if there's any choice left in the intersection of the two 
>then that's up to the OS.

Agreed, Firmware describes hardware capability; when both options are
properly described, prefer-MSI is the right default.

>> 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.
>
>What do you mean by "intent"? Again, people should not be hacking 
>firmware just to influence OS driver policy.
>
>If you mean you have a case where a system does have an MSI controller, 
>and a device which is capable of emitting MSIs, but the device's MSI 
>writes cannot reach that MSI controller, and ACPI is incapable of 
>describing that so Linux ends up incorrectly assuming that MSIs should 
>work, then you have a spec-level ACPI problem and you need to fix RIMT 
>and/or any other relevant tables/properties to be able to indicate that 
>properly; bodging the Linux IOMMU driver alone does not help other OSes, 
>nor even other MSI-capable devices under Linux.

You're right - But RIMT v1.0 lacks a per-IOMMU equivalent of IORT SMMUv3
Flags bit 4 (DeviceID mapping index valid).

I was wondering whether the old ARM IORT compatibility pattern could be
considered here: if a given RIMT IOMMU node fully populates its Interrupt
Wire Array (civ, fiv, piv, pmiv), could that be treated as a temporary
signal for the wired path until RIMT grows an explicit flag?

If that is still too policy-like for ACPI, is there any comparable
transitional pattern that would be more acceptable until RIMT gains the
missing explicit signal?

Thanks,
Fangyu

>Amusingly, that would then be the opposite problem from what we had with 
>Arm SMMU, where initially we couldn't use MSIs with ACPI at all since 
>early IORT version had no way to describe the necessary GIC DeviceID :)
>
>> 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.
>
>But even then it does change the policy in general for all ACPI systems 
>that genuinely do have both and could happily still use MSIs.
>
>Thanks,
>Robin.