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

[Igor Mammedov]

On Thu, 28 Feb 2019 08:46:10 +0800
Wei Yang <address@hidden> 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 <address@hidden> 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 <address@hidden> 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)