Re: [Qemu-devel] [PATCH v3 06/22] memory-device: document MemoryDeviceClass

[David Hildenbrand]

On 25/09/2018 16:19, Igor Mammedov wrote:
> On Thu, 20 Sep 2018 12:32:27 +0200
> David Hildenbrand <address@hidden> wrote:
> 
>> Document the functions and when to not expect errors.
>>
>> Signed-off-by: David Hildenbrand <address@hidden>
>> ---
>>  include/hw/mem/memory-device.h | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
>> index f02b229837..d6853156ff 100644
>> --- a/include/hw/mem/memory-device.h
>> +++ b/include/hw/mem/memory-device.h
>> @@ -29,9 +29,22 @@ typedef struct MemoryDeviceState {
>>      Object parent_obj;
>>  } MemoryDeviceState;
>>  
>> +/**
>> + * MemoryDeviceClass:
>> + * @get_addr: The address of the @md in guest physical memory. "0" means 
>> that
>> + * no address has been specified by the user and that no address has been
>> + * assigned yet.
> While looking at attempts to implement
>  '[Qemu-devel] [PATCH v12 6/6] tpm: add ACPI memory clear interface'
> on QEMU side, where we are trying to clear RAM going via ramblocks list.
> 
> Maybe we are doing it backwards and instead of trying to deal with host
> abstraction RAMBlocks/MemoryRegion and follow up efforts to tag it with
> device model attributes
>   '[PATCH] RFC: mark non-volatile memory region'
> we should do it other way around and approach it from guest point of view
> like firmware would do, i.e.
> make TPM enumerate RAM devices and tell them to clear related memory.
> Concrete device would know how to do it properly and/or how to ask backend
> to do it without need to tag RAMBlocks with device model specific info.
> That would allow to enumerate all RAM without trying to filter out not RAM
> RAMBlocks and not pollute host specific RAMBlock/MemoryRegion with device
> specific flags.

I agree, I consider it also as a problem that many different mechanism
in the code assume that any RAMBlock can e.g. be read/written etc. Or,
as you said, try to resolve certain properties from RAMBlocks. If we
could let the devices handle details (e.g. memory-device class callbacks
or similar), we would move the logic to a place where we actually know
what is happenig.

> 
> However we can't do it right now, since built-in memory doesn't have
> a corresponding device model and we use MemoryRegions directly.
> Maybe we should use memory-devices and create derivative 'ramchip' device
> to represent builtin RAM and use it instead.

Yes, we would need some sort of internal DIMM (however, as you correctly
state, DIMM is the wrong term, it really is *some* kind of ram device).

> 
> So if we were to do that, we would need to make get_addr() return value 0
> a valid one (suggest (uint64_t)-1 as unset value) and/or make built in
> memory work outside of device_memory region as short cut impl.
> 

Something like that will certainly work, but require many other changes.
E.g. the device memory region has to be sized and setup completely
different. But then, we can change the default (have to whatch out about
compatibility handling when e.g. addr=0 is passed to e.g. dimms on the
cmdline).

-- 

Thanks,

David / dhildenb