| Message ID | 20180611121655.19616-11-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | pc-dimm: next bunch of cleanups | expand |
On Mon, Jun 11, 2018 at 02:16:54PM +0200, David Hildenbrand wrote: > We'll be factoring out some pc-dimm specific and some memory-device > checks next. > > Signed-off-by: David Hildenbrand <david@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> ppc parts Acked-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/i386/pc.c | 2 ++ > hw/mem/pc-dimm.c | 5 +++++ > hw/ppc/spapr.c | 1 + > include/hw/mem/pc-dimm.h | 2 ++ > 4 files changed, 10 insertions(+) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 017396fe84..dc8e7b033b 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1695,6 +1695,8 @@ static void pc_dimm_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'"); > return; > } > + > + pc_dimm_memory_pre_plug(dev, MACHINE(hotplug_dev), errp); > } > > static void pc_dimm_plug(HotplugHandler *hotplug_dev, > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c > index bc79dd04d8..995ce22d8d 100644 > --- a/hw/mem/pc-dimm.c > +++ b/hw/mem/pc-dimm.c > @@ -27,6 +27,11 @@ > #include "sysemu/numa.h" > #include "trace.h" > > +void pc_dimm_memory_pre_plug(DeviceState *dev, MachineState *machine, > + Error **errp) > +{ > +} > + > void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine, > uint64_t align, Error **errp) > { > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 214286fd2f..54eddc0069 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3202,6 +3202,7 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > goto out; > } > > + pc_dimm_memory_pre_plug(dev, MACHINE(hotplug_dev), errp); > out: > g_free(mem_dev); > } > diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h > index f0e6867803..7d46a0a0cb 100644 > --- a/include/hw/mem/pc-dimm.h > +++ b/include/hw/mem/pc-dimm.h > @@ -80,6 +80,8 @@ typedef struct PCDIMMDeviceClass { > > int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp); > > +void pc_dimm_memory_pre_plug(DeviceState *dev, MachineState *machine, > + Error **errp); > void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine, > uint64_t align, Error **errp); > void pc_dimm_memory_unplug(DeviceState *dev, MachineState *machine);
On Mon, 11 Jun 2018 14:16:54 +0200 David Hildenbrand <david@redhat.com> wrote: > We'll be factoring out some pc-dimm specific and some memory-device > checks next. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > hw/i386/pc.c | 2 ++ > hw/mem/pc-dimm.c | 5 +++++ > hw/ppc/spapr.c | 1 + > include/hw/mem/pc-dimm.h | 2 ++ > 4 files changed, 10 insertions(+) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 017396fe84..dc8e7b033b 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1695,6 +1695,8 @@ static void pc_dimm_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, keeping ^^^^^ similar to newly introduced pc_dimm_memory_pre_plug() and I have no clue what to suggest as alternative > error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'"); > return; > } > + > + pc_dimm_memory_pre_plug(dev, MACHINE(hotplug_dev), errp); > } > > static void pc_dimm_plug(HotplugHandler *hotplug_dev, > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c > index bc79dd04d8..995ce22d8d 100644 > --- a/hw/mem/pc-dimm.c > +++ b/hw/mem/pc-dimm.c > @@ -27,6 +27,11 @@ > #include "sysemu/numa.h" > #include "trace.h" > > +void pc_dimm_memory_pre_plug(DeviceState *dev, MachineState *machine, > + Error **errp) > +{ > +} why introducing shim without anything? I'd just merge it with following patch and clarify (in commit message) why the rest of pre_plug code isn't moved here as well. > void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine, > uint64_t align, Error **errp) > { > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 214286fd2f..54eddc0069 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3202,6 +3202,7 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > goto out; > } > > + pc_dimm_memory_pre_plug(dev, MACHINE(hotplug_dev), errp); > out: > g_free(mem_dev); > } > diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h > index f0e6867803..7d46a0a0cb 100644 > --- a/include/hw/mem/pc-dimm.h > +++ b/include/hw/mem/pc-dimm.h > @@ -80,6 +80,8 @@ typedef struct PCDIMMDeviceClass { > > int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp); > > +void pc_dimm_memory_pre_plug(DeviceState *dev, MachineState *machine, > + Error **errp); > void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine, > uint64_t align, Error **errp); > void pc_dimm_memory_unplug(DeviceState *dev, MachineState *machine);
On 13.06.2018 15:10, Igor Mammedov wrote: > On Mon, 11 Jun 2018 14:16:54 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> We'll be factoring out some pc-dimm specific and some memory-device >> checks next. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> hw/i386/pc.c | 2 ++ >> hw/mem/pc-dimm.c | 5 +++++ >> hw/ppc/spapr.c | 1 + >> include/hw/mem/pc-dimm.h | 2 ++ >> 4 files changed, 10 insertions(+) >> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >> index 017396fe84..dc8e7b033b 100644 >> --- a/hw/i386/pc.c >> +++ b/hw/i386/pc.c >> @@ -1695,6 +1695,8 @@ static void pc_dimm_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > keeping ^^^^^ > similar to newly introduced pc_dimm_memory_pre_plug() > and I have no clue what to suggest as alternative Can you elaborate? In pc.c we now have: - static void pc_dimm_plug() - static void pc_dimm_unplug_request() - static void pc_dimm_unplug() And I add - static void pc_dimm_pre_plug() I am sticking to the existing naming scheme. (maybe the problem is that PC_DIMM should never have been named PC_DIMM but simply DIMM, then the "pc_" prefix would be unique) > >> error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'"); >> return; >> } >> + >> + pc_dimm_memory_pre_plug(dev, MACHINE(hotplug_dev), errp); >> } >> >> static void pc_dimm_plug(HotplugHandler *hotplug_dev, >> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c >> index bc79dd04d8..995ce22d8d 100644 >> --- a/hw/mem/pc-dimm.c >> +++ b/hw/mem/pc-dimm.c >> @@ -27,6 +27,11 @@ >> #include "sysemu/numa.h" >> #include "trace.h" >> >> +void pc_dimm_memory_pre_plug(DeviceState *dev, MachineState *machine, >> + Error **errp) >> +{ >> +} > why introducing shim without anything? Because you requested for review to split things up :) I can easily squash this. > I'd just merge it with following patch and clarify (in commit message) > why the rest of pre_plug code isn't moved here as well.
On Wed, 13 Jun 2018 16:15:48 +0200 David Hildenbrand <david@redhat.com> wrote: > On 13.06.2018 15:10, Igor Mammedov wrote: > > On Mon, 11 Jun 2018 14:16:54 +0200 > > David Hildenbrand <david@redhat.com> wrote: > > > >> We'll be factoring out some pc-dimm specific and some memory-device > >> checks next. > >> > >> Signed-off-by: David Hildenbrand <david@redhat.com> > >> --- > >> hw/i386/pc.c | 2 ++ > >> hw/mem/pc-dimm.c | 5 +++++ > >> hw/ppc/spapr.c | 1 + > >> include/hw/mem/pc-dimm.h | 2 ++ > >> 4 files changed, 10 insertions(+) > >> > >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c > >> index 017396fe84..dc8e7b033b 100644 > >> --- a/hw/i386/pc.c > >> +++ b/hw/i386/pc.c > >> @@ -1695,6 +1695,8 @@ static void pc_dimm_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > > keeping ^^^^^ > > similar to newly introduced pc_dimm_memory_pre_plug() > > and I have no clue what to suggest as alternative > > Can you elaborate? It's just that pc_dimm_pre_plug and pc_dimm_memory_pre_plug are very similar so it become confusing and with name alone you can't figure if both do the same or different things. Looking at 11/11 maybe you could just drop this and the next patch for now as there isn't obvious (if any) demand for it within this series at all. And introduce similar patch when it's actually required. I might imagine following naming in future: pc_dimm_pre_plug() memory_device_pre_plug() ... some pc-dimm only stuff ... virtio_mem_pre_plug() memory_device_pre_plug() ... some virtio-mem only stuff ... > In pc.c we now have: > - static void pc_dimm_plug() > - static void pc_dimm_unplug_request() > - static void pc_dimm_unplug() > > And I add > - static void pc_dimm_pre_plug() > > I am sticking to the existing naming scheme. > > (maybe the problem is that PC_DIMM should never have been named PC_DIMM > but simply DIMM, then the "pc_" prefix would be unique) Agreed, but now type name is part of interface so we are stuck with it. > >> error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'"); > >> return; > >> } > >> + > >> + pc_dimm_memory_pre_plug(dev, MACHINE(hotplug_dev), errp); > >> } > >> > >> static void pc_dimm_plug(HotplugHandler *hotplug_dev, > >> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c > >> index bc79dd04d8..995ce22d8d 100644 > >> --- a/hw/mem/pc-dimm.c > >> +++ b/hw/mem/pc-dimm.c > >> @@ -27,6 +27,11 @@ > >> #include "sysemu/numa.h" > >> #include "trace.h" > >> > >> +void pc_dimm_memory_pre_plug(DeviceState *dev, MachineState *machine, > >> + Error **errp) > >> +{ > >> +} > > why introducing shim without anything? > > Because you requested for review to split things up :) > > I can easily squash this. > > > I'd just merge it with following patch and clarify (in commit message) > > why the rest of pre_plug code isn't moved here as well. > >
On 15.06.2018 11:34, Igor Mammedov wrote: > On Wed, 13 Jun 2018 16:15:48 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> On 13.06.2018 15:10, Igor Mammedov wrote: >>> On Mon, 11 Jun 2018 14:16:54 +0200 >>> David Hildenbrand <david@redhat.com> wrote: >>> >>>> We'll be factoring out some pc-dimm specific and some memory-device >>>> checks next. >>>> >>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>> --- >>>> hw/i386/pc.c | 2 ++ >>>> hw/mem/pc-dimm.c | 5 +++++ >>>> hw/ppc/spapr.c | 1 + >>>> include/hw/mem/pc-dimm.h | 2 ++ >>>> 4 files changed, 10 insertions(+) >>>> >>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >>>> index 017396fe84..dc8e7b033b 100644 >>>> --- a/hw/i386/pc.c >>>> +++ b/hw/i386/pc.c >>>> @@ -1695,6 +1695,8 @@ static void pc_dimm_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, >>> keeping ^^^^^ >>> similar to newly introduced pc_dimm_memory_pre_plug() >>> and I have no clue what to suggest as alternative >> >> Can you elaborate? > > It's just that pc_dimm_pre_plug and pc_dimm_memory_pre_plug > are very similar so it become confusing and with name alone > you can't figure if both do the same or different things. > > Looking at 11/11 maybe you could just drop this and the next > patch for now as there isn't obvious (if any) demand for it > within this series at all. > And introduce similar patch when it's actually required. > > I might imagine following naming in future: > > pc_dimm_pre_plug() > memory_device_pre_plug() > ... some pc-dimm only stuff ... > > virtio_mem_pre_plug() > memory_device_pre_plug() > ... some virtio-mem only stuff ... We will always have the chain $machine_XXX_pre_plug() ... some machine specific stuff pc_dimm_memory_pre_plug() ... some pc-dimm specific stuff memory_device_pre_plug() I can rename pc_dimm_(plug|unplug ...) to pc_memory_(plug|unplug ...) and pc_dimm_memory_(plug|unplug ...) to pc_dimm_(plug|unplug ...) That way we match spapr naming scheme: spapr_memory_plug/spapr_memory_unplug
On Fri, 15 Jun 2018 11:48:33 +0200 David Hildenbrand <david@redhat.com> wrote: > On 15.06.2018 11:34, Igor Mammedov wrote: > > On Wed, 13 Jun 2018 16:15:48 +0200 > > David Hildenbrand <david@redhat.com> wrote: > > > >> On 13.06.2018 15:10, Igor Mammedov wrote: > >>> On Mon, 11 Jun 2018 14:16:54 +0200 > >>> David Hildenbrand <david@redhat.com> wrote: > >>> > >>>> We'll be factoring out some pc-dimm specific and some memory-device > >>>> checks next. > >>>> > >>>> Signed-off-by: David Hildenbrand <david@redhat.com> > >>>> --- > >>>> hw/i386/pc.c | 2 ++ > >>>> hw/mem/pc-dimm.c | 5 +++++ > >>>> hw/ppc/spapr.c | 1 + > >>>> include/hw/mem/pc-dimm.h | 2 ++ > >>>> 4 files changed, 10 insertions(+) > >>>> > >>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c > >>>> index 017396fe84..dc8e7b033b 100644 > >>>> --- a/hw/i386/pc.c > >>>> +++ b/hw/i386/pc.c > >>>> @@ -1695,6 +1695,8 @@ static void pc_dimm_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > >>> keeping ^^^^^ > >>> similar to newly introduced pc_dimm_memory_pre_plug() > >>> and I have no clue what to suggest as alternative > >> > >> Can you elaborate? > > > > It's just that pc_dimm_pre_plug and pc_dimm_memory_pre_plug > > are very similar so it become confusing and with name alone > > you can't figure if both do the same or different things. > > > > Looking at 11/11 maybe you could just drop this and the next > > patch for now as there isn't obvious (if any) demand for it > > within this series at all. > > And introduce similar patch when it's actually required. > > > > I might imagine following naming in future: > > > > pc_dimm_pre_plug() > > memory_device_pre_plug() > > ... some pc-dimm only stuff ... > > > > virtio_mem_pre_plug() > > memory_device_pre_plug() > > ... some virtio-mem only stuff ... > > We will always have the chain > > $machine_XXX_pre_plug() > ... some machine specific stuff > pc_dimm_memory_pre_plug() > ... some pc-dimm specific stuff > memory_device_pre_plug() > > I can rename > > pc_dimm_(plug|unplug ...) to > pc_memory_(plug|unplug ...) > > and pc_dimm_memory_(plug|unplug ...) to > pc_dimm_(plug|unplug ...) > > That way we match spapr naming scheme: > > spapr_memory_plug/spapr_memory_unplug sounds good to me
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 017396fe84..dc8e7b033b 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1695,6 +1695,8 @@ static void pc_dimm_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'"); return; } + + pc_dimm_memory_pre_plug(dev, MACHINE(hotplug_dev), errp); } static void pc_dimm_plug(HotplugHandler *hotplug_dev, diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c index bc79dd04d8..995ce22d8d 100644 --- a/hw/mem/pc-dimm.c +++ b/hw/mem/pc-dimm.c @@ -27,6 +27,11 @@ #include "sysemu/numa.h" #include "trace.h" +void pc_dimm_memory_pre_plug(DeviceState *dev, MachineState *machine, + Error **errp) +{ +} + void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine, uint64_t align, Error **errp) { diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 214286fd2f..54eddc0069 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -3202,6 +3202,7 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, goto out; } + pc_dimm_memory_pre_plug(dev, MACHINE(hotplug_dev), errp); out: g_free(mem_dev); } diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h index f0e6867803..7d46a0a0cb 100644 --- a/include/hw/mem/pc-dimm.h +++ b/include/hw/mem/pc-dimm.h @@ -80,6 +80,8 @@ typedef struct PCDIMMDeviceClass { int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp); +void pc_dimm_memory_pre_plug(DeviceState *dev, MachineState *machine, + Error **errp); void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine, uint64_t align, Error **errp); void pc_dimm_memory_unplug(DeviceState *dev, MachineState *machine);
We'll be factoring out some pc-dimm specific and some memory-device checks next. Signed-off-by: David Hildenbrand <david@redhat.com> --- hw/i386/pc.c | 2 ++ hw/mem/pc-dimm.c | 5 +++++ hw/ppc/spapr.c | 1 + include/hw/mem/pc-dimm.h | 2 ++ 4 files changed, 10 insertions(+)