Re: [Qemu-devel] [PATCH 2/3] virtio: access ISR atomically

[Paolo Bonzini]

On 15/11/2016 16:03, Christian Borntraeger wrote:
> On 11/15/2016 02:46 PM, Paolo Bonzini wrote:
>> This will be needed once dataplane will be able to set it outside
>> the big QEMU lock.
>>
>> Signed-off-by: Paolo Bonzini <address@hidden>
> 
> This is fixed by the followup patch, but this patch alone gives me
> 
> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c: In function 
> ‘virtio_notify_config’:
> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1375:20: error: ‘vq’ undeclared 
> (first use in this function)
>      virtio_set_isr(vq->vdev, 0x3);
>                    ^
> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1375:20: note: each undeclared 
> identifier is reported only once for each function it appears in
> /home/cborntra/REPOS/qemu/rules.mak:60: recipe for target 
> 'hw/virtio/virtio.o' failed
> make[1]: *** [hw/virtio/virtio.o] Error 1
> make[1]: *** Waiting for unfinished jobs....

Oops, will post v2.

Paolo

> 
>> ---
>>  hw/virtio/virtio-mmio.c |  6 +++---
>>  hw/virtio/virtio-pci.c  |  9 +++------
>>  hw/virtio/virtio.c      | 18 +++++++++++++-----
>>  3 files changed, 19 insertions(+), 14 deletions(-)
>>
>> 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 62001b4..d5e99b0 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:
>> @@ -1377,9 +1376,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 89b0b80..35255ad 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(vq->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);
>>
>