[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3] lsi_scsi: Reselection needed to remove pendi
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH v3] lsi_scsi: Reselection needed to remove pending commands from queue |
Date: |
Wed, 31 Oct 2018 10:21:55 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0 |
On 30/10/2018 22:42, George Kennedy wrote:
> Under heavy IO (e.g. fio) the queue is not checked frequently enough for
> pending commands. As a result some pending commands are timed out by the
> linux sym53c8xx driver, which sends SCSI Abort messages for the timed out
> commands. The SCSI Abort messages result in linux errors, which show up
> on the console and in /var/log/messages.
>
> e.g.
> sd 0:0:3:0: [sdd] tag#33 ABORT operation started
> scsi target0:0:3: control msgout:
> 80 20 47 d
> sd 0:0:3:0: ABORT operation complete.
> scsi target0:0:4: message d sent on bad reselection
>
> When the current command completes, check if there is a pending command
> on the queue and if a pending command exists, set a flag indicating
> that a Wait Reselect command is needed to handle a queued pending command.
>
> When a Wait Reselect is needed, intercept and save the current DMA Scripts
> Ptr (DSP) contents and load it instead with the pointer to the Reselection
> Scripts. When Reselection has completed, restore the original DSP contents.
>
> Signed-off-by: George Kennedy <address@hidden>
> ---
> hw/scsi/lsi53c895a.c | 51 ++++++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 40 insertions(+), 11 deletions(-)
>
> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> index d1e6534..b783d72 100644
> --- a/hw/scsi/lsi53c895a.c
> +++ b/hw/scsi/lsi53c895a.c
> @@ -219,6 +219,8 @@ typedef struct {
> int command_complete;
> QTAILQ_HEAD(, lsi_request) queue;
> lsi_request *current;
> + bool want_resel; /* need resel to handle queued completed cmds */
> + uint32_t resel_dsp; /* DMA Scripts Ptr (DSP) of reselection scsi scripts
> */
>
> uint32_t dsa;
> uint32_t temp;
> @@ -298,6 +300,18 @@ static inline int lsi_irq_on_rsl(LSIState *s)
> return (s->sien0 & LSI_SIST0_RSL) && (s->scid & LSI_SCID_RRE);
> }
>
> +static lsi_request *get_pending_req(LSIState *s)
> +{
> + lsi_request *p;
> +
> + QTAILQ_FOREACH(p, &s->queue, next) {
> + if (p->pending) {
> + return p;
> + }
> + }
> + return NULL;
> +}
> +
> static void lsi_soft_reset(LSIState *s)
> {
> trace_lsi_reset();
> @@ -446,7 +460,6 @@ static void lsi_update_irq(LSIState *s)
> {
> int level;
> static int last_level;
> - lsi_request *p;
>
> /* It's unclear whether the DIP/SIP bits should be cleared when the
> Interrupt Status Registers are cleared or when istat0 is read.
> @@ -477,12 +490,12 @@ static void lsi_update_irq(LSIState *s)
> lsi_set_irq(s, level);
>
> if (!level && lsi_irq_on_rsl(s) && !(s->scntl1 & LSI_SCNTL1_CON)) {
> + lsi_request *p;
> +
> trace_lsi_update_irq_disconnected();
> - QTAILQ_FOREACH(p, &s->queue, next) {
> - if (p->pending) {
> - lsi_reselect(s, p);
> - break;
> - }
> + p = get_pending_req(s);
> + if (p) {
> + lsi_reselect(s, p);
> }
> }
> }
> @@ -759,6 +772,8 @@ static void lsi_command_complete(SCSIRequest *req,
> uint32_t status, size_t resid
> lsi_request_free(s, s->current);
> scsi_req_unref(req);
> }
> + s->want_resel = get_pending_req(s) ? true : false;
> +
> lsi_resume_script(s);
> }
>
> @@ -1064,11 +1079,13 @@ static void lsi_wait_reselect(LSIState *s)
>
> trace_lsi_wait_reselect();
>
> - QTAILQ_FOREACH(p, &s->queue, next) {
> - if (p->pending) {
> - lsi_reselect(s, p);
> - break;
> - }
> + if (s->current) {
> + return;
> + }
> +
> + p = get_pending_req(s);
> + if (p) {
> + lsi_reselect(s, p);
> }
> if (s->current == NULL) {
> s->waiting = 1;
> @@ -1082,6 +1099,7 @@ static void lsi_execute_script(LSIState *s)
> uint32_t addr, addr_high;
> int opcode;
> int insn_processed = 0;
> + uint32_t save_dsp = 0;
>
> s->istat1 |= LSI_ISTAT1_SRUN;
> again:
> @@ -1260,7 +1278,14 @@ again:
> s->scntl1 &= ~LSI_SCNTL1_CON;
> break;
> case 2: /* Wait Reselect */
> + if (!s->resel_dsp) {
> + s->resel_dsp = s->dsp - 8;
> + }
What is the reason for the "if"?
> if (!lsi_irq_on_rsl(s)) {
> + if (save_dsp) {
> + s->dsp = save_dsp;
> + save_dsp = 0;
> + }
> lsi_wait_reselect(s);
> }
> break;
> @@ -1476,6 +1501,10 @@ again:
>
> if (insn & (1 << 28)) {
> addr = s->dsa + sextract32(addr, 0, 24);
> + } else if (s->want_resel && s->resel_dsp && !(insn & (1 << 24)))
> {
> + save_dsp = s->dsp;
> + s->dsp = s->resel_dsp;
> + s->want_resel = false;
> }
Can you explain why this is placed under the LOAD AND STORE instruction?
Your previous patch has, more specifically, a store from the scratch
register into memory. In the Linux scripts this occurs twice, both
after a DISCONNECT instruction. Is that the place where you want to hook?
(All this is a bit of a hack of course, and I would rather have the
actual solution that adds the appropriate RESELECTION and MESSAGE IN
phases whenever the device has finished the data transfer. But with a
FIXME comment I guess it might be okay).
Paolo