Re: [PATCH] [HVM] Prevent usb driver crashes in Windows

[Steve Ofsthun <sofsthun@virtualiron.com>]

Keir Fraser wrote:
> What precisely is the race? Qemu-dm is single threaded. Memcpy() probably
> ends up doing int copies anyway if things are appropriately aligned
> (although we might not want to rely on that).

The race is between a domU guest cpu running guest driver code and a dom0
cpu running the timer based USB emulation code in QEMU.

A Windows domU driver is accessing and modifying the USB controller data
simultaneously with the USB controller emulation in QEMU.  The controller
data consists of linked lists/queues that a real hardware USB controller
would access/modify atomically.  The USB emulation in QEMU was updating
queue/list entries with cpu_physical_memory_rw() which ultimately calls
memcpy.  This allowed the windows USB subsystem to see partial updates to
physical addresses in these lists/queues that were not stored there by
Windows.  Windows responded to these transient "data corruptions" with a
BSOD.  We generally see this when stacking many Windows guests that are
using a usb mouse/tablet pointing device.

The attached patch simply uses word copies when alignment and size are
appropriate.

Steve
> 
>  -- Keir
> 
> On 6/6/07 17:00, "Ben Guthro" <bguthro@virtualiron.com> wrote:
> 
>> qemu-word-tearing.patch:
>> Use atomic updates to read/write usb controller data.
>> This can be done because:
>>     a) word copies on x86 are atomic
>>     b) The USB spec requires word alignment
>>
>> This will need to be enhanced once USB 1.2 is supported.
>>
>> Signed-off-by: Steve Ofsthun <sofsthun@virtualiron.com>
>>
>> diff -r 86fa3e4277f6 tools/ioemu/target-i386-dm/exec-dm.c
>> --- a/tools/ioemu/target-i386-dm/exec-dm.c Mon Jun 04 11:26:30 2007 -0400
>> +++ b/tools/ioemu/target-i386-dm/exec-dm.c Mon Jun 04 11:29:03 2007 -0400
>> @@ -434,6 +434,28 @@ extern unsigned long *logdirty_bitmap;
>>  extern unsigned long *logdirty_bitmap;
>>  extern unsigned long logdirty_bitmap_size;
>>  
>> +/*
>> + * Replace the standard byte memcpy with a int memcpy for appropriately sized
>> + * memory copy operations.  Some users (USB-UHCI) can not tolerate the
>> possible
>> + * word tearing that can result from a guest concurrently writing a memory
>> + * structure while the qemu device model is modifying the same location.
>> + * Forcing a int sized read/write prevents the guest from seeing a partially
>> + * written int sized atom.
>> + */
>> +void memcpy32(void *dst, void *src, size_t n)
>> +{
>> +    if ((n % sizeof(int)) != 0) {
>> +        memcpy(dst, src, n);
>> +        return;
>> +    }
>> +    n /= sizeof(int);
>> +    while(n--) {
>> +        *(int *)dst = *(int *)src;
>> +        dst += sizeof(int);
>> +        src += sizeof(int);
>> +    }
>> +}
>> +
>>  void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
>>                              int len, int is_write)
>>  {
>> @@ -470,7 +492,7 @@ void cpu_physical_memory_rw(target_phys_
>>                  }
>>              } else if ((ptr = phys_ram_addr(addr)) != NULL) {
>>                  /* Writing to RAM */
>> -                memcpy(ptr, buf, l);
>> +                memcpy32(ptr, buf, l);
>>                  if (logdirty_bitmap != NULL) {
>>                      /* Record that we have dirtied this frame */
>>                      unsigned long pfn = addr >> TARGET_PAGE_BITS;
>> @@ -506,7 +528,7 @@ void cpu_physical_memory_rw(target_phys_
>>                  }
>>              } else if ((ptr = phys_ram_addr(addr)) != NULL) {
>>                  /* Reading from RAM */
>> -                memcpy(buf, ptr, l);
>> +                memcpy32(buf, ptr, l);
>>              } else {
>>                  /* Neither RAM nor known MMIO space */
>>                  memset(buf, 0xff, len);
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel


-- 
Steve Ofsthun - Virtual Iron Software, Inc.