linux-kernel - RE: [PATCH v3 6/7] Drivers: hv: vmbus: Get the IRQ number from DT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
 <SN6PR02MB41571723DB27E0A29877854ED4BE2@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Mon, 5 Aug 2024 03:03:04 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Roman Kisel <romank@...ux.microsoft.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

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.

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
>