Re: [PATCH v4 12/15] vdpa: block migration if device has unsupported features

[Jason Wang <jasowang@redhat.com>]

在 2023/3/2 03:32, Eugenio Perez Martin 写道:
> On Mon, Feb 27, 2023 at 9:20 AM Jason Wang <jasowang@redhat.com> wrote:
>> On Mon, Feb 27, 2023 at 4:15 PM Jason Wang <jasowang@redhat.com> wrote:
>>>
>>> 在 2023/2/24 23:54, Eugenio Pérez 写道:
>>>> A vdpa net device must initialize with SVQ in order to be migratable at
>>>> this moment, and initialization code verifies some conditions.  If the
>>>> device is not initialized with the x-svq parameter, it will not expose
>>>> _F_LOG so the vhost subsystem will block VM migration from its
>>>> initialization.
>>>>
>>>> Next patches change this, so we need to verify migration conditions
>>>> differently.
>>>>
>>>> QEMU only supports a subset of net features in SVQ, and it cannot
>>>> migrate state that cannot track or restore in the destination.  Add a
>>>> migration blocker if the device offer an unsupported feature.
>>>>
>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>> ---
>>>> v3: add mirgation blocker properly so vhost_dev can handle it.
>>>> ---
>>>>    net/vhost-vdpa.c | 12 ++++++++----
>>>>    1 file changed, 8 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>>>> index 4f983df000..094dc1c2d0 100644
>>>> --- a/net/vhost-vdpa.c
>>>> +++ b/net/vhost-vdpa.c
>>>> @@ -795,7 +795,8 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
>>>>                                           int nvqs,
>>>>                                           bool is_datapath,
>>>>                                           bool svq,
>>>> -                                       struct vhost_vdpa_iova_range iova_range)
>>>> +                                       struct vhost_vdpa_iova_range iova_range,
>>>> +                                       uint64_t features)
>>>>    {
>>>>        NetClientState *nc = NULL;
>>>>        VhostVDPAState *s;
>>>> @@ -818,7 +819,10 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
>>>>        s->vhost_vdpa.shadow_vqs_enabled = svq;
>>>>        s->vhost_vdpa.iova_range = iova_range;
>>>>        s->vhost_vdpa.shadow_data = svq;
>>>> -    if (!is_datapath) {
>>>> +    if (queue_pair_index == 0) {
>>>> +        vhost_vdpa_net_valid_svq_features(features,
>>>> +                                          &s->vhost_vdpa.migration_blocker);
>>>
>>> Since we do validation at initialization, is this necessary to valid
>>> once again in other places?
>> Ok, after reading patch 13, I think the question is:
>>
>> The validation seems to be independent to net, can we valid it once
>> during vhost_vdpa_init()?
>>
> vhost_vdpa_net_valid_svq_features also checks for net features. In
> particular, all the non transport features must be in
> vdpa_svq_device_features.
>
> This is how we protect that the device / guest will never negotiate
> things like VLAN filtering support, as SVQ still does not know how to
> restore at the destination.
>
> In the VLAN filtering case CVQ is needed to restore VLAN, so it is
> covered by patch 11/15. But other future features may need support for
> restoring it in the destination.


I wonder how hard to have a general validation code let net specific 
code to advertise a blacklist to avoid code duplication.

Thanks


>
> Thanks!
>
>> Thanks
>>
>>> Thanks
>>>
>>>
>>>> +    } else if (!is_datapath) {
>>>>            s->cvq_cmd_out_buffer = qemu_memalign(qemu_real_host_page_size(),
>>>>                                                vhost_vdpa_net_cvq_cmd_page_len());
>>>>            memset(s->cvq_cmd_out_buffer, 0, vhost_vdpa_net_cvq_cmd_page_len());
>>>> @@ -956,7 +960,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
>>>>        for (i = 0; i < queue_pairs; i++) {
>>>>            ncs[i] = net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name,
>>>>                                         vdpa_device_fd, i, 2, true, opts->x_svq,
>>>> -                                     iova_range);
>>>> +                                     iova_range, features);
>>>>            if (!ncs[i])
>>>>                goto err;
>>>>        }
>>>> @@ -964,7 +968,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
>>>>        if (has_cvq) {
>>>>            nc = net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name,
>>>>                                     vdpa_device_fd, i, 1, false,
>>>> -                                 opts->x_svq, iova_range);
>>>> +                                 opts->x_svq, iova_range, features);
>>>>            if (!nc)
>>>>                goto err;
>>>>        }