Re: [Qemu-devel] [PATCH v1 05/11] spapr: move memory hotplug size check into plug code

[David Hildenbrand]

On 13.06.2018 15:57, Igor Mammedov wrote:
> On Wed, 13 Jun 2018 13:05:53 +0200
> David Hildenbrand <address@hidden> wrote:
> 
>> On 13.06.2018 13:01, Igor Mammedov wrote:
>>> On Mon, 11 Jun 2018 14:16:49 +0200
>>> David Hildenbrand <address@hidden> wrote:
>>>   
>>>> This might look like a step backwards, but it is not. get_memory_region()
>>>> should not be called on uninititalized devices. In general, only
>>>> properties should be access, but no "derived" satte like the memory
>>>> region.
>>>>
>>>> 1. We need duplicate error checks if memdev is actually already set.
>>>>    realize() performs these checks, no need to duplicate.  
>>> it's not duplicate, if a machine doesn't access to memory region
>>> in preplug time (hence doesn't check), then device itself would check it,
>>> check won't be missed by accident.
>>> (yep it's more code but more robust at the same time, so I'd leave it as 
>>> is)  
>>
>> Checking at two places for the same thing == duplicate checks
> device models and there users are separate entities hence I consider
> checks are separate. If user code can be written without adding extra checks
> it's fine. But if device model doesn't have its own checks when and is
> used in by new user code without checks also, it's going to break.
> 
> So it would be hard to convince me that consolidating error handling
> between in-depended layers is a good idea in general and particularly
> in this case.
> 
> I'd just drop this error cleanups altogether so that they won't get
> in the way of actual changes you are aiming for (unless you have to do it).
> 
>>>> 2. This is bad practise as one can see when looking at the NVDIMM
>>>>    implemetation. The call does not return sane data before the device
>>>>    is realized. Although spapr does not use NVDIMM, conceptually it is
>>>>    wrong.
>>>>
>>>> So let's just move this call to the right place. We can then cleanup
>>>> get_memory_region().  
>>> So I have to say no to this particular patch.
>>> It is indeed a step backwards and it looks like workaround for broken 
>>> nvdimm impl.
>>>
>>> Firstly, memdev property must be set for dimm device and
>>> a user accessing memory region first must check for error.
>>> More details below.
>>>   
>>
>> You assume that any class function can be called at any time. And I
>> don't think this is the way to go.
> Not any time, in the case of get_memory_region() it should work at
> pre_plug() time as all the pieces for it are already there.
> So we should make it work correctly for NVDIMM instead of 
> succumbing to it and running wild.
> we should stick to canonical sequence
>   object_new -> set props -> realize
> I don't see any reason to violate it in this case other than laziness.
> 

See my replay to patch nr. 7.

In contrast to what you suggest, I propose converting all nvdimm
properties to static properties (because that's what they actually are).
The validation/initialization of them should happen at a central place,
not at various places (realize(), property setters, or even
get_memory_region()) what you suggest.

I propose a separate function for this (prepare() on DeviceClass), where
we can split of the applicable parts of realize() that just perform
property checks + basic initialization (e.g. of derived properties like
the memory region in case of NVDIMM).

-- 

Thanks,

David / dhildenb