Re: [Qemu-devel] [QEMU PATCH] hw/misc/ivshmem: Remove deprecated "ivshmem" legacy device

[Markus Armbruster]

Thomas Huth <address@hidden> writes:

> On 2018-12-18 18:50, Markus Armbruster wrote:
>> Thomas Huth <address@hidden> writes:
>> 
>>> It's been marked as deprecated in QEMU v2.6.0 already, so really nobody
>>> should use the legacy "ivshmem" device anymore (but use ivshmem-plain or
>>> ivshmem-doorbell instead). Time to remove the deprecated device now.
>>>
>>> Signed-off-by: Thomas Huth <address@hidden>
>>> ---
>>>  docs/specs/ivshmem-spec.txt |   8 +-
>>>  hw/i386/pc_piix.c           |   4 -
>>>  hw/misc/ivshmem.c           | 206 
>>> +-------------------------------------------
>>>  qemu-deprecated.texi        |   5 --
>>>  scripts/device-crash-test   |   1 -
>>>  tests/ivshmem-test.c        |  65 +++++---------
>>>  6 files changed, 29 insertions(+), 260 deletions(-)
>>>
>>> diff --git a/docs/specs/ivshmem-spec.txt b/docs/specs/ivshmem-spec.txt
>>> index a1f5499..042f7ea 100644
>>> --- a/docs/specs/ivshmem-spec.txt
>>> +++ b/docs/specs/ivshmem-spec.txt
>>> @@ -17,12 +17,16 @@ get interrupted by its peers.
>>>  
>>>  There are two basic configurations:
>>>  
>>> -- Just shared memory: -device ivshmem-plain,memdev=HMB,...
>>> +- Just shared memory:
>>> +
>>> +      -device ivshmem-plain,memdev=HMB,...
>>>  
>>>    This uses host memory backend HMB.  It should have option "share"
>>>    set.
>>>  
>>> -- Shared memory plus interrupts: -device ivshmem,chardev=CHR,vectors=N,...
>>> +- Shared memory plus interrupts:
>>> +
>>> +      -device ivshmem-doorbell,chardev=CHR,vectors=N,...
>>>  
>>>    An ivshmem server must already be running on the host.  The device
>>>    connects to the server's UNIX domain socket via character device
>> 
>> Just whitespace.  Intentional?
>
> It's not just whitespace, I had to change "-device ivshmem" into
> "-device ivshmem-doorbell" here, and that did not fit into one line anymore.

Oww, that should've been done in commit 5400c02b90b.  Turns out I'm not
only blind to mistakes in my own texts, I'm also blind to the fixes.
Suggest to add to the commit message:

    Belatedly update a mention of the deprecated "ivshmem" in
    docs/specs/ivshmem-spec.txt to "ivshmem-doorbell".  Missed in commit
    5400c02b90b.

>>> @@ -882,8 +864,8 @@ static void ivshmem_common_realize(PCIDevice *dev, 
>>> Error **errp)
>>>      IVShmemState *s = IVSHMEM_COMMON(dev);
>>>      Error *err = NULL;
>>>      uint8_t *pci_conf;
>>> -    uint8_t attr = PCI_BASE_ADDRESS_SPACE_MEMORY |
>>> -        PCI_BASE_ADDRESS_MEM_PREFETCH;
>>> +    const uint8_t attr = PCI_BASE_ADDRESS_SPACE_MEMORY |
>>> +        PCI_BASE_ADDRESS_MEM_PREFETCH | PCI_BASE_ADDRESS_MEM_TYPE_64;
>>>      Error *local_err = NULL;
>>>  
>>>      /* IRQFD requires MSI */
>> 
>> Sure this belongs to this patch?
>
> Yes. I had to remove the "if (s->not_legacy_32bit)" below, so I moved
> the PCI_BASE_ADDRESS_MEM_TYPE_64 to the other bits above. I could also
> leave it below, but I think that would look a little bit lonely there
> without the if-statement.

I missed that part over the addition of const; I should've read more
closely.

I don't care for const here, but I don't really mind it either.  But
let's eliminate the variable instead.  It's used just once, and there's
considerable distance.

>>> @@ -903,10 +885,6 @@ static void ivshmem_common_realize(PCIDevice *dev, 
>>> Error **errp)
>>>      pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY,
>>>                       &s->ivshmem_mmio);
>>>  
>>> -    if (s->not_legacy_32bit) {
>>> -        attr |= PCI_BASE_ADDRESS_MEM_TYPE_64;
>>> -    }
>>> -
>>>      if (s->hostmem != NULL) {
>>>          IVSHMEM_DPRINTF("using hostmem\n");
>>>  
> [...]
>>> diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
>>> index c37b196..9811d66 100644
>>> --- a/tests/ivshmem-test.c
>>> +++ b/tests/ivshmem-test.c
>>> @@ -305,20 +305,18 @@ static void *server_thread(void *data)
>>>      return NULL;
>>>  }
>>>  
>>> -static void setup_vm_with_server(IVState *s, int nvectors, bool msi)
>>> +static void setup_vm_with_server(IVState *s, int nvectors)
>>>  {
>>> -    char *cmd = g_strdup_printf("-chardev socket,id=chr0,path=%s,nowait "
>>> -                                "-device 
>>> ivshmem%s,chardev=chr0,vectors=%d",
>>> -                                tmpserver,
>>> -                                msi ? "-doorbell" : ",size=1M,msi=off",
>>> -                                nvectors);
>>> +    char *cmd = g_strdup_printf("-chardev socket,id=chr0,path=%s,nowait 
>>> -device"
>> 
>> Awkward line break.
>> 
>>> +                                " 
>>> ivshmem-doorbell,chardev=chr0,vectors=%d",
>>> +                                tmpserver, nvectors);
>> 
>> Suggest
>> 
>>        char *cmd = g_strdup_printf("-chardev socket,id=chr0,path=%s,nowait "
>>                        "-device ivshmem-doorbell,chardev=chr0,vectors=%d",
>>                        tmpserver, nvectors);
>
> Ok, I can do that in v2.
>
>  Thomas

Thanks!