[PATCH] virtio-vga: fix virtio-vga bar ordering

[PATCH] virtio-vga: fix virtio-vga bar ordering

[Anthoine Bourgeois]

With virtio-vga, pci bar are reordered. Bar #2 is used for compatibility
with stdvga. By default, bar #2 is used by virtio modern io bar.
This bar is the last one introduce in the virtio pci bar layout and it's
crushed by the virtio-vga reordering. So virtio-vga and
modern-pio-notify are incompatible because virtio-vga failed to
initialize with this option.

This fix exchange the modern io bar with the modern memory bar,
replacing the msix bar that is never impacted anyway.

Signed-off-by: Anthoine Bourgeois <anthoine.bourgeois@gmail.com>
---
 hw/display/virtio-vga.c | 2 +-
 hw/virtio/virtio-pci.c  | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c
index 2b4c2aa126..f5f8737c60 100644
--- a/hw/display/virtio-vga.c
+++ b/hw/display/virtio-vga.c
@@ -113,7 +113,7 @@ static void virtio_vga_base_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
      * the stdvga mmio registers at the start of bar #2.
      */
     vpci_dev->modern_mem_bar_idx = 2;
-    vpci_dev->msix_bar_idx = 4;
+    vpci_dev->modern_io_bar_idx = 4;
 
     if (!(vpci_dev->flags & VIRTIO_PCI_FLAG_PAGE_PER_VQ)) {
         /*
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 4cb784389c..9c5efaa06e 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1705,6 +1705,7 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
      *
      *   region 0   --  virtio legacy io bar
      *   region 1   --  msi-x bar
+     *   region 2   --  virtio modern io bar
      *   region 4+5 --  virtio modern memory (64bit) bar
      *
      */
-- 
2.20.1

Re: [PATCH] virtio-vga: fix virtio-vga bar ordering

[Michael S. Tsirkin]

On Tue, Apr 21, 2020 at 11:48:53PM +0200, Anthoine Bourgeois wrote:
> With virtio-vga, pci bar are reordered. Bar #2 is used for compatibility
> with stdvga. By default, bar #2 is used by virtio modern io bar.
> This bar is the last one introduce in the virtio pci bar layout and it's
> crushed by the virtio-vga reordering. So virtio-vga and
> modern-pio-notify are incompatible because virtio-vga failed to
> initialize with this option.
> 
> This fix exchange the modern io bar with the modern memory bar,
> replacing the msix bar that is never impacted anyway.
> 
> Signed-off-by: Anthoine Bourgeois <anthoine.bourgeois@gmail.com>

Such changes generally need to be tied to machine version.


> ---
>  hw/display/virtio-vga.c | 2 +-
>  hw/virtio/virtio-pci.c  | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c
> index 2b4c2aa126..f5f8737c60 100644
> --- a/hw/display/virtio-vga.c
> +++ b/hw/display/virtio-vga.c
> @@ -113,7 +113,7 @@ static void virtio_vga_base_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
>       * the stdvga mmio registers at the start of bar #2.
>       */
>      vpci_dev->modern_mem_bar_idx = 2;
> -    vpci_dev->msix_bar_idx = 4;
> +    vpci_dev->modern_io_bar_idx = 4;
>  
>      if (!(vpci_dev->flags & VIRTIO_PCI_FLAG_PAGE_PER_VQ)) {
>          /*
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 4cb784389c..9c5efaa06e 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1705,6 +1705,7 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
>       *
>       *   region 0   --  virtio legacy io bar
>       *   region 1   --  msi-x bar
> +     *   region 2   --  virtio modern io bar
>       *   region 4+5 --  virtio modern memory (64bit) bar
>       *
>       */
> -- 
> 2.20.1

Re: [PATCH] virtio-vga: fix virtio-vga bar ordering

[Gerd Hoffmann]

> This fix exchange the modern io bar with the modern memory bar,
> replacing the msix bar that is never impacted anyway.

Well, msix was placed in bar 4 intentionally.  That keeps bar 1 (default
msix location) free, so we have the option to turn bar 0 (vga compat
vram) into a 64bit bar without shuffling around things.

> -    vpci_dev->msix_bar_idx = 4;

Please don't.

> +    vpci_dev->modern_io_bar_idx = 4;

We can use bar 5 instead.

Alternatively just throw an error saying that modern-pio-notify is not
supported.

> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 4cb784389c..9c5efaa06e 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1705,6 +1705,7 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
>       *
>       *   region 0   --  virtio legacy io bar
>       *   region 1   --  msi-x bar
> +     *   region 2   --  virtio modern io bar
>       *   region 4+5 --  virtio modern memory (64bit) bar

Separate patch please.  Also worth noting that the modern io bar is off
by default.

cheers,
  Gerd

Re: [PATCH] virtio-vga: fix virtio-vga bar ordering

[Gerd Hoffmann]

On Wed, Apr 22, 2020 at 02:04:36AM -0400, Michael S. Tsirkin wrote:
> On Tue, Apr 21, 2020 at 11:48:53PM +0200, Anthoine Bourgeois wrote:
> > With virtio-vga, pci bar are reordered. Bar #2 is used for compatibility
> > with stdvga. By default, bar #2 is used by virtio modern io bar.
> > This bar is the last one introduce in the virtio pci bar layout and it's
> > crushed by the virtio-vga reordering. So virtio-vga and
> > modern-pio-notify are incompatible because virtio-vga failed to
> > initialize with this option.
> > 
> > This fix exchange the modern io bar with the modern memory bar,
> > replacing the msix bar that is never impacted anyway.
> > 
> > Signed-off-by: Anthoine Bourgeois <anthoine.bourgeois@gmail.com>
> 
> Such changes generally need to be tied to machine version.

Given that modern-pio-notify is off by default and
virtio-vga,modern-pio-notify=on is broken I think we don't need to worry
about compatibility in this specific case (assuming the patch is updated
to not shuffle around the msix bar, see other reply).

cheers,
  Gerd

Re: [PATCH] virtio-vga: fix virtio-vga bar ordering

[Michael S. Tsirkin]

On Wed, Apr 22, 2020 at 12:49:41PM +0200, Gerd Hoffmann wrote:
> On Wed, Apr 22, 2020 at 02:04:36AM -0400, Michael S. Tsirkin wrote:
> > On Tue, Apr 21, 2020 at 11:48:53PM +0200, Anthoine Bourgeois wrote:
> > > With virtio-vga, pci bar are reordered. Bar #2 is used for compatibility
> > > with stdvga. By default, bar #2 is used by virtio modern io bar.
> > > This bar is the last one introduce in the virtio pci bar layout and it's
> > > crushed by the virtio-vga reordering. So virtio-vga and
> > > modern-pio-notify are incompatible because virtio-vga failed to
> > > initialize with this option.
> > > 
> > > This fix exchange the modern io bar with the modern memory bar,
> > > replacing the msix bar that is never impacted anyway.
> > > 
> > > Signed-off-by: Anthoine Bourgeois <anthoine.bourgeois@gmail.com>
> > 
> > Such changes generally need to be tied to machine version.
> 
> Given that modern-pio-notify is off by default and
> virtio-vga,modern-pio-notify=on is broken I think we don't need to worry
> about compatibility in this specific case (assuming the patch is updated
> to not shuffle around the msix bar, see other reply).
> 
> cheers,
>   Gerd

OK, just worth documenting that this will break cross-version
migration if modern-pio-notify is enabled.

Re: [PATCH] virtio-vga: fix virtio-vga bar ordering

[Anthoine Bourgeois]

On Wed, Apr 22, 2020 at 12:46:57PM +0200, Gerd Hoffmann wrote:
>> This fix exchange the modern io bar with the modern memory bar,
>> replacing the msix bar that is never impacted anyway.
>
>Well, msix was placed in bar 4 intentionally.  That keeps bar 1 (default
>msix location) free, so we have the option to turn bar 0 (vga compat
>vram) into a 64bit bar without shuffling around things.

That's a really good reason I didn't think of.
Just a question, why didn't we choose the virtio-vga order to avoid
shuffling from the beginning? Vga came after and we keep the
compatibility ?

>
>> -    vpci_dev->msix_bar_idx = 4;
>
>Please don't.
>
>> +    vpci_dev->modern_io_bar_idx = 4;
>
>We can use bar 5 instead.
>
>Alternatively just throw an error saying that modern-pio-notify is not
>supported.

As you like. I sent a v2 with modern_io_bar_idx to 5 but I can do a v3
with an error thrown.

>
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index 4cb784389c..9c5efaa06e 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -1705,6 +1705,7 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
>>       *
>>       *   region 0   --  virtio legacy io bar
>>       *   region 1   --  msi-x bar
>> +     *   region 2   --  virtio modern io bar
>>       *   region 4+5 --  virtio modern memory (64bit) bar
>
>Separate patch please.  Also worth noting that the modern io bar is off
>by default.

Done.

Thank you,
Anthoine

Re: [PATCH] virtio-vga: fix virtio-vga bar ordering

[Gerd Hoffmann]

  Hi,

> Just a question, why didn't we choose the virtio-vga order to avoid
> shuffling from the beginning? Vga came after and we keep the
> compatibility ?

Well, transitional virtio devices need bar 0 for legacy virtio
compatibility (io bar), so using bar 1 for msix makes sense in that
case.

virtio-vga is new enough that it supports modern only so it doesn't need
to worry about legacy virtio compatibility.  It does have to worry about
vga compatibility though.  Therefore it uses a bar layout different from
anyone else ...

cheers,
  Gerd