* [PATCH 01/10] x86/paravirt: make paravirt_patch_call() and paravirt_patch_jmp() static
From: Juergen Gross @ 2018-08-10 11:52 UTC (permalink / raw)
To: linux-kernel, xen-devel, x86, virtualization
Cc: Juergen Gross, boris.ostrovsky, rusty, mingo, hpa, akataria, tglx
In-Reply-To: <20180810115252.18213-1-jgross@suse.com>
paravirt_patch_call() and paravirt_patch_jmp() are used in paravirt.c
only. Convert them to static.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
arch/x86/include/asm/paravirt_types.h | 6 ------
arch/x86/kernel/paravirt.c | 12 ++++++------
2 files changed, 6 insertions(+), 12 deletions(-)
diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
index 180bc0bff0fb..036b2f88f105 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -370,12 +370,6 @@ extern struct pv_lock_ops pv_lock_ops;
unsigned paravirt_patch_ident_32(void *insnbuf, unsigned len);
unsigned paravirt_patch_ident_64(void *insnbuf, unsigned len);
-unsigned paravirt_patch_call(void *insnbuf,
- const void *target, u16 tgt_clobbers,
- unsigned long addr, u16 site_clobbers,
- unsigned len);
-unsigned paravirt_patch_jmp(void *insnbuf, const void *target,
- unsigned long addr, unsigned len);
unsigned paravirt_patch_default(u8 type, u16 clobbers, void *insnbuf,
unsigned long addr, unsigned len);
diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
index 930c88341e4e..ce560b916b1f 100644
--- a/arch/x86/kernel/paravirt.c
+++ b/arch/x86/kernel/paravirt.c
@@ -80,10 +80,10 @@ struct branch {
u32 delta;
} __attribute__((packed));
-unsigned paravirt_patch_call(void *insnbuf,
- const void *target, u16 tgt_clobbers,
- unsigned long addr, u16 site_clobbers,
- unsigned len)
+static unsigned paravirt_patch_call(void *insnbuf,
+ const void *target, u16 tgt_clobbers,
+ unsigned long addr, u16 site_clobbers,
+ unsigned len)
{
struct branch *b = insnbuf;
unsigned long delta = (unsigned long)target - (addr+5);
@@ -102,8 +102,8 @@ unsigned paravirt_patch_call(void *insnbuf,
return 5;
}
-unsigned paravirt_patch_jmp(void *insnbuf, const void *target,
- unsigned long addr, unsigned len)
+static unsigned paravirt_patch_jmp(void *insnbuf, const void *target,
+ unsigned long addr, unsigned len)
{
struct branch *b = insnbuf;
unsigned long delta = (unsigned long)target - (addr+5);
--
2.13.7
^ permalink raw reply related
* [PATCH 00/10] x86/paravirt: several cleanups
From: Juergen Gross @ 2018-08-10 11:52 UTC (permalink / raw)
To: linux-kernel, xen-devel, x86, virtualization
Cc: Juergen Gross, boris.ostrovsky, rusty, mingo, hpa, akataria, tglx
This series removes some no longer needed stuff from paravirt
infrastructure and puts large quantities of paravirt ops under a new
config option PARAVIRT_XXL which is selected by XEN_PV only.
A pvops kernel without XEN_PV being configured is about 2.5% smaller
with this series applied.
tip commit 5800dc5c19f34e6e03b5adab1282535cb102fafd ("x86/paravirt:
Fix spectre-v2 mitigations for paravirt guests") is a prerequisite
for this series.
The last 4 patches of this series require my Xen cleanup series
https://lore.kernel.org/lkml/20180717120113.12756-1-jgross@suse.com/
which hides more Xen PV-only code behind CONFIG_XEN_PV.
Juergen Gross (10):
x86/paravirt: make paravirt_patch_call() and paravirt_patch_jmp()
static
x86/paravirt: remove clobbers parameter from paravirt patch functions
x86/paravirt: remove clobbers from struct paravirt_patch_site
x86/paravirt: use a single ops structure
x86/paravirt: remove unused paravirt bits
x86/paravirt: introduce new config option PARAVIRT_XXL
x86/paravirt: move items in pv_info under PARAVIRT_XXL umbrella
x86/paravirt: move the Xen-only pv_cpu_ops under the PARAVIRT_XXL
umbrella
x86/paravirt: move the Xen-only pv_irq_ops under the PARAVIRT_XXL
umbrella
x86/paravirt: move the Xen-only pv_mmu_ops under the PARAVIRT_XXL
umbrella
arch/x86/Kconfig | 3 +
arch/x86/hyperv/mmu.c | 4 +-
arch/x86/include/asm/debugreg.h | 2 +-
arch/x86/include/asm/desc.h | 4 +-
arch/x86/include/asm/fixmap.h | 2 +-
arch/x86/include/asm/irqflags.h | 56 +++---
arch/x86/include/asm/mmu_context.h | 4 +-
arch/x86/include/asm/msr.h | 4 +-
arch/x86/include/asm/paravirt.h | 183 +++++++++---------
arch/x86/include/asm/paravirt_types.h | 65 +++----
arch/x86/include/asm/pgalloc.h | 2 +-
arch/x86/include/asm/pgtable-3level_types.h | 2 +-
arch/x86/include/asm/pgtable.h | 7 +-
arch/x86/include/asm/processor.h | 4 +-
arch/x86/include/asm/ptrace.h | 3 +-
arch/x86/include/asm/segment.h | 2 +-
arch/x86/include/asm/special_insns.h | 4 +-
arch/x86/kernel/alternative.c | 2 +-
arch/x86/kernel/asm-offsets.c | 15 +-
arch/x86/kernel/asm-offsets_64.c | 9 +-
arch/x86/kernel/cpu/common.c | 4 +-
arch/x86/kernel/cpu/vmware.c | 4 +-
arch/x86/kernel/head_64.S | 2 +-
arch/x86/kernel/kvm.c | 18 +-
arch/x86/kernel/kvmclock.c | 4 +-
arch/x86/kernel/paravirt-spinlocks.c | 15 +-
arch/x86/kernel/paravirt.c | 290 ++++++++++++++--------------
arch/x86/kernel/paravirt_patch_32.c | 9 +-
arch/x86/kernel/paravirt_patch_64.c | 11 +-
arch/x86/kernel/tsc.c | 2 +-
arch/x86/kernel/vsmp_64.c | 17 +-
arch/x86/xen/Kconfig | 1 +
arch/x86/xen/enlighten_pv.c | 32 +--
arch/x86/xen/irq.c | 2 +-
arch/x86/xen/mmu_hvm.c | 2 +-
arch/x86/xen/mmu_pv.c | 28 +--
arch/x86/xen/spinlock.c | 12 +-
arch/x86/xen/time.c | 4 +-
drivers/xen/time.c | 2 +-
39 files changed, 430 insertions(+), 406 deletions(-)
--
2.13.7
^ permalink raw reply
* Re: [PATCH] drm: qxl: Fix error handling at qxl_device_init
From: Gerd Hoffmann @ 2018-08-10 6:03 UTC (permalink / raw)
To: Anton Vasilyev
Cc: ldv-project, David Airlie, linux-kernel, dri-devel,
virtualization, Dave Airlie
In-Reply-To: <20180727115440.11112-1-vasilyev@ispras.ru>
On Fri, Jul 27, 2018 at 02:54:40PM +0300, Anton Vasilyev wrote:
> If qxl_device_init fails on creating resources and does not report it,
> then qxl module will catch null pointer exception on remove, or on
> probe's error path.
>
> The patch adds error path with resources release into qxl_device_init.
>
> Found by Linux Driver Verification project (linuxtesting.org).
Pushed to drm-misc-next.
thanks,
Gerd
^ permalink raw reply
* Re: [PATCH] drm/cirrus: Use drm_framebuffer_put to avoid kernel oops in clean-up
From: Gerd Hoffmann @ 2018-08-10 6:03 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: airlied, dri-devel, virtualization
In-Reply-To: <20180720112743.27159-1-tzimmermann@suse.de>
On Fri, Jul 20, 2018 at 01:27:43PM +0200, Thomas Zimmermann wrote:
> In the Cirrus driver, the regular clean-up code also performs the clean-up
> of a failed initialization. If the fbdev's framebuffer was not initialized,
> the clean-up will fail within drm_framebuffer_unregister_private. Booting
> with cirrus.bpp=16 triggers this bug.
>
> The framebuffer is currently stored directly within struct cirrus_fbdev. To
> fix the bug, we turn it into a pointer that is only set for initialized
> framebuffers. The fbdev's clean-up code skips uninitialized framebuffers.
>
> The memory for struct drm_framebuffer is allocated dynamically. This requires
> additional error handling within cirrusfb_create. The framebuffer clean-up is
> now performed by drm_framebuffer_put, which also frees the data strcuture's
> memory.
pushed to drm-misc-next (also the other ones, except the failing ttm_put
patches).
thanks,
Gerd
^ permalink raw reply
* Re: [PATCH] drm/qxl: Replace ttm_bo_unref with ttm_bo_put
From: Gerd Hoffmann @ 2018-08-09 15:30 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: airlied, dri-devel, virtualization
In-Reply-To: <20180731063559.11629-1-tzimmermann@suse.de>
> diff --git a/drivers/gpu/drm/qxl/qxl_gem.c b/drivers/gpu/drm/qxl/qxl_gem.c
> index f5c1e7872e92..89606c819d82 100644
> --- a/drivers/gpu/drm/qxl/qxl_gem.c
> +++ b/drivers/gpu/drm/qxl/qxl_gem.c
> @@ -40,7 +40,7 @@ void qxl_gem_object_free(struct drm_gem_object *gobj)
> qxl_surface_evict(qdev, qobj, false);
>
> tbo = &qobj->tbo;
> - ttm_bo_unref(&tbo);
> + ttm_bo_put(tbo);
Same here (using drm-misc-next btw).
cheers,
Gerd
^ permalink raw reply
* Re: [PATCH] drm/cirrus: Replace ttm_bo_unref with ttm_bo_put
From: Gerd Hoffmann @ 2018-08-09 15:29 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: airlied, dri-devel, virtualization
In-Reply-To: <20180731063128.11041-1-tzimmermann@suse.de>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
> drivers/gpu/drm/cirrus/cirrus_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/cirrus/cirrus_main.c b/drivers/gpu/drm/cirrus/cirrus_main.c
> index 60d54e10a34d..57f8fe6d020b 100644
> --- a/drivers/gpu/drm/cirrus/cirrus_main.c
> +++ b/drivers/gpu/drm/cirrus/cirrus_main.c
> @@ -269,7 +269,7 @@ static void cirrus_bo_unref(struct cirrus_bo **bo)
> return;
>
> tbo = &((*bo)->bo);
> - ttm_bo_unref(&tbo);
> + ttm_bo_put(tbo);
Fails to build too.
cheers,
Gerd
^ permalink raw reply
* Re: [PATCH] drm/bochs: Replace ttm_bo_unref with ttm_bo_put
From: Gerd Hoffmann @ 2018-08-09 15:27 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: dri-devel, virtualization
In-Reply-To: <20180731062851.10812-1-tzimmermann@suse.de>
> diff --git a/drivers/gpu/drm/bochs/bochs_mm.c b/drivers/gpu/drm/bochs/bochs_mm.c
> index 39cd08416773..c9c7097030ca 100644
> --- a/drivers/gpu/drm/bochs/bochs_mm.c
> +++ b/drivers/gpu/drm/bochs/bochs_mm.c
> @@ -430,7 +430,7 @@ static void bochs_bo_unref(struct bochs_bo **bo)
> return;
>
> tbo = &((*bo)->bo);
> - ttm_bo_unref(&tbo);
> + ttm_bo_put(tbo);
fails to build:
CC [M] drivers/gpu/drm/bochs/bochs_mm.o
/home/kraxel/projects/linux/drivers/gpu/drm/bochs/bochs_mm.c: In function ‘bochs_bo_unref’:
/home/kraxel/projects/linux/drivers/gpu/drm/bochs/bochs_mm.c:433:2: error: implicit declaration of function ‘ttm_bo_put’ [-Werror=implicit-function-declaration]
ttm_bo_put(tbo);
^
cc1: some warnings being treated as errors
cheers,
Gerd
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC 0/4] Virtio uses DMA API for all devices
From: Christoph Hellwig @ 2018-08-09 5:40 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: robh, srikar, Michael S. Tsirkin, mpe, Will Deacon, linux-kernel,
linuxram, virtualization, Christoph Hellwig,
jean-philippe.brucker, paulus, marc.zyngier, joe, robin.murphy,
david, linuxppc-dev, elfring, haren, Anshuman Khandual
In-Reply-To: <98eb367ce322ad84baa31e3c7beffc4a42be8458.camel@kernel.crashing.org>
On Thu, Aug 09, 2018 at 08:13:32AM +1000, Benjamin Herrenschmidt wrote:
> > > - if (xen_domain())
> > > + if (xen_domain() || pseries_secure_vm())
> > > return true;
> >
> > I don't think it's pseries specific actually. E.g. I suspect AMD SEV
> > might benefit from the same kind of hack.
>
> As long as they can provide the same guarantee that the DMA ops are
> completely equivalent between virtio and other PCI devices, at least on
> the same bus, ie, we don't have to go hack special DMA ops.
>
> I think the latter is really what Christoph wants to avoid for good
> reasons.
Yes. I also generally want to avoid too much arch specific magic.
FYI, I'm off to a week-long vacation today, don't expect quick replies.
^ permalink raw reply
* Re: [RFC 0/4] Virtio uses DMA API for all devices
From: Benjamin Herrenschmidt @ 2018-08-09 2:00 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: robh, srikar, mpe, Will Deacon, linux-kernel, linuxram,
virtualization, Christoph Hellwig, jean-philippe.brucker, paulus,
marc.zyngier, joe, robin.murphy, david, linuxppc-dev, elfring,
haren, Anshuman Khandual
In-Reply-To: <98eb367ce322ad84baa31e3c7beffc4a42be8458.camel@kernel.crashing.org>
On Thu, 2018-08-09 at 08:13 +1000, Benjamin Herrenschmidt wrote:
> > For completeness, virtio could also have its own bounce buffer
> > outside of DMA API one. I don't see lots of benefits to this
> > though.
>
> Not fan of that either...
To elaborate a bit ...
For our secure VMs, we will need bounce buffering for everything
anyway. virtio, emulated PCI, or vfio.
By ensuring that we create an identity mapping in the IOMMU for
the bounce buffering pool, we enable virtio "legacy/direct" to
use the same mapping ops as things using the iommu.
That said, we still need somewhere in arch/powerpc a set of dma
ops which we'll attach to all PCI devices of a secure VM to force
bouncing always, rather than just based on address (which is what
the standard swiotlb ones do)... Unless we can tweak the swiotlb
"threshold" for example by using an empty mask.
We'll need the same set of DMA ops for VIO devices too, not just PCI.
Cheers,
Ben.
^ permalink raw reply
* Re: [RFC 0/4] Virtio uses DMA API for all devices
From: Benjamin Herrenschmidt @ 2018-08-08 22:13 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: robh, srikar, mpe, Will Deacon, linux-kernel, linuxram,
virtualization, Christoph Hellwig, jean-philippe.brucker, paulus,
marc.zyngier, joe, robin.murphy, david, linuxppc-dev, elfring,
haren, Anshuman Khandual
In-Reply-To: <20180808232210-mutt-send-email-mst@kernel.org>
On Wed, 2018-08-08 at 23:31 +0300, Michael S. Tsirkin wrote:
> On Wed, Aug 08, 2018 at 11:18:13PM +1000, Benjamin Herrenschmidt wrote:
> > Sure, but all of this is just the configuration of the iommu. But I
> > think we agree here, and your point remains valid, indeed my proposed
> > hack:
> >
> > > if ((flags & VIRTIO_F_IOMMU_PLATFORM) || arch_virtio_wants_dma_ops())
> >
> > Will only work if the IOMMU and non-IOMMU path are completely equivalent.
> >
> > We can provide that guarantee for our secure VM case, but not generally so if
> > we were to go down the route of a quirk in virtio, it might be better to
> > make it painfully obvious that it's specific to that one case with a different
> > kind of turd:
> >
> > - if (xen_domain())
> > + if (xen_domain() || pseries_secure_vm())
> > return true;
>
> I don't think it's pseries specific actually. E.g. I suspect AMD SEV
> might benefit from the same kind of hack.
As long as they can provide the same guarantee that the DMA ops are
completely equivalent between virtio and other PCI devices, at least on
the same bus, ie, we don't have to go hack special DMA ops.
I think the latter is really what Christoph wants to avoid for good
reasons.
> > So to summarize, and make sure I'm not missing something, the two approaches
> > at hand are either:
> >
> > 1- The above, which is a one liner and contained in the guest, so that's nice, but
> > also means another turd in virtio which isn't ...
> >
> > 2- We force pseries to always set VIRTIO_F_IOMMU_PLATFORM, but with the current
> > architecture on our side that will force virtio to always go through an emulated
> > iommu, as pseries doesn't have the concept of a real bypass window, and thus will
> > impact performance for both secure and non-secure VMs.
> >
> > 3- Invent a property that can be put in selected PCI device tree nodes that
> > indicates that for that device specifically, the iommu can be bypassed, along with
> > a hypercall to turn that bypass on/off. Virtio would then use VIRTIO_F_IOMMU_PLATFORM
> > but its DT nodes would also have that property and Linux would notice it and turn
> > bypass on.
>
> For completeness, virtio could also have its own bounce buffer
> outside of DMA API one. I don't see lots of benefits to this
> though.
Not fan of that either...
> > The resulting properties of those options are:
> >
> > 1- Is what I want because it's the simplest, provides the best performance now,
> > and works without code changes to qemu or non-secure Linux. However it does
> > add a tiny turd to virtio which is annoying.
> >
> > 2- This works but it puts the iommu in the way always, thus reducing virtio performance
> > accross the board for pseries unless we only do that for secure VMs but that is
> > difficult (as discussed earlier).
> >
> > 3- This would recover the performance lost in -2-, however it requires qemu *and*
> > guest changes. Specifically, existing guests (RHEL 7 etc...) would get the
> > performance hit of -2- unless modified to call that 'enable bypass' call, which
> > isn't great.
> >
> > So imho we have to chose one of 3 not-great solutions here... Unless I missed
> > something in your ideas of course.
> >
^ permalink raw reply
* Re: [PATCH] vhost/scsi: increase VHOST_SCSI_PREALLOC_PROT_SGLS to 2048
From: Michael S. Tsirkin @ 2018-08-08 20:42 UTC (permalink / raw)
To: Greg Edwards; +Cc: pbonzini, virtualization
In-Reply-To: <20180808192955.1115-1-gedwards@ddn.com>
On Wed, Aug 08, 2018 at 01:29:55PM -0600, Greg Edwards wrote:
> The current value of VHOST_SCSI_PREALLOC_PROT_SGLS is too small to
> accommodate larger I/Os, e.g. 16-32 MiB, when the VIRTIO_SCSI_F_T10_PI
> feature bit is negotiated and the backing store supports T10 PI.
>
> vhost-scsi rejects the command with errors like:
>
> [ 59.581317] vhost_scsi_calc_sgls: requested sgl_count: 1820 exceeds pre-allocated max_sgls: 512
>
> Signed-off-by: Greg Edwards <gedwards@ddn.com>
> ---
> drivers/vhost/scsi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index 17fcd3b2e686..8c32cf58d6fa 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -56,7 +56,7 @@
> #define VHOST_SCSI_DEFAULT_TAGS 256
> #define VHOST_SCSI_PREALLOC_SGLS 2048
> #define VHOST_SCSI_PREALLOC_UPAGES 2048
> -#define VHOST_SCSI_PREALLOC_PROT_SGLS 512
> +#define VHOST_SCSI_PREALLOC_PROT_SGLS 2048
>
> struct vhost_scsi_inflight {
> /* Wait for the flush operation to finish */
I guess it's ok since PREALLOC_SGLS is already 2K ... or
am I missing something. Paolo, any input on this?
> --
> 2.17.1
^ permalink raw reply
* Re: [RFC 0/4] Virtio uses DMA API for all devices
From: Michael S. Tsirkin @ 2018-08-08 20:31 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: robh, srikar, mpe, Will Deacon, linux-kernel, linuxram,
virtualization, Christoph Hellwig, jean-philippe.brucker, paulus,
marc.zyngier, joe, robin.murphy, david, linuxppc-dev, elfring,
haren, Anshuman Khandual
In-Reply-To: <b8b9150a747453c070ad3b0e4c92d2b1b052ad06.camel@kernel.crashing.org>
On Wed, Aug 08, 2018 at 11:18:13PM +1000, Benjamin Herrenschmidt wrote:
> Sure, but all of this is just the configuration of the iommu. But I
> think we agree here, and your point remains valid, indeed my proposed
> hack:
>
> > if ((flags & VIRTIO_F_IOMMU_PLATFORM) || arch_virtio_wants_dma_ops())
>
> Will only work if the IOMMU and non-IOMMU path are completely equivalent.
>
> We can provide that guarantee for our secure VM case, but not generally so if
> we were to go down the route of a quirk in virtio, it might be better to
> make it painfully obvious that it's specific to that one case with a different
> kind of turd:
>
> - if (xen_domain())
> + if (xen_domain() || pseries_secure_vm())
> return true;
I don't think it's pseries specific actually. E.g. I suspect AMD SEV
might benefit from the same kind of hack.
> So to summarize, and make sure I'm not missing something, the two approaches
> at hand are either:
>
> 1- The above, which is a one liner and contained in the guest, so that's nice, but
> also means another turd in virtio which isn't ...
>
> 2- We force pseries to always set VIRTIO_F_IOMMU_PLATFORM, but with the current
> architecture on our side that will force virtio to always go through an emulated
> iommu, as pseries doesn't have the concept of a real bypass window, and thus will
> impact performance for both secure and non-secure VMs.
>
> 3- Invent a property that can be put in selected PCI device tree nodes that
> indicates that for that device specifically, the iommu can be bypassed, along with
> a hypercall to turn that bypass on/off. Virtio would then use VIRTIO_F_IOMMU_PLATFORM
> but its DT nodes would also have that property and Linux would notice it and turn
> bypass on.
For completeness, virtio could also have its own bounce buffer
outside of DMA API one. I don't see lots of benefits to this
though.
> The resulting properties of those options are:
>
> 1- Is what I want because it's the simplest, provides the best performance now,
> and works without code changes to qemu or non-secure Linux. However it does
> add a tiny turd to virtio which is annoying.
>
> 2- This works but it puts the iommu in the way always, thus reducing virtio performance
> accross the board for pseries unless we only do that for secure VMs but that is
> difficult (as discussed earlier).
>
> 3- This would recover the performance lost in -2-, however it requires qemu *and*
> guest changes. Specifically, existing guests (RHEL 7 etc...) would get the
> performance hit of -2- unless modified to call that 'enable bypass' call, which
> isn't great.
>
> So imho we have to chose one of 3 not-great solutions here... Unless I missed
> something in your ideas of course.
>
> Cheers,
> Ben.
>
>
^ permalink raw reply
* Re: [PATCH net] vhost: reset metadata cache when initializing new IOTLB
From: David Miller @ 2018-08-08 16:45 UTC (permalink / raw)
To: jasowang; +Cc: netdev, virtualization, linux-kernel, kvm, mst
In-Reply-To: <1533699784-4950-1-git-send-email-jasowang@redhat.com>
From: Jason Wang <jasowang@redhat.com>
Date: Wed, 8 Aug 2018 11:43:04 +0800
> We need to reset metadata cache during new IOTLB initialization,
> otherwise the stale pointers to previous IOTLB may be still accessed
> which will lead a use after free.
>
> Reported-by: syzbot+c51e6736a1bf614b3272@syzkaller.appspotmail.com
> Fixes: f88949138058 ("vhost: introduce O(1) vq metadata cache")
> Signed-off-by: Jason Wang <jasowang@redhat.com>
Applied and queued up for -stable, thanks Jason.
^ permalink raw reply
* Re: [RFC 0/4] Virtio uses DMA API for all devices
From: Benjamin Herrenschmidt @ 2018-08-08 13:18 UTC (permalink / raw)
To: Christoph Hellwig
Cc: robh, srikar, Michael S. Tsirkin, mpe, Will Deacon, linux-kernel,
linuxram, virtualization, jean-philippe.brucker, paulus,
marc.zyngier, joe, robin.murphy, david, linuxppc-dev, elfring,
haren, Anshuman Khandual
In-Reply-To: <20180808123036.GA2525@infradead.org>
On Wed, 2018-08-08 at 05:30 -0700, Christoph Hellwig wrote:
> On Wed, Aug 08, 2018 at 08:07:49PM +1000, Benjamin Herrenschmidt wrote:
> > Qemu virtio bypasses that iommu when the VIRTIO_F_IOMMU_PLATFORM flag
> > is not set (default) but there's nothing in the device-tree to tell the
> > guest about this since it's a violation of our pseries architecture, so
> > we just rely on Linux virtio "knowing" that it happens. It's a bit
> > yucky but that's now history...
>
> That is ugly as hell, but it is how virtio works everywhere, so nothing
> special so far.
Yup.
> > Essentially pseries "architecturally" does not have the concept of not
> > having an iommu in the way and qemu violates that architecture today.
> >
> > (Remember it comes from pHyp, our priorietary HV, which we are somewhat
> > mimmicing here).
>
> It shouldnt be too hard to have a dt property that communicates this,
> should it?
We could invent something I suppose. The additional problem then (yeah
I know ... what a mess) is that qemu doesn't create the DT for PCI
devices, the firmware (SLOF) inside the guest does using normal PCI
probing.
That said, that FW could know about all the virtio vendor/device IDs,
check the VIRTIO_F_IOMMU_PLATFORM and set that property accordingly...
messy but doable. It's not a bus property (see my other reply below as
this could complicate things with your bus mask).
But we are drifting from the problem at hand :-) You propose we do set
VIRTIO_F_IOMMU_PLATFORM so we aren't in the above case, and the bypass
stuff works, so no need to touch it.
See my recap at the end of the email to make sure I understand fully
what you suggest.
> > So if we always set VIRTIO_F_IOMMU_PLATFORM, it *will* force all virtio
> > through that iommu and performance will suffer (esp vhost I suspect),
> > especially since adding/removing translations in the iommu is a
> > hypercall.
> Well, we'd nee to make sure that for this particular bus we skip the
> actualy iommu.
It's not a bus property. Qemu will happily mix up everything on the
same bus, that includes emulated devices that go through the emulated
iommu, real VFIO devices that go through an actual HW iommu and virtio
that bypasses everything.
This makes things tricky in general (not just in my powerpc secure VM
case) since, at least on powerpc but I suppose elsewhere too, iommu
related properties tend to be per "bus" while here, qemu will mix and
match.
But again, I think we are drifting away from the topic, see below
> > > It would not be the same effect. The problem with that is that you must
> > > now assumes that your qemu knows that for example you might be passing
> > > a dma offset if the bus otherwise requires it.
> >
> > I would assume that arch_virtio_wants_dma_ops() only returns true when
> > no such offsets are involved, at least in our case that would be what
> > happens.
>
> That would work, but we're really piling hacĸs ontop of hacks here.
Sort-of :-) At least none of what we are discussing now involves
touching the dma_ops themselves so we are not in the way of your big
cleanup operation here. But yeah, let's continue discussing your other
solution below.
> > > Or in other words:
> > > you potentially break the contract between qemu and the guest of always
> > > passing down physical addresses. If we explicitly change that contract
> > > through using a flag that says you pass bus address everything is fine.
> >
> > For us a "bus address" is behind the iommu so that's what
> > VIRTIO_F_IOMMU_PLATFORM does already. We don't have the concept of a
> > bus address that is different. I suppose it's an ARMism to have DMA
> > offsets that are separate from iommus ?
>
> No, a lot of platforms support a bus address that has an offset from
> the physical address. including a lot of power platforms:
Ok, just talking past each other :-) For all the powerpc ones, these
*do* go through the iommu, which is what I meant. It's just a window of
the iommu that provides some kind of direct mapping of memory.
For pseries, there is no such thing however. What we do to avoid
constant map/unmap of iommu PTEs in pseries guests is that we use
hypercalls to create a 64-bit window and populate all its PTEs with an
identity mapping. But that's not as efficient as a real bypass.
There are good historical reasons for that, since pseries is a guest
platform, its memory is never really where the guest thinks it is, so
you always need an iommu to remap. Even for virtual devices, since for
most of them, in the "IBM" pHyp model, the "peer" is actually another
partition, so the virtual iommu handles translating accross the two
partitions.
Same goes with cell in HW, no real bypass, just the iommu being
confiured with very large pages and a fixed mapping.
powernv has a separate physical window that can be configured as a real
bypass though, so does the U4 DART. Not sure about the FSL one.
But yeah, your point stands, this is just implementation details.
> arch/powerpc/kernel/pci-common.c: set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
> arch/powerpc/platforms/cell/iommu.c: set_dma_offset(dev, cell_dma_nommu_offset);
> arch/powerpc/platforms/cell/iommu.c: set_dma_offset(dev, addr);
> arch/powerpc/platforms/powernv/pci-ioda.c: set_dma_offset(&pdev->dev, pe->tce_bypass_base);
> arch/powerpc/platforms/powernv/pci-ioda.c: set_dma_offset(&pdev->dev, (1ULL << 32));
> arch/powerpc/platforms/powernv/pci-ioda.c: set_dma_offset(&dev->dev, pe->tce_bypass_base);
> arch/powerpc/platforms/pseries/iommu.c: set_dma_offset(dev, dma_offset);
> arch/powerpc/sysdev/dart_iommu.c: set_dma_offset(&dev->dev, DART_U4_BYPASS_BASE);
> arch/powerpc/sysdev/fsl_pci.c: set_dma_offset(dev, pci64_dma_offset);
>
> to make things worse some platforms (at least on arm/arm64/mips/x86) can
> also require additional banking where it isn't even a single linear map
> but multiples windows.
Sure, but all of this is just the configuration of the iommu. But I
think we agree here, and your point remains valid, indeed my proposed
hack:
> if ((flags & VIRTIO_F_IOMMU_PLATFORM) || arch_virtio_wants_dma_ops())
Will only work if the IOMMU and non-IOMMU path are completely equivalent.
We can provide that guarantee for our secure VM case, but not generally so if
we were to go down the route of a quirk in virtio, it might be better to
make it painfully obvious that it's specific to that one case with a different
kind of turd:
- if (xen_domain())
+ if (xen_domain() || pseries_secure_vm())
return true;
So to summarize, and make sure I'm not missing something, the two approaches
at hand are either:
1- The above, which is a one liner and contained in the guest, so that's nice, but
also means another turd in virtio which isn't ...
2- We force pseries to always set VIRTIO_F_IOMMU_PLATFORM, but with the current
architecture on our side that will force virtio to always go through an emulated
iommu, as pseries doesn't have the concept of a real bypass window, and thus will
impact performance for both secure and non-secure VMs.
3- Invent a property that can be put in selected PCI device tree nodes that
indicates that for that device specifically, the iommu can be bypassed, along with
a hypercall to turn that bypass on/off. Virtio would then use VIRTIO_F_IOMMU_PLATFORM
but its DT nodes would also have that property and Linux would notice it and turn
bypass on.
The resulting properties of those options are:
1- Is what I want because it's the simplest, provides the best performance now,
and works without code changes to qemu or non-secure Linux. However it does
add a tiny turd to virtio which is annoying.
2- This works but it puts the iommu in the way always, thus reducing virtio performance
accross the board for pseries unless we only do that for secure VMs but that is
difficult (as discussed earlier).
3- This would recover the performance lost in -2-, however it requires qemu *and*
guest changes. Specifically, existing guests (RHEL 7 etc...) would get the
performance hit of -2- unless modified to call that 'enable bypass' call, which
isn't great.
So imho we have to chose one of 3 not-great solutions here... Unless I missed
something in your ideas of course.
Cheers,
Ben.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC 0/4] Virtio uses DMA API for all devices
From: Christoph Hellwig @ 2018-08-08 12:30 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: robh, srikar, Michael S. Tsirkin, mpe, Will Deacon, linux-kernel,
linuxram, virtualization, Christoph Hellwig,
jean-philippe.brucker, paulus, marc.zyngier, joe, robin.murphy,
david, linuxppc-dev, elfring, haren, Anshuman Khandual
In-Reply-To: <4b596883892b5cb5560bef26fcd249e7107173ac.camel@kernel.crashing.org>
On Wed, Aug 08, 2018 at 08:07:49PM +1000, Benjamin Herrenschmidt wrote:
> Qemu virtio bypasses that iommu when the VIRTIO_F_IOMMU_PLATFORM flag
> is not set (default) but there's nothing in the device-tree to tell the
> guest about this since it's a violation of our pseries architecture, so
> we just rely on Linux virtio "knowing" that it happens. It's a bit
> yucky but that's now history...
That is ugly as hell, but it is how virtio works everywhere, so nothing
special so far.
> Essentially pseries "architecturally" does not have the concept of not
> having an iommu in the way and qemu violates that architecture today.
>
> (Remember it comes from pHyp, our priorietary HV, which we are somewhat
> mimmicing here).
It shouldnt be too hard to have a dt property that communicates this,
should it?
> So if we always set VIRTIO_F_IOMMU_PLATFORM, it *will* force all virtio
> through that iommu and performance will suffer (esp vhost I suspect),
> especially since adding/removing translations in the iommu is a
> hypercall.
Well, we'd nee to make sure that for this particular bus we skip the
actualy iommu.
> > It would not be the same effect. The problem with that is that you must
> > now assumes that your qemu knows that for example you might be passing
> > a dma offset if the bus otherwise requires it.
>
> I would assume that arch_virtio_wants_dma_ops() only returns true when
> no such offsets are involved, at least in our case that would be what
> happens.
That would work, but we're really piling hacĸs ontop of hacks here.
> > Or in other words:
> > you potentially break the contract between qemu and the guest of always
> > passing down physical addresses. If we explicitly change that contract
> > through using a flag that says you pass bus address everything is fine.
>
> For us a "bus address" is behind the iommu so that's what
> VIRTIO_F_IOMMU_PLATFORM does already. We don't have the concept of a
> bus address that is different. I suppose it's an ARMism to have DMA
> offsets that are separate from iommus ?
No, a lot of platforms support a bus address that has an offset from
the physical address. including a lot of power platforms:
arch/powerpc/kernel/pci-common.c: set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
arch/powerpc/platforms/cell/iommu.c: set_dma_offset(dev, cell_dma_nommu_offset);
arch/powerpc/platforms/cell/iommu.c: set_dma_offset(dev, addr);
arch/powerpc/platforms/powernv/pci-ioda.c: set_dma_offset(&pdev->dev, pe->tce_bypass_base);
arch/powerpc/platforms/powernv/pci-ioda.c: set_dma_offset(&pdev->dev, (1ULL << 32));
arch/powerpc/platforms/powernv/pci-ioda.c: set_dma_offset(&dev->dev, pe->tce_bypass_base);
arch/powerpc/platforms/pseries/iommu.c: set_dma_offset(dev, dma_offset);
arch/powerpc/sysdev/dart_iommu.c: set_dma_offset(&dev->dev, DART_U4_BYPASS_BASE);
arch/powerpc/sysdev/fsl_pci.c: set_dma_offset(dev, pci64_dma_offset);
to make things worse some platforms (at least on arm/arm64/mips/x86) can
also require additional banking where it isn't even a single linear map
but multiples windows.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH v3] drm/cirrus: flip default from 24bpp to 16bpp
From: Gerd Hoffmann @ 2018-08-08 11:13 UTC (permalink / raw)
To: dri-devel
Cc: David Airlie, Dave Airlie, open list,
open list:DRM DRIVER FOR QEMU'S CIRRUS DEVICE
The problem with 24bpp is that it is a rather unusual depth these days,
cirrus is pretty much the only relevant device still using that, and it
is a endless source of issues. Wayland doesn't support it at all. Bugs
in Xorg keep showing up.
Typically either 32bpp or 16bpp are used. Using 32bpp would limit the
resolution to 800x600 due to hardware constrains. So lets go with 16bpp.
Also use the default depth for the framebuffer console and
mode_info->preferred_depth.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
drivers/gpu/drm/cirrus/cirrus_drv.c | 4 ++--
drivers/gpu/drm/cirrus/cirrus_fbdev.c | 3 +--
drivers/gpu/drm/cirrus/cirrus_mode.c | 2 +-
3 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/cirrus/cirrus_drv.c b/drivers/gpu/drm/cirrus/cirrus_drv.c
index 69c4e352dd..b4f7be056c 100644
--- a/drivers/gpu/drm/cirrus/cirrus_drv.c
+++ b/drivers/gpu/drm/cirrus/cirrus_drv.c
@@ -16,11 +16,11 @@
#include "cirrus_drv.h"
int cirrus_modeset = -1;
-int cirrus_bpp = 24;
+int cirrus_bpp = 16;
MODULE_PARM_DESC(modeset, "Disable/Enable modesetting");
module_param_named(modeset, cirrus_modeset, int, 0400);
-MODULE_PARM_DESC(bpp, "Max bits-per-pixel (default:24)");
+MODULE_PARM_DESC(bpp, "Max bits-per-pixel (default:16)");
module_param_named(bpp, cirrus_bpp, int, 0400);
/*
diff --git a/drivers/gpu/drm/cirrus/cirrus_fbdev.c b/drivers/gpu/drm/cirrus/cirrus_fbdev.c
index 32fbfba2c6..b329a54290 100644
--- a/drivers/gpu/drm/cirrus/cirrus_fbdev.c
+++ b/drivers/gpu/drm/cirrus/cirrus_fbdev.c
@@ -271,7 +271,6 @@ int cirrus_fbdev_init(struct cirrus_device *cdev)
{
struct cirrus_fbdev *gfbdev;
int ret;
- int bpp_sel = 24;
/*bpp_sel = 8;*/
gfbdev = kzalloc(sizeof(struct cirrus_fbdev), GFP_KERNEL);
@@ -296,7 +295,7 @@ int cirrus_fbdev_init(struct cirrus_device *cdev)
/* disable all the possible outputs/crtcs before entering KMS mode */
drm_helper_disable_unused_functions(cdev->dev);
- return drm_fb_helper_initial_config(&gfbdev->helper, bpp_sel);
+ return drm_fb_helper_initial_config(&gfbdev->helper, cirrus_bpp);
}
void cirrus_fbdev_fini(struct cirrus_device *cdev)
diff --git a/drivers/gpu/drm/cirrus/cirrus_mode.c b/drivers/gpu/drm/cirrus/cirrus_mode.c
index c91b9b054e..91128b1d8d 100644
--- a/drivers/gpu/drm/cirrus/cirrus_mode.c
+++ b/drivers/gpu/drm/cirrus/cirrus_mode.c
@@ -518,7 +518,7 @@ int cirrus_modeset_init(struct cirrus_device *cdev)
cdev->dev->mode_config.max_height = CIRRUS_MAX_FB_HEIGHT;
cdev->dev->mode_config.fb_base = cdev->mc.vram_base;
- cdev->dev->mode_config.preferred_depth = 24;
+ cdev->dev->mode_config.preferred_depth = cirrus_bpp;
/* don't prefer a shadow on virt GPU */
cdev->dev->mode_config.prefer_shadow = 0;
--
2.9.3
^ permalink raw reply related
* Re: [RFC 0/4] Virtio uses DMA API for all devices
From: Benjamin Herrenschmidt @ 2018-08-08 10:07 UTC (permalink / raw)
To: Christoph Hellwig
Cc: robh, srikar, Michael S. Tsirkin, mpe, Will Deacon, linux-kernel,
linuxram, virtualization, jean-philippe.brucker, paulus,
marc.zyngier, joe, robin.murphy, david, linuxppc-dev, elfring,
haren, Anshuman Khandual
In-Reply-To: <20180808063158.GA2474@infradead.org>
On Tue, 2018-08-07 at 23:31 -0700, Christoph Hellwig wrote:
>
> You don't need to set them the time you go secure. You just need to
> set the flag from the beginning on any VM you might want to go secure.
> Or for simplicity just any VM - if the DT/ACPI tables exposed by
> qemu are good enough that will always exclude a iommu and not set a
> DMA offset, so nothing will change on the qemu side of he processing,
> and with the new direct calls for the direct dma ops performance in
> the guest won't change either.
So that's where I'm not sure things are "good enough" due to how
pseries works. (remember it's paravirtualized).
A pseries system starts with a default iommu on all devices, that uses
translation using 4k entires with a "pinhole" window (usually 2G with
qemu iirc). There's no "pass through" by default.
Qemu virtio bypasses that iommu when the VIRTIO_F_IOMMU_PLATFORM flag
is not set (default) but there's nothing in the device-tree to tell the
guest about this since it's a violation of our pseries architecture, so
we just rely on Linux virtio "knowing" that it happens. It's a bit
yucky but that's now history...
Essentially pseries "architecturally" does not have the concept of not
having an iommu in the way and qemu violates that architecture today.
(Remember it comes from pHyp, our priorietary HV, which we are somewhat
mimmicing here).
So if we always set VIRTIO_F_IOMMU_PLATFORM, it *will* force all virtio
through that iommu and performance will suffer (esp vhost I suspect),
especially since adding/removing translations in the iommu is a
hypercall.
Now, we do have HV APIs to create a second window that's "permanently
mapped" to the guest memory, thus avoiding dynamic map/unmaps, and
Linux can make use of this but I don't know if that works with qemu and
the performance impact with vhost.
So the situation isn't that great.... On the other hand, I think the
other approach works for us:
> > It's nicer if we have a way in the guest virtio driver to do something
> > along the lines of
> >
> > if ((flags & VIRTIO_F_IOMMU_PLATFORM) || arch_virtio_wants_dma_ops())
> >
> > Which would have the same effect and means the issue is entirely
> > contained in the guest.
>
> It would not be the same effect. The problem with that is that you must
> now assumes that your qemu knows that for example you might be passing
> a dma offset if the bus otherwise requires it.
I would assume that arch_virtio_wants_dma_ops() only returns true when
no such offsets are involved, at least in our case that would be what
happens.
> Or in other words:
> you potentially break the contract between qemu and the guest of always
> passing down physical addresses. If we explicitly change that contract
> through using a flag that says you pass bus address everything is fine.
For us a "bus address" is behind the iommu so that's what
VIRTIO_F_IOMMU_PLATFORM does already. We don't have the concept of a
bus address that is different. I suppose it's an ARMism to have DMA
offsets that are separate from iommus ?
> Note that in practice your scheme will probably just work for your
> initial prototype, but chances are it will get us in trouble later on.
Not on pseries, at least not in any way I can think of mind you... but
maybe other architectures would abuse it... We could add a WARN_ON if
that calls returns true on a bus with an offset I suppose.
Cheers,
Ben.
^ permalink raw reply
* [PATCH v2] drm/cirrus: flip default from 24bpp to 16bpp
From: Gerd Hoffmann @ 2018-08-08 9:06 UTC (permalink / raw)
To: dri-devel
Cc: David Airlie, Dave Airlie, open list,
open list:DRM DRIVER FOR QEMU'S CIRRUS DEVICE
The problem with 24bpp is that it is a rather unusual depth these days,
cirrus is pretty much the only relevant device still using that, and it
is a endless source of issues. Wayland doesn't support it at all. Bugs
in Xorg keep showing up.
Typically either 32bpp or 16bpp are used. Using 32bpp would limit the
resolution to 800x600 due to hardware constrains. So lets go with 16bpp.
Also use the default depth for the framebuffer console.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
drivers/gpu/drm/cirrus/cirrus_drv.c | 4 ++--
drivers/gpu/drm/cirrus/cirrus_fbdev.c | 3 +--
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/cirrus/cirrus_drv.c b/drivers/gpu/drm/cirrus/cirrus_drv.c
index 69c4e352dd..b4f7be056c 100644
--- a/drivers/gpu/drm/cirrus/cirrus_drv.c
+++ b/drivers/gpu/drm/cirrus/cirrus_drv.c
@@ -16,11 +16,11 @@
#include "cirrus_drv.h"
int cirrus_modeset = -1;
-int cirrus_bpp = 24;
+int cirrus_bpp = 16;
MODULE_PARM_DESC(modeset, "Disable/Enable modesetting");
module_param_named(modeset, cirrus_modeset, int, 0400);
-MODULE_PARM_DESC(bpp, "Max bits-per-pixel (default:24)");
+MODULE_PARM_DESC(bpp, "Max bits-per-pixel (default:16)");
module_param_named(bpp, cirrus_bpp, int, 0400);
/*
diff --git a/drivers/gpu/drm/cirrus/cirrus_fbdev.c b/drivers/gpu/drm/cirrus/cirrus_fbdev.c
index 32fbfba2c6..b329a54290 100644
--- a/drivers/gpu/drm/cirrus/cirrus_fbdev.c
+++ b/drivers/gpu/drm/cirrus/cirrus_fbdev.c
@@ -271,7 +271,6 @@ int cirrus_fbdev_init(struct cirrus_device *cdev)
{
struct cirrus_fbdev *gfbdev;
int ret;
- int bpp_sel = 24;
/*bpp_sel = 8;*/
gfbdev = kzalloc(sizeof(struct cirrus_fbdev), GFP_KERNEL);
@@ -296,7 +295,7 @@ int cirrus_fbdev_init(struct cirrus_device *cdev)
/* disable all the possible outputs/crtcs before entering KMS mode */
drm_helper_disable_unused_functions(cdev->dev);
- return drm_fb_helper_initial_config(&gfbdev->helper, bpp_sel);
+ return drm_fb_helper_initial_config(&gfbdev->helper, cirrus_bpp);
}
void cirrus_fbdev_fini(struct cirrus_device *cdev)
--
2.9.3
^ permalink raw reply related
* Re: [PATCH] drm/cirrus: flip default to 32bpp
From: Gerd Hoffmann @ 2018-08-08 8:46 UTC (permalink / raw)
To: Adam Jackson, dri-devel, David Airlie, Dave Airlie, open list,
open list:DRM DRIVER FOR QEMU'S CIRRUS DEVICE
In-Reply-To: <20180709073924.GB3008@phenom.ffwll.local>
On Mon, Jul 09, 2018 at 09:39:24AM +0200, Daniel Vetter wrote:
> On Fri, Jul 06, 2018 at 02:35:07PM -0400, Adam Jackson wrote:
> > On Fri, 2018-07-06 at 11:12 +0200, Gerd Hoffmann wrote:
> > > cirrus can handle 1024x768 (and slightly higher) with 24bpp depth.
> > > cirrus can handle up to 800x600 with 32bpp.
> >
> > 16bpp is maybe a better choice? Nobody's using cirrus because they care
> > about color fidelity and it'll use less CPU to update. There's
> > precedent here, mgag200 defaults to 16bpp on sufficiently memory-
> > impaired devices.
>
> Yeah nouveau does the same fallback to 16bpp if there's not enough vram.
> So do a bunch of other drivers.
Makes sense. But appearently 16bpp support bitrotted over time, when
just flipping the default the driver Oopses at init time. I'll look
into it.
cheers,
Gerd
^ permalink raw reply
* Re: [RFC 0/4] Virtio uses DMA API for all devices
From: Christoph Hellwig @ 2018-08-08 6:31 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: robh, srikar, Michael S. Tsirkin, mpe, Will Deacon, linux-kernel,
linuxram, virtualization, Christoph Hellwig,
jean-philippe.brucker, paulus, marc.zyngier, joe, robin.murphy,
david, linuxppc-dev, elfring, haren, Anshuman Khandual
In-Reply-To: <2103ecfe52d23cec03f185d08a87bfad9c9d82b5.camel@kernel.crashing.org>
On Wed, Aug 08, 2018 at 06:32:45AM +1000, Benjamin Herrenschmidt wrote:
> As for the flag itself, while we could set it from qemu when we get
> notified that the guest is going secure, both Michael and I think it's
> rather gross, it requires qemu to go iterate all virtio devices and
> "poke" something into them.
You don't need to set them the time you go secure. You just need to
set the flag from the beginning on any VM you might want to go secure.
Or for simplicity just any VM - if the DT/ACPI tables exposed by
qemu are good enough that will always exclude a iommu and not set a
DMA offset, so nothing will change on the qemu side of he processing,
and with the new direct calls for the direct dma ops performance in
the guest won't change either.
> It's nicer if we have a way in the guest virtio driver to do something
> along the lines of
>
> if ((flags & VIRTIO_F_IOMMU_PLATFORM) || arch_virtio_wants_dma_ops())
>
> Which would have the same effect and means the issue is entirely
> contained in the guest.
It would not be the same effect. The problem with that is that you must
now assumes that your qemu knows that for example you might be passing
a dma offset if the bus otherwise requires it. Or in other words:
you potentially break the contract between qemu and the guest of always
passing down physical addresses. If we explicitly change that contract
through using a flag that says you pass bus address everything is fine.
Note that in practice your scheme will probably just work for your
initial prototype, but chances are it will get us in trouble later on.
^ permalink raw reply
* [PATCH net] vhost: reset metadata cache when initializing new IOTLB
From: Jason Wang @ 2018-08-08 3:43 UTC (permalink / raw)
To: mst, jasowang; +Cc: netdev, linux-kernel, kvm, virtualization
We need to reset metadata cache during new IOTLB initialization,
otherwise the stale pointers to previous IOTLB may be still accessed
which will lead a use after free.
Reported-by: syzbot+c51e6736a1bf614b3272@syzkaller.appspotmail.com
Fixes: f88949138058 ("vhost: introduce O(1) vq metadata cache")
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/vhost.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index a502f1a..ed31145 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1560,9 +1560,12 @@ int vhost_init_device_iotlb(struct vhost_dev *d, bool enabled)
d->iotlb = niotlb;
for (i = 0; i < d->nvqs; ++i) {
- mutex_lock(&d->vqs[i]->mutex);
- d->vqs[i]->iotlb = niotlb;
- mutex_unlock(&d->vqs[i]->mutex);
+ struct vhost_virtqueue *vq = d->vqs[i];
+
+ mutex_lock(&vq->mutex);
+ vq->iotlb = niotlb;
+ __vhost_vq_meta_reset(vq);
+ mutex_unlock(&vq->mutex);
}
vhost_umem_clean(oiotlb);
--
2.7.4
^ permalink raw reply related
* Re: KASAN: use-after-free Read in iotlb_access_ok
From: Jason Wang @ 2018-08-08 2:52 UTC (permalink / raw)
To: syzbot, kvm, linux-kernel, mst, netdev, syzkaller-bugs,
virtualization
In-Reply-To: <000000000000eb92240572d68452@google.com>
On 2018年08月07日 19:16, syzbot wrote:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: e30cb13c5a09 Merge
> git://git.kernel.org/pub/scm/linux/kern..
> git tree: upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=10a153e0400000
> kernel config: https://syzkaller.appspot.com/x/.config?x=2dc0cd7c2eefb46f
> dashboard link:
> https://syzkaller.appspot.com/bug?extid=c51e6736a1bf614b3272
> compiler: gcc (GCC) 8.0.1 20180413 (experimental)
>
> Unfortunately, I don't have any reproducer for this crash yet.
>
> IMPORTANT: if you fix the bug, please add the following tag to the
> commit:
> Reported-by: syzbot+c51e6736a1bf614b3272@syzkaller.appspotmail.com
>
> ==================================================================
> BUG: KASAN: use-after-free in vhost_vq_meta_fetch
> drivers/vhost/vhost.c:702 [inline]
> BUG: KASAN: use-after-free in iotlb_access_ok+0x5c9/0x600
> drivers/vhost/vhost.c:1177
> Read of size 8 at addr ffff880197df2fc0 by task vhost-8938/8941
>
> CPU: 0 PID: 8941 Comm: vhost-8938 Not tainted 4.18.0-rc7+ #174
> Hardware name: Google Google Compute Engine/Google Compute Engine,
> BIOS Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x1c9/0x2b4 lib/dump_stack.c:113
> print_address_description+0x6c/0x20b mm/kasan/report.c:256
> kasan_report_error mm/kasan/report.c:354 [inline]
> kasan_report.cold.7+0x242/0x2fe mm/kasan/report.c:412
> __asan_report_load8_noabort+0x14/0x20 mm/kasan/report.c:433
> vhost_vq_meta_fetch drivers/vhost/vhost.c:702 [inline]
> iotlb_access_ok+0x5c9/0x600 drivers/vhost/vhost.c:1177
> vq_iotlb_prefetch+0x10e/0x230 drivers/vhost/vhost.c:1214
> handle_rx+0x247/0x1f80 drivers/vhost/net.c:799
> handle_rx_net+0x19/0x20 drivers/vhost/net.c:934
> vhost_worker+0x283/0x490 drivers/vhost/vhost.c:360
> kthread+0x345/0x410 kernel/kthread.c:246
> ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:412
>
> Allocated by task 8938:
> save_stack+0x43/0xd0 mm/kasan/kasan.c:448
> set_track mm/kasan/kasan.c:460 [inline]
> kasan_kmalloc+0xc4/0xe0 mm/kasan/kasan.c:553
> kmem_cache_alloc_trace+0x152/0x780 mm/slab.c:3620
> kmalloc include/linux/slab.h:513 [inline]
> vhost_new_umem_range+0xcb/0x7c0 drivers/vhost/vhost.c:911
> vhost_process_iotlb_msg drivers/vhost/vhost.c:1000 [inline]
> vhost_chr_write_iter+0xe53/0x1a00 drivers/vhost/vhost.c:1043
> vhost_net_chr_write_iter+0x59/0x70 drivers/vhost/net.c:1399
> call_write_iter include/linux/fs.h:1793 [inline]
> new_sync_write fs/read_write.c:474 [inline]
> __vfs_write+0x6c6/0x9f0 fs/read_write.c:487
> vfs_write+0x1f8/0x560 fs/read_write.c:549
> ksys_write+0x101/0x260 fs/read_write.c:598
> __do_sys_write fs/read_write.c:610 [inline]
> __se_sys_write fs/read_write.c:607 [inline]
> __x64_sys_write+0x73/0xb0 fs/read_write.c:607
> do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> Freed by task 8950:
> save_stack+0x43/0xd0 mm/kasan/kasan.c:448
> set_track mm/kasan/kasan.c:460 [inline]
> __kasan_slab_free+0x11a/0x170 mm/kasan/kasan.c:521
> kasan_slab_free+0xe/0x10 mm/kasan/kasan.c:528
> __cache_free mm/slab.c:3498 [inline]
> kfree+0xd9/0x260 mm/slab.c:3813
> vhost_umem_free+0x944/0x14d0 drivers/vhost/vhost.c:576
> vhost_umem_clean+0x83/0xf0 drivers/vhost/vhost.c:588
> vhost_init_device_iotlb+0x1d7/0x290 drivers/vhost/vhost.c:1568
> vhost_net_set_features drivers/vhost/net.c:1292 [inline]
> vhost_net_ioctl+0xff3/0x1a80 drivers/vhost/net.c:1357
> vfs_ioctl fs/ioctl.c:46 [inline]
> file_ioctl fs/ioctl.c:500 [inline]
> do_vfs_ioctl+0x1de/0x1720 fs/ioctl.c:684
> ksys_ioctl+0xa9/0xd0 fs/ioctl.c:701
> __do_sys_ioctl fs/ioctl.c:708 [inline]
> __se_sys_ioctl fs/ioctl.c:706 [inline]
> __x64_sys_ioctl+0x73/0xb0 fs/ioctl.c:706
> do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> The buggy address belongs to the object at ffff880197df2f80
> which belongs to the cache kmalloc-96 of size 96
> The buggy address is located 64 bytes inside of
> 96-byte region [ffff880197df2f80, ffff880197df2fe0)
> The buggy address belongs to the page:
> page:ffffea00065f7c80 count:1 mapcount:0 mapping:ffff8801dac004c0
> index:0x0
> flags: 0x2fffc0000000100(slab)
> raw: 02fffc0000000100 ffffea0007530148 ffffea000663b888 ffff8801dac004c0
> raw: 0000000000000000 ffff880197df2000 0000000100000020 0000000000000000
> page dumped because: kasan: bad access detected
>
> Memory state around the buggy address:
> ffff880197df2e80: fb fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
> ffff880197df2f00: fb fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
>> ffff880197df2f80: fb fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
> ^
> ffff880197df3000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
> ffff880197df3080: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
> ==================================================================
>
>
> ---
> This bug is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at syzkaller@googlegroups.com.
>
> syzbot will keep track of this bug report. See:
> https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with
> syzbot.
Looks like the metadata IOTLB needs to be reset during set_features.
Will post a fix.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC 0/4] Virtio uses DMA API for all devices
From: Benjamin Herrenschmidt @ 2018-08-07 20:32 UTC (permalink / raw)
To: Christoph Hellwig
Cc: robh, srikar, Michael S. Tsirkin, mpe, Will Deacon, linux-kernel,
linuxram, virtualization, jean-philippe.brucker, paulus,
marc.zyngier, joe, robin.murphy, david, linuxppc-dev, elfring,
haren, Anshuman Khandual
In-Reply-To: <20180807135505.GA29034@infradead.org>
On Tue, 2018-08-07 at 06:55 -0700, Christoph Hellwig wrote:
> On Tue, Aug 07, 2018 at 04:42:44PM +1000, Benjamin Herrenschmidt wrote:
> > Note that I can make it so that the same DMA ops (basically standard
> > swiotlb ops without arch hacks) work for both "direct virtio" and
> > "normal PCI" devices.
> >
> > The trick is simply in the arch to setup the iommu to map the swiotlb
> > bounce buffer pool 1:1 in the iommu, so the iommu essentially can be
> > ignored without affecting the physical addresses.
> >
> > If I do that, *all* I need is a way, from the guest itself (again, the
> > other side dosn't know anything about it), to force virtio to use the
> > DMA ops as if there was an iommu, that is, use whatever dma ops were
> > setup by the platform for the pci device.
>
> In that case just setting VIRTIO_F_IOMMU_PLATFORM in the flags should
> do the work (even if that isn't strictly what the current definition
> of the flag actually means). On the qemu side you'll need to make
> sure you have a way to set VIRTIO_F_IOMMU_PLATFORM without emulating
> an iommu, but with code to take dma offsets into account if your
> plaform has any (various power plaforms seem to have them, not sure
> if it affects your config).
Something like that yes. I prefer a slightly different way, see below,
any but in both cases, it should alleviate your concerns since it means
there would be no particular mucking around with DMA ops at all, virtio
would just use whatever "normal" ops we establish for all PCI devices
on that platform, which will be standard ones.
(swiotlb ones today and the new "integrates" ones you're cooking
tomorrow).
As for the flag itself, while we could set it from qemu when we get
notified that the guest is going secure, both Michael and I think it's
rather gross, it requires qemu to go iterate all virtio devices and
"poke" something into them.
It also means qemu will need some other internal nasty flag that says
"set that bit but don't do iommu".
It's nicer if we have a way in the guest virtio driver to do something
along the lines of
if ((flags & VIRTIO_F_IOMMU_PLATFORM) || arch_virtio_wants_dma_ops())
Which would have the same effect and means the issue is entirely
contained in the guest.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH net-next] net: allow to call netif_reset_xps_queues() under cpu_read_lock
From: David Miller @ 2018-08-07 20:15 UTC (permalink / raw)
To: avagin; +Cc: amritha.nambiar, mst, netdev, virtualization, avagin
In-Reply-To: <20180807041454.18315-1-avagin@openvz.org>
From: Andrei Vagin <avagin@openvz.org>
Date: Mon, 6 Aug 2018 21:14:54 -0700
> This patch adds an ability to call __netif_set_xps_queue under
> cpu_read_lock().
Please don't add conditional locking using a boolean argument.
Simply wrap calls to __netif_set_xps_queue() with cpu read
lock held by the caller or similar.
^ permalink raw reply
* Re: [RFC 0/4] Virtio uses DMA API for all devices
From: Christoph Hellwig @ 2018-08-07 13:55 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: robh, srikar, Michael S. Tsirkin, mpe, Will Deacon, linux-kernel,
linuxram, virtualization, Christoph Hellwig,
jean-philippe.brucker, paulus, marc.zyngier, joe, robin.murphy,
david, linuxppc-dev, elfring, haren, Anshuman Khandual
In-Reply-To: <aa59c7f8556bd4b332394a1dcf2d4a8faf3dc4a2.camel@kernel.crashing.org>
On Tue, Aug 07, 2018 at 04:42:44PM +1000, Benjamin Herrenschmidt wrote:
> Note that I can make it so that the same DMA ops (basically standard
> swiotlb ops without arch hacks) work for both "direct virtio" and
> "normal PCI" devices.
>
> The trick is simply in the arch to setup the iommu to map the swiotlb
> bounce buffer pool 1:1 in the iommu, so the iommu essentially can be
> ignored without affecting the physical addresses.
>
> If I do that, *all* I need is a way, from the guest itself (again, the
> other side dosn't know anything about it), to force virtio to use the
> DMA ops as if there was an iommu, that is, use whatever dma ops were
> setup by the platform for the pci device.
In that case just setting VIRTIO_F_IOMMU_PLATFORM in the flags should
do the work (even if that isn't strictly what the current definition
of the flag actually means). On the qemu side you'll need to make
sure you have a way to set VIRTIO_F_IOMMU_PLATFORM without emulating
an iommu, but with code to take dma offsets into account if your
plaform has any (various power plaforms seem to have them, not sure
if it affects your config).
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox