Hi, On 6/12/21 8:24 AM, Liu Cyrus wrote: > Hi folks, this is a suggestion for fixing this bug. > I'm willing to discuss this with you because I'm new to contribute to QEMU.
Thanks for your fix! You didn't Cc'ed the maintainers of the SCSI subsystem (see https://wiki.qemu.org/Contribute/SubmitAPatch#CC_the_relevant_maintainer ) so I'm doing it for you: $ ./scripts/get_maintainer.pl -f hw/scsi/lsi53c895a.c Paolo Bonzini <pbonz...@redhat.com> (supporter:SCSI) Fam Zheng <f...@euphon.net> (reviewer:SCSI) > > Best, > Qiang Liu > From: cyruscyliu <cyruscy...@gmail.com <mailto:cyruscy...@gmail.com>> > > An assertion failure can be triggered in the lsi53c810 emulator by a guest > when ((s->sstat1 & 0x7) == PHASE_DO) || (s->sstat1 & 0x7) == PHASE_DI)) > && (!s->current) holds. > Check s->sstat1 and s->current in lsi_reg_writeb before lsi_execute_script() > to discard this MMIO write. > > Fixes: 7d8406be69ce ("PCI SCSI HBA emulation.") > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/305 > <https://gitlab.com/qemu-project/qemu/-/issues/305> > Buglink: https://bugs.launchpad.net/qemu/+bug/1908515 > <https://bugs.launchpad.net/qemu/+bug/1908515> It seems you didn't send your patch with the proper tool, see https://wiki.qemu.org/Contribute/SubmitAPatch#Use_git_format-patch Please also mention the reporter: Reported-by: Cheolwoo Myung <cwmy...@snu.ac.kr> > Signed-off-by: cyruscyliu <cyruscy...@gmail.com > <mailto:cyruscy...@gmail.com>> Also your git-config is missing your name. Fixable using: $ git config user.name "Liu Cyrus" for your QEMU repository, or globally: $ git config --global user.name "Liu Cyrus" > --- > hw/scsi/lsi53c895a.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c > index e2c1918..5c08f7f 100644 > --- a/hw/scsi/lsi53c895a.c > +++ b/hw/scsi/lsi53c895a.c > @@ -1919,6 +1919,10 @@ static void lsi_reg_writeb(LSIState *s, int > offset, uint8_t val) > lsi_update_irq(s); > } > if (s->waiting == LSI_WAIT_RESELECT && val & LSI_ISTAT0_SIGP) { > + if (!(((((s->sstat1 & 0x7) == PHASE_DO) > + || (s->sstat1 & 0x7) == PHASE_DI)) > + && s->current)) > + break; > trace_lsi_awoken(); > s->waiting = LSI_NOWAIT; > s->dsp = s->dnad; > @@ -1980,8 +1984,13 @@ static void lsi_reg_writeb(LSIState *s, int > offset, uint8_t val) > * instruction. Is this correct? > */ > if ((s->dmode & LSI_DMODE_MAN) == 0 > - && (s->istat1 & LSI_ISTAT1_SRUN) == 0) > + && (s->istat1 & LSI_ISTAT1_SRUN) == 0) { > + if (!(((((s->sstat1 & 0x7) == PHASE_DO) > + || (s->sstat1 & 0x7) == PHASE_DI)) > + && s->current)) Instead of duplicating multiple times the same complex code, you could add an helper once and call it. Something like: static bool can_execute(LSIState *s) { if (!s->current) { return false; } switch (s->sstat1 & 0x7) { case PHASE_DO: case PHASE_DI: return true; default: return false; } } which is while being more verbose, is easier to read. Then you could use: if (!can_execute(s)) { ... } > + break; > lsi_execute_script(s); > + } > break; > CASE_SET_REG32(dsps, 0x30) > CASE_SET_REG32(scratch[0], 0x34) > @@ -2001,8 +2010,13 @@ static void lsi_reg_writeb(LSIState *s, int > offset, uint8_t val) > * FIXME: if s->waiting != LSI_NOWAIT, this will only execute one > * instruction. Is this correct? > */ > - if ((val & LSI_DCNTL_STD) && (s->istat1 & LSI_ISTAT1_SRUN) == 0) > + if ((val & LSI_DCNTL_STD) && (s->istat1 & LSI_ISTAT1_SRUN) == 0) { > + if (!(((((s->sstat1 & 0x7) == PHASE_DO) > + || (s->sstat1 & 0x7) == PHASE_DI)) > + && s->current)) > + break; > lsi_execute_script(s); > + } > break; > case 0x40: /* SIEN0 */ > s->sien0 = val; > -- > 2.7.4 However back to the bug you are trying to fix, I wonder if your check is correct regarding the hardware we are modelling. Have you looked at the specifications? See https://docs.broadcom.com/doc/12352475 Chapter 5.3 Block Move Instruction (from SCSI SCRIPTS Instruction Set), "DMA Command" register. Why are we reaching these places with s->current == NULL in the first place? We are probably missing something earlier. Regards, Phil.
