Re: [Qemu-ppc] [Qemu-devel] [PATCH v1 06/11] pc-dimm: don't allow to access "size" before the device was realized

[David Hildenbrand]

On 15.06.2018 11:16, Igor Mammedov wrote:
> On Thu, 14 Jun 2018 16:10:14 +0200
> David Hildenbrand <address@hidden> wrote:
> 
>> On 14.06.2018 15:24, Igor Mammedov wrote:
>>> On Wed, 13 Jun 2018 16:03:35 +0200
>>> David Hildenbrand <address@hidden> wrote:
>>>   
>>>> On 13.06.2018 14:56, Igor Mammedov wrote:  
>>>>> On Mon, 11 Jun 2018 14:16:50 +0200
>>>>> David Hildenbrand <address@hidden> wrote:
>>>>>     
>>>>>> "size" should not be queried before the device was realized. Let' make
>>>>>> that explicit.
>>>>>>
>>>>>> Signed-off-by: David Hildenbrand <address@hidden>    
>>>>> It's generic property getter, it should work even before realize is 
>>>>> called.    
>>>>
>>>> That's the point, as we can see in NVDIMM code , it is *not* a generic
>>>> property getter.  
>>> it is added by 
>>>     object_property_add(obj, PC_DIMM_SIZE_PROP, "uint64", pc_dimm_get_size, 
>>>      
>>>                         NULL, NULL, NULL, &error_abort); 
>>> hence it's generic QOM property as opposed to memory device class specific 
>>> callbacks.
>>>   
>>>>  
>>>>>
>>>>> if issues described in 5/11 are properly fixed, this patch won't be 
>>>>> needed.
>>>>>
>>>>> So drop this patch
>>>>>
>>>>>     
>>>>>> ---
>>>>>>  hw/mem/pc-dimm.c | 6 ++++++
>>>>>>  1 file changed, 6 insertions(+)
>>>>>>
>>>>>> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
>>>>>> index 86fbcf2d0c..5294734529 100644
>>>>>> --- a/hw/mem/pc-dimm.c
>>>>>> +++ b/hw/mem/pc-dimm.c
>>>>>> @@ -166,6 +166,12 @@ static void pc_dimm_get_size(Object *obj, Visitor 
>>>>>> *v, const char *name,
>>>>>>      PCDIMMDevice *dimm = PC_DIMM(obj);
>>>>>>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj);
>>>>>>  
>>>>>> +    if (!DEVICE(obj)->realized) {
>>>>>> +        error_setg(errp, "Property \"%s\" not accessible before 
>>>>>> realized",
>>>>>> +                   name);
>>>>>> +        return;
>>>>>> +    }
>>>>>> +
>>>>>>      mr = ddc->get_memory_region(dimm, errp);  
>>> just make ddc->get_memory_region return error if 'memdev' isn't set yet
>>> and add local_error handling here as proper property assessor should do.
>>>   
>>
>> Does not help for NVDIMM.
> NVDIMM should be fixed as it's broken now and works just by accident.
> I don't like tailoring infrastructure changes to a particular broken
> device especially when the later could be made to work within current
> framework.
> 

Yes, but I will go the path of static properties for NVDIMM instead.
Trying to initialize the memory region in a property setter
("label-size") and then bailing out because another property (memdev) is
not set looks strange. I will do checks+initialization in realize() and
in get_memory_region(). This looks better in my point of view and looks
like a nice NVDIMM refactoring (at least judging from the patches I have
on my list now :) ).

-- 

Thanks,

David / dhildenb