Re: [PATCH 4/4 RESEND] PCI: hv: Propagate coherence from VMbus device to PCI device

[Robin Murphy]

On 2022-03-17 16:25, Michael Kelley via iommu wrote:
> PCI pass-thru devices in a Hyper-V VM are represented as a VMBus
> device and as a PCI device.  The coherence of the VMbus device is
> set based on the VMbus node in ACPI, but the PCI device has no
> ACPI node and defaults to not hardware coherent.  This results
> in extra software coherence management overhead on ARM64 when
> devices are hardware coherent.
>
> Fix this by propagating the coherence of the VMbus device to the
> PCI device.  There's no effect on x86/x64 where devices are
> always hardware coherent.
>
> Signed-off-by: Michael Kelley <mikelley@xxxxxxxxxxxxx>
> ---
>   drivers/pci/controller/pci-hyperv.c | 17 +++++++++++++----
>   1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index ae0bc2f..14276f5 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -49,6 +49,7 @@
>   #include <linux/refcount.h>
>   #include <linux/irqdomain.h>
>   #include <linux/acpi.h>
> +#include <linux/dma-map-ops.h>
>   #include <asm/mshyperv.h>
>   
>   /*
> @@ -2142,9 +2143,9 @@ static void hv_pci_remove_slots(struct hv_pcibus_device *hbus)
>   }
>   
>   /*
> - * Set NUMA node for the devices on the bus
> + * Set NUMA node and DMA coherence for the devices on the bus
>    */
> -static void hv_pci_assign_numa_node(struct hv_pcibus_device *hbus)
> +static void hv_pci_assign_properties(struct hv_pcibus_device *hbus)
>   {
>   	struct pci_dev *dev;
>   	struct pci_bus *bus = hbus->bridge->bus;
> @@ -2167,6 +2168,14 @@ static void hv_pci_assign_numa_node(struct hv_pcibus_device *hbus)
>   				     numa_map_to_online_node(
>   					     hv_dev->desc.virtual_numa_node));
>   
> +		/*
> +		 * On ARM64, propagate the DMA coherence from the VMbus device
> +		 * to the corresponding PCI device. On x86/x64, these calls
> +		 * have no effect because DMA is always hardware coherent.
> +		 */
> +		dev_set_dma_coherent(&dev->dev,
> +			dev_is_dma_coherent(&hbus->hdev->device));

Eww... if you really have to do this, I'd prefer to see a proper 
hv_dma_configure() helper implemented and wired up to 
pci_dma_configure(). Although since it's a generic property I guess at 
worst pci_dma_configure could perhaps propagate coherency from the host 
bridge to its children by itself in the absence of any other firmware 
info. And it's built-in so could use arch_setup_dma_ops() like everyone 
else.

Robin.

> +
>   		put_pcichild(hv_dev);
>   	}
>   }
> @@ -2191,7 +2200,7 @@ static int create_root_hv_pci_bus(struct hv_pcibus_device *hbus)
>   		return error;
>   
>   	pci_lock_rescan_remove();
> -	hv_pci_assign_numa_node(hbus);
> +	hv_pci_assign_properties(hbus);
>   	pci_bus_assign_resources(bridge->bus);
>   	hv_pci_assign_slots(hbus);
>   	pci_bus_add_devices(bridge->bus);
> @@ -2458,7 +2467,7 @@ static void pci_devices_present_work(struct work_struct *work)
>   		 */
>   		pci_lock_rescan_remove();
>   		pci_scan_child_bus(hbus->bridge->bus);
> -		hv_pci_assign_numa_node(hbus);
> +		hv_pci_assign_properties(hbus);
>   		hv_pci_assign_slots(hbus);
>   		pci_unlock_rescan_remove();
>   		break;