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

[David Hildenbrand]

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. It's unfortunate but we
can't do anything about it.

-- 

Thanks,

David / dhildenb