Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()

[Jason Wang <jasowang@redhat.com>]

在 2022/4/6 下午11:31, Michael S. Tsirkin 写道:
> On Wed, Apr 06, 2022 at 03:04:32PM +0200, Cornelia Huck wrote:
>> On Wed, Apr 06 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>
>>> On Wed, Apr 06, 2022 at 04:35:37PM +0800, Jason Wang wrote:
>>>> This patch implements PCI version of synchronize_vqs().
>>>>
>>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>>> Cc: "Paul E. McKenney" <paulmck@kernel.org>
>>>> Cc: Marc Zyngier <maz@kernel.org>
>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>> Please add implementations at least for ccw and mmio.
>> I'm not sure what (if anything) can/should be done for ccw...
>>
>>>> ---
>>>>   drivers/virtio/virtio_pci_common.c | 14 ++++++++++++++
>>>>   drivers/virtio/virtio_pci_common.h |  2 ++
>>>>   drivers/virtio/virtio_pci_legacy.c |  1 +
>>>>   drivers/virtio/virtio_pci_modern.c |  2 ++
>>>>   4 files changed, 19 insertions(+)
>>>>
>>>> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
>>>> index d724f676608b..b78c8bc93a97 100644
>>>> --- a/drivers/virtio/virtio_pci_common.c
>>>> +++ b/drivers/virtio/virtio_pci_common.c
>>>> @@ -37,6 +37,20 @@ void vp_synchronize_vectors(struct virtio_device *vdev)
>>>>   		synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
>>>>   }
>>>>   
>>>> +void vp_synchronize_vqs(struct virtio_device *vdev)
>>>> +{
>>>> +	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
>>>> +	int i;
>>>> +
>>>> +	if (vp_dev->intx_enabled) {
>>>> +		synchronize_irq(vp_dev->pci_dev->irq);
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	for (i = 0; i < vp_dev->msix_vectors; ++i)
>>>> +		synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
>>>> +}
>>>> +
>> ...given that this seems to synchronize threaded interrupt handlers?
> No, any handlers at all. The point is to make sure any memory changes
> made prior to this op are visible to callbacks.
>
> Jason, maybe add that to the documentation?


Sure.


>
>> Halil, do you think ccw needs to do anything? (AFAICS, we only have one
>> 'irq' for channel devices anyway, and the handler just calls the
>> relevant callbacks directly.)
> Then you need to synchronize with that.


Have a quick glance at the codes, it looks to me we can synchronize with 
the IO_INTERRUPT. (Assuming all callbacks are triggered via 
ccw_device_irq()).

Thanks


>
>>>>   /* the notify function used when creating a virt queue */
>>>>   bool vp_notify(struct virtqueue *vq)
>>>>   {