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