Message ID | 20221107203431.368306-1-eric.auger@redhat.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp2287130wru; Mon, 7 Nov 2022 12:50:32 -0800 (PST) X-Google-Smtp-Source: AMsMyM41mDM48R8DiGZJ2VKR6L5fvgBsY8z/C9n3HkJ7zsWfcMy257vgO35PZZiOTdT2k7ttIdMe X-Received: by 2002:a17:906:3fd2:b0:78d:b793:5ef9 with SMTP id k18-20020a1709063fd200b0078db7935ef9mr48302522ejj.496.1667854232292; Mon, 07 Nov 2022 12:50:32 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1667854232; cv=none; d=google.com; s=arc-20160816; b=XSmA3/NrJGgsHljF5fKKEVwsaFQ3tD1qGqBZU8Vt6JAvUqA+OTwaHAdUPn1+ohvIgg ggUelI/fafIfT2/aV0fLb7VyD/+Hz4W0UKPRkNbqNuwLQs3gFBiyOhcB75+ntuIaGs8h yv6+pRc/SuXyyHSeDKJSxdpHqImPTX2Bh8HrVCruw+njlflERbi7Thby2kLJ1CAA1sn9 GOEO/vFtg1ijueFlxuuYLmRV9Q/RwaSLY2Q3rFpplOUjyHVPjQvsiAbd4zHIBXQ2p6kV 5UIYkxiA3tAWM/txKBAyqKq8PQKIpAI89STAAK2NxClhDBxI5bfOQyh2fySlMaPaFGRm bB4w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=drX9+vl44zYzpBoXJCuJVBuSeyp+bHZXGq/s8MCBeRs=; b=z2Tl/h+VIFDEI2cfFNac89mUVc1XLu6ffmsyaCNpG+kFCBsLUVcLRuS43Lp+uckItS oekqMZicrQNAbHnRqlxCcsMWnFrbvhWMKhT/0kWQc8eFf2om/jg7fG1v0AV0zZYZYIcp 40UMRGYjJyJmVwBgQca5cBsUcp/Mmue3Gwe1YX7PqYb2Gazq8RQzvgbEMu8lbGx2BeVT KNy57pN/h6easJFqnAExpWG5T/irPtHx7ryzdeC64DGXMyvwyyIIEvTh+cV66kNPBGph wRtP5OuIIHUXYCXhDffB0Oz0cWIqWDkMRDEAXVc61J5vC80ntXomUi7lIx9TN1QY1C8t STCg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b="CRq/YTid"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id mp3-20020a1709071b0300b007417c6edb0asi10739260ejc.402.2022.11.07.12.50.08; Mon, 07 Nov 2022 12:50:32 -0800 (PST) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b="CRq/YTid"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232060AbiKGUgp (ORCPT <rfc822;hjfbswb@gmail.com> + 99 others); Mon, 7 Nov 2022 15:36:45 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:34066 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232411AbiKGUgo (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 7 Nov 2022 15:36:44 -0500 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 00C9C286C6 for <linux-kernel@vger.kernel.org>; Mon, 7 Nov 2022 12:35:48 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1667853348; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding; bh=drX9+vl44zYzpBoXJCuJVBuSeyp+bHZXGq/s8MCBeRs=; b=CRq/YTidVMc2Pzp5vSE1TCpnxXFSIZZ2B2vOjG/UfoLdSCRA4sf8Jr/rdHDQkVthUoU8cT HF7pZA1X+75ZvVgTtA4hdslQL+Z/ClRJZRldXhEmHBDTubk9oMIyB4ZHbV4mzVpEAc79/z 3iXPXgpKfoONqfJ7GRWb0So3ZsdKc/U= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-149-rRg4vbBDOryX_Fp3et26Yg-1; Mon, 07 Nov 2022 15:35:43 -0500 X-MC-Unique: rRg4vbBDOryX_Fp3et26Yg-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 9C26D806004; Mon, 7 Nov 2022 20:35:37 +0000 (UTC) Received: from laptop.redhat.com (unknown [10.39.192.106]) by smtp.corp.redhat.com (Postfix) with ESMTP id D8B9AC159CD; Mon, 7 Nov 2022 20:35:25 +0000 (UTC) From: Eric Auger <eric.auger@redhat.com> To: eric.auger.pro@gmail.com, eric.auger@redhat.com, mst@redhat.com, jasowang@redhat.com, kvm@vger.kernel.org, virtualization@lists.linux-foundation.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org Cc: peterx@redhat.com Subject: [RFC] vhost: Clear the pending messages on vhost_init_device_iotlb() Date: Mon, 7 Nov 2022 21:34:31 +0100 Message-Id: <20221107203431.368306-1-eric.auger@redhat.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 3.1 on 10.11.54.8 X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1748871919710895022?= X-GMAIL-MSGID: =?utf-8?q?1748871919710895022?= |
Series |
[RFC] vhost: Clear the pending messages on vhost_init_device_iotlb()
|
|
Commit Message
Eric Auger
Nov. 7, 2022, 8:34 p.m. UTC
When the vhost iotlb is used along with a guest virtual iommu
and the guest gets rebooted, some MISS messages may have been
recorded just before the reboot and spuriously executed by
the virtual iommu after the reboot. Despite the device iotlb gets
re-initialized, the messages are not cleared. Fix that by calling
vhost_clear_msg() at the end of vhost_init_device_iotlb().
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
drivers/vhost/vhost.c | 1 +
1 file changed, 1 insertion(+)
Comments
On Mon, Nov 07, 2022 at 09:34:31PM +0100, Eric Auger wrote: > When the vhost iotlb is used along with a guest virtual iommu > and the guest gets rebooted, some MISS messages may have been > recorded just before the reboot and spuriously executed by > the virtual iommu after the reboot. Despite the device iotlb gets > re-initialized, the messages are not cleared. Fix that by calling > vhost_clear_msg() at the end of vhost_init_device_iotlb(). > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > --- > drivers/vhost/vhost.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index 40097826cff0..422a1fdee0ca 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -1751,6 +1751,7 @@ int vhost_init_device_iotlb(struct vhost_dev *d, bool enabled) > } > > vhost_iotlb_free(oiotlb); > + vhost_clear_msg(d); > > return 0; > } Hmm. Can't messages meanwhile get processes and affect the new iotlb? > -- > 2.37.3
Hi Michael, On 11/7/22 21:42, Michael S. Tsirkin wrote: > On Mon, Nov 07, 2022 at 09:34:31PM +0100, Eric Auger wrote: >> When the vhost iotlb is used along with a guest virtual iommu >> and the guest gets rebooted, some MISS messages may have been >> recorded just before the reboot and spuriously executed by >> the virtual iommu after the reboot. Despite the device iotlb gets >> re-initialized, the messages are not cleared. Fix that by calling >> vhost_clear_msg() at the end of vhost_init_device_iotlb(). >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> --- >> drivers/vhost/vhost.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >> index 40097826cff0..422a1fdee0ca 100644 >> --- a/drivers/vhost/vhost.c >> +++ b/drivers/vhost/vhost.c >> @@ -1751,6 +1751,7 @@ int vhost_init_device_iotlb(struct vhost_dev *d, bool enabled) >> } >> >> vhost_iotlb_free(oiotlb); >> + vhost_clear_msg(d); >> >> return 0; >> } > Hmm. Can't messages meanwhile get processes and affect the > new iotlb? Isn't the msg processing stopped at the moment this function is called (VHOST_SET_FEATURES)? Thanks Eric > > >> -- >> 2.37.3
On Mon, Nov 07, 2022 at 10:10:06PM +0100, Eric Auger wrote: > Hi Michael, > On 11/7/22 21:42, Michael S. Tsirkin wrote: > > On Mon, Nov 07, 2022 at 09:34:31PM +0100, Eric Auger wrote: > >> When the vhost iotlb is used along with a guest virtual iommu > >> and the guest gets rebooted, some MISS messages may have been > >> recorded just before the reboot and spuriously executed by > >> the virtual iommu after the reboot. Despite the device iotlb gets > >> re-initialized, the messages are not cleared. Fix that by calling > >> vhost_clear_msg() at the end of vhost_init_device_iotlb(). > >> > >> Signed-off-by: Eric Auger <eric.auger@redhat.com> > >> --- > >> drivers/vhost/vhost.c | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > >> index 40097826cff0..422a1fdee0ca 100644 > >> --- a/drivers/vhost/vhost.c > >> +++ b/drivers/vhost/vhost.c > >> @@ -1751,6 +1751,7 @@ int vhost_init_device_iotlb(struct vhost_dev *d, bool enabled) > >> } > >> > >> vhost_iotlb_free(oiotlb); > >> + vhost_clear_msg(d); > >> > >> return 0; > >> } > > Hmm. Can't messages meanwhile get processes and affect the > > new iotlb? > Isn't the msg processing stopped at the moment this function is called > (VHOST_SET_FEATURES)? > > Thanks > > Eric It's pretty late here I'm not sure. You tell me what prevents it. BTW vhost_init_device_iotlb gets enabled parameter but ignores it, we really should drop that. Also, it looks like if features are set with VIRTIO_F_ACCESS_PLATFORM and then cleared, iotlb is not properly cleared - bug? > > > > > >> -- > >> 2.37.3
On Tue, Nov 8, 2022 at 7:06 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Nov 07, 2022 at 10:10:06PM +0100, Eric Auger wrote: > > Hi Michael, > > On 11/7/22 21:42, Michael S. Tsirkin wrote: > > > On Mon, Nov 07, 2022 at 09:34:31PM +0100, Eric Auger wrote: > > >> When the vhost iotlb is used along with a guest virtual iommu > > >> and the guest gets rebooted, some MISS messages may have been > > >> recorded just before the reboot and spuriously executed by > > >> the virtual iommu after the reboot. Despite the device iotlb gets > > >> re-initialized, the messages are not cleared. Fix that by calling > > >> vhost_clear_msg() at the end of vhost_init_device_iotlb(). > > >> > > >> Signed-off-by: Eric Auger <eric.auger@redhat.com> > > >> --- > > >> drivers/vhost/vhost.c | 1 + > > >> 1 file changed, 1 insertion(+) > > >> > > >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > >> index 40097826cff0..422a1fdee0ca 100644 > > >> --- a/drivers/vhost/vhost.c > > >> +++ b/drivers/vhost/vhost.c > > >> @@ -1751,6 +1751,7 @@ int vhost_init_device_iotlb(struct vhost_dev *d, bool enabled) > > >> } > > >> > > >> vhost_iotlb_free(oiotlb); > > >> + vhost_clear_msg(d); > > >> > > >> return 0; > > >> } > > > Hmm. Can't messages meanwhile get processes and affect the > > > new iotlb? > > Isn't the msg processing stopped at the moment this function is called > > (VHOST_SET_FEATURES)? > > > > Thanks > > > > Eric > > It's pretty late here I'm not sure. You tell me what prevents it. So the proposed code assumes that Qemu doesn't process device IOTLB before VHOST_SET_FEAETURES. Consider there's no reset in the general vhost uAPI, I wonder if it's better to move the clear to device code like VHOST_NET_SET_BACKEND. So we can clear it per vq? > > BTW vhost_init_device_iotlb gets enabled parameter but ignores > it, we really should drop that. Yes. > > Also, it looks like if features are set with VIRTIO_F_ACCESS_PLATFORM > and then cleared, iotlb is not properly cleared - bug? Not sure, old IOTLB may still work. But for safety, we need to disable device IOTLB in this case. Thanks > > > > > > > > > > >> -- > > >> 2.37.3 >
On 11/8/22 04:09, Jason Wang wrote: > On Tue, Nov 8, 2022 at 7:06 AM Michael S. Tsirkin <mst@redhat.com> wrote: >> On Mon, Nov 07, 2022 at 10:10:06PM +0100, Eric Auger wrote: >>> Hi Michael, >>> On 11/7/22 21:42, Michael S. Tsirkin wrote: >>>> On Mon, Nov 07, 2022 at 09:34:31PM +0100, Eric Auger wrote: >>>>> When the vhost iotlb is used along with a guest virtual iommu >>>>> and the guest gets rebooted, some MISS messages may have been >>>>> recorded just before the reboot and spuriously executed by >>>>> the virtual iommu after the reboot. Despite the device iotlb gets >>>>> re-initialized, the messages are not cleared. Fix that by calling >>>>> vhost_clear_msg() at the end of vhost_init_device_iotlb(). >>>>> >>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>>>> --- >>>>> drivers/vhost/vhost.c | 1 + >>>>> 1 file changed, 1 insertion(+) >>>>> >>>>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >>>>> index 40097826cff0..422a1fdee0ca 100644 >>>>> --- a/drivers/vhost/vhost.c >>>>> +++ b/drivers/vhost/vhost.c >>>>> @@ -1751,6 +1751,7 @@ int vhost_init_device_iotlb(struct vhost_dev *d, bool enabled) >>>>> } >>>>> >>>>> vhost_iotlb_free(oiotlb); >>>>> + vhost_clear_msg(d); >>>>> >>>>> return 0; >>>>> } >>>> Hmm. Can't messages meanwhile get processes and affect the >>>> new iotlb? >>> Isn't the msg processing stopped at the moment this function is called >>> (VHOST_SET_FEATURES)? >>> >>> Thanks >>> >>> Eric >> It's pretty late here I'm not sure. You tell me what prevents it. > So the proposed code assumes that Qemu doesn't process device IOTLB > before VHOST_SET_FEAETURES. Consider there's no reset in the general > vhost uAPI, I wonder if it's better to move the clear to device code > like VHOST_NET_SET_BACKEND. So we can clear it per vq? OK I will look at this alternative > >> BTW vhost_init_device_iotlb gets enabled parameter but ignores >> it, we really should drop that. > Yes. Yes I saw that too. I will send a patch. > >> Also, it looks like if features are set with VIRTIO_F_ACCESS_PLATFORM >> and then cleared, iotlb is not properly cleared - bug? > Not sure, old IOTLB may still work. But for safety, we need to disable > device IOTLB in this case. OK Thanks Eric > > Thanks > >> >>>> >>>>> -- >>>>> 2.37.3
On Tue, Nov 08, 2022 at 11:09:36AM +0800, Jason Wang wrote: > On Tue, Nov 8, 2022 at 7:06 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Mon, Nov 07, 2022 at 10:10:06PM +0100, Eric Auger wrote: > > > Hi Michael, > > > On 11/7/22 21:42, Michael S. Tsirkin wrote: > > > > On Mon, Nov 07, 2022 at 09:34:31PM +0100, Eric Auger wrote: > > > >> When the vhost iotlb is used along with a guest virtual iommu > > > >> and the guest gets rebooted, some MISS messages may have been > > > >> recorded just before the reboot and spuriously executed by > > > >> the virtual iommu after the reboot. Despite the device iotlb gets > > > >> re-initialized, the messages are not cleared. Fix that by calling > > > >> vhost_clear_msg() at the end of vhost_init_device_iotlb(). > > > >> > > > >> Signed-off-by: Eric Auger <eric.auger@redhat.com> > > > >> --- > > > >> drivers/vhost/vhost.c | 1 + > > > >> 1 file changed, 1 insertion(+) > > > >> > > > >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > > >> index 40097826cff0..422a1fdee0ca 100644 > > > >> --- a/drivers/vhost/vhost.c > > > >> +++ b/drivers/vhost/vhost.c > > > >> @@ -1751,6 +1751,7 @@ int vhost_init_device_iotlb(struct vhost_dev *d, bool enabled) > > > >> } > > > >> > > > >> vhost_iotlb_free(oiotlb); > > > >> + vhost_clear_msg(d); > > > >> > > > >> return 0; > > > >> } > > > > Hmm. Can't messages meanwhile get processes and affect the > > > > new iotlb? > > > Isn't the msg processing stopped at the moment this function is called > > > (VHOST_SET_FEATURES)? > > > > > > Thanks > > > > > > Eric > > > > It's pretty late here I'm not sure. You tell me what prevents it. > > So the proposed code assumes that Qemu doesn't process device IOTLB > before VHOST_SET_FEAETURES. Consider there's no reset in the general > vhost uAPI, I wonder if it's better to move the clear to device code > like VHOST_NET_SET_BACKEND. So we can clear it per vq? Hmm this makes no sense to me. iommu sits between backend and frontend. Tying one to another is going to backfire. I'm thinking more along the lines of doing everything under iotlb_lock. > > > > BTW vhost_init_device_iotlb gets enabled parameter but ignores > > it, we really should drop that. > > Yes. > > > > > Also, it looks like if features are set with VIRTIO_F_ACCESS_PLATFORM > > and then cleared, iotlb is not properly cleared - bug? > > Not sure, old IOTLB may still work. But for safety, we need to disable > device IOTLB in this case. > > Thanks > > > > > > > > > > > > > > > > >> -- > > > >> 2.37.3 > >
On Tue, Nov 8, 2022 at 4:56 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, Nov 08, 2022 at 11:09:36AM +0800, Jason Wang wrote: > > On Tue, Nov 8, 2022 at 7:06 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Mon, Nov 07, 2022 at 10:10:06PM +0100, Eric Auger wrote: > > > > Hi Michael, > > > > On 11/7/22 21:42, Michael S. Tsirkin wrote: > > > > > On Mon, Nov 07, 2022 at 09:34:31PM +0100, Eric Auger wrote: > > > > >> When the vhost iotlb is used along with a guest virtual iommu > > > > >> and the guest gets rebooted, some MISS messages may have been > > > > >> recorded just before the reboot and spuriously executed by > > > > >> the virtual iommu after the reboot. Despite the device iotlb gets > > > > >> re-initialized, the messages are not cleared. Fix that by calling > > > > >> vhost_clear_msg() at the end of vhost_init_device_iotlb(). > > > > >> > > > > >> Signed-off-by: Eric Auger <eric.auger@redhat.com> > > > > >> --- > > > > >> drivers/vhost/vhost.c | 1 + > > > > >> 1 file changed, 1 insertion(+) > > > > >> > > > > >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > > > >> index 40097826cff0..422a1fdee0ca 100644 > > > > >> --- a/drivers/vhost/vhost.c > > > > >> +++ b/drivers/vhost/vhost.c > > > > >> @@ -1751,6 +1751,7 @@ int vhost_init_device_iotlb(struct vhost_dev *d, bool enabled) > > > > >> } > > > > >> > > > > >> vhost_iotlb_free(oiotlb); > > > > >> + vhost_clear_msg(d); > > > > >> > > > > >> return 0; > > > > >> } > > > > > Hmm. Can't messages meanwhile get processes and affect the > > > > > new iotlb? > > > > Isn't the msg processing stopped at the moment this function is called > > > > (VHOST_SET_FEATURES)? > > > > > > > > Thanks > > > > > > > > Eric > > > > > > It's pretty late here I'm not sure. You tell me what prevents it. > > > > So the proposed code assumes that Qemu doesn't process device IOTLB > > before VHOST_SET_FEAETURES. Consider there's no reset in the general > > vhost uAPI, I wonder if it's better to move the clear to device code > > like VHOST_NET_SET_BACKEND. So we can clear it per vq? > > Hmm this makes no sense to me. iommu sits between backend > and frontend. Tying one to another is going to backfire. I think we need to emulate what real devices are doing. Device should clear the page fault message during reset, so the driver won't read anything after reset. But we don't have a per device stop or reset message for vhost-net. That's why the VHOST_NET_SET_BACKEND came into my mind. > > I'm thinking more along the lines of doing everything > under iotlb_lock. I think the problem is we need to find a proper place to clear the message. So I don't get how iotlb_lock can help: the message could be still read from user space after the backend is set to NULL. Thanks > > > > > > > > > BTW vhost_init_device_iotlb gets enabled parameter but ignores > > > it, we really should drop that. > > > > Yes. > > > > > > > > Also, it looks like if features are set with VIRTIO_F_ACCESS_PLATFORM > > > and then cleared, iotlb is not properly cleared - bug? > > > > Not sure, old IOTLB may still work. But for safety, we need to disable > > device IOTLB in this case. > > > > Thanks > > > > > > > > > > > > > > > > > > > > > > >> -- > > > > >> 2.37.3 > > > >
On Tue, Nov 08, 2022 at 05:13:50PM +0800, Jason Wang wrote: > On Tue, Nov 8, 2022 at 4:56 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Tue, Nov 08, 2022 at 11:09:36AM +0800, Jason Wang wrote: > > > On Tue, Nov 8, 2022 at 7:06 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > On Mon, Nov 07, 2022 at 10:10:06PM +0100, Eric Auger wrote: > > > > > Hi Michael, > > > > > On 11/7/22 21:42, Michael S. Tsirkin wrote: > > > > > > On Mon, Nov 07, 2022 at 09:34:31PM +0100, Eric Auger wrote: > > > > > >> When the vhost iotlb is used along with a guest virtual iommu > > > > > >> and the guest gets rebooted, some MISS messages may have been > > > > > >> recorded just before the reboot and spuriously executed by > > > > > >> the virtual iommu after the reboot. Despite the device iotlb gets > > > > > >> re-initialized, the messages are not cleared. Fix that by calling > > > > > >> vhost_clear_msg() at the end of vhost_init_device_iotlb(). > > > > > >> > > > > > >> Signed-off-by: Eric Auger <eric.auger@redhat.com> > > > > > >> --- > > > > > >> drivers/vhost/vhost.c | 1 + > > > > > >> 1 file changed, 1 insertion(+) > > > > > >> > > > > > >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > > > > >> index 40097826cff0..422a1fdee0ca 100644 > > > > > >> --- a/drivers/vhost/vhost.c > > > > > >> +++ b/drivers/vhost/vhost.c > > > > > >> @@ -1751,6 +1751,7 @@ int vhost_init_device_iotlb(struct vhost_dev *d, bool enabled) > > > > > >> } > > > > > >> > > > > > >> vhost_iotlb_free(oiotlb); > > > > > >> + vhost_clear_msg(d); > > > > > >> > > > > > >> return 0; > > > > > >> } > > > > > > Hmm. Can't messages meanwhile get processes and affect the > > > > > > new iotlb? > > > > > Isn't the msg processing stopped at the moment this function is called > > > > > (VHOST_SET_FEATURES)? > > > > > > > > > > Thanks > > > > > > > > > > Eric > > > > > > > > It's pretty late here I'm not sure. You tell me what prevents it. > > > > > > So the proposed code assumes that Qemu doesn't process device IOTLB > > > before VHOST_SET_FEAETURES. Consider there's no reset in the general > > > vhost uAPI, I wonder if it's better to move the clear to device code > > > like VHOST_NET_SET_BACKEND. So we can clear it per vq? > > > > Hmm this makes no sense to me. iommu sits between backend > > and frontend. Tying one to another is going to backfire. > > I think we need to emulate what real devices are doing. Device should > clear the page fault message during reset, so the driver won't read > anything after reset. But we don't have a per device stop or reset > message for vhost-net. That's why the VHOST_NET_SET_BACKEND came into > my mind. That's not a reset message. Userspace can switch backends at will. I guess we could check when backend is set to -1. It's a hack but might work. > > > > I'm thinking more along the lines of doing everything > > under iotlb_lock. > > I think the problem is we need to find a proper place to clear the > message. So I don't get how iotlb_lock can help: the message could be > still read from user space after the backend is set to NULL. > > Thanks Well I think the real problem is this. vhost_net_set_features does: if ((features & (1ULL << VIRTIO_F_ACCESS_PLATFORM))) { if (vhost_init_device_iotlb(&n->dev, true)) goto out_unlock; } so we get a new iotlb each time features are set. But features can be changes while device is running. E.g. VHOST_F_LOG_ALL Let's just say this hack of reusing feature bits for backend was not my brightest idea :( > > > > > > > > > > > > > > BTW vhost_init_device_iotlb gets enabled parameter but ignores > > > > it, we really should drop that. > > > > > > Yes. > > > > > > > > > > > Also, it looks like if features are set with VIRTIO_F_ACCESS_PLATFORM > > > > and then cleared, iotlb is not properly cleared - bug? > > > > > > Not sure, old IOTLB may still work. But for safety, we need to disable > > > device IOTLB in this case. > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > > > > > > > > > >> -- > > > > > >> 2.37.3 > > > > > >
Hi Michael, Jason, On 11/8/22 10:31, Michael S. Tsirkin wrote: > On Tue, Nov 08, 2022 at 05:13:50PM +0800, Jason Wang wrote: >> On Tue, Nov 8, 2022 at 4:56 PM Michael S. Tsirkin <mst@redhat.com> wrote: >>> On Tue, Nov 08, 2022 at 11:09:36AM +0800, Jason Wang wrote: >>>> On Tue, Nov 8, 2022 at 7:06 AM Michael S. Tsirkin <mst@redhat.com> wrote: >>>>> On Mon, Nov 07, 2022 at 10:10:06PM +0100, Eric Auger wrote: >>>>>> Hi Michael, >>>>>> On 11/7/22 21:42, Michael S. Tsirkin wrote: >>>>>>> On Mon, Nov 07, 2022 at 09:34:31PM +0100, Eric Auger wrote: >>>>>>>> When the vhost iotlb is used along with a guest virtual iommu >>>>>>>> and the guest gets rebooted, some MISS messages may have been >>>>>>>> recorded just before the reboot and spuriously executed by >>>>>>>> the virtual iommu after the reboot. Despite the device iotlb gets >>>>>>>> re-initialized, the messages are not cleared. Fix that by calling >>>>>>>> vhost_clear_msg() at the end of vhost_init_device_iotlb(). >>>>>>>> >>>>>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>>>>>>> --- >>>>>>>> drivers/vhost/vhost.c | 1 + >>>>>>>> 1 file changed, 1 insertion(+) >>>>>>>> >>>>>>>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >>>>>>>> index 40097826cff0..422a1fdee0ca 100644 >>>>>>>> --- a/drivers/vhost/vhost.c >>>>>>>> +++ b/drivers/vhost/vhost.c >>>>>>>> @@ -1751,6 +1751,7 @@ int vhost_init_device_iotlb(struct vhost_dev *d, bool enabled) >>>>>>>> } >>>>>>>> >>>>>>>> vhost_iotlb_free(oiotlb); >>>>>>>> + vhost_clear_msg(d); >>>>>>>> >>>>>>>> return 0; >>>>>>>> } >>>>>>> Hmm. Can't messages meanwhile get processes and affect the >>>>>>> new iotlb? >>>>>> Isn't the msg processing stopped at the moment this function is called >>>>>> (VHOST_SET_FEATURES)? >>>>>> >>>>>> Thanks >>>>>> >>>>>> Eric >>>>> It's pretty late here I'm not sure. You tell me what prevents it. >>>> So the proposed code assumes that Qemu doesn't process device IOTLB >>>> before VHOST_SET_FEAETURES. Consider there's no reset in the general >>>> vhost uAPI, I wonder if it's better to move the clear to device code >>>> like VHOST_NET_SET_BACKEND. So we can clear it per vq? >>> Hmm this makes no sense to me. iommu sits between backend >>> and frontend. Tying one to another is going to backfire. >> I think we need to emulate what real devices are doing. Device should >> clear the page fault message during reset, so the driver won't read >> anything after reset. But we don't have a per device stop or reset >> message for vhost-net. That's why the VHOST_NET_SET_BACKEND came into >> my mind. > That's not a reset message. Userspace can switch backends at will. > I guess we could check when backend is set to -1. > It's a hack but might work. > >>> I'm thinking more along the lines of doing everything >>> under iotlb_lock. >> I think the problem is we need to find a proper place to clear the >> message. So I don't get how iotlb_lock can help: the message could be >> still read from user space after the backend is set to NULL. >> >> Thanks > Well I think the real problem is this. > > vhost_net_set_features does: > > if ((features & (1ULL << VIRTIO_F_ACCESS_PLATFORM))) { > if (vhost_init_device_iotlb(&n->dev, true)) > goto out_unlock; > } > > > so we get a new iotlb each time features are set. > > But features can be changes while device is running. > E.g. > VHOST_F_LOG_ALL > > > Let's just say this hack of reusing feature bits for backend > was not my brightest idea :( > Isn't vhost_init_device_iotlb() racy then, as d->iotlb is first updated with niotlb and later d->vqs[i]->iotlb is updated with niotlb. What does garantee this is done atomically? Shouldn't we hold the dev->mutex to make all the sequence atomic and include vhost_clear_msg()? Can't the vhost_clear_msg() take the dev lock? Thanks Eric > > > >>> >>> >>>>> BTW vhost_init_device_iotlb gets enabled parameter but ignores >>>>> it, we really should drop that. >>>> Yes. >>>> >>>>> Also, it looks like if features are set with VIRTIO_F_ACCESS_PLATFORM >>>>> and then cleared, iotlb is not properly cleared - bug? >>>> Not sure, old IOTLB may still work. But for safety, we need to disable >>>> device IOTLB in this case. >>>> >>>> Thanks >>>> >>>>> >>>>>>> >>>>>>>> -- >>>>>>>> 2.37.3
On Tue, Nov 8, 2022 at 5:31 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, Nov 08, 2022 at 05:13:50PM +0800, Jason Wang wrote: > > On Tue, Nov 8, 2022 at 4:56 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Tue, Nov 08, 2022 at 11:09:36AM +0800, Jason Wang wrote: > > > > On Tue, Nov 8, 2022 at 7:06 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > On Mon, Nov 07, 2022 at 10:10:06PM +0100, Eric Auger wrote: > > > > > > Hi Michael, > > > > > > On 11/7/22 21:42, Michael S. Tsirkin wrote: > > > > > > > On Mon, Nov 07, 2022 at 09:34:31PM +0100, Eric Auger wrote: > > > > > > >> When the vhost iotlb is used along with a guest virtual iommu > > > > > > >> and the guest gets rebooted, some MISS messages may have been > > > > > > >> recorded just before the reboot and spuriously executed by > > > > > > >> the virtual iommu after the reboot. Despite the device iotlb gets > > > > > > >> re-initialized, the messages are not cleared. Fix that by calling > > > > > > >> vhost_clear_msg() at the end of vhost_init_device_iotlb(). > > > > > > >> > > > > > > >> Signed-off-by: Eric Auger <eric.auger@redhat.com> > > > > > > >> --- > > > > > > >> drivers/vhost/vhost.c | 1 + > > > > > > >> 1 file changed, 1 insertion(+) > > > > > > >> > > > > > > >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > > > > > >> index 40097826cff0..422a1fdee0ca 100644 > > > > > > >> --- a/drivers/vhost/vhost.c > > > > > > >> +++ b/drivers/vhost/vhost.c > > > > > > >> @@ -1751,6 +1751,7 @@ int vhost_init_device_iotlb(struct vhost_dev *d, bool enabled) > > > > > > >> } > > > > > > >> > > > > > > >> vhost_iotlb_free(oiotlb); > > > > > > >> + vhost_clear_msg(d); > > > > > > >> > > > > > > >> return 0; > > > > > > >> } > > > > > > > Hmm. Can't messages meanwhile get processes and affect the > > > > > > > new iotlb? > > > > > > Isn't the msg processing stopped at the moment this function is called > > > > > > (VHOST_SET_FEATURES)? > > > > > > > > > > > > Thanks > > > > > > > > > > > > Eric > > > > > > > > > > It's pretty late here I'm not sure. You tell me what prevents it. > > > > > > > > So the proposed code assumes that Qemu doesn't process device IOTLB > > > > before VHOST_SET_FEAETURES. Consider there's no reset in the general > > > > vhost uAPI, I wonder if it's better to move the clear to device code > > > > like VHOST_NET_SET_BACKEND. So we can clear it per vq? > > > > > > Hmm this makes no sense to me. iommu sits between backend > > > and frontend. Tying one to another is going to backfire. > > > > I think we need to emulate what real devices are doing. Device should > > clear the page fault message during reset, so the driver won't read > > anything after reset. But we don't have a per device stop or reset > > message for vhost-net. That's why the VHOST_NET_SET_BACKEND came into > > my mind. > > That's not a reset message. Userspace can switch backends at will. > I guess we could check when backend is set to -1. > It's a hack but might work. Yes, that's what I meant actually. > > > > > > > I'm thinking more along the lines of doing everything > > > under iotlb_lock. > > > > I think the problem is we need to find a proper place to clear the > > message. So I don't get how iotlb_lock can help: the message could be > > still read from user space after the backend is set to NULL. > > > > Thanks > > Well I think the real problem is this. > > vhost_net_set_features does: > > if ((features & (1ULL << VIRTIO_F_ACCESS_PLATFORM))) { > if (vhost_init_device_iotlb(&n->dev, true)) > goto out_unlock; > } > > > so we get a new iotlb each time features are set. Right, but this looks like another independent issue that needs to be fixed. > > But features can be changes while device is running. > E.g. > VHOST_F_LOG_ALL > > > Let's just say this hack of reusing feature bits for backend > was not my brightest idea :( > Probably :) Thanks > > > > > > > > > > > > > > > > > > > > > > > BTW vhost_init_device_iotlb gets enabled parameter but ignores > > > > > it, we really should drop that. > > > > > > > > Yes. > > > > > > > > > > > > > > Also, it looks like if features are set with VIRTIO_F_ACCESS_PLATFORM > > > > > and then cleared, iotlb is not properly cleared - bug? > > > > > > > > Not sure, old IOTLB may still work. But for safety, we need to disable > > > > device IOTLB in this case. > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >> -- > > > > > > >> 2.37.3 > > > > > > > > >
On Tue, Nov 8, 2022 at 6:17 PM Eric Auger <eric.auger@redhat.com> wrote: > > Hi Michael, Jason, > > On 11/8/22 10:31, Michael S. Tsirkin wrote: > > On Tue, Nov 08, 2022 at 05:13:50PM +0800, Jason Wang wrote: > >> On Tue, Nov 8, 2022 at 4:56 PM Michael S. Tsirkin <mst@redhat.com> wrote: > >>> On Tue, Nov 08, 2022 at 11:09:36AM +0800, Jason Wang wrote: > >>>> On Tue, Nov 8, 2022 at 7:06 AM Michael S. Tsirkin <mst@redhat.com> wrote: > >>>>> On Mon, Nov 07, 2022 at 10:10:06PM +0100, Eric Auger wrote: > >>>>>> Hi Michael, > >>>>>> On 11/7/22 21:42, Michael S. Tsirkin wrote: > >>>>>>> On Mon, Nov 07, 2022 at 09:34:31PM +0100, Eric Auger wrote: > >>>>>>>> When the vhost iotlb is used along with a guest virtual iommu > >>>>>>>> and the guest gets rebooted, some MISS messages may have been > >>>>>>>> recorded just before the reboot and spuriously executed by > >>>>>>>> the virtual iommu after the reboot. Despite the device iotlb gets > >>>>>>>> re-initialized, the messages are not cleared. Fix that by calling > >>>>>>>> vhost_clear_msg() at the end of vhost_init_device_iotlb(). > >>>>>>>> > >>>>>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com> > >>>>>>>> --- > >>>>>>>> drivers/vhost/vhost.c | 1 + > >>>>>>>> 1 file changed, 1 insertion(+) > >>>>>>>> > >>>>>>>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > >>>>>>>> index 40097826cff0..422a1fdee0ca 100644 > >>>>>>>> --- a/drivers/vhost/vhost.c > >>>>>>>> +++ b/drivers/vhost/vhost.c > >>>>>>>> @@ -1751,6 +1751,7 @@ int vhost_init_device_iotlb(struct vhost_dev *d, bool enabled) > >>>>>>>> } > >>>>>>>> > >>>>>>>> vhost_iotlb_free(oiotlb); > >>>>>>>> + vhost_clear_msg(d); > >>>>>>>> > >>>>>>>> return 0; > >>>>>>>> } > >>>>>>> Hmm. Can't messages meanwhile get processes and affect the > >>>>>>> new iotlb? > >>>>>> Isn't the msg processing stopped at the moment this function is called > >>>>>> (VHOST_SET_FEATURES)? > >>>>>> > >>>>>> Thanks > >>>>>> > >>>>>> Eric > >>>>> It's pretty late here I'm not sure. You tell me what prevents it. > >>>> So the proposed code assumes that Qemu doesn't process device IOTLB > >>>> before VHOST_SET_FEAETURES. Consider there's no reset in the general > >>>> vhost uAPI, I wonder if it's better to move the clear to device code > >>>> like VHOST_NET_SET_BACKEND. So we can clear it per vq? > >>> Hmm this makes no sense to me. iommu sits between backend > >>> and frontend. Tying one to another is going to backfire. > >> I think we need to emulate what real devices are doing. Device should > >> clear the page fault message during reset, so the driver won't read > >> anything after reset. But we don't have a per device stop or reset > >> message for vhost-net. That's why the VHOST_NET_SET_BACKEND came into > >> my mind. > > That's not a reset message. Userspace can switch backends at will. > > I guess we could check when backend is set to -1. > > It's a hack but might work. > > > >>> I'm thinking more along the lines of doing everything > >>> under iotlb_lock. > >> I think the problem is we need to find a proper place to clear the > >> message. So I don't get how iotlb_lock can help: the message could be > >> still read from user space after the backend is set to NULL. > >> > >> Thanks > > Well I think the real problem is this. > > > > vhost_net_set_features does: > > > > if ((features & (1ULL << VIRTIO_F_ACCESS_PLATFORM))) { > > if (vhost_init_device_iotlb(&n->dev, true)) > > goto out_unlock; > > } > > > > > > so we get a new iotlb each time features are set. > > > > But features can be changes while device is running. > > E.g. > > VHOST_F_LOG_ALL > > > > > > Let's just say this hack of reusing feature bits for backend > > was not my brightest idea :( > > > > Isn't vhost_init_device_iotlb() racy then, as d->iotlb is first updated with niotlb and later d->vqs[i]->iotlb is updated with niotlb. What does garantee this is done atomically? > > Shouldn't we hold the dev->mutex to make all the sequence atomic and > include vhost_clear_msg()? Can't the vhost_clear_msg() take the dev lock? It depends on where we want to place the vhost_clear_msg(), e.g in most of the device ioctl, the dev->mutex has been held. Thanks > > Thanks > > Eric > > > > > > > > >>> > >>> > >>>>> BTW vhost_init_device_iotlb gets enabled parameter but ignores > >>>>> it, we really should drop that. > >>>> Yes. > >>>> > >>>>> Also, it looks like if features are set with VIRTIO_F_ACCESS_PLATFORM > >>>>> and then cleared, iotlb is not properly cleared - bug? > >>>> Not sure, old IOTLB may still work. But for safety, we need to disable > >>>> device IOTLB in this case. > >>>> > >>>> Thanks > >>>> > >>>>> > >>>>>>> > >>>>>>>> -- > >>>>>>>> 2.37.3 >
Hi Jason, On 11/9/22 04:44, Jason Wang wrote: > On Tue, Nov 8, 2022 at 6:17 PM Eric Auger <eric.auger@redhat.com> wrote: >> Hi Michael, Jason, >> >> On 11/8/22 10:31, Michael S. Tsirkin wrote: >>> On Tue, Nov 08, 2022 at 05:13:50PM +0800, Jason Wang wrote: >>>> On Tue, Nov 8, 2022 at 4:56 PM Michael S. Tsirkin <mst@redhat.com> wrote: >>>>> On Tue, Nov 08, 2022 at 11:09:36AM +0800, Jason Wang wrote: >>>>>> On Tue, Nov 8, 2022 at 7:06 AM Michael S. Tsirkin <mst@redhat.com> wrote: >>>>>>> On Mon, Nov 07, 2022 at 10:10:06PM +0100, Eric Auger wrote: >>>>>>>> Hi Michael, >>>>>>>> On 11/7/22 21:42, Michael S. Tsirkin wrote: >>>>>>>>> On Mon, Nov 07, 2022 at 09:34:31PM +0100, Eric Auger wrote: >>>>>>>>>> When the vhost iotlb is used along with a guest virtual iommu >>>>>>>>>> and the guest gets rebooted, some MISS messages may have been >>>>>>>>>> recorded just before the reboot and spuriously executed by >>>>>>>>>> the virtual iommu after the reboot. Despite the device iotlb gets >>>>>>>>>> re-initialized, the messages are not cleared. Fix that by calling >>>>>>>>>> vhost_clear_msg() at the end of vhost_init_device_iotlb(). >>>>>>>>>> >>>>>>>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>>>>>>>>> --- >>>>>>>>>> drivers/vhost/vhost.c | 1 + >>>>>>>>>> 1 file changed, 1 insertion(+) >>>>>>>>>> >>>>>>>>>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >>>>>>>>>> index 40097826cff0..422a1fdee0ca 100644 >>>>>>>>>> --- a/drivers/vhost/vhost.c >>>>>>>>>> +++ b/drivers/vhost/vhost.c >>>>>>>>>> @@ -1751,6 +1751,7 @@ int vhost_init_device_iotlb(struct vhost_dev *d, bool enabled) >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> vhost_iotlb_free(oiotlb); >>>>>>>>>> + vhost_clear_msg(d); >>>>>>>>>> >>>>>>>>>> return 0; >>>>>>>>>> } >>>>>>>>> Hmm. Can't messages meanwhile get processes and affect the >>>>>>>>> new iotlb? >>>>>>>> Isn't the msg processing stopped at the moment this function is called >>>>>>>> (VHOST_SET_FEATURES)? >>>>>>>> >>>>>>>> Thanks >>>>>>>> >>>>>>>> Eric >>>>>>> It's pretty late here I'm not sure. You tell me what prevents it. >>>>>> So the proposed code assumes that Qemu doesn't process device IOTLB >>>>>> before VHOST_SET_FEAETURES. Consider there's no reset in the general >>>>>> vhost uAPI, I wonder if it's better to move the clear to device code >>>>>> like VHOST_NET_SET_BACKEND. So we can clear it per vq? >>>>> Hmm this makes no sense to me. iommu sits between backend >>>>> and frontend. Tying one to another is going to backfire. >>>> I think we need to emulate what real devices are doing. Device should >>>> clear the page fault message during reset, so the driver won't read >>>> anything after reset. But we don't have a per device stop or reset >>>> message for vhost-net. That's why the VHOST_NET_SET_BACKEND came into >>>> my mind. >>> That's not a reset message. Userspace can switch backends at will. >>> I guess we could check when backend is set to -1. >>> It's a hack but might work. >>> >>>>> I'm thinking more along the lines of doing everything >>>>> under iotlb_lock. >>>> I think the problem is we need to find a proper place to clear the >>>> message. So I don't get how iotlb_lock can help: the message could be >>>> still read from user space after the backend is set to NULL. >>>> >>>> Thanks >>> Well I think the real problem is this. >>> >>> vhost_net_set_features does: >>> >>> if ((features & (1ULL << VIRTIO_F_ACCESS_PLATFORM))) { >>> if (vhost_init_device_iotlb(&n->dev, true)) >>> goto out_unlock; >>> } >>> >>> >>> so we get a new iotlb each time features are set. >>> >>> But features can be changes while device is running. >>> E.g. >>> VHOST_F_LOG_ALL >>> >>> >>> Let's just say this hack of reusing feature bits for backend >>> was not my brightest idea :( >>> >> Isn't vhost_init_device_iotlb() racy then, as d->iotlb is first updated with niotlb and later d->vqs[i]->iotlb is updated with niotlb. What does garantee this is done atomically? >> >> Shouldn't we hold the dev->mutex to make all the sequence atomic and >> include vhost_clear_msg()? Can't the vhost_clear_msg() take the dev lock? > It depends on where we want to place the vhost_clear_msg(), e.g in > most of the device ioctl, the dev->mutex has been held. OK, I will double check and respin accordingly Eric > > Thanks > >> Thanks >> >> Eric >> >>> >>> >>>>> >>>>>>> BTW vhost_init_device_iotlb gets enabled parameter but ignores >>>>>>> it, we really should drop that. >>>>>> Yes. >>>>>> >>>>>>> Also, it looks like if features are set with VIRTIO_F_ACCESS_PLATFORM >>>>>>> and then cleared, iotlb is not properly cleared - bug? >>>>>> Not sure, old IOTLB may still work. But for safety, we need to disable >>>>>> device IOTLB in this case. >>>>>> >>>>>> Thanks >>>>>> >>>>>>>>>> -- >>>>>>>>>> 2.37.3
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index 40097826cff0..422a1fdee0ca 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -1751,6 +1751,7 @@ int vhost_init_device_iotlb(struct vhost_dev *d, bool enabled) } vhost_iotlb_free(oiotlb); + vhost_clear_msg(d); return 0; }