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

[Thomas Huth <thuth@redhat.com>]

On 2018-12-18 18:50, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> 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 <thuth@redhat.com>
>> ---
>>  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.

>> @@ -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.

>> @@ -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