Re: [Qemu-devel] [PULL 00/47] virtio, pc: fixes and features

[Xiao Guangrong]

On 11/01/2016 09:21 PM, Igor Mammedov wrote:
> On Tue, 1 Nov 2016 00:48:11 +0200
> "Michael S. Tsirkin" <address@hidden> wrote:
>
> > On Mon, Oct 31, 2016 at 10:50:31AM +0100, Igor Mammedov wrote:
> > > On Sun, 30 Oct 2016 23:23:18 +0200
> > > "Michael S. Tsirkin" <address@hidden> wrote:
> > >
> > > > The following changes since commit 5b2ecabaeabc17f032197246c4846b9ba95ba8a6:
> > > >
> > > >   Merge remote-tracking branch 'remotes/kraxel/tags/pull-ui-20161028-1' into 
> > > > staging (2016-10-28 17:59:04 +0100)
> > > >
> > > > are available in the git repository at:
> > > >
> > > >   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
> > > >
> > > > for you to fetch changes up to f082ec0225bd15c71e0b4697d2df3af7bad65d7f:
> > > >
> > > >   acpi: fix assert failure caused by commit 35c5a52d (2016-10-30 20:06:25 +0200)
> > > >
> > > > ----------------------------------------------------------------
> > > > virtio, pc: fixes and features
> > > >
> > > > nvdimm hotplug support
> > > Michael,
> > >
> > > Could you drop nvdimm hotplug from pull request (I should review at least once 
> > > as it touches not only NVDIMMs but a generic hotplug infrastructure)
> > >
> > > and keep only nvdimm fixes/cleanups for now?
> >
> > If I drop it now it won't be in the next QEMU and it seems like
> > a valuable feature. The comments so far are about minor style
> > improvements that IMO can be addressed by patches on top.
> wrt nvdimm hotplug support it's not about style issues but rather
> design issues: for example:
>  - it extends general hotplug framework unnecessarily instead of
>    figuring out how it works.
>  - adds not needed locks
> maybe there is more and all of that was posted just a day before
> this pull request so I haven't even had a chance to review it properly.
>
>
>
> > We can always revert if you see bigger issues, but let's enable the
> > testing of this feature.
> if it didn't mess with general infrastructure, I wouldn't care much.
> But it does so I'd rather avoid merging not yet ready series just for
> the sake of getting it in.
>
> I haven't reviewed 28-35 patches either but they are all cleanups/
> fixes of current nvdimm code and local to it so don't mind them
> getting merged.
>
> However I suggest dropping 36-39 patches from this pull request
> as not yet ready for merging.

Igor,

Thank for your hard work to review this patchset. Only two patches
related to general infrastructure that are:
[PULL 37/47] nvdimm acpi: introduce fit buffer
[PULL 39/47] pc: memhp: enable nvdimm device hotplug

The issue you pointed out in [PULL 37/47] can be easily improved
on the top of this patchset. Could you please look at the 39th,
if there is no big issue, could it be merged first?