Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Message-ID: <de5e15c0-0dcb-4cf1-bd4b-bd48c3c59c80@oracle.com>
Date:   Mon, 30 Oct 2023 10:05:39 -0400
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC] vdpa/mlx5: preserve CVQ vringh index
To:     Jason Wang <jasowang@redhat.com>
Cc:     virtualization@lists.linux-foundation.org,
        linux-kernel@vger.kernel.org,
        "Michael S. Tsirkin" <mst@redhat.com>, Eli Cohen <elic@nvidia.com>,
        Si-Wei Liu <si-wei.liu@oracle.com>,
        Dragos Tatulea <dtatulea@nvidia.com>,
        Xuan Zhuo <xuanzhuo@linux.alibaba.com>
References: <1698350834-415881-1-git-send-email-steven.sistare@oracle.com>
 <CACGkMEuO6o3Ys9NcHJpa9w5EiS-ugsiaBbBKEHKHpPSrKBKJow@mail.gmail.com>
Content-Language: en-US
From:   Steven Sistare <steven.sistare@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <CACGkMEuO6o3Ys9NcHJpa9w5EiS-ugsiaBbBKEHKHpPSrKBKJow@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Vld2SnZoelBlMUNPQ1hqNVpERXVXc1RiL0Jidkk1SThoeW5na0tSY1FseWJ6?=
 =?utf-8?B?WnRiMWUwWng3NGlES0Y0SEFUcjFEd1RMR21xVFczY0xnOGdBY1EwQUVqbmJI?=
 =?utf-8?B?UHcva3NOeTlEaFlsQlJuMGQvdVpWbVo4cWJCR1paMnhGNXlJRnJRNHRvblJm?=
 =?utf-8?B?TGJ5eGZ4MmlXZTA4ZkFLRDVTMlN1dGlwSU5GVFdvQlhTcVY3Sll2clozZXQ5?=
 =?utf-8?B?K0hBUnMxTnViZHd4Ky81d0J6ZVIzcWttVTVSdTNrd1RCRGhZK2Y4Zmk3aEV1?=
 =?utf-8?B?YU0vSndMNDUyby9KYmpKWDFKT3kyellpNVJweVBRcWlMb0V1NWQ0RDV0TVlT?=
 =?utf-8?B?aXhMV0p2RWlQdU81SE9rb3l4aGZIdkdZaFpPMndETlVDUWdTZ3pBK0NvaDZX?=
 =?utf-8?B?c2FRcU1lQThKOTNvSFd4MXZsYzZqbGpnWEZuek96bXZNZVFXYXk0bW91OEZs?=
 =?utf-8?B?TEhIU3VVR3RIM2NCQWszaU5IZU9jWFFTTU95MHZ1cmJVMUowQmRoeWJIZk5V?=
 =?utf-8?B?d09JdGNaODFvZU45TEd2V2cwUlVtVFZGWFpDek9KdnVGdkRzRnFMNVROOUFN?=
 =?utf-8?B?aUxiVXZ1RDR0QXlENC9aaXR0aVdyZ1BMdFZGSVQ4S3pQWmovWFBKdnRtVXB0?=
 =?utf-8?B?M1Y0OFBFNnY2d1dOUktPaHZ0MEhwSVlyU3E1Y3lMaGRxeFFYSWtYUmFmenV4?=
 =?utf-8?B?RnAzeTZzQTk0QkIyMEdzeFVqMCt5Y25GZk95WTk3bVovQTM4NU1teDRCSlJZ?=
 =?utf-8?B?ZzhnZUVTTE5qVkpHOTI1ZEhodjNuejkxaThWNStXQVV0c0tPUXpBTGgvL2xt?=
 =?utf-8?B?MXlJVUtkWVA4UkJzalcvOFN1Qnd0ZWRYSEFlSUZPdkd5czJjc2lDQjBDclRr?=
 =?utf-8?B?WWljTjVPK09iUFB6MWZMV2xwUjZ1dC8zbEltSjAvOHBmcFJud0FuQVBrVC94?=
 =?utf-8?B?N3lTdDdEZEd2eE0rb2NWZXdCeDcxckhTWEszNUlHOWM1cmpZQmhOMC9zb3N4?=
 =?utf-8?B?bVlONDdWakxWcHVaUXJOelh6SGlLRzcyMmpObCtuaFNNRFNuUDcvOVU0Q25r?=
 =?utf-8?B?d1QveVVLc2VsWElsMnpIVGY5elJiNVMvSXBLVDZWWWFCTkRscnMyY2pjTS80?=
 =?utf-8?B?ZEJKODFrSDJBWUFkTEhZbUV5Uys3ZUhDY1lCeEpKTERETjV3L0lQUDl1aTBI?=
 =?utf-8?B?U2haS0I3bjU3a3dHVkFoRHpRMlRFNTdnMXVHMWhnSjV4NHZEL3c0ZGxzQjVE?=
 =?utf-8?B?RjIveXBOcy9IVHFQQ1VleEwrTklkVGdRdTRjK0cycUwyOTR3cnVCeXpDOVA1?=
 =?utf-8?B?QWNKOTVEektlR1BLQ0lKU3M1V0RONC80UVVGVytrUzd0Y2I1KzQwSDllTUQ1?=
 =?utf-8?B?UFNiSGFjVC8xQUoxL3BJUHI5L1FQUVZVZkQ2WjJ2OWxYYW9aMlNKcnhRWnk5?=
 =?utf-8?B?TDBLWWtaY01keXFYNDBTZVVuY1hQNURxbDFqN1lvU3pYRlgrTDdjSFF2bDha?=
 =?utf-8?B?Z1l5QjByWmFDQWZVcWxCcllRdWlPcTFvaUU1WDJETklHc1Z5bDRxc0lzLzVu?=
 =?utf-8?B?aTU2d0xJOTNkNFNVVmR4cWJwQnVhVEF5QTN4bTAzNGZtRGVnc25TNGJYditU?=
 =?utf-8?B?Y1BEbXRSbkgySUQ5SVFPVi9wM2haR0sxWTNWSU1yVVlCQzlPeElxMkJZQWVl?=
 =?utf-8?B?NC8zSmppSTJNQ0tQTzNvRXlKeERJM3hlOUdmVXFKQlZxSG9SUFZ3Vk5oSDZ1?=
 =?utf-8?B?RGlVV2VVa3BQSXhYWkVTNWQ1NERqVFF3QkdrQWF5eG1rb2RIdVNTVmNaQ2Jq?=
 =?utf-8?B?aTdxMkMwTXkyeXBWVkRRY2lQWHg0dVNFVHRoQzZIb1E1ZFhzQ3VpWFhVb3l6?=
 =?utf-8?B?K2pvOUFhMmpMZTVza3p0OCtSdm1SdjVsb1JnaWtDcHFHaHJKdlArdE1KSnc4?=
 =?utf-8?B?Z2MwSHVZdURpY3Uxenp0b3JhT3g2WUxzeXgvbnFHaUZ6UldnZjdNcVZxV0M5?=
 =?utf-8?B?UGtJRzFBZ2hlYTR4NEVUdkNBci96YWZBOWZqWkh6SFBqcWNnV3FHVVdYYitR?=
 =?utf-8?B?TVJTb1crZmlIZnRqTnNXVnFlRFdsbm4wMVZEeUI3RnI5N1FBV3BNS3BEakhw?=
 =?utf-8?B?QzBTMlNwalp0bVZpSWVHNzBFT1NkV2E0aGlyQXhrWWdSTDBYRkxoajk5MUpO?=
 =?utf-8?B?WWc9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?utf-8?B?Z2VDd1lQUGJndjk0RE5sbWw1SlFFaGJlb09hV2kwK01kNk9PNmlBejRVUGhT?=
 =?utf-8?B?QThGNmEyT0ZnMkVwZGY4alFHMlZHSUZzY1BlWXhFMHJ3NUFNWjN5Tk9UVkdt?=
 =?utf-8?B?eEw4M1dydWZGTEdpUjNEL0V1YVBFT2tJVFJhdUp0TUt4SlZ0SzV1NUdQTGEr?=
 =?utf-8?B?VHQrYVJXMTBLRjJnbDA1OFZMVmhjUjl1UTE4dFRNazZsM05vVUJPeGZMZnhu?=
 =?utf-8?B?YnJjT1dYSUl5YXBVSGo3WFh3aGdrMHNMR0luR3Y2WGNGSXl0dG9VQ1F3S0hH?=
 =?utf-8?B?WUQrR2t1TWtMYnhhak9CR1dYTk43SUxZUWQ2NWw0ay9hT2tHOWd5SGZXTkF4?=
 =?utf-8?B?ZGdmOC9QTnVwYk9rSTlhbDRtT0RkNER2Q3ZvUmg0L1NIQTRBdzB1dEYzRWd2?=
 =?utf-8?B?bEIzSDhMYnpQdHppZ1FOaEZGZ1hEOHRNWlNvdkprSysxa1Jqa2ViZWZNQUo5?=
 =?utf-8?B?UHYzeTVRUVl2VGVXbEhuR1A4clNVKzFMZGpSeCtCR2pJcTExZFBpcHlGWjkv?=
 =?utf-8?B?eFU0YjFxQndyTDBJZGpscWJvYldleVNEcHZHY3V0dlJrbUJMbm9reUpyVk1F?=
 =?utf-8?B?TDRRVStCQnhCV2pzbzRrcjV1TVovSFd2Q0hPQ2lMQ29VTTZVbVNoS1EzOEdj?=
 =?utf-8?B?ZVY4YVlDeVdrdndnM2dxYit6MFV0T2lVZTdTVUV5WXQ3MnI1ZVR3ZnRrRXZw?=
 =?utf-8?B?c0pUOWp2UzBQUEdZZEFnU3J3eUwydDRLYnJoQ3J2WndLYXJLL2xTR3h4VVBa?=
 =?utf-8?B?cXhldENrWDRnREtVTlRBSCtTUVByR2lYRHJCbTJLdHlsbTNQUER3cStoekJt?=
 =?utf-8?B?bTdPT2VCdXpOd3RBeEtMdjRIaWJ4ektNZXdqZ05QTkdFV0k1UkdhN2JQNVBy?=
 =?utf-8?B?RkZBSHJtRHlOUnBZU1pON3AxbE0zOHJlalViWnpJTGpmMW5adktzdmpIbVM1?=
 =?utf-8?B?eVFhdHZTZFVmM1RjRUN4NWFrWmN3QlpvUmRFeEtXNjNoWkRoQnN2WjJGUnFM?=
 =?utf-8?B?RE1VT2pVcVJCbXNQSmUxRnpjZ2hwRjMxb3ZzQy9LaEhCM1dHb281emtQVHJF?=
 =?utf-8?B?MUtWOW1lTVVEZVN0ZGtlMTkwUTZDWWZjc0F1NzFoUUM1OTJDb2ZqRis0RHFr?=
 =?utf-8?B?V3NSeEhnakZDUk93ZzIzeTlRYlNaWENhVHRuakV4LzQwcld0M2h1UnAweTNN?=
 =?utf-8?B?Si9zbWp0YTdGMkh0QmZOM29zYmY2b2hmR1g1VkZncElYNjRxWkYvR2JXTzUw?=
 =?utf-8?Q?COD9nUZ4uZrOjGR?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 64f313f4-a4d7-4042-700d-08dbd9514dd4
X-MS-Exchange-CrossTenant-AuthSource: SA2PR10MB4684.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Oct 2023 14:05:42.6164
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: klSR3k7P/BY9xhTQZymYv3nrm8zdP6oAZTfYEtFthz91oZpiUx6q1OkSnvn/C7YxSGXgzqtI6JZWzPaAyrOGUNmW2IWm86F7OSAMrsYfQ54=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR10MB7013
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.987,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-10-30_10,2023-10-27_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 adultscore=0 mlxscore=0
 suspectscore=0 mlxlogscore=999 bulkscore=0 phishscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2310240000
 definitions=main-2310300107
X-Proofpoint-GUID: 8j_Nm4Gy1uRHeGRckaZai463SC-wB-1W
X-Proofpoint-ORIG-GUID: 8j_Nm4Gy1uRHeGRckaZai463SC-wB-1W
X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (fry.vger.email [0.0.0.0]); Mon, 30 Oct 2023 07:06:30 -0700 (PDT)

On 10/27/2023 2:31 AM, Jason Wang wrote:
> On Fri, Oct 27, 2023 at 4:07 AM Steve Sistare <steven.sistare@oracle.com> wrote:
>>
>> mlx5_vdpa does not preserve userland's view of vring base for the control
>> queue in the following sequence:
>>
>> ioctl VHOST_SET_VRING_BASE
>> ioctl VHOST_VDPA_SET_STATUS VIRTIO_CONFIG_S_DRIVER_OK
>>   mlx5_vdpa_set_status()
>>     setup_cvq_vring()
>>       vringh_init_iotlb()
>>         vringh_init_kern()
>>           vrh->last_avail_idx = 0;
>> ioctl VHOST_GET_VRING_BASE
>>
>> To fix, restore the value of cvq->vring.last_avail_idx after calling
>> vringh_init_iotlb.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>>  drivers/vdpa/mlx5/net/mlx5_vnet.c |  7 ++++++-
>>  drivers/vhost/vringh.c            | 30 ++++++++++++++++++++++++++++++
>>  include/linux/vringh.h            |  2 ++
>>  3 files changed, 38 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> index 946488b8989f..f64758143115 100644
>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> @@ -2795,13 +2795,18 @@ static int setup_cvq_vring(struct mlx5_vdpa_dev *mvdev)
>>         struct mlx5_control_vq *cvq = &mvdev->cvq;
>>         int err = 0;
>>
>> -       if (mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ))
>> +       if (mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)) {
>> +               u16 last_avail_idx = cvq->vring.last_avail_idx;
>> +
>>                 err = vringh_init_iotlb(&cvq->vring, mvdev->actual_features,
>>                                         MLX5_CVQ_MAX_ENT, false,
>>                                         (struct vring_desc *)(uintptr_t)cvq->desc_addr,
>>                                         (struct vring_avail *)(uintptr_t)cvq->driver_addr,
>>                                         (struct vring_used *)(uintptr_t)cvq->device_addr);
>>
>> +               if (!err)
>> +                       vringh_set_base_iotlb(&cvq->vring, last_avail_idx);
> 
> Btw, vringh_set_base_iotlb() deserves an independent patch and it
> seems it is not specific to IOTLB, 

Agreed on both.  I initially submitted the smallest change needed to show the problem.

so we probably need an indirection
> to have vringh_set_base() first.

Not sure what you mean.  I have defined:

    static inline int __vringh_set_base() ...

    int vringh_set_base_iotlb()
        return __vringh_set_base(vrh, idx, putu16_iotlb);

to which I would add:

    int vringh_set_base_user()
        return __vringh_set_base(vrh, idx, putu16_user);

    int vringh_set_base_kern()
        return __vringh_set_base(vrh, idx, putu16_kern;

all in the same patch.

The call site in mlx5_vnet.c would be a 2nd patch.

- Steve

> Or I wonder if it's better to just introduce a new parameter to
> vringh_init_iotlb()...

I considered that, but IMO the parameter list there is already large, and it would
be strange to add a parameter for the initial value of avail, but not for used, and
no one needs the latter.

- Steve

>> +       }
>>         return err;
>>  }
>>
>> diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
>> index 7b8fd977f71c..799762c83007 100644
>> --- a/drivers/vhost/vringh.c
>> +++ b/drivers/vhost/vringh.c
>> @@ -595,6 +595,24 @@ static inline void __vringh_notify_disable(struct vringh *vrh,
>>         }
>>  }
>>
>> +static inline int __vringh_set_base(struct vringh *vrh, u16 idx,
>> +                           int (*putu16)(const struct vringh *vrh,
>> +                               __virtio16 *p, u16 val))
>> +{
>> +    int ret;
>> +
>> +    ret = putu16(vrh, &vrh->vring.avail->idx, idx);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = putu16(vrh, &vrh->vring.used->idx, idx);
>> +    if (ret)
>> +        return ret;
>> +
>> +    vrh->last_avail_idx = vrh->last_used_idx = idx;
>> +    return 0;
>> +}
>> +
>>  /* Userspace access helpers: in this case, addresses are really userspace. */
>>  static inline int getu16_user(const struct vringh *vrh, u16 *val, const __virtio16 *p)
>>  {
>> @@ -1456,6 +1474,18 @@ void vringh_set_iotlb(struct vringh *vrh, struct vhost_iotlb *iotlb,
>>  }
>>  EXPORT_SYMBOL(vringh_set_iotlb);
>>
>> +/**
>> + * vringh_set_base_iotlb - set avail_idx and used_idx
>> + * @vrh: the vring
>> + * @idx: the value to set
>> + */
>> +int vringh_set_base_iotlb(struct vringh *vrh, u16 idx)
>> +{
>> +    return __vringh_set_base(vrh, idx, putu16_iotlb);
>> +}
>> +EXPORT_SYMBOL(vringh_set_base_iotlb);
>> +
>> +
>>  /**
>>   * vringh_getdesc_iotlb - get next available descriptor from ring with
>>   * IOTLB.
>> diff --git a/include/linux/vringh.h b/include/linux/vringh.h
>> index c3a8117dabe8..e9b8af4e6a5e 100644
>> --- a/include/linux/vringh.h
>> +++ b/include/linux/vringh.h
>> @@ -306,6 +306,8 @@ int vringh_init_iotlb_va(struct vringh *vrh, u64 features,
>>                          struct vring_avail *avail,
>>                          struct vring_used *used);
>>
>> +int vringh_set_base_iotlb(struct vringh *vrh, u16 idx);
>> +
>>  int vringh_getdesc_iotlb(struct vringh *vrh,
>>                          struct vringh_kiov *riov,
>>                          struct vringh_kiov *wiov,
>> --
>> 2.39.3
>>
>