Re: [PATCH net] virtio-net: enable big mode correctly
On Thu, Nov 25, 2021 at 03:28:31PM +0800, Jason Wang wrote: > On Thu, Nov 25, 2021 at 3:26 PM Michael S. Tsirkin wrote: > > > > On Thu, Nov 25, 2021 at 03:20:07PM +0800, Jason Wang wrote: > > > On Thu, Nov 25, 2021 at 3:15 PM Michael S. Tsirkin > > > wrote: > > > > > > > > On Thu, Nov 25, 2021 at 03:11:58PM +0800, Jason Wang wrote: > > > > > On Thu, Nov 25, 2021 at 3:00 PM Michael S. Tsirkin > > > > > wrote: > > > > > > > > > > > > On Thu, Nov 25, 2021 at 02:05:47PM +0800, Jason Wang wrote: > > > > > > > When VIRTIO_NET_F_MTU feature is not negotiated, we assume a very > > > > > > > large max_mtu. In this case, using small packet mode is not > > > > > > > correct > > > > > > > since it may breaks the networking when MTU is grater than > > > > > > > ETH_DATA_LEN. > > > > > > > > > > > > > > To have a quick fix, simply enable the big packet mode when > > > > > > > VIRTIO_NET_F_MTU is not negotiated. > > > > > > > > > > > > This will slow down dpdk hosts which disable mergeable buffers > > > > > > and send standard MTU sized packets. > > > > > > > > > > > > > We can do optimization on top. > > > > > > > > > > > > I don't think it works like this, increasing mtu > > > > > > from guest >4k never worked, > > > > > > > > > > Looking at add_recvbuf_small() it's actually GOOD_PACKET_LEN if I was > > > > > not wrong. > > > > > > > > OK, even more so then. > > > > > > > > > > we can't regress everyone's > > > > > > performance with a promise to maybe sometime bring it back. > > > > > > > > > > So consider it never work before I wonder if we can assume a 1500 as > > > > > max_mtu value instead of simply using MAX_MTU? > > > > > > > > > > Thanks > > > > > > > > You want to block guests from setting MTU to a value >GOOD_PACKET_LEN? > > > > > > Yes, or fix the issue to let large packets on RX work (e.g as the TODO > > > said, size the buffer: for <=4K mtu continue to work as > > > add_recvbuf_small(), for >= 4K switch to use big). > > > > Right. The difficulty is with changing modes, current code isn't > > designed for it. > > I think it might work if we reset the device during the mode change. > > Thanks For sure. It's hard to do without races though, and we need to carefully restore all the programming done so far. Maybe it will be easier if we do something like disable_irq to reliably suppress interrupts from hardware. > > > > > > Maybe ... it will prevent sending large packets which did work ... > > > > > > Yes, but it's strange to allow TX but not RX > > > > > > > I'd tread carefully here, and I don't think this kind of thing is net > > > > material. > > > > > > I agree consider it can't be fixed easily. > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > Reported-by: Eli Cohen > > > > > > > Cc: Eli Cohen > > > > > > > Signed-off-by: Jason Wang > > > > > > > > > > > > > > --- > > > > > > > drivers/net/virtio_net.c | 7 --- > > > > > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > > > > > index 7c43bfc1ce44..83ae3ef5eb11 100644 > > > > > > > --- a/drivers/net/virtio_net.c > > > > > > > +++ b/drivers/net/virtio_net.c > > > > > > > @@ -3200,11 +3200,12 @@ static int virtnet_probe(struct > > > > > > > virtio_device *vdev) > > > > > > > dev->mtu = mtu; > > > > > > > dev->max_mtu = mtu; > > > > > > > > > > > > > > - /* TODO: size buffers correctly in this case. */ > > > > > > > - if (dev->mtu > ETH_DATA_LEN) > > > > > > > - vi->big_packets = true; > > > > > > > } > > > > > > > > > > > > > > + /* TODO: size buffers correctly in this case. */ > > > > > > > + if (dev->max_mtu > ETH_DATA_LEN) > > > > > > > + vi->big_packets = true; > > > > > > > + > > > > > > > if (vi->any_header_sg) > > > > > > > dev->needed_headroom = vi->hdr_len; > > > > > > > > > > > > > > -- > > > > > > > 2.25.1 > > > > > > > > > > > > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] virtio/vsock: fix the transport to work with VMADDR_CID_ANY
The VMADDR_CID_ANY flag used by a socket means that the socket isn't bound to any specific CID. For example, a host vsock server may want to be bound with VMADDR_CID_ANY, so that a guest vsock client can connect to the host server with CID=VMADDR_CID_HOST (i.e. 2), and meanwhile, a host vsock client can connect to the same local server with CID=VMADDR_CID_LOCAL (i.e. 1). The current implementation sets the destination socket's svm_cid to a fixed CID value after the first client's connection, which isn't an expected operation. For example, if the guest client first connects to the host server, the server's svm_cid gets set to VMADDR_CID_HOST, then other host clients won't be able to connect to the server anymore. Reproduce steps: 1. Run the host server: socat VSOCK-LISTEN:1234,fork - 2. Run a guest client to connect to the host server: socat - VSOCK-CONNECT:2:1234 3. Run a host client to connect to the host server: socat - VSOCK-CONNECT:1:1234 Without this patch, step 3. above fails to connect, and socat complains "socat[1720] E connect(5, AF=40 cid:1 port:1234, 16): Connection reset by peer". With this patch, the above works well. Signed-off-by: Wei Wang --- net/vmw_vsock/virtio_transport_common.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c index 59ee1be5a6dd..5c60fae10569 100644 --- a/net/vmw_vsock/virtio_transport_common.c +++ b/net/vmw_vsock/virtio_transport_common.c @@ -1298,9 +1298,6 @@ void virtio_transport_recv_pkt(struct virtio_transport *t, space_available = virtio_transport_space_update(sk, pkt); - /* Update CID in case it has changed after a transport reset event */ - vsk->local_addr.svm_cid = dst.svm_cid; - if (space_available) sk->sk_write_space(sk); -- 2.25.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [RFC] hypercall-vsock: add a new vsock transport
On Thursday, November 25, 2021 2:38 PM, Jason Wang wrote: > > We thought about virtio-mmio. There are some barriers: > > 1) It wasn't originally intended for x86 machines. The only machine > > type in QEMU that supports it (to run on x86) is microvm. But > > "microvm" doesn’t support TDX currently, and adding this support might > need larger effort. > > Can you explain why microvm needs larger effort? It looks to me it fits for > TDX > perfectly since it has less attack surface. The main thing is TDVF doesn’t support microvm so far (the based OVMF support for microvm is still under their community discussion). Do you guys think it is possible to add virtio-mmio support for q35? (e.g. create a special platform bus in some fashion for memory mapped devices) Not sure if the effort would be larger. Thanks, Wei ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [PATCH] virtio/vsock: fix the transport to work with VMADDR_CID_ANY
On Thursday, November 25, 2021 3:16 PM, Wang, Wei W wrote: > - /* Update CID in case it has changed after a transport reset event */ > - vsk->local_addr.svm_cid = dst.svm_cid; > - > if (space_available) > sk->sk_write_space(sk); > Not sure if anybody knows how this affects the transport reset. Thanks, Wei ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio/vsock: fix the transport to work with VMADDR_CID_ANY
On Thu, Nov 25, 2021 at 09:27:40AM +, Wang, Wei W wrote:
On Thursday, November 25, 2021 3:16 PM, Wang, Wei W wrote:
- /* Update CID in case it has changed after a transport reset event */
- vsk->local_addr.svm_cid = dst.svm_cid;
-
if (space_available)
sk->sk_write_space(sk);
Not sure if anybody knows how this affects the transport reset.
I believe the primary use case is when a guest is migrated.
After the migration, the transport gets a reset event from the
hypervisor and all connected sockets are closed. The ones in listen
remain open though.
Also the guest's CID may have changed after migration. So if an
application has open listening sockets, bound to the old CID, this
should ensure that the socket continues to be usable.
The patch would then change this behavior.
So maybe to avoid problems, we could update the CID only if it is
different from VMADDR_CID_ANY:
if (vsk->local_addr.svm_cid != VMADDR_CID_ANY)
vsk->local_addr.svm_cid = dst.svm_cid;
When this code was written, a guest only supported a single transport,
so it could only have one CID assigned, so that wasn't a problem.
For that reason I'll add this Fixes tag:
Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
Thanks,
Stefano
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-mmio: harden interrupt
Hi Jason,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.16-rc2 next-20211125]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Jason-Wang/virtio-mmio-harden-interrupt/20211125-143334
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
5f53fa508db098c9d372423a6dac31c8a5679cdf
config: m68k-allyesconfig
(https://download.01.org/0day-ci/archive/20211125/202111251934.ybhaqyh7-...@intel.com/config)
compiler: m68k-linux-gcc (GCC) 11.2.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
#
https://github.com/0day-ci/linux/commit/e19a8a1a95bd891090863b2d6828b8dc55d3633f
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Jason-Wang/virtio-mmio-harden-interrupt/20211125-143334
git checkout e19a8a1a95bd891090863b2d6828b8dc55d3633f
# save the config file to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross
ARCH=m68k
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All warnings (new ones prefixed by >>):
>> drivers/virtio/virtio_mmio.c:105:6: warning: no previous prototype for
>> 'vm_disable_cbs' [-Wmissing-prototypes]
105 | void vm_disable_cbs(struct virtio_device *vdev)
| ^~
>> drivers/virtio/virtio_mmio.c:121:6: warning: no previous prototype for
>> 'vm_enable_cbs' [-Wmissing-prototypes]
121 | void vm_enable_cbs(struct virtio_device *vdev)
| ^
vim +/vm_disable_cbs +105 drivers/virtio/virtio_mmio.c
103
104 /* disable irq handlers */
> 105 void vm_disable_cbs(struct virtio_device *vdev)
106 {
107 struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
108 int irq = platform_get_irq(vm_dev->pdev, 0);
109
110 /*
111 * The below synchronize() guarantees that any
112 * interrupt for this line arriving after
113 * synchronize_irq() has completed is guaranteed to see
114 * intx_soft_enabled == false.
115 */
116 WRITE_ONCE(vm_dev->intr_soft_enabled, false);
117 synchronize_irq(irq);
118 }
119
120 /* enable irq handlers */
> 121 void vm_enable_cbs(struct virtio_device *vdev)
122 {
123 struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
124 int irq = platform_get_irq(vm_dev->pdev, 0);
125
126 disable_irq(irq);
127 /*
128 * The above disable_irq() provides TSO ordering and
129 * as such promotes the below store to store-release.
130 */
131 WRITE_ONCE(vm_dev->intr_soft_enabled, true);
132 enable_irq(irq);
133 return;
134 }
135
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 net-next 21/26] ice: add XDP and XSK generic per-channel statistics
Daniel Borkmann writes:
> Hi Alexander,
>
> On 11/23/21 5:39 PM, Alexander Lobakin wrote:
> [...]
>
> Just commenting on ice here as one example (similar applies to other drivers):
>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
>> b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
>> index 1dd7e84f41f8..7dc287bc3a1a 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
>> @@ -258,6 +258,8 @@ static void ice_clean_xdp_irq(struct ice_tx_ring
>> *xdp_ring)
>> xdp_ring->next_dd = ICE_TX_THRESH - 1;
>> xdp_ring->next_to_clean = ntc;
>> ice_update_tx_ring_stats(xdp_ring, total_pkts, total_bytes);
>> +xdp_update_tx_drv_stats(&xdp_ring->xdp_stats->xdp_tx, total_pkts,
>> +total_bytes);
>> }
>>
>> /**
>> @@ -277,6 +279,7 @@ int ice_xmit_xdp_ring(void *data, u16 size, struct
>> ice_tx_ring *xdp_ring)
>> ice_clean_xdp_irq(xdp_ring);
>>
>> if (!unlikely(ICE_DESC_UNUSED(xdp_ring))) {
>> +xdp_update_tx_drv_full(&xdp_ring->xdp_stats->xdp_tx);
>> xdp_ring->tx_stats.tx_busy++;
>> return ICE_XDP_CONSUMED;
>> }
>> diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c
>> b/drivers/net/ethernet/intel/ice/ice_xsk.c
>> index ff55cb415b11..62ef47a38d93 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_xsk.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
>> @@ -454,42 +454,58 @@ ice_construct_skb_zc(struct ice_rx_ring *rx_ring,
>> struct xdp_buff **xdp_arr)
>>* @xdp: xdp_buff used as input to the XDP program
>>* @xdp_prog: XDP program to run
>>* @xdp_ring: ring to be used for XDP_TX action
>> + * @lrstats: onstack Rx XDP stats
>>*
>>* Returns any of ICE_XDP_{PASS, CONSUMED, TX, REDIR}
>>*/
>> static int
>> ice_run_xdp_zc(struct ice_rx_ring *rx_ring, struct xdp_buff *xdp,
>> - struct bpf_prog *xdp_prog, struct ice_tx_ring *xdp_ring)
>> + struct bpf_prog *xdp_prog, struct ice_tx_ring *xdp_ring,
>> + struct xdp_rx_drv_stats_local *lrstats)
>> {
>> int err, result = ICE_XDP_PASS;
>> u32 act;
>>
>> +lrstats->bytes += xdp->data_end - xdp->data;
>> +lrstats->packets++;
>> +
>> act = bpf_prog_run_xdp(xdp_prog, xdp);
>>
>> if (likely(act == XDP_REDIRECT)) {
>> err = xdp_do_redirect(rx_ring->netdev, xdp, xdp_prog);
>> -if (err)
>> +if (err) {
>> +lrstats->redirect_errors++;
>> goto out_failure;
>> +}
>> +lrstats->redirect++;
>> return ICE_XDP_REDIR;
>> }
>>
>> switch (act) {
>> case XDP_PASS:
>> +lrstats->pass++;
>> break;
>> case XDP_TX:
>> result = ice_xmit_xdp_buff(xdp, xdp_ring);
>> -if (result == ICE_XDP_CONSUMED)
>> +if (result == ICE_XDP_CONSUMED) {
>> +lrstats->tx_errors++;
>> goto out_failure;
>> +}
>> +lrstats->tx++;
>> break;
>> default:
>> bpf_warn_invalid_xdp_action(act);
>> -fallthrough;
>> +lrstats->invalid++;
>> +goto out_failure;
>> case XDP_ABORTED:
>> +lrstats->aborted++;
>> out_failure:
>> trace_xdp_exception(rx_ring->netdev, xdp_prog, act);
>> -fallthrough;
>> +result = ICE_XDP_CONSUMED;
>> +break;
>> case XDP_DROP:
>> result = ICE_XDP_CONSUMED;
>> +lrstats->drop++;
>> break;
>> }
>
> Imho, the overall approach is way too bloated. I can see the
> packets/bytes but now we have 3 counter updates with return codes
> included and then the additional sync of the on-stack counters into
> the ring counters via xdp_update_rx_drv_stats(). So we now need
> ice_update_rx_ring_stats() as well as xdp_update_rx_drv_stats() which
> syncs 10 different stat counters via u64_stats_add() into the per ring
> ones. :/
>
> I'm just taking our XDP L4LB in Cilium as an example: there we already
> count errors and export them via per-cpu map that eventually lead to
> XDP_DROP cases including the /reason/ which caused the XDP_DROP (e.g.
> Prometheus can then scrape these insights from all the nodes in the
> cluster). Given the different action codes are very often application
> specific, there's not much debugging that you can do when /only/
> looking at `ip link xdpstats` to gather insight on *why* some of these
> actions were triggered (e.g. fib lookup failure, etc). If really of
> interest, then maybe libxdp could have such per-action counters as
> opt-in in its call chain..
To me, standardising these counters is less about helping people debug
their XDP programs (as you say, you can put your own telemetry into
those), and more about making XDP less "mystical" to the system
administrator (who may not be the sam
Re: [RFC] hypercall-vsock: add a new vsock transport
On Thu, Nov 25, 2021 at 08:43:55AM +, Wang, Wei W wrote:
> On Thursday, November 25, 2021 2:38 PM, Jason Wang wrote:
> > > We thought about virtio-mmio. There are some barriers:
> > > 1) It wasn't originally intended for x86 machines. The only machine
> > > type in QEMU that supports it (to run on x86) is microvm. But
> > > "microvm" doesn’t support TDX currently, and adding this support might
> > need larger effort.
> >
> > Can you explain why microvm needs larger effort? It looks to me it fits for
> > TDX
> > perfectly since it has less attack surface.
>
> The main thing is TDVF doesn’t support microvm so far (the based OVMF
> support for microvm is still under their community discussion).
Initial microvm support (direct kernel boot only) is merged in upstream
OVMF. Better device support is underway: virtio-mmio patches are out
for review, patches for pcie support exist.
TDX patches for OVMF are under review upstream, I havn't noticed
anything which would be a blocker for microvm. If it doesn't work
out-of-the-box it should be mostly wiring up things needed on guest
(ovmf) and/or host (qemu) side.
(same goes for sev btw).
> Do you guys think it is possible to add virtio-mmio support for q35?
> (e.g. create a special platform bus in some fashion for memory mapped devices)
> Not sure if the effort would be larger.
I'd rather explore the microvm path than making q35 even more
frankenstein than it already is.
Also the pcie host bridge is present in q35 no matter what, so one of
the reasons to use virtio-mmio ("we can reduce the attach surface by
turning off pcie") goes away.
take care,
Gerd
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-mmio: harden interrupt
Hi Jason,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.16-rc2 next-20211125]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Jason-Wang/virtio-mmio-harden-interrupt/20211125-143334
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
5f53fa508db098c9d372423a6dac31c8a5679cdf
config: mips-buildonly-randconfig-r003-20211125
(https://download.01.org/0day-ci/archive/20211125/202111252001.z5tli1np-...@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project
67a1c45def8a75061203461ab0060c75c864df1c)
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
# install mips cross compiling tool for clang build
# apt-get install binutils-mips-linux-gnu
#
https://github.com/0day-ci/linux/commit/e19a8a1a95bd891090863b2d6828b8dc55d3633f
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Jason-Wang/virtio-mmio-harden-interrupt/20211125-143334
git checkout e19a8a1a95bd891090863b2d6828b8dc55d3633f
# save the config file to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1
ARCH=mips
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All warnings (new ones prefixed by >>):
>> drivers/virtio/virtio_mmio.c:105:6: warning: no previous prototype for
>> function 'vm_disable_cbs' [-Wmissing-prototypes]
void vm_disable_cbs(struct virtio_device *vdev)
^
drivers/virtio/virtio_mmio.c:105:1: note: declare 'static' if the function
is not intended to be used outside of this translation unit
void vm_disable_cbs(struct virtio_device *vdev)
^
static
>> drivers/virtio/virtio_mmio.c:121:6: warning: no previous prototype for
>> function 'vm_enable_cbs' [-Wmissing-prototypes]
void vm_enable_cbs(struct virtio_device *vdev)
^
drivers/virtio/virtio_mmio.c:121:1: note: declare 'static' if the function
is not intended to be used outside of this translation unit
void vm_enable_cbs(struct virtio_device *vdev)
^
static
2 warnings generated.
vim +/vm_disable_cbs +105 drivers/virtio/virtio_mmio.c
103
104 /* disable irq handlers */
> 105 void vm_disable_cbs(struct virtio_device *vdev)
106 {
107 struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
108 int irq = platform_get_irq(vm_dev->pdev, 0);
109
110 /*
111 * The below synchronize() guarantees that any
112 * interrupt for this line arriving after
113 * synchronize_irq() has completed is guaranteed to see
114 * intx_soft_enabled == false.
115 */
116 WRITE_ONCE(vm_dev->intr_soft_enabled, false);
117 synchronize_irq(irq);
118 }
119
120 /* enable irq handlers */
> 121 void vm_enable_cbs(struct virtio_device *vdev)
122 {
123 struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
124 int irq = platform_get_irq(vm_dev->pdev, 0);
125
126 disable_irq(irq);
127 /*
128 * The above disable_irq() provides TSO ordering and
129 * as such promotes the below store to store-release.
130 */
131 WRITE_ONCE(vm_dev->intr_soft_enabled, true);
132 enable_irq(irq);
133 return;
134 }
135
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] net/mlx5_vdpa: Increase the limit on the number of virtuques
On Thu, Nov 25, 2021 at 09:29:53AM +0200, Eli Cohen wrote:
> On Thu, Nov 25, 2021 at 02:21:43AM -0500, Michael S. Tsirkin wrote:
> > On Wed, Nov 24, 2021 at 07:19:53PM +0200, Eli Cohen wrote:
> > > Increase the limit on the maximum number of supported virtqueues to 256
> > > to match hardware capabilities.
> >
> > Hmm and are we going to have to tweak it each time new hardware/firmware
> > is out? Can't this be queried in some way?
>
> I thought to make the allocation dynamic once we have support for
> setting max queues through vdpa tool.
Well this will make things a bit harder to figure out then,
right now you can assume no vdpa tool support -> max 16 VQs.
The patch breaks this. Is there a motivation to up this right now,
or should we just wait a bit for vdpa tool support?
> > In fact there's a suggestion in code to remove the limitation -
> > any plans to do this?
>
> Can you be more speicifc, where?
It's right there in the patch:
/* We will remove this limitation once mlx5_vdpa_alloc_resources()
* provides for driver space allocation
*/
>
> >
> > > Signed-off-by: Eli Cohen
> >
> > typo in subject
> What typo?
virtuques. ispell is your friend.
> >
> > > ---
> > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > index ed7a63e48335..8f2918a8efc6 100644
> > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > @@ -135,7 +135,7 @@ struct mlx5_vdpa_virtqueue {
> > > /* We will remove this limitation once mlx5_vdpa_alloc_resources()
> > > * provides for driver space allocation
> > > */
> > > -#define MLX5_MAX_SUPPORTED_VQS 16
> > > +#define MLX5_MAX_SUPPORTED_VQS 256
> > >
> > > static bool is_index_valid(struct mlx5_vdpa_dev *mvdev, u16 idx)
> > > {
> > > --
> > > 2.33.1
> >
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] Bluetooth: virtio_bt: fix device removal
Device removal is clearly out of virtio spec: it attempts to remove
unused buffers from a VQ before invoking device reset. To fix, make
open/close NOPs and do all cleanup/setup in probe/remove.
The cost here is a single skb wasted on an unused bt device - which
seems modest.
NB: with this fix in place driver still suffers from a race condition if
an interrupt triggers while device is being reset. Work on a fix for
that issue is in progress.
Signed-off-by: Michael S. Tsirkin
---
Note: completely untested, in particular the device isn't supported in QEMU.
Please do not queue directly - please help review and test and ack,
and I will queue this together with reset fixes.
Thanks!
drivers/bluetooth/virtio_bt.c | 18 --
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/bluetooth/virtio_bt.c b/drivers/bluetooth/virtio_bt.c
index 24a9258962fa..aea33ba9522c 100644
--- a/drivers/bluetooth/virtio_bt.c
+++ b/drivers/bluetooth/virtio_bt.c
@@ -50,8 +50,11 @@ static int virtbt_add_inbuf(struct virtio_bluetooth *vbt)
static int virtbt_open(struct hci_dev *hdev)
{
- struct virtio_bluetooth *vbt = hci_get_drvdata(hdev);
+ return 0;
+}
+static int virtbt_open_vdev(struct virtio_bluetooth *vbt)
+{
if (virtbt_add_inbuf(vbt) < 0)
return -EIO;
@@ -61,7 +64,11 @@ static int virtbt_open(struct hci_dev *hdev)
static int virtbt_close(struct hci_dev *hdev)
{
- struct virtio_bluetooth *vbt = hci_get_drvdata(hdev);
+ return 0;
+}
+
+static int virtbt_close_vdev(struct virtio_bluetooth *vbt)
+{
int i;
cancel_work_sync(&vbt->rx);
@@ -351,8 +358,14 @@ static int virtbt_probe(struct virtio_device *vdev)
goto failed;
}
+ virtio_device_ready(vdev);
+ if (virtbt_open_vdev(vbt))
+ goto open_failed;
+
return 0;
+open_failed:
+ hci_free_dev(hdev);
failed:
vdev->config->del_vqs(vdev);
return err;
@@ -365,6 +378,7 @@ static void virtbt_remove(struct virtio_device *vdev)
hci_unregister_dev(hdev);
vdev->config->reset(vdev);
+ virtbt_close_vdev(vbt);
hci_free_dev(hdev);
vbt->hdev = NULL;
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] vdpa: Consider device id larger than 31
virtio device id value can be more than 31. Hence, use BIT_ULL in
assignment.
Fixes: 33b347503f01 ("vdpa: Define vdpa mgmt device, ops and a netlink
interface")
Reported-by: kernel test robot
Reported-by: Dan Carpenter
Signed-off-by: Parav Pandit
---
drivers/vdpa/vdpa.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 7332a74a4b00..e91c71aeeddf 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -404,7 +404,7 @@ static int vdpa_mgmtdev_fill(const struct vdpa_mgmt_dev
*mdev, struct sk_buff *m
goto msg_err;
while (mdev->id_table[i].device) {
- supported_classes |= BIT(mdev->id_table[i].device);
+ supported_classes |= BIT_ULL(mdev->id_table[i].device);
i++;
}
--
2.26.2
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [PATCH] net/mlx5_vdpa: Offer VIRTIO_NET_F_MTU when setting MTU
> From: Eli Cohen
> Sent: Thursday, November 25, 2021 1:32 PM
>
> On Thu, Nov 25, 2021 at 07:29:18AM +0200, Parav Pandit wrote:
> >
> >
> > > From: Eli Cohen
> > > Sent: Wednesday, November 24, 2021 10:40 PM
> > >
> > > Make sure to offer VIRTIO_NET_F_MTU since we configure the MTU based
> > > on what was queried from the device.
> > >
> > > This allows the virtio driver to allocate large enough buffers based
> > > on the reported MTU.
> > >
> > > Signed-off-by: Eli Cohen
> > > ---
> > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > index 465e832f2ad1..ed7a63e48335 100644
> > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > @@ -1956,6 +1956,7 @@ static u64 mlx5_vdpa_get_features(struct
> > > vdpa_device *vdev)
> > > ndev->mvdev.mlx_features |=
> > > BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR);
> > > ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MQ);
> > > ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_STATUS);
> > > + ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MTU);
> > It is better to set this feature bit along with the writing the RO
> > config.mtu
> area, adjacent to query_mtu() call.
>
> Why is that so important? We always query the mtu and if the query fails, we
> fail the initialization so it does not look critical.
It is not so important. But it is more appropriate to set read only field and
its associated feature bit at one place, which never changes.
There is no need to perform OR operation for those many feature bits on every
get_features() callback when they are immutable.
So I wanted to move setting other 5 feature bits assignment at one place in
device initialization time similar to how you have done VALID_FEATURES_MASK.
Something like below.
But again, its minor.
If you happen to revise the series (I think you should for the supporting
dumpit in future), please consider one time assignment like below.
diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 63813fbb5f62..21802b25b0f5 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -1890,11 +1890,6 @@ static u64 mlx5_vdpa_get_features(struct vdpa_device
*vdev)
ndev->mvdev.mlx_features |= mlx_to_vritio_features(dev_features);
if (MLX5_CAP_DEV_VDPA_EMULATION(mvdev->mdev, virtio_version_1_0))
ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_F_VERSION_1);
- ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_F_ACCESS_PLATFORM);
- ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_CTRL_VQ);
- ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR);
- ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MQ);
- ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_STATUS);
print_features(mvdev, ndev->mvdev.mlx_features, false);
return ndev->mvdev.mlx_features;
@@ -2522,6 +2517,14 @@ static int event_handler(struct notifier_block *nb,
unsigned long event, void *p
return ret;
}
+#define DEFAULT_FEATURES \
+ BIT_ULL(VIRTIO_F_ACCESS_PLATFORM) | \
+ BIT_ULL(VIRTIO_NET_F_CTRL_VQ) | \
+ BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR) \
+ BIT_ULL(VIRTIO_NET_F_MQ) \
+ BIT_ULL(VIRTIO_NET_F_STATUS) \
+ BIT_ULL(VIRTIO_NET_F_MTU)
+
static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
const struct vdpa_dev_set_config *add_config)
{
@@ -2565,6 +2568,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev
*v_mdev, const char *name,
goto err_mtu;
ndev->config.mtu = cpu_to_mlx5vdpa16(mvdev, mtu);
+ ndev->mvdev.mlx_features = DEFAULT_FEATURES;
if (get_link_state(mvdev))
ndev->config.status |= cpu_to_mlx5vdpa16(mvdev,
VIRTIO_NET_S_LINK_UP);
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [PATH v1 1/2] vdpa: Add support for querying vendor statistics
> From: Eli Cohen
> Sent: Thursday, November 25, 2021 1:37 PM
>
> On Thu, Nov 25, 2021 at 07:34:21AM +0200, Parav Pandit wrote:
> >
> >
> > > From: Eli Cohen
> > > Sent: Wednesday, November 24, 2021 10:26 PM
> > >
> > > Add support for querying virtqueue statistics. Supported statistics are:
> > >
> > > received_desc - number of descriptors received for the virtqueue
> > > completed_desc - number of descriptors completed for the virtqueue
> > >
> > > A descriptors using indirect buffers is still counted as 1. In
> > > addition, N chained descriptors are counted correctly N times as one would
> expect.
> > >
> > > A new callback was added to vdpa_config_ops which provides the means
> > > for the vdpa driver to return statistics results.
> > >
> > > The interface allows for reading all the supported virtqueues,
> > > including the control virtqueue if it exists, by returning the next queue
> index to query.
> > >
> > > Examples:
> > > 1. Read statisitics for the virtqueue at index 1 $ vdpa dev vstats
> > > show vdpa-a qidx 0
> > > vdpa-a:
> > > qidx 0 rx received_desc 256 completed_desc 9
> > >
> > > 2. Read statisitics for all the virtqueues $ vdpa dev vstats show
> > > vdpa-a
> > > vdpa-a:
> > > qidx 0 rx received_desc 256 completed_desc 9 qidx 1 tx received_desc
> > > 21 completed_desc 21 qidx 2 ctrl received_desc 0 completed_desc 0
> > >
> > > Signed-off-by: Eli Cohen
> > > ---
> > > v0 -> v1:
> > > Emphasize that we're dealing with vendor specific counters.
> > > Terminate query loop on error
> > >
> > > drivers/vdpa/vdpa.c | 144
> ++
> > > include/linux/vdpa.h | 10 +++
> > > include/uapi/linux/vdpa.h | 9 +++
> > > 3 files changed, 163 insertions(+)
> > >
> > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index
> > > 7332a74a4b00..da658c80ff2a 100644
> > > --- a/drivers/vdpa/vdpa.c
> > > +++ b/drivers/vdpa/vdpa.c
> > > @@ -781,6 +781,90 @@ vdpa_dev_config_fill(struct vdpa_device *vdev,
> > > struct sk_buff *msg, u32 portid,
> > > return err;
> > > }
> > >
> > > +static int vdpa_fill_stats_rec(struct vdpa_device *vdev, struct
> > > +sk_buff *msg, u16 *index) {
> > > + struct vdpa_vq_stats stats;
> > Better to name it vdpa_vq_vstats as this is reflecting vendor stats.
> ok
> > > +
> > > +static int vdpa_dev_net_stats_fill(struct vdpa_device *vdev, struct
> > vdpa_dev_net_vstats_fill
> ok
> >
> > > +sk_buff *msg, u16 index) {
> > > + int err;
> > > +
> > > + if (!vdev->config->get_vq_stats)
> > > + return -EOPNOTSUPP;
> > > +
> > > + if (index != VDPA_INVAL_QUEUE_INDEX) {
> > > + err = vdpa_fill_stats_rec(vdev, msg, &index);
> > > + if (err)
> > > + return err;
> > > + } else {
> > > + index = 0;
> > > + do {
> > > + err = vdpa_fill_stats_rec(vdev, msg, &index);
> > > + if (err)
> > > + return err;
> > > + } while (index != VDPA_INVAL_QUEUE_INDEX);
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int vdpa_dev_stats_fill(struct vdpa_device *vdev, struct
> > > +sk_buff *msg,
> > > u32 portid,
> > > +u32 seq, int flags, u16 index) {
> > > + u32 device_id;
> > > + void *hdr;
> > > + int err;
> > > +
> > > + hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
> > > + VDPA_CMD_DEV_VSTATS_GET);
> > > + if (!hdr)
> > > + return -EMSGSIZE;
> > > +
> > > + if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev-
> > > >dev))) {
> > > + err = -EMSGSIZE;
> > > + goto undo_msg;
> > > + }
> > > +
> > > + device_id = vdev->config->get_device_id(vdev);
> > > + if (nla_put_u32(msg, VDPA_ATTR_DEV_ID, device_id)) {
> > > + err = -EMSGSIZE;
> > > + goto undo_msg;
> > > + }
> > > +
> > > + err = vdpa_dev_net_stats_fill(vdev, msg, index);
> > > +
> > > + genlmsg_end(msg, hdr);
> > > +
> > > + return err;
> > > +
> > > +undo_msg:
> > > + genlmsg_cancel(msg, hdr);
> > > + return err;
> > > +}
> > > +
> > > static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb,
> > > struct genl_info *info) {
> > > struct vdpa_device *vdev;
> > > @@ -862,6 +946,59 @@ vdpa_nl_cmd_dev_config_get_dumpit(struct
> > > sk_buff *msg, struct netlink_callback *
> > > return msg->len;
> > > }
> > >
> > > +static int vdpa_nl_cmd_dev_stats_get_doit(struct sk_buff *skb,
> > > + struct genl_info *info)
> > > +{
> > > + struct vdpa_device *vdev;
> > > + struct sk_buff *msg;
> > > + const char *devname;
> > > + struct device *dev;
> > > + u16 index = -1;
> > > + int err;
> > > +
> > > + if (!info->attrs[VDPA_ATTR_DEV_NAME])
> > > + return -EINVAL;
> > > +
> > > + if (info->attrs[VDPA_ATTR_DEV_QUEUE_INDEX])
> > > + index = nla_get_u16(info-
> > > >attrs[VDPA_ATTR_DEV_QUEUE_INDEX]);
> > > +
> > > + devname = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
> > > + msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KER
Re: [PATCH] net/mlx5_vdpa: Offer VIRTIO_NET_F_MTU when setting MTU
On Thu, Nov 25, 2021 at 06:27:15PM +, Parav Pandit wrote:
>
>
> > From: Eli Cohen
> > Sent: Thursday, November 25, 2021 1:32 PM
> >
> > On Thu, Nov 25, 2021 at 07:29:18AM +0200, Parav Pandit wrote:
> > >
> > >
> > > > From: Eli Cohen
> > > > Sent: Wednesday, November 24, 2021 10:40 PM
> > > >
> > > > Make sure to offer VIRTIO_NET_F_MTU since we configure the MTU based
> > > > on what was queried from the device.
> > > >
> > > > This allows the virtio driver to allocate large enough buffers based
> > > > on the reported MTU.
> > > >
> > > > Signed-off-by: Eli Cohen
> > > > ---
> > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 1 +
> > > > 1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > index 465e832f2ad1..ed7a63e48335 100644
> > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > @@ -1956,6 +1956,7 @@ static u64 mlx5_vdpa_get_features(struct
> > > > vdpa_device *vdev)
> > > > ndev->mvdev.mlx_features |=
> > > > BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR);
> > > > ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MQ);
> > > > ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_STATUS);
> > > > + ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MTU);
> > > It is better to set this feature bit along with the writing the RO
> > > config.mtu
> > area, adjacent to query_mtu() call.
> >
> > Why is that so important? We always query the mtu and if the query fails, we
> > fail the initialization so it does not look critical.
> It is not so important. But it is more appropriate to set read only field and
> its associated feature bit at one place, which never changes.
> There is no need to perform OR operation for those many feature bits on every
> get_features() callback when they are immutable.
>
> So I wanted to move setting other 5 feature bits assignment at one place in
> device initialization time similar to how you have done VALID_FEATURES_MASK.
> Something like below.
> But again, its minor.
> If you happen to revise the series (I think you should for the supporting
> dumpit in future), please consider one time assignment like below.
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 63813fbb5f62..21802b25b0f5 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -1890,11 +1890,6 @@ static u64 mlx5_vdpa_get_features(struct vdpa_device
> *vdev)
> ndev->mvdev.mlx_features |= mlx_to_vritio_features(dev_features);
> if (MLX5_CAP_DEV_VDPA_EMULATION(mvdev->mdev, virtio_version_1_0))
> ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_F_VERSION_1);
> - ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_F_ACCESS_PLATFORM);
> - ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_CTRL_VQ);
> - ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR);
> - ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MQ);
> - ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_STATUS);
>
> print_features(mvdev, ndev->mvdev.mlx_features, false);
> return ndev->mvdev.mlx_features;
> @@ -2522,6 +2517,14 @@ static int event_handler(struct notifier_block *nb,
> unsigned long event, void *p
> return ret;
> }
>
> +#define DEFAULT_FEATURES \
> + BIT_ULL(VIRTIO_F_ACCESS_PLATFORM) | \
> + BIT_ULL(VIRTIO_NET_F_CTRL_VQ) | \
> + BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR) \
> + BIT_ULL(VIRTIO_NET_F_MQ) \
> + BIT_ULL(VIRTIO_NET_F_STATUS) \
> + BIT_ULL(VIRTIO_NET_F_MTU)
> +
I would just open-code it, don't see what good does the macro do.
A single assignment is a bit clearer I think I agree,
there's not much of a difference.
> static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> const struct vdpa_dev_set_config *add_config)
> {
> @@ -2565,6 +2568,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev
> *v_mdev, const char *name,
> goto err_mtu;
>
> ndev->config.mtu = cpu_to_mlx5vdpa16(mvdev, mtu);
> + ndev->mvdev.mlx_features = DEFAULT_FEATURES;
>
> if (get_link_state(mvdev))
> ndev->config.status |= cpu_to_mlx5vdpa16(mvdev,
> VIRTIO_NET_S_LINK_UP);
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] Bluetooth: virtio_bt: fix device removal
On Thu, Nov 25, 2021 at 09:02:01PM +0100, Marcel Holtmann wrote:
> Hi Michael,
>
> > Device removal is clearly out of virtio spec: it attempts to remove
> > unused buffers from a VQ before invoking device reset. To fix, make
> > open/close NOPs and do all cleanup/setup in probe/remove.
>
> so the virtbt_{open,close} as NOP is not really what a driver is suppose
> to be doing. These are transport enable/disable callbacks from the BT
> Core towards the driver. It maps to a device being enabled/disabled by
> something like bluetoothd for example. So if disabled, I expect that no
> resources/queues are in use.
>
> Maybe I misunderstand the virtio spec in that regard, but I would like
> to keep this fundamental concept of a Bluetooth driver. It does work
> with all other transports like USB, SDIO, UART etc.
>
> > The cost here is a single skb wasted on an unused bt device - which
> > seems modest.
>
> There should be no buffer used if the device is powered off. We also don’t
> have any USB URBs in-flight if the transport is not active.
>
> > NB: with this fix in place driver still suffers from a race condition if
> > an interrupt triggers while device is being reset. Work on a fix for
> > that issue is in progress.
>
> In the virtbt_close() callback we should deactivate all interrupts.
>
> Regards
>
> Marcel
If you want to do that then device has to be reset on close,
and fully reinitialized on open.
Can you work on a patch like that?
Given I don't have the device such a rework is probably more
than I can undertake.
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] Bluetooth: virtio_bt: fix device removal
On Thu, Nov 25, 2021 at 10:01:25PM +0100, Marcel Holtmann wrote:
> Hi Michael,
>
> >>> Device removal is clearly out of virtio spec: it attempts to remove
> >>> unused buffers from a VQ before invoking device reset. To fix, make
> >>> open/close NOPs and do all cleanup/setup in probe/remove.
> >>
> >> so the virtbt_{open,close} as NOP is not really what a driver is suppose
> >> to be doing. These are transport enable/disable callbacks from the BT
> >> Core towards the driver. It maps to a device being enabled/disabled by
> >> something like bluetoothd for example. So if disabled, I expect that no
> >> resources/queues are in use.
> >>
> >> Maybe I misunderstand the virtio spec in that regard, but I would like
> >> to keep this fundamental concept of a Bluetooth driver. It does work
> >> with all other transports like USB, SDIO, UART etc.
> >>
> >>> The cost here is a single skb wasted on an unused bt device - which
> >>> seems modest.
> >>
> >> There should be no buffer used if the device is powered off. We also don’t
> >> have any USB URBs in-flight if the transport is not active.
> >>
> >>> NB: with this fix in place driver still suffers from a race condition if
> >>> an interrupt triggers while device is being reset. Work on a fix for
> >>> that issue is in progress.
> >>
> >> In the virtbt_close() callback we should deactivate all interrupts.
> >>
> >
> > If you want to do that then device has to be reset on close,
> > and fully reinitialized on open.
> > Can you work on a patch like that?
> > Given I don't have the device such a rework is probably more
> > than I can undertake.
>
> so you mean move virtio_find_vqs() into virtbt_open() and del_vqs() into
> virtbt_close()?
And reset before del_vqs.
> Or is there are way to set up the queues without starting them?
>
> However I am failing to understand your initial concern, we do reset()
> before del_vqs() in virtbt_remove(). Should we be doing something different
> in virtbt_close() other than virtqueue_detach_unused_buf(). Why would I
> keep buffers attached if they are not used.
>
> Regards
>
> Marcel
They are not used at that point but until device is reset can use them.
Also, if you then proceed to open without a reset, and kick,
device will start by processing the original buffers, crashing
or corrupting memory.
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio/vsock: fix the transport to work with VMADDR_CID_ANY
Hi Wei,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on mst-vhost/linux-next]
[also build test WARNING on net-next/master net/master linus/master v5.16-rc2
next-20211125]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Wei-Wang/virtio-vsock-fix-the-transport-to-work-with-VMADDR_CID_ANY/20211125-163238
base: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next
config: riscv-allyesconfig
(https://download.01.org/0day-ci/archive/20211126/202111260614.iagwvzym-...@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 11.2.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
#
https://github.com/0day-ci/linux/commit/007dbd2e6e604bf8b17a4cec1357113a26983838
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Wei-Wang/virtio-vsock-fix-the-transport-to-work-with-VMADDR_CID_ANY/20211125-163238
git checkout 007dbd2e6e604bf8b17a4cec1357113a26983838
# save the config file to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross
ARCH=riscv
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All warnings (new ones prefixed by >>):
net/vmw_vsock/virtio_transport_common.c: In function
'virtio_transport_recv_pkt':
>> net/vmw_vsock/virtio_transport_common.c:1246:28: warning: variable 'vsk' set
>> but not used [-Wunused-but-set-variable]
1246 | struct vsock_sock *vsk;
|^~~
vim +/vsk +1246 net/vmw_vsock/virtio_transport_common.c
e4b1ef152f53d5e Arseny Krasnov 2021-06-11 1238
06a8fc78367d070 Asias He 2016-07-28 1239 /* We are under the
virtio-vsock's vsock->rx_lock or vhost-vsock's vq->mutex
06a8fc78367d070 Asias He 2016-07-28 1240 * lock.
06a8fc78367d070 Asias He 2016-07-28 1241 */
4c7246dc45e2706 Stefano Garzarella 2019-11-14 1242 void
virtio_transport_recv_pkt(struct virtio_transport *t,
4c7246dc45e2706 Stefano Garzarella 2019-11-14 1243
struct virtio_vsock_pkt *pkt)
06a8fc78367d070 Asias He 2016-07-28 1244 {
06a8fc78367d070 Asias He 2016-07-28 1245 struct sockaddr_vm src,
dst;
06a8fc78367d070 Asias He 2016-07-28 @1246 struct vsock_sock *vsk;
06a8fc78367d070 Asias He 2016-07-28 1247 struct sock *sk;
06a8fc78367d070 Asias He 2016-07-28 1248 bool space_available;
06a8fc78367d070 Asias He 2016-07-28 1249
f83f12d660d1171 Michael S. Tsirkin 2016-12-06 1250 vsock_addr_init(&src,
le64_to_cpu(pkt->hdr.src_cid),
06a8fc78367d070 Asias He 2016-07-28 1251
le32_to_cpu(pkt->hdr.src_port));
f83f12d660d1171 Michael S. Tsirkin 2016-12-06 1252 vsock_addr_init(&dst,
le64_to_cpu(pkt->hdr.dst_cid),
06a8fc78367d070 Asias He 2016-07-28 1253
le32_to_cpu(pkt->hdr.dst_port));
06a8fc78367d070 Asias He 2016-07-28 1254
06a8fc78367d070 Asias He 2016-07-28 1255
trace_virtio_transport_recv_pkt(src.svm_cid, src.svm_port,
06a8fc78367d070 Asias He 2016-07-28 1256
dst.svm_cid, dst.svm_port,
06a8fc78367d070 Asias He 2016-07-28 1257
le32_to_cpu(pkt->hdr.len),
06a8fc78367d070 Asias He 2016-07-28 1258
le16_to_cpu(pkt->hdr.type),
06a8fc78367d070 Asias He 2016-07-28 1259
le16_to_cpu(pkt->hdr.op),
06a8fc78367d070 Asias He 2016-07-28 1260
le32_to_cpu(pkt->hdr.flags),
06a8fc78367d070 Asias He 2016-07-28 1261
le32_to_cpu(pkt->hdr.buf_alloc),
06a8fc78367d070 Asias He 2016-07-28 1262
le32_to_cpu(pkt->hdr.fwd_cnt));
06a8fc78367d070 Asias He 2016-07-28 1263
e4b1ef152f53d5e Arseny Krasnov 2021-06-11 1264 if
(!virtio_transport_valid_type(le16_to_cpu(pkt->hdr.type))) {
4c7246dc45e2706 Stefano Garzarella 2019-11-14 1265
(void)virtio_transport_reset_no_sock(t, pkt);
06a8fc78367d070 Asias He 2016-07-28 1266 goto free_pkt;
06a8fc78367d070 Asias He 2016-07-28 1267 }
06a8fc78367d070 Asias He 2016-07-28 1268
06a8fc78367d070 Asias He 2016-07-28 1269 /* The socket must be
in connected or bound table
06
Re: [PATCH] Bluetooth: virtio_bt: fix device removal
On Thu, Nov 25, 2021 at 10:58:56PM +0100, Marcel Holtmann wrote:
> Hi Michael,
>
> > Device removal is clearly out of virtio spec: it attempts to remove
> > unused buffers from a VQ before invoking device reset. To fix, make
> > open/close NOPs and do all cleanup/setup in probe/remove.
>
> so the virtbt_{open,close} as NOP is not really what a driver is suppose
> to be doing. These are transport enable/disable callbacks from the BT
> Core towards the driver. It maps to a device being enabled/disabled by
> something like bluetoothd for example. So if disabled, I expect that no
> resources/queues are in use.
>
> Maybe I misunderstand the virtio spec in that regard, but I would like
> to keep this fundamental concept of a Bluetooth driver. It does work
> with all other transports like USB, SDIO, UART etc.
>
> > The cost here is a single skb wasted on an unused bt device - which
> > seems modest.
>
> There should be no buffer used if the device is powered off. We also
> don’t
> have any USB URBs in-flight if the transport is not active.
>
> > NB: with this fix in place driver still suffers from a race condition if
> > an interrupt triggers while device is being reset. Work on a fix for
> > that issue is in progress.
>
> In the virtbt_close() callback we should deactivate all interrupts.
>
> >>>
> >>> If you want to do that then device has to be reset on close,
> >>> and fully reinitialized on open.
> >>> Can you work on a patch like that?
> >>> Given I don't have the device such a rework is probably more
> >>> than I can undertake.
> >>
> >> so you mean move virtio_find_vqs() into virtbt_open() and del_vqs() into
> >> virtbt_close()?
> >
> > And reset before del_vqs.
> >
> >> Or is there are way to set up the queues without starting them?
> >>
> >> However I am failing to understand your initial concern, we do reset()
> >> before del_vqs() in virtbt_remove(). Should we be doing something different
> >> in virtbt_close() other than virtqueue_detach_unused_buf(). Why would I
> >> keep buffers attached if they are not used.
> >>
> >
> > They are not used at that point but until device is reset can use them.
> > Also, if you then proceed to open without a reset, and kick,
> > device will start by processing the original buffers, crashing
> > or corrupting memory.
>
> so the only valid usage is like this:
>
> vdev->config->reset(vdev);
>
> while ((.. = virtqueue_detach_unused_buf(vq))) {
> }
>
> vdev->config->del_vqs(vdev);
>
> If I make virtbt_{open,close} a NOP, then I keep adding an extra SKB to inbuf
> on
> every power cycle (ifup/ifdown).
So make sure you don't :)
> How does netdev handle this?
>
> Regards
>
> Marcel
For net, open adds buffers to vq. close does not free them up -
they stay in the vq until device is removed.
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [PATCH] virtio/vsock: fix the transport to work with VMADDR_CID_ANY
On Thursday, November 25, 2021 6:41 PM, Stefano Garzarella wrote:
> On Thu, Nov 25, 2021 at 09:27:40AM +, Wang, Wei W wrote:
> >On Thursday, November 25, 2021 3:16 PM, Wang, Wei W wrote:
> >> - /* Update CID in case it has changed after a transport reset event */
> >> - vsk->local_addr.svm_cid = dst.svm_cid;
> >> -
> >>if (space_available)
> >>sk->sk_write_space(sk);
> >>
> >
> >Not sure if anybody knows how this affects the transport reset.
>
> I believe the primary use case is when a guest is migrated.
>
> After the migration, the transport gets a reset event from the hypervisor and
> all connected sockets are closed. The ones in listen remain open though.
>
> Also the guest's CID may have changed after migration. So if an application
> has
> open listening sockets, bound to the old CID, this should ensure that the
> socket
> continues to be usable.
OK, thanks for sharing the background.
>
> The patch would then change this behavior.
>
> So maybe to avoid problems, we could update the CID only if it is different
> from VMADDR_CID_ANY:
>
> if (vsk->local_addr.svm_cid != VMADDR_CID_ANY)
> vsk->local_addr.svm_cid = dst.svm_cid;
>
>
> When this code was written, a guest only supported a single transport, so it
> could only have one CID assigned, so that wasn't a problem.
> For that reason I'll add this Fixes tag:
> Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
Sounds good to me.
Thanks,
Wei
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH AUTOSEL 5.15 7/7] virtio-mem: support VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE
From: David Hildenbrand
[ Upstream commit 61082ad6a6e1f999eef7e7e90046486c87933b1e ]
The initial virtio-mem spec states that while unplugged memory should not
be read, the device still has to allow for reading unplugged memory inside
the usable region. The primary motivation for this default handling was
to simplify bringup of virtio-mem, because there were corner cases where
Linux might have accidentially read unplugged memory inside added Linux
memory blocks.
In the meantime, we:
1. Removed /dev/kmem in commit bbcd53c96071 ("drivers/char: remove
/dev/kmem for good")
2. Disallowed access to virtio-mem device memory via /dev/mem in
commit 2128f4e21aa2 ("virtio-mem: disallow mapping virtio-mem memory via
/dev/mem")
3. Sanitized access to virtio-mem device memory via /proc/kcore in
commit 0daa322b8ff9 ("fs/proc/kcore: don't read offline sections,
logically offline pages and hwpoisoned pages")
4. Sanitized access to virtio-mem device memory via /proc/vmcore in
commit ce2814622e84 ("virtio-mem: kdump mode to sanitize /proc/vmcore
access")
"Accidential" access to unplugged memory is no longer possible; we can
support the new VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE feature that will be
required by some hypervisors implementing virtio-mem in the near future.
Acked-by: Michael S. Tsirkin
Cc: "Michael S. Tsirkin"
Cc: Jason Wang
Cc: Marek Kedzierski
Cc: Hui Zhu
Cc: Sebastien Boeuf
Cc: Pankaj Gupta
Cc: Wei Yang
Signed-off-by: David Hildenbrand
Signed-off-by: Sasha Levin
---
drivers/virtio/virtio_mem.c | 1 +
include/uapi/linux/virtio_mem.h | 9 ++---
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
index bef8ad6bf4661..78dfdc9c98a1c 100644
--- a/drivers/virtio/virtio_mem.c
+++ b/drivers/virtio/virtio_mem.c
@@ -2758,6 +2758,7 @@ static unsigned int virtio_mem_features[] = {
#if defined(CONFIG_NUMA) && defined(CONFIG_ACPI_NUMA)
VIRTIO_MEM_F_ACPI_PXM,
#endif
+ VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE,
};
static const struct virtio_device_id virtio_mem_id_table[] = {
diff --git a/include/uapi/linux/virtio_mem.h b/include/uapi/linux/virtio_mem.h
index 70e01c687d5eb..e9122f1d0e0cb 100644
--- a/include/uapi/linux/virtio_mem.h
+++ b/include/uapi/linux/virtio_mem.h
@@ -68,9 +68,10 @@
* explicitly triggered (VIRTIO_MEM_REQ_UNPLUG).
*
* There are no guarantees what will happen if unplugged memory is
- * read/written. Such memory should, in general, not be touched. E.g.,
- * even writing might succeed, but the values will simply be discarded at
- * random points in time.
+ * read/written. In general, unplugged memory should not be touched, because
+ * the resulting action is undefined. There is one exception: without
+ * VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE, unplugged memory inside the usable
+ * region can be read, to simplify creation of memory dumps.
*
* It can happen that the device cannot process a request, because it is
* busy. The device driver has to retry later.
@@ -87,6 +88,8 @@
/* node_id is an ACPI PXM and is valid */
#define VIRTIO_MEM_F_ACPI_PXM 0
+/* unplugged memory must not be accessed */
+#define VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE1
/* --- virtio-mem: guest -> host requests --- */
--
2.33.0
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH AUTOSEL 5.10 3/4] virtio-mem: support VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE
From: David Hildenbrand
[ Upstream commit 61082ad6a6e1f999eef7e7e90046486c87933b1e ]
The initial virtio-mem spec states that while unplugged memory should not
be read, the device still has to allow for reading unplugged memory inside
the usable region. The primary motivation for this default handling was
to simplify bringup of virtio-mem, because there were corner cases where
Linux might have accidentially read unplugged memory inside added Linux
memory blocks.
In the meantime, we:
1. Removed /dev/kmem in commit bbcd53c96071 ("drivers/char: remove
/dev/kmem for good")
2. Disallowed access to virtio-mem device memory via /dev/mem in
commit 2128f4e21aa2 ("virtio-mem: disallow mapping virtio-mem memory via
/dev/mem")
3. Sanitized access to virtio-mem device memory via /proc/kcore in
commit 0daa322b8ff9 ("fs/proc/kcore: don't read offline sections,
logically offline pages and hwpoisoned pages")
4. Sanitized access to virtio-mem device memory via /proc/vmcore in
commit ce2814622e84 ("virtio-mem: kdump mode to sanitize /proc/vmcore
access")
"Accidential" access to unplugged memory is no longer possible; we can
support the new VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE feature that will be
required by some hypervisors implementing virtio-mem in the near future.
Acked-by: Michael S. Tsirkin
Cc: "Michael S. Tsirkin"
Cc: Jason Wang
Cc: Marek Kedzierski
Cc: Hui Zhu
Cc: Sebastien Boeuf
Cc: Pankaj Gupta
Cc: Wei Yang
Signed-off-by: David Hildenbrand
Signed-off-by: Sasha Levin
---
drivers/virtio/virtio_mem.c | 1 +
include/uapi/linux/virtio_mem.h | 9 ++---
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
index 181e2f18beae5..1afdd8ca00bbb 100644
--- a/drivers/virtio/virtio_mem.c
+++ b/drivers/virtio/virtio_mem.c
@@ -1925,6 +1925,7 @@ static unsigned int virtio_mem_features[] = {
#if defined(CONFIG_NUMA) && defined(CONFIG_ACPI_NUMA)
VIRTIO_MEM_F_ACPI_PXM,
#endif
+ VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE,
};
static const struct virtio_device_id virtio_mem_id_table[] = {
diff --git a/include/uapi/linux/virtio_mem.h b/include/uapi/linux/virtio_mem.h
index 70e01c687d5eb..e9122f1d0e0cb 100644
--- a/include/uapi/linux/virtio_mem.h
+++ b/include/uapi/linux/virtio_mem.h
@@ -68,9 +68,10 @@
* explicitly triggered (VIRTIO_MEM_REQ_UNPLUG).
*
* There are no guarantees what will happen if unplugged memory is
- * read/written. Such memory should, in general, not be touched. E.g.,
- * even writing might succeed, but the values will simply be discarded at
- * random points in time.
+ * read/written. In general, unplugged memory should not be touched, because
+ * the resulting action is undefined. There is one exception: without
+ * VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE, unplugged memory inside the usable
+ * region can be read, to simplify creation of memory dumps.
*
* It can happen that the device cannot process a request, because it is
* busy. The device driver has to retry later.
@@ -87,6 +88,8 @@
/* node_id is an ACPI PXM and is valid */
#define VIRTIO_MEM_F_ACPI_PXM 0
+/* unplugged memory must not be accessed */
+#define VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE1
/* --- virtio-mem: guest -> host requests --- */
--
2.33.0
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2] virtio/vsock: fix the transport to work with VMADDR_CID_ANY
The VMADDR_CID_ANY flag used by a socket means that the socket isn't bound
to any specific CID. For example, a host vsock server may want to be bound
with VMADDR_CID_ANY, so that a guest vsock client can connect to the host
server with CID=VMADDR_CID_HOST (i.e. 2), and meanwhile, a host vsock
client can connect to the same local server with CID=VMADDR_CID_LOCAL
(i.e. 1).
The current implementation sets the destination socket's svm_cid to a
fixed CID value after the first client's connection, which isn't an
expected operation. For example, if the guest client first connects to the
host server, the server's svm_cid gets set to VMADDR_CID_HOST, then other
host clients won't be able to connect to the server anymore.
Reproduce steps:
1. Run the host server:
socat VSOCK-LISTEN:1234,fork -
2. Run a guest client to connect to the host server:
socat - VSOCK-CONNECT:2:1234
3. Run a host client to connect to the host server:
socat - VSOCK-CONNECT:1:1234
Without this patch, step 3. above fails to connect, and socat complains
"socat[1720] E connect(5, AF=40 cid:1 port:1234, 16): Connection
reset by peer".
With this patch, the above works well.
Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
Signed-off-by: Wei Wang
---
net/vmw_vsock/virtio_transport_common.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/vmw_vsock/virtio_transport_common.c
b/net/vmw_vsock/virtio_transport_common.c
index 59ee1be5a6dd..ec2c2afbf0d0 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -1299,7 +1299,8 @@ void virtio_transport_recv_pkt(struct virtio_transport *t,
space_available = virtio_transport_space_update(sk, pkt);
/* Update CID in case it has changed after a transport reset event */
- vsk->local_addr.svm_cid = dst.svm_cid;
+ if (vsk->local_addr.svm_cid != VMADDR_CID_ANY)
+ vsk->local_addr.svm_cid = dst.svm_cid;
if (space_available)
sk->sk_write_space(sk);
base-commit: 5f53fa508db098c9d372423a6dac31c8a5679cdf
--
2.25.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vdpa: Consider device id larger than 31
On Fri, Nov 26, 2021 at 2:09 AM Parav Pandit wrote:
>
> virtio device id value can be more than 31. Hence, use BIT_ULL in
> assignment.
>
> Fixes: 33b347503f01 ("vdpa: Define vdpa mgmt device, ops and a netlink
> interface")
> Reported-by: kernel test robot
> Reported-by: Dan Carpenter
> Signed-off-by: Parav Pandit
Acked-by: Jason Wang
> ---
> drivers/vdpa/vdpa.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 7332a74a4b00..e91c71aeeddf 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -404,7 +404,7 @@ static int vdpa_mgmtdev_fill(const struct vdpa_mgmt_dev
> *mdev, struct sk_buff *m
> goto msg_err;
>
> while (mdev->id_table[i].device) {
> - supported_classes |= BIT(mdev->id_table[i].device);
> + supported_classes |= BIT_ULL(mdev->id_table[i].device);
> i++;
> }
>
> --
> 2.26.2
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATH v1 2/2] vdpa/mlx5: Add support for reading descriptor statistics
On Thu, Nov 25, 2021 at 3:59 PM Eli Cohen wrote:
>
> On Thu, Nov 25, 2021 at 12:57:53PM +0800, Jason Wang wrote:
> > On Thu, Nov 25, 2021 at 12:56 AM Eli Cohen wrote:
> > >
> > > Implement the get_vq_stats calback of vdpa_config_ops to return the
> > > statistics for a virtqueue.
> > >
> > > Signed-off-by: Eli Cohen
> > > ---
> > > V0 -> V1:
> > > Use mutex to sync stats query with change of number of queues
> > >
> >
> > [...]
> >
> > > +static int mlx5_get_vq_stats(struct vdpa_device *vdev, u16 *idx,
> > > +struct vdpa_vq_stats *stats)
> > > +{
> > > + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > + struct mlx5_vdpa_virtqueue *mvq = &ndev->vqs[*idx];
> > > + struct mlx5_control_vq *cvq;
> > > + int err;
> > > +
> > > + mutex_lock(&ndev->numq_lock);
> > > + if ((!ctrl_vq_active(mvdev) && *idx >= ndev->cur_num_vqs) ||
> > > + (*idx != ctrl_vq_idx(mvdev) && *idx >= ndev->cur_num_vqs)) {
> > > + err = -EINVAL;
> > > + goto out;
> >
> > Interesting, I wonder if it's simpler that we just allow stats up to
> > the max vqs. It's sometimes useful to dump the stats of all the vqs so
> > we can let that policy to userspace. Then we can get rid of the mutex.
> >
> If a VQ is not active then I don't have stats for it. The hardware
> object is not available to be queried.
I wonder if it's ok that we just return 0 in this case?
Btw, the cvq counters:
+
+ if (*idx == ctrl_vq_idx(mvdev)) {
+ cvq = &mvdev->cvq;
+ stats->received_desc = cvq->head;
+ stats->completed_desc = cvq->head;
+ stats->ctrl_vq = true;
+ *idx = VDPA_INVAL_QUEUE_INDEX;
+ err = 0;
+ goto out;
+ }
Seems not to consider the case that the head can wrap around.
Thanks
>
> > Thanks
> >
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-mmio: harden interrupt
On Thu, Nov 25, 2021 at 8:08 PM kernel test robot wrote:
>
> Hi Jason,
>
> I love your patch! Perhaps something to improve:
>
> [auto build test WARNING on linus/master]
> [also build test WARNING on v5.16-rc2 next-20211125]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
Will fix this in V2.
Thanks
>
> url:
> https://github.com/0day-ci/linux/commits/Jason-Wang/virtio-mmio-harden-interrupt/20211125-143334
> base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> 5f53fa508db098c9d372423a6dac31c8a5679cdf
> config: mips-buildonly-randconfig-r003-20211125
> (https://download.01.org/0day-ci/archive/20211125/202111252001.z5tli1np-...@intel.com/config)
> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project
> 67a1c45def8a75061203461ab0060c75c864df1c)
> 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
> # install mips cross compiling tool for clang build
> # apt-get install binutils-mips-linux-gnu
> #
> https://github.com/0day-ci/linux/commit/e19a8a1a95bd891090863b2d6828b8dc55d3633f
> git remote add linux-review https://github.com/0day-ci/linux
> git fetch --no-tags linux-review
> Jason-Wang/virtio-mmio-harden-interrupt/20211125-143334
> git checkout e19a8a1a95bd891090863b2d6828b8dc55d3633f
> # save the config file to linux build tree
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1
> ARCH=mips
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot
>
> All warnings (new ones prefixed by >>):
>
> >> drivers/virtio/virtio_mmio.c:105:6: warning: no previous prototype for
> >> function 'vm_disable_cbs' [-Wmissing-prototypes]
>void vm_disable_cbs(struct virtio_device *vdev)
> ^
>drivers/virtio/virtio_mmio.c:105:1: note: declare 'static' if the function
> is not intended to be used outside of this translation unit
>void vm_disable_cbs(struct virtio_device *vdev)
>^
>static
> >> drivers/virtio/virtio_mmio.c:121:6: warning: no previous prototype for
> >> function 'vm_enable_cbs' [-Wmissing-prototypes]
>void vm_enable_cbs(struct virtio_device *vdev)
> ^
>drivers/virtio/virtio_mmio.c:121:1: note: declare 'static' if the function
> is not intended to be used outside of this translation unit
>void vm_enable_cbs(struct virtio_device *vdev)
>^
>static
>2 warnings generated.
>
>
> vim +/vm_disable_cbs +105 drivers/virtio/virtio_mmio.c
>
>103
>104 /* disable irq handlers */
> > 105 void vm_disable_cbs(struct virtio_device *vdev)
>106 {
>107 struct virtio_mmio_device *vm_dev =
> to_virtio_mmio_device(vdev);
>108 int irq = platform_get_irq(vm_dev->pdev, 0);
>109
>110 /*
>111 * The below synchronize() guarantees that any
>112 * interrupt for this line arriving after
>113 * synchronize_irq() has completed is guaranteed to see
>114 * intx_soft_enabled == false.
>115 */
>116 WRITE_ONCE(vm_dev->intr_soft_enabled, false);
>117 synchronize_irq(irq);
>118 }
>119
>120 /* enable irq handlers */
> > 121 void vm_enable_cbs(struct virtio_device *vdev)
>122 {
>123 struct virtio_mmio_device *vm_dev =
> to_virtio_mmio_device(vdev);
>124 int irq = platform_get_irq(vm_dev->pdev, 0);
>125
>126 disable_irq(irq);
>127 /*
>128 * The above disable_irq() provides TSO ordering and
>129 * as such promotes the below store to store-release.
>130 */
>131 WRITE_ONCE(vm_dev->intr_soft_enabled, true);
>132 enable_irq(irq);
>133 return;
>134 }
>135
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH V2] virtio-mmio: harden interrupt
This patch tries to make sure the virtio interrupt handler for MMIO
won't be called after a reset and before virtio_device_ready(). We
can't use IRQF_NO_AUTOEN since we're using shared interrupt
(IRQF_SHARED). So this patch tracks the interrupt enabling status in a
new intr_soft_enabled variable and toggle it during in
vm_disable/enable_interrupts(). The MMIO interrupt handler will check
intr_soft_enabled before processing the actual interrupt.
Signed-off-by: Jason Wang
---
Changes since V1:
- Silent compling warnings
drivers/virtio/virtio_mmio.c | 37
1 file changed, 37 insertions(+)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 56128b9c46eb..c517afdd2cc5 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -90,6 +90,7 @@ struct virtio_mmio_device {
/* a list of queues so we can dispatch IRQs */
spinlock_t lock;
struct list_head virtqueues;
+ bool intr_soft_enabled;
};
struct virtio_mmio_vq_info {
@@ -100,7 +101,37 @@ struct virtio_mmio_vq_info {
struct list_head node;
};
+/* disable irq handlers */
+static void vm_disable_cbs(struct virtio_device *vdev)
+{
+ struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
+ int irq = platform_get_irq(vm_dev->pdev, 0);
+ /*
+* The below synchronize() guarantees that any
+* interrupt for this line arriving after
+* synchronize_irq() has completed is guaranteed to see
+* intx_soft_enabled == false.
+*/
+ WRITE_ONCE(vm_dev->intr_soft_enabled, false);
+ synchronize_irq(irq);
+}
+
+/* enable irq handlers */
+static void vm_enable_cbs(struct virtio_device *vdev)
+{
+ struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
+ int irq = platform_get_irq(vm_dev->pdev, 0);
+
+ disable_irq(irq);
+ /*
+* The above disable_irq() provides TSO ordering and
+* as such promotes the below store to store-release.
+*/
+ WRITE_ONCE(vm_dev->intr_soft_enabled, true);
+ enable_irq(irq);
+ return;
+}
/* Configuration interface */
@@ -262,6 +293,8 @@ static void vm_reset(struct virtio_device *vdev)
/* 0 status means a reset. */
writel(0, vm_dev->base + VIRTIO_MMIO_STATUS);
+ /* Disable VQ/configuration callbacks. */
+ vm_disable_cbs(vdev);
}
@@ -288,6 +321,9 @@ static irqreturn_t vm_interrupt(int irq, void *opaque)
unsigned long flags;
irqreturn_t ret = IRQ_NONE;
+ if (!READ_ONCE(vm_dev->intr_soft_enabled))
+ return IRQ_NONE;
+
/* Read and acknowledge interrupts */
status = readl(vm_dev->base + VIRTIO_MMIO_INTERRUPT_STATUS);
writel(status, vm_dev->base + VIRTIO_MMIO_INTERRUPT_ACK);
@@ -529,6 +565,7 @@ static bool vm_get_shm_region(struct virtio_device *vdev,
}
static const struct virtio_config_ops virtio_mmio_config_ops = {
+ .enable_cbs = vm_enable_cbs,
.get= vm_get,
.set= vm_set,
.generation = vm_generation,
--
2.25.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
