Re: [PATCH v6 14/33] pc-dimm: drop the prefix of pc-dimm
On Fri, Oct 30, 2015 at 01:56:08PM +0800, Xiao Guangrong wrote:
> This patch is generated by this script:
>
> find ./ -name "*.[ch]" -o -name "*.json" -o -name "trace-events" -type f \
> | xargs sed -i "s/PC_DIMM/DIMM/g"
>
> find ./ -name "*.[ch]" -o -name "*.json" -o -name "trace-events" -type f \
> | xargs sed -i "s/PCDIMM/DIMM/g"
>
> find ./ -name "*.[ch]" -o -name "*.json" -o -name "trace-events" -type f \
> | xargs sed -i "s/pc_dimm/dimm/g"
>
> find ./ -name "trace-events" -type f | xargs sed -i "s/pc-dimm/dimm/g"
>
> It prepares the work which abstracts dimm device type for both pc-dimm and
> nvdimm
>
> Signed-off-by: Xiao Guangrong
I can see two ways this patchset can get merged
- merge refactorings first, nvdimm support on top
- merge nvdimm support first, refactor code on top
The way you put it in the middle of the series
allows neither. And I definitely favor option 2:
it's easier to reason about the best way to refactor code
when you have multiple users before you.
> ---
> hmp.c | 2 +-
> hw/acpi/ich9.c | 6 +-
> hw/acpi/memory_hotplug.c| 16 ++---
> hw/acpi/piix4.c | 6 +-
> hw/i386/pc.c| 32 -
> hw/mem/pc-dimm.c| 148
>
> hw/ppc/spapr.c | 18 ++---
> include/hw/mem/pc-dimm.h| 62 -
> numa.c | 2 +-
> qapi-schema.json| 8 +--
> qmp.c | 2 +-
> stubs/qmp_pc_dimm_device_list.c | 2 +-
> trace-events| 8 +--
> 13 files changed, 156 insertions(+), 156 deletions(-)
>
> diff --git a/hmp.c b/hmp.c
> index 5048eee..5c617d2 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1952,7 +1952,7 @@ void hmp_info_memory_devices(Monitor *mon, const QDict
> *qdict)
> MemoryDeviceInfoList *info_list = qmp_query_memory_devices(&err);
> MemoryDeviceInfoList *info;
> MemoryDeviceInfo *value;
> -PCDIMMDeviceInfo *di;
> +DIMMDeviceInfo *di;
>
> for (info = info_list; info; info = info->next) {
> value = info->value;
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 1c7fcfa..b0d6a67 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -440,7 +440,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs
> *pm, Error **errp)
> void ich9_pm_device_plug_cb(ICH9LPCPMRegs *pm, DeviceState *dev, Error
> **errp)
> {
> if (pm->acpi_memory_hotplug.is_enabled &&
> -object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> +object_dynamic_cast(OBJECT(dev), TYPE_DIMM)) {
> acpi_memory_plug_cb(&pm->acpi_regs, pm->irq,
> &pm->acpi_memory_hotplug,
> dev, errp);
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> @@ -455,7 +455,7 @@ void ich9_pm_device_unplug_request_cb(ICH9LPCPMRegs *pm,
> DeviceState *dev,
>Error **errp)
> {
> if (pm->acpi_memory_hotplug.is_enabled &&
> -object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> +object_dynamic_cast(OBJECT(dev), TYPE_DIMM)) {
> acpi_memory_unplug_request_cb(&pm->acpi_regs, pm->irq,
>&pm->acpi_memory_hotplug, dev, errp);
> } else {
> @@ -468,7 +468,7 @@ void ich9_pm_device_unplug_cb(ICH9LPCPMRegs *pm,
> DeviceState *dev,
>Error **errp)
> {
> if (pm->acpi_memory_hotplug.is_enabled &&
> -object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> +object_dynamic_cast(OBJECT(dev), TYPE_DIMM)) {
> acpi_memory_unplug_cb(&pm->acpi_memory_hotplug, dev, errp);
> } else {
> error_setg(errp, "acpi: device unplug for not supported device"
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index ce428df..e687852 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -54,23 +54,23 @@ static uint64_t acpi_memory_hotplug_read(void *opaque,
> hwaddr addr,
> o = OBJECT(mdev->dimm);
> switch (addr) {
> case 0x0: /* Lo part of phys address where DIMM is mapped */
> -val = o ? object_property_get_int(o, PC_DIMM_ADDR_PROP, NULL) : 0;
> +val = o ? object_property_get_int(o, DIMM_ADDR_PROP, NULL) : 0;
> trace_mhp_acpi_read_addr_lo(mem_st->selector, val);
> break;
> case 0x4: /* Hi part of phys address where DIMM is mapped */
> -val = o ? object_property_get_int(o, PC_DIMM_ADDR_PROP, NULL) >> 32
> : 0;
> +val = o ? object_property_get_int(o, DIMM_ADDR_PROP, NULL) >> 32 : 0;
> trace_mhp_acpi_read_addr_hi(mem_st->selector, val);
> break;
> case 0x8: /* Lo part of DIMM size */
> -val = o ? object_property_get_int(o, PC_DIMM_SIZE_PROP, NULL) : 0;
> +val = o ? object_property_get_int(o, DIMM_SIZE_PROP, NULL) : 0;
> trace_mhp_acpi_read_size_lo(mem_st->selecto
Re: [Qemu-devel] [PATCH v6 14/33] pc-dimm: drop the prefix of pc-dimm
On 10/31/2015 01:06 AM, Eric Blake wrote:
On 10/29/2015 11:56 PM, Xiao Guangrong wrote:
This patch is generated by this script:
find ./ -name "*.[ch]" -o -name "*.json" -o -name "trace-events" -type f \
| xargs sed -i "s/PC_DIMM/DIMM/g"
find ./ -name "*.[ch]" -o -name "*.json" -o -name "trace-events" -type f \
| xargs sed -i "s/PCDIMM/DIMM/g"
find ./ -name "*.[ch]" -o -name "*.json" -o -name "trace-events" -type f \
| xargs sed -i "s/pc_dimm/dimm/g"
The '-type f' binds only to the '-name trace-events' clause, and not the
'name "*.[ch]"' or '-name "*.json"', but since we don't have any
directories by those names, it didn't matter. You probably could have
eliminated the -type f with no consequences, rather than adding "(" and
")". In fact, you could have used git rather than find to do it:
git grep -il 'pc_\?dimm' | xargs ...
which finds only one additional instance of pc_dimm in
stubs/Makefile.objs - so maybe you should:
git mv stubs/qmp_{pc_,}dimm_device_list.c
either in this patch or as a followup.
Could compress these three sed lines into one, if desired:
find ... | xargs sed -i 's/pc_\?\(dimm\)/\1/ig'
But doesn't really matter. I'm fine if you leave your commit message
as-is, and will let you decide what to do about the stub file name.
Really good lesson for me!
find ./ -name "trace-events" -type f | xargs sed -i "s/pc-dimm/dimm/g"
It prepares the work which abstracts dimm device type for both pc-dimm and
nvdimm
Signed-off-by: Xiao Guangrong
---
Reviewed-by: Eric Blake
qapi-schema.json| 8 +--
Touches public interface files, but does not affect ABI, so it is safe.
Thanks for your sharing and review, Eric!
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH v6 14/33] pc-dimm: drop the prefix of pc-dimm
On 10/31/2015 12:10 AM, Vladimir Sementsov-Ogievskiy wrote: On 30.10.2015 08:56, Xiao Guangrong wrote: This patch is generated by this script: find ./ -name "*.[ch]" -o -name "*.json" -o -name "trace-events" -type f \ | xargs sed -i "s/PC_DIMM/DIMM/g" find ./ -name "*.[ch]" -o -name "*.json" -o -name "trace-events" -type f \ | xargs sed -i "s/PCDIMM/DIMM/g" find ./ -name "*.[ch]" -o -name "*.json" -o -name "trace-events" -type f \ | xargs sed -i "s/pc_dimm/dimm/g" find ./ -name "trace-events" -type f | xargs sed -i "s/pc-dimm/dimm/g" It prepares the work which abstracts dimm device type for both pc-dimm and nvdimm Signed-off-by: Xiao Guangrong --- hmp.c | 2 +- hw/acpi/ich9.c | 6 +- hw/acpi/memory_hotplug.c| 16 ++--- hw/acpi/piix4.c | 6 +- hw/i386/pc.c| 32 - hw/mem/pc-dimm.c| 148 hw/ppc/spapr.c | 18 ++--- include/hw/mem/pc-dimm.h| 62 - numa.c | 2 +- qapi-schema.json| 8 +-- qmp.c | 2 +- stubs/qmp_pc_dimm_device_list.c | 2 +- trace-events| 8 +-- 13 files changed, 156 insertions(+), 156 deletions(-) In the following patches, dimm is a parent for nv-dimm and pc-dimm, so dimm is more abstract when nv-dimm and pc-dimm are more concrete. So for me it is strange, that all these files, all old staff will use an abstract dimm. What the purpose of pc-dimm in this case (which appeared in the following patches)? The logic pc-dimm used can be completely shared by NVDIMM, so we abstracted 'dimm' from pc-dimm, and let the common code handle dimm instead of pc-dimm so that the common code will handle NVDIMM automatically. Actually, pc-dimm just inherit things from dimm, it does not have personal data. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH v6 14/33] pc-dimm: drop the prefix of pc-dimm
On 10/29/2015 11:56 PM, Xiao Guangrong wrote:
> This patch is generated by this script:
>
> find ./ -name "*.[ch]" -o -name "*.json" -o -name "trace-events" -type f \
> | xargs sed -i "s/PC_DIMM/DIMM/g"
>
> find ./ -name "*.[ch]" -o -name "*.json" -o -name "trace-events" -type f \
> | xargs sed -i "s/PCDIMM/DIMM/g"
>
> find ./ -name "*.[ch]" -o -name "*.json" -o -name "trace-events" -type f \
> | xargs sed -i "s/pc_dimm/dimm/g"
The '-type f' binds only to the '-name trace-events' clause, and not the
'name "*.[ch]"' or '-name "*.json"', but since we don't have any
directories by those names, it didn't matter. You probably could have
eliminated the -type f with no consequences, rather than adding "(" and
")". In fact, you could have used git rather than find to do it:
git grep -il 'pc_\?dimm' | xargs ...
which finds only one additional instance of pc_dimm in
stubs/Makefile.objs - so maybe you should:
git mv stubs/qmp_{pc_,}dimm_device_list.c
either in this patch or as a followup.
Could compress these three sed lines into one, if desired:
find ... | xargs sed -i 's/pc_\?\(dimm\)/\1/ig'
But doesn't really matter. I'm fine if you leave your commit message
as-is, and will let you decide what to do about the stub file name.
>
> find ./ -name "trace-events" -type f | xargs sed -i "s/pc-dimm/dimm/g"
>
> It prepares the work which abstracts dimm device type for both pc-dimm and
> nvdimm
>
> Signed-off-by: Xiao Guangrong
> ---
Reviewed-by: Eric Blake
> qapi-schema.json| 8 +--
Touches public interface files, but does not affect ABI, so it is safe.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v6 14/33] pc-dimm: drop the prefix of pc-dimm
On 30.10.2015 08:56, Xiao Guangrong wrote:
This patch is generated by this script:
find ./ -name "*.[ch]" -o -name "*.json" -o -name "trace-events" -type f \
| xargs sed -i "s/PC_DIMM/DIMM/g"
find ./ -name "*.[ch]" -o -name "*.json" -o -name "trace-events" -type f \
| xargs sed -i "s/PCDIMM/DIMM/g"
find ./ -name "*.[ch]" -o -name "*.json" -o -name "trace-events" -type f \
| xargs sed -i "s/pc_dimm/dimm/g"
find ./ -name "trace-events" -type f | xargs sed -i "s/pc-dimm/dimm/g"
It prepares the work which abstracts dimm device type for both pc-dimm and
nvdimm
Signed-off-by: Xiao Guangrong
---
hmp.c | 2 +-
hw/acpi/ich9.c | 6 +-
hw/acpi/memory_hotplug.c| 16 ++---
hw/acpi/piix4.c | 6 +-
hw/i386/pc.c| 32 -
hw/mem/pc-dimm.c| 148
hw/ppc/spapr.c | 18 ++---
include/hw/mem/pc-dimm.h| 62 -
numa.c | 2 +-
qapi-schema.json| 8 +--
qmp.c | 2 +-
stubs/qmp_pc_dimm_device_list.c | 2 +-
trace-events| 8 +--
13 files changed, 156 insertions(+), 156 deletions(-)
In the following patches, dimm is a parent for nv-dimm and pc-dimm, so
dimm is more abstract when nv-dimm and pc-dimm are more concrete. So for
me it is strange, that all these files, all old staff will use an
abstract dimm. What the purpose of pc-dimm in this case (which appeared
in the following patches)?
diff --git a/hmp.c b/hmp.c
index 5048eee..5c617d2 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1952,7 +1952,7 @@ void hmp_info_memory_devices(Monitor *mon, const QDict
*qdict)
MemoryDeviceInfoList *info_list = qmp_query_memory_devices(&err);
MemoryDeviceInfoList *info;
MemoryDeviceInfo *value;
-PCDIMMDeviceInfo *di;
+DIMMDeviceInfo *di;
for (info = info_list; info; info = info->next) {
value = info->value;
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 1c7fcfa..b0d6a67 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -440,7 +440,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm,
Error **errp)
void ich9_pm_device_plug_cb(ICH9LPCPMRegs *pm, DeviceState *dev, Error **errp)
{
if (pm->acpi_memory_hotplug.is_enabled &&
-object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+object_dynamic_cast(OBJECT(dev), TYPE_DIMM)) {
acpi_memory_plug_cb(&pm->acpi_regs, pm->irq, &pm->acpi_memory_hotplug,
dev, errp);
} else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
@@ -455,7 +455,7 @@ void ich9_pm_device_unplug_request_cb(ICH9LPCPMRegs *pm,
DeviceState *dev,
Error **errp)
{
if (pm->acpi_memory_hotplug.is_enabled &&
-object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+object_dynamic_cast(OBJECT(dev), TYPE_DIMM)) {
acpi_memory_unplug_request_cb(&pm->acpi_regs, pm->irq,
&pm->acpi_memory_hotplug, dev, errp);
} else {
@@ -468,7 +468,7 @@ void ich9_pm_device_unplug_cb(ICH9LPCPMRegs *pm,
DeviceState *dev,
Error **errp)
{
if (pm->acpi_memory_hotplug.is_enabled &&
-object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+object_dynamic_cast(OBJECT(dev), TYPE_DIMM)) {
acpi_memory_unplug_cb(&pm->acpi_memory_hotplug, dev, errp);
} else {
error_setg(errp, "acpi: device unplug for not supported device"
diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index ce428df..e687852 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -54,23 +54,23 @@ static uint64_t acpi_memory_hotplug_read(void *opaque,
hwaddr addr,
o = OBJECT(mdev->dimm);
switch (addr) {
case 0x0: /* Lo part of phys address where DIMM is mapped */
-val = o ? object_property_get_int(o, PC_DIMM_ADDR_PROP, NULL) : 0;
+val = o ? object_property_get_int(o, DIMM_ADDR_PROP, NULL) : 0;
trace_mhp_acpi_read_addr_lo(mem_st->selector, val);
break;
case 0x4: /* Hi part of phys address where DIMM is mapped */
-val = o ? object_property_get_int(o, PC_DIMM_ADDR_PROP, NULL) >> 32 :
0;
+val = o ? object_property_get_int(o, DIMM_ADDR_PROP, NULL) >> 32 : 0;
trace_mhp_acpi_read_addr_hi(mem_st->selector, val);
break;
case 0x8: /* Lo part of DIMM size */
-val = o ? object_property_get_int(o, PC_DIMM_SIZE_PROP, NULL) : 0;
+val = o ? object_property_get_int(o, DIMM_SIZE_PROP, NULL) : 0;
trace_mhp_acpi_read_size_lo(mem_st->selector, val);
break;
case 0xc: /* Hi part of DIMM size */
-val = o ? object_property_get_int(o, PC_DIMM_SIZE_PROP, NULL) >> 32 :
0;
+val = o ? object_property_get_int(o, DIMM_SIZE_PROP, N
[PATCH v6 14/33] pc-dimm: drop the prefix of pc-dimm
This patch is generated by this script:
find ./ -name "*.[ch]" -o -name "*.json" -o -name "trace-events" -type f \
| xargs sed -i "s/PC_DIMM/DIMM/g"
find ./ -name "*.[ch]" -o -name "*.json" -o -name "trace-events" -type f \
| xargs sed -i "s/PCDIMM/DIMM/g"
find ./ -name "*.[ch]" -o -name "*.json" -o -name "trace-events" -type f \
| xargs sed -i "s/pc_dimm/dimm/g"
find ./ -name "trace-events" -type f | xargs sed -i "s/pc-dimm/dimm/g"
It prepares the work which abstracts dimm device type for both pc-dimm and
nvdimm
Signed-off-by: Xiao Guangrong
---
hmp.c | 2 +-
hw/acpi/ich9.c | 6 +-
hw/acpi/memory_hotplug.c| 16 ++---
hw/acpi/piix4.c | 6 +-
hw/i386/pc.c| 32 -
hw/mem/pc-dimm.c| 148
hw/ppc/spapr.c | 18 ++---
include/hw/mem/pc-dimm.h| 62 -
numa.c | 2 +-
qapi-schema.json| 8 +--
qmp.c | 2 +-
stubs/qmp_pc_dimm_device_list.c | 2 +-
trace-events| 8 +--
13 files changed, 156 insertions(+), 156 deletions(-)
diff --git a/hmp.c b/hmp.c
index 5048eee..5c617d2 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1952,7 +1952,7 @@ void hmp_info_memory_devices(Monitor *mon, const QDict
*qdict)
MemoryDeviceInfoList *info_list = qmp_query_memory_devices(&err);
MemoryDeviceInfoList *info;
MemoryDeviceInfo *value;
-PCDIMMDeviceInfo *di;
+DIMMDeviceInfo *di;
for (info = info_list; info; info = info->next) {
value = info->value;
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 1c7fcfa..b0d6a67 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -440,7 +440,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm,
Error **errp)
void ich9_pm_device_plug_cb(ICH9LPCPMRegs *pm, DeviceState *dev, Error **errp)
{
if (pm->acpi_memory_hotplug.is_enabled &&
-object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+object_dynamic_cast(OBJECT(dev), TYPE_DIMM)) {
acpi_memory_plug_cb(&pm->acpi_regs, pm->irq, &pm->acpi_memory_hotplug,
dev, errp);
} else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
@@ -455,7 +455,7 @@ void ich9_pm_device_unplug_request_cb(ICH9LPCPMRegs *pm,
DeviceState *dev,
Error **errp)
{
if (pm->acpi_memory_hotplug.is_enabled &&
-object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+object_dynamic_cast(OBJECT(dev), TYPE_DIMM)) {
acpi_memory_unplug_request_cb(&pm->acpi_regs, pm->irq,
&pm->acpi_memory_hotplug, dev, errp);
} else {
@@ -468,7 +468,7 @@ void ich9_pm_device_unplug_cb(ICH9LPCPMRegs *pm,
DeviceState *dev,
Error **errp)
{
if (pm->acpi_memory_hotplug.is_enabled &&
-object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+object_dynamic_cast(OBJECT(dev), TYPE_DIMM)) {
acpi_memory_unplug_cb(&pm->acpi_memory_hotplug, dev, errp);
} else {
error_setg(errp, "acpi: device unplug for not supported device"
diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index ce428df..e687852 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -54,23 +54,23 @@ static uint64_t acpi_memory_hotplug_read(void *opaque,
hwaddr addr,
o = OBJECT(mdev->dimm);
switch (addr) {
case 0x0: /* Lo part of phys address where DIMM is mapped */
-val = o ? object_property_get_int(o, PC_DIMM_ADDR_PROP, NULL) : 0;
+val = o ? object_property_get_int(o, DIMM_ADDR_PROP, NULL) : 0;
trace_mhp_acpi_read_addr_lo(mem_st->selector, val);
break;
case 0x4: /* Hi part of phys address where DIMM is mapped */
-val = o ? object_property_get_int(o, PC_DIMM_ADDR_PROP, NULL) >> 32 :
0;
+val = o ? object_property_get_int(o, DIMM_ADDR_PROP, NULL) >> 32 : 0;
trace_mhp_acpi_read_addr_hi(mem_st->selector, val);
break;
case 0x8: /* Lo part of DIMM size */
-val = o ? object_property_get_int(o, PC_DIMM_SIZE_PROP, NULL) : 0;
+val = o ? object_property_get_int(o, DIMM_SIZE_PROP, NULL) : 0;
trace_mhp_acpi_read_size_lo(mem_st->selector, val);
break;
case 0xc: /* Hi part of DIMM size */
-val = o ? object_property_get_int(o, PC_DIMM_SIZE_PROP, NULL) >> 32 :
0;
+val = o ? object_property_get_int(o, DIMM_SIZE_PROP, NULL) >> 32 : 0;
trace_mhp_acpi_read_size_hi(mem_st->selector, val);
break;
case 0x10: /* node proximity for _PXM method */
-val = o ? object_property_get_int(o, PC_DIMM_NODE_PROP, NULL) : 0;
+val = o ? object_property_get_int(o, DIMM_NODE_PROP, NULL) : 0;
trace_mhp_acpi_read_pxm(mem_st->selector, val);
break;
case 0x14: /* pack and return is_* fields */
