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

[Markus Armbruster]

David Hildenbrand <address@hidden> writes:

> On 31.01.19 19:19, Markus Armbruster wrote:
>> David Hildenbrand <address@hidden> writes:
>> 
>>> From: Pankaj Gupta <address@hidden>
>>>
>>> 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 <address@hidden>
>>> [ MemoryDevice/MemoryRegion changes, cleanups, addr property "memaddr",
>>>   split up patches, unplug handler ]
>>> Signed-off-by: David Hildenbrand <address@hidden>
>>> ---
>> [...]
>>> 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.

Looks like my end-of-year mental garbage collection was overeager again.

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

Doc generator shortcoming *sigh*.

QAPI part
Acked-by: Markus Armbruster <address@hidden>

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