Re: [PATCH] scsi: virtio_scsi: remove unnecessary condition check
On Thu, 9 Jul 2020 11:06:07 -0400, Xianting Tian wrote: > kmem_cache_destroy and mempool_destroy can correctly handle > null pointer parameter, so there is no need to check if the > parameter is null before calling kmem_cache_destroy and > mempool_destroy. Applied to 5.9/scsi-queue, thanks! [1/1] scsi: virtio_scsi: Remove unnecessary condition check https://git.kernel.org/mkp/scsi/c/92e8d0323a51 -- Martin K. Petersen Oracle Linux Engineering ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_balloon: clear modern features under legacy
On Sun, Jul 12, 2020 at 8:10 AM Michael S. Tsirkin wrote:
>
> On Fri, Jul 10, 2020 at 09:13:41AM -0700, Alexander Duyck wrote:
> > On Fri, Jul 10, 2020 at 4:31 AM Michael S. Tsirkin wrote:
> > >
> > > Page reporting features were never supported by legacy hypervisors.
> > > Supporting them poses a problem: should we use native endian-ness (like
> > > current code assumes)? Or little endian-ness like the virtio spec says?
> > > Rather than try to figure out, and since results of
> > > incorrect endian-ness are dire, let's just block this configuration.
> > >
> > > Cc: sta...@vger.kernel.org
> > > Signed-off-by: Michael S. Tsirkin
> >
> > So I am not sure about the patch description. In the case of page
> > poison and free page reporting I don't think we are defining anything
> > that doesn't already have a definition of how to use in legacy.
> > Specifically the virtio_balloon_config is already defined as having
> > all fields as little endian in legacy mode, and there is a definition
> > for all of the fields in a virtqueue and how they behave in legacy
> > mode.
> >
> > As far as I can see the only item that may be an issue is the command
> > ID being supplied via the virtqueue for free page hinting, which
> > appears to be in native endian-ness. Otherwise it would have fallen
> > into the same category since it is making use of virtio_balloon_config
> > and a virtqueue for supplying the page location and length.
>
>
>
> So as you point out correctly balloon spec says all fields are little
> endian. Fair enough.
> Problem is when virtio 1 is not negotiated, then this is not what the
> driver assumes for any except a handlful of fields.
>
> But yes it mostly works out.
>
> For example:
>
>
> static void update_balloon_size(struct virtio_balloon *vb)
> {
> u32 actual = vb->num_pages;
>
> /* Legacy balloon config space is LE, unlike all other devices. */
> if (!virtio_has_feature(vb->vdev, VIRTIO_F_VERSION_1))
> actual = (__force u32)cpu_to_le32(actual);
>
> virtio_cwrite(vb->vdev, struct virtio_balloon_config, actual,
> );
> }
>
>
> this is LE even without VIRTIO_F_VERSION_1, so matches spec.
>
> /* Start with poison val of 0 representing general init */
> __u32 poison_val = 0;
>
> /*
> * Let the hypervisor know that we are expecting a
> * specific value to be written back in balloon pages.
> */
> if (!want_init_on_free())
> memset(_val, PAGE_POISON, sizeof(poison_val));
>
> virtio_cwrite(vb->vdev, struct virtio_balloon_config,
> poison_val, _val);
>
>
> actually this writes a native endian-ness value. All bytes happen to be
> the same though, and host only cares about 0 or non 0 ATM.
So we are safe assuming it is a repeating value, but for correctness
maybe we should make certain to cast this as a le32 value. I can
submit a patch to do that.
> As you say correctly the command id is actually assumed native endian:
>
>
> static u32 virtio_balloon_cmd_id_received(struct virtio_balloon *vb)
> {
> if (test_and_clear_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
>>config_read_bitmap))
> virtio_cread(vb->vdev, struct virtio_balloon_config,
> free_page_hint_cmd_id,
> >cmd_id_received_cache);
>
> return vb->cmd_id_received_cache;
> }
>
>
> So guest assumes native, host assumes LE.
This wasn't even the one I was talking about, but now that you point
it out this is definately bug. The command ID I was talking about was
the one being passed via the descriptor ring. That one I believe is
native on both sides.
>
>
>
> > > ---
> > > drivers/virtio/virtio_balloon.c | 9 +
> > > 1 file changed, 9 insertions(+)
> > >
> > > diff --git a/drivers/virtio/virtio_balloon.c
> > > b/drivers/virtio/virtio_balloon.c
> > > index 5d4b891bf84f..b9bc03345157 100644
> > > --- a/drivers/virtio/virtio_balloon.c
> > > +++ b/drivers/virtio/virtio_balloon.c
> > > @@ -1107,6 +1107,15 @@ static int virtballoon_restore(struct
> > > virtio_device *vdev)
> > >
> > > static int virtballoon_validate(struct virtio_device *vdev)
> > > {
> > > + /*
> > > +* Legacy devices never specified how modern features should
> > > behave.
> > > +* E.g. which endian-ness to use? Better not to assume anything.
> > > +*/
> > > + if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> > > + __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT);
> > > + __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_PAGE_POISON);
> > > + __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_REPORTING);
> > > + }
> > > /*
> > > * Inform the hypervisor that our pages are poisoned or
> > > * initialized. If
Re: [PATCH v3 02/19] compiler.h: Split {READ, WRITE}_ONCE definitions out into rwonce.h
On Fri, Jul 10, 2020 at 05:51:46PM +0100, Will Deacon wrote:
> In preparation for allowing architectures to define their own
> implementation of the READ_ONCE() macro, move the generic
> {READ,WRITE}_ONCE() definitions out of the unwieldy 'linux/compiler.h'
> file and into a new 'rwonce.h' header under 'asm-generic'.
>
> Acked-by: Paul E. McKenney
> Signed-off-by: Will Deacon
> ---
> include/asm-generic/Kbuild| 1 +
> include/asm-generic/barrier.h | 2 +-
> include/asm-generic/rwonce.h | 91 +++
> include/linux/compiler.h | 83 +---
> 4 files changed, 95 insertions(+), 82 deletions(-)
> create mode 100644 include/asm-generic/rwonce.h
>
> diff --git a/include/asm-generic/Kbuild b/include/asm-generic/Kbuild
> index 44ec80e70518..74b0612601dd 100644
> --- a/include/asm-generic/Kbuild
> +++ b/include/asm-generic/Kbuild
> @@ -45,6 +45,7 @@ mandatory-y += pci.h
> mandatory-y += percpu.h
> mandatory-y += pgalloc.h
> mandatory-y += preempt.h
> +mandatory-y += rwonce.h
> mandatory-y += sections.h
> mandatory-y += serial.h
> mandatory-y += shmparam.h
> diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
> index 2eacaf7d62f6..8116744bb82c 100644
> --- a/include/asm-generic/barrier.h
> +++ b/include/asm-generic/barrier.h
> @@ -13,7 +13,7 @@
>
> #ifndef __ASSEMBLY__
>
> -#include
> +#include
>
> #ifndef nop
> #define nop()asm volatile ("nop")
> diff --git a/include/asm-generic/rwonce.h b/include/asm-generic/rwonce.h
> new file mode 100644
> index ..92cc2f223cb3
> --- /dev/null
> +++ b/include/asm-generic/rwonce.h
> @@ -0,0 +1,91 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Prevent the compiler from merging or refetching reads or writes. The
> + * compiler is also forbidden from reordering successive instances of
> + * READ_ONCE and WRITE_ONCE, but only when the compiler is aware of some
> + * particular ordering. One way to make the compiler aware of ordering is to
> + * put the two invocations of READ_ONCE or WRITE_ONCE in different C
> + * statements.
> + *
> + * These two macros will also work on aggregate data types like structs or
> + * unions.
> + *
> + * Their two major use cases are: (1) Mediating communication between
> + * process-level code and irq/NMI handlers, all running on the same CPU,
> + * and (2) Ensuring that the compiler does not fold, spindle, or otherwise
> + * mutilate accesses that either do not require ordering or that interact
> + * with an explicit memory barrier or atomic instruction that provides the
> + * required ordering.
> + */
> +#ifndef __ASM_GENERIC_RWONCE_H
> +#define __ASM_GENERIC_RWONCE_H
> +
> +#ifndef __ASSEMBLY__
> +
> +#include
> +#include
> +#include
> +
> +#include
> +
> +/*
> + * Use __READ_ONCE() instead of READ_ONCE() if you do not require any
> + * atomicity or dependency ordering guarantees. Note that this may result
> + * in tears!
> + */
> +#define __READ_ONCE(x) (*(const volatile __unqual_scalar_typeof(x)
> *)&(x))
> +
> +#define __READ_ONCE_SCALAR(x)
> \
> +({ \
> + __unqual_scalar_typeof(x) __x = __READ_ONCE(x); \
> + smp_read_barrier_depends(); \
> + (typeof(x))__x; \
> +})
> +
> +#define READ_ONCE(x) \
> +({ \
> + compiletime_assert_rwonce_type(x); \
Does it make sense if we also move the definition of this compile time
assertion into rwonce.h too?
Regards,
Boqun
> + __READ_ONCE_SCALAR(x); \
> +})
> +
[...]
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 12/24] scsi: virtio_scsi: Demote seemingly unintentional kerneldoc header
On Mon, 13 Jul 2020, Michael S. Tsirkin wrote: > On Mon, Jul 13, 2020 at 08:59:49AM +0100, Lee Jones wrote: > > This is the only use of kerneldoc in the sourcefile and no > > descriptions are provided. > > > > Fixes the following W=1 kernel build warning(s): > > > > drivers/scsi/virtio_scsi.c:109: warning: Function parameter or member > > 'vscsi' not described in 'virtscsi_complete_cmd' > > drivers/scsi/virtio_scsi.c:109: warning: Function parameter or member > > 'buf' not described in 'virtscsi_complete_cmd' > > > > Cc: "Michael S. Tsirkin" > > Cc: Jason Wang > > Cc: Stefan Hajnoczi > > Cc: virtualization@lists.linux-foundation.org > > Signed-off-by: Lee Jones > > Acked-by: Paolo Bonzini > > Acked-by: Michael S. Tsirkin Thank you Michael. > Pls merge with the rest of the patches (which tree is this for?) My assumption is that Martin will take all of these (and the other sets) via the SCSI repo. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 07/19] vhost: Remove redundant use of read_barrier_depends() barrier
On Fri, Jul 10, 2020 at 05:51:51PM +0100, Will Deacon wrote:
> Since commit 76ebbe78f739 ("locking/barriers: Add implicit
> smp_read_barrier_depends() to READ_ONCE()"), there is no need to use
> smp_read_barrier_depends() outside of the Alpha architecture code.
>
> Unfortunately, there is precisely _one_ user in the vhost code, and
> there isn't an obvious READ_ONCE() access making the barrier
> redundant. However, on closer inspection (thanks, Jason), it appears
> that vring synchronisation between the producer and consumer occurs via
> the 'avail_idx' field, which is followed up by an rmb() in
> vhost_get_vq_desc(), making the read_barrier_depends() redundant on
> Alpha.
>
> Jason says:
>
> | I'm also confused about the barrier here, basically in driver side
> | we did:
> |
> | 1) allocate pages
> | 2) store pages in indirect->addr
> | 3) smp_wmb()
> | 4) increase the avail idx (somehow a tail pointer of vring)
> |
> | in vhost we did:
> |
> | 1) read avail idx
> | 2) smp_rmb()
> | 3) read indirect->addr
> | 4) read from indirect->addr
> |
> | It looks to me even the data dependency barrier is not necessary
> | since we have rmb() which is sufficient for us to the correct
> | indirect->addr and driver are not expected to do any writing to
> | indirect->addr after avail idx is increased
>
> Remove the redundant barrier invocation.
>
> Suggested-by: Jason Wang
> Acked-by: Paul E. McKenney
> Signed-off-by: Will Deacon
I agree
Acked-by: Michael S. Tsirkin
Pls merge with the rest of the patchset.
> ---
> drivers/vhost/vhost.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index d7b8df3edffc..74d135ee7e26 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -2092,11 +2092,6 @@ static int get_indirect(struct vhost_virtqueue *vq,
> return ret;
> }
> iov_iter_init(, READ, vq->indirect, ret, len);
> -
> - /* We will use the result as an address to read from, so most
> - * architectures only need a compiler barrier here. */
> - read_barrier_depends();
> -
> count = len / sizeof desc;
> /* Buffers are chained via a 16 bit next field, so
>* we can have at most 2^16 of these. */
> --
> 2.27.0.383.g050319c2ae-goog
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 12/24] scsi: virtio_scsi: Demote seemingly unintentional kerneldoc header
On Mon, Jul 13, 2020 at 08:59:49AM +0100, Lee Jones wrote: > This is the only use of kerneldoc in the sourcefile and no > descriptions are provided. > > Fixes the following W=1 kernel build warning(s): > > drivers/scsi/virtio_scsi.c:109: warning: Function parameter or member > 'vscsi' not described in 'virtscsi_complete_cmd' > drivers/scsi/virtio_scsi.c:109: warning: Function parameter or member 'buf' > not described in 'virtscsi_complete_cmd' > > Cc: "Michael S. Tsirkin" > Cc: Jason Wang > Cc: Stefan Hajnoczi > Cc: virtualization@lists.linux-foundation.org > Signed-off-by: Lee Jones > Acked-by: Paolo Bonzini Acked-by: Michael S. Tsirkin Pls merge with the rest of the patches (which tree is this for?) > --- > drivers/scsi/virtio_scsi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c > index 0e0910c5b9424..56875467e4984 100644 > --- a/drivers/scsi/virtio_scsi.c > +++ b/drivers/scsi/virtio_scsi.c > @@ -100,7 +100,7 @@ static void virtscsi_compute_resid(struct scsi_cmnd *sc, > u32 resid) > scsi_set_resid(sc, resid); > } > > -/** > +/* > * virtscsi_complete_cmd - finish a scsi_cmd and invoke scsi_done > * > * Called with vq_lock held. > -- > 2.25.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vsock/virtio: annotate 'the_virtio_vsock' RCU pointer
On Fri, Jul 10, 2020 at 02:12:43PM +0200, Stefano Garzarella wrote:
> Commit 0deab087b16a ("vsock/virtio: use RCU to avoid use-after-free
> on the_virtio_vsock") starts to use RCU to protect 'the_virtio_vsock'
> pointer, but we forgot to annotate it.
>
> This patch adds the annotation to fix the following sparse errors:
>
> net/vmw_vsock/virtio_transport.c:73:17: error: incompatible types in
> comparison expression (different address spaces):
> net/vmw_vsock/virtio_transport.c:73:17:struct virtio_vsock [noderef]
> __rcu *
> net/vmw_vsock/virtio_transport.c:73:17:struct virtio_vsock *
> net/vmw_vsock/virtio_transport.c:171:17: error: incompatible types in
> comparison expression (different address spaces):
> net/vmw_vsock/virtio_transport.c:171:17:struct virtio_vsock [noderef]
> __rcu *
> net/vmw_vsock/virtio_transport.c:171:17:struct virtio_vsock *
> net/vmw_vsock/virtio_transport.c:207:17: error: incompatible types in
> comparison expression (different address spaces):
> net/vmw_vsock/virtio_transport.c:207:17:struct virtio_vsock [noderef]
> __rcu *
> net/vmw_vsock/virtio_transport.c:207:17:struct virtio_vsock *
> net/vmw_vsock/virtio_transport.c:561:13: error: incompatible types in
> comparison expression (different address spaces):
> net/vmw_vsock/virtio_transport.c:561:13:struct virtio_vsock [noderef]
> __rcu *
> net/vmw_vsock/virtio_transport.c:561:13:struct virtio_vsock *
> net/vmw_vsock/virtio_transport.c:612:9: error: incompatible types in
> comparison expression (different address spaces):
> net/vmw_vsock/virtio_transport.c:612:9:struct virtio_vsock [noderef]
> __rcu *
> net/vmw_vsock/virtio_transport.c:612:9:struct virtio_vsock *
> net/vmw_vsock/virtio_transport.c:631:9: error: incompatible types in
> comparison expression (different address spaces):
> net/vmw_vsock/virtio_transport.c:631:9:struct virtio_vsock [noderef]
> __rcu *
> net/vmw_vsock/virtio_transport.c:631:9:struct virtio_vsock *
>
> Fixes: 0deab087b16a ("vsock/virtio: use RCU to avoid use-after-free on
> the_virtio_vsock")
> Reported-by: Michael S. Tsirkin
> Signed-off-by: Stefano Garzarella
Acked-by: Michael S. Tsirkin
who's merging this? Dave?
> ---
> net/vmw_vsock/virtio_transport.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/vmw_vsock/virtio_transport.c
> b/net/vmw_vsock/virtio_transport.c
> index dfbaf6bd8b1c..2700a63ab095 100644
> --- a/net/vmw_vsock/virtio_transport.c
> +++ b/net/vmw_vsock/virtio_transport.c
> @@ -22,7 +22,7 @@
> #include
>
> static struct workqueue_struct *virtio_vsock_workqueue;
> -static struct virtio_vsock *the_virtio_vsock;
> +static struct virtio_vsock __rcu *the_virtio_vsock;
> static DEFINE_MUTEX(the_virtio_vsock_mutex); /* protects the_virtio_vsock */
>
> struct virtio_vsock {
> --
> 2.26.2
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/7] kvm/vfio: detect assigned device via irqbypass manager
On Mon, Jul 13, 2020 at 04:13:35PM +0800, Jason Wang wrote:
>
> On 2020/7/13 上午5:06, Michael S. Tsirkin wrote:
> > On Sun, Jul 12, 2020 at 10:49:21PM +0800, Zhu Lingshan wrote:
> > > We used to detect assigned device via VFIO manipulated device
> > > conters. This is less flexible consider VFIO is not the only
> > > interface for assigned device. vDPA devices has dedicated
> > > backed hardware as well. So this patch tries to detect
> > > the assigned device via irqbypass manager.
> > >
> > > We will increase/decrease the assigned device counter in kvm/x86.
> > > Both vDPA and VFIO would go through this code path.
> > >
> > > This code path only affect x86 for now.
> > >
> > > Signed-off-by: Zhu Lingshan
> >
> > I think it's best to leave VFIO alone. Add appropriate APIs for VDPA,
> > worry about converting existing users later.
>
>
>
> Just to make sure I understand, did you mean:
>
> 1) introduce another bridge for vDPA
>
> or
>
> 2) only detect vDPA via bypass manager? (we can leave VFIO code as is, then
> the assigned device counter may increase/decrease twice if VFIO use irq
> bypass manager which should be no harm).
>
> Thanks
2 is probably easier to justify. 1 would depend on the specific bridge
proposed.
>
> >
> > > ---
> > > arch/x86/kvm/x86.c | 10 --
> > > virt/kvm/vfio.c| 2 --
> > > 2 files changed, 8 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index 00c88c2..20c07d3 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -10624,11 +10624,17 @@ int kvm_arch_irq_bypass_add_producer(struct
> > > irq_bypass_consumer *cons,
> > > {
> > > struct kvm_kernel_irqfd *irqfd =
> > > container_of(cons, struct kvm_kernel_irqfd, consumer);
> > > + int ret;
> > > irqfd->producer = prod;
> > > + kvm_arch_start_assignment(irqfd->kvm);
> > > + ret = kvm_x86_ops.update_pi_irte(irqfd->kvm,
> > > + prod->irq, irqfd->gsi, 1);
> > > +
> > > + if (ret)
> > > + kvm_arch_end_assignment(irqfd->kvm);
> > > - return kvm_x86_ops.update_pi_irte(irqfd->kvm,
> > > -prod->irq, irqfd->gsi, 1);
> > > + return ret;
> > > }
> > > void kvm_arch_irq_bypass_del_producer(struct irq_bypass_consumer *cons,
> > > diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
> > > index 8fcbc50..111da52 100644
> > > --- a/virt/kvm/vfio.c
> > > +++ b/virt/kvm/vfio.c
> > > @@ -226,7 +226,6 @@ static int kvm_vfio_set_group(struct kvm_device *dev,
> > > long attr, u64 arg)
> > > list_add_tail(>node, >group_list);
> > > kvg->vfio_group = vfio_group;
> > > - kvm_arch_start_assignment(dev->kvm);
> > > mutex_unlock(>lock);
> > > @@ -254,7 +253,6 @@ static int kvm_vfio_set_group(struct kvm_device *dev,
> > > long attr, u64 arg)
> > > continue;
> > > list_del(>node);
> > > - kvm_arch_end_assignment(dev->kvm);
> > > #ifdef CONFIG_SPAPR_TCE_IOMMU
> > > kvm_spapr_tce_release_vfio_group(dev->kvm,
> > >
> > > kvg->vfio_group);
> > > --
> > > 1.8.3.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 00/18] Allow architectures to override __READ_ONCE()
On Fri, Jul 10, 2020 at 05:51:44PM +0100, Will Deacon wrote:
> SeongJae Park (1):
> Documentation/barriers/kokr: Remove references to
> [smp_]read_barrier_depends()
>
> Will Deacon (18):
> tools: bpf: Use local copy of headers including uapi/linux/filter.h
> compiler.h: Split {READ,WRITE}_ONCE definitions out into rwonce.h
> asm/rwonce: Allow __READ_ONCE to be overridden by the architecture
> alpha: Override READ_ONCE() with barriered implementation
> asm/rwonce: Remove smp_read_barrier_depends() invocation
> asm/rwonce: Don't pull into 'asm-generic/rwonce.h'
> vhost: Remove redundant use of read_barrier_depends() barrier
> alpha: Replace smp_read_barrier_depends() usage with smp_[r]mb()
> locking/barriers: Remove definitions for [smp_]read_barrier_depends()
> Documentation/barriers: Remove references to
> [smp_]read_barrier_depends()
> tools/memory-model: Remove smp_read_barrier_depends() from informal
> doc
> include/linux: Remove smp_read_barrier_depends() from comments
> checkpatch: Remove checks relating to [smp_]read_barrier_depends()
> arm64: Reduce the number of header files pulled into vmlinux.lds.S
> arm64: alternatives: Split up alternative.h
> arm64: cpufeatures: Add capability for LDAPR instruction
> arm64: alternatives: Remove READ_ONCE() usage during patch operation
> arm64: lto: Strengthen READ_ONCE() to acquire when CONFIG_LTO=y
Acked-by: Peter Zijlstra (Intel)
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vhost/scsi: fix up req type endian-ness
On Fri, Jul 10, 2020 at 06:48:51AM -0400, Michael S. Tsirkin wrote: > vhost/scsi doesn't handle type conversion correctly > for request type when using virtio 1.0 and up for BE, > or cross-endian platforms. > > Fix it up using vhost_32_to_cpu. > > Cc: sta...@vger.kernel.org > Signed-off-by: Michael S. Tsirkin > --- > drivers/vhost/scsi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Stefan Hajnoczi signature.asc Description: PGP signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vsock/virtio: annotate 'the_virtio_vsock' RCU pointer
On Fri, Jul 10, 2020 at 02:12:43PM +0200, Stefano Garzarella wrote:
> Commit 0deab087b16a ("vsock/virtio: use RCU to avoid use-after-free
> on the_virtio_vsock") starts to use RCU to protect 'the_virtio_vsock'
> pointer, but we forgot to annotate it.
>
> This patch adds the annotation to fix the following sparse errors:
>
> net/vmw_vsock/virtio_transport.c:73:17: error: incompatible types in
> comparison expression (different address spaces):
> net/vmw_vsock/virtio_transport.c:73:17:struct virtio_vsock [noderef]
> __rcu *
> net/vmw_vsock/virtio_transport.c:73:17:struct virtio_vsock *
> net/vmw_vsock/virtio_transport.c:171:17: error: incompatible types in
> comparison expression (different address spaces):
> net/vmw_vsock/virtio_transport.c:171:17:struct virtio_vsock [noderef]
> __rcu *
> net/vmw_vsock/virtio_transport.c:171:17:struct virtio_vsock *
> net/vmw_vsock/virtio_transport.c:207:17: error: incompatible types in
> comparison expression (different address spaces):
> net/vmw_vsock/virtio_transport.c:207:17:struct virtio_vsock [noderef]
> __rcu *
> net/vmw_vsock/virtio_transport.c:207:17:struct virtio_vsock *
> net/vmw_vsock/virtio_transport.c:561:13: error: incompatible types in
> comparison expression (different address spaces):
> net/vmw_vsock/virtio_transport.c:561:13:struct virtio_vsock [noderef]
> __rcu *
> net/vmw_vsock/virtio_transport.c:561:13:struct virtio_vsock *
> net/vmw_vsock/virtio_transport.c:612:9: error: incompatible types in
> comparison expression (different address spaces):
> net/vmw_vsock/virtio_transport.c:612:9:struct virtio_vsock [noderef]
> __rcu *
> net/vmw_vsock/virtio_transport.c:612:9:struct virtio_vsock *
> net/vmw_vsock/virtio_transport.c:631:9: error: incompatible types in
> comparison expression (different address spaces):
> net/vmw_vsock/virtio_transport.c:631:9:struct virtio_vsock [noderef]
> __rcu *
> net/vmw_vsock/virtio_transport.c:631:9:struct virtio_vsock *
>
> Fixes: 0deab087b16a ("vsock/virtio: use RCU to avoid use-after-free on
> the_virtio_vsock")
> Reported-by: Michael S. Tsirkin
> Signed-off-by: Stefano Garzarella
> ---
> net/vmw_vsock/virtio_transport.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Stefan Hajnoczi
signature.asc
Description: PGP signature
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 6/7] ifcvf: replace irq_request/free with helpers in vDPA core.
On 2020/7/12 下午10:49, Zhu Lingshan wrote:
This commit replaced irq_request/free() with helpers in vDPA
core, so that it can request/free irq and setup irq offloading
on order.
Signed-off-by: Zhu Lingshan
---
drivers/vdpa/ifcvf/ifcvf_main.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index f5a60c1..65b84e1 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -47,11 +47,12 @@ static void ifcvf_free_irq(struct ifcvf_adapter *adapter,
int queues)
{
struct pci_dev *pdev = adapter->pdev;
struct ifcvf_hw *vf = >vf;
+ struct vdpa_device *vdpa = >vdpa;
int i;
for (i = 0; i < queues; i++)
- devm_free_irq(>dev, vf->vring[i].irq, >vring[i]);
+ vdpa_free_vq_irq(>dev, vdpa, vf->vring[i].irq, i,
>vring[i]);
ifcvf_free_irq_vectors(pdev);
}
@@ -60,6 +61,7 @@ static int ifcvf_request_irq(struct ifcvf_adapter *adapter)
{
struct pci_dev *pdev = adapter->pdev;
struct ifcvf_hw *vf = >vf;
+ struct vdpa_device *vdpa = >vdpa;
int vector, i, ret, irq;
ret = pci_alloc_irq_vectors(pdev, IFCVF_MAX_INTR,
@@ -73,6 +75,7 @@ static int ifcvf_request_irq(struct ifcvf_adapter *adapter)
pci_name(pdev));
vector = 0;
irq = pci_irq_vector(pdev, vector);
+ /* config interrupt */
Unnecessary changes.
Thanks
ret = devm_request_irq(>dev, irq,
ifcvf_config_changed, 0,
vf->config_msix_name, vf);
@@ -82,13 +85,11 @@ static int ifcvf_request_irq(struct ifcvf_adapter *adapter)
pci_name(pdev), i);
vector = i + IFCVF_MSI_QUEUE_OFF;
irq = pci_irq_vector(pdev, vector);
- ret = devm_request_irq(>dev, irq,
+ ret = vdpa_alloc_vq_irq(>dev, vdpa, irq,
ifcvf_intr_handler, 0,
vf->vring[i].msix_name,
- >vring[i]);
+ >vring[i], i);
if (ret) {
- IFCVF_ERR(pdev,
- "Failed to request irq for vq %d\n", i);
ifcvf_free_irq(adapter, i);
return ret;
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 5/7] virtio_vdpa: init IRQ offloading function pointers to NULL.
On 2020/7/12 下午10:49, Zhu Lingshan wrote: This commit initialize IRQ offloading function pointers in virtio_vdpa_driver to NULL. Becasue irq offloading only focus on VMs for vhost_vdpa. Signed-off-by: Zhu Lingshan --- drivers/virtio/virtio_vdpa.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c index c30eb55..1e8acb9 100644 --- a/drivers/virtio/virtio_vdpa.c +++ b/drivers/virtio/virtio_vdpa.c @@ -386,6 +386,8 @@ static void virtio_vdpa_remove(struct vdpa_device *vdpa) }, .probe = virtio_vdpa_probe, .remove = virtio_vdpa_remove, + .setup_vq_irq = NULL, + .unsetup_vq_irq = NULL, }; Is this really needed consider the it's static? Thanks module_vdpa_driver(virtio_vdpa_driver); ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 4/7] vDPA: implement IRQ offloading helpers in vDPA core
On 2020/7/12 下午10:49, Zhu Lingshan wrote:
This commit implements IRQ offloading helpers in vDPA core by
introducing two couple of functions:
vdpa_alloc_vq_irq() and vdpa_free_vq_irq(): request irq and free
irq, will setup irq offloading if irq_bypass is enabled.
vdpa_setup_irq() and vdpa_unsetup_irq(): supportive functions,
will call vhost_vdpa helpers.
Signed-off-by: Zhu Lingshan
This patch should go before patch 3.
---
drivers/vdpa/vdpa.c | 46 ++
drivers/vhost/Kconfig | 1 +
drivers/vhost/vdpa.c | 2 ++
include/linux/vdpa.h | 11 +++
4 files changed, 60 insertions(+)
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index ff6562f..d8eba01 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -163,6 +163,52 @@ void vdpa_unregister_driver(struct vdpa_driver *drv)
}
EXPORT_SYMBOL_GPL(vdpa_unregister_driver);
+static void vdpa_setup_irq(struct vdpa_device *vdev, int qid, int irq)
+{
+ struct vdpa_driver *drv = drv_to_vdpa(vdev->dev.driver);
+
+#ifdef CONFIG_HAVE_KVM_IRQ_BYPASS
Let's don't do the check here. It's the responsibility of driver to
decide what it should do.
+ if (drv->setup_vq_irq)
+ drv->setup_vq_irq(vdev, qid, irq);
+#endif
+}
+
+static void vdpa_unsetup_irq(struct vdpa_device *vdev, int qid)
+{
+ struct vdpa_driver *drv = drv_to_vdpa(vdev->dev.driver);
+
+#ifdef CONFIG_HAVE_KVM_IRQ_BYPASS
+ if (drv->unsetup_vq_irq)
+ drv->unsetup_vq_irq(vdev, qid);
+#endif
+}
+
+int vdpa_alloc_vq_irq(struct device *dev, struct vdpa_device *vdev,
+ unsigned int irq, irq_handler_t handler,
+ unsigned long irqflags, const char *devname, void *dev_id,
+ int qid)
+{
+ int ret;
+
+ ret = devm_request_irq(dev, irq, handler, irqflags, devname, dev_id);
+ if (ret)
+ dev_err(dev, "Failed to request irq for vq %d\n", qid);
+ else
+ vdpa_setup_irq(vdev, qid, irq);
I'd like to squash the vdpa_setup_irq logic here.
+
+ return ret;
+
+}
+EXPORT_SYMBOL_GPL(vdpa_alloc_vq_irq);
+
+void vdpa_free_vq_irq(struct device *dev, struct vdpa_device *vdev, int irq,
+int qid, void *dev_id)
+{
+ devm_free_irq(dev, irq, dev_id);
+ vdpa_unsetup_irq(vdev, qid);
+}
+EXPORT_SYMBOL_GPL(vdpa_free_vq_irq);
+
static int vdpa_init(void)
{
return bus_register(_bus);
diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
Let squash the vhost patch into patch 3.
index d3688c6..587fbae 100644
--- a/drivers/vhost/Kconfig
+++ b/drivers/vhost/Kconfig
@@ -65,6 +65,7 @@ config VHOST_VDPA
tristate "Vhost driver for vDPA-based backend"
depends on EVENTFD
select VHOST
+ select IRQ_BYPASS_MANAGER
depends on VDPA
help
This kernel module can be loaded in host kernel to accelerate
diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 92683e4..6e25158 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -1020,6 +1020,8 @@ static void vhost_vdpa_remove(struct vdpa_device *vdpa)
},
.probe = vhost_vdpa_probe,
.remove = vhost_vdpa_remove,
+ .setup_vq_irq = vhost_vdpa_setup_vq_irq,
+ .unsetup_vq_irq = vhost_vdpa_unsetup_vq_irq,
};
static int __init vhost_vdpa_init(void)
diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index 239db79..9f9b245 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h
@@ -220,17 +220,28 @@ struct vdpa_device *__vdpa_alloc_device(struct device
*parent,
int vdpa_register_device(struct vdpa_device *vdev);
void vdpa_unregister_device(struct vdpa_device *vdev);
+int vdpa_alloc_vq_irq(struct device *dev, struct vdpa_device *vdev,
+ unsigned int irq, irq_handler_t handler,
+ unsigned long irqflags, const char *devname, void *dev_id,
+ int qid);
+void vdpa_free_vq_irq(struct device *dev, struct vdpa_device *vdev, int irq,
+ int qid, void *dev_id);
You need to document the devres implications of those two helpers.
+
/**
* vdpa_driver - operations for a vDPA driver
* @driver: underlying device driver
* @probe: the function to call when a device is found. Returns 0 or -errno.
* @remove: the function to call when a device is removed.
+ * @setup_vq_irq: setup irq bypass for a vhost_vdpa vq.
+ * @unsetup_vq_irq: unsetup irq bypass for a vhost_vdpa vq.
Though irq bypass is used by vhost-vdpa, it's not necessarily to be true
in the future. So it's better not to mention irqbypass in the doc here.
Thanks
*/
struct vdpa_driver {
struct device_driver driver;
int (*probe)(struct vdpa_device *vdev);
void (*remove)(struct vdpa_device *vdev);
+ void (*setup_vq_irq)(struct vdpa_device *vdev, int qid, int irq);
+ void
Re: [PATCH 3/7] vhost_vdpa: implement IRQ offloading functions in vhost_vdpa
On 2020/7/12 下午10:49, Zhu Lingshan wrote:
This patch introduce a set of functions for setup/unsetup
and update irq offloading respectively by register/unregister
and re-register the irq_bypass_producer.
Signed-off-by: Zhu Lingshan
---
drivers/vhost/vdpa.c | 69
1 file changed, 69 insertions(+)
diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 2fcc422..92683e4 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -115,6 +115,63 @@ static irqreturn_t vhost_vdpa_config_cb(void *private)
return IRQ_HANDLED;
}
+static void vhost_vdpa_setup_vq_irq(struct vdpa_device *dev, int qid, int irq)
+{
+ struct vhost_vdpa *v = vdpa_get_drvdata(dev);
+ struct vhost_virtqueue *vq = >vqs[qid];
+ int ret;
+
+ vq_err(vq, "setup irq bypass for vq %d with irq = %d\n", qid, irq);
+ spin_lock(>call_ctx.ctx_lock);
+ if (!vq->call_ctx.ctx)
+ return;
+
+ vq->call_ctx.producer.token = vq->call_ctx.ctx;
+ vq->call_ctx.producer.irq = irq;
+ ret = irq_bypass_register_producer(>call_ctx.producer);
+ spin_unlock(>call_ctx.ctx_lock);
+
+ if (unlikely(ret))
+ vq_err(vq,
+ "irq bypass producer (token %p registration fails: %d\n",
+ vq->call_ctx.producer.token, ret);
Not sure this deserves a vq_err(), irq will be relayed through eventfd
if irq bypass manager can't work.
+}
+
+static void vhost_vdpa_unsetup_vq_irq(struct vdpa_device *dev, int qid)
+{
+ struct vhost_vdpa *v = vdpa_get_drvdata(dev);
+ struct vhost_virtqueue *vq = >vqs[qid];
+
+ spin_lock(>call_ctx.ctx_lock);
+ irq_bypass_unregister_producer(>call_ctx.producer);
+ spin_unlock(>call_ctx.ctx_lock);
+
+ vq_err(vq, "unsetup irq bypass for vq %d\n", qid);
Why call vq_err() here?
+}
+
+static void vhost_vdpa_update_vq_irq(struct vhost_virtqueue *vq)
+{
+ struct eventfd_ctx *ctx;
+ void *token;
+
+ spin_lock(>call_ctx.ctx_lock);
+ ctx = vq->call_ctx.ctx;
+ token = vq->call_ctx.producer.token;
+ if (ctx == token)
+ return;
Need do unlock here.
+
+ if (!ctx && token)
+ irq_bypass_unregister_producer(>call_ctx.producer);
+
+ if (ctx && ctx != token) {
+ irq_bypass_unregister_producer(>call_ctx.producer);
+ vq->call_ctx.producer.token = ctx;
+ irq_bypass_register_producer(>call_ctx.producer);
+ }
+
+ spin_unlock(>call_ctx.ctx_lock);
This should be rare so I'd use simple codes just do unregister and register.
+}
+
static void vhost_vdpa_reset(struct vhost_vdpa *v)
{
struct vdpa_device *vdpa = v->vdpa;
@@ -332,6 +389,7 @@ static long vhost_vdpa_set_config_call(struct vhost_vdpa
*v, u32 __user *argp)
return 0;
}
+
Unnecessary change.
static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
void __user *argp)
{
@@ -390,6 +448,16 @@ static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v,
unsigned int cmd,
cb.private = NULL;
}
ops->set_vq_cb(vdpa, idx, );
+#ifdef CONFIG_HAVE_KVM_IRQ_BYPASS
+ /*
+* if it has a non-zero irq, means there is a
+* previsouly registered irq_bypass_producer,
+* we should update it when ctx (its token)
+* changes.
+*/
+ if (vq->call_ctx.producer.irq)
+ vhost_vdpa_update_vq_irq(vq);
+#endif
break;
case VHOST_SET_VRING_NUM:
@@ -741,6 +809,7 @@ static int vhost_vdpa_open(struct inode *inode, struct file
*filep)
vqs[i] = >vqs[i];
vqs[i]->handle_kick = handle_vq_kick;
}
+
Unnecessary change.
Thanks
vhost_dev_init(dev, vqs, nvqs, 0, 0, 0, false,
vhost_vdpa_process_iotlb_msg);
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/7] kvm/vfio: detect assigned device via irqbypass manager
On 2020/7/13 上午5:06, Michael S. Tsirkin wrote:
On Sun, Jul 12, 2020 at 10:49:21PM +0800, Zhu Lingshan wrote:
We used to detect assigned device via VFIO manipulated device
conters. This is less flexible consider VFIO is not the only
interface for assigned device. vDPA devices has dedicated
backed hardware as well. So this patch tries to detect
the assigned device via irqbypass manager.
We will increase/decrease the assigned device counter in kvm/x86.
Both vDPA and VFIO would go through this code path.
This code path only affect x86 for now.
Signed-off-by: Zhu Lingshan
I think it's best to leave VFIO alone. Add appropriate APIs for VDPA,
worry about converting existing users later.
Just to make sure I understand, did you mean:
1) introduce another bridge for vDPA
or
2) only detect vDPA via bypass manager? (we can leave VFIO code as is,
then the assigned device counter may increase/decrease twice if VFIO use
irq bypass manager which should be no harm).
Thanks
---
arch/x86/kvm/x86.c | 10 --
virt/kvm/vfio.c| 2 --
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 00c88c2..20c07d3 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10624,11 +10624,17 @@ int kvm_arch_irq_bypass_add_producer(struct
irq_bypass_consumer *cons,
{
struct kvm_kernel_irqfd *irqfd =
container_of(cons, struct kvm_kernel_irqfd, consumer);
+ int ret;
irqfd->producer = prod;
+ kvm_arch_start_assignment(irqfd->kvm);
+ ret = kvm_x86_ops.update_pi_irte(irqfd->kvm,
+prod->irq, irqfd->gsi, 1);
+
+ if (ret)
+ kvm_arch_end_assignment(irqfd->kvm);
- return kvm_x86_ops.update_pi_irte(irqfd->kvm,
- prod->irq, irqfd->gsi, 1);
+ return ret;
}
void kvm_arch_irq_bypass_del_producer(struct irq_bypass_consumer *cons,
diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
index 8fcbc50..111da52 100644
--- a/virt/kvm/vfio.c
+++ b/virt/kvm/vfio.c
@@ -226,7 +226,6 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long
attr, u64 arg)
list_add_tail(>node, >group_list);
kvg->vfio_group = vfio_group;
- kvm_arch_start_assignment(dev->kvm);
mutex_unlock(>lock);
@@ -254,7 +253,6 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg)
continue;
list_del(>node);
- kvm_arch_end_assignment(dev->kvm);
#ifdef CONFIG_SPAPR_TCE_IOMMU
kvm_spapr_tce_release_vfio_group(dev->kvm,
kvg->vfio_group);
--
1.8.3.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC]: mm,power: introduce MADV_WIPEONSUSPEND
On Sun 12-07-20 09:22:28, Pavel Machek wrote: > On Tue 2020-07-07 12:00:41, Colm MacCarthaigh wrote: > > > > > > On 7 Jul 2020, at 9:37, Pavel Machek wrote: > > > Please go through the thread and try to understand it. > > > > > > You'd need syscalls per get_randomness(), not per migration. > > > > I think one check per get_randomness() is sufficient, though putting it at > > the end of the critical section rather than the beginning helps. > > Yeah, well, one syscall is still enough to make it useless. I am sorry but I really do not follow. Why would you want to call a syscall on each get_randomness invocation? Why is it not enough to simply have a flag that tells that an external event has happened and reinitialize if the flag is set? Yes this wouldn't be really sync operation but does that matter? Is using a few random numbers from the old pool just because the notifier hasn't processed and flag the situation a major security concern? Btw. let me just clarify that I am not by any means pushing a solution like that. All I am saying is that MADV_WIPEONSUSPEND is inherently subtle interface that we likely want to avoid. -- Michal Hocko SUSE Labs ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 12/24] scsi: virtio_scsi: Demote seemingly unintentional kerneldoc header
This is the only use of kerneldoc in the sourcefile and no descriptions are provided. Fixes the following W=1 kernel build warning(s): drivers/scsi/virtio_scsi.c:109: warning: Function parameter or member 'vscsi' not described in 'virtscsi_complete_cmd' drivers/scsi/virtio_scsi.c:109: warning: Function parameter or member 'buf' not described in 'virtscsi_complete_cmd' Cc: "Michael S. Tsirkin" Cc: Jason Wang Cc: Stefan Hajnoczi Cc: virtualization@lists.linux-foundation.org Signed-off-by: Lee Jones Acked-by: Paolo Bonzini --- drivers/scsi/virtio_scsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c index 0e0910c5b9424..56875467e4984 100644 --- a/drivers/scsi/virtio_scsi.c +++ b/drivers/scsi/virtio_scsi.c @@ -100,7 +100,7 @@ static void virtscsi_compute_resid(struct scsi_cmnd *sc, u32 resid) scsi_set_resid(sc, resid); } -/** +/* * virtscsi_complete_cmd - finish a scsi_cmd and invoke scsi_done * * Called with vq_lock held. -- 2.25.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/7] kvm/vfio: detect assigned device via irqbypass manager
On 2020/7/12 下午11:29, Alex Williamson wrote:
On Sun, 12 Jul 2020 22:49:21 +0800
Zhu Lingshan wrote:
We used to detect assigned device via VFIO manipulated device
conters. This is less flexible consider VFIO is not the only
interface for assigned device. vDPA devices has dedicated
backed hardware as well. So this patch tries to detect
the assigned device via irqbypass manager.
We will increase/decrease the assigned device counter in kvm/x86.
Both vDPA and VFIO would go through this code path.
This code path only affect x86 for now.
No it doesn't, it only adds VFIO support to x86, but it removes it from
architecture neutral code.
Do you mean we should introduce a kvm_irq_bypass_add_producer and do
kvm_arch_start_assignment( ) there?
Also a VFIO device does not necessarily
make use of the irqbypass manager, this depends on platform support and
enablement of this feature.
Yes, we should keep the VFIO part unchanged.
Thanks
Therefore, NAK. Thanks,
Alex
Signed-off-by: Zhu Lingshan
---
arch/x86/kvm/x86.c | 10 --
virt/kvm/vfio.c| 2 --
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 00c88c2..20c07d3 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10624,11 +10624,17 @@ int kvm_arch_irq_bypass_add_producer(struct
irq_bypass_consumer *cons,
{
struct kvm_kernel_irqfd *irqfd =
container_of(cons, struct kvm_kernel_irqfd, consumer);
+ int ret;
irqfd->producer = prod;
+ kvm_arch_start_assignment(irqfd->kvm);
+ ret = kvm_x86_ops.update_pi_irte(irqfd->kvm,
+prod->irq, irqfd->gsi, 1);
+
+ if (ret)
+ kvm_arch_end_assignment(irqfd->kvm);
- return kvm_x86_ops.update_pi_irte(irqfd->kvm,
- prod->irq, irqfd->gsi, 1);
+ return ret;
}
void kvm_arch_irq_bypass_del_producer(struct irq_bypass_consumer *cons,
diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
index 8fcbc50..111da52 100644
--- a/virt/kvm/vfio.c
+++ b/virt/kvm/vfio.c
@@ -226,7 +226,6 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long
attr, u64 arg)
list_add_tail(>node, >group_list);
kvg->vfio_group = vfio_group;
- kvm_arch_start_assignment(dev->kvm);
mutex_unlock(>lock);
@@ -254,7 +253,6 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg)
continue;
list_del(>node);
- kvm_arch_end_assignment(dev->kvm);
#ifdef CONFIG_SPAPR_TCE_IOMMU
kvm_spapr_tce_release_vfio_group(dev->kvm,
kvg->vfio_group);
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[vhost:config-endian 38/39] drivers/platform/mellanox/mlxbf-tmfifo.c:1241:22: error: expected ')' before ';' token
tree: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git
config-endian
head: df43c8f58f42ec36e91740f91ea7360f63213004
commit: e1e22056bc3641f340ed27012cfd1b8b05f83a0a [38/39] fixup! virtio_net:
correct tags for config space fields
config: arm64-allyesconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout e1e22056bc3641f340ed27012cfd1b8b05f83a0a
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross
ARCH=arm64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>):
drivers/platform/mellanox/mlxbf-tmfifo.c: In function 'mlxbf_tmfifo_probe':
drivers/platform/mellanox/mlxbf-tmfifo.c:1237:70: warning: value computed is
not used [-Wunused-value]
1237 | #define MLXBF_TMFIFO_LITTLE_ENDIAN (virtio_legacy_is_little_endian()
|| \
|
~^~~~
1238 |(MLXBF_TMFIFO_NET_FEATURES & (1ULL << VIRTIO_F_VERSION_1))
|~~
drivers/platform/mellanox/mlxbf-tmfifo.c:1240:37: note: in expansion of
macro 'MLXBF_TMFIFO_LITTLE_ENDIAN'
1240 | net_config.mtu = __cpu_to_virtio16(MLXBF_TMFIFO_LITTLE_ENDIAN,
| ^~
>> drivers/platform/mellanox/mlxbf-tmfifo.c:1241:22: error: expected ')' before
>> ';' token
1241 | ETH_DATA_LEN);
| ^
| )
>> drivers/platform/mellanox/mlxbf-tmfifo.c:1240:19: error: too few arguments
>> to function '__cpu_to_virtio16'
1240 | net_config.mtu = __cpu_to_virtio16(MLXBF_TMFIFO_LITTLE_ENDIAN,
| ^
In file included from include/linux/virtio_config.h:8,
from drivers/platform/mellanox/mlxbf-tmfifo.c:18:
include/linux/virtio_byteorder.h:24:26: note: declared here
24 | static inline __virtio16 __cpu_to_virtio16(bool little_endian, u16
val)
| ^
>> drivers/platform/mellanox/mlxbf-tmfifo.c:1290:1: error: expected declaration
>> or statement at end of input
1290 | MODULE_AUTHOR("Mellanox Technologies");
| ^
>> drivers/platform/mellanox/mlxbf-tmfifo.c:1232:3: error: label 'fail' used
>> but not defined
1232 | goto fail;
| ^~~~
At top level:
drivers/platform/mellanox/mlxbf-tmfifo.c:1183:12: warning:
'mlxbf_tmfifo_probe' defined but not used [-Wunused-function]
1183 | static int mlxbf_tmfifo_probe(struct platform_device *pdev)
|^~
drivers/platform/mellanox/mlxbf-tmfifo.c:1170:13: warning:
'mlxbf_tmfifo_cleanup' defined but not used [-Wunused-function]
1170 | static void mlxbf_tmfifo_cleanup(struct mlxbf_tmfifo *fifo)
| ^~~~
drivers/platform/mellanox/mlxbf-tmfifo.c:1128:13: warning:
'mlxbf_tmfifo_get_cfg_mac' defined but not used [-Wunused-function]
1128 | static void mlxbf_tmfifo_get_cfg_mac(u8 *mac)
| ^~~~
vim +1241 drivers/platform/mellanox/mlxbf-tmfifo.c
1181
1182 /* Probe the TMFIFO. */
1183 static int mlxbf_tmfifo_probe(struct platform_device *pdev)
1184 {
1185 struct virtio_net_config net_config;
1186 struct device *dev = >dev;
1187 struct mlxbf_tmfifo *fifo;
1188 int i, rc;
1189
1190 fifo = devm_kzalloc(dev, sizeof(*fifo), GFP_KERNEL);
1191 if (!fifo)
1192 return -ENOMEM;
1193
1194 spin_lock_init(>spin_lock[0]);
1195 spin_lock_init(>spin_lock[1]);
1196 INIT_WORK(>work, mlxbf_tmfifo_work_handler);
1197 mutex_init(>lock);
1198
1199 /* Get the resource of the Rx FIFO. */
1200 fifo->rx_base = devm_platform_ioremap_resource(pdev, 0);
1201 if (IS_ERR(fifo->rx_base))
1202 return PTR_ERR(fifo->rx_base);
1203
1204 /* Get the resource of the Tx FIFO. */
1205 fifo->tx_base = devm_platform_ioremap_resource(pdev, 1);
1206 if (IS_ERR(fifo->tx_base))
1207 return PTR_ERR(fifo->tx_base);
1208
1209 platform_set_drvdata(pdev, fifo);
1210
1211 timer_setup(>timer, mlxbf_tmfifo_timer, 0);
1212
1213 for (i = 0; i < MLXBF_TM_MAX_IRQ; i++) {
1214 fifo->irq_info[i].index = i;
1215 fifo->irq_info[i].fifo = fifo;
1216 fifo->irq_info[i].irq = platform_get_irq(pdev, i);
1217
