Re: [Qemu-devel] [PATCH 2/6] vga: increase priority of 0xa0000 memory region

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4374 bytes --]

On Tue, Jan 03, 2017 at 11:37:20PM +0100, Hervé Poussineau wrote:
> Le 03/01/2017 à 00:02, David Gibson a écrit :
> > On Thu, Dec 29, 2016 at 11:12:12PM +0100, Hervé Poussineau wrote:
> > > VGA device registers vram as BAR 0. If this BAR is activated as a very low address which
> > > crosses 0xa0000-0xbffff, low memory region is not accessible anymore.
> > > 
> > > This fixes display on PReP machine if we enable PCI mapping at
> > > address 0.
> > 
> > This commit message needs more information.  What exactly is the VGA
> > BAR colliding with?  Why does the other thing have higher priority?
> > Why is it safe for the VGA BAR to override the other thing?  Why is
> > this safe on all platforms?
> 
> VGA has (basically) two memory regions:
> - the legacy one, from 0xa0000 to 0xbffff
> - a memory region describing the whole VRAM, configurable with PCI BAR 0.

Yes, that I know.  Note that both the fixed 0xa0000 and the BAR value
are offsets with *PCI* memory space, which may not be the same as
addresses in system memory space.  PCI memory space is identity mapped
into system address space on x86, IIRC, but it's not on spapr.  AFAICT
it's not identity mapped on PReP either (see raven_pcihost_initfn()).
So the VGA legacy window will actually be at 0xc00a0000 and BAR0 at
(0xc0000000 + BAR value) in the _system_ address space if I'm reading
the PReP PCI bridge code correctly.

> In QEMU, mapping PCI at address 0 is not permitted (MachineClass->pci_allow_0_address is false by default),

I believe this is because of the identity map of PCI memory space on
x86 (well, PC, strictly speaking).  Because that's not the case on
PReP, it should probably set allow_0_address = true.

> except on arm/virt and ppc/spapr. So, this is usually not a problem as all PCI BARs (including video PCI BAR 0) are not mapped at
> address 0, and so both memory regions can't collide.
> 
> When trying Linux on ppc/40p (introduced later in this patchset), I saw that Linux assigns
> PCI BAR addresses in the order of PCI devices detection, and VGA PCI BAR 0 ends up at address 0.

Hmm.  You're saying that Linux is assigning BAR 0 so that it collides
with the legacy memory region?  That sounds like a Linux bug.
Or.. does Linux always assign BARs on PReP, or only if the firmware
doesn't?  Maybe the guest firmware needs to assign the BARs properly
to avoid this collision.

> However, Linux assumes that 0xa0000-0xbffff is still available to drive the VGA card in text mode.
> Without changing the region priority, VGA output is garbled (as writes directly go to VGA memory).
> When increasing the region priority, VGA output is restored (as writes go to legacy address space).

Right.  But by increasing the priority, you're effectively making a
hole in BAR0.  That sounds like it will cause chaos if after boot you
load a "real" VGA driver which tries to use the full video RAM through
BAR0 instead of via the legacy area.

> arm/virt by default doesn't have a PCI VGA card.
> ppc/spapr may probably have the same problem as ppc/40p. However, as VGA PCI BAR 0 and VGA legacy space
> both have a priority of 1, it may probably have a problem. I only need to convince Linux to use address 0 for VGA PCI BAR.
> 
> Hervé
> 
> > 
> > > Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> > > ---
> > >  hw/display/vga.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/display/vga.c b/hw/display/vga.c
> > > index 2a88b3c..c573f35 100644
> > > --- a/hw/display/vga.c
> > > +++ b/hw/display/vga.c
> > > @@ -2265,7 +2265,7 @@ void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space,
> > >      memory_region_add_subregion_overlap(address_space,
> > >                                          0x000a0000,
> > >                                          vga_io_memory,
> > > -                                        1);
> > > +                                        2);
> > >      memory_region_set_coalescing(vga_io_memory);
> > >      if (init_vga_ports) {
> > >          portio_list_init(&s->vga_port_list, obj, vga_ports, s, "vga");
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]