[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 24/38] ivshmem: Propagate errors through ivshmem
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 24/38] ivshmem: Propagate errors through ivshmem_recv_setup() |
Date: |
Wed, 02 Mar 2016 20:35:17 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Marc-André Lureau <address@hidden> writes:
> On Mon, Feb 29, 2016 at 7:40 PM, Markus Armbruster <address@hidden> wrote:
>> This kills off the funny state described in the previous commit.
>>
>> Simplify ivshmem_io_read() accordingly, and update documentation.
>>
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>> docs/specs/ivshmem-spec.txt | 20 ++++----
>> hw/misc/ivshmem.c | 121
>> +++++++++++++++++++++++++++-----------------
>> qemu-doc.texi | 9 +---
>> 3 files changed, 87 insertions(+), 63 deletions(-)
>>
>> diff --git a/docs/specs/ivshmem-spec.txt b/docs/specs/ivshmem-spec.txt
>> index 4fc6f37..3eb8c97 100644
>> --- a/docs/specs/ivshmem-spec.txt
>> +++ b/docs/specs/ivshmem-spec.txt
>> @@ -62,11 +62,11 @@ There are two ways to use this device:
>> likely want to write a kernel driver to handle interrupts. Requires
>> the device to be configured for interrupts, obviously.
>>
>> -If the device is configured for interrupts, BAR2 is initially invalid.
>> -It becomes safely accessible only after the ivshmem server provided
>> -the shared memory. Guest software should wait for the IVPosition
>> -register (described below) to become non-negative before accessing
>> -BAR2.
>> +Before QEMU 2.6.0, BAR2 can initially be invalid if the device is
>> +configured for interrupts. It becomes safely accessible only after
>> +the ivshmem server provided the shared memory. Guest software should
>> +wait for the IVPosition register (described below) to become
>> +non-negative before accessing BAR2.
>>
>> The device is not capable to tell guest software whether it is
>> configured for interrupts.
>> @@ -82,7 +82,7 @@ BAR 0 contains the following registers:
>> 4 4 read/write 0 Interrupt Status
>> bit 0: peer interrupt
>> bit 1..31: reserved
>> - 8 4 read-only 0 or -1 IVPosition
>> + 8 4 read-only 0 or ID IVPosition
>> 12 4 write-only N/A Doorbell
>> bit 0..15: vector
>> bit 16..31: peer ID
>> @@ -100,12 +100,14 @@ when an interrupt request from a peer is received.
>> Reading the
>> register clears it.
>>
>> IVPosition Register: if the device is not configured for interrupts,
>> -this is zero. Else, it's -1 for a short while after reset, then
>> -changes to the device's ID (between 0 and 65535).
>> +this is zero. Else, it is the device's ID (between 0 and 65535).
>> +
>> +Before QEMU 2.6.0, the register may read -1 for a short while after
>> +reset.
>>
>> There is no good way for software to find out whether the device is
>> configured for interrupts. A positive IVPosition means interrupts,
>> -but zero could be either. The initial -1 cannot be reliably observed.
>> +but zero could be either.
>>
>> Doorbell Register: writing this register requests to interrupt a peer.
>> The written value's high 16 bits are the ID of the peer to interrupt,
>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>> index 352937f..831da53 100644
>> --- a/hw/misc/ivshmem.c
>> +++ b/hw/misc/ivshmem.c
>> @@ -234,12 +234,7 @@ static uint64_t ivshmem_io_read(void *opaque, hwaddr
>> addr,
>> break;
>>
>> case IVPOSITION:
>> - /* return my VM ID if the memory is mapped */
>> - if (memory_region_is_mapped(&s->ivshmem)) {
>> - ret = s->vm_id;
>> - } else {
>> - ret = -1;
>> - }
>> + ret = s->vm_id;
>> break;
>>
>> default:
>> @@ -511,7 +506,8 @@ static bool fifo_update_and_get_i64(IVShmemState *s,
>> return false;
>> }
>>
>> -static int ivshmem_add_kvm_msi_virq(IVShmemState *s, int vector)
>> +static void ivshmem_add_kvm_msi_virq(IVShmemState *s, int vector,
>> + Error **errp)
>
> I prefer to return -1 in case of error, even if Error** is also returned.
You know, I'd prefer that, too, and I've argued for it unsuccessfully.
As it is, we fairly consistently return void when the function returns
errors through Error ** and has no non-error value.
>> {
>> PCIDevice *pdev = PCI_DEVICE(s);
>> MSIMessage msg = msix_get_message(pdev, vector);
>> @@ -522,22 +518,21 @@ static int ivshmem_add_kvm_msi_virq(IVShmemState *s,
>> int vector)
>>
>> ret = kvm_irqchip_add_msi_route(kvm_state, msg, pdev);
>> if (ret < 0) {
>> - error_report("ivshmem: kvm_irqchip_add_msi_route failed");
>> - return -1;
>> + error_setg(errp, "kvm_irqchip_add_msi_route failed");
>> + return;
>> }
>>
>> s->msi_vectors[vector].virq = ret;
>> s->msi_vectors[vector].pdev = pdev;
>> -
>> - return 0;
>> }
>>
>> -static void setup_interrupt(IVShmemState *s, int vector)
>> +static void setup_interrupt(IVShmemState *s, int vector, Error **errp)
>> {
>> EventNotifier *n = &s->peers[s->vm_id].eventfds[vector];
>> bool with_irqfd = kvm_msi_via_irqfd_enabled() &&
>> ivshmem_has_feature(s, IVSHMEM_MSI);
>> PCIDevice *pdev = PCI_DEVICE(s);
>> + Error *err = NULL;
>>
>> IVSHMEM_DPRINTF("setting up interrupt for vector: %d\n", vector);
>>
>> @@ -546,13 +541,16 @@ static void setup_interrupt(IVShmemState *s, int
>> vector)
>> watch_vector_notifier(s, n, vector);
>> } else if (msix_enabled(pdev)) {
>> IVSHMEM_DPRINTF("with irqfd\n");
>> - if (ivshmem_add_kvm_msi_virq(s, vector) < 0) {
>> + ivshmem_add_kvm_msi_virq(s, vector, &err);
>> + if (err) {
>> + error_propagate(errp, err);
>> return;
>
> That would make this simpler, avoiding local err variables, and
> propagate. But you seem to prefer that form. I don't know if there is
> any conventions (I am used to glib conventions, and usually a bool
> success is returned, even if the function takes a GError)
Does GLib spell out this convention somewhere?
I can perhaps try to cook up a patch to demonstrate the advantages of
returning a success/failure value even with Error **, and try to get our
convention changed.
Until then, we better stick to the existing convention, whether we like
it or not.
Thanks!
[...]