Re: [PATCH for 8.1 v2 5/6] vdpa: move CVQ isolation check to net_init_vhost_vdpa

[Eugenio Perez Martin]

On Fri, Mar 31, 2023 at 10:00 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2023/3/30 18:42, Eugenio Perez Martin 写道:
> > On Thu, Mar 30, 2023 at 8:23 AM Jason Wang <jasowang@redhat.com> wrote:
> >> On Thu, Mar 30, 2023 at 2:20 PM Jason Wang <jasowang@redhat.com> wrote:
> >>> On Fri, Mar 24, 2023 at 3:54 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> >>>> Evaluating it at start time instead of initialization time may make the
> >>>> guest capable of dynamically adding or removing migration blockers.
> >>>>
> >>>> Also, moving to initialization reduces the number of ioctls in the
> >>>> migration, reducing failure possibilities.
> >>>>
> >>>> As a drawback we need to check for CVQ isolation twice: one time with no
> >>>> MQ negotiated and another one acking it, as long as the device supports
> >>>> it.  This is because Vring ASID / group management is based on vq
> >>>> indexes, but we don't know the index of CVQ before negotiating MQ.
> >>> We need to fail if we see a device that can isolate cvq without MQ but
> >>> not with MQ.
> >>>
> >>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >>>> ---
> >>>> v2: Take out the reset of the device from vhost_vdpa_cvq_is_isolated
> >>>> ---
> >>>>   net/vhost-vdpa.c | 194 ++++++++++++++++++++++++++++++++++++-----------
> >>>>   1 file changed, 151 insertions(+), 43 deletions(-)
> >>>>
> >>>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> >>>> index 4397c0d4b3..db2c9afcb3 100644
> >>>> --- a/net/vhost-vdpa.c
> >>>> +++ b/net/vhost-vdpa.c
> >>>> @@ -43,6 +43,13 @@ typedef struct VhostVDPAState {
> >>>>
> >>>>       /* The device always have SVQ enabled */
> >>>>       bool always_svq;
> >>>> +
> >>>> +    /* The device can isolate CVQ in its own ASID if MQ is negotiated */
> >>>> +    bool cvq_isolated_mq;
> >>>> +
> >>>> +    /* The device can isolate CVQ in its own ASID if MQ is not 
> >>>> negotiated */
> >>>> +    bool cvq_isolated;
> >>> As stated above, if we need a device that cvq_isolated_mq^cvq_isolated
> >>> == true, we need to fail. This may reduce the complexity of the code?
> >>>
> >>> Thanks
> >> Since we are the mediation layer, Qemu can alway choose to negotiate
> >> MQ regardless whether or not it is supported by the guest. In this
> >> way, we can have a stable virtqueue index for cvq.
> >>
> > I think it is a great idea and it simplifies this patch somehow.
> > However, we need something like the queue mapping [1] to do so :).
> >
> > To double confirm:
> > * If the device supports MQ, only probe MQ. If not, only probe !MQ.
> > * Only store cvq_isolated in VhostVDPAState.
> >
> > Now, if the device does not negotiate MQ but the device supports MQ:
>
>
> I'm not sure I understand here, if device supports MQ it should accepts
> MQ or we can fail the initialization here.
>

My fault, I wanted to say "if the device offers MQ but the driver does
not acks it".

>
> > * All the requests to queue 3 must be redirected to the last queue in
> > the device. That includes set_vq_address, notifiers regions, etc.
>
>
> This also means we will only mediate the case:
>
> 1) Qemu emulated virtio-net has 1 queue but device support multiple queue
>
> but not
>
> 2) Qemu emulated virtio-net has M queue but device support N queue (N>M)
>

Right.

>
> >
> > I'm totally ok to go this route but it's not immediate.
>
>
> Yes but I mean, we can start from failing the device if
> cvq_isolated_mq^cvq_isolated == true
>

So probe the two cases but set VhostVDPAState->cvq_isolated =
cvq_isolated && cvq_mq_isolated then? No map involved that way, and
all parents should behave that way.

> (or I wonder if we can meet this condition for any existing parents).

I don't think so, but I think we need to probe the two anyway.
Otherwise we may change the dataplane asid too.

Thanks!