Re: [PATCH v6 4/4] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded

[Thomas Huth]

On 05/02/2023 05.07, Alexander Bulekov wrote:

This protects devices from bh->mmio reentrancy issues.

Reviewed-by: Darren Kenny 
Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Alexander Bulekov 
---

...

diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
index 65c4979c3c..f077c1b255 100644
--- a/hw/9pfs/xen-9p-backend.c
+++ b/hw/9pfs/xen-9p-backend.c
@@ -441,7 +441,9 @@ static int xen_9pfs_connect(struct XenLegacyDevice *xendev)
  xen_9pdev->rings[i].ring.out = xen_9pdev->rings[i].data +
 XEN_FLEX_RING_SIZE(ring_order);
  
-xen_9pdev->rings[i].bh = qemu_bh_new(xen_9pfs_bh, _9pdev->rings[i]);

+xen_9pdev->rings[i].bh = qemu_bh_new_guarded(xen_9pfs_bh,
+ _9pdev->rings[i],
+ 
(xen_9pdev)->mem_reentrancy_guard);


xen_9pdev is not derived from DeviceState, so you must not cast it with 
DEVICE().



diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 7ce001cacd..37091150cb 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1508,7 +1508,8 @@ static void ahci_cmd_done(const IDEDMA *dma)
  ahci_write_fis_d2h(ad);
  
  if (ad->port_regs.cmd_issue && !ad->check_bh) {

-ad->check_bh = qemu_bh_new(ahci_check_cmd_bh, ad);
+ad->check_bh = qemu_bh_new_guarded(ahci_check_cmd_bh, ad,
+   (ad)->mem_reentrancy_guard);
  qemu_bh_schedule(ad->check_bh);
  }
  }


Dito - ad is not derived from DeviceState, so you cannot use DEVICE() here.

(This was causing the crash in the macOS CI job)


diff --git a/hw/ide/core.c b/hw/ide/core.c
index 5d1039378f..8c8d1a8ec2 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -519,7 +519,8 @@ BlockAIOCB *ide_issue_trim(
  
  iocb = blk_aio_get(_aiocb_info, s->blk, cb, cb_opaque);

  iocb->s = s;
-iocb->bh = qemu_bh_new(ide_trim_bh_cb, iocb);
+iocb->bh = qemu_bh_new_guarded(ide_trim_bh_cb, iocb,
+   (s)->mem_reentrancy_guard);


IDEState s is also not directly derived from DeviceState. Not sure, but 
maybe you can get to the device here in a similar way that is done in 
ide_identify() :


 IDEDevice *dev = s->unit ? s->bus->slave : s->bus->master;

?


diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 746f07c4d2..309cebacc6 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -908,8 +908,9 @@ static void virtio_balloon_device_realize(DeviceState *dev, 
Error **errp)
  precopy_add_notifier(>free_page_hint_notify);
  
  object_ref(OBJECT(s->iothread));

-s->free_page_bh = aio_bh_new(iothread_get_aio_context(s->iothread),
- virtio_ballloon_get_free_page_hints, s);
+s->free_page_bh = 
aio_bh_new_guarded(iothread_get_aio_context(s->iothread),
+ 
virtio_ballloon_get_free_page_hints, s,
+ (s)->mem_reentrancy_guard);


You could use "dev" instead of "s" here to get rid of the DEVICE() cast.

The remaining changes look fine to me.

 Thomas

Re: [PATCH v6 4/4] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded

[Paul Durrant]

On 01/03/2023 20:54, Michael S. Tsirkin wrote:

On Sat, Feb 04, 2023 at 11:07:37PM -0500, Alexander Bulekov wrote:

This protects devices from bh->mmio reentrancy issues.

Reviewed-by: Darren Kenny 
Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Alexander Bulekov 


Reviewed-by: Michael S. Tsirkin 



Xen parts...

Reviewed-by: Paul Durrant 

Re: [PATCH v6 4/4] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded

[Michael S. Tsirkin]

On Sat, Feb 04, 2023 at 11:07:37PM -0500, Alexander Bulekov wrote:
> This protects devices from bh->mmio reentrancy issues.
> 
> Reviewed-by: Darren Kenny 
> Reviewed-by: Stefan Hajnoczi 
> Signed-off-by: Alexander Bulekov 

Reviewed-by: Michael S. Tsirkin 

> ---
>  hw/9pfs/xen-9p-backend.c| 4 +++-
>  hw/block/dataplane/virtio-blk.c | 3 ++-
>  hw/block/dataplane/xen-block.c  | 5 +++--
>  hw/char/virtio-serial-bus.c | 3 ++-
>  hw/display/qxl.c| 9 ++---
>  hw/display/virtio-gpu.c | 6 --
>  hw/ide/ahci.c   | 3 ++-
>  hw/ide/core.c   | 3 ++-
>  hw/misc/imx_rngc.c  | 6 --
>  hw/misc/macio/mac_dbdma.c   | 2 +-
>  hw/net/virtio-net.c | 3 ++-
>  hw/nvme/ctrl.c  | 6 --
>  hw/scsi/mptsas.c| 3 ++-
>  hw/scsi/scsi-bus.c  | 3 ++-
>  hw/scsi/vmw_pvscsi.c| 3 ++-
>  hw/usb/dev-uas.c| 3 ++-
>  hw/usb/hcd-dwc2.c   | 3 ++-
>  hw/usb/hcd-ehci.c   | 3 ++-
>  hw/usb/hcd-uhci.c   | 2 +-
>  hw/usb/host-libusb.c| 6 --
>  hw/usb/redirect.c   | 6 --
>  hw/usb/xen-usb.c| 3 ++-
>  hw/virtio/virtio-balloon.c  | 5 +++--
>  hw/virtio/virtio-crypto.c   | 3 ++-
>  24 files changed, 63 insertions(+), 33 deletions(-)
> 
> diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
> index 65c4979c3c..f077c1b255 100644
> --- a/hw/9pfs/xen-9p-backend.c
> +++ b/hw/9pfs/xen-9p-backend.c
> @@ -441,7 +441,9 @@ static int xen_9pfs_connect(struct XenLegacyDevice 
> *xendev)
>  xen_9pdev->rings[i].ring.out = xen_9pdev->rings[i].data +
> XEN_FLEX_RING_SIZE(ring_order);
>  
> -xen_9pdev->rings[i].bh = qemu_bh_new(xen_9pfs_bh, 
> _9pdev->rings[i]);
> +xen_9pdev->rings[i].bh = qemu_bh_new_guarded(xen_9pfs_bh,
> + _9pdev->rings[i],
> + 
> (xen_9pdev)->mem_reentrancy_guard);
>  xen_9pdev->rings[i].out_cons = 0;
>  xen_9pdev->rings[i].out_size = 0;
>  xen_9pdev->rings[i].inprogress = false;
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index b28d81737e..a6202997ee 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -127,7 +127,8 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, 
> VirtIOBlkConf *conf,
>  } else {
>  s->ctx = qemu_get_aio_context();
>  }
> -s->bh = aio_bh_new(s->ctx, notify_guest_bh, s);
> +s->bh = aio_bh_new_guarded(s->ctx, notify_guest_bh, s,
> +   (vdev)->mem_reentrancy_guard);
>  s->batch_notify_vqs = bitmap_new(conf->num_queues);
>  
>  *dataplane = s;
> diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c
> index 2785b9e849..e31806b317 100644
> --- a/hw/block/dataplane/xen-block.c
> +++ b/hw/block/dataplane/xen-block.c
> @@ -632,8 +632,9 @@ XenBlockDataPlane *xen_block_dataplane_create(XenDevice 
> *xendev,
>  } else {
>  dataplane->ctx = qemu_get_aio_context();
>  }
> -dataplane->bh = aio_bh_new(dataplane->ctx, xen_block_dataplane_bh,
> -   dataplane);
> +dataplane->bh = aio_bh_new_guarded(dataplane->ctx, 
> xen_block_dataplane_bh,
> +   dataplane,
> +   
> (xendev)->mem_reentrancy_guard);
>  
>  return dataplane;
>  }
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index 7d4601cb5d..dd619f0731 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -985,7 +985,8 @@ static void virtser_port_device_realize(DeviceState *dev, 
> Error **errp)
>  return;
>  }
>  
> -port->bh = qemu_bh_new(flush_queued_data_bh, port);
> +port->bh = qemu_bh_new_guarded(flush_queued_data_bh, port,
> +   >mem_reentrancy_guard);
>  port->elem = NULL;
>  }
>  
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index ec712d3ca2..c0460c4ef1 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -2201,11 +2201,14 @@ static void qxl_realize_common(PCIQXLDevice *qxl, 
> Error **errp)
>  
>  qemu_add_vm_change_state_handler(qxl_vm_change_state_handler, qxl);
>  
> -qxl->update_irq = qemu_bh_new(qxl_update_irq_bh, qxl);
> +qxl->update_irq = qemu_bh_new_guarded(qxl_update_irq_bh, qxl,
> +  
> (qxl)->mem_reentrancy_guard);
>  qxl_reset_state(qxl);
>  
> -qxl->update_area_bh = qemu_bh_new(qxl_render_update_area_bh, qxl);
> -qxl->ssd.cursor_bh = qemu_bh_new(qemu_spice_cursor_refresh_bh, 
> >ssd);
> +qxl->update_area_bh = qemu_bh_new_guarded(qxl_render_update_area_bh, qxl,
> +  
>