Message-ID: <eb0ffccd-4ec8-42c8-86a3-ae1a7f25fc9c@linux.microsoft.com>
Date: Mon, 5 Aug 2024 09:26:32 -0700
From: Roman Kisel <romank@...ux.microsoft.com>
To: Michael Kelley <mhklinux@...look.com>, Arnd Bergmann <arnd@...db.de>,
 Krzysztof Kozlowski <krzk@...nel.org>,
 "bhelgaas@...gle.com" <bhelgaas@...gle.com>, Borislav Petkov <bp@...en8.de>,
 Catalin Marinas <catalin.marinas@....com>,
 Dave Hansen <dave.hansen@...ux.intel.com>, Dexuan Cui <decui@...rosoft.com>,
 Haiyang Zhang <haiyangz@...rosoft.com>, "H. Peter Anvin" <hpa@...or.com>,
 Krzysztof Wilczyński <kw@...ux.com>,
 "K. Y. Srinivasan" <kys@...rosoft.com>, Len Brown <lenb@...nel.org>,
 Lorenzo Pieralisi <lpieralisi@...nel.org>, Ingo Molnar <mingo@...hat.com>,
 "Rafael J . Wysocki" <rafael@...nel.org>, Rob Herring <robh@...nel.org>,
 Thomas Gleixner <tglx@...utronix.de>, Wei Liu <wei.liu@...nel.org>,
 Will Deacon <will@...nel.org>,
 "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
 Linux-Arch <linux-arch@...r.kernel.org>,
 "linux-arm-kernel@...ts.infradead.org"
 <linux-arm-kernel@...ts.infradead.org>,
 "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
 "x86@...nel.org" <x86@...nel.org>
Cc: "apais@...rosoft.com" <apais@...rosoft.com>,
 "benhill@...rosoft.com" <benhill@...rosoft.com>,
 "ssengar@...rosoft.com" <ssengar@...rosoft.com>,
 "sunilmut@...rosoft.com" <sunilmut@...rosoft.com>,
 "vdso@...bites.dev" <vdso@...bites.dev>
Subject: Re: [PATCH v3 6/7] Drivers: hv: vmbus: Get the IRQ number from DT



On 8/4/2024 8:03 PM, Michael Kelley wrote:
> From: Roman Kisel <romank@...ux.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

Powered by blists - more mailing lists