Re: [Qemu-devel] [PATCH RFCv2 4/9] virtio-pmem: Prototype

[David Hildenbrand <david@redhat.com>]

On 31.01.19 19:19, Markus Armbruster wrote:
> David Hildenbrand <david@redhat.com> writes:
> 
>> From: Pankaj Gupta <pagupta@redhat.com>
>>
>> This is the current protoype of virtio-pmem. Support will require
>> machine changes for the architectures that will support it, so it will
>> not yet be compiled.
>>
>> TODO:
>> - Use separate struct for tracking requests internally
>> - Move request/response structs to linux headers
>> - Factor out linux header sync
>> - Drop debug printfs
>>
>> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
>> [ MemoryDevice/MemoryRegion changes, cleanups, addr property "memaddr",
>>   split up patches, unplug handler ]
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
> [...]
>> diff --git a/include/standard-headers/linux/virtio_ids.h b/include/standard-headers/linux/virtio_ids.h
>> index 6d5c3b2d4f..346389565a 100644
>> --- a/include/standard-headers/linux/virtio_ids.h
>> +++ b/include/standard-headers/linux/virtio_ids.h
>> @@ -43,5 +43,6 @@
>>  #define VIRTIO_ID_INPUT        18 /* virtio input */
>>  #define VIRTIO_ID_VSOCK        19 /* virtio vsock transport */
>>  #define VIRTIO_ID_CRYPTO       20 /* virtio crypto */
>> +#define VIRTIO_ID_PMEM         25 /* virtio pmem */
>>  
>>  #endif /* _LINUX_VIRTIO_IDS_H */
>> diff --git a/qapi/misc.json b/qapi/misc.json
>> index 24d20a880a..b71eca2666 100644
>> --- a/qapi/misc.json
>> +++ b/qapi/misc.json
>> @@ -2949,16 +2949,42 @@
>>            }
>>  }
>>  
>> +##
>> +# @VirtioPMEMDeviceInfo:
>> +#
>> +# VirtioPMEM state information
>> +#
>> +# @id: device's ID
>> +#
>> +# @memaddr: physical address in memory, where device is mapped
>> +#
>> +# @size: size of memory that the device provides
>> +#
>> +# @memdev: memory backend linked with device
>> +#
>> +# Since: 4.0
>> +##
> 
> This is like PCDIMMDeviceInfo less @slot, @node, @hotplugged,
> @hotpluggable, and with @addr renamed to @memaddr.
> 
> Any particular reason for the rename?

I answered the same question already and thought I documented it
somewhere ... but looks like it went missing.

We cannot use the "addr" property as that is already used e.g. for
virtio-pci/pci devices. And we will have e.g. virtio-pmem-pci as a proy.
So we have to chose a different one (unfortunately). We decided to also
use the name of the property in this struct here, as it will otherwise
be terribly confusing for the user.

-device virtio-pmem-pci,id=p0,bus=bux0,addr=0x01,memaddr=0x1000000...

> 
>> +{ 'struct': 'VirtioPMEMDeviceInfo',
>> +  'data': { '*id': 'str',
>> +            'memaddr': 'size',
>> +            'size': 'size',
>> +            'memdev': 'str'
>> +          }
>> +}
>> +
>>  ##
>>  # @MemoryDeviceInfo:
>>  #
>>  # Union containing information about a memory device
>>  #
>> +# nvdimm is included since 2.12. virtio-pmem is included since 4.0.
>> +#
> 
> Let's stick to the way we document similar things elsewhere:
> 
>    # @nvdimm: since 2.12
>    #
>    # @virtio-pmem: since 4.0

Sounds good, doesn't work :)

(tried this already, the checker will complain that these fields don't
exist)

Thanks!

>    #
> 
>>  # Since: 2.1
>>  ##
>>  { 'union': 'MemoryDeviceInfo',
>>    'data': { 'dimm': 'PCDIMMDeviceInfo',
>> -            'nvdimm': 'PCDIMMDeviceInfo'
>> +            'nvdimm': 'PCDIMMDeviceInfo',
>> +            'virtio-pmem': 'VirtioPMEMDeviceInfo'
>>            }
>>  }


-- 

Thanks,

David / dhildenb