[PATCH (for 4.6)] x86/hvm: Unconditionally buffer writes to VRAM

[PATCH (for 4.6)] x86/hvm: Unconditionally buffer writes to VRAM

[Paul Durrant]

When c/s 3bbaaec09 "unify stdvga mmio intercept with standard mmio
intercept" was added, a small semantic change was made. Prior to
this patch the hypervisor unconditionally sent all guest writes
to the VGA aperture as buffered ioreqs, whereas after the patch it
only does this when the VGA model is in 'stdvga' mode (sequencer
register #7 == 0).

When installing Windows 7 (64-bit) using the default QEMU VGA model
(== cirrus), Windows leaves 'stdvga' mode early in boot and hence
all further writes to the VGA aperture are done using synchronous
ioreqs which slows down boot by several orders of magnitude (thanks
to the elaborate splash screen that Windows presents). This can be
viewed as a regression and so this patch re-instates previous
buffering behaviour.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Tested-by: Wei Liu <wei.liu2@citrix.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/stdvga.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index 6d22b22..f50bff7 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -441,7 +441,7 @@ static int stdvga_mem_write(const struct hvm_io_handler *handler,
     };
     struct hvm_ioreq_server *srv;
 
-    if ( !s->cache )
+    if ( !s->cache || !s->stdvga )
         goto done;
 
     /* Intercept mmio write */
@@ -503,9 +503,6 @@ static bool_t stdvga_mem_accept(const struct hvm_io_handler *handler,
 
     spin_lock(&s->lock);
 
-    if ( !s->stdvga )
-        goto reject;
-
     if ( p->dir == IOREQ_WRITE && p->count > 1 )
     {
         /*
@@ -526,7 +523,7 @@ static bool_t stdvga_mem_accept(const struct hvm_io_handler *handler,
 
         goto reject;
     }
-    else if ( p->dir == IOREQ_READ && !s->cache )
+    else if ( p->dir == IOREQ_READ && (!s->cache || !s->stdvga) )
         goto reject;
 
     /* s->lock intentionally held */
-- 
1.7.10.4

Re: [PATCH (for 4.6)] x86/hvm: Unconditionally buffer writes to VRAM

[Andrew Cooper]

On 16/07/15 10:24, Paul Durrant wrote:
> When c/s 3bbaaec09 "unify stdvga mmio intercept with standard mmio
> intercept" was added, a small semantic change was made. Prior to
> this patch the hypervisor unconditionally sent all guest writes
> to the VGA aperture as buffered ioreqs, whereas after the patch it
> only does this when the VGA model is in 'stdvga' mode (sequencer
> register #7 == 0).
>
> When installing Windows 7 (64-bit) using the default QEMU VGA model
> (== cirrus), Windows leaves 'stdvga' mode early in boot and hence
> all further writes to the VGA aperture are done using synchronous
> ioreqs which slows down boot by several orders of magnitude (thanks
> to the elaborate splash screen that Windows presents). This can be
> viewed as a regression and so this patch re-instates previous
> buffering behaviour.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Tested-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>

This is unfortunate, but should be returned to its previous behaviour.

Reviewed-by: Andrew Cooper <andrew.cooper@citrix.com>

> ---
>  xen/arch/x86/hvm/stdvga.c |    7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
> index 6d22b22..f50bff7 100644
> --- a/xen/arch/x86/hvm/stdvga.c
> +++ b/xen/arch/x86/hvm/stdvga.c
> @@ -441,7 +441,7 @@ static int stdvga_mem_write(const struct hvm_io_handler *handler,
>      };
>      struct hvm_ioreq_server *srv;
>  
> -    if ( !s->cache )
> +    if ( !s->cache || !s->stdvga )
>          goto done;
>  
>      /* Intercept mmio write */
> @@ -503,9 +503,6 @@ static bool_t stdvga_mem_accept(const struct hvm_io_handler *handler,
>  
>      spin_lock(&s->lock);
>  
> -    if ( !s->stdvga )
> -        goto reject;
> -
>      if ( p->dir == IOREQ_WRITE && p->count > 1 )
>      {
>          /*
> @@ -526,7 +523,7 @@ static bool_t stdvga_mem_accept(const struct hvm_io_handler *handler,
>  
>          goto reject;
>      }
> -    else if ( p->dir == IOREQ_READ && !s->cache )
> +    else if ( p->dir == IOREQ_READ && (!s->cache || !s->stdvga) )
>          goto reject;
>  
>      /* s->lock intentionally held */

Re: [PATCH (for 4.6)] x86/hvm: Unconditionally buffer writes to VRAM

[Ian Campbell]

On Thu, 2015-07-16 at 10:34 +0100, Andrew Cooper wrote:
> On 16/07/15 10:24, Paul Durrant wrote:
> > When c/s 3bbaaec09 "unify stdvga mmio intercept with standard mmio
> > intercept" was added, a small semantic change was made. Prior to
> > this patch the hypervisor unconditionally sent all guest writes
> > to the VGA aperture as buffered ioreqs, whereas after the patch it
> > only does this when the VGA model is in 'stdvga' mode (sequencer
> > register #7 == 0).
> >
> > When installing Windows 7 (64-bit) using the default QEMU VGA model
> > (== cirrus), Windows leaves 'stdvga' mode early in boot and hence
> > all further writes to the VGA aperture are done using synchronous
> > ioreqs which slows down boot by several orders of magnitude (thanks
> > to the elaborate splash screen that Windows presents). This can be
> > viewed as a regression and so this patch re-instates previous
> > buffering behaviour.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > Tested-by: Wei Liu <wei.liu2@citrix.com>
> > Cc: Keir Fraser <keir@xen.org>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> This is unfortunate,

OOI why is it unfortunate? IOW why wouldn't we want buffer all accesses
to the VRAM (leaving aside that perhaps the original authors only
intended to do it for StdVGA).

Re: [PATCH (for 4.6)] x86/hvm: Unconditionally buffer writes to VRAM

[Paul Durrant]

> -----Original Message-----
> From: Ian Campbell [mailto:ian.campbell@citrix.com]
> Sent: 16 July 2015 11:13
> To: Andrew Cooper
> Cc: Paul Durrant; xen-devel@lists.xenproject.org; Keir (Xen.org); Jan Beulich
> Subject: Re: [Xen-devel] [PATCH (for 4.6)] x86/hvm: Unconditionally buffer
> writes to VRAM
> 
> On Thu, 2015-07-16 at 10:34 +0100, Andrew Cooper wrote:
> > On 16/07/15 10:24, Paul Durrant wrote:
> > > When c/s 3bbaaec09 "unify stdvga mmio intercept with standard mmio
> > > intercept" was added, a small semantic change was made. Prior to
> > > this patch the hypervisor unconditionally sent all guest writes
> > > to the VGA aperture as buffered ioreqs, whereas after the patch it
> > > only does this when the VGA model is in 'stdvga' mode (sequencer
> > > register #7 == 0).
> > >
> > > When installing Windows 7 (64-bit) using the default QEMU VGA model
> > > (== cirrus), Windows leaves 'stdvga' mode early in boot and hence
> > > all further writes to the VGA aperture are done using synchronous
> > > ioreqs which slows down boot by several orders of magnitude (thanks
> > > to the elaborate splash screen that Windows presents). This can be
> > > viewed as a regression and so this patch re-instates previous
> > > buffering behaviour.
> > >
> > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > > Tested-by: Wei Liu <wei.liu2@citrix.com>
> > > Cc: Keir Fraser <keir@xen.org>
> > > Cc: Jan Beulich <jbeulich@suse.com>
> > > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> >
> > This is unfortunate,
> 
> OOI why is it unfortunate? IOW why wouldn't we want buffer all accesses
> to the VRAM (leaving aside that perhaps the original authors only
> intended to do it for StdVGA).

Well, VRAM (mapped through a PCI BAR) can't be buffered in general. For instance, the Cirrus model in QEMU re-maps its I/O ports into the first 256 bytes. I think we are ok to always buffer writes to the VGA aperture though as I can't find any obvious side effects (in QEMU's common code).

  Paul

> 

Re: [PATCH (for 4.6)] x86/hvm: Unconditionally buffer writes to VRAM

[Ian Campbell]

On Thu, 2015-07-16 at 11:20 +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Ian Campbell [mailto:ian.campbell@citrix.com]
> > Sent: 16 July 2015 11:13
> > To: Andrew Cooper
> > Cc: Paul Durrant; xen-devel@lists.xenproject.org; Keir (Xen.org); Jan Beulich
> > Subject: Re: [Xen-devel] [PATCH (for 4.6)] x86/hvm: Unconditionally buffer
> > writes to VRAM
> > 
> > On Thu, 2015-07-16 at 10:34 +0100, Andrew Cooper wrote:
> > > On 16/07/15 10:24, Paul Durrant wrote:
> > > > When c/s 3bbaaec09 "unify stdvga mmio intercept with standard mmio
> > > > intercept" was added, a small semantic change was made. Prior to
> > > > this patch the hypervisor unconditionally sent all guest writes
> > > > to the VGA aperture as buffered ioreqs, whereas after the patch it
> > > > only does this when the VGA model is in 'stdvga' mode (sequencer
> > > > register #7 == 0).
> > > >
> > > > When installing Windows 7 (64-bit) using the default QEMU VGA model
> > > > (== cirrus), Windows leaves 'stdvga' mode early in boot and hence
> > > > all further writes to the VGA aperture are done using synchronous
> > > > ioreqs which slows down boot by several orders of magnitude (thanks
> > > > to the elaborate splash screen that Windows presents). This can be
> > > > viewed as a regression and so this patch re-instates previous
> > > > buffering behaviour.
> > > >
> > > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > > > Tested-by: Wei Liu <wei.liu2@citrix.com>
> > > > Cc: Keir Fraser <keir@xen.org>
> > > > Cc: Jan Beulich <jbeulich@suse.com>
> > > > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > >
> > > This is unfortunate,
> > 
> > OOI why is it unfortunate? IOW why wouldn't we want buffer all accesses
> > to the VRAM (leaving aside that perhaps the original authors only
> > intended to do it for StdVGA).
> 
> Well, VRAM (mapped through a PCI BAR) can't be buffered in general.
> For instance, the Cirrus model in QEMU re-maps its I/O ports into the
> first 256 bytes. I think we are ok to always buffer writes to the VGA
> aperture though as I can't find any obvious side effects (in QEMU's
> common code).

Ah, so the aperture can contain things other than the framebuffer, makes
sense!

> 
>   Paul
> 
> > 
>