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

[Igor Mammedov]

On Mon, 25 Feb 2019 12:47:14 +0000
Wei Yang <address@hidden> wrote:

> On Mon, Feb 25, 2019 at 09:05:37AM +0100, Igor Mammedov wrote:
> >On Sat, 23 Feb 2019 00:02:49 +0000
> >Wei Yang <address@hidden> wrote:
> >  
> >> On Thu, Feb 21, 2019 at 03:50:04PM +0100, Igor Mammedov wrote:  
> >> >On Wed, 20 Feb 2019 08:51:21 +0800
> >> >Wei Yang <address@hidden> wrote:
> >> >  
> >> >> Three trivial cleanup for pc-dimm.
> >> >> 
> >> >> Patch [1] remove the check on class->hotpluggable since pc-dimm is 
> >> >> always
> >> >> hotpluggable.
> >> >> Patch [2] remove nvdimm_realize
> >> >> Patch [2] remove pcdimm realize-callback  
> >> >even though this series doesn't break anything, I disagree with it
> >> >conceptually as it makes device less abstracted and make it more
> >> >dependent on how existing machine code uses it.
> >> >I'd drop whole series.
> >> >  
> >> 
> >> Is Patch [1] also make device more dependent on existing implementation?  
> >yes, it's implementation detail that PCDIMM&Co are hotpluggable now.
> >  
> >> For example, when we look at the counterpart of acpi_memory_plug_cb():
> >> 
> >>     acpi_pcihp_device_plug_cb
> >> 
> >> which handle the pci device hotplug. We don't check the hotpluggable
> >> property for pci devices.
> >> 
> >> 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.

> >> For Patch[2][3], I agree with you.
> >>   
>