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

[Markus Armbruster]

David Hildenbrand <address@hidden> writes:

> On 21/09/2018 14:28, Markus Armbruster wrote:
>> 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.
>
> Almost correct.
>
> virtio-pmem-pci is a proxy of virtio-pci and aliases all properties
> (that part is confusing). So all properties of virtio-pci become
> properties of virtio-pmem-pci. And the user only configures
> virtio-pmem-pci via the properties.
>
>> 
>> 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?
>> 
>
> Yes, I chose to name it like the property. (that felt to be the right
> thing). As far as I see this is perfectly fine.

Now work that into the commit message or perhaps a comment, please :)

>                                                 It's unfortunate but we
> can't do anything about it.

Well, if we really, really wanted to, we could: rename pc-dimm's
property, keep the old name as a deprecated alias.  Would that be better
than the inconsistency, and the code you need to work around it?  You
decide.