Re: [PATCH] Fix assertion failure in lsi53c810 emulator

[Qiang Liu]

Hi Phil,

> 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:
Thank you!

> 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 <cwmyung@snu.ac.kr>
>
> 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"
Thank you again. I'm sorry that I do miss several basic settings.
I will do better next time.

> Instead of duplicating multiple times the same complex code, you could
> add a helper once and call it.
This is really a good idea.

> However back to the bug you are trying to fix, I wonder if your check
> is correct regarding the hardware we are modeling. 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.
To be honest, I didn't check the specification at that time. I formed this patch
following the idea that we could discard an invalid MMIO operation to
avoid crashing.
Does it seem that this idea is not enough to form a good patch? What are
the best practices to fix an assertion failure in QEMU?

> Why are we reaching these places with s->current == NULL in the first
> place? We are probably missing something earlier.
I checked the specification for several hours today, but it is too
difficult for me.
I need more time to understand it and form a better patch.

When reproducing the crash, I found that I didn't prepare any script to be
executed by lsi_execute_script. However, `insn = read_dword(s, s->dsp)` did get
an instruction at `s->dsp`. Maybe I should check this more.

Best,
Qiang