| Message ID | 20200421214853.14412-1-anthoine.bourgeois@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio-vga: fix virtio-vga bar ordering | expand |
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
> 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
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
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.
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
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
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 * */
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(-)