All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 1/5] lsi: use ldn_le_p()/stn_le_p()
@ 2019-03-04 18:09 Sven Schnelle
  2019-03-04 18:09 ` [Qemu-devel] [PATCH 2/5] lsi: use enum type for s->waiting Sven Schnelle
                   ` (5 more replies)
  0 siblings, 6 replies; 15+ messages in thread
From: Sven Schnelle @ 2019-03-04 18:09 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Sven Schnelle, Fam Zheng, open list:All patches CC here

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 hw/scsi/lsi53c895a.c | 24 ++++--------------------
 1 file changed, 4 insertions(+), 20 deletions(-)

diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index 25c6926039..6d280f8b77 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -289,8 +289,7 @@ typedef struct {
     uint8_t sbr;
     uint32_t adder;
 
-    /* Script ram is stored as 32-bit words in host byteorder.  */
-    uint32_t script_ram[2048];
+    uint8_t script_ram[8192];
 } LSIState;
 
 #define TYPE_LSI53C810  "lsi53c810"
@@ -2077,29 +2076,14 @@ static void lsi_ram_write(void *opaque, hwaddr addr,
                           uint64_t val, unsigned size)
 {
     LSIState *s = opaque;
-    uint32_t newval;
-    uint32_t mask;
-    int shift;
-
-    newval = s->script_ram[addr >> 2];
-    shift = (addr & 3) * 8;
-    mask = ((uint64_t)1 << (size * 8)) - 1;
-    newval &= ~(mask << shift);
-    newval |= val << shift;
-    s->script_ram[addr >> 2] = newval;
+    stn_le_p(s->script_ram + addr, size, val);
 }
 
 static uint64_t lsi_ram_read(void *opaque, hwaddr addr,
                              unsigned size)
 {
     LSIState *s = opaque;
-    uint32_t val;
-    uint32_t mask;
-
-    val = s->script_ram[addr >> 2];
-    mask = ((uint64_t)1 << (size * 8)) - 1;
-    val >>= (addr & 3) * 8;
-    return val & mask;
+    return ldn_le_p(s->script_ram + addr, size);
 }
 
 static const MemoryRegionOps lsi_ram_ops = {
@@ -2242,7 +2226,7 @@ static const VMStateDescription vmstate_lsi_scsi = {
         VMSTATE_BUFFER_UNSAFE(scratch, LSIState, 0, 18 * sizeof(uint32_t)),
         VMSTATE_UINT8(sbr, LSIState),
 
-        VMSTATE_BUFFER_UNSAFE(script_ram, LSIState, 0, 2048 * sizeof(uint32_t)),
+        VMSTATE_BUFFER_UNSAFE(script_ram, LSIState, 0, 8192),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 2/5] lsi: use enum type for s->waiting
  2019-03-04 18:09 [Qemu-devel] [PATCH 1/5] lsi: use ldn_le_p()/stn_le_p() Sven Schnelle
@ 2019-03-04 18:09 ` Sven Schnelle
  2019-03-04 23:18   ` Philippe Mathieu-Daudé
  2019-03-04 18:09 ` [Qemu-devel] [PATCH 3/5] lsi: use enum type for s->msg_action Sven Schnelle
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 15+ messages in thread
From: Sven Schnelle @ 2019-03-04 18:09 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Sven Schnelle, Fam Zheng, open list:All patches CC here

This makes the code easier to read - no functional change.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 hw/scsi/lsi53c895a.c | 42 +++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index 6d280f8b77..fb9c6db4b2 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -194,6 +194,13 @@ typedef struct lsi_request {
     QTAILQ_ENTRY(lsi_request) next;
 } lsi_request;
 
+enum {
+    LSI_NOWAIT,
+    LSI_WAIT_RESELECT, /* Wait Reselect instruction has been issued */
+    LSI_DMA_SCRIPTS, /* processing DMA from lsi_execute_script */
+    LSI_DMA_IN_PROGRESS, /* DMA operation is in progress */
+};
+
 typedef struct {
     /*< private >*/
     PCIDevice parent_obj;
@@ -212,10 +219,6 @@ typedef struct {
     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;
@@ -322,7 +325,7 @@ static void lsi_soft_reset(LSIState *s)
 
     s->msg_action = 0;
     s->msg_len = 0;
-    s->waiting = 0;
+    s->waiting = LSI_NOWAIT;
     s->dsa = 0;
     s->dnad = 0;
     s->dbc = 0;
@@ -564,10 +567,10 @@ static void lsi_bad_phase(LSIState *s, int out, int new_phase)
 static void lsi_resume_script(LSIState *s)
 {
     if (s->waiting != 2) {
-        s->waiting = 0;
+        s->waiting = LSI_NOWAIT;
         lsi_execute_script(s);
     } else {
-        s->waiting = 0;
+        s->waiting = LSI_NOWAIT;
     }
 }
 
@@ -744,7 +747,7 @@ static int lsi_queue_req(LSIState *s, SCSIRequest *req, uint32_t len)
        Since no interrupt stacking is implemented in the emulation, it
        is also required that there are no pending interrupts waiting
        for service from the device driver. */
-    if (s->waiting == 1 ||
+    if (s->waiting == LSI_WAIT_RESELECT ||
         (lsi_irq_on_rsl(s) && !(s->scntl1 & LSI_SCNTL1_CON) &&
          !(s->istat0 & (LSI_ISTAT0_SIP | LSI_ISTAT0_DIP)))) {
         /* Reselect device.  */
@@ -789,7 +792,7 @@ static void lsi_transfer_data(SCSIRequest *req, uint32_t len)
     int out;
 
     assert(req->hba_private);
-    if (s->waiting == 1 || req->hba_private != s->current ||
+    if (s->waiting == LSI_WAIT_RESELECT || req->hba_private != s->current ||
         (lsi_irq_on_rsl(s) && !(s->scntl1 & LSI_SCNTL1_CON))) {
         if (lsi_queue_req(s, req, len)) {
             return;
@@ -803,7 +806,7 @@ static void lsi_transfer_data(SCSIRequest *req, uint32_t len)
     s->current->dma_len = len;
     s->command_complete = 1;
     if (s->waiting) {
-        if (s->waiting == 1 || s->dbc == 0) {
+        if (s->waiting == LSI_WAIT_RESELECT || s->dbc == 0) {
             lsi_resume_script(s);
         } else {
             lsi_do_dma(s, out);
@@ -1093,7 +1096,7 @@ static void lsi_wait_reselect(LSIState *s)
         lsi_reselect(s, p);
     }
     if (s->current == NULL) {
-        s->waiting = 1;
+        s->waiting = LSI_WAIT_RESELECT;
     }
 }
 
@@ -1202,16 +1205,16 @@ again:
         s->dnad64 = addr_high;
         switch (s->sstat1 & 0x7) {
         case PHASE_DO:
-            s->waiting = 2;
+            s->waiting = LSI_DMA_SCRIPTS;
             lsi_do_dma(s, 1);
             if (s->waiting)
-                s->waiting = 3;
+                s->waiting = LSI_DMA_IN_PROGRESS;
             break;
         case PHASE_DI:
-            s->waiting = 2;
+            s->waiting = LSI_DMA_SCRIPTS;
             lsi_do_dma(s, 0);
             if (s->waiting)
-                s->waiting = 3;
+                s->waiting = LSI_DMA_IN_PROGRESS;
             break;
         case PHASE_CMD:
             lsi_do_command(s);
@@ -1278,6 +1281,7 @@ again:
                 }
                 s->sbcl |= LSI_SBCL_BSY;
                 lsi_set_phase(s, PHASE_MO);
+                s->waiting = LSI_NOWAIT;
                 break;
             case 1: /* Disconnect */
                 trace_lsi_execute_script_io_disconnect();
@@ -1542,7 +1546,7 @@ again:
             }
         }
     }
-    if (insn_processed > 10000 && !s->waiting) {
+    if (insn_processed > 10000 && s->waiting == LSI_NOWAIT) {
         /* Some windows drivers make the device spin waiting for a memory
            location to change.  If we have been executed a lot of code then
            assume this is the case and force an unexpected device disconnect.
@@ -1554,7 +1558,7 @@ again:
         }
         lsi_script_scsi_interrupt(s, LSI_SIST0_UDC, 0);
         lsi_disconnect(s);
-    } else if (s->istat1 & LSI_ISTAT1_SRUN && !s->waiting) {
+    } else if (s->istat1 & LSI_ISTAT1_SRUN && s->waiting == LSI_NOWAIT) {
         if (s->dcntl & LSI_DCNTL_SSM) {
             lsi_script_dma_interrupt(s, LSI_DSTAT_SSI);
         } else {
@@ -1885,9 +1889,9 @@ static void lsi_reg_writeb(LSIState *s, int offset, uint8_t val)
             s->istat0 &= ~LSI_ISTAT0_INTF;
             lsi_update_irq(s);
         }
-        if (s->waiting == 1 && val & LSI_ISTAT0_SIGP) {
+        if (s->waiting == LSI_WAIT_RESELECT && val & LSI_ISTAT0_SIGP) {
             trace_lsi_awoken();
-            s->waiting = 0;
+            s->waiting = LSI_NOWAIT;
             s->dsp = s->dnad;
             lsi_execute_script(s);
         }
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 3/5] lsi: use enum type for s->msg_action
  2019-03-04 18:09 [Qemu-devel] [PATCH 1/5] lsi: use ldn_le_p()/stn_le_p() Sven Schnelle
  2019-03-04 18:09 ` [Qemu-devel] [PATCH 2/5] lsi: use enum type for s->waiting Sven Schnelle
@ 2019-03-04 18:09 ` Sven Schnelle
  2019-03-04 23:14   ` Philippe Mathieu-Daudé
  2019-03-04 18:09 ` [Qemu-devel] [PATCH 4/5] lsi: use SCSI phase names instead of numbers in trace Sven Schnelle
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 15+ messages in thread
From: Sven Schnelle @ 2019-03-04 18:09 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Sven Schnelle, Fam Zheng, open list:All patches CC here

This makes the code easier to read - no functional change.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 hw/scsi/lsi53c895a.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index fb9c6db4b2..d1eb2cf074 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -201,6 +201,13 @@ enum {
     LSI_DMA_IN_PROGRESS, /* DMA operation is in progress */
 };
 
+enum {
+    LSI_MSG_ACTION_COMMAND,
+    LSI_MSG_ACTION_DISCONNECT,
+    LSI_MSG_ACTION_DOUT,
+    LSI_MSG_ACTION_DIN,
+};
+
 typedef struct {
     /*< private >*/
     PCIDevice parent_obj;
@@ -214,8 +221,6 @@ typedef struct {
 
     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];
@@ -323,7 +328,7 @@ static void lsi_soft_reset(LSIState *s)
     trace_lsi_reset();
     s->carry = 0;
 
-    s->msg_action = 0;
+    s->msg_action = LSI_MSG_ACTION_COMMAND;
     s->msg_len = 0;
     s->waiting = LSI_NOWAIT;
     s->dsa = 0;
@@ -686,7 +691,7 @@ static void lsi_reselect(LSIState *s, lsi_request *p)
     trace_lsi_reselect(id);
     s->scntl1 |= LSI_SCNTL1_CON;
     lsi_set_phase(s, PHASE_MI);
-    s->msg_action = p->out ? 2 : 3;
+    s->msg_action = p->out ? LSI_MSG_ACTION_DOUT : LSI_MSG_ACTION_DIN;
     s->current->dma_len = p->pending;
     lsi_add_msg_byte(s, 0x80);
     if (s->current->tag & LSI_TAG_VALID) {
@@ -857,7 +862,7 @@ static void lsi_do_command(LSIState *s)
             lsi_add_msg_byte(s, 4); /* DISCONNECT */
             /* wait data */
             lsi_set_phase(s, PHASE_MI);
-            s->msg_action = 1;
+            s->msg_action = LSI_MSG_ACTION_DISCONNECT;
             lsi_queue_command(s);
         } else {
             /* wait command complete */
@@ -878,7 +883,7 @@ static void lsi_do_status(LSIState *s)
     s->sfbr = status;
     pci_dma_write(PCI_DEVICE(s), s->dnad, &status, 1);
     lsi_set_phase(s, PHASE_MI);
-    s->msg_action = 1;
+    s->msg_action = LSI_MSG_ACTION_DISCONNECT;
     lsi_add_msg_byte(s, 0); /* COMMAND COMPLETE */
 }
 
@@ -901,16 +906,16 @@ static void lsi_do_msgin(LSIState *s)
         /* ??? Check if ATN (not yet implemented) is asserted and maybe
            switch to PHASE_MO.  */
         switch (s->msg_action) {
-        case 0:
+        case LSI_MSG_ACTION_COMMAND:
             lsi_set_phase(s, PHASE_CMD);
             break;
-        case 1:
+        case LSI_MSG_ACTION_DISCONNECT:
             lsi_disconnect(s);
             break;
-        case 2:
+        case LSI_MSG_ACTION_DOUT:
             lsi_set_phase(s, PHASE_DO);
             break;
-        case 3:
+        case LSI_MSG_ACTION_DIN:
             lsi_set_phase(s, PHASE_DI);
             break;
         default:
@@ -1062,7 +1067,7 @@ bad:
     qemu_log_mask(LOG_UNIMP, "Unimplemented message 0x%02x\n", msg);
     lsi_set_phase(s, PHASE_MI);
     lsi_add_msg_byte(s, 7); /* MESSAGE REJECT */
-    s->msg_action = 0;
+    s->msg_action = LSI_MSG_ACTION_COMMAND;
 }
 
 #define LSI_BUF_SIZE 4096
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 4/5] lsi: use SCSI phase names instead of numbers in trace
  2019-03-04 18:09 [Qemu-devel] [PATCH 1/5] lsi: use ldn_le_p()/stn_le_p() Sven Schnelle
  2019-03-04 18:09 ` [Qemu-devel] [PATCH 2/5] lsi: use enum type for s->waiting Sven Schnelle
  2019-03-04 18:09 ` [Qemu-devel] [PATCH 3/5] lsi: use enum type for s->msg_action Sven Schnelle
@ 2019-03-04 18:09 ` Sven Schnelle
  2019-03-04 23:10   ` Philippe Mathieu-Daudé
  2019-03-04 18:09 ` [Qemu-devel] [PATCH 5/5] lsi: return dfifo value Sven Schnelle
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 15+ messages in thread
From: Sven Schnelle @ 2019-03-04 18:09 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Sven Schnelle, Fam Zheng, open list:All patches CC here

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 hw/scsi/lsi53c895a.c | 24 ++++++++++++++++++------
 hw/scsi/trace-events |  4 ++--
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index d1eb2cf074..f635d56e0f 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -184,6 +184,17 @@ static const char *names[] = {
 /* Flag set if this is a tagged command.  */
 #define LSI_TAG_VALID     (1 << 16)
 
+static const char *scsi_phases[] = {
+    "DOUT",
+    "DIN",
+    "CMD",
+    "STATUS",
+    "RSVIN",
+    "RSVOUT",
+    "MSGOUT",
+    "MSGIN"
+};
+
 typedef struct lsi_request {
     SCSIRequest *req;
     uint32_t tag;
@@ -1201,8 +1212,9 @@ again:
             s->ia = s->dsp - 12;
         }
         if ((s->sstat1 & PHASE_MASK) != ((insn >> 24) & 7)) {
-            trace_lsi_execute_script_blockmove_badphase(s->sstat1 & PHASE_MASK,
-                                                        (insn >> 24) & 7);
+            trace_lsi_execute_script_blockmove_badphase(
+                    scsi_phases[s->sstat1 & PHASE_MASK],
+                    scsi_phases[(insn >> 24) & 7]);
             lsi_script_scsi_interrupt(s, LSI_SIST0_MA, 0);
             break;
         }
@@ -1234,8 +1246,8 @@ again:
             lsi_do_msgin(s);
             break;
         default:
-            qemu_log_mask(LOG_UNIMP, "lsi_scsi: Unimplemented phase %d\n",
-                          s->sstat1 & PHASE_MASK);
+            qemu_log_mask(LOG_UNIMP, "lsi_scsi: Unimplemented phase %s\n",
+                          scsi_phases[s->sstat1 & PHASE_MASK]);
         }
         s->dfifo = s->dbc & 0xff;
         s->ctest5 = (s->ctest5 & 0xfc) | ((s->dbc >> 8) & 3);
@@ -1462,9 +1474,9 @@ again:
             }
             if (cond == jmp && (insn & (1 << 17))) {
                 trace_lsi_execute_script_tc_compp(
-                        (s->sstat1 & PHASE_MASK),
+                        scsi_phases[(s->sstat1 & PHASE_MASK)],
                         jmp ? '=' : '!',
-                        ((insn >> 24) & 7));
+                        scsi_phases[((insn >> 24) & 7)]);
                 cond = (s->sstat1 & PHASE_MASK) == ((insn >> 24) & 7);
             }
             if (cond == jmp && (insn & (1 << 18))) {
diff --git a/hw/scsi/trace-events b/hw/scsi/trace-events
index 29aaa752d1..34feb6f42d 100644
--- a/hw/scsi/trace-events
+++ b/hw/scsi/trace-events
@@ -268,7 +268,7 @@ lsi_memcpy(uint32_t dest, uint32_t src, int count) "memcpy dest 0x%"PRIx32" src
 lsi_wait_reselect(void) "Wait Reselect"
 lsi_execute_script(uint32_t dsp, uint32_t insn, uint32_t addr) "SCRIPTS dsp=0x%"PRIx32" opcode 0x%"PRIx32" arg 0x%"PRIx32
 lsi_execute_script_blockmove_delayed(void) "Delayed select timeout"
-lsi_execute_script_blockmove_badphase(uint8_t phase, uint8_t expected) "Wrong phase got %d expected %d"
+lsi_execute_script_blockmove_badphase(const char *phase, const char *expected) "Wrong phase got %s expected %s"
 lsi_execute_script_io_alreadyreselected(void) "Already reselected, jumping to alternative address"
 lsi_execute_script_io_selected(uint8_t id, const char *atn) "Selected target %d%s"
 lsi_execute_script_io_disconnect(void) "Wait Disconnect"
@@ -278,7 +278,7 @@ lsi_execute_script_io_opcode(const char *opcode, int reg, const char *opname, ui
 lsi_execute_script_tc_nop(void) "NOP"
 lsi_execute_script_tc_delayedselect_timeout(void) "Delayed select timeout"
 lsi_execute_script_tc_compc(int result) "Compare carry %d"
-lsi_execute_script_tc_compp(uint8_t phase, int op, uint8_t insn_phase) "Compare phase %d %c= %d"
+lsi_execute_script_tc_compp(const char *phase, int op, const char *insn_phase) "Compare phase %s %c= %s"
 lsi_execute_script_tc_compd(uint32_t sfbr, uint8_t mask, int op, int result) "Compare data 0x%"PRIx32" & 0x%x %c= 0x%x"
 lsi_execute_script_tc_jump(uint32_t addr) "Jump to 0x%"PRIx32
 lsi_execute_script_tc_call(uint32_t addr) "Call 0x%"PRIx32
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 5/5] lsi: return dfifo value
  2019-03-04 18:09 [Qemu-devel] [PATCH 1/5] lsi: use ldn_le_p()/stn_le_p() Sven Schnelle
                   ` (2 preceding siblings ...)
  2019-03-04 18:09 ` [Qemu-devel] [PATCH 4/5] lsi: use SCSI phase names instead of numbers in trace Sven Schnelle
@ 2019-03-04 18:09 ` Sven Schnelle
  2019-03-04 18:40 ` [Qemu-devel] [PATCH 1/5] lsi: use ldn_le_p()/stn_le_p() Eric Blake
  2019-03-04 23:21 ` Philippe Mathieu-Daudé
  5 siblings, 0 replies; 15+ messages in thread
From: Sven Schnelle @ 2019-03-04 18:09 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Sven Schnelle, Fam Zheng, open list:All patches CC here

Code was assigning DFIFO, but didn't return the value to users.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 hw/scsi/lsi53c895a.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index f635d56e0f..a0e0c57b9b 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -1683,7 +1683,7 @@ static uint8_t lsi_reg_readb(LSIState *s, int offset)
         break;
     CASE_GET_REG32(temp, 0x1c)
     case 0x20: /* DFIFO */
-        ret = 0;
+        ret = s->dfifo;
         break;
     case 0x21: /* CTEST4 */
         ret = s->ctest4;
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 1/5] lsi: use ldn_le_p()/stn_le_p()
  2019-03-04 18:09 [Qemu-devel] [PATCH 1/5] lsi: use ldn_le_p()/stn_le_p() Sven Schnelle
                   ` (3 preceding siblings ...)
  2019-03-04 18:09 ` [Qemu-devel] [PATCH 5/5] lsi: return dfifo value Sven Schnelle
@ 2019-03-04 18:40 ` Eric Blake
  2019-03-04 20:38   ` Sven Schnelle
  2019-03-04 23:21 ` Philippe Mathieu-Daudé
  5 siblings, 1 reply; 15+ messages in thread
From: Eric Blake @ 2019-03-04 18:40 UTC (permalink / raw)
  To: Sven Schnelle, Paolo Bonzini; +Cc: Fam Zheng, open list:All patches CC here

On 3/4/19 12:09 PM, Sven Schnelle wrote:
> Signed-off-by: Sven Schnelle <svens@stackframe.org>

The commit header says "what" (good), but the commit body says nothing 
at all (generally, it should say "why"). If you give your reviewers a 
reason why it is good to use the new functions, it makes it easier to 
apply your patch.

Also, don't forget to send a 0/5 cover letter when sending a patch 
series; you can have git do this for you with 'git config 
format.coverletter auto'. https://wiki.qemu.org/Contribute/SubmitAPatch 
has more hints for improved patch handling.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 1/5] lsi: use ldn_le_p()/stn_le_p()
  2019-03-04 18:40 ` [Qemu-devel] [PATCH 1/5] lsi: use ldn_le_p()/stn_le_p() Eric Blake
@ 2019-03-04 20:38   ` Sven Schnelle
  2019-03-04 21:15     ` Eric Blake
  0 siblings, 1 reply; 15+ messages in thread
From: Sven Schnelle @ 2019-03-04 20:38 UTC (permalink / raw)
  To: Eric Blake; +Cc: Paolo Bonzini, Fam Zheng, open list:All patches CC here

Hi Eric,

On Mon, Mar 04, 2019 at 12:40:50PM -0600, Eric Blake wrote:
> On 3/4/19 12:09 PM, Sven Schnelle wrote:
> > Signed-off-by: Sven Schnelle <svens@stackframe.org>
> 
> The commit header says "what" (good), but the commit body says nothing at
> all (generally, it should say "why"). If you give your reviewers a reason
> why it is good to use the new functions, it makes it easier to apply your
> patch.
> 
> Also, don't forget to send a 0/5 cover letter when sending a patch series;
> you can have git do this for you with 'git config format.coverletter auto'.
> https://wiki.qemu.org/Contribute/SubmitAPatch has more hints for improved
> patch handling.

Thanks, will keep this in mind, sorry. Should i resend the series?

Regards
Sven

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 1/5] lsi: use ldn_le_p()/stn_le_p()
  2019-03-04 20:38   ` Sven Schnelle
@ 2019-03-04 21:15     ` Eric Blake
  0 siblings, 0 replies; 15+ messages in thread
From: Eric Blake @ 2019-03-04 21:15 UTC (permalink / raw)
  To: Sven Schnelle; +Cc: Paolo Bonzini, Fam Zheng, open list:All patches CC here

On 3/4/19 2:38 PM, Sven Schnelle wrote:
> Hi Eric,
> 
> On Mon, Mar 04, 2019 at 12:40:50PM -0600, Eric Blake wrote:
>> On 3/4/19 12:09 PM, Sven Schnelle wrote:
>>> Signed-off-by: Sven Schnelle <svens@stackframe.org>
>>
>> The commit header says "what" (good), but the commit body says nothing at
>> all (generally, it should say "why"). If you give your reviewers a reason
>> why it is good to use the new functions, it makes it easier to apply your
>> patch.
>>
>> Also, don't forget to send a 0/5 cover letter when sending a patch series;
>> you can have git do this for you with 'git config format.coverletter auto'.
>> https://wiki.qemu.org/Contribute/SubmitAPatch has more hints for improved
>> patch handling.
> 
> Thanks, will keep this in mind, sorry. Should i resend the series?

Up to you, but if it were me, I'd probably wait a day or two for any 
other review comments to address those at the same time, or for a 
definitive answer from the particular maintainer that will be including 
your patches.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] lsi: use SCSI phase names instead of numbers in trace
  2019-03-04 18:09 ` [Qemu-devel] [PATCH 4/5] lsi: use SCSI phase names instead of numbers in trace Sven Schnelle
@ 2019-03-04 23:10   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-03-04 23:10 UTC (permalink / raw)
  To: Sven Schnelle, Paolo Bonzini; +Cc: Fam Zheng, open list:All patches CC here

Hi Sven,

On 3/4/19 7:09 PM, Sven Schnelle wrote:
> Signed-off-by: Sven Schnelle <svens@stackframe.org>
> ---
>  hw/scsi/lsi53c895a.c | 24 ++++++++++++++++++------
>  hw/scsi/trace-events |  4 ++--
>  2 files changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> index d1eb2cf074..f635d56e0f 100644
> --- a/hw/scsi/lsi53c895a.c
> +++ b/hw/scsi/lsi53c895a.c
> @@ -184,6 +184,17 @@ static const char *names[] = {
>  /* Flag set if this is a tagged command.  */
>  #define LSI_TAG_VALID     (1 << 16)
>  
> +static const char *scsi_phases[] = {
> +    "DOUT",
> +    "DIN",
> +    "CMD",
> +    "STATUS",
> +    "RSVIN",
> +    "RSVOUT",
> +    "MSGOUT",
> +    "MSGIN"
> +};

Some developper might use your code but forget to use value &
PHASE_MASK, trying on small values, then another user uses bigger values
and this array will return unexpected result.

static const char *scsi_phase_name(int phase)
{
  static const char *scsi_phases[8] = {
    [PHASE_DO] = "DOUT",
    [PHASE_DI] = "DIN",
    [PHASE_CMD] = "CMD",
    [PHASE_ST] = "STATUS",
    [PHASE_MO] = "MSGOUT",
    [PHASE_MI] = "MSGIN",
  };
  phase &= PHASE_MASK;
  return scsi_phases[phase] ? scsi_phases[phase] : "RESERVED";
}

You can also keep your RSVIN/RSVOUT:

static const char *scsi_phase_name(int phase)
{
  static const char *scsi_phases[8] = {
    "DOUT",
    "DIN",
    "CMD",
    "STATUS",
    "RSVIN",
    "RSVOUT",
    "MSGOUT",
    "MSGIN"
  };
  return scsi_phases[phase & PHASE_MASK];
}

In both cases, calls are easier to read:

            trace_lsi_execute_script_blockmove_badphase(
                    scsi_phase_name(s->sstat1),
                    scsi_phase_name(insn >> 24));

> +
>  typedef struct lsi_request {
>      SCSIRequest *req;
>      uint32_t tag;
> @@ -1201,8 +1212,9 @@ again:
>              s->ia = s->dsp - 12;
>          }
>          if ((s->sstat1 & PHASE_MASK) != ((insn >> 24) & 7)) {
> -            trace_lsi_execute_script_blockmove_badphase(s->sstat1 & PHASE_MASK,
> -                                                        (insn >> 24) & 7);
> +            trace_lsi_execute_script_blockmove_badphase(
> +                    scsi_phases[s->sstat1 & PHASE_MASK],
> +                    scsi_phases[(insn >> 24) & 7]);
>              lsi_script_scsi_interrupt(s, LSI_SIST0_MA, 0);
>              break;
>          }
> @@ -1234,8 +1246,8 @@ again:
>              lsi_do_msgin(s);
>              break;
>          default:
> -            qemu_log_mask(LOG_UNIMP, "lsi_scsi: Unimplemented phase %d\n",
> -                          s->sstat1 & PHASE_MASK);
> +            qemu_log_mask(LOG_UNIMP, "lsi_scsi: Unimplemented phase %s\n",
> +                          scsi_phases[s->sstat1 & PHASE_MASK]);
>          }
>          s->dfifo = s->dbc & 0xff;
>          s->ctest5 = (s->ctest5 & 0xfc) | ((s->dbc >> 8) & 3);
> @@ -1462,9 +1474,9 @@ again:
>              }
>              if (cond == jmp && (insn & (1 << 17))) {
>                  trace_lsi_execute_script_tc_compp(
> -                        (s->sstat1 & PHASE_MASK),
> +                        scsi_phases[(s->sstat1 & PHASE_MASK)],

No need for parenthesis here,

>                          jmp ? '=' : '!',
> -                        ((insn >> 24) & 7));
> +                        scsi_phases[((insn >> 24) & 7)]);

Ditto.

>                  cond = (s->sstat1 & PHASE_MASK) == ((insn >> 24) & 7);
>              }
>              if (cond == jmp && (insn & (1 << 18))) {
> diff --git a/hw/scsi/trace-events b/hw/scsi/trace-events
> index 29aaa752d1..34feb6f42d 100644
> --- a/hw/scsi/trace-events
> +++ b/hw/scsi/trace-events
> @@ -268,7 +268,7 @@ lsi_memcpy(uint32_t dest, uint32_t src, int count) "memcpy dest 0x%"PRIx32" src
>  lsi_wait_reselect(void) "Wait Reselect"
>  lsi_execute_script(uint32_t dsp, uint32_t insn, uint32_t addr) "SCRIPTS dsp=0x%"PRIx32" opcode 0x%"PRIx32" arg 0x%"PRIx32
>  lsi_execute_script_blockmove_delayed(void) "Delayed select timeout"
> -lsi_execute_script_blockmove_badphase(uint8_t phase, uint8_t expected) "Wrong phase got %d expected %d"
> +lsi_execute_script_blockmove_badphase(const char *phase, const char *expected) "Wrong phase got %s expected %s"
>  lsi_execute_script_io_alreadyreselected(void) "Already reselected, jumping to alternative address"
>  lsi_execute_script_io_selected(uint8_t id, const char *atn) "Selected target %d%s"
>  lsi_execute_script_io_disconnect(void) "Wait Disconnect"
> @@ -278,7 +278,7 @@ lsi_execute_script_io_opcode(const char *opcode, int reg, const char *opname, ui
>  lsi_execute_script_tc_nop(void) "NOP"
>  lsi_execute_script_tc_delayedselect_timeout(void) "Delayed select timeout"
>  lsi_execute_script_tc_compc(int result) "Compare carry %d"
> -lsi_execute_script_tc_compp(uint8_t phase, int op, uint8_t insn_phase) "Compare phase %d %c= %d"
> +lsi_execute_script_tc_compp(const char *phase, int op, const char *insn_phase) "Compare phase %s %c= %s"

Since you change this line, can you fix 'int op' -> 'char op'?
Or as a previous cleanup patch... There are other '%c' misused.

>  lsi_execute_script_tc_compd(uint32_t sfbr, uint8_t mask, int op, int result) "Compare data 0x%"PRIx32" & 0x%x %c= 0x%x"
>  lsi_execute_script_tc_jump(uint32_t addr) "Jump to 0x%"PRIx32
>  lsi_execute_script_tc_call(uint32_t addr) "Call 0x%"PRIx32
> 

Regards,

Phil.

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] lsi: use enum type for s->msg_action
  2019-03-04 18:09 ` [Qemu-devel] [PATCH 3/5] lsi: use enum type for s->msg_action Sven Schnelle
@ 2019-03-04 23:14   ` Philippe Mathieu-Daudé
  2019-03-04 23:22     ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-03-04 23:14 UTC (permalink / raw)
  To: Sven Schnelle, Paolo Bonzini; +Cc: Fam Zheng, open list:All patches CC here

On 3/4/19 7:09 PM, Sven Schnelle wrote:
> This makes the code easier to read - no functional change.
> 
> Signed-off-by: Sven Schnelle <svens@stackframe.org>
> ---
>  hw/scsi/lsi53c895a.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> index fb9c6db4b2..d1eb2cf074 100644
> --- a/hw/scsi/lsi53c895a.c
> +++ b/hw/scsi/lsi53c895a.c
> @@ -201,6 +201,13 @@ enum {
>      LSI_DMA_IN_PROGRESS, /* DMA operation is in progress */
>  };
>  
> +enum {
Since someone unfamiliar with the device could add:

       LSI_MSG_ACTION_DEBUG,

> +    LSI_MSG_ACTION_COMMAND,
> +    LSI_MSG_ACTION_DISCONNECT,
> +    LSI_MSG_ACTION_DOUT,
> +    LSI_MSG_ACTION_DIN,

... it is safer to assign values when enum are not expected to change:

      LSI_MSG_ACTION_COMMAND = 0,
      LSI_MSG_ACTION_DISCONNECT = 1,
      LSI_MSG_ACTION_DOUT = 2,
      LSI_MSG_ACTION_DIN = 3,

With that change:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +};
> +
>  typedef struct {
>      /*< private >*/
>      PCIDevice parent_obj;
> @@ -214,8 +221,6 @@ typedef struct {
>  
>      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];
> @@ -323,7 +328,7 @@ static void lsi_soft_reset(LSIState *s)
>      trace_lsi_reset();
>      s->carry = 0;
>  
> -    s->msg_action = 0;
> +    s->msg_action = LSI_MSG_ACTION_COMMAND;
>      s->msg_len = 0;
>      s->waiting = LSI_NOWAIT;
>      s->dsa = 0;
> @@ -686,7 +691,7 @@ static void lsi_reselect(LSIState *s, lsi_request *p)
>      trace_lsi_reselect(id);
>      s->scntl1 |= LSI_SCNTL1_CON;
>      lsi_set_phase(s, PHASE_MI);
> -    s->msg_action = p->out ? 2 : 3;
> +    s->msg_action = p->out ? LSI_MSG_ACTION_DOUT : LSI_MSG_ACTION_DIN;
>      s->current->dma_len = p->pending;
>      lsi_add_msg_byte(s, 0x80);
>      if (s->current->tag & LSI_TAG_VALID) {
> @@ -857,7 +862,7 @@ static void lsi_do_command(LSIState *s)
>              lsi_add_msg_byte(s, 4); /* DISCONNECT */
>              /* wait data */
>              lsi_set_phase(s, PHASE_MI);
> -            s->msg_action = 1;
> +            s->msg_action = LSI_MSG_ACTION_DISCONNECT;
>              lsi_queue_command(s);
>          } else {
>              /* wait command complete */
> @@ -878,7 +883,7 @@ static void lsi_do_status(LSIState *s)
>      s->sfbr = status;
>      pci_dma_write(PCI_DEVICE(s), s->dnad, &status, 1);
>      lsi_set_phase(s, PHASE_MI);
> -    s->msg_action = 1;
> +    s->msg_action = LSI_MSG_ACTION_DISCONNECT;
>      lsi_add_msg_byte(s, 0); /* COMMAND COMPLETE */
>  }
>  
> @@ -901,16 +906,16 @@ static void lsi_do_msgin(LSIState *s)
>          /* ??? Check if ATN (not yet implemented) is asserted and maybe
>             switch to PHASE_MO.  */
>          switch (s->msg_action) {
> -        case 0:
> +        case LSI_MSG_ACTION_COMMAND:
>              lsi_set_phase(s, PHASE_CMD);
>              break;
> -        case 1:
> +        case LSI_MSG_ACTION_DISCONNECT:
>              lsi_disconnect(s);
>              break;
> -        case 2:
> +        case LSI_MSG_ACTION_DOUT:
>              lsi_set_phase(s, PHASE_DO);
>              break;
> -        case 3:
> +        case LSI_MSG_ACTION_DIN:
>              lsi_set_phase(s, PHASE_DI);
>              break;
>          default:
> @@ -1062,7 +1067,7 @@ bad:
>      qemu_log_mask(LOG_UNIMP, "Unimplemented message 0x%02x\n", msg);
>      lsi_set_phase(s, PHASE_MI);
>      lsi_add_msg_byte(s, 7); /* MESSAGE REJECT */
> -    s->msg_action = 0;
> +    s->msg_action = LSI_MSG_ACTION_COMMAND;
>  }
>  
>  #define LSI_BUF_SIZE 4096
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 2/5] lsi: use enum type for s->waiting
  2019-03-04 18:09 ` [Qemu-devel] [PATCH 2/5] lsi: use enum type for s->waiting Sven Schnelle
@ 2019-03-04 23:18   ` Philippe Mathieu-Daudé
  2019-03-05  7:17     ` Sven Schnelle
  0 siblings, 1 reply; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-03-04 23:18 UTC (permalink / raw)
  To: Sven Schnelle, Paolo Bonzini; +Cc: Fam Zheng, open list:All patches CC here

On 3/4/19 7:09 PM, Sven Schnelle wrote:
> This makes the code easier to read - no functional change.
> 
> Signed-off-by: Sven Schnelle <svens@stackframe.org>
> ---
>  hw/scsi/lsi53c895a.c | 42 +++++++++++++++++++++++-------------------
>  1 file changed, 23 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> index 6d280f8b77..fb9c6db4b2 100644
> --- a/hw/scsi/lsi53c895a.c
> +++ b/hw/scsi/lsi53c895a.c
> @@ -194,6 +194,13 @@ typedef struct lsi_request {
>      QTAILQ_ENTRY(lsi_request) next;
>  } lsi_request;
>  
> +enum {
> +    LSI_NOWAIT,

You forgot the comment for NOWAIT.

> +    LSI_WAIT_RESELECT, /* Wait Reselect instruction has been issued */
> +    LSI_DMA_SCRIPTS, /* processing DMA from lsi_execute_script */
> +    LSI_DMA_IN_PROGRESS, /* DMA operation is in progress */
> +};
> +
>  typedef struct {
>      /*< private >*/
>      PCIDevice parent_obj;
> @@ -212,10 +219,6 @@ typedef struct {
>      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;

When a field is not used by migration, you can declare it as enum:

       enum {
           LSI_NOWAIT = 0, /* SCRIPTS are running or stopped */
           LSI_WAIT_RESELECT = 1, /* Wait Reselect instruction has been
issued */
           LSI_DMA_SCRIPTS = 2, /* processing DMA from lsi_execute_script */
           LSI_DMA_IN_PROGRESS = 3, /* DMA operation is in progress */
       } waiting;

This gives hints to the compiler about values to check.

>      SCSIBus bus;
>      int current_lun;
> @@ -322,7 +325,7 @@ static void lsi_soft_reset(LSIState *s)
>  
>      s->msg_action = 0;
>      s->msg_len = 0;
> -    s->waiting = 0;
> +    s->waiting = LSI_NOWAIT;
>      s->dsa = 0;
>      s->dnad = 0;
>      s->dbc = 0;
> @@ -564,10 +567,10 @@ static void lsi_bad_phase(LSIState *s, int out, int new_phase)
>  static void lsi_resume_script(LSIState *s)
>  {
>      if (s->waiting != 2) {
> -        s->waiting = 0;
> +        s->waiting = LSI_NOWAIT;
>          lsi_execute_script(s);
>      } else {
> -        s->waiting = 0;
> +        s->waiting = LSI_NOWAIT;
>      }
>  }
>  
> @@ -744,7 +747,7 @@ static int lsi_queue_req(LSIState *s, SCSIRequest *req, uint32_t len)
>         Since no interrupt stacking is implemented in the emulation, it
>         is also required that there are no pending interrupts waiting
>         for service from the device driver. */
> -    if (s->waiting == 1 ||
> +    if (s->waiting == LSI_WAIT_RESELECT ||
>          (lsi_irq_on_rsl(s) && !(s->scntl1 & LSI_SCNTL1_CON) &&
>           !(s->istat0 & (LSI_ISTAT0_SIP | LSI_ISTAT0_DIP)))) {
>          /* Reselect device.  */
> @@ -789,7 +792,7 @@ static void lsi_transfer_data(SCSIRequest *req, uint32_t len)
>      int out;
>  
>      assert(req->hba_private);
> -    if (s->waiting == 1 || req->hba_private != s->current ||
> +    if (s->waiting == LSI_WAIT_RESELECT || req->hba_private != s->current ||
>          (lsi_irq_on_rsl(s) && !(s->scntl1 & LSI_SCNTL1_CON))) {
>          if (lsi_queue_req(s, req, len)) {
>              return;
> @@ -803,7 +806,7 @@ static void lsi_transfer_data(SCSIRequest *req, uint32_t len)
>      s->current->dma_len = len;
>      s->command_complete = 1;
>      if (s->waiting) {
> -        if (s->waiting == 1 || s->dbc == 0) {
> +        if (s->waiting == LSI_WAIT_RESELECT || s->dbc == 0) {
>              lsi_resume_script(s);
>          } else {
>              lsi_do_dma(s, out);
> @@ -1093,7 +1096,7 @@ static void lsi_wait_reselect(LSIState *s)
>          lsi_reselect(s, p);
>      }
>      if (s->current == NULL) {
> -        s->waiting = 1;
> +        s->waiting = LSI_WAIT_RESELECT;
>      }
>  }
>  
> @@ -1202,16 +1205,16 @@ again:
>          s->dnad64 = addr_high;
>          switch (s->sstat1 & 0x7) {
>          case PHASE_DO:
> -            s->waiting = 2;
> +            s->waiting = LSI_DMA_SCRIPTS;
>              lsi_do_dma(s, 1);
>              if (s->waiting)
> -                s->waiting = 3;
> +                s->waiting = LSI_DMA_IN_PROGRESS;
>              break;
>          case PHASE_DI:
> -            s->waiting = 2;
> +            s->waiting = LSI_DMA_SCRIPTS;
>              lsi_do_dma(s, 0);
>              if (s->waiting)
> -                s->waiting = 3;
> +                s->waiting = LSI_DMA_IN_PROGRESS;
>              break;
>          case PHASE_CMD:
>              lsi_do_command(s);
> @@ -1278,6 +1281,7 @@ again:
>                  }
>                  s->sbcl |= LSI_SBCL_BSY;
>                  lsi_set_phase(s, PHASE_MO);
> +                s->waiting = LSI_NOWAIT;
>                  break;
>              case 1: /* Disconnect */
>                  trace_lsi_execute_script_io_disconnect();
> @@ -1542,7 +1546,7 @@ again:
>              }
>          }
>      }
> -    if (insn_processed > 10000 && !s->waiting) {
> +    if (insn_processed > 10000 && s->waiting == LSI_NOWAIT) {
>          /* Some windows drivers make the device spin waiting for a memory
>             location to change.  If we have been executed a lot of code then
>             assume this is the case and force an unexpected device disconnect.
> @@ -1554,7 +1558,7 @@ again:
>          }
>          lsi_script_scsi_interrupt(s, LSI_SIST0_UDC, 0);
>          lsi_disconnect(s);
> -    } else if (s->istat1 & LSI_ISTAT1_SRUN && !s->waiting) {
> +    } else if (s->istat1 & LSI_ISTAT1_SRUN && s->waiting == LSI_NOWAIT) {
>          if (s->dcntl & LSI_DCNTL_SSM) {
>              lsi_script_dma_interrupt(s, LSI_DSTAT_SSI);
>          } else {
> @@ -1885,9 +1889,9 @@ static void lsi_reg_writeb(LSIState *s, int offset, uint8_t val)
>              s->istat0 &= ~LSI_ISTAT0_INTF;
>              lsi_update_irq(s);
>          }
> -        if (s->waiting == 1 && val & LSI_ISTAT0_SIGP) {
> +        if (s->waiting == LSI_WAIT_RESELECT && val & LSI_ISTAT0_SIGP) {
>              trace_lsi_awoken();
> -            s->waiting = 0;
> +            s->waiting = LSI_NOWAIT;
>              s->dsp = s->dnad;
>              lsi_execute_script(s);
>          }
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 1/5] lsi: use ldn_le_p()/stn_le_p()
  2019-03-04 18:09 [Qemu-devel] [PATCH 1/5] lsi: use ldn_le_p()/stn_le_p() Sven Schnelle
                   ` (4 preceding siblings ...)
  2019-03-04 18:40 ` [Qemu-devel] [PATCH 1/5] lsi: use ldn_le_p()/stn_le_p() Eric Blake
@ 2019-03-04 23:21 ` Philippe Mathieu-Daudé
  5 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-03-04 23:21 UTC (permalink / raw)
  To: Sven Schnelle, Paolo Bonzini; +Cc: Fam Zheng, open list:All patches CC here

On 3/4/19 7:09 PM, Sven Schnelle wrote:
> Signed-off-by: Sven Schnelle <svens@stackframe.org>
> ---
>  hw/scsi/lsi53c895a.c | 24 ++++--------------------
>  1 file changed, 4 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> index 25c6926039..6d280f8b77 100644
> --- a/hw/scsi/lsi53c895a.c
> +++ b/hw/scsi/lsi53c895a.c
> @@ -289,8 +289,7 @@ typedef struct {
>      uint8_t sbr;
>      uint32_t adder;
>  
> -    /* Script ram is stored as 32-bit words in host byteorder.  */
> -    uint32_t script_ram[2048];
> +    uint8_t script_ram[8192];

I'm tempted to use here:

       uint8_t script_ram[2048 * sizeof(uint32_t)];

>  } LSIState;
>  
>  #define TYPE_LSI53C810  "lsi53c810"
> @@ -2077,29 +2076,14 @@ static void lsi_ram_write(void *opaque, hwaddr addr,
>                            uint64_t val, unsigned size)
>  {
>      LSIState *s = opaque;
> -    uint32_t newval;
> -    uint32_t mask;
> -    int shift;
> -
> -    newval = s->script_ram[addr >> 2];
> -    shift = (addr & 3) * 8;
> -    mask = ((uint64_t)1 << (size * 8)) - 1;
> -    newval &= ~(mask << shift);
> -    newval |= val << shift;
> -    s->script_ram[addr >> 2] = newval;
> +    stn_le_p(s->script_ram + addr, size, val);
>  }
>  
>  static uint64_t lsi_ram_read(void *opaque, hwaddr addr,
>                               unsigned size)
>  {
>      LSIState *s = opaque;
> -    uint32_t val;
> -    uint32_t mask;
> -

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> -    val = s->script_ram[addr >> 2];
> -    mask = ((uint64_t)1 << (size * 8)) - 1;
> -    val >>= (addr & 3) * 8;
> -    return val & mask;
> +    return ldn_le_p(s->script_ram + addr, size);
>  }
>  
>  static const MemoryRegionOps lsi_ram_ops = {
> @@ -2242,7 +2226,7 @@ static const VMStateDescription vmstate_lsi_scsi = {
>          VMSTATE_BUFFER_UNSAFE(scratch, LSIState, 0, 18 * sizeof(uint32_t)),
>          VMSTATE_UINT8(sbr, LSIState),
>  
> -        VMSTATE_BUFFER_UNSAFE(script_ram, LSIState, 0, 2048 * sizeof(uint32_t)),
> +        VMSTATE_BUFFER_UNSAFE(script_ram, LSIState, 0, 8192),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] lsi: use enum type for s->msg_action
  2019-03-04 23:14   ` Philippe Mathieu-Daudé
@ 2019-03-04 23:22     ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-03-04 23:22 UTC (permalink / raw)
  To: Sven Schnelle, Paolo Bonzini; +Cc: Fam Zheng, open list:All patches CC here

On 3/5/19 12:14 AM, Philippe Mathieu-Daudé wrote:
> On 3/4/19 7:09 PM, Sven Schnelle wrote:
>> This makes the code easier to read - no functional change.
>>
>> Signed-off-by: Sven Schnelle <svens@stackframe.org>
>> ---
>>  hw/scsi/lsi53c895a.c | 27 ++++++++++++++++-----------
>>  1 file changed, 16 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
>> index fb9c6db4b2..d1eb2cf074 100644
>> --- a/hw/scsi/lsi53c895a.c
>> +++ b/hw/scsi/lsi53c895a.c
>> @@ -201,6 +201,13 @@ enum {
>>      LSI_DMA_IN_PROGRESS, /* DMA operation is in progress */
>>  };
>>  
>> +enum {
> Since someone unfamiliar with the device could add:
> 
>        LSI_MSG_ACTION_DEBUG,
> 
>> +    LSI_MSG_ACTION_COMMAND,
>> +    LSI_MSG_ACTION_DISCONNECT,
>> +    LSI_MSG_ACTION_DOUT,
>> +    LSI_MSG_ACTION_DIN,
> 
> ... it is safer to assign values when enum are not expected to change:
> 
>       LSI_MSG_ACTION_COMMAND = 0,
>       LSI_MSG_ACTION_DISCONNECT = 1,
>       LSI_MSG_ACTION_DOUT = 2,
>       LSI_MSG_ACTION_DIN = 3,
> 
> With that change:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
>> +};
>> +
>>  typedef struct {
>>      /*< private >*/
>>      PCIDevice parent_obj;
>> @@ -214,8 +221,6 @@ typedef struct {
>>  
>>      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;

I'd also declare 'enum msg_action' here (same comments as patch 2 of
this series).
Regardless the Reviewed-by tag stands.

>>      int msg_len;
>>      uint8_t msg[LSI_MAX_MSGIN_LEN];
>> @@ -323,7 +328,7 @@ static void lsi_soft_reset(LSIState *s)
>>      trace_lsi_reset();
>>      s->carry = 0;
>>  
>> -    s->msg_action = 0;
>> +    s->msg_action = LSI_MSG_ACTION_COMMAND;
>>      s->msg_len = 0;
>>      s->waiting = LSI_NOWAIT;
>>      s->dsa = 0;
>> @@ -686,7 +691,7 @@ static void lsi_reselect(LSIState *s, lsi_request *p)
>>      trace_lsi_reselect(id);
>>      s->scntl1 |= LSI_SCNTL1_CON;
>>      lsi_set_phase(s, PHASE_MI);
>> -    s->msg_action = p->out ? 2 : 3;
>> +    s->msg_action = p->out ? LSI_MSG_ACTION_DOUT : LSI_MSG_ACTION_DIN;
>>      s->current->dma_len = p->pending;
>>      lsi_add_msg_byte(s, 0x80);
>>      if (s->current->tag & LSI_TAG_VALID) {
>> @@ -857,7 +862,7 @@ static void lsi_do_command(LSIState *s)
>>              lsi_add_msg_byte(s, 4); /* DISCONNECT */
>>              /* wait data */
>>              lsi_set_phase(s, PHASE_MI);
>> -            s->msg_action = 1;
>> +            s->msg_action = LSI_MSG_ACTION_DISCONNECT;
>>              lsi_queue_command(s);
>>          } else {
>>              /* wait command complete */
>> @@ -878,7 +883,7 @@ static void lsi_do_status(LSIState *s)
>>      s->sfbr = status;
>>      pci_dma_write(PCI_DEVICE(s), s->dnad, &status, 1);
>>      lsi_set_phase(s, PHASE_MI);
>> -    s->msg_action = 1;
>> +    s->msg_action = LSI_MSG_ACTION_DISCONNECT;
>>      lsi_add_msg_byte(s, 0); /* COMMAND COMPLETE */
>>  }
>>  
>> @@ -901,16 +906,16 @@ static void lsi_do_msgin(LSIState *s)
>>          /* ??? Check if ATN (not yet implemented) is asserted and maybe
>>             switch to PHASE_MO.  */
>>          switch (s->msg_action) {
>> -        case 0:
>> +        case LSI_MSG_ACTION_COMMAND:
>>              lsi_set_phase(s, PHASE_CMD);
>>              break;
>> -        case 1:
>> +        case LSI_MSG_ACTION_DISCONNECT:
>>              lsi_disconnect(s);
>>              break;
>> -        case 2:
>> +        case LSI_MSG_ACTION_DOUT:
>>              lsi_set_phase(s, PHASE_DO);
>>              break;
>> -        case 3:
>> +        case LSI_MSG_ACTION_DIN:
>>              lsi_set_phase(s, PHASE_DI);
>>              break;
>>          default:
>> @@ -1062,7 +1067,7 @@ bad:
>>      qemu_log_mask(LOG_UNIMP, "Unimplemented message 0x%02x\n", msg);
>>      lsi_set_phase(s, PHASE_MI);
>>      lsi_add_msg_byte(s, 7); /* MESSAGE REJECT */
>> -    s->msg_action = 0;
>> +    s->msg_action = LSI_MSG_ACTION_COMMAND;
>>  }
>>  
>>  #define LSI_BUF_SIZE 4096
>>

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 2/5] lsi: use enum type for s->waiting
  2019-03-04 23:18   ` Philippe Mathieu-Daudé
@ 2019-03-05  7:17     ` Sven Schnelle
  2019-03-05 12:07       ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 15+ messages in thread
From: Sven Schnelle @ 2019-03-05  7:17 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Paolo Bonzini, Fam Zheng, open list:All patches CC here

Hi Philippe,

On Tue, Mar 05, 2019 at 12:18:01AM +0100, Philippe Mathieu-Daudé wrote:
> >  
> > +enum {
> > +    LSI_NOWAIT,
> 
> You forgot the comment for NOWAIT.

I thought LSI_NOWAIT is self-explaining, but will add that.

> >      int waiting;
> 
> When a field is not used by migration, you can declare it as enum:
> 
>        enum {
>            LSI_NOWAIT = 0, /* SCRIPTS are running or stopped */
>            LSI_WAIT_RESELECT = 1, /* Wait Reselect instruction has been
> issued */
>            LSI_DMA_SCRIPTS = 2, /* processing DMA from lsi_execute_script */
>            LSI_DMA_IN_PROGRESS = 3, /* DMA operation is in progress */
>        } waiting;
> 
> This gives hints to the compiler about values to check.

But it is used by migration, so this doesn't apply here? I had a typedef enum before,
but this doesn't compile.

Otherwise thanks for reviewing.

Regards
Sven

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 2/5] lsi: use enum type for s->waiting
  2019-03-05  7:17     ` Sven Schnelle
@ 2019-03-05 12:07       ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-03-05 12:07 UTC (permalink / raw)
  To: Sven Schnelle; +Cc: Fam Zheng, Paolo Bonzini, open list:All patches CC here

On 3/5/19 8:17 AM, Sven Schnelle wrote:
> Hi Philippe,
> 
> On Tue, Mar 05, 2019 at 12:18:01AM +0100, Philippe Mathieu-Daudé wrote:
>>>  
>>> +enum {
>>> +    LSI_NOWAIT,
>>
>> You forgot the comment for NOWAIT.
> 
> I thought LSI_NOWAIT is self-explaining, but will add that.
> 
>>>      int waiting;
>>
>> When a field is not used by migration, you can declare it as enum:
>>
>>        enum {
>>            LSI_NOWAIT = 0, /* SCRIPTS are running or stopped */
>>            LSI_WAIT_RESELECT = 1, /* Wait Reselect instruction has been
>> issued */
>>            LSI_DMA_SCRIPTS = 2, /* processing DMA from lsi_execute_script */
>>            LSI_DMA_IN_PROGRESS = 3, /* DMA operation is in progress */
>>        } waiting;
>>
>> This gives hints to the compiler about values to check.
> 
> But it is used by migration, so this doesn't apply here? I had a typedef enum before,
> but this doesn't compile.

Oh you are right, I didn't check... So with the updated comment:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
> Otherwise thanks for reviewing.
> 
> Regards
> Sven
> 
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-03-05 12:08 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-04 18:09 [Qemu-devel] [PATCH 1/5] lsi: use ldn_le_p()/stn_le_p() Sven Schnelle
2019-03-04 18:09 ` [Qemu-devel] [PATCH 2/5] lsi: use enum type for s->waiting Sven Schnelle
2019-03-04 23:18   ` Philippe Mathieu-Daudé
2019-03-05  7:17     ` Sven Schnelle
2019-03-05 12:07       ` Philippe Mathieu-Daudé
2019-03-04 18:09 ` [Qemu-devel] [PATCH 3/5] lsi: use enum type for s->msg_action Sven Schnelle
2019-03-04 23:14   ` Philippe Mathieu-Daudé
2019-03-04 23:22     ` Philippe Mathieu-Daudé
2019-03-04 18:09 ` [Qemu-devel] [PATCH 4/5] lsi: use SCSI phase names instead of numbers in trace Sven Schnelle
2019-03-04 23:10   ` Philippe Mathieu-Daudé
2019-03-04 18:09 ` [Qemu-devel] [PATCH 5/5] lsi: return dfifo value Sven Schnelle
2019-03-04 18:40 ` [Qemu-devel] [PATCH 1/5] lsi: use ldn_le_p()/stn_le_p() Eric Blake
2019-03-04 20:38   ` Sven Schnelle
2019-03-04 21:15     ` Eric Blake
2019-03-04 23:21 ` Philippe Mathieu-Daudé

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.