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

[Pankaj Gupta]

> 
> 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?

Sure.

Thanks,
Pankaj

> 
> > 
> > * 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 :( )
> 
> --
> 
> Thanks,
> 
> David / dhildenb
>