Re: [Qemu-devel] [PATCH v2 0/3] PCDIMM cleanup

[Wei Yang <richardw.yang@linux.intel.com>]

On Thu, Feb 28, 2019 at 02:57:07PM +0100, Igor Mammedov wrote:
>On Thu, 28 Feb 2019 08:46:10 +0800
>Wei Yang <richardw.yang@linux.intel.com> wrote:
>
>> On Wed, Feb 27, 2019 at 06:27:49PM +0100, Igor Mammedov wrote:
>> >On Wed, 27 Feb 2019 13:59:20 +0000
>> >Wei Yang <richard.weiyang@gmail.com> wrote:
>> >  
>> >> On Wed, Feb 27, 2019 at 02:12:42PM +0100, Igor Mammedov wrote:  
>> >> >On Mon, 25 Feb 2019 12:47:14 +0000
>> >> >Wei Yang <richard.weiyang@gmail.com> wrote:
>> >> >    
>> >> >> >> To me, this is a general rule for PCDIMM, they are hotpluggable.      
>> >> >> >yes, PCDIMMs are hotpluggable but apci device (piix4pm/ich9/...) handling hotplug
>> >> >> >should ignore any non-hotpluggable one. If you remove check and then later
>> >> >> >someone else would add non-hotpluggable memory device or make PC-DIMMs or its
>> >> >> >variant of non-hotpluggable one, acpi device handling will break.
>> >> >> >Hence I'd prefer to keep the check.  
>> >> >> >       
>> >> >> 
>> >> >> Ok, if we'd support un-hotpluggable mem device, we could retain this
>> >> >> check. But this maybe not a proper place.
>> >> >> 
>> >> >> Based on my understanding, at this point, every thing has been done
>> >> >> properly. If this check pass, we will send an acpi interrupt to notify
>> >> >> the guest. In case this is an un-hotpluggable device, every thing looks
>> >> >> good but no effect in guest. Because we skip the notification.
>> >> >> 
>> >> >> Maybe we need to move the check in pre-plug?    
>> >> >And what would happen then and afterwards?
>> >> >
>> >> >Point is to make one of the handlers in chain to ignore plug event
>> >> >(i.e. do not generate SCI event) while the rest of handlers complete
>> >> >successfully mapping device in address space and whatever else.
>> >> >    
>> >> 
>> >> This will have a well prepared device in system but guest is not
>> >> notified, right?  
>> >yes, it's theoretically possible to move check up the call stack
>> >to machine level and not call secondary hotplug handler on non hotplugged
>> >device but that again depends on what secondary hotplug handler does.
>> >I'd rather keep logic independent here unless there is good reason not
>> >to do so.
>> >
>> >  
>> >> My idea to move the check in pre-plug will result the qemu return when
>> >> it see no SCI event will be generated, so there is no device created.
>> >> 
>> >> I guess current behavior is a designed decision?  
>> >I'd say so.
>> >PS:
>> >QEMU's hotplug_hadler API is misnamed at this point as it handles both
>> >cold (basic device wiring) and hotplug (processing hotplug).
>> >Maybe we should rename it but I'm not irritated enough by it to do so.
>> >  
>> 
>> After re-read the code, I found something more.
>> 
>> First let me describe my understanding a bit.
>> 
>> To hotplug a device, several part are related:
>> 
>>     * device itself
>>     * machine's hotplug_handler
>>     * bus's hotplug_handler
>>     * acpi configuration
>> 
>> Currently, some checks are mixed, which makes the logic not that clear.
>> 
>> Let's come back to the problem in this thread.
>> 
>> The check in concern is the device's hotpluggable property. And when we look
>> into device_set_realized, this property is already checked at the very
>> beginning. This means when we go all the way down to acpi_memory_plug_cb(), if
>> this device is un-hotpluggable, it is must not a hotplugged device. And the
>> acpi_send_event() will not be triggered.
>> 
>> Based on my understanding, mdev->dimm and mdev->is_enabld looks still
>> necessary to be set for a un-hotplugged device. For both hotpluggable and
>> un-hotpluggable device. Skip these two steps if the device is not hotpluggable
>> looks not consistent with others?
>it might be inconsistent and broken since we don't actually have
>a nonhotpluggable memory device yet. But I'd would fix it when such device
>is added (it might depend on being added device whether it needs to be tracked
>by acpi memory hotplug path or if it uses other means in which case check
>is correct)
>

Ok, got your point.

-- 
Wei Yang
Help you, Help me