Re: [Qemu-devel] [PATCH v1 07/11] pc-dimm: get_memory_region() can never fail

[David Hildenbrand <david@redhat.com>]

On 14.06.2018 17:00, Igor Mammedov wrote:
> On Wed, 13 Jun 2018 16:50:54 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 13.06.2018 16:07, David Hildenbrand wrote:
>>> On 13.06.2018 15:03, Igor Mammedov wrote:  
>>>> On Mon, 11 Jun 2018 14:16:51 +0200
>>>> David Hildenbrand <david@redhat.com> wrote:
>>>>  
>>>>> We already verify when realizing that the memdev property has been
>>>>> set. We have no more accesses to get_memory_region() before the device
>>>>> is realized.  
>>>> this stems from assumption that get_memory_region shouldn't be called
>>>> before devices realize is executed, which I don't agree to begin with.
>>>>
>>>> However wrt error handling, we should probably leave device internal error
>>>> up to devices and make check for error only in pre_plug handler
>>>> (since pre_plug was successful, the rest machine helpers would have
>>>> access to the same region until device is removed.
>>>>  
>>>
>>> Something like a generic Device "validate()"/"pre_realize()" function,
>>> that can be called before realize() and validates all properties
>>> (+initializes derived properties) would be something I could agree to.
>>>
>>> Then we could drop all error handling from access functions (as they
>>> have been validated early during pre_plug())
>>>
>>> Moving all checks out of realize into pre_plug() looks ugly, because we
>>> have implementation details split across c-files.
>>>   
>>
>> I am thinking about something like this
>>
>> From ee8da4f5774ff5fa190466674e36ee99020033c4 Mon Sep 17 00:00:00 2001
>> From: David Hildenbrand <david@redhat.com>
>> Date: Wed, 13 Jun 2018 16:41:12 +0200
>> Subject: [PATCH RFC] device: add "prepare()" function
>>
>> The realize() function usually does several things. It validates
>> properties, inititalizes state based on properties and performs
>> e.g. registrations in the system that require "unrealize()" to be called
>> when removing the device.
>>
>> In a pre_plug hotplug handler, we usually want to access certain
>> properties or derived properties, e.g. to perform advanced checks
>> (for resource asignment). Now we have the problem, that realize() was
>> not called yet once we reach pre_plug(). So we theoretically have to
>> duplicate checks and add error paths for cases that can
>> theoretically never happen.
> I care less about duplicate checks in completely different parts of code,
> and I'd even insist on device:realize checks being self contained and not
> rely on any other external checks performed by users of device. And vice
> verse layer that uses device should do it's own checks when necessary
> and not rely on device's verification. That way loosely coupled code
> wouldn't fall apart whenever we drop or change checks in one of the parts.
> 
> The only case where I'd make concession is minimizing error handling
> on hot path for performance reasons and this is not the case here.
> 
>> Let's add the "prepare()" callback, which can be used to validate
>> properties and inititalize some state based on these. It will be called
>> once all static properties have been inititalized, but before the
>> pre_plug hotplug handler is activated. The pre_plug handler can than
>> rely on the fact that basic checks already were performed.
> 
> pre_plug isn't part of device, it's a separate part that might vary
> depending on machine and which might modify device properties along
> the way and then exaggerating we would need 'prepare2()' and after
> that 'pre_plug2()' and ...

That's how two parties (device vs hotplug handler) work together to get
results done ... Just like inititalize() -> realized() vs. pre_plug ->
plug(). There has to be some hand shaking.

> 
> Hence I dislike idea of adding more callbacks. I'd rather have a property
> setter do the job of initializing state of device when necessary instead
> of adding more callbacks. Which is in nvdimm case a bit more code compared
> to doing it in realize() but should be possible to implement.

Although I strongly disagree (especially about the fact of calling class
functions *anytime* after a device has been created but not initialized)
I will implement it like that for now (otherwise I won't be making any
progress here but instead be creating more and more problems to solve).


nvdimm will only have static properties. When realizing or when calling
get_memory_region(), properties will be checked and the nvdimm_mr
inititalized, if not already done.

-- 

Thanks,

David / dhildenb