Re: [Qemu-devel] [PATCH v3 17/22] virtio-pmem: prototype

[Markus Armbruster]

David Hildenbrand <address@hidden> writes:

> On 21/09/2018 10:07, Markus Armbruster wrote:
>> Quick review of just the QAPI part.
>> 
>> 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.
>>>
>>> Signed-off-by: Pankaj Gupta <address@hidden>
>>> [ MemoryDevice/MemoryRegion changes, cleanups, addr property "memaddr",
>>>   split up patches ]
>>> Signed-off-by: David Hildenbrand <address@hidden>
>> [...]
>>> diff --git a/qapi/misc.json b/qapi/misc.json
>>> index 7c36de0464..cadbca26ac 100644
>>> --- a/qapi/misc.json
>>> +++ b/qapi/misc.json
>>> @@ -2907,6 +2907,29 @@
>>>            }
>>>  }
>>>  
>>> +##
>>> +# @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: 3.1
>>> +##
>>> +{ 'struct': 'VirtioPMemDeviceInfo',
>>> +  'data': { '*id': 'str',
>>> +            'memaddr': 'size',
>>> +            'size': 'size',
>>> +            'memdev': 'str'
>>> +          }
>>> +}
>> 
>> This set of members is a proper subset of PCDIMMDeviceInfo's, except
>> 
>> * It uses 'size' instead of 'int', which is an improvement.  Improve
>>   PCDIMMDeviceInfo as well?
>
> That certainly makes sense.
>
> @Pankaj do you want to take care of that?
>
>> 
>> * The physical address member is called 'memaddr' instead of 'addr'.
>>   Why?
>> 
>
> Very good point. Have a look at "memory-device: add device class
> function set_addr()" (patch #10).
>
> Summary: The property name on the device will be called "memaddr", as
> "addr" is already (unfortunately) used for virtio addressing, that's why
> I feel like we should name it here "memaddr", too.
>
> ("addr" is too generic, a collision had to happen :( )

Hmm.  Let's see whether I understand.

Existing device "pc-dimm" has property "addr", and it's the physical
address.

Abstract device "pci-device" has property "addr", and it's the PCI
address.

Device "virtio-pmem-pci" is a "virtio-pci" is a "pci-device".  It thus
inherits "addr".  You therefore can't name the physical address property
"addr", and name it "memaddr" instead.

There is no such clash in VirtioPMemDeviceInfo.  You could name the
member "addr" there.  But that would trade the inconsistency with
PCDIMMDeviceInfo for an inconsistency with the device property.

Is this correct?