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

[David Hildenbrand]

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

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

-- 

Thanks,

David / dhildenb