| Message ID | 20200227210454.18217-1-alistair.francis@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bochs: Remove vga write | expand |
On Thu, Feb 27, 2020 at 01:04:54PM -0800, Alistair Francis wrote: > The QEMU model for the Bochs display has no VGA memory section at offset > 0x400 [1]. By writing to this register Linux can create a write to > unassigned memory which depending on machine and architecture can result > in a store fault. > > I don't see any reference to this address at OSDev [2] or in the Bochs > source code. > > Removing this write still allows graphics to work inside QEMU with > the bochs-display. It's not that simple. The driver also handles the qemu stdvga (-device VGA, -device secondary-vga) which *does* need the vga port write. There is no way for the guest to figure whenever the device is secondary-vga or bochs-display. So how about fixing things on the host side? Does qemu patch below help? Maybe another possible approach is to enable/disable vga access per arch. On x86 this doesn't cause any problems. I guess you are on risc-v? cheers, Gerd diff --git a/hw/display/bochs-display.c b/hw/display/bochs-display.c index 62085f9fc063..e93e838243b8 100644 --- a/hw/display/bochs-display.c +++ b/hw/display/bochs-display.c @@ -151,6 +151,26 @@ static const MemoryRegionOps bochs_display_qext_ops = { .endianness = DEVICE_LITTLE_ENDIAN, }; +static uint64_t dummy_read(void *ptr, hwaddr addr, unsigned size) +{ + return -1; +} + +static void dummy_write(void *ptr, hwaddr addr, + uint64_t val, unsigned size) +{ +} + +static const MemoryRegionOps dummy_ops = { + .read = dummy_read, + .write = dummy_write, + .valid.min_access_size = 1, + .valid.max_access_size = 4, + .impl.min_access_size = 1, + .impl.max_access_size = 1, + .endianness = DEVICE_LITTLE_ENDIAN, +}; + static int bochs_display_get_mode(BochsDisplayState *s, BochsDisplayMode *mode) { @@ -284,8 +304,8 @@ static void bochs_display_realize(PCIDevice *dev, Error **errp) memory_region_init_io(&s->qext, obj, &bochs_display_qext_ops, s, "qemu extended regs", PCI_VGA_QEXT_SIZE); - memory_region_init(&s->mmio, obj, "bochs-display-mmio", - PCI_VGA_MMIO_SIZE); + memory_region_init_io(&s->mmio, obj, &dummy_ops, NULL, + "bochs-display-mmio", PCI_VGA_MMIO_SIZE); memory_region_add_subregion(&s->mmio, PCI_VGA_BOCHS_OFFSET, &s->vbe); memory_region_add_subregion(&s->mmio, PCI_VGA_QEXT_OFFSET, &s->qext);
On Fri, Feb 28, 2020 at 1:57 AM Gerd Hoffmann <kraxel@redhat.com> wrote: > > On Thu, Feb 27, 2020 at 01:04:54PM -0800, Alistair Francis wrote: > > The QEMU model for the Bochs display has no VGA memory section at offset > > 0x400 [1]. By writing to this register Linux can create a write to > > unassigned memory which depending on machine and architecture can result > > in a store fault. > > > > I don't see any reference to this address at OSDev [2] or in the Bochs > > source code. > > > > Removing this write still allows graphics to work inside QEMU with > > the bochs-display. > > It's not that simple. The driver also handles the qemu stdvga (-device > VGA, -device secondary-vga) which *does* need the vga port write. > There is no way for the guest to figure whenever the device is > secondary-vga or bochs-display. > > So how about fixing things on the host side? Does qemu patch below > help? That patch looks like it will fix the problem, but it doesn't seem like the correct fix. I would rather avoid adding a large chunk of dummy I/O to handle the two devices. > > Maybe another possible approach is to enable/disable vga access per > arch. On x86 this doesn't cause any problems. I guess you are on > risc-v? I would prefer this option. I do see this on RISC-V, but I suspect the issue will appear on other architectures (although how they handle I/O failures in QEMU is a different story). Can I just do the VGA write if x86? Alistair > > cheers, > Gerd > > diff --git a/hw/display/bochs-display.c b/hw/display/bochs-display.c > index 62085f9fc063..e93e838243b8 100644 > --- a/hw/display/bochs-display.c > +++ b/hw/display/bochs-display.c > @@ -151,6 +151,26 @@ static const MemoryRegionOps bochs_display_qext_ops = { > .endianness = DEVICE_LITTLE_ENDIAN, > }; > > +static uint64_t dummy_read(void *ptr, hwaddr addr, unsigned size) > +{ > + return -1; > +} > + > +static void dummy_write(void *ptr, hwaddr addr, > + uint64_t val, unsigned size) > +{ > +} > + > +static const MemoryRegionOps dummy_ops = { > + .read = dummy_read, > + .write = dummy_write, > + .valid.min_access_size = 1, > + .valid.max_access_size = 4, > + .impl.min_access_size = 1, > + .impl.max_access_size = 1, > + .endianness = DEVICE_LITTLE_ENDIAN, > +}; > + > static int bochs_display_get_mode(BochsDisplayState *s, > BochsDisplayMode *mode) > { > @@ -284,8 +304,8 @@ static void bochs_display_realize(PCIDevice *dev, Error **errp) > memory_region_init_io(&s->qext, obj, &bochs_display_qext_ops, s, > "qemu extended regs", PCI_VGA_QEXT_SIZE); > > - memory_region_init(&s->mmio, obj, "bochs-display-mmio", > - PCI_VGA_MMIO_SIZE); > + memory_region_init_io(&s->mmio, obj, &dummy_ops, NULL, > + "bochs-display-mmio", PCI_VGA_MMIO_SIZE); > memory_region_add_subregion(&s->mmio, PCI_VGA_BOCHS_OFFSET, &s->vbe); > memory_region_add_subregion(&s->mmio, PCI_VGA_QEXT_OFFSET, &s->qext); > >
On Mon, Mar 02, 2020 at 02:14:02PM -0800, Alistair Francis wrote: > On Fri, Feb 28, 2020 at 1:57 AM Gerd Hoffmann <kraxel@redhat.com> wrote: > > > > On Thu, Feb 27, 2020 at 01:04:54PM -0800, Alistair Francis wrote: > > > The QEMU model for the Bochs display has no VGA memory section at offset > > > 0x400 [1]. By writing to this register Linux can create a write to > > > unassigned memory which depending on machine and architecture can result > > > in a store fault. > > > > > > I don't see any reference to this address at OSDev [2] or in the Bochs > > > source code. > > > > > > Removing this write still allows graphics to work inside QEMU with > > > the bochs-display. > > > > It's not that simple. The driver also handles the qemu stdvga (-device > > VGA, -device secondary-vga) which *does* need the vga port write. > > There is no way for the guest to figure whenever the device is > > secondary-vga or bochs-display. > > > > So how about fixing things on the host side? Does qemu patch below > > help? > > That patch looks like it will fix the problem, but it doesn't seem > like the correct fix. I would rather avoid adding a large chunk of > dummy I/O to handle the two devices. It's just a single handler for the parent mmio region, so we have a defined default action instead of undefined behavior. Patch just posted to qemu-devel, lets see what others think ... > > Maybe another possible approach is to enable/disable vga access per > > arch. On x86 this doesn't cause any problems. I guess you are on > > risc-v? > > I would prefer this option. I do see this on RISC-V, but I suspect the > issue will appear on other architectures (although how they handle I/O > failures in QEMU is a different story). > > Can I just do the VGA write if x86? I know ppc needs it too. Not sure about other architectures. I'd suggest to do it the other way around: blacklist known-problematic archs. cheers, Gerd
On Mon, Mar 2, 2020 at 10:24 PM Gerd Hoffmann <kraxel@redhat.com> wrote: > > On Mon, Mar 02, 2020 at 02:14:02PM -0800, Alistair Francis wrote: > > On Fri, Feb 28, 2020 at 1:57 AM Gerd Hoffmann <kraxel@redhat.com> wrote: > > > > > > On Thu, Feb 27, 2020 at 01:04:54PM -0800, Alistair Francis wrote: > > > > The QEMU model for the Bochs display has no VGA memory section at offset > > > > 0x400 [1]. By writing to this register Linux can create a write to > > > > unassigned memory which depending on machine and architecture can result > > > > in a store fault. > > > > > > > > I don't see any reference to this address at OSDev [2] or in the Bochs > > > > source code. > > > > > > > > Removing this write still allows graphics to work inside QEMU with > > > > the bochs-display. > > > > > > It's not that simple. The driver also handles the qemu stdvga (-device > > > VGA, -device secondary-vga) which *does* need the vga port write. > > > There is no way for the guest to figure whenever the device is > > > secondary-vga or bochs-display. > > > > > > So how about fixing things on the host side? Does qemu patch below > > > help? > > > > That patch looks like it will fix the problem, but it doesn't seem > > like the correct fix. I would rather avoid adding a large chunk of > > dummy I/O to handle the two devices. > > It's just a single handler for the parent mmio region, so we have a > defined default action instead of undefined behavior. > > Patch just posted to qemu-devel, lets see what others think ... Thanks, let's wait and see what happens. > > > > Maybe another possible approach is to enable/disable vga access per > > > arch. On x86 this doesn't cause any problems. I guess you are on > > > risc-v? > > > > I would prefer this option. I do see this on RISC-V, but I suspect the > > issue will appear on other architectures (although how they handle I/O > > failures in QEMU is a different story). > > > > Can I just do the VGA write if x86? > > I know ppc needs it too. Not sure about other architectures. I'd > suggest to do it the other way around: blacklist known-problematic > archs. Argh, that is a little uglier. Let's circle back after receiving feedback on the QEMU patch. Alistair > > cheers, > Gerd >
diff --git a/drivers/gpu/drm/bochs/bochs_hw.c b/drivers/gpu/drm/bochs/bochs_hw.c index b615b7dfdd9d..dfb2a5363c62 100644 --- a/drivers/gpu/drm/bochs/bochs_hw.c +++ b/drivers/gpu/drm/bochs/bochs_hw.c @@ -10,19 +10,6 @@ /* ---------------------------------------------------------------------- */ -static void bochs_vga_writeb(struct bochs_device *bochs, u16 ioport, u8 val) -{ - if (WARN_ON(ioport < 0x3c0 || ioport > 0x3df)) - return; - - if (bochs->mmio) { - int offset = ioport - 0x3c0 + 0x400; - writeb(val, bochs->mmio + offset); - } else { - outb(val, ioport); - } -} - static u16 bochs_dispi_read(struct bochs_device *bochs, u16 reg) { u16 ret = 0; @@ -217,8 +204,6 @@ void bochs_hw_setmode(struct bochs_device *bochs, bochs->xres, bochs->yres, bochs->bpp, bochs->yres_virtual); - bochs_vga_writeb(bochs, 0x3c0, 0x20); /* unblank */ - bochs_dispi_write(bochs, VBE_DISPI_INDEX_ENABLE, 0); bochs_dispi_write(bochs, VBE_DISPI_INDEX_BPP, bochs->bpp); bochs_dispi_write(bochs, VBE_DISPI_INDEX_XRES, bochs->xres);
The QEMU model for the Bochs display has no VGA memory section at offset 0x400 [1]. By writing to this register Linux can create a write to unassigned memory which depending on machine and architecture can result in a store fault. I don't see any reference to this address at OSDev [2] or in the Bochs source code. Removing this write still allows graphics to work inside QEMU with the bochs-display. 1: https://gitlab.com/qemu-project/qemu/-/blob/master/hw/display/bochs-display.c#L264 2. https://wiki.osdev.org/Bochs_VBE_Extensions Reported-by: Khem Raj <raj.khem@gmail.com> Signed-off-by: Alistair Francis <alistair.francis@wdc.com> --- drivers/gpu/drm/bochs/bochs_hw.c | 15 --------------- 1 file changed, 15 deletions(-)