Re: [PATCH v3 6/7] Drivers: hv: vmbus: Get the IRQ number from DT

[Roman Kisel <romank@linux.microsoft.com>]

On 8/4/2024 8:03 PM, Michael Kelley wrote:
> From: Roman Kisel <romank@linux.microsoft.com> Sent: Monday, July 29, 2024 9:51 AM
>>
>>
>> On 7/27/2024 2:17 AM, Arnd Bergmann wrote:
>>> On Sat, Jul 27, 2024, at 10:56, Krzysztof Kozlowski wrote:
>>>> On 27/07/2024 00:59, Roman Kisel wrote:
>>>>> @@ -2338,6 +2372,21 @@ static int vmbus_device_add(struct platform_device *pdev)
>>>>>    		cur_res = &res->sibling;
>>>>>    	}
>>>>>
>>>>> +	/*
>>>>> +	 * Hyper-V always assumes DMA cache coherency, and the DMA subsystem
>>>>> +	 * might default to 'not coherent' on some architectures.
>>>>> +	 * Avoid high-cost cache coherency maintenance done by the CPU.
>>>>> +	 */
>>>>> +#if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
>>>>> +	defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
>>>>> +	defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
>>>>> +
>>>>> +	if (!of_property_read_bool(np, "dma-coherent"))
>>>>> +		pr_warn("Assuming cache coherent DMA transactions, no 'dma-coherent' node supplied\n");
>>>>
>>>> Why do you need this property at all, if it is allways dma-coherent? Are
>>>> you supporting dma-noncoherent somewhere?
>>>
>>> It's just a sanity check that the DT is well-formed.
> 
> In my view, this chunk of code can be dropped entirely. The guest
> should believe what the Hyper-V host tells it via DT, and that includes
> operating in non-coherent mode. There might be some future case
> where non-coherent DMA is correct. In such a case, we don't want to
> have to come back and remove an overly aggressive sanity test from
> Linux kernel code.
> 
> As Arnd noted, the dma-coherent (or dma-noncoherent) property should
> be interpreted and applied to the device by common code. If that's not
> working for some reason in this case, we should investigate why not.
> 
> Note that the ACPI code for VMBus does the same thing -- it believes and
> uses whatever the _CCA property says. The exception is that there
> are deployed version of Hyper-V that don't set _CCA at all, contrary to the
> ACPI spec. So there's a hack in vmbus_acpi_add() to work around this case
> and force coherent_dma. But that's the only place where the current
> Hyper-V assumption of coherence comes into play. I sincerely hope Hyper-V
> ensures that the DT correctly includes dma-coherent from the start, and
> that we don't have to replicate the hack on the DT side.
> 
I was replicating the _CCA hack diligently a bit much too much, agreed. 
This great conversation really gives me reassurance that the code 
doesn't have to be paranoid, and I can happily remove this if statement.

> Michael
> 
>>>
>>> Since the dma-coherent property is interpreted by common code, it's
>>> not up to hv to change the default for the platform. I'm not sure
>>> if the presence of CONFIG_ARCH_HAS_SYNC_DMA_* options is the correct
>>> check to determine that an architecture defaults to noncoherent
>>> though, as the function may be needed to do something else.
>> I used the ifdef as the dma_coherent field is declared under these macros:
>>
>> #if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
>> 	defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
>> 	defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
>> extern bool dma_default_coherent;
>> static inline bool dev_is_dma_coherent(struct device *dev)
>> {
>> 	return dev->dma_coherent;
>> }
>> #else
>> #define dma_default_coherent true
>>
>> static inline bool dev_is_dma_coherent(struct device *dev)
>> {
>> 	return true;
>> }
>>
>> i.e., there is no API to set dma_coherent. As I see it, the options
>> are either warn the user if they forgot to add `dma-coherent`
>>
>> if (!dev_is_dma_coherent(dev)) pr_warn("add dma-coherent to be faster\n"),
>>
>> or warn and force the flag to true. Maybe just warn
>> the user I think now... The code will be cleaner (no need to emulate
>> a-would-be set_dma_coherent) , and the user will
>> know how to make the system perform at its best.
>>
>> Appreciate sharing the reservations about that piece!
>>
>>>
>>> The global "dma_default_coherent' may be a better thing to check
>>> for. This is e.g. set on powerpc64, riscv and on specific mips
>>> platforms, but it's never set on arm64 as far as I can tell.
>>>
>>>        Arnd
>>
>> --
>> Thank you,
>> Roman
>>

-- 
Thank you,
Roman