Re: [libvirt] [PATCH v2 4/4] hw/display: Remove "rombar" hack from vga-pci and vmware_vga

On 09/12/19 14:12, Philippe Mathieu-Daudé wrote: > On 12/9/19 1:52 PM, Thomas Huth wrote: >> Now that the old pc-0.x machine types have been removed, we do not need >> the old "rombar" hacks anymore. >> >> Signed-off-by: Thomas Huth <thuth(a)redhat.com&gt; >> --- >>   hw/display/vga-pci.c    | 5 ----- >>   hw/display/vga.c        | 4 +--- >>   hw/display/vmware_vga.c | 5 ----- >>   3 files changed, 1 insertion(+), 13 deletions(-) >> >> diff --git a/hw/display/vga-pci.c b/hw/display/vga-pci.c >> index a27b88122d..cfe095713e 100644 >> --- a/hw/display/vga-pci.c >> +++ b/hw/display/vga-pci.c >> @@ -264,11 +264,6 @@ static void pci_std_vga_realize(PCIDevice *dev, >> Error **errp) >>             pci_register_bar(&d->dev, 2, >> PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio); >>       } >> - >> -    if (!dev->rom_bar) { >> -        /* compatibility with pc-0.13 and older */ >> -        vga_init_vbe(s, OBJECT(dev), pci_address_space(dev)); >> -    } >>   } >>     static void pci_std_vga_init(Object *obj) >> diff --git a/hw/display/vga.c b/hw/display/vga.c >> index 82ebe53610..636586a476 100644 >> --- a/hw/display/vga.c >> +++ b/hw/display/vga.c >> @@ -2304,9 +2304,7 @@ void vga_init(VGACommonState *s, Object *obj, >> MemoryRegion *address_space, >>     void vga_init_vbe(VGACommonState *s, Object *obj, MemoryRegion >> *system_memory) >>   { >> -    /* With pc-0.12 and below we map both the PCI BAR and the fixed >> VBE region, >> -     * so use an alias to avoid double-mapping the same region. >> -     */ >> +    /* Use an alias to avoid double-mapping the same region */ > > If I understand the comment correctly, we can now revert commit > 8294a64d7f9 and and remove the alias, isn't it? Yes, even inline vga_init_vbe and remove vbe_mapped.

----------------- 8< ---------------- From: Paolo Bonzini <pbonzini(a)redhat.com&gt; Subject: [PATCH] vga: cleanup mapping of VRAM for non-PCI VGA vga_init_vbe is now used only from ISA VGA cards. Since the alias is not needed anymore, remove it (effectively reverting commit 8294a64d7f, "vga: fix vram double-mapping with -vga std and -M pc-0.12", 2012-05-29) and the new unused vbe_mapped field of VGACommonState. The function now consists of a single memory_region_add_subregion call, so we can inline it and avoid incorrect usage from PCI cards. Suggested-by: Philippe Mathieu-Daudé <philmd(a)redhat.com&gt; Signed-off-by: Paolo Bonzini <pbonzini(a)redhat.com&gt; diff --git a/hw/display/vga-isa-mm.c b/hw/display/vga-isa-mm.c index e9c43e5530..06f93c4ef5 100644 --- a/hw/display/vga-isa-mm.c +++ b/hw/display/vga-isa-mm.c @@ -106,6 +106,9 @@ int isa_vga_mm_init(hwaddr vram_base, s->vga.con = graphic_console_init(NULL, 0, s->vga.hw_ops, s); - vga_init_vbe(&s->vga, NULL, address_space); + memory_region_add_subregion(system_memory,

+ VBE_DISPI_LFB_PHYSICAL_ADDRESS, + &s->vram); + return 0; } diff --git a/hw/display/vga-isa.c b/hw/display/vga-isa.c index 873e5e9706..5b0b567835 100644 --- a/hw/display/vga-isa.c +++ b/hw/display/vga-isa.c @@ -76,7 +76,9 @@ static void vga_isa_realizefn(DeviceState *dev, Error **errp) memory_region_set_coalescing(vga_io_memory); s->con = graphic_console_init(DEVICE(dev), 0, s->hw_ops, s); - vga_init_vbe(s, OBJECT(dev), isa_address_space(isadev)); + memory_region_add_subregion(system_memory,

+ VBE_DISPI_LFB_PHYSICAL_ADDRESS, + &s->vram); /* ROM BIOS */ rom_add_vga(VGABIOS_FILENAME); } diff --git a/hw/display/vga.c b/hw/display/vga.c index 636586a476..061fd9ab8f 100644 --- a/hw/display/vga.c +++ b/hw/display/vga.c @@ -2301,15 +2301,3 @@ void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space, portio_list_add(&s->vbe_port_list, address_space_io, 0x1ce); } } - -void vga_init_vbe(VGACommonState *s, Object *obj, MemoryRegion *system_memory) -{ - /* Use an alias to avoid double-mapping the same region */ - memory_region_init_alias(&s->vram_vbe, obj, "vram.vbe", - &s->vram, 0, memory_region_size(&s->vram)); - /* XXX: use optimized standard vga accesses */ - memory_region_add_subregion(system_memory, - VBE_DISPI_LFB_PHYSICAL_ADDRESS, - &s->vram_vbe); - s->vbe_mapped = 1; -} diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h index 55c418eab5..3081be445d 100644 --- a/hw/display/vga_int.h +++ b/hw/display/vga_int.h @@ -106,7 +106,6 @@ typedef struct VGACommonState { uint32_t vbe_start_addr; uint32_t vbe_line_offset; uint32_t vbe_bank_mask; - int vbe_mapped;