Re: [Xen-devel] [PATCH] x86/hvm: make sure stdvga cache cannot be re-enabled

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 05/11/15 12:17, Paul Durrant wrote:
> As soon as the cache is disabled, it will become out-of-sync with the
> VGA device model and since no mechanism exists to acquire current VRAM
> state from the device model, re-enabling it leads to stale data
> being seen by the guest.
>
> The problem can be seen by deliberately crashing a Windows guest; the
> BSOD output is corrupted.
>
> This patch changes the existing 'cache' boolean in hvm_hw_stdvga into a
> tri-state enum and only allows the state to move from 'uninitialized' to
> 'enabled'. Once the cache state becomes 'disabled' it will remain so for
> the lifetime of the VM.

Should identify that this is a regression introduced by c/s
3bbaaec09b1b942f5624dee176da6e416d31f982

>
> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> Cc: Keir Fraser <keir@xxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, with one small
issue which could be fixed on commit...

> ---
>  xen/arch/x86/hvm/save.c      |  2 +-
>  xen/arch/x86/hvm/stdvga.c    | 50 
> ++++++++++++++++++++++++++++++++------------
>  xen/include/asm-x86/hvm/io.h |  8 ++++++-
>  3 files changed, 45 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/save.c b/xen/arch/x86/hvm/save.c
> index 4660beb..f7d4999 100644
> --- a/xen/arch/x86/hvm/save.c
> +++ b/xen/arch/x86/hvm/save.c
> @@ -73,7 +73,7 @@ int arch_hvm_load(struct domain *d, struct hvm_save_header 
> *hdr)
>      d->arch.hvm_domain.sync_tsc = rdtsc();
>  
>      /* VGA state is not saved/restored, so we nobble the cache. */
> -    d->arch.hvm_domain.stdvga.cache = 0;
> +    d->arch.hvm_domain.stdvga.cache = STDVGA_CACHE_DISABLED;
>  
>      return 0;
>  }
> diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
> index 02a97f9..246c629 100644
> --- a/xen/arch/x86/hvm/stdvga.c
> +++ b/xen/arch/x86/hvm/stdvga.c
> @@ -101,6 +101,37 @@ static void vram_put(struct hvm_hw_stdvga *s, void *p)
>      unmap_domain_page(p);
>  }
>  
> +static void stdvga_try_cache_enable(struct hvm_hw_stdvga *s)
> +{
> +    /*
> +     * Caching mode can only be enabled if the the cache has
> +     * never been used before. As soon as it is disabled, it will
> +     * become out-of-sync with the VGA device model and since no
> +     * mechanism exists to acquire current VRAM state from the
> +     * device model, re-enabling it would lead to stale data being
> +     * seen by the guest.
> +     */
> +    if ( s->cache != STDVGA_CACHE_UNINITIALIZED )
> +        return;
> +
> +    gdprintk(XENLOG_INFO, "entering caching mode\n");
> +    s->cache = STDVGA_CACHE_ENABLED;
> +}
> +
> +static void stdvga_cache_disable(struct hvm_hw_stdvga *s)
> +{
> +    if ( s->cache != STDVGA_CACHE_ENABLED )
> +        return;
> +
> +    gdprintk(XENLOG_INFO, "leaving caching mode\n");
> +    s->cache = STDVGA_CACHE_DISABLED;
> +}
> +
> +static bool_t stdvga_cache_is_enabled(struct hvm_hw_stdvga *s)

const struct hvm_hw_stdvga *s

~Andrew

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