Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230605110644.151211-1-sgarzare@redhat.com> <20230605084104-mutt-send-email-mst@kernel.org>
 <24fjdwp44hovz3d3qkzftmvjie45er3g3boac7aezpvzbwvuol@lmo47ydvnqau>
 <20230605085840-mutt-send-email-mst@kernel.org> <gi2hngx3ndsgz5d2rpqjywdmou5vxhd7xgi5z2lbachr7yoos4@kpifz37oz2et>
 <20230605095404-mutt-send-email-mst@kernel.org> <32ejjuvhvcicv7wjuetkv34qtlpa657n4zlow4eq3fsi2twozk@iqnd2t5tw2an>
 <CACGkMEu3PqQ99UoKF5NHgVADD3q=BF6jhLiyumeT4S1QCqN1tw@mail.gmail.com>
 <20230606085643-mutt-send-email-mst@kernel.org> <CAGxU2F7fkgL-HpZdj=5ZEGNWcESCHQpgRAYQA3W2sPZaoEpNyQ@mail.gmail.com>
 <20230607054246-mutt-send-email-mst@kernel.org>
In-Reply-To: <20230607054246-mutt-send-email-mst@kernel.org>
From:   Jason Wang <jasowang@redhat.com>
Date:   Thu, 8 Jun 2023 08:42:15 +0800
Message-ID: <CACGkMEuUapKvUYiJiLwtsN+x941jafDKS9tuSkiNrvkrrSmQkg@mail.gmail.com>
Subject: Re: [PATCH] vhost-vdpa: filter VIRTIO_F_RING_PACKED feature
To:     "Michael S. Tsirkin" <mst@redhat.com>
Cc:     Stefano Garzarella <sgarzare@redhat.com>,
        Shannon Nelson <shannon.nelson@amd.com>,
        virtualization@lists.linux-foundation.org, netdev@vger.kernel.org,
        =?UTF-8?Q?Eugenio_P=C3=A9rez?= <eperezma@redhat.com>,
        Tiwei Bie <tiwei.bie@intel.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Jun 7, 2023 at 5:43=E2=80=AFPM Michael S. Tsirkin <mst@redhat.com> =
wrote:
>
> On Wed, Jun 07, 2023 at 10:39:15AM +0200, Stefano Garzarella wrote:
> > On Tue, Jun 6, 2023 at 2:58=E2=80=AFPM Michael S. Tsirkin <mst@redhat.c=
om> wrote:
> > >
> > > On Tue, Jun 06, 2023 at 09:29:22AM +0800, Jason Wang wrote:
> > > > On Mon, Jun 5, 2023 at 10:58=E2=80=AFPM Stefano Garzarella <sgarzar=
e@redhat.com> wrote:
> > > > >
> > > > > On Mon, Jun 05, 2023 at 09:54:57AM -0400, Michael S. Tsirkin wrot=
e:
> > > > > >On Mon, Jun 05, 2023 at 03:30:35PM +0200, Stefano Garzarella wro=
te:
> > > > > >> On Mon, Jun 05, 2023 at 09:00:25AM -0400, Michael S. Tsirkin w=
rote:
> > > > > >> > On Mon, Jun 05, 2023 at 02:54:20PM +0200, Stefano Garzarella=
 wrote:
> > > > > >> > > On Mon, Jun 05, 2023 at 08:41:54AM -0400, Michael S. Tsirk=
in wrote:
> > > > > >> > > > On Mon, Jun 05, 2023 at 01:06:44PM +0200, Stefano Garzar=
ella wrote:
> > > > > >> > > > > vhost-vdpa IOCTLs (eg. VHOST_GET_VRING_BASE, VHOST_SET=
_VRING_BASE)
> > > > > >> > > > > don't support packed virtqueue well yet, so let's filt=
er the
> > > > > >> > > > > VIRTIO_F_RING_PACKED feature for now in vhost_vdpa_get=
_features().
> > > > > >> > > > >
> > > > > >> > > > > This way, even if the device supports it, we don't ris=
k it being
> > > > > >> > > > > negotiated, then the VMM is unable to set the vring st=
ate properly.
> > > > > >> > > > >
> > > > > >> > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based back=
end")
> > > > > >> > > > > Cc: stable@vger.kernel.org
> > > > > >> > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com=
>
> > > > > >> > > > > ---
> > > > > >> > > > >
> > > > > >> > > > > Notes:
> > > > > >> > > > >     This patch should be applied before the "[PATCH v2=
 0/3] vhost_vdpa:
> > > > > >> > > > >     better PACKED support" series [1] and backported i=
n stable branches.
> > > > > >> > > > >
> > > > > >> > > > >     We can revert it when we are sure that everything =
is working with
> > > > > >> > > > >     packed virtqueues.
> > > > > >> > > > >
> > > > > >> > > > >     Thanks,
> > > > > >> > > > >     Stefano
> > > > > >> > > > >
> > > > > >> > > > >     [1] https://lore.kernel.org/virtualization/2023042=
4225031.18947-1-shannon.nelson@amd.com/
> > > > > >> > > >
> > > > > >> > > > I'm a bit lost here. So why am I merging "better PACKED =
support" then?
> > > > > >> > >
> > > > > >> > > To really support packed virtqueue with vhost-vdpa, at tha=
t point we would
> > > > > >> > > also have to revert this patch.
> > > > > >> > >
> > > > > >> > > I wasn't sure if you wanted to queue the series for this m=
erge window.
> > > > > >> > > In that case do you think it is better to send this patch =
only for stable
> > > > > >> > > branches?
> > > > > >> > > > Does this patch make them a NOP?
> > > > > >> > >
> > > > > >> > > Yep, after applying the "better PACKED support" series and=
 being
> > > > > >> > > sure that
> > > > > >> > > the IOCTLs of vhost-vdpa support packed virtqueue, we shou=
ld revert this
> > > > > >> > > patch.
> > > > > >> > >
> > > > > >> > > Let me know if you prefer a different approach.
> > > > > >> > >
> > > > > >> > > I'm concerned that QEMU uses vhost-vdpa IOCTLs thinking th=
at the kernel
> > > > > >> > > interprets them the right way, when it does not.
> > > > > >> > >
> > > > > >> > > Thanks,
> > > > > >> > > Stefano
> > > > > >> > >
> > > > > >> >
> > > > > >> > If this fixes a bug can you add Fixes tags to each of them? =
Then it's ok
> > > > > >> > to merge in this window. Probably easier than the elaborate
> > > > > >> > mask/unmask dance.
> > > > > >>
> > > > > >> CCing Shannon (the original author of the "better PACKED suppo=
rt"
> > > > > >> series).
> > > > > >>
> > > > > >> IIUC Shannon is going to send a v3 of that series to fix the
> > > > > >> documentation, so Shannon can you also add the Fixes tags?
> > > > > >>
> > > > > >> Thanks,
> > > > > >> Stefano
> > > > > >
> > > > > >Well this is in my tree already. Just reply with
> > > > > >Fixes: <>
> > > > > >to each and I will add these tags.
> > > > >
> > > > > I tried, but it is not easy since we added the support for packed
> > > > > virtqueue in vdpa and vhost incrementally.
> > > > >
> > > > > Initially I was thinking of adding the same tag used here:
> > > > >
> > > > > Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
> > > > >
> > > > > Then I discovered that vq_state wasn't there, so I was thinking o=
f
> > > > >
> > > > > Fixes: 530a5678bc00 ("vdpa: support packed virtqueue for set/get_=
vq_state()")
> > > > >
> > > > > So we would have to backport quite a few patches into the stable =
branches.
> > > > > I don't know if it's worth it...
> > > > >
> > > > > I still think it is better to disable packed in the stable branch=
es,
> > > > > otherwise I have to make a list of all the patches we need.
> > > > >
> > > > > Any other ideas?
> > > >
> > > > AFAIK, except for vp_vdpa, pds seems to be the first parent that
> > > > supports packed virtqueue. Users should not notice anything wrong i=
f
> > > > they don't use packed virtqueue. And the problem of vp_vdpa + packe=
d
> > > > virtqueue came since the day0 of vp_vdpa. It seems fine to do nothi=
ng
> > > > I guess.
> > > >
> > > > Thanks
> > >
> > >
> > > I have a question though, what if down the road there
> > > is a new feature that needs more changes? It will be
> > > broken too just like PACKED no?
> > > Shouldn't vdpa have an allowlist of features it knows how
> > > to support?
> >
> > It looks like we had it, but we took it out (by the way, we were
> > enabling packed even though we didn't support it):
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/comm=
it/?id=3D6234f80574d7569444d8718355fa2838e92b158b
> >
> > The only problem I see is that for each new feature we have to modify
> > the kernel.
> > Could we have new features that don't require handling by vhost-vdpa?
> >
> > Thanks,
> > Stefano
>
> Jason what do you say to reverting this?

I may miss something but I don't see any problem with vDPA core.

It's the duty of the parents to advertise the features it has. For example,

1) If some kernel version that is packed is not supported via
set_vq_state, parents should not advertise PACKED features in this
case.
2) If the kernel has support packed set_vq_state(), but it's emulated
cvq doesn't support, parents should not advertise PACKED as well

If a parent violates the above 2, it looks like a bug of the parents.

Thanks

>
> --
> MST
>