Re: [Xen-devel] Deadlock in stdvga_mem_accept() with emulation

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.07.15 at 11:30, <Paul.Durrant@xxxxxxxxxx> wrote:
>>  -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: 13 July 2015 10:28
>> To: Paul Durrant
>> Cc: Razvan Cojocaru; Andrew Cooper; xen-devel@xxxxxxxxxxxxx; Keir
>> (Xen.org)
>> Subject: RE: [Xen-devel] Deadlock in stdvga_mem_accept() with emulation
>> 
>> >>> On 13.07.15 at 11:05, <Paul.Durrant@xxxxxxxxxx> wrote:
>> > --- a/xen/arch/x86/hvm/stdvga.c
>> > +++ b/xen/arch/x86/hvm/stdvga.c
>> > @@ -490,11 +490,18 @@ static bool_t stdvga_mem_accept(const struct
>> > hvm_io_handle
>> >  {
>> >      struct hvm_hw_stdvga *s = &current->domain-
>> >arch.hvm_domain.stdvga;
>> >
>> > +    /*
>> > +     * The range check must be done without taking any locks, to avoid
>> > +     * deadlock when hvm_mmio_internal() is called from
>> > +     * hvm_copy_to/from_guest_phys() in hvm_process_io_intercept().
>> > +     */
>> > +    if ( (hvm_mmio_first_byte(p) < VGA_MEM_BASE) ||
>> > +         (hvm_mmio_last_byte(p) >= (VGA_MEM_BASE + VGA_MEM_SIZE))
>> )
>> > +        return 0;
>> > +
>> >      spin_lock(&s->lock);
>> >
>> > -    if ( !s->stdvga ||
>> > -         (hvm_mmio_first_byte(p) < VGA_MEM_BASE) ||
>> > -         (hvm_mmio_last_byte(p) >= (VGA_MEM_BASE + VGA_MEM_SIZE)) )
>> > +    if ( !s->stdvga )
>> >          goto reject;
>> >
>> >      if ( p->dir == IOREQ_WRITE && p->count > 1 )
>> 
>> But won't the problem continue to exist if the address falls within the
>> VGA range? I.e. isn't the problem that the two uses of
>> hvm_mmio_internal() are quite different - while
>> hvm_hap_nested_page_fault() immediately afterwards calls a
>> handle_mmio() variant (which would even seem to call for the lock not
>> getting dropped between them), __hvm_copy() uses it as just a check.
>> 
>> I.e. perhaps better to convert the lock to a recursive one?
> 
> I think we are ok because the stdvga model will never actually accept the 
> I/O since MMIO <-> MMIO rep mov is explicitly disallowed.

True, for now at least.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel