Re: [PATCH v1 1/2] vdpa: Add support for querying vendor statistics
On 3/17/2022 7:27 PM, Jason Wang wrote: On Fri, Mar 18, 2022 at 8:59 AM Si-Wei Liu wrote: On 3/16/2022 7:32 PM, Jason Wang wrote: On Thu, Mar 17, 2022 at 6:00 AM Si-Wei Liu wrote: On 3/16/2022 12:10 AM, Eli Cohen wrote: From: Si-Wei Liu Sent: Wednesday, March 16, 2022 8:52 AM To: Eli Cohen Cc: m...@redhat.com; jasow...@redhat.com; virtualization@lists.linux-foundation.org; epere...@redhat.com; amore...@redhat.com; lviv...@redhat.com; sgarz...@redhat.com; Parav Pandit Subject: Re: [PATCH v1 1/2] vdpa: Add support for querying vendor statistics On 3/15/2022 2:10 AM, Eli Cohen wrote: <...snip...> Say you got a vdpa net device created with 4 data queue pairs and a control vq. On boot some guest firmware may support just F_CTRL_VQ but not F_MQ, then the index for the control vq in guest ends up with 2, as in this case there's only a single queue pair enabled for rx (index 0) and tx (index 1). From the host driver (e.g. mlx5_vdpa) perspective, the control vq is the last vq following 8 If the host sees F_MQ was not negotiated but F_CTRL_VQ was, then it knows that control VQ index is 2 Right, but I don't see this feature negotiation info getting returned from your vdpa_dev_vendor_stats_fill(), or did I miss something? How do you plan for host user to get this info? If you meant another "vdpa dev show" command to query negotiated features ahead, this won't get the same lock protected as the time you run the stat query. It's very easy to miss that ephemeral queue index. Right, so I suggested to include the negotiated features in the netlink message for the statistics. That would save us from using two system calls to get the information required and it answers your concern with respect to locking. I think Jason was reluctant to adding this attribute to the message but can't find where he explained the reasoning. Maybe Jason can clarify and correct me, but I just did not get the same impression as what you said? I just skimmed through all of the emails in the thread, only finding that he didn't want device specific attribute such as queue type to get returned by the vdpa core, which I agree. I'm not sure if he's explicitly against piggyback negotiated features to aid userspace parsing the index. I think we need piggyback the negotiated features, otherwise as you mentioned, we will probably get in-consistency. Great. Thanks for confirming it. But a question for the "host queue index", as mentioned before. It's something that is not defined in the spec, so technically, vendor can do any mappings between it and the index what guest can see. I feel like we need to clarify it in the spec first. I have been thinking about this for some while today. Actually I am not against exposing the host queue index to the spec, as we know it's somewhat implicitly defined in the QEMU device model for multiqueue. The thing is, I'm not sure if there's extra benefit than this minor requirement (*) given that all of the other vDPA kAPI are taking the guest queue index rather than the host queue index. Rethink of this, consider currently we do this via vendor stats, so it's probably fine. Maybe we can have a better netlink API like "vendor_queue_index" etc then everything should be fine. True. Or if there's netlink API that simply dumps the stats for all of the available queues in one shot, that would serve our cloud use case quite well. :) It works for mlx5_vdpa as the control vq is implemented in the software, so it can map to whatever guest qindex it wishes to. But would it cause extra trouble for some other emulated vDPA device or other vendor's vDPA such as ifcvf to fabricate a fake mapping between the host queue index and the one guest can see? I would have to send a heads-up ahead that the current vhost-vdpa mq implementation in upstream QEMU has some issue in mapping the host qindex to the guest one. This would become a problem with MQ enabled vdpa device and a non-MQ supporting guest e.g. OVMF, for which I'm about to share some RFC patches shortly to demonstrate the issue. Sure. Please see the RFC patch just sent with the subject "vhost_net: should not use max_queue_pairs for non-mq guest", option #3 is to leverage host queue index. If exposing the host queue index to the spec turns is essential to resolving this issue and maybe help with software virtio QEMU implementation too, I won't hesitate to expose this important implementation detail to the spec. (*) another means that may somehow address my use case is to use some magic keyword e.g. "ctrlvq" to identify the control vq. Implementation wise, we can extensively pass -1 to indicate the last guest qindex to the get_vq_vstat() API given that we know for sure the ctrlvq is the last queue in the array when the relevant features are present. Since the negotiated features are piggybacked, it's not hard for the vdpa tool to tell apart whether the last queue is a control vq or not. For virtqueue index (guest index) defined in the spe
Re: [PATCH] vdpa: Update man page with added support to configure max vq pair
On 3/15/2022 6:13 AM, Eli Cohen wrote: Update man page to include information how to configure the max virtqueue pairs for a vdpa device when creating one. Signed-off-by: Eli Cohen --- man/man8/vdpa-dev.8 | 6 ++ 1 file changed, 6 insertions(+) diff --git a/man/man8/vdpa-dev.8 b/man/man8/vdpa-dev.8 index aa21ae3acbd8..432867c65182 100644 --- a/man/man8/vdpa-dev.8 +++ b/man/man8/vdpa-dev.8 @@ -33,6 +33,7 @@ vdpa-dev \- vdpa device configuration .I MGMTDEV .RI "[ mac " MACADDR " ]" .RI "[ mtu " MTU " ]" +.RI "[ max_vqp " MAX_VQ_PAIRS " ]" Here it introduces the max_vqp option to the SYNOPSIS. I would be nice to describe what it means and which device type is applicable in the below section: .PP .BI mac " MACADDR" - specifies the mac address for the new vdpa device. This is applicable only for the network type of vdpa device. This is optional. .BI mtu " MTU" - specifies the mtu for the new vdpa device. This is applicable only for the network type of vdpa device. This is optional. Otherwise looks good to me. Reviewed-by: Si-Wei Liu Thanks, -Siwei .ti -8 .B vdpa dev del @@ -119,6 +120,11 @@ vdpa dev add name foo mgmtdev vdpa_sim_net mac 00:11:22:33:44:55 mtu 9000 Add the vdpa device named foo on the management device vdpa_sim_net with mac address of 00:11:22:33:44:55 and mtu of 9000 bytes. .RE .PP +vdpa dev add name foo mgmtdev auxiliary/mlx5_core.sf.1 mac 00:11:22:33:44:55 max_vqp 8 +.RS 4 +Add the vdpa device named foo on the management device auxiliary/mlx5_core.sf.1 with mac address of 00:11:22:33:44:55 and max 8 virtqueue pairs +.RE +.PP vdpa dev del foo .RS 4 Delete the vdpa device named foo which was previously created. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vdpa: Update man page with added support to configure max vq pair
On 3/15/2022 6:13 AM, Eli Cohen wrote: Update man page to include information how to configure the max virtqueue pairs for a vdpa device when creating one. Signed-off-by: Eli Cohen --- man/man8/vdpa-dev.8 | 6 ++ 1 file changed, 6 insertions(+) diff --git a/man/man8/vdpa-dev.8 b/man/man8/vdpa-dev.8 index aa21ae3acbd8..432867c65182 100644 --- a/man/man8/vdpa-dev.8 +++ b/man/man8/vdpa-dev.8 @@ -33,6 +33,7 @@ vdpa-dev \- vdpa device configuration .I MGMTDEV .RI "[ mac " MACADDR " ]" .RI "[ mtu " MTU " ]" +.RI "[ max_vqp " MAX_VQ_PAIRS " ]" Here it introduces the max_vqp option to the SYNOPSIS. I would be nice to describe what it means and which device type is applicable in the below section: .PP .BI mac " MACADDR" - specifies the mac address for the new vdpa device. This is applicable only for the network type of vdpa device. This is optional. .BI mtu " MTU" - specifies the mtu for the new vdpa device. This is applicable only for the network type of vdpa device. This is optional. Otherwise looks good to me. Reviewed-by: Si-Wei Liu Thanks, -Siwei .ti -8 .B vdpa dev del @@ -119,6 +120,11 @@ vdpa dev add name foo mgmtdev vdpa_sim_net mac 00:11:22:33:44:55 mtu 9000 Add the vdpa device named foo on the management device vdpa_sim_net with mac address of 00:11:22:33:44:55 and mtu of 9000 bytes. .RE .PP +vdpa dev add name foo mgmtdev auxiliary/mlx5_core.sf.1 mac 00:11:22:33:44:55 max_vqp 8 +.RS 4 +Add the vdpa device named foo on the management device auxiliary/mlx5_core.sf.1 with mac address of 00:11:22:33:44:55 and max 8 virtqueue pairs +.RE +.PP vdpa dev del foo .RS 4 Delete the vdpa device named foo which was previously created. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] dt-bindings: virtio: mmio: add optional virtio,wakeup property
On 18/03/2022 03:10, Minghao Xue wrote: > Hi Jean and folks, > This is just an optional flag which could be used on an embedded system. > For example, if we want to use an virtio-input device as a virtual power > key to wake up the virtual machine, we can set this flag in the device > tree. > Currently, virio-mmio driver does not implement suspend/resume > callback(maybe no need). So we want to check this flag and call > enable_irq_wake() accordingly in vm_find_vqs(). There is a generic wakeup-source property. How is this one different that you need a separate one? Best regards, Krzysztof ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH RESEND] VMCI: Check exclusive_vectors when freeing interrupt 1
On Thu, Mar 17, 2022 at 10:58:43PM -0700, vd...@vmware.com wrote:
> From: Vishnu Dasa
>
> free_irq() may be called to free an interrupt that was not
> allocated. Add missing 'if' statement to check for
> exclusive_vectors when freeing interrupt 1.
>
> Fixes: cc68f2177fcb ("VMCI: dma dg: register dummy IRQ handlers for DMA
> datagrams")
> Reported-by: Dan Carpenter
> Reviewed-by: Bryan Tan
> Reviewed-by: Rajesh Jalisatgi
> Signed-off-by: Vishnu Dasa
> ---
> drivers/misc/vmw_vmci/vmci_guest.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/misc/vmw_vmci/vmci_guest.c
> b/drivers/misc/vmw_vmci/vmci_guest.c
> index 6596a54daa88..57a6157209a1 100644
> --- a/drivers/misc/vmw_vmci/vmci_guest.c
> +++ b/drivers/misc/vmw_vmci/vmci_guest.c
> @@ -862,7 +862,9 @@ static int vmci_guest_probe_device(struct pci_dev *pdev,
> return 0;
>
> err_free_bm_irq:
> - free_irq(pci_irq_vector(pdev, 1), vmci_dev);
> + if (vmci_dev->exclusive_vectors)
> + free_irq(pci_irq_vector(pdev, 1), vmci_dev);
> +
> err_free_irq:
> free_irq(pci_irq_vector(pdev, 0), vmci_dev);
> tasklet_kill(&vmci_dev->datagram_tasklet);
> --
> 2.25.1
>
You sent the previous version 2 days before this, and 5 days before
that. Please relax and don't start worrying unless it's been 2 weeks.
thanks,
greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vhost: handle error while adding split ranges to iotlb
On Sat, Mar 12, 2022 at 07:41:21PM +0530, Anirudh Rayabharam wrote:
vhost_iotlb_add_range_ctx() handles the range [0, ULONG_MAX] by
splitting it into two ranges and adding them separately. The return
value of adding the first range to the iotlb is currently ignored.
Check the return value and bail out in case of an error.
We could add:
Fixes: e2ae38cf3d91 ("vhost: fix hung thread due to erroneous iotlb entries")
Signed-off-by: Anirudh Rayabharam
---
drivers/vhost/iotlb.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/vhost/iotlb.c b/drivers/vhost/iotlb.c
index 40b098320b2a..5829cf2d0552 100644
--- a/drivers/vhost/iotlb.c
+++ b/drivers/vhost/iotlb.c
@@ -62,8 +62,12 @@ int vhost_iotlb_add_range_ctx(struct vhost_iotlb *iotlb,
*/
if (start == 0 && last == ULONG_MAX) {
u64 mid = last / 2;
+ int err = vhost_iotlb_add_range_ctx(iotlb, start, mid, addr,
+ perm, opaque);
+
+ if (err)
+ return err;
- vhost_iotlb_add_range_ctx(iotlb, start, mid, addr, perm,
opaque);
addr += mid + 1;
start = mid + 1;
}
--
2.35.1
Reviewed-by: Stefano Garzarella
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
