From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932688AbbLNOAZ (ORCPT ); Mon, 14 Dec 2015 09:00:25 -0500 Received: from mx1.redhat.com ([209.132.183.28]:56060 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932566AbbLNOAW (ORCPT ); Mon, 14 Dec 2015 09:00:22 -0500 Date: Mon, 14 Dec 2015 16:00:15 +0200 From: "Michael S. Tsirkin" To: Alexander Duyck Cc: kvm@vger.kernel.org, linux-pci@vger.kernel.org, x86@kernel.org, linux-kernel@vger.kernel.org, alexander.duyck@gmail.com, qemu-devel@nongnu.org, tianyu.lan@intel.com, yang.zhang.wz@gmail.com, konrad.wilk@oracle.com, dgilbert@redhat.com, agraf@suse.de, alex.williamson@redhat.com Subject: Re: [RFC PATCH 3/3] x86: Create dma_mark_dirty to dirty pages used for DMA by VM guest Message-ID: <20151214113016-mutt-send-email-mst@redhat.com> References: <20151213212557.5410.48577.stgit@localhost.localdomain> <20151213212831.5410.84365.stgit@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151213212831.5410.84365.stgit@localhost.localdomain> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Dec 13, 2015 at 01:28:31PM -0800, Alexander Duyck wrote: > This patch is meant to provide the guest with a way of flagging DMA pages > as being dirty to the host when using a direct-assign device within a > guest. The advantage to this approach is that it is fairly simple, however > It currently has a singificant impact on device performance in all the > scenerios where it won't be needed. > > As such this is really meant only as a proof of concept and to get the ball > rolling in terms of figuring out how best to approach the issue of dirty > page tracking for a guest that is using a direct assigned device. In > addition with just this patch it should be possible to modify current > migration approaches so that instead of having to hot-remove the device > before starting the migration this can instead be delayed until the period > before the final stop and copy. > > Signed-off-by: Alexander Duyck > --- > arch/arm/include/asm/dma-mapping.h | 3 ++- > arch/arm64/include/asm/dma-mapping.h | 5 ++--- > arch/ia64/include/asm/dma.h | 1 + > arch/mips/include/asm/dma-mapping.h | 1 + > arch/powerpc/include/asm/swiotlb.h | 1 + > arch/tile/include/asm/dma-mapping.h | 1 + > arch/unicore32/include/asm/dma-mapping.h | 1 + > arch/x86/Kconfig | 11 +++++++++++ > arch/x86/include/asm/swiotlb.h | 26 ++++++++++++++++++++++++++ > drivers/xen/swiotlb-xen.c | 6 ++++++ > lib/swiotlb.c | 6 ++++++ > 11 files changed, 58 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h > index ccb3aa64640d..1962d7b471c7 100644 > --- a/arch/arm/include/asm/dma-mapping.h > +++ b/arch/arm/include/asm/dma-mapping.h > @@ -167,7 +167,8 @@ static inline bool dma_capable(struct device *dev, dma_addr_t addr, size_t size) > return 1; > } > > -static inline void dma_mark_clean(void *addr, size_t size) { } > +static inline void dma_mark_clean(void *addr, size_t size) {} > +static inline void dma_mark_dirty(void *addr, size_t size) {} > > extern int arm_dma_set_mask(struct device *dev, u64 dma_mask); > > diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h > index 61e08f360e31..8d24fe11c8a3 100644 > --- a/arch/arm64/include/asm/dma-mapping.h > +++ b/arch/arm64/include/asm/dma-mapping.h > @@ -84,9 +84,8 @@ static inline bool dma_capable(struct device *dev, dma_addr_t addr, size_t size) > return addr + size - 1 <= *dev->dma_mask; > } > > -static inline void dma_mark_clean(void *addr, size_t size) > -{ > -} > +static inline void dma_mark_clean(void *addr, size_t size) {} > +static inline void dma_mark_dirty(void *addr, size_t size) {} > > #endif /* __KERNEL__ */ > #endif /* __ASM_DMA_MAPPING_H */ > diff --git a/arch/ia64/include/asm/dma.h b/arch/ia64/include/asm/dma.h > index 4d97f60f1ef5..d92ebeb2758e 100644 > --- a/arch/ia64/include/asm/dma.h > +++ b/arch/ia64/include/asm/dma.h > @@ -20,5 +20,6 @@ extern unsigned long MAX_DMA_ADDRESS; > #define free_dma(x) > > void dma_mark_clean(void *addr, size_t size); > +static inline void dma_mark_dirty(void *addr, size_t size) {} > > #endif /* _ASM_IA64_DMA_H */ > diff --git a/arch/mips/include/asm/dma-mapping.h b/arch/mips/include/asm/dma-mapping.h > index e604f760c4a0..567f6e03e337 100644 > --- a/arch/mips/include/asm/dma-mapping.h > +++ b/arch/mips/include/asm/dma-mapping.h > @@ -28,6 +28,7 @@ static inline bool dma_capable(struct device *dev, dma_addr_t addr, size_t size) > } > > static inline void dma_mark_clean(void *addr, size_t size) {} > +static inline void dma_mark_dirty(void *addr, size_t size) {} > > #include > > diff --git a/arch/powerpc/include/asm/swiotlb.h b/arch/powerpc/include/asm/swiotlb.h > index de99d6e29430..b694e8399e28 100644 > --- a/arch/powerpc/include/asm/swiotlb.h > +++ b/arch/powerpc/include/asm/swiotlb.h > @@ -16,6 +16,7 @@ > extern struct dma_map_ops swiotlb_dma_ops; > > static inline void dma_mark_clean(void *addr, size_t size) {} > +static inline void dma_mark_dirty(void *addr, size_t size) {} > > extern unsigned int ppc_swiotlb_enable; > int __init swiotlb_setup_bus_notifier(void); > diff --git a/arch/tile/include/asm/dma-mapping.h b/arch/tile/include/asm/dma-mapping.h > index 96ac6cce4a32..79953f09e938 100644 > --- a/arch/tile/include/asm/dma-mapping.h > +++ b/arch/tile/include/asm/dma-mapping.h > @@ -58,6 +58,7 @@ static inline phys_addr_t dma_to_phys(struct device *dev, dma_addr_t daddr) > } > > static inline void dma_mark_clean(void *addr, size_t size) {} > +static inline void dma_mark_dirty(void *addr, size_t size) {} > > static inline void set_dma_ops(struct device *dev, struct dma_map_ops *ops) > { > diff --git a/arch/unicore32/include/asm/dma-mapping.h b/arch/unicore32/include/asm/dma-mapping.h > index 8140e053ccd3..b9d357ab122d 100644 > --- a/arch/unicore32/include/asm/dma-mapping.h > +++ b/arch/unicore32/include/asm/dma-mapping.h > @@ -49,6 +49,7 @@ static inline phys_addr_t dma_to_phys(struct device *dev, dma_addr_t daddr) > } > > static inline void dma_mark_clean(void *addr, size_t size) {} > +static inline void dma_mark_dirty(void *addr, size_t size) {} > > static inline void dma_cache_sync(struct device *dev, void *vaddr, > size_t size, enum dma_data_direction direction) > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index db3622f22b61..f0b09156d7d8 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -841,6 +841,17 @@ config SWIOTLB > with more than 3 GB of memory. > If unsure, say Y. > > +config SWIOTLB_PAGE_DIRTYING > + bool "SWIOTLB page dirtying" > + depends on SWIOTLB > + default n > + ---help--- > + SWIOTLB page dirtying support provides a means for the guest to > + trigger write faults on pages which received DMA from the device > + without changing the data contained within. By doing this the > + guest can then support migration assuming the device and any > + remaining pages are unmapped prior to the CPU itself being halted. > + > config IOMMU_HELPER > def_bool y > depends on CALGARY_IOMMU || GART_IOMMU || SWIOTLB || AMD_IOMMU > diff --git a/arch/x86/include/asm/swiotlb.h b/arch/x86/include/asm/swiotlb.h > index ab05d73e2bb7..7f9f2e76d081 100644 > --- a/arch/x86/include/asm/swiotlb.h > +++ b/arch/x86/include/asm/swiotlb.h > @@ -29,6 +29,32 @@ static inline void pci_swiotlb_late_init(void) > > static inline void dma_mark_clean(void *addr, size_t size) {} > > +/* > + * Make certain that the pages get marked as dirty > + * now that the device has completed the DMA transaction. > + * > + * Without this we run the risk of a guest migration missing > + * the pages that the device has written to as they are not > + * tracked as a part of the dirty page tracking. > + */ > +static inline void dma_mark_dirty(void *addr, size_t size) > +{ > +#ifdef CONFIG_SWIOTLB_PAGE_DIRTYING I like where this is going. However as distributions don't like shipping multiple kernels, I think we also need a way to configure this at runtime, even if enabled at build time. How about - mark dirty is enabled at boot if requested (e.g. by kernel command line) - mark dirty can later be disabled/enabled by sysctl (Enabling at runtime might be a bit tricky as it has to sync with all CPUs - use e.g. RCU for this?). This way distro can use a guest agent to disable dirtying until before migration starts. > + unsigned long pg_addr, start; > + > + start = (unsigned long)addr; > + pg_addr = PAGE_ALIGN(start + size); > + start &= ~(sizeof(atomic_t) - 1); > + > + /* trigger a write fault on each page, excluding first page */ > + while ((pg_addr -= PAGE_SIZE) > start) > + atomic_add(0, (atomic_t *)pg_addr); > + > + /* trigger a write fault on first word of DMA */ > + atomic_add(0, (atomic_t *)start); start might not be aligned correctly for a cast to atomic_t. It's harmless to do this for any memory, so I think you should just do this for 1st byte of all pages including the first one. > +#endif /* CONFIG_SWIOTLB_PAGE_DIRTYING */ > +} > + > extern void *x86_swiotlb_alloc_coherent(struct device *hwdev, size_t size, > dma_addr_t *dma_handle, gfp_t flags, > struct dma_attrs *attrs); > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > index 2154c70e47da..1533b3eefb67 100644 > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -456,6 +456,9 @@ void xen_swiotlb_unmap_page(struct device *hwdev, dma_addr_t dev_addr, > */ > if (dir == DMA_FROM_DEVICE) > dma_mark_clean(phys_to_virt(paddr), size); > + > + if (dir != DMA_TO_DEVICE) > + dma_mark_dirty(phys_to_virt(paddr), size); > } > EXPORT_SYMBOL_GPL(xen_swiotlb_unmap_page); > > @@ -485,6 +488,9 @@ xen_swiotlb_sync_single_for_cpu(struct device *hwdev, dma_addr_t dev_addr, > > if (dir == DMA_FROM_DEVICE) > dma_mark_clean(phys_to_virt(paddr), size); > + > + if (dir != DMA_TO_DEVICE) > + dma_mark_dirty(phys_to_virt(paddr), size); > } > EXPORT_SYMBOL_GPL(xen_swiotlb_sync_single_for_cpu); > > diff --git a/lib/swiotlb.c b/lib/swiotlb.c > index 384ac06217b2..4223d6c54724 100644 > --- a/lib/swiotlb.c > +++ b/lib/swiotlb.c > @@ -802,6 +802,9 @@ void swiotlb_unmap_page(struct device *hwdev, dma_addr_t dev_addr, > */ > if (dir == DMA_FROM_DEVICE) > dma_mark_clean(phys_to_virt(paddr), size); > + > + if (dir != DMA_TO_DEVICE) > + dma_mark_dirty(phys_to_virt(paddr), size); > } > EXPORT_SYMBOL_GPL(swiotlb_unmap_page); > > @@ -830,6 +833,9 @@ swiotlb_sync_single_for_cpu(struct device *hwdev, dma_addr_t dev_addr, > > if (dir == DMA_FROM_DEVICE) > dma_mark_clean(phys_to_virt(paddr), size); > + > + if (dir != DMA_TO_DEVICE) > + dma_mark_dirty(phys_to_virt(paddr), size); > } > EXPORT_SYMBOL(swiotlb_sync_single_for_cpu); >