| Message ID | 20161116180551.9611-3-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Nov 16, 2016 at 07:05:50PM +0100, Paolo Bonzini wrote: > @@ -1318,10 +1318,18 @@ void virtio_del_queue(VirtIODevice *vdev, int n) > vdev->vq[n].vring.num_default = 0; > } > > +static void virtio_set_isr(VirtIODevice *vdev, int value) > +{ > + uint8_t old = atomic_read(&vdev->isr); > + if ((old & value) != value) { > + atomic_or(&vdev->isr, value); > + } > +} > + Paolo, can you pls comment on why is it done like this, as opposed to a single atomic_or?
----- Original Message ----- > From: "Michael S. Tsirkin" <mst@redhat.com> > To: "Paolo Bonzini" <pbonzini@redhat.com> > Cc: qemu-devel@nongnu.org, "alex williamson" <alex.williamson@redhat.com>, borntraeger@de.ibm.com, felipe@nutanix.com > Sent: Thursday, November 17, 2016 6:55:50 PM > Subject: Re: [PATCH 2/3] virtio: access ISR atomically > > On Wed, Nov 16, 2016 at 07:05:50PM +0100, Paolo Bonzini wrote: > > @@ -1318,10 +1318,18 @@ void virtio_del_queue(VirtIODevice *vdev, int n) > > vdev->vq[n].vring.num_default = 0; > > } > > > > +static void virtio_set_isr(VirtIODevice *vdev, int value) > > +{ > > + uint8_t old = atomic_read(&vdev->isr); > > + if ((old & value) != value) { > > + atomic_or(&vdev->isr, value); > > + } > > +} > > + > > Paolo, can you pls comment on why is it done like this, > as opposed to a single atomic_or? To avoid cacheline bouncing in the common case where MSI is active but the host doesn't read ISR. Paolo
On Thu, Nov 17, 2016 at 02:49:58PM -0500, Paolo Bonzini wrote: > > > ----- Original Message ----- > > From: "Michael S. Tsirkin" <mst@redhat.com> > > To: "Paolo Bonzini" <pbonzini@redhat.com> > > Cc: qemu-devel@nongnu.org, "alex williamson" <alex.williamson@redhat.com>, borntraeger@de.ibm.com, felipe@nutanix.com > > Sent: Thursday, November 17, 2016 6:55:50 PM > > Subject: Re: [PATCH 2/3] virtio: access ISR atomically > > > > On Wed, Nov 16, 2016 at 07:05:50PM +0100, Paolo Bonzini wrote: > > > @@ -1318,10 +1318,18 @@ void virtio_del_queue(VirtIODevice *vdev, int n) > > > vdev->vq[n].vring.num_default = 0; > > > } > > > > > > +static void virtio_set_isr(VirtIODevice *vdev, int value) > > > +{ > > > + uint8_t old = atomic_read(&vdev->isr); > > > + if ((old & value) != value) { > > > + atomic_or(&vdev->isr, value); > > > + } > > > +} > > > + > > > > Paolo, can you pls comment on why is it done like this, > > as opposed to a single atomic_or? > > To avoid cacheline bouncing in the common case where MSI is active > but the host doesn't read ISR. > > Paolo You mean the guest does not read ISR? And so this helps keep the field read-mostly for all CPUs, sharing the cache line. OK but this is worth documenting.
----- Original Message ----- > From: "Michael S. Tsirkin" <mst@redhat.com> > To: "Paolo Bonzini" <pbonzini@redhat.com> > Cc: qemu-devel@nongnu.org, "alex williamson" <alex.williamson@redhat.com>, borntraeger@de.ibm.com, felipe@nutanix.com > Sent: Thursday, November 17, 2016 11:33:24 PM > Subject: Re: [PATCH 2/3] virtio: access ISR atomically > > On Thu, Nov 17, 2016 at 02:49:58PM -0500, Paolo Bonzini wrote: > > > > > > ----- Original Message ----- > > > From: "Michael S. Tsirkin" <mst@redhat.com> > > > To: "Paolo Bonzini" <pbonzini@redhat.com> > > > Cc: qemu-devel@nongnu.org, "alex williamson" > > > <alex.williamson@redhat.com>, borntraeger@de.ibm.com, felipe@nutanix.com > > > Sent: Thursday, November 17, 2016 6:55:50 PM > > > Subject: Re: [PATCH 2/3] virtio: access ISR atomically > > > > > > On Wed, Nov 16, 2016 at 07:05:50PM +0100, Paolo Bonzini wrote: > > > > @@ -1318,10 +1318,18 @@ void virtio_del_queue(VirtIODevice *vdev, int > > > > n) > > > > vdev->vq[n].vring.num_default = 0; > > > > } > > > > > > > > +static void virtio_set_isr(VirtIODevice *vdev, int value) > > > > +{ > > > > + uint8_t old = atomic_read(&vdev->isr); > > > > + if ((old & value) != value) { > > > > + atomic_or(&vdev->isr, value); > > > > + } > > > > +} > > > > + > > > > > > Paolo, can you pls comment on why is it done like this, > > > as opposed to a single atomic_or? > > > > To avoid cacheline bouncing in the common case where MSI is active > > but the host doesn't read ISR. > > You mean the guest does not read ISR? > And so this helps keep the field read-mostly for all CPUs, > sharing the cache line. OK but this is worth documenting. Yes, v3 on the way. Paolo
diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c index a30270f..17412cb 100644 --- a/hw/virtio/virtio-mmio.c +++ b/hw/virtio/virtio-mmio.c @@ -191,7 +191,7 @@ static uint64_t virtio_mmio_read(void *opaque, hwaddr offset, unsigned size) return virtio_queue_get_addr(vdev, vdev->queue_sel) >> proxy->guest_page_shift; case VIRTIO_MMIO_INTERRUPTSTATUS: - return vdev->isr; + return atomic_read(&vdev->isr); case VIRTIO_MMIO_STATUS: return vdev->status; case VIRTIO_MMIO_HOSTFEATURESSEL: @@ -299,7 +299,7 @@ static void virtio_mmio_write(void *opaque, hwaddr offset, uint64_t value, } break; case VIRTIO_MMIO_INTERRUPTACK: - vdev->isr &= ~value; + atomic_and(&vdev->isr, ~value); virtio_update_irq(vdev); break; case VIRTIO_MMIO_STATUS: @@ -347,7 +347,7 @@ static void virtio_mmio_update_irq(DeviceState *opaque, uint16_t vector) if (!vdev) { return; } - level = (vdev->isr != 0); + level = (atomic_read(&vdev->isr) != 0); DPRINTF("virtio_mmio setting IRQ %d\n", level); qemu_set_irq(proxy->irq, level); } diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index 97b32fe..521ba0b 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -73,7 +73,7 @@ static void virtio_pci_notify(DeviceState *d, uint16_t vector) msix_notify(&proxy->pci_dev, vector); else { VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); - pci_set_irq(&proxy->pci_dev, vdev->isr & 1); + pci_set_irq(&proxy->pci_dev, atomic_read(&vdev->isr) & 1); } } @@ -449,8 +449,7 @@ static uint32_t virtio_ioport_read(VirtIOPCIProxy *proxy, uint32_t addr) break; case VIRTIO_PCI_ISR: /* reading from the ISR also clears it. */ - ret = vdev->isr; - vdev->isr = 0; + ret = atomic_xchg(&vdev->isr, 0); pci_irq_deassert(&proxy->pci_dev); break; case VIRTIO_MSI_CONFIG_VECTOR: @@ -1379,9 +1378,7 @@ static uint64_t virtio_pci_isr_read(void *opaque, hwaddr addr, { VirtIOPCIProxy *proxy = opaque; VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); - uint64_t val = vdev->isr; - - vdev->isr = 0; + uint64_t val = atomic_xchg(&vdev->isr, 0); pci_irq_deassert(&proxy->pci_dev); return val; diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index b7d5828..ecf13bd 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -945,7 +945,7 @@ void virtio_reset(void *opaque) vdev->guest_features = 0; vdev->queue_sel = 0; vdev->status = 0; - vdev->isr = 0; + atomic_set(&vdev->isr, 0); vdev->config_vector = VIRTIO_NO_VECTOR; virtio_notify_vector(vdev, vdev->config_vector); @@ -1318,10 +1318,18 @@ void virtio_del_queue(VirtIODevice *vdev, int n) vdev->vq[n].vring.num_default = 0; } +static void virtio_set_isr(VirtIODevice *vdev, int value) +{ + uint8_t old = atomic_read(&vdev->isr); + if ((old & value) != value) { + atomic_or(&vdev->isr, value); + } +} + void virtio_irq(VirtQueue *vq) { trace_virtio_irq(vq); - vq->vdev->isr |= 0x01; + virtio_set_isr(vq->vdev, 0x1); virtio_notify_vector(vq->vdev, vq->vector); } @@ -1355,7 +1363,7 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq) } trace_virtio_notify(vdev, vq); - vdev->isr |= 0x01; + virtio_set_isr(vq->vdev, 0x1); virtio_notify_vector(vdev, vq->vector); } @@ -1364,7 +1372,7 @@ void virtio_notify_config(VirtIODevice *vdev) if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) return; - vdev->isr |= 0x03; + virtio_set_isr(vdev, 0x3); vdev->generation++; virtio_notify_vector(vdev, vdev->config_vector); } @@ -1895,7 +1903,7 @@ void virtio_init(VirtIODevice *vdev, const char *name, vdev->device_id = device_id; vdev->status = 0; - vdev->isr = 0; + atomic_set(&vdev->isr, 0); vdev->queue_sel = 0; vdev->config_vector = VIRTIO_NO_VECTOR; vdev->vq = g_malloc0(sizeof(VirtQueue) * VIRTIO_QUEUE_MAX);
This will be needed once dataplane will be able to set it outside the big QEMU lock. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- v1->v2: squash syntax error fix from patch 3 [Christian] hw/virtio/virtio-mmio.c | 6 +++--- hw/virtio/virtio-pci.c | 9 +++------ hw/virtio/virtio.c | 18 +++++++++++++----- 3 files changed, 19 insertions(+), 14 deletions(-)