linux-kernel - Re: [PATCH v2 1/2] Drivers: hv: vmbus: Propagate VMbus coherence to each VMbus device

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]
Date:   Thu, 24 Mar 2022 11:59:10 +0000
From:   Robin Murphy <robin.murphy@....com>
To:     Michael Kelley <mikelley@...rosoft.com>, sthemmin@...rosoft.com,
        kys@...rosoft.com, haiyangz@...rosoft.com, wei.liu@...nel.org,
        decui@...rosoft.com, rafael@...nel.org, lenb@...nel.org,
        lorenzo.pieralisi@....com, robh@...nel.org, kw@...ux.com,
        bhelgaas@...gle.com, hch@....de, m.szyprowski@...sung.com,
        linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-hyperv@...r.kernel.org, linux-pci@...r.kernel.org,
        iommu@...ts.linux-foundation.org
Subject: Re: [PATCH v2 1/2] Drivers: hv: vmbus: Propagate VMbus coherence to
 each VMbus device

On 2022-03-23 20:31, Michael Kelley wrote:
> VMbus synthetic devices are not represented in the ACPI DSDT -- only
> the top level VMbus device is represented. As a result, on ARM64
> coherence information in the _CCA method is not specified for
> synthetic devices, so they default to not hardware coherent.
> Drivers for some of these synthetic devices have been recently
> updated to use the standard DMA APIs, and they are incurring extra
> overhead of unneeded software coherence management.
> 
> Fix this by propagating coherence information from the VMbus node
> in ACPI to the individual synthetic devices. There's no effect on
> x86/x64 where devices are always hardware coherent.
> 
> Signed-off-by: Michael Kelley <mikelley@...rosoft.com>
> ---
>   drivers/hv/hv_common.c         | 11 +++++++++++
>   drivers/hv/vmbus_drv.c         | 23 +++++++++++++++++++++++
>   include/asm-generic/mshyperv.h |  1 +
>   3 files changed, 35 insertions(+)
> 
> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> index 181d16b..820e814 100644
> --- a/drivers/hv/hv_common.c
> +++ b/drivers/hv/hv_common.c
> @@ -20,6 +20,7 @@
>   #include <linux/panic_notifier.h>
>   #include <linux/ptrace.h>
>   #include <linux/slab.h>
> +#include <linux/dma-map-ops.h>
>   #include <asm/hyperv-tlfs.h>
>   #include <asm/mshyperv.h>
>   
> @@ -216,6 +217,16 @@ bool hv_query_ext_cap(u64 cap_query)
>   }
>   EXPORT_SYMBOL_GPL(hv_query_ext_cap);
>   
> +void hv_setup_dma_ops(struct device *dev, bool coherent)
> +{
> +	/*
> +	 * Hyper-V does not offer a vIOMMU in the guest
> +	 * VM, so pass 0/NULL for the IOMMU settings
> +	 */
> +	arch_setup_dma_ops(dev, 0, 0, NULL, coherent);
> +}
> +EXPORT_SYMBOL_GPL(hv_setup_dma_ops);
> +
>   bool hv_is_hibernation_supported(void)
>   {
>   	return !hv_root_partition && acpi_sleep_state_supported(ACPI_STATE_S4);
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 12a2b37..2d2c54c 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -905,6 +905,14 @@ static int vmbus_probe(struct device *child_device)
>   	struct hv_device *dev = device_to_hv_device(child_device);
>   	const struct hv_vmbus_device_id *dev_id;
>   
> +	/*
> +	 * On ARM64, propagate the DMA coherence setting from the top level
> +	 * VMbus ACPI device to the child VMbus device being added here.
> +	 * On x86/x64 coherence is assumed and these calls have no effect.
> +	 */
> +	hv_setup_dma_ops(child_device,
> +		device_get_dma_attr(&hv_acpi_dev->dev) == DEV_DMA_COHERENT);

Would you mind hooking up the hv_bus.dma_configure method to do this? 
Feel free to fold hv_setup_dma_ops entirely into that if you're not 
likely to need to call it from anywhere else.

> +
>   	dev_id = hv_vmbus_get_id(drv, dev);
>   	if (drv->probe) {
>   		ret = drv->probe(dev, dev_id);
> @@ -2428,6 +2436,21 @@ static int vmbus_acpi_add(struct acpi_device *device)
>   
>   	hv_acpi_dev = device;
>   
> +	/*
> +	 * Older versions of Hyper-V for ARM64 fail to include the _CCA
> +	 * method on the top level VMbus device in the DSDT. But devices
> +	 * are hardware coherent in all current Hyper-V use cases, so fix
> +	 * up the ACPI device to behave as if _CCA is present and indicates
> +	 * hardware coherence.
> +	 */
> +	ACPI_COMPANION_SET(&device->dev, device);
> +	if (IS_ENABLED(CONFIG_ACPI_CCA_REQUIRED) &&
> +	    device_get_dma_attr(&device->dev) == DEV_DMA_NOT_SUPPORTED) {
> +		pr_info("No ACPI _CCA found; assuming coherent device I/O\n");
> +		device->flags.cca_seen = true;
> +		device->flags.coherent_dma = true;
> +	}

I'm not the biggest fan of this, especially since I'm not convinced that 
there are any out-of-support deployments of ARM64 Hyper-V that can't be 
updated. However I suppose it's not "real" firmware, and one Hyper-V 
component is at liberty to hack another Hyper-V component's data if it 
really wants to...

If you can hook up .dma_configure, or clarify if it wouldn't work,

Acked-by: Robin Murphy <robin.murphy@....com>

Cheers,
Robin.

> +
>   	result = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
>   					vmbus_walk_resources, NULL);
>   
> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
> index c08758b..c05d2ce 100644
> --- a/include/asm-generic/mshyperv.h
> +++ b/include/asm-generic/mshyperv.h
> @@ -269,6 +269,7 @@ static inline int cpumask_to_vpset_noself(struct hv_vpset *vpset,
>   u64 hv_ghcb_hypercall(u64 control, void *input, void *output, u32 input_size);
>   void hyperv_cleanup(void);
>   bool hv_query_ext_cap(u64 cap_query);
> +void hv_setup_dma_ops(struct device *dev, bool coherent);
>   void *hv_map_memory(void *addr, unsigned long size);
>   void hv_unmap_memory(void *addr);
>   #else /* CONFIG_HYPERV */