[Qemu-devel] LSI: avoid infinite loops

[Qemu-devel] LSI: avoid infinite loops

[Marcelo Tosatti]

The Windows driver has SCRIPTS code which busy loops on main memory. So
give the CPU's a chance to run if that happens.

Fixes
http://sourceforge.net/tracker/index.php?func=detail&aid=1895893&group_id=180599&atid=893831

Paul, I'm not conviced that a smarter algorithm to determine this
conditions is necessary... perhaps it would be wise to increase the
"200" magic number.

The scsi-disk.c patch in the bugzilla entry has been submitted but ignored.

diff --git a/qemu/hw/lsi53c895a.c b/qemu/hw/lsi53c895a.c
index 72ed5c3..2691418 100644
--- a/qemu/hw/lsi53c895a.c
+++ b/qemu/hw/lsi53c895a.c
@@ -840,9 +840,11 @@ static void lsi_execute_script(LSIState *s)
     uint32_t insn;
     uint32_t addr;
     int opcode;
+    int insns_processed = 0;
 
     s->istat1 |= LSI_ISTAT1_SRUN;
 again:
+    insns_processed++;
     insn = read_dword(s, s->dsp);
     addr = read_dword(s, s->dsp + 4);
     DPRINTF("SCRIPTS dsp=%08x opcode %08x arg %08x\n", s->dsp, insn, addr);
@@ -1197,8 +1199,13 @@ again:
             }
         }
     }
-    /* ??? Need to avoid infinite loops.  */
-    if (s->istat1 & LSI_ISTAT1_SRUN && !s->waiting) {
+    /*
+     * The Windows driver downloads SCRIPT code which busy loops 
+     * on main memory, so give the CPU's a chance to run if that
+     * happens.
+     */
+    if (insns_processed < 200 && s->istat1 & LSI_ISTAT1_SRUN &&
+       !s->waiting) {
         if (s->dcntl & LSI_DCNTL_SSM) {
             lsi_script_dma_interrupt(s, LSI_DSTAT_SSI);
         } else {

[Qemu-devel] Re: LSI: avoid infinite loops

[Paul Brook]

On Thursday 08 May 2008, Marcelo Tosatti wrote:
> The Windows driver has SCRIPTS code which busy loops on main memory. So
> give the CPU's a chance to run if that happens.

I'm kinda surprised this works.  What causes the scripts engine to be 
restarted?

Paul

[Qemu-devel] Re: LSI: avoid infinite loops

[Marcelo Tosatti]

On Thu, May 08, 2008 at 12:21:54AM +0100, Paul Brook wrote:
> On Thursday 08 May 2008, Marcelo Tosatti wrote:
> > The Windows driver has SCRIPTS code which busy loops on main memory. So
> > give the CPU's a chance to run if that happens.
> 
> I'm kinda surprised this works.  What causes the scripts engine to be 
> restarted?

LSI_ISTAT0_SIGP.

Re: [Qemu-devel] Re: LSI: avoid infinite loops

[Paul Brook]

On Thursday 08 May 2008, Marcelo Tosatti wrote:
> On Thu, May 08, 2008 at 12:21:54AM +0100, Paul Brook wrote:
> > On Thursday 08 May 2008, Marcelo Tosatti wrote:
> > > The Windows driver has SCRIPTS code which busy loops on main memory. So
> > > give the CPU's a chance to run if that happens.
> >
> > I'm kinda surprised this works.  What causes the scripts engine to be
> > restarted?
>
> LSI_ISTAT0_SIGP.

In that case my surprise continues, and this is looking like an unbelievably 
horrid hack.

By my reading you're making LSI_ISTAT0_SIGP effect whatever instruction 
happens to be executing when we stall. You get doubly lucky because (a) the 
guest OS decides to bang on SIGP, even though it doesn't need to. And (b) the 
last instruction executed happens to have set dnad to a value that "works". 
I'm guessing you always happen to stop execution on the conditional jump 
instruction and taking that jump doesn't cause any bad effects, right?

Paul

Re: [Qemu-devel] Re: LSI: avoid infinite loops

[Paul Brook]

On Thursday 08 May 2008, Paul Brook wrote:
> On Thursday 08 May 2008, Marcelo Tosatti wrote:
> > On Thu, May 08, 2008 at 12:21:54AM +0100, Paul Brook wrote:
> > > On Thursday 08 May 2008, Marcelo Tosatti wrote:
> > > > The Windows driver has SCRIPTS code which busy loops on main memory.
> > > > So give the CPU's a chance to run if that happens.
> > >
> > > I'm kinda surprised this works.  What causes the scripts engine to be
> > > restarted?
> >
> > LSI_ISTAT0_SIGP.
>
> In that case my surprise continues, and this is looking like an
> unbelievably horrid hack.
>
> By my reading you're making LSI_ISTAT0_SIGP effect whatever instruction
> happens to be executing when we stall. You get doubly lucky because (a) the
> guest OS decides to bang on SIGP, even though it doesn't need to. And (b)
> the last instruction executed happens to have set dnad to a value that
> "works". I'm guessing you always happen to stop execution on the
> conditional jump instruction and taking that jump doesn't cause any bad
> effects, right?

Oh, I'd also be worried what happens if an async IO operation completes at 
this point. lsi_command_complete is liable to trample all over your state.

Paul

Re: [Qemu-devel] Re: LSI: avoid infinite loops

[Marcelo Tosatti]

On Thu, May 08, 2008 at 01:52:11AM +0100, Paul Brook wrote:
> On Thursday 08 May 2008, Paul Brook wrote:
> > On Thursday 08 May 2008, Marcelo Tosatti wrote:
> > > On Thu, May 08, 2008 at 12:21:54AM +0100, Paul Brook wrote:
> > > > On Thursday 08 May 2008, Marcelo Tosatti wrote:
> > > > > The Windows driver has SCRIPTS code which busy loops on main memory.
> > > > > So give the CPU's a chance to run if that happens.
> > > >
> > > > I'm kinda surprised this works.  What causes the scripts engine to be
> > > > restarted?
> > >
> > > LSI_ISTAT0_SIGP.
> >
> > In that case my surprise continues, and this is looking like an
> > unbelievably horrid hack.
> >
> > By my reading you're making LSI_ISTAT0_SIGP effect whatever instruction
> > happens to be executing when we stall. You get doubly lucky because (a) the
> > guest OS decides to bang on SIGP, even though it doesn't need to. And (b)
> > the last instruction executed happens to have set dnad to a value that
> > "works". I'm guessing you always happen to stop execution on the
> > conditional jump instruction and taking that jump doesn't cause any bad
> > effects, right?
> 
> Oh, I'd also be worried what happens if an async IO operation completes at 
> this point. lsi_command_complete is liable to trample all over your state.

So what do you suggest as a proper fix?

Re: [Qemu-devel] Re: LSI: avoid infinite loops

[Marcelo Tosatti]

On Thu, May 08, 2008 at 12:13:15AM -0300, Marcelo Tosatti wrote:
> On Thu, May 08, 2008 at 01:52:11AM +0100, Paul Brook wrote:
> > On Thursday 08 May 2008, Paul Brook wrote:
> > > On Thursday 08 May 2008, Marcelo Tosatti wrote:
> > > > On Thu, May 08, 2008 at 12:21:54AM +0100, Paul Brook wrote:
> > > > > On Thursday 08 May 2008, Marcelo Tosatti wrote:
> > > > > > The Windows driver has SCRIPTS code which busy loops on main memory.
> > > > > > So give the CPU's a chance to run if that happens.
> > > > >
> > > > > I'm kinda surprised this works.  What causes the scripts engine to be
> > > > > restarted?
> > > >
> > > > LSI_ISTAT0_SIGP.
> > >
> > > In that case my surprise continues, and this is looking like an
> > > unbelievably horrid hack.
> > >
> > > By my reading you're making LSI_ISTAT0_SIGP effect whatever instruction
> > > happens to be executing when we stall. You get doubly lucky because (a) the
> > > guest OS decides to bang on SIGP, even though it doesn't need to. And (b)
> > > the last instruction executed happens to have set dnad to a value that
> > > "works". I'm guessing you always happen to stop execution on the
> > > conditional jump instruction and taking that jump doesn't cause any bad
> > > effects, right?
> > 
> > Oh, I'd also be worried what happens if an async IO operation completes at 
> > this point. lsi_command_complete is liable to trample all over your state.
> 
> So what do you suggest as a proper fix?

Paul, 

What do you suggest as a proper fix to this problem?

Re: [Qemu-devel] Re: LSI: avoid infinite loops

[Paul Brook]

> > > > > > > The Windows driver has SCRIPTS code which busy loops on main
> > > > > > > memory. So give the CPU's a chance to run if that happens.
> > > > > >
> > > > > > I'm kinda surprised this works.  What causes the scripts engine
> > > > > > to be restarted?
> > > > >
> > > > > LSI_ISTAT0_SIGP.
> > > >
> > > > In that case my surprise continues, and this is looking like an
> > > > unbelievably horrid hack.
> > > >
> > > > By my reading you're making LSI_ISTAT0_SIGP effect whatever
> > > > instruction happens to be executing when we stall. You get doubly
> > > > lucky because (a) the guest OS decides to bang on SIGP, even though
> > > > it doesn't need to. And (b) the last instruction executed happens to
> > > > have set dnad to a value that "works". I'm guessing you always happen
> > > > to stop execution on the conditional jump instruction and taking that
> > > > jump doesn't cause any bad effects, right?
> > >
> > > Oh, I'd also be worried what happens if an async IO operation completes
> > > at this point. lsi_command_complete is liable to trample all over your
> > > state.
> >
> > So what do you suggest as a proper fix?
>
> What do you suggest as a proper fix to this problem?

At minimum you need to address the issues I've raised with your current patch.  
Stalling execution temporarily every few hundred instructions and waiting for 
SIGP (or some other trigger) before resuming may be acceptable.  Aborting 
execution and relying on very specific guest OS behavior to give correct 
results is not.  The current code is written with the assumption that 
execution will only stop at very specific points. Your patch breaks this 
assumption.

Ideally you'd also do proper loop detection rather than setting an arbitrary 
limit. I wouldn't be surprised if a good OS can create very long queues.

Paul