Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()
On Wed, Apr 13 2022, Jason Wang wrote:
> On Wed, Apr 13, 2022 at 12:49 AM Cornelia Huck wrote:
>>
>> On Tue, Apr 12 2022, Halil Pasic wrote:
>>
>> > On Mon, 11 Apr 2022 16:27:41 +0200
>> > Cornelia Huck wrote:
>>
>> >> My main concern is that we would need to synchronize against a single
>> >> interrupt that covers all kinds of I/O interrupts, not just a single
>> >> device...
>> >>
>> >
>> > Could we synchronize on struct airq_info's lock member? If we were
>> > to grab all of these that might be involved...
>>
>> Hm, that could possibly narrow the sync down to a subset, which seems
>> better. For devices still using classic interrupts, per-device sync
>> would be easy.
>>
>> >
>> > AFAIU for the synchronize implementation we need a lock or a set of locks
>> > that contain all the possible vring_interrupt() calls with the queuues
>> > that belong to the given device as a critical section. That way, one
>> > has the acquire's and release's in place so that the vrign_interrupt()
>> > either guaranteed to finish before the change of driver_ready is
>> > guaranteed to be complete, or it is guaranteed to see the change.
>> >
>> > In any case, I guess we should first get clear on the first part. I.e.
>> > when do we want to allow host->guest notifications.
>>
>> Also, whether we just care about vring interrupts, or general device
>> interrupts (not sure if a config change interrupt may also trigger
>> things we do not want to trigger?)
>
> I think only vring interrupts, since the config interrupt hardening is
> done via 22b7050a024d7 ("virtio: defer config changed notifications")
Ah thanks, I even reviewed that one back then :)
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()
On Wed, Apr 13, 2022 at 12:49 AM Cornelia Huck wrote:
>
> On Tue, Apr 12 2022, Halil Pasic wrote:
>
> > On Mon, 11 Apr 2022 16:27:41 +0200
> > Cornelia Huck wrote:
>
> >> My main concern is that we would need to synchronize against a single
> >> interrupt that covers all kinds of I/O interrupts, not just a single
> >> device...
> >>
> >
> > Could we synchronize on struct airq_info's lock member? If we were
> > to grab all of these that might be involved...
>
> Hm, that could possibly narrow the sync down to a subset, which seems
> better. For devices still using classic interrupts, per-device sync
> would be easy.
>
> >
> > AFAIU for the synchronize implementation we need a lock or a set of locks
> > that contain all the possible vring_interrupt() calls with the queuues
> > that belong to the given device as a critical section. That way, one
> > has the acquire's and release's in place so that the vrign_interrupt()
> > either guaranteed to finish before the change of driver_ready is
> > guaranteed to be complete, or it is guaranteed to see the change.
> >
> > In any case, I guess we should first get clear on the first part. I.e.
> > when do we want to allow host->guest notifications.
>
> Also, whether we just care about vring interrupts, or general device
> interrupts (not sure if a config change interrupt may also trigger
> things we do not want to trigger?)
I think only vring interrupts, since the config interrupt hardening is
done via 22b7050a024d7 ("virtio: defer config changed notifications")
Thanks
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()
On Tue, Apr 12 2022, Halil Pasic wrote: > On Mon, 11 Apr 2022 16:27:41 +0200 > Cornelia Huck wrote: >> My main concern is that we would need to synchronize against a single >> interrupt that covers all kinds of I/O interrupts, not just a single >> device... >> > > Could we synchronize on struct airq_info's lock member? If we were > to grab all of these that might be involved... Hm, that could possibly narrow the sync down to a subset, which seems better. For devices still using classic interrupts, per-device sync would be easy. > > AFAIU for the synchronize implementation we need a lock or a set of locks > that contain all the possible vring_interrupt() calls with the queuues > that belong to the given device as a critical section. That way, one > has the acquire's and release's in place so that the vrign_interrupt() > either guaranteed to finish before the change of driver_ready is > guaranteed to be complete, or it is guaranteed to see the change. > > In any case, I guess we should first get clear on the first part. I.e. > when do we want to allow host->guest notifications. Also, whether we just care about vring interrupts, or general device interrupts (not sure if a config change interrupt may also trigger things we do not want to trigger?) ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()
On Tue, 12 Apr 2022 10:24:35 +0800 Jason Wang wrote: > > Regarding the question "are we safe against notifications before > > indicators have been registered" I think we really need to think about > > something like Secure Execution. We don't have, and we are unlikely > > to have in hardware virtio-ccw implementations, and for a malicious > > hypervisor > > that has full access to the guest memory hardening makes no sense. > > Does s390 have something like memory encryption? (I guess yes). In the > case of x86 VM encryption, the I/O buffers were now done via software > IOTLB, that's why hardening of the virtio driver is needed to prevent > the hypervisor to poke the swiotlb etc. Yep! Secure Execution is a confidential computing solution which is much like encrypted guest memory, except for one gets exceptions when trying to access private memory instead of ending up with garbage because of the encryption. These improvements are IMHO relevant to us! Regards, Halil ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()
On Tue, Apr 12, 2022 at 8:02 AM Halil Pasic wrote:
>
> On Mon, 11 Apr 2022 16:27:41 +0200
> Cornelia Huck wrote:
>
> > On Mon, Apr 11 2022, Jason Wang wrote:
> >
> > > On Sun, Apr 10, 2022 at 3:51 PM Michael S. Tsirkin
> > > wrote:
> > >>
> > >> On Fri, Apr 08, 2022 at 03:03:07PM +0200, Halil Pasic wrote:
> > >> > On Wed, 06 Apr 2022 15:04:32 +0200
> > >> > Cornelia Huck wrote:
> > >> >
> > >> > > On Wed, Apr 06 2022, "Michael S. Tsirkin" 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
> > >> > > >> Cc: Peter Zijlstra
> > >> > > >> Cc: "Paul E. McKenney"
> > >> > > >> Cc: Marc Zyngier
> > >> > > >> Signed-off-by: Jason Wang
> > >> > > >
> > >> > > > Please add implementations at least for ccw and mmio.
> > >> > >
> > >> > > I'm not sure what (if anything) can/should be done for ccw...
> > >> >
> > >> > If nothing needs to be done I would like to have at least a comment in
> > >> > the code that explains why. So that somebody who reads the code
> > >> > doesn't wonder: why is virtio-ccw not implementing that callback.
> > >>
> > >> Right.
> > >>
> > >> I am currently thinking instead of making this optional in the
> > >> core we should make it mandatory, and have transports which do not
> > >> need to sync have an empty stub with documentation explaining why.
> >
> > Yes, that makes sense to me. If we can explain why we don't need to do
> > anything, we should keep that explanation easily accessible.
> >
> > >>
> > >> Also, do we want to document this sync is explicitly for irq
> > >> enable/disable?
> > >> synchronize_irq_enable_disable?
> > >
> > > I would not since the transport is not guaranteed to use an interrupt
> > > for callbacks.
> > >
> > >>
> > >>
> > >> > >
> > >> > > >
> > >> > > >> ---
> > >> > > >> 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?
> > >> > > 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.)
> > >> >
> > >> > Sorry I don't understand enough yet. A more verbose documentation on
> > >> > "virtio_synchronize_vqs - synchronize with virtqueue callbacks" would
> > >> > surely benefit me. It may be more than enough for a back-belt but it
> > >> > ain't enough for me to tell what is the callback supposed to
> > >> > accomplish.
> >
> > +1 for more explanations.
> >
> > >> >
> > >> > I will have to study this discussion and the code more thoroughly.
> > >> > Tentatively I side with Jason and Michael in a sense, that I don't
> > >> > believe virtio-ccw is safe against rough interrupts.
> > >
> > > That's my feeling as well.
> >
> > I'd say ccw is safe against "notification interrupts before indicators
> > have been registered".
>
> I believe Jason's scope is broader than that. Let me try to explain. A
> quote form the standard:
> """
> 3.1.1 Driver Requirements: Device Initialization
> The driver MUST follow this sequence to initialize a device:
> 1. Reset the device.
> 2. Set the ACKNOWLEDGE status bit: the guest OS has noticed the device.
> 3. Set the DRIVER status bit: the guest OS knows how to drive the device.
> 4. Read device feature bits, and write the subset of feature bits
> understood by the OS and driver to the device. During this step the driver
> MAY read (but MUST NOT write) the device-specific configuration
Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()
On Mon, Apr 11, 2022 at 4:56 PM Michael S. Tsirkin wrote:
>
> On Mon, Apr 11, 2022 at 04:22:19PM +0800, Jason Wang wrote:
> > On Sun, Apr 10, 2022 at 3:51 PM Michael S. Tsirkin wrote:
> > >
> > > On Fri, Apr 08, 2022 at 03:03:07PM +0200, Halil Pasic wrote:
> > > > On Wed, 06 Apr 2022 15:04:32 +0200
> > > > Cornelia Huck wrote:
> > > >
> > > > > On Wed, Apr 06 2022, "Michael S. Tsirkin" 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
> > > > > >> Cc: Peter Zijlstra
> > > > > >> Cc: "Paul E. McKenney"
> > > > > >> Cc: Marc Zyngier
> > > > > >> Signed-off-by: Jason Wang
> > > > > >
> > > > > > Please add implementations at least for ccw and mmio.
> > > > >
> > > > > I'm not sure what (if anything) can/should be done for ccw...
> > > >
> > > > If nothing needs to be done I would like to have at least a comment in
> > > > the code that explains why. So that somebody who reads the code
> > > > doesn't wonder: why is virtio-ccw not implementing that callback.
> > >
> > > Right.
> > >
> > > I am currently thinking instead of making this optional in the
> > > core we should make it mandatory, and have transports which do not
> > > need to sync have an empty stub with documentation explaining why.
> > >
> > > Also, do we want to document this sync is explicitly for irq
> > > enable/disable?
> > > synchronize_irq_enable_disable?
> >
> > I would not since the transport is not guaranteed to use an interrupt
> > for callbacks.
>
> OK but let's then document this in more detail.
> More readers will wonder about what is the callback
> trying to accomplish, and Halil requested that as well.
>
> For example, let's document why is sync required on enable.
Ok.
Thanks
>
> > >
> > >
> > > > >
> > > > > >
> > > > > >> ---
> > > > > >> 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?
> > > > > 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.)
> > > >
> > > > Sorry I don't understand enough yet. A more verbose documentation on
> > > > "virtio_synchronize_vqs - synchronize with virtqueue callbacks" would
> > > > surely benefit me. It may be more than enough for a back-belt but it
> > > > ain't enough for me to tell what is the callback supposed to accomplish.
> > > >
> > > > I will have to study this discussion and the code more thoroughly.
> > > > Tentatively I side with Jason and Michael in a sense, that I don't
> > > > believe virtio-ccw is safe against rough interrupts.
> >
> > That's my feeling as well.
> >
> > Thanks
> >
> > > >
> > > > Sorry for the late response. I intend to revisit this on Monday. If
> > > > I don't please feel encouraged to ping.
> > > >
> > > > Regards,
> > > > Halil
> > >
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()
On Mon, 11 Apr 2022 16:27:41 +0200
Cornelia Huck wrote:
> On Mon, Apr 11 2022, Jason Wang wrote:
>
> > On Sun, Apr 10, 2022 at 3:51 PM Michael S. Tsirkin wrote:
> >
> >>
> >> On Fri, Apr 08, 2022 at 03:03:07PM +0200, Halil Pasic wrote:
> >> > On Wed, 06 Apr 2022 15:04:32 +0200
> >> > Cornelia Huck wrote:
> >> >
> >> > > On Wed, Apr 06 2022, "Michael S. Tsirkin" 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
> >> > > >> Cc: Peter Zijlstra
> >> > > >> Cc: "Paul E. McKenney"
> >> > > >> Cc: Marc Zyngier
> >> > > >> Signed-off-by: Jason Wang
> >> > > >
> >> > > > Please add implementations at least for ccw and mmio.
> >> > >
> >> > > I'm not sure what (if anything) can/should be done for ccw...
> >> >
> >> > If nothing needs to be done I would like to have at least a comment in
> >> > the code that explains why. So that somebody who reads the code
> >> > doesn't wonder: why is virtio-ccw not implementing that callback.
> >>
> >> Right.
> >>
> >> I am currently thinking instead of making this optional in the
> >> core we should make it mandatory, and have transports which do not
> >> need to sync have an empty stub with documentation explaining why.
>
> Yes, that makes sense to me. If we can explain why we don't need to do
> anything, we should keep that explanation easily accessible.
>
> >>
> >> Also, do we want to document this sync is explicitly for irq
> >> enable/disable?
> >> synchronize_irq_enable_disable?
> >
> > I would not since the transport is not guaranteed to use an interrupt
> > for callbacks.
> >
> >>
> >>
> >> > >
> >> > > >
> >> > > >> ---
> >> > > >> 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?
> >> > > 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.)
> >> >
> >> > Sorry I don't understand enough yet. A more verbose documentation on
> >> > "virtio_synchronize_vqs - synchronize with virtqueue callbacks" would
> >> > surely benefit me. It may be more than enough for a back-belt but it
> >> > ain't enough for me to tell what is the callback supposed to accomplish.
> >> >
>
> +1 for more explanations.
>
> >> >
> >> > I will have to study this discussion and the code more thoroughly.
> >> > Tentatively I side with Jason and Michael in a sense, that I don't
> >> > believe virtio-ccw is safe against rough interrupts.
> >
> > That's my feeling as well.
>
> I'd say ccw is safe against "notification interrupts before indicators
> have been registered".
I believe Jason's scope is broader than that. Let me try to explain. A
quote form the standard:
"""
3.1.1 Driver Requirements: Device Initialization
The driver MUST follow this sequence to initialize a device:
1. Reset the device.
2. Set the ACKNOWLEDGE status bit: the guest OS has noticed the device.
3. Set the DRIVER status bit: the guest OS knows how to drive the device.
4. Read device feature bits, and write the subset of feature bits
understood by the OS and driver to the device. During this step the driver MAY
read (but MUST NOT write) the device-specific configuration fields to check
that it can support the device before accepting it.
5. Set the FEATURES_OK status bit. The driver MUST NOT accept new feature
bits after this step.
6. Re-read device status to ensure the FEATURES_OK bit is still set:
Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()
On Mon, Apr 11 2022, Jason Wang wrote:
> On Sun, Apr 10, 2022 at 3:51 PM Michael S. Tsirkin wrote:
>>
>> On Fri, Apr 08, 2022 at 03:03:07PM +0200, Halil Pasic wrote:
>> > On Wed, 06 Apr 2022 15:04:32 +0200
>> > Cornelia Huck wrote:
>> >
>> > > On Wed, Apr 06 2022, "Michael S. Tsirkin" 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
>> > > >> Cc: Peter Zijlstra
>> > > >> Cc: "Paul E. McKenney"
>> > > >> Cc: Marc Zyngier
>> > > >> Signed-off-by: Jason Wang
>> > > >
>> > > > Please add implementations at least for ccw and mmio.
>> > >
>> > > I'm not sure what (if anything) can/should be done for ccw...
>> >
>> > If nothing needs to be done I would like to have at least a comment in
>> > the code that explains why. So that somebody who reads the code
>> > doesn't wonder: why is virtio-ccw not implementing that callback.
>>
>> Right.
>>
>> I am currently thinking instead of making this optional in the
>> core we should make it mandatory, and have transports which do not
>> need to sync have an empty stub with documentation explaining why.
Yes, that makes sense to me. If we can explain why we don't need to do
anything, we should keep that explanation easily accessible.
>>
>> Also, do we want to document this sync is explicitly for irq enable/disable?
>> synchronize_irq_enable_disable?
>
> I would not since the transport is not guaranteed to use an interrupt
> for callbacks.
>
>>
>>
>> > >
>> > > >
>> > > >> ---
>> > > >> 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?
>> > > 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.)
>> >
>> > Sorry I don't understand enough yet. A more verbose documentation on
>> > "virtio_synchronize_vqs - synchronize with virtqueue callbacks" would
>> > surely benefit me. It may be more than enough for a back-belt but it
>> > ain't enough for me to tell what is the callback supposed to accomplish.
+1 for more explanations.
>> >
>> > I will have to study this discussion and the code more thoroughly.
>> > Tentatively I side with Jason and Michael in a sense, that I don't
>> > believe virtio-ccw is safe against rough interrupts.
>
> That's my feeling as well.
I'd say ccw is safe against "notification interrupts before indicators
have been registered". For the reverse case, maybe we should always
invalidate the indicators in the reset case? More information regarding
the attack vector would help here :)
My main concern is that we would need to synchronize against a single
interrupt that covers all kinds of I/O interrupts, not just a single
device...
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()
On Mon, Apr 11, 2022 at 04:22:19PM +0800, Jason Wang wrote:
> On Sun, Apr 10, 2022 at 3:51 PM Michael S. Tsirkin wrote:
> >
> > On Fri, Apr 08, 2022 at 03:03:07PM +0200, Halil Pasic wrote:
> > > On Wed, 06 Apr 2022 15:04:32 +0200
> > > Cornelia Huck wrote:
> > >
> > > > On Wed, Apr 06 2022, "Michael S. Tsirkin" 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
> > > > >> Cc: Peter Zijlstra
> > > > >> Cc: "Paul E. McKenney"
> > > > >> Cc: Marc Zyngier
> > > > >> Signed-off-by: Jason Wang
> > > > >
> > > > > Please add implementations at least for ccw and mmio.
> > > >
> > > > I'm not sure what (if anything) can/should be done for ccw...
> > >
> > > If nothing needs to be done I would like to have at least a comment in
> > > the code that explains why. So that somebody who reads the code
> > > doesn't wonder: why is virtio-ccw not implementing that callback.
> >
> > Right.
> >
> > I am currently thinking instead of making this optional in the
> > core we should make it mandatory, and have transports which do not
> > need to sync have an empty stub with documentation explaining why.
> >
> > Also, do we want to document this sync is explicitly for irq enable/disable?
> > synchronize_irq_enable_disable?
>
> I would not since the transport is not guaranteed to use an interrupt
> for callbacks.
OK but let's then document this in more detail.
More readers will wonder about what is the callback
trying to accomplish, and Halil requested that as well.
For example, let's document why is sync required on enable.
> >
> >
> > > >
> > > > >
> > > > >> ---
> > > > >> 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?
> > > > 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.)
> > >
> > > Sorry I don't understand enough yet. A more verbose documentation on
> > > "virtio_synchronize_vqs - synchronize with virtqueue callbacks" would
> > > surely benefit me. It may be more than enough for a back-belt but it
> > > ain't enough for me to tell what is the callback supposed to accomplish.
> > >
> > > I will have to study this discussion and the code more thoroughly.
> > > Tentatively I side with Jason and Michael in a sense, that I don't
> > > believe virtio-ccw is safe against rough interrupts.
>
> That's my feeling as well.
>
> Thanks
>
> > >
> > > Sorry for the late response. I intend to revisit this on Monday. If
> > > I don't please feel encouraged to ping.
> > >
> > > Regards,
> > > Halil
> >
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()
On Sun, Apr 10, 2022 at 3:51 PM Michael S. Tsirkin wrote:
>
> On Fri, Apr 08, 2022 at 03:03:07PM +0200, Halil Pasic wrote:
> > On Wed, 06 Apr 2022 15:04:32 +0200
> > Cornelia Huck wrote:
> >
> > > On Wed, Apr 06 2022, "Michael S. Tsirkin" 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
> > > >> Cc: Peter Zijlstra
> > > >> Cc: "Paul E. McKenney"
> > > >> Cc: Marc Zyngier
> > > >> Signed-off-by: Jason Wang
> > > >
> > > > Please add implementations at least for ccw and mmio.
> > >
> > > I'm not sure what (if anything) can/should be done for ccw...
> >
> > If nothing needs to be done I would like to have at least a comment in
> > the code that explains why. So that somebody who reads the code
> > doesn't wonder: why is virtio-ccw not implementing that callback.
>
> Right.
>
> I am currently thinking instead of making this optional in the
> core we should make it mandatory, and have transports which do not
> need to sync have an empty stub with documentation explaining why.
>
> Also, do we want to document this sync is explicitly for irq enable/disable?
> synchronize_irq_enable_disable?
I would not since the transport is not guaranteed to use an interrupt
for callbacks.
>
>
> > >
> > > >
> > > >> ---
> > > >> 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?
> > > 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.)
> >
> > Sorry I don't understand enough yet. A more verbose documentation on
> > "virtio_synchronize_vqs - synchronize with virtqueue callbacks" would
> > surely benefit me. It may be more than enough for a back-belt but it
> > ain't enough for me to tell what is the callback supposed to accomplish.
> >
> > I will have to study this discussion and the code more thoroughly.
> > Tentatively I side with Jason and Michael in a sense, that I don't
> > believe virtio-ccw is safe against rough interrupts.
That's my feeling as well.
Thanks
> >
> > Sorry for the late response. I intend to revisit this on Monday. If
> > I don't please feel encouraged to ping.
> >
> > Regards,
> > Halil
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()
On Fri, Apr 08, 2022 at 03:03:07PM +0200, Halil Pasic wrote:
> On Wed, 06 Apr 2022 15:04:32 +0200
> Cornelia Huck wrote:
>
> > On Wed, Apr 06 2022, "Michael S. Tsirkin" 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
> > >> Cc: Peter Zijlstra
> > >> Cc: "Paul E. McKenney"
> > >> Cc: Marc Zyngier
> > >> Signed-off-by: Jason Wang
> > >
> > > Please add implementations at least for ccw and mmio.
> >
> > I'm not sure what (if anything) can/should be done for ccw...
>
> If nothing needs to be done I would like to have at least a comment in
> the code that explains why. So that somebody who reads the code
> doesn't wonder: why is virtio-ccw not implementing that callback.
Right.
I am currently thinking instead of making this optional in the
core we should make it mandatory, and have transports which do not
need to sync have an empty stub with documentation explaining why.
Also, do we want to document this sync is explicitly for irq enable/disable?
synchronize_irq_enable_disable?
> >
> > >
> > >> ---
> > >> 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?
> > 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.)
>
> Sorry I don't understand enough yet. A more verbose documentation on
> "virtio_synchronize_vqs - synchronize with virtqueue callbacks" would
> surely benefit me. It may be more than enough for a back-belt but it
> ain't enough for me to tell what is the callback supposed to accomplish.
>
> I will have to study this discussion and the code more thoroughly.
> Tentatively I side with Jason and Michael in a sense, that I don't
> believe virtio-ccw is safe against rough interrupts.
>
> Sorry for the late response. I intend to revisit this on Monday. If
> I don't please feel encouraged to ping.
>
> Regards,
> Halil
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()
On Wed, 06 Apr 2022 15:04:32 +0200
Cornelia Huck wrote:
> On Wed, Apr 06 2022, "Michael S. Tsirkin" 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
> >> Cc: Peter Zijlstra
> >> Cc: "Paul E. McKenney"
> >> Cc: Marc Zyngier
> >> Signed-off-by: Jason Wang
> >
> > Please add implementations at least for ccw and mmio.
>
> I'm not sure what (if anything) can/should be done for ccw...
If nothing needs to be done I would like to have at least a comment in
the code that explains why. So that somebody who reads the code
doesn't wonder: why is virtio-ccw not implementing that callback.
>
> >
> >> ---
> >> 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?
> 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.)
Sorry I don't understand enough yet. A more verbose documentation on
"virtio_synchronize_vqs - synchronize with virtqueue callbacks" would
surely benefit me. It may be more than enough for a back-belt but it
ain't enough for me to tell what is the callback supposed to accomplish.
I will have to study this discussion and the code more thoroughly.
Tentatively I side with Jason and Michael in a sense, that I don't
believe virtio-ccw is safe against rough interrupts.
Sorry for the late response. I intend to revisit this on Monday. If
I don't please feel encouraged to ping.
Regards,
Halil
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()
On Thu, Apr 7, 2022 at 3:53 PM Cornelia Huck wrote:
>
> On Thu, Apr 07 2022, Jason Wang wrote:
>
> > 在 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" 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
> > Cc: Peter Zijlstra
> > Cc: "Paul E. McKenney"
> > Cc: Marc Zyngier
> > Signed-off-by: Jason Wang
> 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()).
>
> Not quite, adapter interrupts are device-independent, but they are
> relevant for vring interrupts.
>
> That would mean that we would need to synchronize _all_ channel I/O
> interrupts, which looks like a huge hammer. But do we really need that
> at all?
We don't, we only need to synchronize with the vring callbacks, to make sure:
1) the memory changes that is done before this op is visible to the
callbacks that came after this op
2) the memory changes that is done after this op is not visible to
callbacks that came before this op
>
> The last patch in this series seems to be concerned with the "no vring
> interrupts if the device is not ready" case, so it needs to synchronize
> vring interrupts with device reset and setting the device status to
> ready. For virtio-ccw, both reset and setting the status are channel I/O
> operations, i.e. starting a program and waiting for the final device
> interrupt for it, so synchronization (on a device level) is already
> happening in a way. So I'm not sure if any extra synchronization is
> actually needed in this case, but maybe I'm misunderstanding.
>
> Do you have further use cases in mind?
Its goal is to prevent the buggy or malicus device/host from attacking
the driver/guest. One use case is the confidential computing where
guest memory is encrypted and the guest doesn't trust the hypervisor.
In that case, the device can try to raise the interrupt after
request_irq but before DRIVER_OK, which tries to trigger the vq
callbacks when the device is not fully initialized:
request_irq()
virtio_specific_setup() // vq callbacks to be triggered in the middle
virtio_device_ready()
Thanks
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()
On Thu, Apr 07 2022, Jason Wang wrote:
> 在 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" 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
> Cc: Peter Zijlstra
> Cc: "Paul E. McKenney"
> Cc: Marc Zyngier
> Signed-off-by: Jason Wang
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()).
Not quite, adapter interrupts are device-independent, but they are
relevant for vring interrupts.
That would mean that we would need to synchronize _all_ channel I/O
interrupts, which looks like a huge hammer. But do we really need that
at all?
The last patch in this series seems to be concerned with the "no vring
interrupts if the device is not ready" case, so it needs to synchronize
vring interrupts with device reset and setting the device status to
ready. For virtio-ccw, both reset and setting the status are channel I/O
operations, i.e. starting a program and waiting for the final device
interrupt for it, so synchronization (on a device level) is already
happening in a way. So I'm not sure if any extra synchronization is
actually needed in this case, but maybe I'm misunderstanding.
Do you have further use cases in mind?
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()
在 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" 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
Cc: Peter Zijlstra
Cc: "Paul E. McKenney"
Cc: Marc Zyngier
Signed-off-by: Jason Wang
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)
{
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()
On Wed, Apr 06, 2022 at 03:04:32PM +0200, Cornelia Huck wrote:
> On Wed, Apr 06 2022, "Michael S. Tsirkin" 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
> >> Cc: Peter Zijlstra
> >> Cc: "Paul E. McKenney"
> >> Cc: Marc Zyngier
> >> Signed-off-by: Jason Wang
> >
> > 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?
> 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.
> >> /* the notify function used when creating a virt queue */
> >> bool vp_notify(struct virtqueue *vq)
> >> {
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()
On Wed, Apr 06 2022, "Michael S. Tsirkin" 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
>> Cc: Peter Zijlstra
>> Cc: "Paul E. McKenney"
>> Cc: Marc Zyngier
>> Signed-off-by: Jason Wang
>
> 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?
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.)
>> /* the notify function used when creating a virt queue */
>> bool vp_notify(struct virtqueue *vq)
>> {
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()
On Wed, Apr 06, 2022 at 04:35:37PM +0800, Jason Wang wrote:
> This patch implements PCI version of synchronize_vqs().
>
> Cc: Thomas Gleixner
> Cc: Peter Zijlstra
> Cc: "Paul E. McKenney"
> Cc: Marc Zyngier
> Signed-off-by: Jason Wang
Please add implementations at least for ccw and mmio.
> ---
> 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));
> +}
> +
> /* the notify function used when creating a virt queue */
> bool vp_notify(struct virtqueue *vq)
> {
> diff --git a/drivers/virtio/virtio_pci_common.h
> b/drivers/virtio/virtio_pci_common.h
> index eb17a29fc7ef..2b84d5c1b5bc 100644
> --- a/drivers/virtio/virtio_pci_common.h
> +++ b/drivers/virtio/virtio_pci_common.h
> @@ -105,6 +105,8 @@ static struct virtio_pci_device *to_vp_device(struct
> virtio_device *vdev)
> void vp_synchronize_vectors(struct virtio_device *vdev);
> /* the notify function used when creating a virt queue */
> bool vp_notify(struct virtqueue *vq);
> +/* synchronize with callbacks */
> +void vp_synchronize_vqs(struct virtio_device *vdev);
> /* the config->del_vqs() implementation */
> void vp_del_vqs(struct virtio_device *vdev);
> /* the config->find_vqs() implementation */
> diff --git a/drivers/virtio/virtio_pci_legacy.c
> b/drivers/virtio/virtio_pci_legacy.c
> index 6f4e34ce96b8..5a9e62320edc 100644
> --- a/drivers/virtio/virtio_pci_legacy.c
> +++ b/drivers/virtio/virtio_pci_legacy.c
> @@ -192,6 +192,7 @@ static const struct virtio_config_ops
> virtio_pci_config_ops = {
> .reset = vp_reset,
> .find_vqs = vp_find_vqs,
> .del_vqs= vp_del_vqs,
> + .synchronize_vqs = vp_synchronize_vqs,
> .get_features = vp_get_features,
> .finalize_features = vp_finalize_features,
> .bus_name = vp_bus_name,
> diff --git a/drivers/virtio/virtio_pci_modern.c
> b/drivers/virtio/virtio_pci_modern.c
> index a2671a20ef77..584850389855 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -394,6 +394,7 @@ static const struct virtio_config_ops
> virtio_pci_config_nodev_ops = {
> .reset = vp_reset,
> .find_vqs = vp_modern_find_vqs,
> .del_vqs= vp_del_vqs,
> + .synchronize_vqs = vp_synchronize_vqs,
> .get_features = vp_get_features,
> .finalize_features = vp_finalize_features,
> .bus_name = vp_bus_name,
> @@ -411,6 +412,7 @@ static const struct virtio_config_ops
> virtio_pci_config_ops = {
> .reset = vp_reset,
> .find_vqs = vp_modern_find_vqs,
> .del_vqs= vp_del_vqs,
> + .synchronize_vqs = vp_synchronize_vqs,
> .get_features = vp_get_features,
> .finalize_features = vp_finalize_features,
> .bus_name = vp_bus_name,
> --
> 2.25.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH V2 4/5] virtio-pci: implement synchronize_vqs()
This patch implements PCI version of synchronize_vqs().
Cc: Thomas Gleixner
Cc: Peter Zijlstra
Cc: "Paul E. McKenney"
Cc: Marc Zyngier
Signed-off-by: Jason Wang
---
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));
+}
+
/* the notify function used when creating a virt queue */
bool vp_notify(struct virtqueue *vq)
{
diff --git a/drivers/virtio/virtio_pci_common.h
b/drivers/virtio/virtio_pci_common.h
index eb17a29fc7ef..2b84d5c1b5bc 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -105,6 +105,8 @@ static struct virtio_pci_device *to_vp_device(struct
virtio_device *vdev)
void vp_synchronize_vectors(struct virtio_device *vdev);
/* the notify function used when creating a virt queue */
bool vp_notify(struct virtqueue *vq);
+/* synchronize with callbacks */
+void vp_synchronize_vqs(struct virtio_device *vdev);
/* the config->del_vqs() implementation */
void vp_del_vqs(struct virtio_device *vdev);
/* the config->find_vqs() implementation */
diff --git a/drivers/virtio/virtio_pci_legacy.c
b/drivers/virtio/virtio_pci_legacy.c
index 6f4e34ce96b8..5a9e62320edc 100644
--- a/drivers/virtio/virtio_pci_legacy.c
+++ b/drivers/virtio/virtio_pci_legacy.c
@@ -192,6 +192,7 @@ static const struct virtio_config_ops virtio_pci_config_ops
= {
.reset = vp_reset,
.find_vqs = vp_find_vqs,
.del_vqs= vp_del_vqs,
+ .synchronize_vqs = vp_synchronize_vqs,
.get_features = vp_get_features,
.finalize_features = vp_finalize_features,
.bus_name = vp_bus_name,
diff --git a/drivers/virtio/virtio_pci_modern.c
b/drivers/virtio/virtio_pci_modern.c
index a2671a20ef77..584850389855 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -394,6 +394,7 @@ static const struct virtio_config_ops
virtio_pci_config_nodev_ops = {
.reset = vp_reset,
.find_vqs = vp_modern_find_vqs,
.del_vqs= vp_del_vqs,
+ .synchronize_vqs = vp_synchronize_vqs,
.get_features = vp_get_features,
.finalize_features = vp_finalize_features,
.bus_name = vp_bus_name,
@@ -411,6 +412,7 @@ static const struct virtio_config_ops virtio_pci_config_ops
= {
.reset = vp_reset,
.find_vqs = vp_modern_find_vqs,
.del_vqs= vp_del_vqs,
+ .synchronize_vqs = vp_synchronize_vqs,
.get_features = vp_get_features,
.finalize_features = vp_finalize_features,
.bus_name = vp_bus_name,
--
2.25.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
