* RE: [PATCH v4] virtio_blk: add DISCARD and WRIET ZEROES commands support
From: Liu, Changpeng @ 2018-05-31 23:53 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: pbonzini@redhat.com, cavery@redhat.com,
virtualization@lists.linux-foundation.org
In-Reply-To: <20180531103047.GB27838@stefanha-x1.localdomain>
> -----Original Message-----
> From: Stefan Hajnoczi [mailto:stefanha@redhat.com]
> Sent: Thursday, May 31, 2018 6:31 PM
> To: Liu, Changpeng <changpeng.liu@intel.com>
> Cc: virtualization@lists.linux-foundation.org; cavery@redhat.com;
> jasowang@redhat.com; pbonzini@redhat.com; Wang, Wei W
> <wei.w.wang@intel.com>
> Subject: Re: [PATCH v4] virtio_blk: add DISCARD and WRIET ZEROES commands
> support
>
> On Tue, May 29, 2018 at 09:42:24AM +0800, Changpeng Liu wrote:
> > num = blk_rq_map_sg(hctx->queue, req, vbr->sg);
> > if (num) {
> > - if (rq_data_dir(req) == WRITE)
> > + if (rq_data_dir(req) == WRITE || type == VIRTIO_BLK_T_DISCARD
> ||
> > + type == VIRTIO_BLK_T_WRITE_ZEROES)
> > vbr->out_hdr.type |= cpu_to_virtio32(vblk->vdev,
> VIRTIO_BLK_T_OUT);
>
> The VIRTIO specification says:
>
> The type of the request is either a read (VIRTIO_BLK_T_IN), a write
> (VIRTIO_BLK_T_OUT), a discard (VIRTIO_BLK_T_DISCARD), a write zeroes
> (VIRTIO_BLK_T_WRITE_ZEROES) or a flush (VIRTIO_BLK_T_FLUSH).
>
> But this patch actually uses VIRTIO_BLK_T_DISCARD | VIRTIO_BLK_T_OUT or
> VIRTIO_BLK_T_WRITE_ZEROES | VIRTIO_BLK_T_OUT. "either A or B" is
> exclusive, it does not mean "A and B".
Hi Stefan,
For the new add DISCARD and WRITE ZEROES commands, I defined the
type code to 11 and 13, so the last bit always is 1, there is no difference
in practice.
But I believe the specification says VIRTIO_BLK_T_OUT means direction, so
OR the two bits together should compliance with the specification.
>
> Can you clarify whether the spec needs to be changed or what the purpose
> of ORing in the VIRTIO_BLK_T_OUT bit is?
^ permalink raw reply
* Re: [PATCH net-next v12 2/5] netvsc: refactor notifier/event handling code to use the failover framework
From: Siwei Liu @ 2018-05-31 20:41 UTC (permalink / raw)
To: Michael S. Tsirkin, venu.busireddy
Cc: Alexander Duyck, virtio-dev, Jiri Pirko, Jakub Kicinski,
Sridhar Samudrala, virtualization, Netdev, anjali.singhai,
aaron.f.brown, David Miller
In-Reply-To: <20180531212356-mutt-send-email-mst@kernel.org>
On Thu, May 31, 2018 at 11:35 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Wed, May 30, 2018 at 10:06:35PM -0400, Stephen Hemminger wrote:
>> On Thu, 24 May 2018 09:55:14 -0700
>> Sridhar Samudrala <sridhar.samudrala@intel.com> wrote:
>>
>> > Use the registration/notification framework supported by the generic
>> > failover infrastructure.
>> >
>> > Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
>>
>> Why was this merged? It was never signed off by any of the netvsc maintainers,
>> and there were still issues unresolved.
>>
>> There are also namespaces issues I am fixing and this breaks them.
>> Will start my patch set with a revert for this. Sorry
>
> As long as you finish the patch set with re-integrating with failover,
> that's fine IMHO.
>
> I suspect it's easier to add the code to failover though - namespace
> things likely affect virtio as well. Lookup by ID would be an optional
> feature for virtio, but probably a useful one - I won't ask you
I would think for production uses this is a required feature and
should be enabled by default.
Venu (cc'ed) is working on the group ID stuff currently for pairing
virtio and passthrough devices. Would appreciate feedback on the group
ID proposal on virtio-dev.
-Siwei
> to add it to virtio but it could be a mode in failover
> that virtio will activate down the road. And reducing the number of
> times we look cards up based on ID can only be a good thing.
>
> --
> MST
^ permalink raw reply
* Re: [PATCH net-next v12 2/5] netvsc: refactor notifier/event handling code to use the failover framework
From: Michael S. Tsirkin @ 2018-05-31 18:35 UTC (permalink / raw)
To: Stephen Hemminger
Cc: alexander.h.duyck, virtio-dev, jiri, kubakici, Sridhar Samudrala,
virtualization, loseweigh, netdev, anjali.singhai, aaron.f.brown,
davem
In-Reply-To: <20180530220635.206ee6d7@shemminger-XPS-13-9360>
On Wed, May 30, 2018 at 10:06:35PM -0400, Stephen Hemminger wrote:
> On Thu, 24 May 2018 09:55:14 -0700
> Sridhar Samudrala <sridhar.samudrala@intel.com> wrote:
>
> > Use the registration/notification framework supported by the generic
> > failover infrastructure.
> >
> > Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
>
> Why was this merged? It was never signed off by any of the netvsc maintainers,
> and there were still issues unresolved.
>
> There are also namespaces issues I am fixing and this breaks them.
> Will start my patch set with a revert for this. Sorry
As long as you finish the patch set with re-integrating with failover,
that's fine IMHO.
I suspect it's easier to add the code to failover though - namespace
things likely affect virtio as well. Lookup by ID would be an optional
feature for virtio, but probably a useful one - I won't ask you
to add it to virtio but it could be a mode in failover
that virtio will activate down the road. And reducing the number of
times we look cards up based on ID can only be a good thing.
--
MST
^ permalink raw reply
* Re: [RFC V2] virtio: Add platform specific DMA API translation for virito devices
From: Michael S. Tsirkin @ 2018-05-31 17:43 UTC (permalink / raw)
To: Anshuman Khandual
Cc: robh, Ram Pai, linux-kernel, virtualization, hch, joe,
linuxppc-dev, elfring, david
In-Reply-To: <0c508eb2-08df-3f76-c260-90cf7137af80@linux.vnet.ibm.com>
On Thu, May 31, 2018 at 09:09:24AM +0530, Anshuman Khandual wrote:
> On 05/24/2018 12:51 PM, Ram Pai wrote:
> > On Wed, May 23, 2018 at 09:50:02PM +0300, Michael S. Tsirkin wrote:
> >> subj: s/virito/virtio/
> >>
> > ..snip..
> >>> machine_subsys_initcall_sync(pseries, tce_iommu_bus_notifier_init);
> >>> +
> >>> +bool platform_forces_virtio_dma(struct virtio_device *vdev)
> >>> +{
> >>> + /*
> >>> + * On protected guest platforms, force virtio core to use DMA
> >>> + * MAP API for all virtio devices. But there can also be some
> >>> + * exceptions for individual devices like virtio balloon.
> >>> + */
> >>> + return (of_find_compatible_node(NULL, NULL, "ibm,ultravisor") != NULL);
> >>> +}
> >>
> >> Isn't this kind of slow? vring_use_dma_api is on
> >> data path and supposed to be very fast.
> >
> > Yes it is slow and not ideal. This won't be the final code. The final
> > code will cache the information in some global variable and used
> > in this function.
>
> Right this will be a simple check based on a global variable.
>
Pls work on a long term solution. Short term needs can be served by
enabling the iommu platform in qemu.
--
MST
^ permalink raw reply
* Re: [PATCH net-next] virtio_net: fix error return code in virtnet_probe()
From: Michael S. Tsirkin @ 2018-05-31 17:37 UTC (permalink / raw)
To: Wei Yongjun; +Cc: Sridhar Samudrala, netdev, kernel-janitors, virtualization
In-Reply-To: <1527732307-145609-1-git-send-email-weiyongjun1@huawei.com>
On Thu, May 31, 2018 at 02:05:07AM +0000, Wei Yongjun wrote:
> Fix to return a negative error code from the failover create fail error
> handling case instead of 0, as done elsewhere in this function.
>
> Fixes: ba5e4426e80e ("virtio_net: Extend virtio to use VF datapath when available")
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/net/virtio_net.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 8f08a3e..2d55e2a 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2935,8 +2935,10 @@ static int virtnet_probe(struct virtio_device *vdev)
>
> if (virtio_has_feature(vdev, VIRTIO_NET_F_STANDBY)) {
> vi->failover = net_failover_create(vi->dev);
> - if (IS_ERR(vi->failover))
> + if (IS_ERR(vi->failover)) {
> + err = PTR_ERR(vi->failover);
> goto free_vqs;
> + }
> }
>
> err = register_netdev(dev);
^ permalink raw reply
* Re: [PATCH net-next v12 2/5] netvsc: refactor notifier/event handling code to use the failover framework
From: Michael S. Tsirkin @ 2018-05-31 17:34 UTC (permalink / raw)
To: Stephen Hemminger
Cc: alexander.h.duyck, virtio-dev, jiri, kubakici, Samudrala, Sridhar,
virtualization, loseweigh, netdev, anjali.singhai, aaron.f.brown,
davem
In-Reply-To: <20180531085812.0b13afef@shemminger-XPS-13-9360>
On Thu, May 31, 2018 at 08:58:12AM -0400, Stephen Hemminger wrote:
> On Wed, 30 May 2018 20:03:11 -0700
> "Samudrala, Sridhar" <sridhar.samudrala@intel.com> wrote:
>
> > On 5/30/2018 7:06 PM, Stephen Hemminger wrote:
> > > On Thu, 24 May 2018 09:55:14 -0700
> > > Sridhar Samudrala <sridhar.samudrala@intel.com> wrote:
> > >
> > >> Use the registration/notification framework supported by the generic
> > >> failover infrastructure.
> > >>
> > >> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> > > Why was this merged? It was never signed off by any of the netvsc maintainers,
> > > and there were still issues unresolved.
> > >
> > > There are also namespaces issues I am fixing and this breaks them.
> > > Will start my patch set with a revert for this. Sorry
> >
> > I would appreciate if you can make the fixes on top of this patch series. I tried hard
> > to make sure that netvsc functionality and behavior doesn't change.
> >
> > It is possible that there could be some bugs introduced, but they can be fixed.
> > Looks like Wei already found a bug and submitted a fix for that.
> >
>
> Ok, but several of these may clash with what you want for virtio.
> Like:
> - VF should be moved to namespace of virt device
> - VF should be associated based on message from host with serial # not
> registration notifier and MAC address.
> - control operations should use master device reference rather than
> searching based on MAC.
>
> As you can see these are structural changes.
We might want to do these for virtio as well, at least as
an option.
--
MST
^ permalink raw reply
* Re: [PATCH] virtio_pci: support enabling VFs
From: Alexander Duyck @ 2018-05-31 14:27 UTC (permalink / raw)
To: Tiwei Bie
Cc: Duyck, Alexander H, virtio-dev@lists.oasis-open.org,
mst@redhat.com, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, Wang, Zhihong,
bhelgaas@google.com, Rustad, Mark D
In-Reply-To: <20180531032049.GB15516@debian>
On Wed, May 30, 2018 at 8:20 PM, Tiwei Bie <tiwei.bie@intel.com> wrote:
> On Thu, May 31, 2018 at 01:11:37AM +0800, Rustad, Mark D wrote:
>> On May 30, 2018, at 9:54 AM, Duyck, Alexander H
>> <alexander.h.duyck@intel.com> wrote:
>>
>> > On Wed, 2018-05-30 at 09:44 -0700, Rustad, Mark D wrote:
>> > > On May 30, 2018, at 9:22 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > >
>> > > > > +static int virtio_pci_sriov_configure(struct pci_dev *pci_dev, int
>> > > > > num_vfs)
>> > > > > +{
>> > > > > + struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
>> > > > > + struct virtio_device *vdev = &vp_dev->vdev;
>> > > > > + int (*sriov_configure)(struct pci_dev *pci_dev, int num_vfs);
>> > > > > +
>> > > > > + if (!(vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_DRIVER_OK))
>> > > > > + return -EBUSY;
>> > > > > +
>> > > > > + if (!__virtio_test_bit(vdev, VIRTIO_F_SR_IOV))
>> > > > > + return -EINVAL;
>> > > > > +
>> > > > > + sriov_configure = pci_sriov_configure_simple;
>> > > > > + if (sriov_configure == NULL)
>> > > > > + return -ENOENT;
>> > > >
>> > > > BTW what is all this trickery in aid of?
>> > >
>> > > When SR-IOV support is not compiled into the kernel,
>> > > pci_sriov_configure_simple is #defined as NULL. This allows it to compile
>> > > in that case, even though there is utterly no way for it to be called in
>> > > that case. It is an alternative to #ifs in the code.
>> >
>> > Why even have the call though? I would wrap all of this in an #ifdef
>> > and strip it out since you couldn't support SR-IOV if it isn't present
>> > in the kernel anyway.
>>
>> I am inclined to agree. In this case, the presence of #ifdefs I think would
>> be clearer. As written, someone will want to get rid of the pointer only to
>> create a build problem when SR-IOV is not configured.
>
> In my opinion, maybe it would be better to make
> pci_sriov_configure_simple() always available
> just like other sriov functions.
>
> Based on the comments in the original patch:
>
> https://patchwork.kernel.org/patch/10353197/
> """
> +/* this is expected to be used as a function pointer, just define as NULL */
> +#define pci_sriov_configure_simple NULL
> """
>
> This function could be defined as NULL just because
> it was expected to be used as a function pointer.
> But actually it could be called directly as a
> function, just like this case.
>
> So I prefer to make this function always available
> just like other sriov functions.
>
> Best regards,
> Tiwei Bie
The fact that you are having to add additional code kind of implies
that maybe this doesn't fall into the pci_sriov_configure_simple case
anymore. The PF itself is defining what the VF can and can't do via
the feature flags you are testing for.
For example how is the bit of code below valid if the kernel itself
doesn't support SR-IOV:
+static void vp_transport_features(struct virtio_device *vdev, u64 features)
+{
+ struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+ struct pci_dev *pci_dev = vp_dev->pci_dev;
+
+ if ((features & BIT_ULL(VIRTIO_F_SR_IOV)) &&
+ pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_SRIOV))
+ __virtio_set_bit(vdev, VIRTIO_F_SR_IOV);
+}
+
It really seems like we should be wrapping these functions at the very
minimum so that they don't imply you have SR-IOV support when it isn't
supported in the kernel.
Also it seems like we should be disabling the VFs if the driver is
unbound from this interface. We need to add logic to disable SR-IOV if
the driver is removed. What we don't want to do is leave VFs allocated
and then have the potential for us to unbind/rebind the driver as the
new driver may change the negotiated features.
- Alex
^ permalink raw reply
* Re: [PATCH net-next v12 2/5] netvsc: refactor notifier/event handling code to use the failover framework
From: Stephen Hemminger @ 2018-05-31 12:58 UTC (permalink / raw)
To: Samudrala, Sridhar
Cc: alexander.h.duyck, virtio-dev, jiri, mst, kubakici, netdev,
virtualization, loseweigh, anjali.singhai, aaron.f.brown, davem
In-Reply-To: <274f0b84-07f1-5cd5-e256-ce4b71358c14@intel.com>
On Wed, 30 May 2018 20:03:11 -0700
"Samudrala, Sridhar" <sridhar.samudrala@intel.com> wrote:
> On 5/30/2018 7:06 PM, Stephen Hemminger wrote:
> > On Thu, 24 May 2018 09:55:14 -0700
> > Sridhar Samudrala <sridhar.samudrala@intel.com> wrote:
> >
> >> Use the registration/notification framework supported by the generic
> >> failover infrastructure.
> >>
> >> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> > Why was this merged? It was never signed off by any of the netvsc maintainers,
> > and there were still issues unresolved.
> >
> > There are also namespaces issues I am fixing and this breaks them.
> > Will start my patch set with a revert for this. Sorry
>
> I would appreciate if you can make the fixes on top of this patch series. I tried hard
> to make sure that netvsc functionality and behavior doesn't change.
>
> It is possible that there could be some bugs introduced, but they can be fixed.
> Looks like Wei already found a bug and submitted a fix for that.
>
Ok, but several of these may clash with what you want for virtio.
Like:
- VF should be moved to namespace of virt device
- VF should be associated based on message from host with serial # not
registration notifier and MAC address.
- control operations should use master device reference rather than
searching based on MAC.
As you can see these are structural changes.
^ permalink raw reply
* Re: [PATCH v4] virtio_blk: add DISCARD and WRIET ZEROES commands support
From: Stefan Hajnoczi @ 2018-05-31 10:30 UTC (permalink / raw)
To: Changpeng Liu; +Cc: pbonzini, cavery, virtualization
In-Reply-To: <1527558144-3825-1-git-send-email-changpeng.liu@intel.com>
[-- Attachment #1.1: Type: text/plain, Size: 884 bytes --]
On Tue, May 29, 2018 at 09:42:24AM +0800, Changpeng Liu wrote:
> num = blk_rq_map_sg(hctx->queue, req, vbr->sg);
> if (num) {
> - if (rq_data_dir(req) == WRITE)
> + if (rq_data_dir(req) == WRITE || type == VIRTIO_BLK_T_DISCARD ||
> + type == VIRTIO_BLK_T_WRITE_ZEROES)
> vbr->out_hdr.type |= cpu_to_virtio32(vblk->vdev, VIRTIO_BLK_T_OUT);
The VIRTIO specification says:
The type of the request is either a read (VIRTIO_BLK_T_IN), a write
(VIRTIO_BLK_T_OUT), a discard (VIRTIO_BLK_T_DISCARD), a write zeroes
(VIRTIO_BLK_T_WRITE_ZEROES) or a flush (VIRTIO_BLK_T_FLUSH).
But this patch actually uses VIRTIO_BLK_T_DISCARD | VIRTIO_BLK_T_OUT or
VIRTIO_BLK_T_WRITE_ZEROES | VIRTIO_BLK_T_OUT. "either A or B" is
exclusive, it does not mean "A and B".
Can you clarify whether the spec needs to be changed or what the purpose
of ORing in the VIRTIO_BLK_T_OUT bit is?
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC V2] virtio: Add platform specific DMA API translation for virito devices
From: Anshuman Khandual @ 2018-05-31 3:39 UTC (permalink / raw)
To: Ram Pai, Michael S. Tsirkin
Cc: robh, linux-kernel, virtualization, hch, joe, linuxppc-dev,
elfring, david
In-Reply-To: <20180524072104.GD6139@ram.oc3035372033.ibm.com>
On 05/24/2018 12:51 PM, Ram Pai wrote:
> On Wed, May 23, 2018 at 09:50:02PM +0300, Michael S. Tsirkin wrote:
>> subj: s/virito/virtio/
>>
> ..snip..
>>> machine_subsys_initcall_sync(pseries, tce_iommu_bus_notifier_init);
>>> +
>>> +bool platform_forces_virtio_dma(struct virtio_device *vdev)
>>> +{
>>> + /*
>>> + * On protected guest platforms, force virtio core to use DMA
>>> + * MAP API for all virtio devices. But there can also be some
>>> + * exceptions for individual devices like virtio balloon.
>>> + */
>>> + return (of_find_compatible_node(NULL, NULL, "ibm,ultravisor") != NULL);
>>> +}
>>
>> Isn't this kind of slow? vring_use_dma_api is on
>> data path and supposed to be very fast.
>
> Yes it is slow and not ideal. This won't be the final code. The final
> code will cache the information in some global variable and used
> in this function.
Right this will be a simple check based on a global variable.
^ permalink raw reply
* Re: [PATCH] virtio_pci: support enabling VFs
From: Tiwei Bie @ 2018-05-31 3:20 UTC (permalink / raw)
To: Duyck, Alexander H, Rustad, Mark D
Cc: virtio-dev@lists.oasis-open.org, mst@redhat.com,
linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, Wang, Zhihong,
bhelgaas@google.com
In-Reply-To: <5063D90B-7955-4F1E-85A2-D8AFD661ACB7@intel.com>
On Thu, May 31, 2018 at 01:11:37AM +0800, Rustad, Mark D wrote:
> On May 30, 2018, at 9:54 AM, Duyck, Alexander H
> <alexander.h.duyck@intel.com> wrote:
>
> > On Wed, 2018-05-30 at 09:44 -0700, Rustad, Mark D wrote:
> > > On May 30, 2018, at 9:22 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > > > +static int virtio_pci_sriov_configure(struct pci_dev *pci_dev, int
> > > > > num_vfs)
> > > > > +{
> > > > > + struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
> > > > > + struct virtio_device *vdev = &vp_dev->vdev;
> > > > > + int (*sriov_configure)(struct pci_dev *pci_dev, int num_vfs);
> > > > > +
> > > > > + if (!(vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_DRIVER_OK))
> > > > > + return -EBUSY;
> > > > > +
> > > > > + if (!__virtio_test_bit(vdev, VIRTIO_F_SR_IOV))
> > > > > + return -EINVAL;
> > > > > +
> > > > > + sriov_configure = pci_sriov_configure_simple;
> > > > > + if (sriov_configure == NULL)
> > > > > + return -ENOENT;
> > > >
> > > > BTW what is all this trickery in aid of?
> > >
> > > When SR-IOV support is not compiled into the kernel,
> > > pci_sriov_configure_simple is #defined as NULL. This allows it to compile
> > > in that case, even though there is utterly no way for it to be called in
> > > that case. It is an alternative to #ifs in the code.
> >
> > Why even have the call though? I would wrap all of this in an #ifdef
> > and strip it out since you couldn't support SR-IOV if it isn't present
> > in the kernel anyway.
>
> I am inclined to agree. In this case, the presence of #ifdefs I think would
> be clearer. As written, someone will want to get rid of the pointer only to
> create a build problem when SR-IOV is not configured.
In my opinion, maybe it would be better to make
pci_sriov_configure_simple() always available
just like other sriov functions.
Based on the comments in the original patch:
https://patchwork.kernel.org/patch/10353197/
"""
+/* this is expected to be used as a function pointer, just define as NULL */
+#define pci_sriov_configure_simple NULL
"""
This function could be defined as NULL just because
it was expected to be used as a function pointer.
But actually it could be called directly as a
function, just like this case.
So I prefer to make this function always available
just like other sriov functions.
Best regards,
Tiwei Bie
^ permalink raw reply
* Re: [RFC V5 PATCH 8/8] vhost: event suppression for packed ring
From: Jason Wang @ 2018-05-31 3:09 UTC (permalink / raw)
To: Wei Xu; +Cc: kvm, mst, netdev, linux-kernel, virtualization
In-Reply-To: <20180530114200.GA23792@wei-ubt>
On 2018年05月30日 19:42, Wei Xu wrote:
>> /* This actually signals the guest, using eventfd. */
>> void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq)
>> {
>> @@ -2802,10 +2930,34 @@ static bool vhost_enable_notify_packed(struct vhost_dev *dev,
>> struct vhost_virtqueue *vq)
>> {
>> struct vring_desc_packed *d = vq->desc_packed + vq->avail_idx;
>> - __virtio16 flags;
>> + __virtio16 flags = RING_EVENT_FLAGS_ENABLE;
>> int ret;
>>
>> - /* FIXME: disable notification through device area */
>> + if (!(vq->used_flags & VRING_USED_F_NO_NOTIFY))
>> + return false;
>> + vq->used_flags &= ~VRING_USED_F_NO_NOTIFY;
> 'used_flags' was originally designed for 1.0, why should we pay attetion to it here?
>
> Wei
It was used to recored whether or not we've disabled notification. Then
we can avoid unnecessary userspace writes or memory barriers.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next v12 2/5] netvsc: refactor notifier/event handling code to use the failover framework
From: Samudrala, Sridhar @ 2018-05-31 3:03 UTC (permalink / raw)
To: Stephen Hemminger
Cc: alexander.h.duyck, virtio-dev, jiri, mst, kubakici, netdev,
virtualization, loseweigh, anjali.singhai, aaron.f.brown, davem
In-Reply-To: <20180530220635.206ee6d7@shemminger-XPS-13-9360>
On 5/30/2018 7:06 PM, Stephen Hemminger wrote:
> On Thu, 24 May 2018 09:55:14 -0700
> Sridhar Samudrala <sridhar.samudrala@intel.com> wrote:
>
>> Use the registration/notification framework supported by the generic
>> failover infrastructure.
>>
>> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> Why was this merged? It was never signed off by any of the netvsc maintainers,
> and there were still issues unresolved.
>
> There are also namespaces issues I am fixing and this breaks them.
> Will start my patch set with a revert for this. Sorry
I would appreciate if you can make the fixes on top of this patch series. I tried hard
to make sure that netvsc functionality and behavior doesn't change.
It is possible that there could be some bugs introduced, but they can be fixed.
Looks like Wei already found a bug and submitted a fix for that.
^ permalink raw reply
* Re: [PATCH net-next v12 1/5] net: Introduce generic failover module
From: Samudrala, Sridhar @ 2018-05-31 2:58 UTC (permalink / raw)
To: Stephen Hemminger
Cc: alexander.h.duyck, virtio-dev, jiri, mst, kubakici, netdev,
virtualization, loseweigh, anjali.singhai, aaron.f.brown, davem
In-Reply-To: <20180530225259.2cf3f7be@shemminger-XPS-13-9360>
On 5/30/2018 7:52 PM, Stephen Hemminger wrote:
> On Fri, 25 May 2018 16:06:58 -0700
> "Samudrala, Sridhar" <sridhar.samudrala@intel.com> wrote:
>
>> On 5/25/2018 3:38 PM, Stephen Hemminger wrote:
>>> On Thu, 24 May 2018 09:55:13 -0700
>>> Sridhar Samudrala <sridhar.samudrala@intel.com> wrote:
>>>
>>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>>> index 03ed492c4e14..0f4ba52b641d 100644
>>>> --- a/include/linux/netdevice.h
>>>> +++ b/include/linux/netdevice.h
>>>> @@ -1421,6 +1421,8 @@ struct net_device_ops {
>>>> * entity (i.e. the master device for bridged veth)
>>>> * @IFF_MACSEC: device is a MACsec device
>>>> * @IFF_NO_RX_HANDLER: device doesn't support the rx_handler hook
>>>> + * @IFF_FAILOVER: device is a failover master device
>>>> + * @IFF_FAILOVER_SLAVE: device is lower dev of a failover master device
>>>> */
>>>> enum netdev_priv_flags {
>>>> IFF_802_1Q_VLAN = 1<<0,
>>>> @@ -1450,6 +1452,8 @@ enum netdev_priv_flags {
>>>> IFF_PHONY_HEADROOM = 1<<24,
>>>> IFF_MACSEC = 1<<25,
>>>> IFF_NO_RX_HANDLER = 1<<26,
>>>> + IFF_FAILOVER = 1<<27,
>>>> + IFF_FAILOVER_SLAVE = 1<<28,
>>>> };
>>> Why is FAILOVER any different than other master/slave relationships.
>>> I don't think you need to take up precious netdev flag bits for this.
>> These are netdev priv flags.
>> Jiri says that IFF_MASTER/IFF_SLAVE are bonding specific flags and cannot be used
>> with other failover mechanisms. Team also doesn't use this flags and it has its own
>> priv_flags.
>>
> This change breaks userspace.
> We already have worked with partners to ignore devices marked as IFF_SLAVE,
> and IFF_SLAVE is visible to user space API's.
I specifically made sure not to remove IFF_SLAVE in the netvsc patch.
>
> NAK
^ permalink raw reply
* Re: [virtio-dev] [PATCH] virtio_pci: support enabling VFs
From: Tiwei Bie @ 2018-05-31 2:55 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Duyck, Alexander H, virtio-dev@lists.oasis-open.org,
linux-pci@vger.kernel.org, LKML,
virtualization@lists.linux-foundation.org, Wang, Zhihong,
Bjorn Helgaas, Rustad, Mark D
In-Reply-To: <20180530190819-mutt-send-email-mst@kernel.org>
On Wed, May 30, 2018 at 07:09:37PM +0300, Michael S. Tsirkin wrote:
> On Wed, May 30, 2018 at 04:03:37PM +0000, Rustad, Mark D wrote:
> > On May 30, 2018, at 1:55 AM, Tiwei Bie <tiwei.bie@intel.com> wrote:
> >
> > > There is a new feature bit allocated in virtio spec to
> > > support SR-IOV (Single Root I/O Virtualization):
> > >
> > > https://github.com/oasis-tcs/virtio-spec/issues/11
> > >
> > > This patch enables the support for this feature bit in
> > > virtio driver.
> > >
> > > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> > > ---
> > > This patch depends on below proposal:
> > > https://lists.oasis-open.org/archives/virtio-dev/201805/msg00154.html
> > >
> > > This patch depends on below patch:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=pci/virtualization&id=8effc395c2097e258fcedfc02ed4a66d45fb4238
> > >
> > > drivers/virtio/virtio_pci_common.c | 20 ++++++++++++++++++++
> > > drivers/virtio/virtio_pci_modern.c | 14 ++++++++++++++
> > > include/uapi/linux/virtio_config.h | 7 ++++++-
> > > 3 files changed, 40 insertions(+), 1 deletion(-)
> > >
> >
> > <snip>
> >
> > > diff --git a/include/uapi/linux/virtio_config.h
> > > b/include/uapi/linux/virtio_config.h
> > > index 308e2096291f..b7c1f4e7d59e 100644
> > > --- a/include/uapi/linux/virtio_config.h
> > > +++ b/include/uapi/linux/virtio_config.h
> > > @@ -49,7 +49,7 @@
> >
> > There is a value in the comment directly before this that should
> > be updated as well to be consistent with the new value for
> > VIRTIO_TRANSPORT_F_END below.
>
> It hasn't been updated to 34 yet.
> I suggest a separate patch to replace the numbers with
> VIRTIO_TRANSPORT_F_START and VIRTIO_TRANSPORT_F_END
> in the comment.
> Maybe replace "e.g. virtio_ring" with "e.g. virtio_ring,
> virtio_pci etc." as well.
Good idea! Thanks for the suggestion! I'll do it.
Best regards,
Tiwei Bie
>
> > > * transport being used (eg. virtio_ring), the rest are per-device feature
> > > * bits. */
> > > #define VIRTIO_TRANSPORT_F_START 28
> > > -#define VIRTIO_TRANSPORT_F_END 34
> > > +#define VIRTIO_TRANSPORT_F_END 38
> > >
> > > #ifndef VIRTIO_CONFIG_NO_LEGACY
> > > /* Do we get callbacks when the ring is completely used, even if we've
> >
> > <snip>
> >
> > --
> > Mark Rustad, Networking Division, Intel Corporation
>
>
^ permalink raw reply
* Re: [PATCH net-next v12 1/5] net: Introduce generic failover module
From: Stephen Hemminger @ 2018-05-31 2:52 UTC (permalink / raw)
To: Samudrala, Sridhar
Cc: alexander.h.duyck, virtio-dev, jiri, mst, kubakici, netdev,
virtualization, loseweigh, anjali.singhai, aaron.f.brown, davem
In-Reply-To: <00d34f67-f26f-0b20-af3f-2add24ae8a5c@intel.com>
On Fri, 25 May 2018 16:06:58 -0700
"Samudrala, Sridhar" <sridhar.samudrala@intel.com> wrote:
> On 5/25/2018 3:38 PM, Stephen Hemminger wrote:
> > On Thu, 24 May 2018 09:55:13 -0700
> > Sridhar Samudrala <sridhar.samudrala@intel.com> wrote:
> >
> >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> >> index 03ed492c4e14..0f4ba52b641d 100644
> >> --- a/include/linux/netdevice.h
> >> +++ b/include/linux/netdevice.h
> >> @@ -1421,6 +1421,8 @@ struct net_device_ops {
> >> * entity (i.e. the master device for bridged veth)
> >> * @IFF_MACSEC: device is a MACsec device
> >> * @IFF_NO_RX_HANDLER: device doesn't support the rx_handler hook
> >> + * @IFF_FAILOVER: device is a failover master device
> >> + * @IFF_FAILOVER_SLAVE: device is lower dev of a failover master device
> >> */
> >> enum netdev_priv_flags {
> >> IFF_802_1Q_VLAN = 1<<0,
> >> @@ -1450,6 +1452,8 @@ enum netdev_priv_flags {
> >> IFF_PHONY_HEADROOM = 1<<24,
> >> IFF_MACSEC = 1<<25,
> >> IFF_NO_RX_HANDLER = 1<<26,
> >> + IFF_FAILOVER = 1<<27,
> >> + IFF_FAILOVER_SLAVE = 1<<28,
> >> };
> > Why is FAILOVER any different than other master/slave relationships.
> > I don't think you need to take up precious netdev flag bits for this.
>
> These are netdev priv flags.
> Jiri says that IFF_MASTER/IFF_SLAVE are bonding specific flags and cannot be used
> with other failover mechanisms. Team also doesn't use this flags and it has its own
> priv_flags.
>
This change breaks userspace.
We already have worked with partners to ignore devices marked as IFF_SLAVE,
and IFF_SLAVE is visible to user space API's.
NAK
^ permalink raw reply
* Re: [PATCH net-next v12 2/5] netvsc: refactor notifier/event handling code to use the failover framework
From: Stephen Hemminger @ 2018-05-31 2:06 UTC (permalink / raw)
To: Sridhar Samudrala
Cc: alexander.h.duyck, virtio-dev, jiri, mst, kubakici, netdev,
virtualization, loseweigh, anjali.singhai, aaron.f.brown, davem
In-Reply-To: <1527180917-39737-3-git-send-email-sridhar.samudrala@intel.com>
On Thu, 24 May 2018 09:55:14 -0700
Sridhar Samudrala <sridhar.samudrala@intel.com> wrote:
> Use the registration/notification framework supported by the generic
> failover infrastructure.
>
> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
Why was this merged? It was never signed off by any of the netvsc maintainers,
and there were still issues unresolved.
There are also namespaces issues I am fixing and this breaks them.
Will start my patch set with a revert for this. Sorry
^ permalink raw reply
* Re: [PATCH net-next] virtio_net: fix error return code in virtnet_probe()
From: Jason Wang @ 2018-05-31 2:05 UTC (permalink / raw)
To: Wei Yongjun, Michael S. Tsirkin, Sridhar Samudrala
Cc: netdev, kernel-janitors, virtualization
In-Reply-To: <1527732307-145609-1-git-send-email-weiyongjun1@huawei.com>
On 2018年05月31日 10:05, Wei Yongjun wrote:
> Fix to return a negative error code from the failover create fail error
> handling case instead of 0, as done elsewhere in this function.
>
> Fixes: ba5e4426e80e ("virtio_net: Extend virtio to use VF datapath when available")
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> ---
> drivers/net/virtio_net.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 8f08a3e..2d55e2a 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2935,8 +2935,10 @@ static int virtnet_probe(struct virtio_device *vdev)
>
> if (virtio_has_feature(vdev, VIRTIO_NET_F_STANDBY)) {
> vi->failover = net_failover_create(vi->dev);
> - if (IS_ERR(vi->failover))
> + if (IS_ERR(vi->failover)) {
> + err = PTR_ERR(vi->failover);
> goto free_vqs;
> + }
> }
>
> err = register_netdev(dev);
>
Acked-by: Jason Wang <jasowang@redhat.com>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v2 00/13] drm: Eliminate plane->fb/crtc usage for atomic drivers
From: Sinclair Yeh @ 2018-05-30 17:41 UTC (permalink / raw)
To: Ville Syrjala
Cc: David Airlie, Daniel Vetter, dri-devel, virtualization,
Eric Anholt, David (ChunMing) Zhou, Thomas Hellstrom,
Joonyoung Shim, Kyungmin Park, amd-gfx, VMware Graphics,
Harry Wentland, linux-arm-msm, intel-gfx, Inki Dae, Deepak Rawat,
Seung-Woo Kim, Rob Clark, Alex Deucher, freedreno,
Christian König
In-Reply-To: <20180525185045.29689-1-ville.syrjala@linux.intel.com>
Thanks Ville.
This series: Reviewed-by: Sinclair Yeh <syeh@vmware.com>
On Fri, May 25, 2018 at 09:50:32PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Here are again the last (?) bits of eliminating the plane->fb/crtc
> usage for atomic drivers. I've pushed everything else (thanks to
> everyone who reviewed them).
>
> Deepak said he'd tested the vmwgfx stuff, so I think it should be
> safe to land. Just missing a bit of review...
>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: amd-gfx@lists.freedesktop.org
> Cc: "Christian König" <christian.koenig@amd.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: David Airlie <airlied@linux.ie>
> Cc: "David (ChunMing) Zhou" <David1.Zhou@amd.com>
> Cc: Deepak Rawat <drawat@vmware.com>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: freedreno@lists.freedesktop.org
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Inki Dae <inki.dae@samsung.com>
> Cc: Joonyoung Shim <jy0922.shim@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: linux-arm-msm@vger.kernel.org
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
> Cc: Sinclair Yeh <syeh@vmware.com>
> Cc: Thomas Hellstrom <thellstrom@vmware.com>
> Cc: virtualization@lists.linux-foundation.org
> Cc: VMware Graphics <linux-graphics-maintainer@vmware.com>
>
> Ville Syrjälä (13):
> drm/vmwgfx: Stop using plane->fb in vmw_kms_atomic_check_modeset()
> drm/vmwgfx: Stop using plane->fb in vmw_kms_helper_dirty()
> drm/vmwgfx: Stop using plane->fb in vmw_kms_update_implicit_fb()
> drm/vmwgfx: Stop updating plane->fb
> drm/vmwgfx: Stop using plane->fb in atomic_enable()
> drm/vmwgfx: Stop messing about with plane->fb/old_fb/crtc
> drm/amdgpu/dc: Stop updating plane->fb
> drm/i915: Stop updating plane->fb/crtc
> drm/exynos: Stop updating plane->crtc
> drm/msm: Stop updating plane->fb/crtc
> drm/virtio: Stop updating plane->crtc
> drm/vc4: Stop updating plane->fb/crtc
> drm: Stop updating plane->crtc/fb/old_fb on atomic drivers
>
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 -
> drivers/gpu/drm/drm_atomic.c | 55 +++--------------------
> drivers/gpu/drm/drm_atomic_helper.c | 15 +------
> drivers/gpu/drm/drm_crtc.c | 8 +++-
> drivers/gpu/drm/drm_fb_helper.c | 7 ---
> drivers/gpu/drm/drm_framebuffer.c | 5 ---
> drivers/gpu/drm/drm_plane.c | 14 +++---
> drivers/gpu/drm/drm_plane_helper.c | 4 +-
> drivers/gpu/drm/exynos/exynos_drm_plane.c | 2 -
> drivers/gpu/drm/i915/intel_atomic_plane.c | 12 -----
> drivers/gpu/drm/i915/intel_display.c | 7 ++-
> drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 1 -
> drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c | 2 -
> drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 1 -
> drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 2 -
> drivers/gpu/drm/vc4/vc4_crtc.c | 3 --
> drivers/gpu/drm/virtio/virtgpu_display.c | 2 -
> drivers/gpu/drm/vmwgfx/vmwgfx_fb.c | 24 ----------
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 24 +++++++---
> drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 2 -
> drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 5 +--
> include/drm/drm_atomic.h | 3 --
> 22 files changed, 46 insertions(+), 154 deletions(-)
>
> --
> 2.16.1
^ permalink raw reply
* Re: [PATCH net] vhost_net: flush batched heads before trying to busy polling
From: David Miller @ 2018-05-30 17:31 UTC (permalink / raw)
To: jasowang; +Cc: netdev, virtualization, linux-kernel, kvm, mst
In-Reply-To: <1527574699-13047-1-git-send-email-jasowang@redhat.com>
From: Jason Wang <jasowang@redhat.com>
Date: Tue, 29 May 2018 14:18:19 +0800
> After commit e2b3b35eb989 ("vhost_net: batch used ring update in rx"),
> we tend to batch updating used heads. But it doesn't flush batched
> heads before trying to do busy polling, this will cause vhost to wait
> for guest TX which waits for the used RX. Fixing by flush batched
> heads before busy loop.
>
> 1 byte TCP_RR performance recovers from 13107.83 to 50402.65.
>
> Fixes: e2b3b35eb989 ("vhost_net: batch used ring update in rx")
> Signed-off-by: Jason Wang <jasowang@redhat.com>
Applied and queued up for -stable.
^ permalink raw reply
* Re: [virtio-dev] [PATCH] virtio_pci: support enabling VFs
From: Michael S. Tsirkin @ 2018-05-30 17:15 UTC (permalink / raw)
To: Duyck, Alexander H
Cc: virtio-dev@lists.oasis-open.org, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org, alexander.duyck@gmail.com,
virtualization@lists.linux-foundation.org, Wang, Zhihong,
bhelgaas@google.com, Rustad, Mark D
In-Reply-To: <1527697588.16245.136.camel@intel.com>
On Wed, May 30, 2018 at 04:26:30PM +0000, Duyck, Alexander H wrote:
> On Wed, 2018-05-30 at 19:22 +0300, Michael S. Tsirkin wrote:
> > On Wed, May 30, 2018 at 09:10:57AM -0700, Alexander Duyck wrote:
> > > On Wed, May 30, 2018 at 1:55 AM, Tiwei Bie <tiwei.bie@intel.com> wrote:
> > > > There is a new feature bit allocated in virtio spec to
> > > > support SR-IOV (Single Root I/O Virtualization):
> > > >
> > > > https://github.com/oasis-tcs/virtio-spec/issues/11
> > > >
> > > > This patch enables the support for this feature bit in
> > > > virtio driver.
> > > >
> > > > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> > >
> > > So from a quick glance it looks like we are leaving SR-IOV enabled if
> > > the driver is removed. Do we want to have that behavior or should we
> > > be adding the code to disable SR-IOV and free the VFs on driver
> > > removal?
> >
> > Could pci core handle it for us somehow?
>
> Maybe, but it would require changes to the pci core to do it.
>
> The problem is some drivers want to leave the VFs there since the PF
> doesn't really do anything, or they have the option of essentially
> putting the VFs into a standby state when the PF is gone.
>
> My main concern is do we care if VFs are allocated and then somebody
> removes the driver and binds a different driver to the interface? If
> not then this code and be left as is, but I just wanted to be certain
> since I know this isn't just enabling SR-IOV we are having to do a
> number of other checks against the virtio device.
Well the spec says features have to be negotiated, and since we reset
the device when we unbind from it I think it's a given we should keep a
driver bound to the PF.
IOW until we are sure we need the capability to keep it enabled, let's
disable it to be safe.
--
MST
^ permalink raw reply
* Re: [PATCH] virtio_pci: support enabling VFs
From: Rustad, Mark D @ 2018-05-30 17:11 UTC (permalink / raw)
To: Duyck, Alexander H
Cc: virtio-dev@lists.oasis-open.org, mst@redhat.com,
linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, Wang, Zhihong,
bhelgaas@google.com
In-Reply-To: <1527699273.29907.2.camel@intel.com>
[-- Attachment #1.1: Type: text/plain, Size: 1571 bytes --]
On May 30, 2018, at 9:54 AM, Duyck, Alexander H
<alexander.h.duyck@intel.com> wrote:
> On Wed, 2018-05-30 at 09:44 -0700, Rustad, Mark D wrote:
>> On May 30, 2018, at 9:22 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>
>>>> +static int virtio_pci_sriov_configure(struct pci_dev *pci_dev, int
>>>> num_vfs)
>>>> +{
>>>> + struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
>>>> + struct virtio_device *vdev = &vp_dev->vdev;
>>>> + int (*sriov_configure)(struct pci_dev *pci_dev, int num_vfs);
>>>> +
>>>> + if (!(vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_DRIVER_OK))
>>>> + return -EBUSY;
>>>> +
>>>> + if (!__virtio_test_bit(vdev, VIRTIO_F_SR_IOV))
>>>> + return -EINVAL;
>>>> +
>>>> + sriov_configure = pci_sriov_configure_simple;
>>>> + if (sriov_configure == NULL)
>>>> + return -ENOENT;
>>>
>>> BTW what is all this trickery in aid of?
>>
>> When SR-IOV support is not compiled into the kernel,
>> pci_sriov_configure_simple is #defined as NULL. This allows it to compile
>> in that case, even though there is utterly no way for it to be called in
>> that case. It is an alternative to #ifs in the code.
>
> Why even have the call though? I would wrap all of this in an #ifdef
> and strip it out since you couldn't support SR-IOV if it isn't present
> in the kernel anyway.
I am inclined to agree. In this case, the presence of #ifdefs I think would
be clearer. As written, someone will want to get rid of the pointer only to
create a build problem when SR-IOV is not configured.
--
Mark Rustad, Networking Division, Intel Corporation
[-- Attachment #1.2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] virtio_pci: support enabling VFs
From: Duyck, Alexander H @ 2018-05-30 16:54 UTC (permalink / raw)
To: Rustad, Mark D, mst@redhat.com
Cc: virtio-dev@lists.oasis-open.org, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, Wang, Zhihong,
bhelgaas@google.com
In-Reply-To: <414C18B1-30FA-4AC0-B47D-F0FBF9832737@intel.com>
On Wed, 2018-05-30 at 09:44 -0700, Rustad, Mark D wrote:
> On May 30, 2018, at 9:22 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> > > +static int virtio_pci_sriov_configure(struct pci_dev *pci_dev, int
> > > num_vfs)
> > > +{
> > > + struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
> > > + struct virtio_device *vdev = &vp_dev->vdev;
> > > + int (*sriov_configure)(struct pci_dev *pci_dev, int num_vfs);
> > > +
> > > + if (!(vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_DRIVER_OK))
> > > + return -EBUSY;
> > > +
> > > + if (!__virtio_test_bit(vdev, VIRTIO_F_SR_IOV))
> > > + return -EINVAL;
> > > +
> > > + sriov_configure = pci_sriov_configure_simple;
> > > + if (sriov_configure == NULL)
> > > + return -ENOENT;
> >
> > BTW what is all this trickery in aid of?
>
> When SR-IOV support is not compiled into the kernel,
> pci_sriov_configure_simple is #defined as NULL. This allows it to compile
> in that case, even though there is utterly no way for it to be called in
> that case. It is an alternative to #ifs in the code.
Why even have the call though? I would wrap all of this in an #ifdef
and strip it out since you couldn't support SR-IOV if it isn't present
in the kernel anyway.
^ permalink raw reply
* Re: [PATCH] virtio_pci: support enabling VFs
From: Rustad, Mark D @ 2018-05-30 16:44 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Duyck, Alexander H, virtio-dev@lists.oasis-open.org,
linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, Wang, Zhihong,
Bjorn Helgaas
In-Reply-To: <20180530192010-mutt-send-email-mst@kernel.org>
[-- Attachment #1.1: Type: text/plain, Size: 996 bytes --]
On May 30, 2018, at 9:22 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> +static int virtio_pci_sriov_configure(struct pci_dev *pci_dev, int
>> num_vfs)
>> +{
>> + struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
>> + struct virtio_device *vdev = &vp_dev->vdev;
>> + int (*sriov_configure)(struct pci_dev *pci_dev, int num_vfs);
>> +
>> + if (!(vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_DRIVER_OK))
>> + return -EBUSY;
>> +
>> + if (!__virtio_test_bit(vdev, VIRTIO_F_SR_IOV))
>> + return -EINVAL;
>> +
>> + sriov_configure = pci_sriov_configure_simple;
>> + if (sriov_configure == NULL)
>> + return -ENOENT;
>
> BTW what is all this trickery in aid of?
When SR-IOV support is not compiled into the kernel,
pci_sriov_configure_simple is #defined as NULL. This allows it to compile
in that case, even though there is utterly no way for it to be called in
that case. It is an alternative to #ifs in the code.
--
Mark Rustad, Networking Division, Intel Corporation
[-- Attachment #1.2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [virtio-dev] [PATCH] virtio_pci: support enabling VFs
From: Duyck, Alexander H @ 2018-05-30 16:26 UTC (permalink / raw)
To: mst@redhat.com, alexander.duyck@gmail.com
Cc: virtio-dev@lists.oasis-open.org, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, Wang, Zhihong,
bhelgaas@google.com, Rustad, Mark D
In-Reply-To: <20180530192215-mutt-send-email-mst@kernel.org>
On Wed, 2018-05-30 at 19:22 +0300, Michael S. Tsirkin wrote:
> On Wed, May 30, 2018 at 09:10:57AM -0700, Alexander Duyck wrote:
> > On Wed, May 30, 2018 at 1:55 AM, Tiwei Bie <tiwei.bie@intel.com> wrote:
> > > There is a new feature bit allocated in virtio spec to
> > > support SR-IOV (Single Root I/O Virtualization):
> > >
> > > https://github.com/oasis-tcs/virtio-spec/issues/11
> > >
> > > This patch enables the support for this feature bit in
> > > virtio driver.
> > >
> > > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> >
> > So from a quick glance it looks like we are leaving SR-IOV enabled if
> > the driver is removed. Do we want to have that behavior or should we
> > be adding the code to disable SR-IOV and free the VFs on driver
> > removal?
>
> Could pci core handle it for us somehow?
Maybe, but it would require changes to the pci core to do it.
The problem is some drivers want to leave the VFs there since the PF
doesn't really do anything, or they have the option of essentially
putting the VFs into a standby state when the PF is gone.
My main concern is do we care if VFs are allocated and then somebody
removes the driver and binds a different driver to the interface? If
not then this code and be left as is, but I just wanted to be certain
since I know this isn't just enabling SR-IOV we are having to do a
number of other checks against the virtio device.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox