[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] hw/i386/pc: check apci hotplug capability befor
From: |
Wei Yang |
Subject: |
Re: [Qemu-devel] [PATCH] hw/i386/pc: check apci hotplug capability before nvdimm's |
Date: |
Thu, 30 May 2019 08:27:54 +0800 |
User-agent: |
Mutt/1.10.1 (2018-07-13) |
On Wed, May 29, 2019 at 10:57:50AM +0200, Igor Mammedov wrote:
>On Wed, 29 May 2019 08:32:14 +0800
>Wei Yang <address@hidden> wrote:
>
>> On Tue, May 28, 2019 at 02:26:27PM +0200, Igor Mammedov wrote:
>> >On Tue, 28 May 2019 09:35:48 +0800
>> >Wei Yang <address@hidden> wrote:
>> >
>> >> On Mon, May 27, 2019 at 02:21:14PM +0200, Igor Mammedov wrote:
>> >> >On Thu, 11 Apr 2019 15:17:39 +0800
>> >> >Wei Yang <address@hidden> wrote:
>> >> >
>> >> >> pc_memory_pre_plug() is called during hotplug for both pc-dimm and
>> >> >> nvdimm. This is more proper to check apci hotplug capability before
>> >> >> check nvdimm specific capability.
>> >> >not sure what this about.
>> >> >Currently we are checking if ACPI is enabled
>> >> > if (!pcms->acpi_dev || !acpi_enabled) { ...
>> >> >before nvdimm check and it looks better to me that we cancel
>> >> >nvdimm hotplug earlier than passing it to
>> >> > hotplug_handler_pre_plug(pcms->acpi_dev, dev, &local_err)
>> >> >with this patch ACPI device handler will be called before
>> >> >nvdimm check happens, so it's +1 unnecessary call chain in
>> >> >the case of nvdimm, which I'd rather not have.
>> >> >
>> >> >Are there any issues with current call flow?
>> >> >(commit message doesn't really explaining why we need this patch)
>> >> >
>> >>
>> >> My idea is to check more generic requirement and then specific one.
>> >>
>> >> For example, the call flow looks like this:
>> >>
>> >> pc_memory_pre_plug
>> >>
>> >> piix4_device_pre_plug_cb | ich9_pm_device_pre_plug_cb
>> >> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
>> >> !lpc->pm.acpi_memory_hotplug.is_enabled)
>> >>
>> >> if (is_nvdimm && !ms->nvdimms_state->is_enabled)
>> >>
>> >>
>> >> In hotplug_handler_pre_plug(), it checks the acpi hotplug capability. And
>> >> then
>> >> if it has memory hotplug capability and is nvdimm, we check whether
>> >> nvdimm is
>> >> enabled.
>> >
>> >I don't think pc_memory_pre_plug() should rely on what
>> >hotplug_handler_pre_plug()
>> >checks or does. Similarly the later is taking care of whatever piix4 needs
>> >to care
>> >and shouldn't care about what machine code does.
>> >
>>
>> Agree. It is not proper to let hotplug_handler_pre_plug() take care about
>> machine code.
>>
>> >Moreover when hotplug_handler_pre_plug() starts to reserve resources, then
>> >if you move check as suggested you'd need to rollback all that
>> >hotplug_handler_pre_plug() done to gracefully abort hotplug.
>> >
>>
>> Confused.
>>
>> hotplug_handler_pre_plug() doesn't reserve resources.
>
>
>it's not currently, but if it would it would not work with your patch properly
>or break unexpectedly since whoever would change hotplug_handler_pre_plug()
>might not notice that machine code need to be taken care of.
>
>Try to consider devices and machine as separate libraries. Which should
>in reasonable limits be independent and work through documented interfaces.
>In that case likehood of breaking something would be less than relying on
>current code impl./call order with implicit inter-dependencies.
>
So the logic here is check machine then device, right? I think this is
reasonable. To be honest, this rule is not that obvious.
Anyway, I think what you mentioned make sense. Thanks
--
Wei Yang
Help you, Help me