Re: [Xen-devel] [PATCH v4 09/17] x86/hvm: unify stdvga mmio intercept with standard mmio intercept

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 24 June 2015 14:59
> To: Paul Durrant
> Cc: Andrew Cooper; xen-devel@xxxxxxxxxxxxxxxxxxxx; Keir (Xen.org)
> Subject: Re: [PATCH v4 09/17] x86/hvm: unify stdvga mmio intercept with
> standard mmio intercept
> 
> >>> On 24.06.15 at 13:24, <paul.durrant@xxxxxxxxxx> wrote:
> > @@ -424,8 +427,17 @@ static void stdvga_mem_writeb(uint64_t addr,
> uint32_t val)
> >      }
> >  }
> >
> > -static void stdvga_mem_write(uint64_t addr, uint64_t data, uint64_t size)
> > +static int stdvga_mem_write(struct vcpu *v, unsigned long addr,
> > +                            unsigned long size, unsigned long data)
> >  {
> > +    ioreq_t p = { .type = IOREQ_TYPE_COPY,
> > +                  .addr = addr,
> > +                  .size = size,
> > +                  .count = 1,
> > +                  .dir = IOREQ_WRITE,
> > +                  .data = data,
> 
> Indentation.
> 

Damn emacs.

> > -    if ( s->stdvga && s->cache )
> > -    {
> > -        switch ( p->type )
> > -        {
> > -        case IOREQ_TYPE_COPY:
> > -            buf = mmio_move(s, p);
> > -            if ( !buf )
> > -                s->cache = 0;
> > -            break;
> > -        default:
> > -            gdprintk(XENLOG_WARNING, "unsupported mmio request type:%d
> "
> > -                     "addr:0x%04x data:0x%04x size:%d count:%d state:%d "
> > -                     "isptr:%d dir:%d df:%d\n",
> > -                     p->type, (int)p->addr, (int)p->data, (int)p->size,
> > -                     (int)p->count, p->state,
> > -                     p->data_is_ptr, p->dir, p->df);
> > -            s->cache = 0;
> > -        }
> 
> I can't see where these cases of clearing s->cache move to.
> 

There's only one case AFAICT, which is if the domain goes through a 
save/restore then s->cache is cleared.

> > -    }
> > -    else
> > -    {
> > -        buf = (p->dir == IOREQ_WRITE);
> > -    }
> > -
> > -    rc = (buf && hvm_buffered_io_send(p));
> > +    rc = s->stdvga && s->cache &&
> > +        (addr >= VGA_MEM_BASE) &&
> > +        ((addr + length) < (VGA_MEM_BASE + VGA_MEM_SIZE));
> 
> Not how the old code also calls hvm_buffered_io_send() when
> !s->stdvga || !s->cache but p->dir == IOREQ_WRITE. Do you
> really mean to drop that?
> 

Hmmm. I'm not sure why it would have done that. It seems wrong. I'll check.

  Paul

> Jan


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