Re: [PATCH] hw/ide: Add the possibility to disable the CompactFlash device in the build

2024-02-01 Thread BALATON Zoltan 

On Thu, 1 Feb 2024, Thomas Huth wrote:

On 01/02/2024 13.54, BALATON Zoltan wrote:

On Thu, 1 Feb 2024, Thomas Huth wrote:

On 01/02/2024 13.39, BALATON Zoltan wrote:

On Thu, 1 Feb 2024, Thomas Huth wrote:
For distros like downstream RHEL, it would be helpful to allow to 
disable

the CompactFlash device. For making this possible, we need a separate
Kconfig switch for this device, and the code should reside in a separate
file.

Signed-off-by: Thomas Huth 
---
hw/ide/qdev-ide.h  | 41 
hw/ide/cf.c    | 58 ++
hw/ide/qdev.c  | 51 ++--
hw/ide/Kconfig |  4 
hw/ide/meson.build |  1 +
5 files changed, 106 insertions(+), 49 deletions(-)
create mode 100644 hw/ide/qdev-ide.h
create mode 100644 hw/ide/cf.c

diff --git a/hw/ide/qdev-ide.h b/hw/ide/qdev-ide.h
new file mode 100644
index 00..3dd977466c
--- /dev/null
+++ b/hw/ide/qdev-ide.h


This may be unrelated to this patch but we already have 
include/hw/ide/internal.h which may be a place these should go in but 
that header is in inlcude because some files outside hw/ide include it. 
I've found three places that include ide/internal.h: hw/arm/sbsa-ref.c, 
hw/i386/pc.c and hw/misc/macio.h. Only macio is really needing internal 
IDE parts the other two just uses some functions so macio is probably the 
reason this wasn't cleaned up yet. In any case, maybe this could go in 
include/hw/ide/internal.h to avoid introducing a new header or somehow 
make this a local header where non-public parts of hw/ide/internal.h 
could be moved in the future. Such as rename include/hw/ide/internal.h to 
ide.h and name this one internal.h maybe?


I don't like headers that much that just collect a lot of only slightly 
related things. That only causes problems again when you have to 
unentangle the stuff one day. So what's wrong with having a dedicated 
header for the stuff in hw/ide/qdev.c ?


Maybe that it's not obvious from the name that it belongs to qdev.c as the 
names are not the same.


I didn't want to just name the header "qdev.h" since that could easily be 
confused in #include statements...
IMHO qdev.c is already a very bad idea for a file name here... maybe 
something like ide-dev.c and ide-dev.h would be better?


The comment at the beginning of qdev.c says "ide bus support" but a lot of 
functions in it have ide_dev prefix. Maybe this comes from the original 
qdev-ification of this code and then adding some more unrelated parts in 
this file. It's clearly beyond scope of this patch to clean all this but 
since you're changing it maybe this is a good opportunity to reduce the 
mess a bit. I'm OK with renaming qdev.c to ide-dev.c or qdev-ide.c or 
whatever else as long as it's clear that the header belongs to the c file.


Also some of the qdev stuff that should be in this header are in 
include/hw/ide/internal.h so these will still be split arbitrarily.


Oh, well, it seems to be a mess already... hw/ide/pci.h includes the 
internal.h header and thus opens the internal definitions to all PCI-based 
IDE devices :-/


I've missed that so besides files directly including internal.h there 
could be others using some stuff from it through pci.h so this makes it 
more difficult to untangle.


If we can agree on a better name for qdev-ide.h, I can try to clean that mess 
up a little bit...


My comment was only that if you make changes then do it in a way that 
leaves the possibility to move stuff here to clean the current situation 
but if you can also do some of it now that's even better but not required.


Regards,
BALATON Zoltan

Re: [PATCH] hw/ide: Add the possibility to disable the CompactFlash device in the build

2024-02-01 Thread Thomas Huth 

On 01/02/2024 13.54, BALATON Zoltan wrote:

On Thu, 1 Feb 2024, Thomas Huth wrote:

On 01/02/2024 13.39, BALATON Zoltan wrote:

On Thu, 1 Feb 2024, Thomas Huth wrote:

For distros like downstream RHEL, it would be helpful to allow to disable
the CompactFlash device. For making this possible, we need a separate
Kconfig switch for this device, and the code should reside in a separate
file.

Signed-off-by: Thomas Huth 
---
hw/ide/qdev-ide.h  | 41 
hw/ide/cf.c    | 58 ++
hw/ide/qdev.c  | 51 ++--
hw/ide/Kconfig |  4 
hw/ide/meson.build |  1 +
5 files changed, 106 insertions(+), 49 deletions(-)
create mode 100644 hw/ide/qdev-ide.h
create mode 100644 hw/ide/cf.c

diff --git a/hw/ide/qdev-ide.h b/hw/ide/qdev-ide.h
new file mode 100644
index 00..3dd977466c
--- /dev/null
+++ b/hw/ide/qdev-ide.h


This may be unrelated to this patch but we already have 
include/hw/ide/internal.h which may be a place these should go in but 
that header is in inlcude because some files outside hw/ide include it. 
I've found three places that include ide/internal.h: hw/arm/sbsa-ref.c, 
hw/i386/pc.c and hw/misc/macio.h. Only macio is really needing internal 
IDE parts the other two just uses some functions so macio is probably the 
reason this wasn't cleaned up yet. In any case, maybe this could go in 
include/hw/ide/internal.h to avoid introducing a new header or somehow 
make this a local header where non-public parts of hw/ide/internal.h 
could be moved in the future. Such as rename include/hw/ide/internal.h to 
ide.h and name this one internal.h maybe?


I don't like headers that much that just collect a lot of only slightly 
related things. That only causes problems again when you have to 
unentangle the stuff one day. So what's wrong with having a dedicated 
header for the stuff in hw/ide/qdev.c ?


Maybe that it's not obvious from the name that it belongs to qdev.c as the 
names are not the same.


I didn't want to just name the header "qdev.h" since that could easily be 
confused in #include statements...
IMHO qdev.c is already a very bad idea for a file name here... maybe 
something like ide-dev.c and ide-dev.h would be better?


Also some of the qdev stuff that should be in this 
header are in include/hw/ide/internal.h so these will still be split 
arbitrarily.


Oh, well, it seems to be a mess already... hw/ide/pci.h includes the 
internal.h header and thus opens the internal definitions to all PCI-based 
IDE devices :-/


If we can agree on a better name for qdev-ide.h, I can try to clean that 
mess up a little bit...


 Thomas

Re: [PATCH] hw/ide: Add the possibility to disable the CompactFlash device in the build

2024-02-01 Thread BALATON Zoltan 

On Thu, 1 Feb 2024, Thomas Huth wrote:

On 01/02/2024 13.39, BALATON Zoltan wrote:

On Thu, 1 Feb 2024, Thomas Huth wrote:

For distros like downstream RHEL, it would be helpful to allow to disable
the CompactFlash device. For making this possible, we need a separate
Kconfig switch for this device, and the code should reside in a separate
file.

Signed-off-by: Thomas Huth 
---
hw/ide/qdev-ide.h  | 41 
hw/ide/cf.c    | 58 ++
hw/ide/qdev.c  | 51 ++--
hw/ide/Kconfig |  4 
hw/ide/meson.build |  1 +
5 files changed, 106 insertions(+), 49 deletions(-)
create mode 100644 hw/ide/qdev-ide.h
create mode 100644 hw/ide/cf.c

diff --git a/hw/ide/qdev-ide.h b/hw/ide/qdev-ide.h
new file mode 100644
index 00..3dd977466c
--- /dev/null
+++ b/hw/ide/qdev-ide.h


This may be unrelated to this patch but we already have 
include/hw/ide/internal.h which may be a place these should go in but that 
header is in inlcude because some files outside hw/ide include it. I've 
found three places that include ide/internal.h: hw/arm/sbsa-ref.c, 
hw/i386/pc.c and hw/misc/macio.h. Only macio is really needing internal IDE 
parts the other two just uses some functions so macio is probably the 
reason this wasn't cleaned up yet. In any case, maybe this could go in 
include/hw/ide/internal.h to avoid introducing a new header or somehow make 
this a local header where non-public parts of hw/ide/internal.h could be 
moved in the future. Such as rename include/hw/ide/internal.h to ide.h and 
name this one internal.h maybe?


I don't like headers that much that just collect a lot of only slightly 
related things. That only causes problems again when you have to unentangle 
the stuff one day. So what's wrong with having a dedicated header for the 
stuff in hw/ide/qdev.c ?


Maybe that it's not obvious from the name that it belongs to qdev.c as the 
names are not the same. Also some of the qdev stuff that should be in this 
header are in include/hw/ide/internal.h so these will still be split 
arbitrarily.


Regards,
BALATON Zoltan

Re: [PATCH] hw/ide: Add the possibility to disable the CompactFlash device in the build

2024-02-01 Thread Thomas Huth 

On 01/02/2024 13.39, BALATON Zoltan wrote:

On Thu, 1 Feb 2024, Thomas Huth wrote:

For distros like downstream RHEL, it would be helpful to allow to disable
the CompactFlash device. For making this possible, we need a separate
Kconfig switch for this device, and the code should reside in a separate
file.

Signed-off-by: Thomas Huth 
---
hw/ide/qdev-ide.h  | 41 
hw/ide/cf.c    | 58 ++
hw/ide/qdev.c  | 51 ++--
hw/ide/Kconfig |  4 
hw/ide/meson.build |  1 +
5 files changed, 106 insertions(+), 49 deletions(-)
create mode 100644 hw/ide/qdev-ide.h
create mode 100644 hw/ide/cf.c

diff --git a/hw/ide/qdev-ide.h b/hw/ide/qdev-ide.h
new file mode 100644
index 00..3dd977466c
--- /dev/null
+++ b/hw/ide/qdev-ide.h


This may be unrelated to this patch but we already have 
include/hw/ide/internal.h which may be a place these should go in but that 
header is in inlcude because some files outside hw/ide include it. I've 
found three places that include ide/internal.h: hw/arm/sbsa-ref.c, 
hw/i386/pc.c and hw/misc/macio.h. Only macio is really needing internal IDE 
parts the other two just uses some functions so macio is probably the reason 
this wasn't cleaned up yet. In any case, maybe this could go in 
include/hw/ide/internal.h to avoid introducing a new header or somehow make 
this a local header where non-public parts of hw/ide/internal.h could be 
moved in the future. Such as rename include/hw/ide/internal.h to ide.h and 
name this one internal.h maybe?


I don't like headers that much that just collect a lot of only slightly 
related things. That only causes problems again when you have to unentangle 
the stuff one day. So what's wrong with having a dedicated header for the 
stuff in hw/ide/qdev.c ?


 Thomas

Re: [PATCH] hw/ide: Add the possibility to disable the CompactFlash device in the build

2024-02-01 Thread BALATON Zoltan 

On Thu, 1 Feb 2024, Thomas Huth wrote:

For distros like downstream RHEL, it would be helpful to allow to disable
the CompactFlash device. For making this possible, we need a separate
Kconfig switch for this device, and the code should reside in a separate
file.

Signed-off-by: Thomas Huth 
---
hw/ide/qdev-ide.h  | 41 
hw/ide/cf.c| 58 ++
hw/ide/qdev.c  | 51 ++--
hw/ide/Kconfig |  4 
hw/ide/meson.build |  1 +
5 files changed, 106 insertions(+), 49 deletions(-)
create mode 100644 hw/ide/qdev-ide.h
create mode 100644 hw/ide/cf.c

diff --git a/hw/ide/qdev-ide.h b/hw/ide/qdev-ide.h
new file mode 100644
index 00..3dd977466c
--- /dev/null
+++ b/hw/ide/qdev-ide.h


This may be unrelated to this patch but we already have 
include/hw/ide/internal.h which may be a place these should go in but that 
header is in inlcude because some files outside hw/ide include it. I've 
found three places that include ide/internal.h: hw/arm/sbsa-ref.c, 
hw/i386/pc.c and hw/misc/macio.h. Only macio is really needing internal 
IDE parts the other two just uses some functions so macio is probably the 
reason this wasn't cleaned up yet. In any case, maybe this could go in 
include/hw/ide/internal.h to avoid introducing a new header or somehow 
make this a local header where non-public parts of hw/ide/internal.h could 
be moved in the future. Such as rename include/hw/ide/internal.h to ide.h 
and name this one internal.h maybe?


Regards,
BALATON Zoltan


@@ -0,0 +1,41 @@
+/*
+ * ide bus support for qdev.
+ *
+ * Copyright (c) 2009 Gerd Hoffmann 
+ *
+ * This code is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see .
+ */
+
+#ifndef QDEV_IDE_H
+#define QDEV_IDE_H
+
+#include "hw/qdev-properties.h"
+#include "hw/block/block.h"
+#include "hw/ide/internal.h"
+
+typedef struct IDEDrive {
+IDEDevice dev;
+} IDEDrive;
+
+#define DEFINE_IDE_DEV_PROPERTIES() \
+DEFINE_BLOCK_PROPERTIES(IDEDrive, dev.conf),\
+DEFINE_BLOCK_ERROR_PROPERTIES(IDEDrive, dev.conf),  \
+DEFINE_PROP_STRING("ver",  IDEDrive, dev.version),  \
+DEFINE_PROP_UINT64("wwn",  IDEDrive, dev.wwn, 0),   \
+DEFINE_PROP_STRING("serial",  IDEDrive, dev.serial),\
+DEFINE_PROP_STRING("model", IDEDrive, dev.model)
+
+void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp);
+
+#endif
diff --git a/hw/ide/cf.c b/hw/ide/cf.c
new file mode 100644
index 00..0b4bb57591
--- /dev/null
+++ b/hw/ide/cf.c
@@ -0,0 +1,58 @@
+/*
+ * ide CompactFlash support
+ *
+ * This code is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see .
+ */
+
+#include "qemu/osdep.h"
+#include "hw/ide/qdev-ide.h"
+#include "qapi/qapi-types-block.h"
+
+static void ide_cf_realize(IDEDevice *dev, Error **errp)
+{
+ide_dev_initfn(dev, IDE_CFATA, errp);
+}
+
+static Property ide_cf_properties[] = {
+DEFINE_IDE_DEV_PROPERTIES(),
+DEFINE_BLOCK_CHS_PROPERTIES(IDEDrive, dev.conf),
+DEFINE_PROP_BIOS_CHS_TRANS("bios-chs-trans",
+IDEDrive, dev.chs_trans, BIOS_ATA_TRANSLATION_AUTO),
+DEFINE_PROP_END_OF_LIST(),
+};
+
+static void ide_cf_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+IDEDeviceClass *k = IDE_DEVICE_CLASS(klass);
+
+k->realize  = ide_cf_realize;
+dc->fw_name = "drive";
+dc->desc= "virtual CompactFlash card";
+device_class_set_props(dc, ide_cf_properties);
+}
+
+static const TypeInfo ide_cf_info = {
+.name  = "ide-cf",
+.parent= TYPE_IDE_DEVICE,
+.instance_size = sizeof(IDEDrive),
+.class_init= ide_cf_class_init,
+};
+
+static void ide_cf_register_type(void)
+{
+type_register_stat

[PATCH] hw/ide: Add the possibility to disable the CompactFlash device in the build

2024-02-01 Thread Thomas Huth 
For distros like downstream RHEL, it would be helpful to allow to disable
the CompactFlash device. For making this possible, we need a separate
Kconfig switch for this device, and the code should reside in a separate
file.

Signed-off-by: Thomas Huth 
---
 hw/ide/qdev-ide.h  | 41 
 hw/ide/cf.c| 58 ++
 hw/ide/qdev.c  | 51 ++--
 hw/ide/Kconfig |  4 
 hw/ide/meson.build |  1 +
 5 files changed, 106 insertions(+), 49 deletions(-)
 create mode 100644 hw/ide/qdev-ide.h
 create mode 100644 hw/ide/cf.c

diff --git a/hw/ide/qdev-ide.h b/hw/ide/qdev-ide.h
new file mode 100644
index 00..3dd977466c
--- /dev/null
+++ b/hw/ide/qdev-ide.h
@@ -0,0 +1,41 @@
+/*
+ * ide bus support for qdev.
+ *
+ * Copyright (c) 2009 Gerd Hoffmann 
+ *
+ * This code is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see .
+ */
+
+#ifndef QDEV_IDE_H
+#define QDEV_IDE_H
+
+#include "hw/qdev-properties.h"
+#include "hw/block/block.h"
+#include "hw/ide/internal.h"
+
+typedef struct IDEDrive {
+IDEDevice dev;
+} IDEDrive;
+
+#define DEFINE_IDE_DEV_PROPERTIES() \
+DEFINE_BLOCK_PROPERTIES(IDEDrive, dev.conf),\
+DEFINE_BLOCK_ERROR_PROPERTIES(IDEDrive, dev.conf),  \
+DEFINE_PROP_STRING("ver",  IDEDrive, dev.version),  \
+DEFINE_PROP_UINT64("wwn",  IDEDrive, dev.wwn, 0),   \
+DEFINE_PROP_STRING("serial",  IDEDrive, dev.serial),\
+DEFINE_PROP_STRING("model", IDEDrive, dev.model)
+
+void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp);
+
+#endif
diff --git a/hw/ide/cf.c b/hw/ide/cf.c
new file mode 100644
index 00..0b4bb57591
--- /dev/null
+++ b/hw/ide/cf.c
@@ -0,0 +1,58 @@
+/*
+ * ide CompactFlash support
+ *
+ * This code is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see .
+ */
+
+#include "qemu/osdep.h"
+#include "hw/ide/qdev-ide.h"
+#include "qapi/qapi-types-block.h"
+
+static void ide_cf_realize(IDEDevice *dev, Error **errp)
+{
+ide_dev_initfn(dev, IDE_CFATA, errp);
+}
+
+static Property ide_cf_properties[] = {
+DEFINE_IDE_DEV_PROPERTIES(),
+DEFINE_BLOCK_CHS_PROPERTIES(IDEDrive, dev.conf),
+DEFINE_PROP_BIOS_CHS_TRANS("bios-chs-trans",
+IDEDrive, dev.chs_trans, BIOS_ATA_TRANSLATION_AUTO),
+DEFINE_PROP_END_OF_LIST(),
+};
+
+static void ide_cf_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+IDEDeviceClass *k = IDE_DEVICE_CLASS(klass);
+
+k->realize  = ide_cf_realize;
+dc->fw_name = "drive";
+dc->desc= "virtual CompactFlash card";
+device_class_set_props(dc, ide_cf_properties);
+}
+
+static const TypeInfo ide_cf_info = {
+.name  = "ide-cf",
+.parent= TYPE_IDE_DEVICE,
+.instance_size = sizeof(IDEDrive),
+.class_init= ide_cf_class_init,
+};
+
+static void ide_cf_register_type(void)
+{
+type_register_static(&ide_cf_info);
+}
+
+type_init(ide_cf_register_type)
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 1b3b4da01d..a2f2d0ea08 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -24,12 +24,9 @@
 #include "qemu/error-report.h"
 #include "qemu/main-loop.h"
 #include "qemu/module.h"
-#include "hw/ide/internal.h"
-#include "hw/qdev-properties.h"
-#include "hw/qdev-properties-system.h"
+#include "hw/ide/qdev-ide.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
-#include "hw/block/block.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/runstate.h"
 #include "qapi/visitor.h"
@@ -158,11 +155,7 @@ int ide_get_bios_chs_trans(BusState *bus, int unit)
 
 /* - */
 
-typedef struct IDEDrive {
-IDEDevice dev;
-} IDEDrive;
-
-static void ide_dev_initfn(IDEDevice *dev,