Re: [PATCH v3 -next] vdpa: mlx5: change Kconfig depends to fix build errors
On Thu, Sep 24, 2020 at 08:47:05AM -0700, Randy Dunlap wrote: > On 9/24/20 3:24 AM, Eli Cohen wrote: > > On Thu, Sep 24, 2020 at 05:30:55AM -0400, Michael S. Tsirkin wrote: > --- linux-next-20200917.orig/drivers/vdpa/Kconfig > +++ linux-next-20200917/drivers/vdpa/Kconfig > @@ -31,7 +31,7 @@ config IFCVF > > config MLX5_VDPA > bool "MLX5 VDPA support library for ConnectX devices" > -depends on MLX5_CORE > +depends on VHOST_IOTLB && MLX5_CORE > default n > >>> > >>> While we are here, can anyone who apply this patch delete the "default n" > >>> line? > >>> It is by default "n". > > > > I can do that > > > >>> > >>> Thanks > >> > >> Hmm other drivers select VHOST_IOTLB, why not do the same? > > v1 used select, but Saeed requested use of depends instead because > select can cause problems. > > > I can't see another driver doing that. Perhaps I can set dependency on > > VHOST which by itself depends on VHOST_IOTLB? > >> > >> > help > Support library for Mellanox VDPA drivers. Provides code that > is > > >> > Saeed what kind of problems? It's used with select in other places, isn't it? > -- > ~Randy ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 -next] vdpa: mlx5: change Kconfig depends to fix build errors
On 9/24/20 3:24 AM, Eli Cohen wrote: > On Thu, Sep 24, 2020 at 05:30:55AM -0400, Michael S. Tsirkin wrote: --- linux-next-20200917.orig/drivers/vdpa/Kconfig +++ linux-next-20200917/drivers/vdpa/Kconfig @@ -31,7 +31,7 @@ config IFCVF config MLX5_VDPA bool "MLX5 VDPA support library for ConnectX devices" - depends on MLX5_CORE + depends on VHOST_IOTLB && MLX5_CORE default n >>> >>> While we are here, can anyone who apply this patch delete the "default n" >>> line? >>> It is by default "n". > > I can do that > >>> >>> Thanks >> >> Hmm other drivers select VHOST_IOTLB, why not do the same? v1 used select, but Saeed requested use of depends instead because select can cause problems. > I can't see another driver doing that. Perhaps I can set dependency on > VHOST which by itself depends on VHOST_IOTLB? >> >> help Support library for Mellanox VDPA drivers. Provides code that is >> -- ~Randy ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 5/6] iommu/virtio: Support topology description in config space
On Fri, Aug 21, 2020 at 03:15:39PM +0200, Jean-Philippe Brucker wrote:
> Platforms without device-tree nor ACPI can provide a topology
> description embedded into the virtio config space. Parse it.
>
> Use PCI FIXUP to probe the config space early, because we need to
> discover the topology before any DMA configuration takes place, and the
> virtio driver may be loaded much later. Since we discover the topology
> description when probing the PCI hierarchy, the virtual IOMMU cannot
> manage other platform devices discovered earlier.
> +struct viommu_cap_config {
> + u8 bar;
> + u32 length; /* structure size */
> + u32 offset; /* structure offset within the bar */
s/the bar/the BAR/ (to match comment below).
> +static void viommu_pci_parse_topology(struct pci_dev *dev)
> +{
> + int ret;
> + u32 features;
> + void __iomem *regs, *common_regs;
> + struct viommu_cap_config cap = {0};
> + struct virtio_pci_common_cfg __iomem *common_cfg;
> +
> + /*
> + * The virtio infrastructure might not be loaded at this point. We need
> + * to access the BARs ourselves.
> + */
> + ret = viommu_pci_find_capability(dev, VIRTIO_PCI_CAP_COMMON_CFG, );
> + if (!ret) {
> + pci_warn(dev, "common capability not found\n");
Is the lack of this capability really an error, i.e., is this
pci_warn() or pci_info()? The "device doesn't have topology
description" below is only pci_dbg(), which suggests that we can live
without this.
Maybe a hint about what "common capability" means?
> + return;
> + }
> +
> + if (pci_enable_device_mem(dev))
> + return;
> +
> + common_regs = pci_iomap(dev, cap.bar, 0);
> + if (!common_regs)
> + return;
> +
> + common_cfg = common_regs + cap.offset;
> +
> + /* Perform the init sequence before we can read the config */
> + ret = viommu_pci_reset(common_cfg);
I guess this is some special device-specific reset, not any kind of
standard PCI reset?
> + if (ret < 0) {
> + pci_warn(dev, "unable to reset device\n");
> + goto out_unmap_common;
> + }
> +
> + iowrite8(VIRTIO_CONFIG_S_ACKNOWLEDGE, _cfg->device_status);
> + iowrite8(VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER,
> + _cfg->device_status);
> +
> + /* Find out if the device supports topology description */
> + iowrite32(0, _cfg->device_feature_select);
> + features = ioread32(_cfg->device_feature);
> +
> + if (!(features & BIT(VIRTIO_IOMMU_F_TOPOLOGY))) {
> + pci_dbg(dev, "device doesn't have topology description");
> + goto out_reset;
> + }
> +
> + ret = viommu_pci_find_capability(dev, VIRTIO_PCI_CAP_DEVICE_CFG, );
> + if (!ret) {
> + pci_warn(dev, "device config capability not found\n");
> + goto out_reset;
> + }
> +
> + regs = pci_iomap(dev, cap.bar, 0);
> + if (!regs)
> + goto out_reset;
> +
> + pci_info(dev, "parsing virtio-iommu topology\n");
> + ret = viommu_parse_topology(>dev, regs + cap.offset,
> + pci_resource_len(dev, 0) - cap.offset);
> + if (ret)
> + pci_warn(dev, "failed to parse topology: %d\n", ret);
> +
> + pci_iounmap(dev, regs);
> +out_reset:
> + ret = viommu_pci_reset(common_cfg);
> + if (ret)
> + pci_warn(dev, "unable to reset device\n");
> +out_unmap_common:
> + pci_iounmap(dev, common_regs);
> +}
> +
> +/*
> + * Catch a PCI virtio-iommu implementation early to get the topology
> description
> + * before we start probing other endpoints.
> + */
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_REDHAT_QUMRANET, 0x1040 +
> VIRTIO_ID_IOMMU,
> + viommu_pci_parse_topology);
> --
> 2.28.0
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 0/6] Add virtio-iommu built-in topology
On Thu, Sep 24, 2020 at 08:41:21AM -0400, Michael S. Tsirkin wrote: > But this has nothing to do with Linux. There is also no guarantee that > the two committees will decide to use exactly the same format. Once one > of them sets the format in stone, we can add support for that format to > linux. If another one is playing nice and uses the same format, we can > use the same parsers. If it doesn't linux will have to follow suit. Or Linux decides to support only one of the formats, which would then be ACPI. Regards, Joerg ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 0/6] Add virtio-iommu built-in topology
On Thu, Sep 24, 2020 at 12:02:55PM +0200, Joerg Roedel wrote: > On Thu, Sep 24, 2020 at 05:38:13AM -0400, Michael S. Tsirkin wrote: > > On Thu, Sep 24, 2020 at 11:21:29AM +0200, Joerg Roedel wrote: > > > On Thu, Sep 24, 2020 at 05:00:35AM -0400, Michael S. Tsirkin wrote: > > > > OK so this looks good. Can you pls repost with the minor tweak > > > > suggested and all acks included, and I will queue this? > > > > > > My NACK still stands, as long as a few questions are open: > > > > > > 1) The format used here will be the same as in the ACPI table? I > > > think the answer to this questions must be Yes, so this leads > > > to the real question: > > > > I am not sure it's a must. > > It is, having only one parser for the ACPI and MMIO descriptions was one > of the selling points for MMIO in past discussions and I think it makes > sense to keep them in sync. > > > We can always tweak the parser if there are slight differences > > between ACPI and virtio formats. > > There is no guarantee that there only need to be "tweaks" until the > ACPI table format is stablized. > > Regards, > > Joerg But this has nothing to do with Linux. There is also no guarantee that the two committees will decide to use exactly the same format. Once one of them sets the format in stone, we can add support for that format to linux. If another one is playing nice and uses the same format, we can use the same parsers. If it doesn't linux will have to follow suit. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 0/6] Add virtio-iommu built-in topology
On Thu, Sep 24, 2020 at 12:29:53PM +0200, Jean-Philippe Brucker wrote: > It's not possible to use exactly the same code for parsing. The access methods can be separate and don't affect the parsing logic. > The access methods are different (need to deal with port-IO for > built-in description on PCI, for example) and more importantly, the > structure is different as well. The ACPI table needs nodes for > virtio-iommu while the built-in description is contained in the > virtio-iommu itself. So the endpoint nodes point to virtio-iommu node > on ACPI, while they don't need a pointer on the built-in desc. I kept > as much as possible common in structures and implementation, but in > the end we still need about 200 unique lines on each side. Will it hurt the non-ACPI version to have the not-needed pointers anyway to keep the parsers the same? Joerg ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 0/6] Add virtio-iommu built-in topology
On Thu, Sep 24, 2020 at 12:02:55PM +0200, Joerg Roedel wrote: > On Thu, Sep 24, 2020 at 05:38:13AM -0400, Michael S. Tsirkin wrote: > > On Thu, Sep 24, 2020 at 11:21:29AM +0200, Joerg Roedel wrote: > > > On Thu, Sep 24, 2020 at 05:00:35AM -0400, Michael S. Tsirkin wrote: > > > > OK so this looks good. Can you pls repost with the minor tweak > > > > suggested and all acks included, and I will queue this? > > > > > > My NACK still stands, as long as a few questions are open: > > > > > > 1) The format used here will be the same as in the ACPI table? I > > > think the answer to this questions must be Yes, so this leads > > > to the real question: > > > > I am not sure it's a must. > > It is, having only one parser for the ACPI and MMIO descriptions was one > of the selling points for MMIO in past discussions and I think it makes > sense to keep them in sync. It's not possible to use exactly the same code for parsing. The access methods are different (need to deal with port-IO for built-in description on PCI, for example) and more importantly, the structure is different as well. The ACPI table needs nodes for virtio-iommu while the built-in description is contained in the virtio-iommu itself. So the endpoint nodes point to virtio-iommu node on ACPI, while they don't need a pointer on the built-in desc. I kept as much as possible common in structures and implementation, but in the end we still need about 200 unique lines on each side. Thanks, Jean > > > We can always tweak the parser if there are slight differences > > between ACPI and virtio formats. > > There is no guarantee that there only need to be "tweaks" until the > ACPI table format is stablized. > > Regards, > > Joerg > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 0/6] Add virtio-iommu built-in topology
On Thu, Sep 24, 2020 at 12:02:55PM +0200, Joerg Roedel wrote: > On Thu, Sep 24, 2020 at 05:38:13AM -0400, Michael S. Tsirkin wrote: > > On Thu, Sep 24, 2020 at 11:21:29AM +0200, Joerg Roedel wrote: > > > On Thu, Sep 24, 2020 at 05:00:35AM -0400, Michael S. Tsirkin wrote: > > > > OK so this looks good. Can you pls repost with the minor tweak > > > > suggested and all acks included, and I will queue this? > > > > > > My NACK still stands, as long as a few questions are open: > > > > > > 1) The format used here will be the same as in the ACPI table? I > > > think the answer to this questions must be Yes, so this leads > > > to the real question: > > > > I am not sure it's a must. > > It is, having only one parser for the ACPI and MMIO descriptions was one > of the selling points for MMIO in past discussions and I think it makes > sense to keep them in sync. So that requirement basically kills the "we have something to play with while the acpi table spec is in progress" argument. Also note that qemu microvm got acpi support meanwhile. Are there other cases where neither ACPI nor DT are available? take care, Gerd ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH 00/24] Control VQ support in vDPA
On Thu, Sep 24, 2020 at 11:21:01AM +0800, Jason Wang wrote: > This series tries to add the support for control virtqueue in vDPA. Please include documentation for both driver authors and vhost-vdpa ioctl users. vhost-vdpa ioctls are only documented with a single sentence. Please add full information on arguments, return values, and a high-level explanation of the feature (like this cover letter) to introduce the API. What is the policy for using virtqueue groups? My guess is: 1. virtio_vdpa simply enables all virtqueue groups. 2. vhost_vdpa relies on userspace policy on how to use virtqueue groups. Are the semantics of virtqueue groups documented somewhere so userspace knows what to do? If a vDPA driver author decides to create N virtqueue groups, N/2 virtqueue groups, or just 1 virtqueue group, how will userspace know what to do? Maybe a document is needed to describe the recommended device-specific virtqueue groups that vDPA drivers should implement (e.g. "put the net control vq into its own virtqueue group")? This could become messy with guidelines. For example, drivers might be shipped that aren't usable for certain use cases just because the author didn't know that a certain virtqueue grouping is advantageous. BTW I like how general this feature is. It seems to allow vDPA devices to be split into sub-devices for further passthrough. Who will write the first vDPA-on-vDPA driver? :) Stefan signature.asc Description: PGP signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 0/6] Add virtio-iommu built-in topology
On Thu, Sep 24, 2020 at 05:38:13AM -0400, Michael S. Tsirkin wrote: > On Thu, Sep 24, 2020 at 11:21:29AM +0200, Joerg Roedel wrote: > > On Thu, Sep 24, 2020 at 05:00:35AM -0400, Michael S. Tsirkin wrote: > > > OK so this looks good. Can you pls repost with the minor tweak > > > suggested and all acks included, and I will queue this? > > > > My NACK still stands, as long as a few questions are open: > > > > 1) The format used here will be the same as in the ACPI table? I > >think the answer to this questions must be Yes, so this leads > >to the real question: > > I am not sure it's a must. It is, having only one parser for the ACPI and MMIO descriptions was one of the selling points for MMIO in past discussions and I think it makes sense to keep them in sync. > We can always tweak the parser if there are slight differences > between ACPI and virtio formats. There is no guarantee that there only need to be "tweaks" until the ACPI table format is stablized. Regards, Joerg ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 0/6] Add virtio-iommu built-in topology
Hi, Adding Al in the loop On 9/24/20 11:38 AM, Michael S. Tsirkin wrote: > On Thu, Sep 24, 2020 at 11:21:29AM +0200, Joerg Roedel wrote: >> On Thu, Sep 24, 2020 at 05:00:35AM -0400, Michael S. Tsirkin wrote: >>> OK so this looks good. Can you pls repost with the minor tweak >>> suggested and all acks included, and I will queue this? >> >> My NACK still stands, as long as a few questions are open: >> >> 1) The format used here will be the same as in the ACPI table? I >> think the answer to this questions must be Yes, so this leads >> to the real question: > > I am not sure it's a must. > We can always tweak the parser if there are slight differences > between ACPI and virtio formats. > > But we do want the virtio format used here to be approved by the virtio > TC, so it won't change. > > Eric, Jean-Philippe, does one of you intend to create a github issue > and request a ballot for the TC? It's been posted end of August with no > changes ... Jean-Philippe, would you? > >> 2) Has the ACPI table format stabalized already? If and only if >> the answer is Yes I will Ack these patches. We don't need to >> wait until the ACPI table format is published in a >> specification update, but at least some certainty that it >> will not change in incompatible ways anymore is needed. >> Al, do you have any news about the the VIOT definition submission to the UEFI ASWG? Thank you in advance Best Regards Eric > > Not that I know, but I don't see why it's a must. > >> So what progress has been made with the ACPI table specification, is it >> just a matter of time to get it approved or are there concerns? >> >> Regards, >> >> Joerg > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 0/6] Add virtio-iommu built-in topology
On Thu, Sep 24, 2020 at 11:21:29AM +0200, Joerg Roedel wrote: > On Thu, Sep 24, 2020 at 05:00:35AM -0400, Michael S. Tsirkin wrote: > > OK so this looks good. Can you pls repost with the minor tweak > > suggested and all acks included, and I will queue this? > > My NACK still stands, as long as a few questions are open: > > 1) The format used here will be the same as in the ACPI table? I > think the answer to this questions must be Yes, so this leads > to the real question: I am not sure it's a must. We can always tweak the parser if there are slight differences between ACPI and virtio formats. But we do want the virtio format used here to be approved by the virtio TC, so it won't change. Eric, Jean-Philippe, does one of you intend to create a github issue and request a ballot for the TC? It's been posted end of August with no changes ... > 2) Has the ACPI table format stabalized already? If and only if > the answer is Yes I will Ack these patches. We don't need to > wait until the ACPI table format is published in a > specification update, but at least some certainty that it > will not change in incompatible ways anymore is needed. > Not that I know, but I don't see why it's a must. > So what progress has been made with the ACPI table specification, is it > just a matter of time to get it approved or are there concerns? > > Regards, > > Joerg ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH 02/24] vhost-vdpa: fix vqs leak in vhost_vdpa_open()
On Thu, Sep 24, 2020 at 11:21:03AM +0800, Jason Wang wrote:
> We need to free vqs during the err path after it has been allocated
> since vhost won't do that for us.
>
> Signed-off-by: Jason Wang
This is a bugfix too right? I don't see it posted separately ...
> ---
> drivers/vhost/vdpa.c | 11 ---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 796fe979f997..9c641274b9f3 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -764,6 +764,12 @@ static void vhost_vdpa_free_domain(struct vhost_vdpa *v)
> v->domain = NULL;
> }
>
> +static void vhost_vdpa_cleanup(struct vhost_vdpa *v)
> +{
> + vhost_dev_cleanup(>vdev);
> + kfree(v->vdev.vqs);
> +}
> +
> static int vhost_vdpa_open(struct inode *inode, struct file *filep)
> {
> struct vhost_vdpa *v;
> @@ -809,7 +815,7 @@ static int vhost_vdpa_open(struct inode *inode, struct
> file *filep)
> return 0;
>
> err_init_iotlb:
> - vhost_dev_cleanup(>vdev);
> + vhost_vdpa_cleanup(v);
> err:
> atomic_dec(>opened);
> return r;
> @@ -840,8 +846,7 @@ static int vhost_vdpa_release(struct inode *inode, struct
> file *filep)
> vhost_vdpa_free_domain(v);
> vhost_vdpa_config_put(v);
> vhost_vdpa_clean_irq(v);
> - vhost_dev_cleanup(>vdev);
> - kfree(v->vdev.vqs);
> + vhost_vdpa_cleanup(v);
> mutex_unlock(>mutex);
>
> atomic_dec(>opened);
> --
> 2.20.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 -next] vdpa: mlx5: change Kconfig depends to fix build errors
On Fri, Sep 18, 2020 at 11:22:45AM +0300, Leon Romanovsky wrote: > On Thu, Sep 17, 2020 at 07:35:03PM -0700, Randy Dunlap wrote: > > From: Randy Dunlap > > > > drivers/vdpa/mlx5/ uses vhost_iotlb*() interfaces, so add a dependency > > on VHOST to eliminate build errors. > > > > ld: drivers/vdpa/mlx5/core/mr.o: in function `add_direct_chain': > > mr.c:(.text+0x106): undefined reference to `vhost_iotlb_itree_first' > > ld: mr.c:(.text+0x1cf): undefined reference to `vhost_iotlb_itree_next' > > ld: mr.c:(.text+0x30d): undefined reference to `vhost_iotlb_itree_first' > > ld: mr.c:(.text+0x3e8): undefined reference to `vhost_iotlb_itree_next' > > ld: drivers/vdpa/mlx5/core/mr.o: in function `_mlx5_vdpa_create_mr': > > mr.c:(.text+0x908): undefined reference to `vhost_iotlb_itree_first' > > ld: mr.c:(.text+0x9e6): undefined reference to `vhost_iotlb_itree_next' > > ld: drivers/vdpa/mlx5/core/mr.o: in function `mlx5_vdpa_handle_set_map': > > mr.c:(.text+0xf1d): undefined reference to `vhost_iotlb_itree_first' > > > > Signed-off-by: Randy Dunlap > > Cc: "Michael S. Tsirkin" > > Cc: Jason Wang > > Cc: virtualization@lists.linux-foundation.org > > Cc: Saeed Mahameed > > Cc: Leon Romanovsky > > Cc: net...@vger.kernel.org > > --- > > v2: change from select to depends on VHOST (Saeed) > > v3: change to depends on VHOST_IOTLB (Jason) > > > > drivers/vdpa/Kconfig |2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > --- linux-next-20200917.orig/drivers/vdpa/Kconfig > > +++ linux-next-20200917/drivers/vdpa/Kconfig > > @@ -31,7 +31,7 @@ config IFCVF > > > > config MLX5_VDPA > > bool "MLX5 VDPA support library for ConnectX devices" > > - depends on MLX5_CORE > > + depends on VHOST_IOTLB && MLX5_CORE > > default n > > While we are here, can anyone who apply this patch delete the "default n" > line? > It is by default "n". > > Thanks Hmm other drivers select VHOST_IOTLB, why not do the same? > > help > > Support library for Mellanox VDPA drivers. Provides code that is > > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 0/6] Add virtio-iommu built-in topology
On Thu, Sep 24, 2020 at 05:00:35AM -0400, Michael S. Tsirkin wrote: > OK so this looks good. Can you pls repost with the minor tweak > suggested and all acks included, and I will queue this? My NACK still stands, as long as a few questions are open: 1) The format used here will be the same as in the ACPI table? I think the answer to this questions must be Yes, so this leads to the real question: 2) Has the ACPI table format stabalized already? If and only if the answer is Yes I will Ack these patches. We don't need to wait until the ACPI table format is published in a specification update, but at least some certainty that it will not change in incompatible ways anymore is needed. So what progress has been made with the ACPI table specification, is it just a matter of time to get it approved or are there concerns? Regards, Joerg ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 0/6] Add virtio-iommu built-in topology
On Fri, Sep 04, 2020 at 06:24:12PM +0200, Auger Eric wrote:
> Hi,
>
> On 8/21/20 3:15 PM, Jean-Philippe Brucker wrote:
> > Add a topology description to the virtio-iommu driver and enable x86
> > platforms.
> >
> > Since [v2] we have made some progress on adding ACPI support for
> > virtio-iommu, which is the preferred boot method on x86. It will be a
> > new vendor-agnostic table describing para-virtual topologies in a
> > minimal format. However some platforms don't use either ACPI or DT for
> > booting (for example microvm), and will need the alternative topology
> > description method proposed here. In addition, since the process to get
> > a new ACPI table will take a long time, this provides a boot method even
> > to ACPI-based platforms, if only temporarily for testing and
> > development.
> >
> > v3:
> > * Add patch 1 that moves virtio-iommu to a subfolder.
> > * Split the rest:
> > * Patch 2 adds topology-helper.c, which will be shared with the ACPI
> > support.
> > * Patch 4 adds definitions.
> > * Patch 5 adds parser in topology.c.
> > * Address other comments.
> >
> > Linux and QEMU patches available at:
> > https://jpbrucker.net/git/linux virtio-iommu/devel
> > https://jpbrucker.net/git/qemu virtio-iommu/devel
> I have tested that series with above QEMU branch on ARM with virtio-net
> and virtio-blk translated devices in non DT mode.
>
> It works for me:
> Tested-by: Eric Auger
>
> Thanks
>
> Eric
OK so this looks good. Can you pls repost with the minor tweak
suggested and all acks included, and I will queue this?
Thanks!
> >
> > [spec] https://lists.oasis-open.org/archives/virtio-dev/202008/msg00067.html
> > [v2]
> > https://lore.kernel.org/linux-iommu/20200228172537.377327-1-jean-phili...@linaro.org/
> > [v1]
> > https://lore.kernel.org/linux-iommu/20200214160413.1475396-1-jean-phili...@linaro.org/
> > [rfc]
> > https://lore.kernel.org/linux-iommu/20191122105000.800410-1-jean-phili...@linaro.org/
> >
> > Jean-Philippe Brucker (6):
> > iommu/virtio: Move to drivers/iommu/virtio/
> > iommu/virtio: Add topology helpers
> > PCI: Add DMA configuration for virtual platforms
> > iommu/virtio: Add topology definitions
> > iommu/virtio: Support topology description in config space
> > iommu/virtio: Enable x86 support
> >
> > drivers/iommu/Kconfig | 18 +-
> > drivers/iommu/Makefile| 3 +-
> > drivers/iommu/virtio/Makefile | 4 +
> > drivers/iommu/virtio/topology-helpers.h | 50 +
> > include/linux/virt_iommu.h| 15 ++
> > include/uapi/linux/virtio_iommu.h | 44
> > drivers/iommu/virtio/topology-helpers.c | 196
> > drivers/iommu/virtio/topology.c | 259 ++
> > drivers/iommu/{ => virtio}/virtio-iommu.c | 4 +
> > drivers/pci/pci-driver.c | 5 +
> > MAINTAINERS | 3 +-
> > 11 files changed, 597 insertions(+), 4 deletions(-)
> > create mode 100644 drivers/iommu/virtio/Makefile
> > create mode 100644 drivers/iommu/virtio/topology-helpers.h
> > create mode 100644 include/linux/virt_iommu.h
> > create mode 100644 drivers/iommu/virtio/topology-helpers.c
> > create mode 100644 drivers/iommu/virtio/topology.c
> > rename drivers/iommu/{ => virtio}/virtio-iommu.c (99%)
> >
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 5/6] iommu/virtio: Support topology description in config space
On Fri, Sep 04, 2020 at 06:05:35PM +0200, Auger Eric wrote: > > +static inline int viommu_pci_find_capability(struct pci_dev *dev, u8 > > cfg_type, > > +struct viommu_cap_config *cap) > not sure the inline is useful here No, I'll drop it Thanks, Jean ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 2/6] iommu/virtio: Add topology helpers
On Fri, Sep 04, 2020 at 06:22:12PM +0200, Auger Eric wrote:
> > +/**
> > + * virt_dma_configure - Configure DMA of virtualized devices
> > + * @dev: the endpoint
> > + *
> > + * Setup the DMA and IOMMU ops of a virtual device, for platforms without
> > DT or
> > + * ACPI.
> > + *
> > + * Return: -EPROBE_DEFER if the device is managed by an IOMMU that hasn't
> > been
> > + * probed yet, 0 otherwise
> > + */
> > +int virt_dma_configure(struct device *dev)
> > +{
> > + const struct iommu_ops *iommu_ops;
> > +
> > + iommu_ops = virt_iommu_setup(dev);
> > + if (IS_ERR_OR_NULL(iommu_ops)) {
> > + int ret = PTR_ERR(iommu_ops);
> > +
> > + if (ret == -EPROBE_DEFER || ret == 0)
> > + return ret;
> > + dev_err(dev, "error %d while setting up virt IOMMU\n", ret);
> > + return 0;
> why do we return 0 here?
So we can fall back to another method (ACPI or DT) if available
> Besides
>
> Reviewed-by: Eric Auger
Thanks!
Jean
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH 01/24] vhost-vdpa: fix backend feature ioctls
On 2020/9/24 下午3:50, Michael S. Tsirkin wrote:
On Thu, Sep 24, 2020 at 11:21:02AM +0800, Jason Wang wrote:
Commit 653055b9acd4 ("vhost-vdpa: support get/set backend features")
introduces two malfunction backend features ioctls:
1) the ioctls was blindly added to vring ioctl instead of vdpa device
ioctl
2) vhost_set_backend_features() was called when dev mutex has already
been held which will lead a deadlock
This patch fixes the above issues.
Cc: Eli Cohen
Reported-by: Zhu Lingshan
Fixes: 653055b9acd4 ("vhost-vdpa: support get/set backend features")
Signed-off-by: Jason Wang
Don't we want the fixes queued right now, as opposed to the rest of the
RFC?
Yes, actually I've posted in before[1].
Adding the patch here is to simplify the work for the guys that want to
do the work on top. E.g for Cindy to start the Qemu prototype.
Thanks
[1] https://www.spinics.net/lists/netdev/msg681247.html
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH 01/24] vhost-vdpa: fix backend feature ioctls
On Thu, Sep 24, 2020 at 11:21:02AM +0800, Jason Wang wrote:
> Commit 653055b9acd4 ("vhost-vdpa: support get/set backend features")
> introduces two malfunction backend features ioctls:
>
> 1) the ioctls was blindly added to vring ioctl instead of vdpa device
>ioctl
> 2) vhost_set_backend_features() was called when dev mutex has already
>been held which will lead a deadlock
>
> This patch fixes the above issues.
>
> Cc: Eli Cohen
> Reported-by: Zhu Lingshan
> Fixes: 653055b9acd4 ("vhost-vdpa: support get/set backend features")
> Signed-off-by: Jason Wang
Don't we want the fixes queued right now, as opposed to the rest of the
RFC?
> ---
> drivers/vhost/vdpa.c | 30 --
> 1 file changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 3fab94f88894..796fe979f997 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -353,8 +353,6 @@ static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v,
> unsigned int cmd,
> struct vdpa_callback cb;
> struct vhost_virtqueue *vq;
> struct vhost_vring_state s;
> - u64 __user *featurep = argp;
> - u64 features;
> u32 idx;
> long r;
>
> @@ -381,18 +379,6 @@ static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v,
> unsigned int cmd,
>
> vq->last_avail_idx = vq_state.avail_index;
> break;
> - case VHOST_GET_BACKEND_FEATURES:
> - features = VHOST_VDPA_BACKEND_FEATURES;
> - if (copy_to_user(featurep, , sizeof(features)))
> - return -EFAULT;
> - return 0;
> - case VHOST_SET_BACKEND_FEATURES:
> - if (copy_from_user(, featurep, sizeof(features)))
> - return -EFAULT;
> - if (features & ~VHOST_VDPA_BACKEND_FEATURES)
> - return -EOPNOTSUPP;
> - vhost_set_backend_features(>vdev, features);
> - return 0;
> }
>
> r = vhost_vring_ioctl(>vdev, cmd, argp);
> @@ -440,8 +426,20 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
> struct vhost_vdpa *v = filep->private_data;
> struct vhost_dev *d = >vdev;
> void __user *argp = (void __user *)arg;
> + u64 __user *featurep = argp;
> + u64 features;
> long r;
>
> + if (cmd == VHOST_SET_BACKEND_FEATURES) {
> + r = copy_from_user(, featurep, sizeof(features));
> + if (r)
> + return r;
> + if (features & ~VHOST_VDPA_BACKEND_FEATURES)
> + return -EOPNOTSUPP;
> + vhost_set_backend_features(>vdev, features);
> + return 0;
> + }
> +
> mutex_lock(>mutex);
>
> switch (cmd) {
> @@ -476,6 +474,10 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
> case VHOST_VDPA_SET_CONFIG_CALL:
> r = vhost_vdpa_set_config_call(v, argp);
> break;
> + case VHOST_GET_BACKEND_FEATURES:
> + features = VHOST_VDPA_BACKEND_FEATURES;
> + r = copy_to_user(featurep, , sizeof(features));
> + break;
> default:
> r = vhost_dev_ioctl(>vdev, cmd, argp);
> if (r == -ENOIOCTLCMD)
> --
> 2.20.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH 01/24] vhost-vdpa: fix backend feature ioctls
On 2020/9/24 下午3:16, Eli Cohen wrote:
On Thu, Sep 24, 2020 at 11:21:02AM +0800, Jason Wang wrote:
Commit 653055b9acd4 ("vhost-vdpa: support get/set backend features")
introduces two malfunction backend features ioctls:
1) the ioctls was blindly added to vring ioctl instead of vdpa device
ioctl
2) vhost_set_backend_features() was called when dev mutex has already
been held which will lead a deadlock
I assume this patch requires some patch in qemu as well. Do you have
such patch?
It's this series: [PATCH 0/3] Vhost-vDPA: batch IOTLB updating.
You were copied.
Thanks
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/8] vhost: add helper to check if a vq has been setup
On 2020/9/24 上午3:12, Mike Christie wrote:
On 9/21/20 9:02 PM, Jason Wang wrote:
On 2020/9/22 上午2:23, Mike Christie wrote:
This adds a helper check if a vq has been setup. The next patches
will use this when we move the vhost scsi cmd preallocation from per
session to per vq. In the per vq case, we only want to allocate cmds
for vqs that have actually been setup and not for all the possible
vqs.
Signed-off-by: Mike Christie
---
drivers/vhost/vhost.c | 9 +
drivers/vhost/vhost.h | 1 +
2 files changed, 10 insertions(+)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index b45519c..5dd9eb1 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -305,6 +305,15 @@ static void vhost_vring_call_reset(struct vhost_vring_call
*call_ctx)
spin_lock_init(_ctx->ctx_lock);
}
+bool vhost_vq_is_setup(struct vhost_virtqueue *vq)
+{
+ if (vq->avail && vq->desc && vq->used && vhost_vq_access_ok(vq))
+ return true;
+ else
+ return false;
+}
+EXPORT_SYMBOL_GPL(vhost_vq_is_setup);
This is probably ok but I wonder maybe we should have something like what vDPA
did (VHOST_SET_VRING_ENABLE) to match virtio 1.0 device definition.
It looks like I can make that work. Some questions:
1. Do you mean a generic VHOST_SET_VRING_ENABLE or a SCSI specific one
VHOST_SCSI_SET_VRING_ENABLE?
It would be better if we can make it generic.
2. I can see the VHOST_VDPA_SET_VRING_ENABLE kernel code and the
vhost_set_vring_enable qemu code, so I have an idea of how it should work for
vhost scsi. However, I'm not sure the requirements for a generic
VHOST_SET_VRING_ENABLE if that is what you meant. I could not find it in the
spec either. Could you send me a pointer to the section?
In the spec, for PCI, it's the queue_enable for modern device.
For example, for vhost-net we seem to enable a device in the
VHOST_NET_SET_BACKEND ioctl, so I'm not sure what behavior should be or needs
to be implemented for net and vsock.
Yes, but VHOST_NET_SET_BACKEND is for the whole device not a specific
virtqueue.
Thanks
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 3/8] vhost scsi: alloc cmds per vq instead of session
On Mon, Sep 21, 2020 at 01:23:03PM -0500, Mike Christie wrote:
> We currently are limited to 256 cmds per session. This leads to problems
> where if the user has increased virtqueue_size to more than 2 or
> cmd_per_lun to more than 256 vhost_scsi_get_tag can fail and the guest
> will get IO errors.
>
> This patch moves the cmd allocation to per vq so we can easily match
> whatever the user has specified for num_queues and
> virtqueue_size/cmd_per_lun. It also makes it easier to control how much
> memory we preallocate. For cases, where perf is not as important and
> we can use the current defaults (1 vq and 128 cmds per vq) memory use
> from preallocate cmds is cut in half. For cases, where we are willing
> to use more memory for higher perf, cmd mem use will now increase as
> the num queues and queue depth increases.
>
> Signed-off-by: Mike Christie
> ---
> drivers/vhost/scsi.c | 204
> ---
> 1 file changed, 127 insertions(+), 77 deletions(-)
>
> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index b22adf0..13311b8 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -52,7 +52,6 @@
> #define VHOST_SCSI_VERSION "v0.1"
> #define VHOST_SCSI_NAMELEN 256
> #define VHOST_SCSI_MAX_CDB_SIZE 32
> -#define VHOST_SCSI_DEFAULT_TAGS 256
> #define VHOST_SCSI_PREALLOC_SGLS 2048
> #define VHOST_SCSI_PREALLOC_UPAGES 2048
> #define VHOST_SCSI_PREALLOC_PROT_SGLS 2048
> @@ -189,6 +188,9 @@ struct vhost_scsi_virtqueue {
>* Writers must also take dev mutex and flush under it.
>*/
> int inflight_idx;
> + struct vhost_scsi_cmd *scsi_cmds;
> + struct sbitmap scsi_tags;
> + int max_cmds;
> };
>
> struct vhost_scsi {
> @@ -324,7 +326,9 @@ static void vhost_scsi_release_cmd(struct se_cmd *se_cmd)
> {
> struct vhost_scsi_cmd *tv_cmd = container_of(se_cmd,
> struct vhost_scsi_cmd, tvc_se_cmd);
> - struct se_session *se_sess = tv_cmd->tvc_nexus->tvn_se_sess;
> + struct vhost_scsi_virtqueue *svq = container_of(tv_cmd->tvc_vq,
> + struct vhost_scsi_virtqueue, vq);
> + struct vhost_scsi_inflight *inflight = tv_cmd->inflight;
> int i;
>
> if (tv_cmd->tvc_sgl_count) {
> @@ -336,8 +340,8 @@ static void vhost_scsi_release_cmd(struct se_cmd *se_cmd)
> put_page(sg_page(_cmd->tvc_prot_sgl[i]));
> }
>
> - vhost_scsi_put_inflight(tv_cmd->inflight);
> - target_free_tag(se_sess, se_cmd);
> + sbitmap_clear_bit(>scsi_tags, se_cmd->map_tag);
> + vhost_scsi_put_inflight(inflight);
> }
>
> static u32 vhost_scsi_sess_get_index(struct se_session *se_sess)
> @@ -566,13 +570,14 @@ static void vhost_scsi_complete_cmd_work(struct
> vhost_work *work)
> }
>
> static struct vhost_scsi_cmd *
> -vhost_scsi_get_tag(struct vhost_virtqueue *vq, struct vhost_scsi_tpg *tpg,
> +vhost_scsi_get_cmd(struct vhost_virtqueue *vq, struct vhost_scsi_tpg *tpg,
> unsigned char *cdb, u64 scsi_tag, u16 lun, u8 task_attr,
> u32 exp_data_len, int data_direction)
> {
> + struct vhost_scsi_virtqueue *svq = container_of(vq,
> + struct vhost_scsi_virtqueue, vq);
> struct vhost_scsi_cmd *cmd;
> struct vhost_scsi_nexus *tv_nexus;
> - struct se_session *se_sess;
> struct scatterlist *sg, *prot_sg;
> struct page **pages;
> int tag, cpu;
> @@ -582,15 +587,14 @@ static void vhost_scsi_complete_cmd_work(struct
> vhost_work *work)
> pr_err("Unable to locate active struct vhost_scsi_nexus\n");
> return ERR_PTR(-EIO);
> }
> - se_sess = tv_nexus->tvn_se_sess;
>
> - tag = sbitmap_queue_get(_sess->sess_tag_pool, );
> + tag = sbitmap_get(>scsi_tags, 0, false);
> if (tag < 0) {
> pr_err("Unable to obtain tag for vhost_scsi_cmd\n");
> return ERR_PTR(-ENOMEM);
> }
After this change, cpu is uninitialized.
>
> - cmd = &((struct vhost_scsi_cmd *)se_sess->sess_cmd_map)[tag];
> + cmd = >scsi_cmds[tag];
> sg = cmd->tvc_sgl;
> prot_sg = cmd->tvc_prot_sgl;
> pages = cmd->tvc_upages;
> @@ -1065,11 +1069,11 @@ static void vhost_scsi_submission_work(struct
> work_struct *work)
> scsi_command_size(cdb),
> VHOST_SCSI_MAX_CDB_SIZE);
> goto err;
> }
> - cmd = vhost_scsi_get_tag(vq, tpg, cdb, tag, lun, task_attr,
> + cmd = vhost_scsi_get_cmd(vq, tpg, cdb, tag, lun, task_attr,
>exp_data_len + prot_bytes,
>data_direction);
> if (IS_ERR(cmd)) {
> - vq_err(vq, "vhost_scsi_get_tag failed %ld\n",
> + vq_err(vq, "vhost_scsi_get_cmd failed %ld\n",
> PTR_ERR(cmd));
>
