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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> Sent: 05 November 2015 12:32
> To: Paul Durrant; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Keir (Xen.org); Jan Beulich
> Subject: Re: [PATCH] x86/hvm: make sure stdvga cache cannot be re-
> enabled
> 
> 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
> 

I'll re-spin with this fixed and regression-introducing commit mentioned in the 
message.

  Paul

> ~Andrew

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