Re: [PATCH] hw/scsi/lsi53c895a: add timer to scripts processing

[Sven Schnelle <svens@stackframe.org>]

BALATON Zoltan <balaton@eik.bme.hu> writes:

> On Thu, 29 Feb 2024, Sven Schnelle wrote:
>> Some OS's like HP-UX 10.20 are spinn
>
> I guess the above line is left here by accident.

Yes.

>> HP-UX 10.20 seems to make the lsi53c895a spinning on a memory location
>> under certain circumstances. As the SCSI controller and CPU are not
>> running at the same time this loop will never finish. After some
>> time, the check loop interrupts with a unexpected device disconnect.
>> This works, but is slow because the kernel resets the scsi controller.
>> Instead of signaling UDC, start a timer and exit the loop. Until the
>> timer fires, the CPU can process instructions until the timer fires.
>> The limit of instructions is also reduced because scripts running
>> on the SCSI processor are usually very short. This keeps the time
>> until the loop-exit short.
>>
>> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Sven Schnelle <svens@stackframe.org>
>> ---
>> hw/scsi/lsi53c895a.c | 33 +++++++++++++++++++++++++--------
>> 1 file changed, 25 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
>> index d607a5f9fb..0b6f1dc72f 100644
>> --- a/hw/scsi/lsi53c895a.c
>> +++ b/hw/scsi/lsi53c895a.c
>> @@ -188,7 +188,7 @@ static const char *names[] = {
>> #define LSI_TAG_VALID     (1 << 16)
>>
>> /* Maximum instructions to process. */
>> -#define LSI_MAX_INSN    10000
>> +#define LSI_MAX_INSN    100
>>
>> typedef struct lsi_request {
>>     SCSIRequest *req;
>> @@ -205,6 +205,7 @@ enum {
>>     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 */
>> +    LSI_WAIT_SCRIPTS, /* SCRIPTS stopped because of instruction count limit */
>> };
>>
>> enum {
>> @@ -224,6 +225,7 @@ struct LSIState {
>>     MemoryRegion ram_io;
>>     MemoryRegion io_io;
>>     AddressSpace pci_io_as;
>> +    QEMUTimer *scripts_timer;
>>
>>     int carry; /* ??? Should this be an a visible register somewhere?  */
>>     int status;
>> @@ -415,6 +417,7 @@ static void lsi_soft_reset(LSIState *s)
>>     s->sbr = 0;
>>     assert(QTAILQ_EMPTY(&s->queue));
>>     assert(!s->current);
>> +    timer_del(s->scripts_timer);
>
> Maybe the rimer needs to be deleted in lsi_scsi_exit() too but I'm not
> sure.

I added it, thanks.

>> }
>>
>> static int lsi_dma_40bit(LSIState *s)
>> @@ -1127,6 +1130,12 @@ static void lsi_wait_reselect(LSIState *s)
>>     }
>> }
>>
>> +static void lsi_scripts_timer_start(LSIState *s)
>> +{
>> +    trace_lsi_scripts_timer_start();
>> +    timer_mod(s->scripts_timer, qemu_clock_get_us(QEMU_CLOCK_VIRTUAL) + 500);
>> +}
>> +
>> static void lsi_execute_script(LSIState *s)
>> {
>>     PCIDevice *pci_dev = PCI_DEVICE(s);
>> @@ -1152,13 +1161,8 @@ again:
>>      * which should be enough for all valid use cases).
>>      */
>
> Does tha above comment need updating to say what the code does now?

Yes, i changed it to describe the new method:

    /*
     * Some windows drivers make the device spin waiting for a memory location
     * to change. If we have executed more than LSI_MAX_INSN instructions then
     * assume this is the case and start a timer. Until the timer fires, the
     * host CPU has a chance to run and change the memory location.
     *
     * Another issue (CVE-2023-0330) can occur if the script is programmed to
     * trigger itself again and again. Avoid this problem by stopping after
     * being called multiple times in a reentrant way (8 is an arbitrary value
     * which should be enough for all valid use cases).
     */

> Regards,
> BALATON Zoltan
>
>>     if (++insn_processed > LSI_MAX_INSN || reentrancy_level > 8) {
>> -        if (!(s->sien0 & LSI_SIST0_UDC)) {
>> -            qemu_log_mask(LOG_GUEST_ERROR,
>> -                          "lsi_scsi: inf. loop with UDC masked");
>> -        }
>> -        lsi_script_scsi_interrupt(s, LSI_SIST0_UDC, 0);
>> -        lsi_disconnect(s);
>> -        trace_lsi_execute_script_stop();
>> +        s->waiting = LSI_WAIT_SCRIPTS;
>> +        lsi_scripts_timer_start(s);
>>         reentrancy_level--;
>>         return;
>>     }
>> @@ -2197,6 +2201,9 @@ static int lsi_post_load(void *opaque, int version_id)
>>         return -EINVAL;
>>     }
>>
>> +    if (s->waiting == LSI_WAIT_SCRIPTS) {
>> +        lsi_scripts_timer_start(s);
>> +    }
>>     return 0;
>> }
>>
>> @@ -2294,6 +2301,15 @@ static const struct SCSIBusInfo lsi_scsi_info = {
>>     .cancel = lsi_request_cancelled
>> };
>>
>> +static void scripts_timer_cb(void *opaque)
>> +{
>> +    LSIState *s = opaque;
>> +
>> +    trace_lsi_scripts_timer_triggered();
>> +    s->waiting = LSI_NOWAIT;
>> +    lsi_execute_script(s);
>> +}
>> +
>> static void lsi_scsi_realize(PCIDevice *dev, Error **errp)
>> {
>>     LSIState *s = LSI53C895A(dev);
>> @@ -2313,6 +2329,7 @@ static void lsi_scsi_realize(PCIDevice *dev, Error **errp)
>>                           "lsi-ram", 0x2000);
>>     memory_region_init_io(&s->io_io, OBJECT(s), &lsi_io_ops, s,
>>                           "lsi-io", 256);
>> +    s->scripts_timer = timer_new_us(QEMU_CLOCK_VIRTUAL, scripts_timer_cb, s);
>>
>>     /*
>>      * Since we use the address-space API to interact with ram_io, disable the
>>