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.y...@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.weiy...@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.weiy...@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