* [PATCH 0/3] rework on the IRQ hardening of virtio
@ 2022-03-24 8:40 Jason Wang
2022-03-24 8:40 ` [PATCH 1/3] virtio: use virtio_device_ready() in virtio_device_restore() Jason Wang
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Jason Wang @ 2022-03-24 8:40 UTC (permalink / raw)
To: mst, jasowang
Cc: virtualization, linux-kernel, maz, tglx, peterz, sgarzare, keirf
Hi All:
This is a rework on the IRQ hardening for virtio which is done
previously by the following commits are reverted:
9e35276a5344 ("virtio_pci: harden MSI-X interrupts")
080cd7c3ac87 ("virtio-pci: harden INTX interrupts")
The reason is that it depends on the IRQF_NO_AUTOEN which may conflict
with the assumption of the affinity managed IRQ that is used by some
virtio drivers. And what's more, it is only done for virtio-pci but
not other transports.
In this rework, I try to implement a general virtio solution which
borrows the idea of the INTX hardening by introducing a boolean for
virtqueue callback enabling and toggle it in virtio_device_ready()
and virtio_reset_device(). Then vring_interrupt() can simply check and
return early if the driver is not ready.
To unbreak legacy setups that may generate IRQ before DRIVER_OK, a
module parameter is introduced to disable the hardening by
default. The features can then be turned on the setups that the
hardening is needed (e.g the confidential computing and other cases).
Please review.
Thanks
Jason Wang (2):
virtio: use virtio_reset_device() when possible
virtio: harden vring IRQ
Stefano Garzarella (1):
virtio: use virtio_device_ready() in virtio_device_restore()
drivers/virtio/virtio.c | 28 ++++++++++++++++++++++++----
drivers/virtio/virtio_ring.c | 9 ++++++++-
include/linux/virtio.h | 4 ++++
include/linux/virtio_config.h | 25 +++++++++++++++++++++++++
4 files changed, 61 insertions(+), 5 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 1/3] virtio: use virtio_device_ready() in virtio_device_restore() 2022-03-24 8:40 [PATCH 0/3] rework on the IRQ hardening of virtio Jason Wang @ 2022-03-24 8:40 ` Jason Wang 2022-03-24 10:48 ` Michael S. Tsirkin 2022-03-24 8:40 ` [PATCH 2/3] virtio: use virtio_reset_device() when possible Jason Wang 2022-03-24 8:40 ` [PATCH 3/3] virtio: harden vring IRQ Jason Wang 2 siblings, 1 reply; 13+ messages in thread From: Jason Wang @ 2022-03-24 8:40 UTC (permalink / raw) To: mst, jasowang Cc: virtualization, linux-kernel, maz, tglx, peterz, sgarzare, keirf From: Stefano Garzarella <sgarzare@redhat.com> This avoids setting DRIVER_OK twice for those drivers that call virtio_device_ready() in the .restore and it will allows us to do extension on virtio_device_ready() without duplicating codes. Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> Signed-off-by: Jason Wang <jasowang@redhat.com> --- drivers/virtio/virtio.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index 22f15f444f75..75c8d560bbd3 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -526,8 +526,9 @@ int virtio_device_restore(struct virtio_device *dev) goto err; } - /* Finally, tell the device we're all set */ - virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK); + /* If restore didn't do it, mark device DRIVER_OK ourselves. */ + if (!(dev->config->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK)) + virtio_device_ready(dev); virtio_config_enable(dev); -- 2.25.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] virtio: use virtio_device_ready() in virtio_device_restore() 2022-03-24 8:40 ` [PATCH 1/3] virtio: use virtio_device_ready() in virtio_device_restore() Jason Wang @ 2022-03-24 10:48 ` Michael S. Tsirkin 2022-03-24 11:03 ` Stefano Garzarella 0 siblings, 1 reply; 13+ messages in thread From: Michael S. Tsirkin @ 2022-03-24 10:48 UTC (permalink / raw) To: Jason Wang Cc: virtualization, linux-kernel, maz, tglx, peterz, sgarzare, keirf On Thu, Mar 24, 2022 at 04:40:02PM +0800, Jason Wang wrote: > From: Stefano Garzarella <sgarzare@redhat.com> > > This avoids setting DRIVER_OK twice for those drivers that call > virtio_device_ready() in the .restore Is this trying to say it's faster? If yes this one looks like a red herring. Yes we skip a write but we replace it with a read which is not better performance-wise. If we want to optimize this, it is better to just do that inside virtio_add_status: diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index 75c8d560bbd3..cd943c31bdbb 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -161,8 +161,14 @@ static void virtio_config_enable(struct virtio_device *dev) void virtio_add_status(struct virtio_device *dev, unsigned int status) { + unsigned int device_status; + might_sleep(); - dev->config->set_status(dev, dev->config->get_status(dev) | status); + + device_status = dev->config->get_status(dev); + + if (status & ~device_status) + dev->config->set_status(dev, device_status | status); } EXPORT_SYMBOL_GPL(virtio_add_status); > and it will allows us to do > extension on virtio_device_ready() without duplicating codes. > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > drivers/virtio/virtio.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > index 22f15f444f75..75c8d560bbd3 100644 > --- a/drivers/virtio/virtio.c > +++ b/drivers/virtio/virtio.c > @@ -526,8 +526,9 @@ int virtio_device_restore(struct virtio_device *dev) > goto err; > } > > - /* Finally, tell the device we're all set */ > - virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK); > + /* If restore didn't do it, mark device DRIVER_OK ourselves. */ I preferred the original comment, it said why we are doing this, new one repeats what code is doing. > + if (!(dev->config->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK)) > + virtio_device_ready(dev); > > virtio_config_enable(dev); > > -- > 2.25.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] virtio: use virtio_device_ready() in virtio_device_restore() 2022-03-24 10:48 ` Michael S. Tsirkin @ 2022-03-24 11:03 ` Stefano Garzarella 2022-03-24 11:07 ` Michael S. Tsirkin 0 siblings, 1 reply; 13+ messages in thread From: Stefano Garzarella @ 2022-03-24 11:03 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Jason Wang, virtualization, linux-kernel, maz, tglx, peterz, keirf On Thu, Mar 24, 2022 at 06:48:05AM -0400, Michael S. Tsirkin wrote: >On Thu, Mar 24, 2022 at 04:40:02PM +0800, Jason Wang wrote: >> From: Stefano Garzarella <sgarzare@redhat.com> >> >> This avoids setting DRIVER_OK twice for those drivers that call >> virtio_device_ready() in the .restore > >Is this trying to say it's faster? Nope, I mean, when I wrote the original version, I meant to do the same things that we do in virtio_dev_probe() where we called virtio_device_ready() which not only set the state, but also called .enable_cbs callback. Was this a side effect and maybe more compliant with the spec? >If yes this one looks like a red herring. Yes we skip a write but we >replace it with a read which is not better performance-wise. >If we want to optimize this, it is better to just do that inside >virtio_add_status: > > > >diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c >index 75c8d560bbd3..cd943c31bdbb 100644 >--- a/drivers/virtio/virtio.c >+++ b/drivers/virtio/virtio.c >@@ -161,8 +161,14 @@ static void virtio_config_enable(struct virtio_device *dev) > > void virtio_add_status(struct virtio_device *dev, unsigned int status) > { >+ unsigned int device_status; >+ > might_sleep(); >- dev->config->set_status(dev, dev->config->get_status(dev) | status); >+ >+ device_status = dev->config->get_status(dev); >+ >+ if (status & ~device_status) >+ dev->config->set_status(dev, device_status | status); > } > EXPORT_SYMBOL_GPL(virtio_add_status); Could there be a case where we want to set the status again even though the device tells us it's already set? I think not, so I guess it's okay. > > >> and it will allows us to do >> extension on virtio_device_ready() without duplicating codes. >> >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >> Signed-off-by: Jason Wang <jasowang@redhat.com> >> --- >> drivers/virtio/virtio.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c >> index 22f15f444f75..75c8d560bbd3 100644 >> --- a/drivers/virtio/virtio.c >> +++ b/drivers/virtio/virtio.c >> @@ -526,8 +526,9 @@ int virtio_device_restore(struct virtio_device *dev) >> goto err; >> } >> >> - /* Finally, tell the device we're all set */ >> - virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK); >> + /* If restore didn't do it, mark device DRIVER_OK ourselves. */ > >I preferred the original comment, it said why we are doing this, >new one repeats what code is doing. I agree, copy & paste from virtio_dev_probe(). Jason can you fix this patch or should I resend? Thanks, Stefano ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] virtio: use virtio_device_ready() in virtio_device_restore() 2022-03-24 11:03 ` Stefano Garzarella @ 2022-03-24 11:07 ` Michael S. Tsirkin 2022-03-24 11:31 ` Stefano Garzarella 0 siblings, 1 reply; 13+ messages in thread From: Michael S. Tsirkin @ 2022-03-24 11:07 UTC (permalink / raw) To: Stefano Garzarella Cc: Jason Wang, virtualization, linux-kernel, maz, tglx, peterz, keirf On Thu, Mar 24, 2022 at 12:03:07PM +0100, Stefano Garzarella wrote: > On Thu, Mar 24, 2022 at 06:48:05AM -0400, Michael S. Tsirkin wrote: > > On Thu, Mar 24, 2022 at 04:40:02PM +0800, Jason Wang wrote: > > > From: Stefano Garzarella <sgarzare@redhat.com> > > > > > > This avoids setting DRIVER_OK twice for those drivers that call > > > virtio_device_ready() in the .restore > > > > Is this trying to say it's faster? > > Nope, I mean, when I wrote the original version, I meant to do the same > things that we do in virtio_dev_probe() where we called > virtio_device_ready() which not only set the state, but also called > .enable_cbs callback. > > Was this a side effect and maybe more compliant with the spec? Sorry I don't understand the question. it says "avoids setting DRIVER_OK twice" - why is that advantageous and worth calling out in the commit log? > > If yes this one looks like a red herring. Yes we skip a write but we > > replace it with a read which is not better performance-wise. > > If we want to optimize this, it is better to just do that inside > > virtio_add_status: > > > > > > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > > index 75c8d560bbd3..cd943c31bdbb 100644 > > --- a/drivers/virtio/virtio.c > > +++ b/drivers/virtio/virtio.c > > @@ -161,8 +161,14 @@ static void virtio_config_enable(struct virtio_device *dev) > > > > void virtio_add_status(struct virtio_device *dev, unsigned int status) > > { > > + unsigned int device_status; > > + > > might_sleep(); > > - dev->config->set_status(dev, dev->config->get_status(dev) | status); > > + > > + device_status = dev->config->get_status(dev); > > + > > + if (status & ~device_status) > > + dev->config->set_status(dev, device_status | status); > > } > > EXPORT_SYMBOL_GPL(virtio_add_status); > > Could there be a case where we want to set the status again even though the > device tells us it's already set? > > I think not, so I guess it's okay. > > > > > > > > and it will allows us to do > > > extension on virtio_device_ready() without duplicating codes. > > > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > --- > > > drivers/virtio/virtio.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > > > index 22f15f444f75..75c8d560bbd3 100644 > > > --- a/drivers/virtio/virtio.c > > > +++ b/drivers/virtio/virtio.c > > > @@ -526,8 +526,9 @@ int virtio_device_restore(struct virtio_device *dev) > > > goto err; > > > } > > > > > > - /* Finally, tell the device we're all set */ > > > - virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK); > > > + /* If restore didn't do it, mark device DRIVER_OK ourselves. */ > > > > I preferred the original comment, it said why we are doing this, > > new one repeats what code is doing. > > I agree, copy & paste from virtio_dev_probe(). > > Jason can you fix this patch or should I resend? > > Thanks, > Stefano ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] virtio: use virtio_device_ready() in virtio_device_restore() 2022-03-24 11:07 ` Michael S. Tsirkin @ 2022-03-24 11:31 ` Stefano Garzarella 2022-03-25 3:05 ` Jason Wang 0 siblings, 1 reply; 13+ messages in thread From: Stefano Garzarella @ 2022-03-24 11:31 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Jason Wang, virtualization, linux-kernel, maz, tglx, peterz, keirf On Thu, Mar 24, 2022 at 07:07:09AM -0400, Michael S. Tsirkin wrote: >On Thu, Mar 24, 2022 at 12:03:07PM +0100, Stefano Garzarella wrote: >> On Thu, Mar 24, 2022 at 06:48:05AM -0400, Michael S. Tsirkin wrote: >> > On Thu, Mar 24, 2022 at 04:40:02PM +0800, Jason Wang wrote: >> > > From: Stefano Garzarella <sgarzare@redhat.com> >> > > >> > > This avoids setting DRIVER_OK twice for those drivers that call >> > > virtio_device_ready() in the .restore >> > >> > Is this trying to say it's faster? >> >> Nope, I mean, when I wrote the original version, I meant to do the same >> things that we do in virtio_dev_probe() where we called >> virtio_device_ready() which not only set the state, but also called >> .enable_cbs callback. >> >> Was this a side effect and maybe more compliant with the spec? > > >Sorry I don't understand the question. it says "avoids setting DRIVER_OK twice" - >why is that advantageous and worth calling out in the commit log? I just wanted to say that it seems strange to set DRIVER_OK twice if we read the spec. I don't think it's wrong, but weird. Yes, maybe we should rewrite the commit message saying that we want to use virtio_device_ready() everywhere to complete the setup before setting DRIVER_OK so we can do all the necessary operations inside (like in patch 3 or call enable_cbs). Jason rewrote the commit log, so I don't know if he agrees. Thanks, Stefano ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] virtio: use virtio_device_ready() in virtio_device_restore() 2022-03-24 11:31 ` Stefano Garzarella @ 2022-03-25 3:05 ` Jason Wang 2022-03-30 5:22 ` Michael S. Tsirkin 0 siblings, 1 reply; 13+ messages in thread From: Jason Wang @ 2022-03-25 3:05 UTC (permalink / raw) To: Stefano Garzarella, Michael S. Tsirkin Cc: virtualization, linux-kernel, maz, tglx, peterz, keirf 在 2022/3/24 下午7:31, Stefano Garzarella 写道: > On Thu, Mar 24, 2022 at 07:07:09AM -0400, Michael S. Tsirkin wrote: >> On Thu, Mar 24, 2022 at 12:03:07PM +0100, Stefano Garzarella wrote: >>> On Thu, Mar 24, 2022 at 06:48:05AM -0400, Michael S. Tsirkin wrote: >>> > On Thu, Mar 24, 2022 at 04:40:02PM +0800, Jason Wang wrote: >>> > > From: Stefano Garzarella <sgarzare@redhat.com> >>> > > >>> > > This avoids setting DRIVER_OK twice for those drivers that call >>> > > virtio_device_ready() in the .restore >>> > >>> > Is this trying to say it's faster? >>> >>> Nope, I mean, when I wrote the original version, I meant to do the same >>> things that we do in virtio_dev_probe() where we called >>> virtio_device_ready() which not only set the state, but also called >>> .enable_cbs callback. >>> >>> Was this a side effect and maybe more compliant with the spec? >> >> >> Sorry I don't understand the question. it says "avoids setting >> DRIVER_OK twice" - >> why is that advantageous and worth calling out in the commit log? > > I just wanted to say that it seems strange to set DRIVER_OK twice if > we read the spec. I don't think it's wrong, but weird. > > Yes, maybe we should rewrite the commit message saying that we want to > use virtio_device_ready() everywhere to complete the setup before > setting DRIVER_OK so we can do all the necessary operations inside > (like in patch 3 or call enable_cbs). > > Jason rewrote the commit log, so I don't know if he agrees. > > Thanks, > Stefano I agree, I will tweak the log in V2. Thanks ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] virtio: use virtio_device_ready() in virtio_device_restore() 2022-03-25 3:05 ` Jason Wang @ 2022-03-30 5:22 ` Michael S. Tsirkin 0 siblings, 0 replies; 13+ messages in thread From: Michael S. Tsirkin @ 2022-03-30 5:22 UTC (permalink / raw) To: Jason Wang Cc: Stefano Garzarella, virtualization, linux-kernel, maz, tglx, peterz, keirf On Fri, Mar 25, 2022 at 11:05:22AM +0800, Jason Wang wrote: > > 在 2022/3/24 下午7:31, Stefano Garzarella 写道: > > On Thu, Mar 24, 2022 at 07:07:09AM -0400, Michael S. Tsirkin wrote: > > > On Thu, Mar 24, 2022 at 12:03:07PM +0100, Stefano Garzarella wrote: > > > > On Thu, Mar 24, 2022 at 06:48:05AM -0400, Michael S. Tsirkin wrote: > > > > > On Thu, Mar 24, 2022 at 04:40:02PM +0800, Jason Wang wrote: > > > > > > From: Stefano Garzarella <sgarzare@redhat.com> > > > > > > > > > > > > This avoids setting DRIVER_OK twice for those drivers that call > > > > > > virtio_device_ready() in the .restore > > > > > > > > > > Is this trying to say it's faster? > > > > > > > > Nope, I mean, when I wrote the original version, I meant to do the same > > > > things that we do in virtio_dev_probe() where we called > > > > virtio_device_ready() which not only set the state, but also called > > > > .enable_cbs callback. > > > > > > > > Was this a side effect and maybe more compliant with the spec? > > > > > > > > > Sorry I don't understand the question. it says "avoids setting > > > DRIVER_OK twice" - > > > why is that advantageous and worth calling out in the commit log? > > > > I just wanted to say that it seems strange to set DRIVER_OK twice if we > > read the spec. I don't think it's wrong, but weird. > > > > Yes, maybe we should rewrite the commit message saying that we want to > > use virtio_device_ready() everywhere to complete the setup before > > setting DRIVER_OK so we can do all the necessary operations inside (like > > in patch 3 or call enable_cbs). > > > > Jason rewrote the commit log, so I don't know if he agrees. > > > > Thanks, > > Stefano > > > I agree, I will tweak the log in V2. > > Thanks Still waiting for that v2. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] virtio: use virtio_reset_device() when possible 2022-03-24 8:40 [PATCH 0/3] rework on the IRQ hardening of virtio Jason Wang 2022-03-24 8:40 ` [PATCH 1/3] virtio: use virtio_device_ready() in virtio_device_restore() Jason Wang @ 2022-03-24 8:40 ` Jason Wang 2022-03-24 11:04 ` Stefano Garzarella 2022-03-24 8:40 ` [PATCH 3/3] virtio: harden vring IRQ Jason Wang 2 siblings, 1 reply; 13+ messages in thread From: Jason Wang @ 2022-03-24 8:40 UTC (permalink / raw) To: mst, jasowang Cc: virtualization, linux-kernel, maz, tglx, peterz, sgarzare, keirf This allows us to do common extension without duplicating codes. Signed-off-by: Jason Wang <jasowang@redhat.com> --- drivers/virtio/virtio.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index 75c8d560bbd3..8dde44ea044a 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -430,7 +430,7 @@ int register_virtio_device(struct virtio_device *dev) /* We always start by resetting the device, in case a previous * driver messed it up. This also tests that code path a little. */ - dev->config->reset(dev); + virtio_reset_device(dev); /* Acknowledge that we've seen the device. */ virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE); @@ -496,7 +496,7 @@ int virtio_device_restore(struct virtio_device *dev) /* We always start by resetting the device, in case a previous * driver messed it up. */ - dev->config->reset(dev); + virtio_reset_device(dev); /* Acknowledge that we've seen the device. */ virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE); -- 2.25.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] virtio: use virtio_reset_device() when possible 2022-03-24 8:40 ` [PATCH 2/3] virtio: use virtio_reset_device() when possible Jason Wang @ 2022-03-24 11:04 ` Stefano Garzarella 0 siblings, 0 replies; 13+ messages in thread From: Stefano Garzarella @ 2022-03-24 11:04 UTC (permalink / raw) To: Jason Wang; +Cc: mst, virtualization, linux-kernel, maz, tglx, peterz, keirf On Thu, Mar 24, 2022 at 04:40:03PM +0800, Jason Wang wrote: >This allows us to do common extension without duplicating codes. > >Signed-off-by: Jason Wang <jasowang@redhat.com> >--- > drivers/virtio/virtio.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > >diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c >index 75c8d560bbd3..8dde44ea044a 100644 >--- a/drivers/virtio/virtio.c >+++ b/drivers/virtio/virtio.c >@@ -430,7 +430,7 @@ int register_virtio_device(struct virtio_device *dev) > > /* We always start by resetting the device, in case a previous > * driver messed it up. This also tests that code path a little. */ >- dev->config->reset(dev); >+ virtio_reset_device(dev); > > /* Acknowledge that we've seen the device. */ > virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE); >@@ -496,7 +496,7 @@ int virtio_device_restore(struct virtio_device *dev) > > /* We always start by resetting the device, in case a previous > * driver messed it up. */ >- dev->config->reset(dev); >+ virtio_reset_device(dev); > > /* Acknowledge that we've seen the device. */ > virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE); >-- >2.25.1 > Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] virtio: harden vring IRQ 2022-03-24 8:40 [PATCH 0/3] rework on the IRQ hardening of virtio Jason Wang 2022-03-24 8:40 ` [PATCH 1/3] virtio: use virtio_device_ready() in virtio_device_restore() Jason Wang 2022-03-24 8:40 ` [PATCH 2/3] virtio: use virtio_reset_device() when possible Jason Wang @ 2022-03-24 8:40 ` Jason Wang 2022-03-24 11:03 ` Michael S. Tsirkin 2 siblings, 1 reply; 13+ messages in thread From: Jason Wang @ 2022-03-24 8:40 UTC (permalink / raw) To: mst, jasowang Cc: virtualization, linux-kernel, maz, tglx, peterz, sgarzare, keirf This is a rework on the previous IRQ hardening that is done for virtio-pci where several drawbacks were found and were reverted: 1) try to use IRQF_NO_AUTOEN which is not friendly to affinity managed IRQ that is used by some device such as virtio-blk 2) done only for PCI transport In this patch, we tries to borrow the idea from the INTX IRQ hardening in the reverted commit 080cd7c3ac87 ("virtio-pci: harden INTX interrupts") by introducing a global irq_soft_enabled variable for each virtio_device. Then we can to toggle it during virtio_reset_device()/virtio_device_ready(). A synchornize_rcu() is used in virtio_reset_device() to synchronize with the IRQ handlers. In the future, we may provide config_ops for the transport that doesn't use IRQ. With this, vring_interrupt() can return check and early if irq_soft_enabled is false. This lead to smp_load_acquire() to be used but the cost should be acceptable. To avoid breaking legacy device which can send IRQ before DRIVER_OK, a module parameter is introduced to enable the hardening so function hardening is disabled by default. Note that the hardening is only done for vring interrupt since the config interrupt hardening is already done in commit 22b7050a024d7 ("virtio: defer config changed notifications"). But the method that is used by config interrupt can't be reused by the vring interrupt handler because it uses spinlock to do the synchronization which is expensive. Signed-off-by: Jason Wang <jasowang@redhat.com> --- drivers/virtio/virtio.c | 19 +++++++++++++++++++ drivers/virtio/virtio_ring.c | 9 ++++++++- include/linux/virtio.h | 4 ++++ include/linux/virtio_config.h | 25 +++++++++++++++++++++++++ 4 files changed, 56 insertions(+), 1 deletion(-) diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index 8dde44ea044a..85e331efa9cc 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -7,6 +7,12 @@ #include <linux/of.h> #include <uapi/linux/virtio_ids.h> +static bool irq_hardening = false; + +module_param(irq_hardening, bool, 0444); +MODULE_PARM_DESC(irq_hardening, + "Disalbe IRQ software processing when it is not expected"); + /* Unique numbering for virtio devices. */ static DEFINE_IDA(virtio_index_ida); @@ -220,6 +226,15 @@ static int virtio_features_ok(struct virtio_device *dev) * */ void virtio_reset_device(struct virtio_device *dev) { + /* + * The below synchronize_rcu() guarantees that any + * interrupt for this line arriving after + * synchronize_rcu() has completed is guaranteed to see + * irq_soft_enabled == false. + */ + WRITE_ONCE(dev->irq_soft_enabled, false); + synchronize_rcu(); + dev->config->reset(dev); } EXPORT_SYMBOL_GPL(virtio_reset_device); @@ -427,6 +442,10 @@ int register_virtio_device(struct virtio_device *dev) spin_lock_init(&dev->config_lock); dev->config_enabled = false; dev->config_change_pending = false; + dev->irq_soft_check = irq_hardening; + + if (dev->irq_soft_check) + dev_info(&dev->dev, "IRQ hardening is enabled\n"); /* We always start by resetting the device, in case a previous * driver messed it up. This also tests that code path a little. */ diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 962f1477b1fa..0170f8c784d8 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -2144,10 +2144,17 @@ static inline bool more_used(const struct vring_virtqueue *vq) return vq->packed_ring ? more_used_packed(vq) : more_used_split(vq); } -irqreturn_t vring_interrupt(int irq, void *_vq) +irqreturn_t vring_interrupt(int irq, void *v) { + struct virtqueue *_vq = v; + struct virtio_device *vdev = _vq->vdev; struct vring_virtqueue *vq = to_vvq(_vq); + if (!virtio_irq_soft_enabled(vdev)) { + dev_warn_once(&vdev->dev, "virtio vring IRQ raised before DRIVER_OK"); + return IRQ_NONE; + } + if (!more_used(vq)) { pr_debug("virtqueue interrupt with no work for %p\n", vq); return IRQ_NONE; diff --git a/include/linux/virtio.h b/include/linux/virtio.h index 5464f398912a..957d6ad604ac 100644 --- a/include/linux/virtio.h +++ b/include/linux/virtio.h @@ -95,6 +95,8 @@ dma_addr_t virtqueue_get_used_addr(struct virtqueue *vq); * @failed: saved value for VIRTIO_CONFIG_S_FAILED bit (for restore) * @config_enabled: configuration change reporting enabled * @config_change_pending: configuration change reported while disabled + * @irq_soft_check: whether or not to check @irq_soft_enabled + * @irq_soft_enabled: callbacks enabled * @config_lock: protects configuration change reporting * @dev: underlying device. * @id: the device type identification (used to match it with a driver). @@ -109,6 +111,8 @@ struct virtio_device { bool failed; bool config_enabled; bool config_change_pending; + bool irq_soft_check; + bool irq_soft_enabled; spinlock_t config_lock; spinlock_t vqs_list_lock; /* Protects VQs list access */ struct device dev; diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h index dafdc7f48c01..9c1b61f2e525 100644 --- a/include/linux/virtio_config.h +++ b/include/linux/virtio_config.h @@ -174,6 +174,24 @@ static inline bool virtio_has_feature(const struct virtio_device *vdev, return __virtio_test_bit(vdev, fbit); } +/* + * virtio_irq_soft_enabled: whether we can execute callbacks + * @vdev: the device + */ +static inline bool virtio_irq_soft_enabled(const struct virtio_device *vdev) +{ + if (!vdev->irq_soft_check) + return true; + + /* + * Read irq_soft_enabled before reading other device specific + * data. Paried with smp_store_relase() in + * virtio_device_ready() and WRITE_ONCE()/synchronize_rcu() in + * virtio_reset_device(). + */ + return smp_load_acquire(&vdev->irq_soft_enabled); +} + /** * virtio_has_dma_quirk - determine whether this device has the DMA quirk * @vdev: the device @@ -236,6 +254,13 @@ void virtio_device_ready(struct virtio_device *dev) if (dev->config->enable_cbs) dev->config->enable_cbs(dev); + /* + * Commit the driver setup before enabling the virtqueue + * callbacks. Paried with smp_load_acuqire() in + * virtio_irq_soft_enabled() + */ + smp_store_release(&dev->irq_soft_enabled, true); + BUG_ON(status & VIRTIO_CONFIG_S_DRIVER_OK); dev->config->set_status(dev, status | VIRTIO_CONFIG_S_DRIVER_OK); } -- 2.25.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] virtio: harden vring IRQ 2022-03-24 8:40 ` [PATCH 3/3] virtio: harden vring IRQ Jason Wang @ 2022-03-24 11:03 ` Michael S. Tsirkin 2022-03-25 3:04 ` Jason Wang 0 siblings, 1 reply; 13+ messages in thread From: Michael S. Tsirkin @ 2022-03-24 11:03 UTC (permalink / raw) To: Jason Wang Cc: virtualization, linux-kernel, maz, tglx, peterz, sgarzare, keirf On Thu, Mar 24, 2022 at 04:40:04PM +0800, Jason Wang wrote: > This is a rework on the previous IRQ hardening that is done for > virtio-pci where several drawbacks were found and were reverted: > > 1) try to use IRQF_NO_AUTOEN which is not friendly to affinity managed IRQ > that is used by some device such as virtio-blk > 2) done only for PCI transport > > In this patch, we tries to borrow the idea from the INTX IRQ hardening > in the reverted commit 080cd7c3ac87 ("virtio-pci: harden INTX interrupts") > by introducing a global irq_soft_enabled variable for each > virtio_device. Then we can to toggle it during > virtio_reset_device()/virtio_device_ready(). A synchornize_rcu() is > used in virtio_reset_device() to synchronize with the IRQ handlers. In > the future, we may provide config_ops for the transport that doesn't > use IRQ. With this, vring_interrupt() can return check and early if > irq_soft_enabled is false. This lead to smp_load_acquire() to be used > but the cost should be acceptable. Maybe it should be but is it? Can't we use synchronize_irq instead? > > To avoid breaking legacy device which can send IRQ before DRIVER_OK, a > module parameter is introduced to enable the hardening so function > hardening is disabled by default. Which devices are these? How come they send an interrupt before there are any buffers in any queues? > > Note that the hardening is only done for vring interrupt since the > config interrupt hardening is already done in commit 22b7050a024d7 > ("virtio: defer config changed notifications"). But the method that is > used by config interrupt can't be reused by the vring interrupt > handler because it uses spinlock to do the synchronization which is > expensive. > > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > drivers/virtio/virtio.c | 19 +++++++++++++++++++ > drivers/virtio/virtio_ring.c | 9 ++++++++- > include/linux/virtio.h | 4 ++++ > include/linux/virtio_config.h | 25 +++++++++++++++++++++++++ > 4 files changed, 56 insertions(+), 1 deletion(-) > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > index 8dde44ea044a..85e331efa9cc 100644 > --- a/drivers/virtio/virtio.c > +++ b/drivers/virtio/virtio.c > @@ -7,6 +7,12 @@ > #include <linux/of.h> > #include <uapi/linux/virtio_ids.h> > > +static bool irq_hardening = false; > + > +module_param(irq_hardening, bool, 0444); > +MODULE_PARM_DESC(irq_hardening, > + "Disalbe IRQ software processing when it is not expected"); > + > /* Unique numbering for virtio devices. */ > static DEFINE_IDA(virtio_index_ida); > > @@ -220,6 +226,15 @@ static int virtio_features_ok(struct virtio_device *dev) > * */ > void virtio_reset_device(struct virtio_device *dev) > { > + /* > + * The below synchronize_rcu() guarantees that any > + * interrupt for this line arriving after > + * synchronize_rcu() has completed is guaranteed to see > + * irq_soft_enabled == false. News to me I did not know synchronize_rcu has anything to do with interrupts. Did not you intend to use synchronize_irq? I am not even 100% sure synchronize_rcu is by design a memory barrier though it's most likely is ... > + */ > + WRITE_ONCE(dev->irq_soft_enabled, false); > + synchronize_rcu(); > + > dev->config->reset(dev); > } > EXPORT_SYMBOL_GPL(virtio_reset_device); Please add comment explaining where it will be enabled. Also, we *really* don't need to synch if it was already disabled, let's not add useless overhead to the boot sequence. > @@ -427,6 +442,10 @@ int register_virtio_device(struct virtio_device *dev) > spin_lock_init(&dev->config_lock); > dev->config_enabled = false; > dev->config_change_pending = false; > + dev->irq_soft_check = irq_hardening; > + > + if (dev->irq_soft_check) > + dev_info(&dev->dev, "IRQ hardening is enabled\n"); > > /* We always start by resetting the device, in case a previous > * driver messed it up. This also tests that code path a little. */ one of the points of hardening is it's also helpful for buggy devices. this flag defeats the purpose. > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index 962f1477b1fa..0170f8c784d8 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -2144,10 +2144,17 @@ static inline bool more_used(const struct vring_virtqueue *vq) > return vq->packed_ring ? more_used_packed(vq) : more_used_split(vq); > } > > -irqreturn_t vring_interrupt(int irq, void *_vq) > +irqreturn_t vring_interrupt(int irq, void *v) > { > + struct virtqueue *_vq = v; > + struct virtio_device *vdev = _vq->vdev; > struct vring_virtqueue *vq = to_vvq(_vq); > > + if (!virtio_irq_soft_enabled(vdev)) { > + dev_warn_once(&vdev->dev, "virtio vring IRQ raised before DRIVER_OK"); > + return IRQ_NONE; > + } > + > if (!more_used(vq)) { > pr_debug("virtqueue interrupt with no work for %p\n", vq); > return IRQ_NONE; > diff --git a/include/linux/virtio.h b/include/linux/virtio.h > index 5464f398912a..957d6ad604ac 100644 > --- a/include/linux/virtio.h > +++ b/include/linux/virtio.h > @@ -95,6 +95,8 @@ dma_addr_t virtqueue_get_used_addr(struct virtqueue *vq); > * @failed: saved value for VIRTIO_CONFIG_S_FAILED bit (for restore) > * @config_enabled: configuration change reporting enabled > * @config_change_pending: configuration change reported while disabled > + * @irq_soft_check: whether or not to check @irq_soft_enabled > + * @irq_soft_enabled: callbacks enabled > * @config_lock: protects configuration change reporting > * @dev: underlying device. > * @id: the device type identification (used to match it with a driver). > @@ -109,6 +111,8 @@ struct virtio_device { > bool failed; > bool config_enabled; > bool config_change_pending; > + bool irq_soft_check; > + bool irq_soft_enabled; > spinlock_t config_lock; > spinlock_t vqs_list_lock; /* Protects VQs list access */ > struct device dev; > diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h > index dafdc7f48c01..9c1b61f2e525 100644 > --- a/include/linux/virtio_config.h > +++ b/include/linux/virtio_config.h > @@ -174,6 +174,24 @@ static inline bool virtio_has_feature(const struct virtio_device *vdev, > return __virtio_test_bit(vdev, fbit); > } > > +/* > + * virtio_irq_soft_enabled: whether we can execute callbacks > + * @vdev: the device > + */ > +static inline bool virtio_irq_soft_enabled(const struct virtio_device *vdev) > +{ > + if (!vdev->irq_soft_check) > + return true; > + > + /* > + * Read irq_soft_enabled before reading other device specific > + * data. Paried with smp_store_relase() in paired > + * virtio_device_ready() and WRITE_ONCE()/synchronize_rcu() in > + * virtio_reset_device(). > + */ > + return smp_load_acquire(&vdev->irq_soft_enabled); > +} > + > /** > * virtio_has_dma_quirk - determine whether this device has the DMA quirk > * @vdev: the device > @@ -236,6 +254,13 @@ void virtio_device_ready(struct virtio_device *dev) > if (dev->config->enable_cbs) > dev->config->enable_cbs(dev); > > + /* > + * Commit the driver setup before enabling the virtqueue > + * callbacks. Paried with smp_load_acuqire() in > + * virtio_irq_soft_enabled() > + */ > + smp_store_release(&dev->irq_soft_enabled, true); > + > BUG_ON(status & VIRTIO_CONFIG_S_DRIVER_OK); > dev->config->set_status(dev, status | VIRTIO_CONFIG_S_DRIVER_OK); > } > -- > 2.25.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] virtio: harden vring IRQ 2022-03-24 11:03 ` Michael S. Tsirkin @ 2022-03-25 3:04 ` Jason Wang 0 siblings, 0 replies; 13+ messages in thread From: Jason Wang @ 2022-03-25 3:04 UTC (permalink / raw) To: Michael S. Tsirkin Cc: virtualization, linux-kernel, maz, tglx, peterz, sgarzare, keirf 在 2022/3/24 下午7:03, Michael S. Tsirkin 写道: > On Thu, Mar 24, 2022 at 04:40:04PM +0800, Jason Wang wrote: >> This is a rework on the previous IRQ hardening that is done for >> virtio-pci where several drawbacks were found and were reverted: >> >> 1) try to use IRQF_NO_AUTOEN which is not friendly to affinity managed IRQ >> that is used by some device such as virtio-blk >> 2) done only for PCI transport >> >> In this patch, we tries to borrow the idea from the INTX IRQ hardening >> in the reverted commit 080cd7c3ac87 ("virtio-pci: harden INTX interrupts") >> by introducing a global irq_soft_enabled variable for each >> virtio_device. Then we can to toggle it during >> virtio_reset_device()/virtio_device_ready(). A synchornize_rcu() is >> used in virtio_reset_device() to synchronize with the IRQ handlers. In >> the future, we may provide config_ops for the transport that doesn't >> use IRQ. With this, vring_interrupt() can return check and early if >> irq_soft_enabled is false. This lead to smp_load_acquire() to be used >> but the cost should be acceptable. > Maybe it should be but is it? Can't we use synchronize_irq instead? Even if we allow the transport driver to synchornize through synchronize_irq() we still need a check in the vring_interrupt(). We do something like the following previously: if (!READ_ONCE(vp_dev->intx_soft_enabled)) return IRQ_NONE; But it looks like a bug since speculative read can be done before the check where the interrupt handler can't see the uncommitted setup which is done by the driver. > >> To avoid breaking legacy device which can send IRQ before DRIVER_OK, a >> module parameter is introduced to enable the hardening so function >> hardening is disabled by default. > Which devices are these? How come they send an interrupt before there > are any buffers in any queues? I copied this from the commit log for 22b7050a024d7 " This change will also benefit old hypervisors (before 2009) that send interrupts without checking DRIVER_OK: previously, the callback could race with driver-specific initialization. " If this is only for config interrupt, I can remove the above log. > >> Note that the hardening is only done for vring interrupt since the >> config interrupt hardening is already done in commit 22b7050a024d7 >> ("virtio: defer config changed notifications"). But the method that is >> used by config interrupt can't be reused by the vring interrupt >> handler because it uses spinlock to do the synchronization which is >> expensive. >> >> Signed-off-by: Jason Wang <jasowang@redhat.com> > >> --- >> drivers/virtio/virtio.c | 19 +++++++++++++++++++ >> drivers/virtio/virtio_ring.c | 9 ++++++++- >> include/linux/virtio.h | 4 ++++ >> include/linux/virtio_config.h | 25 +++++++++++++++++++++++++ >> 4 files changed, 56 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c >> index 8dde44ea044a..85e331efa9cc 100644 >> --- a/drivers/virtio/virtio.c >> +++ b/drivers/virtio/virtio.c >> @@ -7,6 +7,12 @@ >> #include <linux/of.h> >> #include <uapi/linux/virtio_ids.h> >> >> +static bool irq_hardening = false; >> + >> +module_param(irq_hardening, bool, 0444); >> +MODULE_PARM_DESC(irq_hardening, >> + "Disalbe IRQ software processing when it is not expected"); >> + >> /* Unique numbering for virtio devices. */ >> static DEFINE_IDA(virtio_index_ida); >> >> @@ -220,6 +226,15 @@ static int virtio_features_ok(struct virtio_device *dev) >> * */ >> void virtio_reset_device(struct virtio_device *dev) >> { >> + /* >> + * The below synchronize_rcu() guarantees that any >> + * interrupt for this line arriving after >> + * synchronize_rcu() has completed is guaranteed to see >> + * irq_soft_enabled == false. > News to me I did not know synchronize_rcu has anything to do > with interrupts. Did not you intend to use synchronize_irq? > I am not even 100% sure synchronize_rcu is by design a memory barrier > though it's most likely is ... According to the comment above tree RCU version of synchronize_rcu(): """ * RCU read-side critical sections are delimited by rcu_read_lock() * and rcu_read_unlock(), and may be nested. In addition, but only in * v5.0 and later, regions of code across which interrupts, preemption, * or softirqs have been disabled also serve as RCU read-side critical * sections. This includes hardware interrupt handlers, softirq handlers, * and NMI handlers. """ So interrupt handlers are treated as read-side critical sections. And it has the comment for explain the barrier: """ * Note that this guarantee implies further memory-ordering guarantees. * On systems with more than one CPU, when synchronize_rcu() returns, * each CPU is guaranteed to have executed a full memory barrier since * the end of its last RCU read-side critical section whose beginning * preceded the call to synchronize_rcu(). In addition, each CPU having """ So on SMP it provides a full barrier. And for UP/tiny RCU we don't need the barrier, if the interrupt come after WRITE_ONCE() it will see the irq_soft_enabled as false. > >> + */ >> + WRITE_ONCE(dev->irq_soft_enabled, false); >> + synchronize_rcu(); >> + >> dev->config->reset(dev); >> } >> EXPORT_SYMBOL_GPL(virtio_reset_device); > Please add comment explaining where it will be enabled. > Also, we *really* don't need to synch if it was already disabled, > let's not add useless overhead to the boot sequence. Ok. > > >> @@ -427,6 +442,10 @@ int register_virtio_device(struct virtio_device *dev) >> spin_lock_init(&dev->config_lock); >> dev->config_enabled = false; >> dev->config_change_pending = false; >> + dev->irq_soft_check = irq_hardening; >> + >> + if (dev->irq_soft_check) >> + dev_info(&dev->dev, "IRQ hardening is enabled\n"); >> >> /* We always start by resetting the device, in case a previous >> * driver messed it up. This also tests that code path a little. */ > one of the points of hardening is it's also helpful for buggy > devices. this flag defeats the purpose. Do you mean: 1) we need something like config_enable? This seems not easy to be implemented without obvious overhead, mainly the synchronize with the interrupt handlers 2) enable this by default, so I don't object, but this may have some risk for old hypervisors > >> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c >> index 962f1477b1fa..0170f8c784d8 100644 >> --- a/drivers/virtio/virtio_ring.c >> +++ b/drivers/virtio/virtio_ring.c >> @@ -2144,10 +2144,17 @@ static inline bool more_used(const struct vring_virtqueue *vq) >> return vq->packed_ring ? more_used_packed(vq) : more_used_split(vq); >> } >> >> -irqreturn_t vring_interrupt(int irq, void *_vq) >> +irqreturn_t vring_interrupt(int irq, void *v) >> { >> + struct virtqueue *_vq = v; >> + struct virtio_device *vdev = _vq->vdev; >> struct vring_virtqueue *vq = to_vvq(_vq); >> >> + if (!virtio_irq_soft_enabled(vdev)) { >> + dev_warn_once(&vdev->dev, "virtio vring IRQ raised before DRIVER_OK"); >> + return IRQ_NONE; >> + } >> + >> if (!more_used(vq)) { >> pr_debug("virtqueue interrupt with no work for %p\n", vq); >> return IRQ_NONE; >> diff --git a/include/linux/virtio.h b/include/linux/virtio.h >> index 5464f398912a..957d6ad604ac 100644 >> --- a/include/linux/virtio.h >> +++ b/include/linux/virtio.h >> @@ -95,6 +95,8 @@ dma_addr_t virtqueue_get_used_addr(struct virtqueue *vq); >> * @failed: saved value for VIRTIO_CONFIG_S_FAILED bit (for restore) >> * @config_enabled: configuration change reporting enabled >> * @config_change_pending: configuration change reported while disabled >> + * @irq_soft_check: whether or not to check @irq_soft_enabled >> + * @irq_soft_enabled: callbacks enabled >> * @config_lock: protects configuration change reporting >> * @dev: underlying device. >> * @id: the device type identification (used to match it with a driver). >> @@ -109,6 +111,8 @@ struct virtio_device { >> bool failed; >> bool config_enabled; >> bool config_change_pending; >> + bool irq_soft_check; >> + bool irq_soft_enabled; >> spinlock_t config_lock; >> spinlock_t vqs_list_lock; /* Protects VQs list access */ >> struct device dev; >> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h >> index dafdc7f48c01..9c1b61f2e525 100644 >> --- a/include/linux/virtio_config.h >> +++ b/include/linux/virtio_config.h >> @@ -174,6 +174,24 @@ static inline bool virtio_has_feature(const struct virtio_device *vdev, >> return __virtio_test_bit(vdev, fbit); >> } >> >> +/* >> + * virtio_irq_soft_enabled: whether we can execute callbacks >> + * @vdev: the device >> + */ >> +static inline bool virtio_irq_soft_enabled(const struct virtio_device *vdev) >> +{ >> + if (!vdev->irq_soft_check) >> + return true; >> + >> + /* >> + * Read irq_soft_enabled before reading other device specific >> + * data. Paried with smp_store_relase() in > paired Will fix. Thanks > >> + * virtio_device_ready() and WRITE_ONCE()/synchronize_rcu() in >> + * virtio_reset_device(). >> + */ >> + return smp_load_acquire(&vdev->irq_soft_enabled); >> +} >> + >> /** >> * virtio_has_dma_quirk - determine whether this device has the DMA quirk >> * @vdev: the device >> @@ -236,6 +254,13 @@ void virtio_device_ready(struct virtio_device *dev) >> if (dev->config->enable_cbs) >> dev->config->enable_cbs(dev); >> >> + /* >> + * Commit the driver setup before enabling the virtqueue >> + * callbacks. Paried with smp_load_acuqire() in >> + * virtio_irq_soft_enabled() >> + */ >> + smp_store_release(&dev->irq_soft_enabled, true); >> + >> BUG_ON(status & VIRTIO_CONFIG_S_DRIVER_OK); >> dev->config->set_status(dev, status | VIRTIO_CONFIG_S_DRIVER_OK); >> } >> -- >> 2.25.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-03-30 5:22 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-24 8:40 [PATCH 0/3] rework on the IRQ hardening of virtio Jason Wang 2022-03-24 8:40 ` [PATCH 1/3] virtio: use virtio_device_ready() in virtio_device_restore() Jason Wang 2022-03-24 10:48 ` Michael S. Tsirkin 2022-03-24 11:03 ` Stefano Garzarella 2022-03-24 11:07 ` Michael S. Tsirkin 2022-03-24 11:31 ` Stefano Garzarella 2022-03-25 3:05 ` Jason Wang 2022-03-30 5:22 ` Michael S. Tsirkin 2022-03-24 8:40 ` [PATCH 2/3] virtio: use virtio_reset_device() when possible Jason Wang 2022-03-24 11:04 ` Stefano Garzarella 2022-03-24 8:40 ` [PATCH 3/3] virtio: harden vring IRQ Jason Wang 2022-03-24 11:03 ` Michael S. Tsirkin 2022-03-25 3:04 ` Jason Wang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).