[Qemu-devel] [PATCH 0/3] scsi: remove lsi53c895a_create() and lsi53c810_create() functions

[Qemu-devel] [PATCH 0/3] scsi: remove lsi53c895a_create() and lsi53c810_create() functions

[Mark Cave-Ayland]

As part of an upcoming 40p patchset I have a requirement to change the PCI
configuration of the LSI SCSI. However since commits a64aa5785d "hw: Deprecate -drive
if=scsi with non-onboard HBAs" and b891538e81 "hw/ppc/prep: Fix implicit creation of
"-drive if=scsi", the lsi53c8*_create() wrapper functions don't return the device
state itself.

Rather than altering these functions to return PCIDevice I simply went ahead and split
the LSIState structure and QOM types into a new lsi53c895a.h file, as is fairly
standard QEMU practice.

Not only does this enable me to change the PCI configuration of the LSI SCSI device
in an upcoming patchset, but it also allows full access to LSIState if required
(which is fairly similar to how the code was arranged before a64aa5785d).

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


Mark Cave-Ayland (3):
  scsi: move lsi53c895a structures and defines into separate
    lsi53c895a.h file
  scsi: move lsi53c895a_create() and lsi53c810_create() callers to
    pci_create_simple()
  scsi: remove unused lsi53c895a_create() and lsi53c810_create()
    functions

 hw/arm/realview.c            |   5 +-
 hw/arm/versatilepb.c         |   5 +-
 hw/hppa/machine.c            |   5 +-
 hw/ppc/prep.c                |   6 +-
 hw/scsi/lsi53c895a.c         | 130 +---------------------------------------
 include/hw/pci/pci.h         |   3 -
 include/hw/scsi/lsi53c895a.h | 137 +++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 155 insertions(+), 136 deletions(-)
 create mode 100644 include/hw/scsi/lsi53c895a.h

-- 
2.11.0

[Qemu-devel] [PATCH 1/3] scsi: move lsi53c895a structures and defines into separate lsi53c895a.h file

[Mark Cave-Ayland]

There is also one small change to the new header file which is the addition
of the previously missing LSI53C810 define.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/lsi53c895a.c         | 116 +-----------------------------------
 include/hw/scsi/lsi53c895a.h | 137 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 138 insertions(+), 115 deletions(-)
 create mode 100644 include/hw/scsi/lsi53c895a.h

diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index 955ba94800..edb0b13e23 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -18,6 +18,7 @@
 #include "hw/hw.h"
 #include "hw/pci/pci.h"
 #include "hw/scsi/scsi.h"
+#include "hw/scsi/lsi53c895a.h"
 #include "sysemu/dma.h"
 #include "qemu/log.h"
 
@@ -186,124 +187,9 @@ static const char *names[] = {
 #define PHASE_MI          7
 #define PHASE_MASK        7
 
-/* Maximum length of MSG IN data.  */
-#define LSI_MAX_MSGIN_LEN 8
-
 /* Flag set if this is a tagged command.  */
 #define LSI_TAG_VALID     (1 << 16)
 
-typedef struct lsi_request {
-    SCSIRequest *req;
-    uint32_t tag;
-    uint32_t dma_len;
-    uint8_t *dma_buf;
-    uint32_t pending;
-    int out;
-    QTAILQ_ENTRY(lsi_request) next;
-} lsi_request;
-
-typedef struct {
-    /*< private >*/
-    PCIDevice parent_obj;
-    /*< public >*/
-
-    MemoryRegion mmio_io;
-    MemoryRegion ram_io;
-    MemoryRegion io_io;
-    AddressSpace pci_io_as;
-
-    int carry; /* ??? Should this be an a visible register somewhere?  */
-    int status;
-    /* Action to take at the end of a MSG IN phase.
-       0 = COMMAND, 1 = disconnect, 2 = DATA OUT, 3 = DATA IN.  */
-    int msg_action;
-    int msg_len;
-    uint8_t msg[LSI_MAX_MSGIN_LEN];
-    /* 0 if SCRIPTS are running or stopped.
-     * 1 if a Wait Reselect instruction has been issued.
-     * 2 if processing DMA from lsi_execute_script.
-     * 3 if a DMA operation is in progress.  */
-    int waiting;
-    SCSIBus bus;
-    int current_lun;
-    /* The tag is a combination of the device ID and the SCSI tag.  */
-    uint32_t select_tag;
-    int command_complete;
-    QTAILQ_HEAD(, lsi_request) queue;
-    lsi_request *current;
-
-    uint32_t dsa;
-    uint32_t temp;
-    uint32_t dnad;
-    uint32_t dbc;
-    uint8_t istat0;
-    uint8_t istat1;
-    uint8_t dcmd;
-    uint8_t dstat;
-    uint8_t dien;
-    uint8_t sist0;
-    uint8_t sist1;
-    uint8_t sien0;
-    uint8_t sien1;
-    uint8_t mbox0;
-    uint8_t mbox1;
-    uint8_t dfifo;
-    uint8_t ctest2;
-    uint8_t ctest3;
-    uint8_t ctest4;
-    uint8_t ctest5;
-    uint8_t ccntl0;
-    uint8_t ccntl1;
-    uint32_t dsp;
-    uint32_t dsps;
-    uint8_t dmode;
-    uint8_t dcntl;
-    uint8_t scntl0;
-    uint8_t scntl1;
-    uint8_t scntl2;
-    uint8_t scntl3;
-    uint8_t sstat0;
-    uint8_t sstat1;
-    uint8_t scid;
-    uint8_t sxfer;
-    uint8_t socl;
-    uint8_t sdid;
-    uint8_t ssid;
-    uint8_t sfbr;
-    uint8_t stest1;
-    uint8_t stest2;
-    uint8_t stest3;
-    uint8_t sidl;
-    uint8_t stime0;
-    uint8_t respid0;
-    uint8_t respid1;
-    uint32_t mmrs;
-    uint32_t mmws;
-    uint32_t sfs;
-    uint32_t drs;
-    uint32_t sbms;
-    uint32_t dbms;
-    uint32_t dnad64;
-    uint32_t pmjad1;
-    uint32_t pmjad2;
-    uint32_t rbc;
-    uint32_t ua;
-    uint32_t ia;
-    uint32_t sbc;
-    uint32_t csbc;
-    uint32_t scratch[18]; /* SCRATCHA-SCRATCHR */
-    uint8_t sbr;
-    uint32_t adder;
-
-    /* Script ram is stored as 32-bit words in host byteorder.  */
-    uint32_t script_ram[2048];
-} LSIState;
-
-#define TYPE_LSI53C810  "lsi53c810"
-#define TYPE_LSI53C895A "lsi53c895a"
-
-#define LSI53C895A(obj) \
-    OBJECT_CHECK(LSIState, (obj), TYPE_LSI53C895A)
 
 static inline int lsi_irq_on_rsl(LSIState *s)
 {
diff --git a/include/hw/scsi/lsi53c895a.h b/include/hw/scsi/lsi53c895a.h
new file mode 100644
index 0000000000..d80cb78c69
--- /dev/null
+++ b/include/hw/scsi/lsi53c895a.h
@@ -0,0 +1,137 @@
+/*
+ * QEMU LSI53C895A SCSI Host Bus Adapter emulation
+ *
+ * Copyright (c) 2006 CodeSourcery.
+ * Written by Paul Brook
+ *
+ * This code is licensed under the LGPL.
+ */
+
+#ifndef LSI_H
+#define LSI_H
+
+#include "qemu/osdep.h"
+
+#include "hw/hw.h"
+#include "hw/pci/pci.h"
+#include "hw/scsi/scsi.h"
+
+/* Maximum length of MSG IN data.  */
+#define LSI_MAX_MSGIN_LEN 8
+
+typedef struct lsi_request {
+    SCSIRequest *req;
+    uint32_t tag;
+    uint32_t dma_len;
+    uint8_t *dma_buf;
+    uint32_t pending;
+    int out;
+    QTAILQ_ENTRY(lsi_request) next;
+} lsi_request;
+
+typedef struct {
+    /*< private >*/
+    PCIDevice parent_obj;
+    /*< public >*/
+
+    MemoryRegion mmio_io;
+    MemoryRegion ram_io;
+    MemoryRegion io_io;
+    AddressSpace pci_io_as;
+
+    int carry; /* ??? Should this be an a visible register somewhere?  */
+    int status;
+    /* Action to take at the end of a MSG IN phase.
+       0 = COMMAND, 1 = disconnect, 2 = DATA OUT, 3 = DATA IN.  */
+    int msg_action;
+    int msg_len;
+    uint8_t msg[LSI_MAX_MSGIN_LEN];
+    /* 0 if SCRIPTS are running or stopped.
+     * 1 if a Wait Reselect instruction has been issued.
+     * 2 if processing DMA from lsi_execute_script.
+     * 3 if a DMA operation is in progress.  */
+    int waiting;
+    SCSIBus bus;
+    int current_lun;
+    /* The tag is a combination of the device ID and the SCSI tag.  */
+    uint32_t select_tag;
+    int command_complete;
+    QTAILQ_HEAD(, lsi_request) queue;
+    lsi_request *current;
+
+    uint32_t dsa;
+    uint32_t temp;
+    uint32_t dnad;
+    uint32_t dbc;
+    uint8_t istat0;
+    uint8_t istat1;
+    uint8_t dcmd;
+    uint8_t dstat;
+    uint8_t dien;
+    uint8_t sist0;
+    uint8_t sist1;
+    uint8_t sien0;
+    uint8_t sien1;
+    uint8_t mbox0;
+    uint8_t mbox1;
+    uint8_t dfifo;
+    uint8_t ctest2;
+    uint8_t ctest3;
+    uint8_t ctest4;
+    uint8_t ctest5;
+    uint8_t ccntl0;
+    uint8_t ccntl1;
+    uint32_t dsp;
+    uint32_t dsps;
+    uint8_t dmode;
+    uint8_t dcntl;
+    uint8_t scntl0;
+    uint8_t scntl1;
+    uint8_t scntl2;
+    uint8_t scntl3;
+    uint8_t sstat0;
+    uint8_t sstat1;
+    uint8_t scid;
+    uint8_t sxfer;
+    uint8_t socl;
+    uint8_t sdid;
+    uint8_t ssid;
+    uint8_t sfbr;
+    uint8_t stest1;
+    uint8_t stest2;
+    uint8_t stest3;
+    uint8_t sidl;
+    uint8_t stime0;
+    uint8_t respid0;
+    uint8_t respid1;
+    uint32_t mmrs;
+    uint32_t mmws;
+    uint32_t sfs;
+    uint32_t drs;
+    uint32_t sbms;
+    uint32_t dbms;
+    uint32_t dnad64;
+    uint32_t pmjad1;
+    uint32_t pmjad2;
+    uint32_t rbc;
+    uint32_t ua;
+    uint32_t ia;
+    uint32_t sbc;
+    uint32_t csbc;
+    uint32_t scratch[18]; /* SCRATCHA-SCRATCHR */
+    uint8_t sbr;
+    uint32_t adder;
+
+    /* Script ram is stored as 32-bit words in host byteorder.  */
+    uint32_t script_ram[2048];
+} LSIState;
+
+#define TYPE_LSI53C810  "lsi53c810"
+#define LSI53C810(obj) \
+    OBJECT_CHECK(LSIState, (obj), TYPE_LSI53C810)
+
+#define TYPE_LSI53C895A "lsi53c895a"
+#define LSI53C895A(obj) \
+    OBJECT_CHECK(LSIState, (obj), TYPE_LSI53C895A)
+
+#endif
-- 
2.11.0

[Qemu-devel] [PATCH 2/3] scsi: move lsi53c895a_create() and lsi53c810_create() callers to pci_create_simple()

[Mark Cave-Ayland]

As part of commits a64aa5785d "hw: Deprecate -drive if=scsi with non-onboard
HBAs" and b891538e81 "hw/ppc/prep: Fix implicit creation of "-drive if=scsi"
devices" the lsi53c895a_create() and lsi53c810_create() functions were added
to wrap pci_create_simple() and scsi_bus_legacy_handle_cmdline().

Unfortunately this prevents us from changing qdev properties on the device
and/or changing the PCI configuration. Now that we have a separate header file
for these devices it is possible to return the LSI objects themselves, which
enables us to both modify the LSI device configuration as required and also
move the invocation of scsi_bus_legacy_handle_cmdline() to the caller as done
elsewhere.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/arm/realview.c    | 5 ++++-
 hw/arm/versatilepb.c | 5 ++++-
 hw/hppa/machine.c    | 5 ++++-
 hw/ppc/prep.c        | 6 +++++-
 4 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/hw/arm/realview.c b/hw/arm/realview.c
index ab8c14fde3..75168afd4d 100644
--- a/hw/arm/realview.c
+++ b/hw/arm/realview.c
@@ -16,6 +16,7 @@
 #include "hw/arm/primecell.h"
 #include "hw/devices.h"
 #include "hw/pci/pci.h"
+#include "hw/scsi/lsi53c895a.h"
 #include "net/net.h"
 #include "sysemu/sysemu.h"
 #include "hw/boards.h"
@@ -63,6 +64,7 @@ static void realview_init(MachineState *machine,
     MemoryRegion *ram_hack = g_new(MemoryRegion, 1);
     DeviceState *dev, *sysctl, *gpio2, *pl041;
     SysBusDevice *busdev;
+    LSIState *lsi;
     qemu_irq pic[64];
     qemu_irq mmc_irq[2];
     PCIBus *pci_bus = NULL;
@@ -257,7 +259,8 @@ static void realview_init(MachineState *machine,
         }
         n = drive_get_max_bus(IF_SCSI);
         while (n >= 0) {
-            lsi53c895a_create(pci_bus);
+            lsi = LSI53C895A(pci_create_simple(pci_bus, -1, TYPE_LSI53C895A));
+            scsi_bus_legacy_handle_cmdline(&lsi->bus);
             n--;
         }
     }
diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
index 8b74857059..dd9962037e 100644
--- a/hw/arm/versatilepb.c
+++ b/hw/arm/versatilepb.c
@@ -19,6 +19,7 @@
 #include "hw/pci/pci.h"
 #include "hw/i2c/i2c.h"
 #include "hw/boards.h"
+#include "hw/scsi/lsi53c895a.h"
 #include "exec/address-spaces.h"
 #include "hw/block/flash.h"
 #include "qemu/error-report.h"
@@ -189,6 +190,7 @@ static void versatile_init(MachineState *machine, int board_id)
     DeviceState *dev, *sysctl;
     SysBusDevice *busdev;
     DeviceState *pl041;
+    LSIState *lsi;
     PCIBus *pci_bus;
     NICInfo *nd;
     I2CBus *i2c;
@@ -278,7 +280,8 @@ static void versatile_init(MachineState *machine, int board_id)
     }
     n = drive_get_max_bus(IF_SCSI);
     while (n >= 0) {
-        lsi53c895a_create(pci_bus);
+        lsi = LSI53C895A(pci_create_simple(pci_bus, -1, TYPE_LSI53C895A));
+        scsi_bus_legacy_handle_cmdline(&lsi->bus);
         n--;
     }
 
diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index cf7c61c6cc..ddd8ee45e4 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -16,6 +16,7 @@
 #include "hw/ide.h"
 #include "hw/timer/i8254.h"
 #include "hw/char/serial.h"
+#include "hw/scsi/lsi53c895a.h"
 #include "hppa_sys.h"
 #include "qemu/units.h"
 #include "qapi/error.h"
@@ -61,6 +62,7 @@ static void machine_hppa_init(MachineState *machine)
     const char *initrd_filename = machine->initrd_filename;
     PCIBus *pci_bus;
     ISABus *isa_bus;
+    LSIState *lsi;
     qemu_irq rtc_irq, serial_irq;
     char *firmware_filename;
     uint64_t firmware_low, firmware_high;
@@ -115,7 +117,8 @@ static void machine_hppa_init(MachineState *machine)
     }
 
     /* SCSI disk setup. */
-    lsi53c895a_create(pci_bus);
+    lsi = LSI53C895A(pci_create_simple(pci_bus, -1, TYPE_LSI53C895A));
+    scsi_bus_legacy_handle_cmdline(&lsi->bus);
 
     /* Network setup.  e1000 is good enough, failing Tulip support.  */
     for (i = 0; i < nb_nics; i++) {
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index 162b27a3b8..fcb2a41846 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -44,6 +44,7 @@
 #include "hw/input/i8042.h"
 #include "hw/isa/pc87312.h"
 #include "hw/net/ne2000-isa.h"
+#include "hw/scsi/lsi53c895a.h"
 #include "sysemu/arch_init.h"
 #include "sysemu/kvm.h"
 #include "sysemu/qtest.h"
@@ -623,6 +624,7 @@ static void ibm_40p_init(MachineState *machine)
     DeviceState *dev;
     SysBusDevice *pcihost, *s;
     Nvram *m48t59 = NULL;
+    LSIState *lsi;
     PCIBus *pci_bus;
     ISABus *isa_bus;
     void *fw_cfg;
@@ -702,7 +704,9 @@ static void ibm_40p_init(MachineState *machine)
         qdev_prop_set_uint32(dev, "equipment", 0xc0);
         qdev_init_nofail(dev);
 
-        lsi53c810_create(pci_bus, PCI_DEVFN(1, 0));
+        lsi = LSI53C810(pci_create_simple(pci_bus, PCI_DEVFN(1, 0),
+                                          TYPE_LSI53C810));
+        scsi_bus_legacy_handle_cmdline(&lsi->bus);
 
         /* XXX: s3-trio at PCI_DEVFN(2, 0) */
         pci_vga_init(pci_bus);
-- 
2.11.0

[Qemu-devel] [PATCH 3/3] scsi: remove unused lsi53c895a_create() and lsi53c810_create() functions

[Mark Cave-Ayland]

Now that these functions are no longer required they can be removed.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/lsi53c895a.c | 14 --------------
 include/hw/pci/pci.h |  3 ---
 2 files changed, 17 deletions(-)

diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index edb0b13e23..2094b1ebc6 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -2162,17 +2162,3 @@ static void lsi53c895a_register_types(void)
 }
 
 type_init(lsi53c895a_register_types)
-
-void lsi53c895a_create(PCIBus *bus)
-{
-    LSIState *s = LSI53C895A(pci_create_simple(bus, -1, "lsi53c895a"));
-
-    scsi_bus_legacy_handle_cmdline(&s->bus);
-}
-
-void lsi53c810_create(PCIBus *bus, int devfn)
-{
-    LSIState *s = LSI53C895A(pci_create_simple(bus, devfn, "lsi53c810"));
-
-    scsi_bus_legacy_handle_cmdline(&s->bus);
-}
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 990d6fcbde..cb9d023f87 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -707,9 +707,6 @@ PCIDevice *pci_create_simple_multifunction(PCIBus *bus, int devfn,
 PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name);
 PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name);
 
-void lsi53c895a_create(PCIBus *bus);
-void lsi53c810_create(PCIBus *bus, int devfn);
-
 qemu_irq pci_allocate_irq(PCIDevice *pci_dev);
 void pci_set_irq(PCIDevice *pci_dev, int level);
 
-- 
2.11.0

Re: [Qemu-devel] [PATCH 1/3] scsi: move lsi53c895a structures and defines into separate lsi53c895a.h file

[Thomas Huth]

On 2018-09-06 07:57, Mark Cave-Ayland wrote:
> There is also one small change to the new header file which is the addition
> of the previously missing LSI53C810 define.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/scsi/lsi53c895a.c         | 116 +-----------------------------------
>  include/hw/scsi/lsi53c895a.h | 137 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 138 insertions(+), 115 deletions(-)
>  create mode 100644 include/hw/scsi/lsi53c895a.h
[...]
> diff --git a/include/hw/scsi/lsi53c895a.h b/include/hw/scsi/lsi53c895a.h
> new file mode 100644
> index 0000000000..d80cb78c69
> --- /dev/null
> +++ b/include/hw/scsi/lsi53c895a.h
> @@ -0,0 +1,137 @@
> +/*
> + * QEMU LSI53C895A SCSI Host Bus Adapter emulation
> + *
> + * Copyright (c) 2006 CodeSourcery.
> + * Written by Paul Brook
> + *
> + * This code is licensed under the LGPL.
> + */
> +
> +#ifndef LSI_H
> +#define LSI_H
> +
> +#include "qemu/osdep.h"

Please don't include osdep.h from a header, that should only be done
from .c files.

 Thanks,
  Thomas

Re: [Qemu-devel] [PATCH 0/3] scsi: remove lsi53c895a_create() and lsi53c810_create() functions

[Thomas Huth]

On 2018-09-06 07:57, Mark Cave-Ayland wrote:
> As part of an upcoming 40p patchset I have a requirement to change the PCI
> configuration of the LSI SCSI. However since commits a64aa5785d "hw: Deprecate -drive
> if=scsi with non-onboard HBAs" and b891538e81 "hw/ppc/prep: Fix implicit creation of
> "-drive if=scsi", the lsi53c8*_create() wrapper functions don't return the device
> state itself.
> 
> Rather than altering these functions to return PCIDevice I simply went ahead and split
> the LSIState structure and QOM types into a new lsi53c895a.h file, as is fairly
> standard QEMU practice.
> 
> Not only does this enable me to change the PCI configuration of the LSI SCSI device
> in an upcoming patchset, but it also allows full access to LSIState if required
> (which is fairly similar to how the code was arranged before a64aa5785d).

I somehow fail to see that something outside of lsi53c895a.c should
really need to access the internals of LSIState. If there is something
that needs to be configured from the outside, it should be done via QOM
properties instead, shouldn't it? So I think I'd rather prefer if you
could do it the other way round and change the lsi*_create() functions
to return a pointer to PCIDevice instead, if possible.

 Thomas

Re: [Qemu-devel] [PATCH 0/3] scsi: remove lsi53c895a_create() and lsi53c810_create() functions

[Peter Maydell]

On 6 September 2018 at 13:02, Thomas Huth <thuth@redhat.com> wrote:
> I somehow fail to see that something outside of lsi53c895a.c should
> really need to access the internals of LSIState. If there is something
> that needs to be configured from the outside, it should be done via QOM
> properties instead, shouldn't it? So I think I'd rather prefer if you
> could do it the other way round and change the lsi*_create() functions
> to return a pointer to PCIDevice instead, if possible.

Nothing typically does, but the "modern" style of having QOM objects which
use other QOM objects do so by embedding the child object's struct into
the struct of the parent requires that the struct definition is available.
The only thing of it that is used is (effectively) its size and alignment
requirements.

(I had an RFC patch some years back that would allow you to mark
up the fields of the struct as private, so the struct could be
put in the header but the compiler would complain if you tried
to actually access any of its fields:
http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg01846.html )

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/3] scsi: move lsi53c895a structures and defines into separate lsi53c895a.h file

[Mark Cave-Ayland]

On 06/09/18 12:52, Thomas Huth wrote:

> On 2018-09-06 07:57, Mark Cave-Ayland wrote:
>> There is also one small change to the new header file which is the addition
>> of the previously missing LSI53C810 define.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  hw/scsi/lsi53c895a.c         | 116 +-----------------------------------
>>  include/hw/scsi/lsi53c895a.h | 137 +++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 138 insertions(+), 115 deletions(-)
>>  create mode 100644 include/hw/scsi/lsi53c895a.h
> [...]
>> diff --git a/include/hw/scsi/lsi53c895a.h b/include/hw/scsi/lsi53c895a.h
>> new file mode 100644
>> index 0000000000..d80cb78c69
>> --- /dev/null
>> +++ b/include/hw/scsi/lsi53c895a.h
>> @@ -0,0 +1,137 @@
>> +/*
>> + * QEMU LSI53C895A SCSI Host Bus Adapter emulation
>> + *
>> + * Copyright (c) 2006 CodeSourcery.
>> + * Written by Paul Brook
>> + *
>> + * This code is licensed under the LGPL.
>> + */
>> +
>> +#ifndef LSI_H
>> +#define LSI_H
>> +
>> +#include "qemu/osdep.h"
> 
> Please don't include osdep.h from a header, that should only be done
> from .c files.

Ah yes, my mistake (obviously it was a cut/paste from the .c file).

I wonder if this is something to teach checkpatch about?


ATB,

Mark.

Re: [Qemu-devel] [PATCH 0/3] scsi: remove lsi53c895a_create() and lsi53c810_create() functions

[Thomas Huth]

On 2018-09-06 16:50, Peter Maydell wrote:
> On 6 September 2018 at 13:02, Thomas Huth <thuth@redhat.com> wrote:
>> I somehow fail to see that something outside of lsi53c895a.c should
>> really need to access the internals of LSIState. If there is something
>> that needs to be configured from the outside, it should be done via QOM
>> properties instead, shouldn't it? So I think I'd rather prefer if you
>> could do it the other way round and change the lsi*_create() functions
>> to return a pointer to PCIDevice instead, if possible.
> 
> Nothing typically does, but the "modern" style of having QOM objects which
> use other QOM objects do so by embedding the child object's struct into
> the struct of the parent requires that the struct definition is available.

But in this case we don't have anything that "inherits" from LSIState,
so shouldn't we rather follow the "information hiding" principle and
keep the state local to the lsi53c895a.c file? If you want to use a
"LSIState *" from another .c file, you can still put an "anonymous"

 struct LSIState;
 typedef struct LSIState LSIState;

in a header somewhere without revealing the implementation.

I'm fine with putting the whole LSIState into a header file if we really
need it, but in this case, I don't see the point.

 Thomas

Re: [Qemu-devel] [PATCH 0/3] scsi: remove lsi53c895a_create() and lsi53c810_create() functions

[Peter Maydell]

On 6 September 2018 at 17:40, Thomas Huth <thuth@redhat.com> wrote:
> On 2018-09-06 16:50, Peter Maydell wrote:
>> Nothing typically does, but the "modern" style of having QOM objects which
>> use other QOM objects do so by embedding the child object's struct into
>> the struct of the parent requires that the struct definition is available.
>
> But in this case we don't have anything that "inherits" from LSIState,
> so shouldn't we rather follow the "information hiding" principle and
> keep the state local to the lsi53c895a.c file? If you want to use a
> "LSIState *" from another .c file, you can still put an "anonymous"
>
>  struct LSIState;
>  typedef struct LSIState LSIState;

This doesn't work for

typedef struct MySoC {
   MyUART uart;
   LSIState scsi;
   ...
} MySoC;

This isn't inheritance, it's just use ("has-a", not "is-a").

> in a header somewhere without revealing the implementation.
>
> I'm fine with putting the whole LSIState into a header file if we really
> need it, but in this case, I don't see the point.

Looking at the rest of the series there doesn't seem to be
any code that wants to do use-by-embedding, so we can
certainly postpone moving the struct into the header file
until we need it.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/3] scsi: remove lsi53c895a_create() and lsi53c810_create() functions

[Mark Cave-Ayland]

On 06/09/18 17:40, Thomas Huth wrote:

> On 2018-09-06 16:50, Peter Maydell wrote:
>> On 6 September 2018 at 13:02, Thomas Huth <thuth@redhat.com> wrote:
>>> I somehow fail to see that something outside of lsi53c895a.c should
>>> really need to access the internals of LSIState. If there is something
>>> that needs to be configured from the outside, it should be done via QOM
>>> properties instead, shouldn't it? So I think I'd rather prefer if you
>>> could do it the other way round and change the lsi*_create() functions
>>> to return a pointer to PCIDevice instead, if possible.
>>
>> Nothing typically does, but the "modern" style of having QOM objects which
>> use other QOM objects do so by embedding the child object's struct into
>> the struct of the parent requires that the struct definition is available.
> 
> But in this case we don't have anything that "inherits" from LSIState,
> so shouldn't we rather follow the "information hiding" principle and
> keep the state local to the lsi53c895a.c file? If you want to use a
> "LSIState *" from another .c file, you can still put an "anonymous"
> 
>  struct LSIState;
>  typedef struct LSIState LSIState;
> 
> in a header somewhere without revealing the implementation.
> 
> I'm fine with putting the whole LSIState into a header file if we really
> need it, but in this case, I don't see the point.

I completely agree with you that struct members used to configure the
device initialisation should only be done via qdev properties, however
having the struct information and the QOM defines available is very,
very handy.

In terms of information hiding we are a long way away from this, and in
my experience having both the compile time and runtime checks on QOM
macros means that just doing a FOO(pci_create_simple...) or strongly
typing object links when wiring up a board catches just about all errors
developers can make.

Amusingly the main reason I need to expose the LSIState at all is to be
able to call scsi_bus_legacy_handle_cmdline() on the SCSI bus object
itself. I guess you could say that this is an argument in favour of the
existing approach, but then you're effectively moving back to the
equivalent of _init() functions for this one particular case which these
days are considered to be bad.

My feeling is that since the pattern of a separate header with struct
and QOM macros (or "modern" style) is used throughout the rest of the
codebase, why should we make an exception for this one particular case?

I see Peter also mentions about marking members as public/private, but
again in my opinion and experience, with the addition of runtime as well
as compile time QOM casts a huge class of problems has now gone away -
and as a result of this, there are now more urgent problems which is
probably why no-one has really picked up Peter's patch in over 4 years.


ATB,

Mark.

Re: [Qemu-devel] [PATCH 0/3] scsi: remove lsi53c895a_create() and lsi53c810_create() functions

[Thomas Huth]

On 2018-09-06 19:15, Mark Cave-Ayland wrote:
> On 06/09/18 17:40, Thomas Huth wrote:
[...]
> Amusingly the main reason I need to expose the LSIState at all is to be
> able to call scsi_bus_legacy_handle_cmdline() on the SCSI bus object
> itself. I guess you could say that this is an argument in favour of the
> existing approach, but then you're effectively moving back to the
> equivalent of _init() functions for this one particular case which these
> days are considered to be bad.

If you mind that the "init" function creates the object, too, then
simply remove the two "lsi53c*_create" functions and provide a function
that looks like this instead:

void lsi53c8xx_handle_legacy_cmdline(DeviceState *lsi_dev)
{
    LSIState *s = LSI53C895A(lsi_dev);

    scsi_bus_legacy_handle_cmdline(&s->bus);
}

Then you can create the device from another .c file and simply call this
new function instead of scsi_bus_legacy_handle_cmdline() in that other
.c file.

In the long run, I think we should maybe also rather get rid of
scsi_bus_legacy_handle_cmdline() and either remove -drive if=scsi or
provide another more generic solution instead (e.g. some code that scans
the qdev tree for SCSI controllers and attaches the devices declared
with -drive if=scsi automatically there).

> My feeling is that since the pattern of a separate header with struct
> and QOM macros (or "modern" style) is used throughout the rest of the
> codebase, why should we make an exception for this one particular case?

I don't mind too much, so if you post your series again, I won't object
again. But still, even if it's a little bit more work for you now, if we
reconsider in a couple of years that information hiding would be a good
idea, it very likely way more work to make the struct private again, so
I'd really prefer if you could keep it private now if possible.

 Thomas

Re: [Qemu-devel] [PATCH 0/3] scsi: remove lsi53c895a_create() and lsi53c810_create() functions

[Mark Cave-Ayland]

On 07/09/18 07:27, Thomas Huth wrote:

> On 2018-09-06 19:15, Mark Cave-Ayland wrote:
>> On 06/09/18 17:40, Thomas Huth wrote:
> [...]
>> Amusingly the main reason I need to expose the LSIState at all is to be
>> able to call scsi_bus_legacy_handle_cmdline() on the SCSI bus object
>> itself. I guess you could say that this is an argument in favour of the
>> existing approach, but then you're effectively moving back to the
>> equivalent of _init() functions for this one particular case which these
>> days are considered to be bad.
> 
> If you mind that the "init" function creates the object, too, then
> simply remove the two "lsi53c*_create" functions and provide a function
> that looks like this instead:
> 
> void lsi53c8xx_handle_legacy_cmdline(DeviceState *lsi_dev)
> {
>     LSIState *s = LSI53C895A(lsi_dev);
> 
>     scsi_bus_legacy_handle_cmdline(&s->bus);
> }
> 
> Then you can create the device from another .c file and simply call this
> new function instead of scsi_bus_legacy_handle_cmdline() in that other
> .c file.
> 
> In the long run, I think we should maybe also rather get rid of
> scsi_bus_legacy_handle_cmdline() and either remove -drive if=scsi or
> provide another more generic solution instead (e.g. some code that scans
> the qdev tree for SCSI controllers and attaches the devices declared
> with -drive if=scsi automatically there).
> 
>> My feeling is that since the pattern of a separate header with struct
>> and QOM macros (or "modern" style) is used throughout the rest of the
>> codebase, why should we make an exception for this one particular case?
> 
> I don't mind too much, so if you post your series again, I won't object
> again. But still, even if it's a little bit more work for you now, if we
> reconsider in a couple of years that information hiding would be a good
> idea, it very likely way more work to make the struct private again, so
> I'd really prefer if you could keep it private now if possible.

Well your modified function above certainly fixes my particular use case
- I'm just in the process of testing a v2 patchset that implements this
approach which I hope to post shortly.

At the end of the day I suspect that regardless of what is finally
committed, it won't really matter that much given the enormity of the
task to change the way that structs and scope are handled throughout the
codebase. So I'm happy either way :)


ATB,

Mark.