Jason Wang
2022-Nov-08 03:09 UTC
[RFC] vhost: Clear the pending messages on vhost_init_device_iotlb()
On Tue, Nov 8, 2022 at 7:06 AM Michael S. Tsirkin <mst at 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 at 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 >
Eric Auger
2022-Nov-08 07:31 UTC
[RFC] vhost: Clear the pending messages on vhost_init_device_iotlb()
On 11/8/22 04:09, Jason Wang wrote:> On Tue, Nov 8, 2022 at 7:06 AM Michael S. Tsirkin <mst at 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 at 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
Michael S. Tsirkin
2022-Nov-08 08:55 UTC
[RFC] vhost: Clear the pending messages on vhost_init_device_iotlb()
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 at 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 at 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 > >