David Hildenbrand <da...@redhat.com> writes:
> On 21/09/2018 14:28, Markus Armbruster wrote:
>> David Hildenbrand <da...@redhat.com> writes:
>>
>>> On 21/09/2018 10:07, Markus Armbruster wrote:
>>>> Quick review of just the QAPI part.
>>>>
>>>> David Hildenbrand <da...@redhat.com> writes:
>>>>
>>>>> From: Pankaj Gupta <pagu...@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.
>>>>>
>>>>> Signed-off-by: Pankaj Gupta <pagu...@redhat.com>
>>>>> [ MemoryDevice/MemoryRegion changes, cleanups, addr property "memaddr",
>>>>> split up patches ]
>>>>> Signed-off-by: David Hildenbrand <da...@redhat.com>
>>>> [...]
>>>>> 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.
