* [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.