| Message ID | 20230530113838.257755-9-david@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | memory-device: Some cleanups | expand |
On 30/5/23 13:38, David Hildenbrand wrote: > There are no remaining users in the tree, so let's remove it. > > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Richard Henderson <richard.henderson@linaro.org> > Cc: Eduardo Habkost <eduardo@habkost.net> > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > hw/i386/pc.c | 19 ------------------- > include/hw/i386/pc.h | 1 - > 2 files changed, 20 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On Tue, May 30, 2023 at 01:38:36PM +0200, David Hildenbrand wrote: > There are no remaining users in the tree, so let's remove it. > > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Richard Henderson <richard.henderson@linaro.org> > Cc: Eduardo Habkost <eduardo@habkost.net> > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> > Signed-off-by: David Hildenbrand <david@redhat.com> This (with previous patches) means any user changing device-memory-region-size machine property is now broken, right? How do we know there are no users? > --- > hw/i386/pc.c | 19 ------------------- > include/hw/i386/pc.h | 1 - > 2 files changed, 20 deletions(-) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 920aa32b53..c4789e2f35 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1646,21 +1646,6 @@ static HotplugHandler *pc_get_hotplug_handler(MachineState *machine, > return NULL; > } > > -static void > -pc_machine_get_device_memory_region_size(Object *obj, Visitor *v, > - const char *name, void *opaque, > - Error **errp) > -{ > - MachineState *ms = MACHINE(obj); > - int64_t value = 0; > - > - if (ms->device_memory) { > - value = memory_region_size(&ms->device_memory->mr); > - } > - > - visit_type_int(v, name, &value, errp); > -} > - > static void pc_machine_get_vmport(Object *obj, Visitor *v, const char *name, > void *opaque, Error **errp) > { > @@ -1980,10 +1965,6 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) > object_class_property_set_description(oc, PC_MACHINE_MAX_RAM_BELOW_4G, > "Maximum ram below the 4G boundary (32bit boundary)"); > > - object_class_property_add(oc, PC_MACHINE_DEVMEM_REGION_SIZE, "int", > - pc_machine_get_device_memory_region_size, NULL, > - NULL, NULL); > - > object_class_property_add(oc, PC_MACHINE_VMPORT, "OnOffAuto", > pc_machine_get_vmport, pc_machine_set_vmport, > NULL, NULL); > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > index c661e9cc80..6c9ad2d132 100644 > --- a/include/hw/i386/pc.h > +++ b/include/hw/i386/pc.h > @@ -60,7 +60,6 @@ typedef struct PCMachineState { > > #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device" > #define PC_MACHINE_MAX_RAM_BELOW_4G "max-ram-below-4g" > -#define PC_MACHINE_DEVMEM_REGION_SIZE "device-memory-region-size" > #define PC_MACHINE_VMPORT "vmport" > #define PC_MACHINE_SMBUS "smbus" > #define PC_MACHINE_SATA "sata" > -- > 2.40.1
On 30.05.23 15:07, Michael S. Tsirkin wrote: > On Tue, May 30, 2023 at 01:38:36PM +0200, David Hildenbrand wrote: >> There are no remaining users in the tree, so let's remove it. >> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: Richard Henderson <richard.henderson@linaro.org> >> Cc: Eduardo Habkost <eduardo@habkost.net> >> Cc: "Michael S. Tsirkin" <mst@redhat.com> >> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> >> Signed-off-by: David Hildenbrand <david@redhat.com> > > > This (with previous patches) means any user changing > device-memory-region-size machine property is now broken, right? We only had a getter, no setter (for good reason). > How do we know there are no users? We don't. A quick google search makes "device-memory-region-size" and "qom-get" only pop up in BUG fixes for something that appears to be QEMU developer driven. I don't consider it any useful, but if we want to be careful, sure we can leave it around.
On 30.05.23 15:11, David Hildenbrand wrote: > On 30.05.23 15:07, Michael S. Tsirkin wrote: >> On Tue, May 30, 2023 at 01:38:36PM +0200, David Hildenbrand wrote: >>> There are no remaining users in the tree, so let's remove it. >>> >>> Cc: Paolo Bonzini <pbonzini@redhat.com> >>> Cc: Richard Henderson <richard.henderson@linaro.org> >>> Cc: Eduardo Habkost <eduardo@habkost.net> >>> Cc: "Michael S. Tsirkin" <mst@redhat.com> >>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> >>> Signed-off-by: David Hildenbrand <david@redhat.com> >> >> >> This (with previous patches) means any user changing >> device-memory-region-size machine property is now broken, right? > > We only had a getter, no setter (for good reason). > >> How do we know there are no users? > > We don't. A quick google search makes "device-memory-region-size" and > "qom-get" only pop up in BUG fixes for something that appears to be QEMU > developer driven. > > I don't consider it any useful, but if we want to be careful, sure we > can leave it around. Looking further, libvirt doesn't use it (and never used it). I already renamed it in 2018 without anybody complaining: https://www.mail-archive.com/qemu-devel@nongnu.org/msg532101.html So I'm quite confident that nobody will really miss this property.
On 30/5/23 15:11, David Hildenbrand wrote: > On 30.05.23 15:07, Michael S. Tsirkin wrote: >> On Tue, May 30, 2023 at 01:38:36PM +0200, David Hildenbrand wrote: >>> There are no remaining users in the tree, so let's remove it. >>> >>> Cc: Paolo Bonzini <pbonzini@redhat.com> >>> Cc: Richard Henderson <richard.henderson@linaro.org> >>> Cc: Eduardo Habkost <eduardo@habkost.net> >>> Cc: "Michael S. Tsirkin" <mst@redhat.com> >>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> >>> Signed-off-by: David Hildenbrand <david@redhat.com> >> >> >> This (with previous patches) means any user changing >> device-memory-region-size machine property is now broken, right? > > We only had a getter, no setter (for good reason). > >> How do we know there are no users? > > We don't. A quick google search makes "device-memory-region-size" and > "qom-get" only pop up in BUG fixes for something that appears to be QEMU > developer driven. This was my analysis. > I don't consider it any useful, but if we want to be careful, sure we > can leave it around. If we want to keep it, we should move it to generic code IMHO, not PC machine. Otherwise the less unused code the better :)
On Tue, May 30, 2023 at 03:41:58PM +0200, David Hildenbrand wrote: > On 30.05.23 15:11, David Hildenbrand wrote: > > On 30.05.23 15:07, Michael S. Tsirkin wrote: > > > On Tue, May 30, 2023 at 01:38:36PM +0200, David Hildenbrand wrote: > > > > There are no remaining users in the tree, so let's remove it. > > > > > > > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > > > Cc: Richard Henderson <richard.henderson@linaro.org> > > > > Cc: Eduardo Habkost <eduardo@habkost.net> > > > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > > > > Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> > > > > Signed-off-by: David Hildenbrand <david@redhat.com> > > > > > > > > > This (with previous patches) means any user changing > > > device-memory-region-size machine property is now broken, right? > > > > We only had a getter, no setter (for good reason). > > > > > How do we know there are no users? > > > > We don't. A quick google search makes "device-memory-region-size" and > > "qom-get" only pop up in BUG fixes for something that appears to be QEMU > > developer driven. > > > > I don't consider it any useful, but if we want to be careful, sure we > > can leave it around. > > Looking further, libvirt doesn't use it (and never used it). > > I already renamed it in 2018 without anybody complaining: > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg532101.html > > So I'm quite confident that nobody will really miss this property. > > -- > Thanks, > > David / dhildenb OK. In the future we need to be careful and use "x-" prefix for what we don't want to expose.
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 920aa32b53..c4789e2f35 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1646,21 +1646,6 @@ static HotplugHandler *pc_get_hotplug_handler(MachineState *machine, return NULL; } -static void -pc_machine_get_device_memory_region_size(Object *obj, Visitor *v, - const char *name, void *opaque, - Error **errp) -{ - MachineState *ms = MACHINE(obj); - int64_t value = 0; - - if (ms->device_memory) { - value = memory_region_size(&ms->device_memory->mr); - } - - visit_type_int(v, name, &value, errp); -} - static void pc_machine_get_vmport(Object *obj, Visitor *v, const char *name, void *opaque, Error **errp) { @@ -1980,10 +1965,6 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) object_class_property_set_description(oc, PC_MACHINE_MAX_RAM_BELOW_4G, "Maximum ram below the 4G boundary (32bit boundary)"); - object_class_property_add(oc, PC_MACHINE_DEVMEM_REGION_SIZE, "int", - pc_machine_get_device_memory_region_size, NULL, - NULL, NULL); - object_class_property_add(oc, PC_MACHINE_VMPORT, "OnOffAuto", pc_machine_get_vmport, pc_machine_set_vmport, NULL, NULL); diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index c661e9cc80..6c9ad2d132 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -60,7 +60,6 @@ typedef struct PCMachineState { #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device" #define PC_MACHINE_MAX_RAM_BELOW_4G "max-ram-below-4g" -#define PC_MACHINE_DEVMEM_REGION_SIZE "device-memory-region-size" #define PC_MACHINE_VMPORT "vmport" #define PC_MACHINE_SMBUS "smbus" #define PC_MACHINE_SATA "sata"
There are no remaining users in the tree, so let's remove it. Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Richard Henderson <richard.henderson@linaro.org> Cc: Eduardo Habkost <eduardo@habkost.net> Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> Signed-off-by: David Hildenbrand <david@redhat.com> --- hw/i386/pc.c | 19 ------------------- include/hw/i386/pc.h | 1 - 2 files changed, 20 deletions(-)