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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.06.15 at 17:42, <paul.durrant@xxxxxxxxxx> wrote:
> It's clear from the following check in hvmemul_rep_movs:
> 
>     if ( sp2mt == p2m_mmio_direct || dp2mt == p2m_mmio_direct ||
>          (sp2mt == p2m_mmio_dm && dp2mt == p2m_mmio_dm) )
>         return X86EMUL_UNHANDLEABLE;
> 
> that mmio <-> mmio copy is not handled. This means the code in the
> stdvga mmio intercept that explicitly handles mmio <-> mmio copy when
> hvm_copy_to/from_guest_phys() fails is never going to be executed.

Looking at the code I agree with the analysis, but I doubt the code
was put there originally just for fun, and I wonder whether some of
the slowness we "gained" over the years in boot loader graphics
performance isn't related to this code having got hooked off. And in
the end we should get to the point where MMIO -> MMIO transfers
get emulated efficiently anyway, so I wonder whether this patch
moves us in the right direction.

> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -273,6 +273,15 @@ static int hvmemul_acquire_page(unsigned long gmfn, 
> struct page_info **page)
>          return X86EMUL_RETRY;
>      }
>  
> +    /* This code should not be reached if the gmfn is not RAM */
> +    if ( p2m_is_mmio(p2mt) )
> +    {
> +        domain_crash(d);

Log a message at least? Silent domain death is rather ugly to analyze.

> --- a/xen/arch/x86/hvm/stdvga.c
> +++ b/xen/arch/x86/hvm/stdvga.c
> @@ -275,7 +275,8 @@ static uint8_t stdvga_mem_readb(uint64_t addr)
>      return ret;
>  }
>  
> -static uint64_t stdvga_mem_read(uint64_t addr, uint64_t size)
> +static int stdvga_mem_read(struct vcpu *v, unsigned long addr,
> +                           unsigned long size, unsigned long *p_data)
>  {
>      uint64_t data = 0;
>  
> @@ -313,7 +314,9 @@ static uint64_t stdvga_mem_read(uint64_t addr, uint64_t 
> size)
>          break;
>      }
>  
> -    return data;
> +    *p_data = data;

Even if sizeof(long) == sizeof(uint64_t), please try to use
consistent types (i.e. the new function parameter should be of
uint64_t * type).

> +    if (!hvm_buffered_io_send(&p))
> +        return X86EMUL_UNHANDLEABLE;

Coding style.

> +static int stdvga_mem_check(struct vcpu *v, unsigned long addr)
>  {
> +    struct hvm_hw_stdvga *s = &v->domain->arch.hvm_domain.stdvga;
> +    int rc;
>  
>      spin_lock(&s->lock);
>  
> +    rc = s->stdvga && s->cache &&
> +        (addr >= VGA_MEM_BASE) &&
> +        (addr < (VGA_MEM_BASE + VGA_MEM_SIZE));

This (compared with the code being removed) makes clear that the
check() mechanism lacks the size (it did so before, but with your
generalization I think this needs to be corrected).

> +const struct hvm_mmio_ops stdvga_mem_ops = {

static

Jan


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