[PATCH-for-6.2 0/2] hw/display: Do not allow multiple (identical) VGA devices

Philippe Mathieu-Daudé posted 2 patches 2 years, 5 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20211118192020.61245-1-philmd@redhat.com
hw/display/vga_int.h        |  2 +-
hw/display/ati.c            |  4 +++-
hw/display/cirrus_vga.c     |  4 +++-
hw/display/cirrus_vga_isa.c |  4 +++-
hw/display/qxl.c            |  4 +++-
hw/display/vga-isa-mm.c     |  3 ++-
hw/display/vga-isa.c        | 11 ++---------
hw/display/vga-pci.c        |  8 ++++++--
hw/display/vga.c            | 17 ++++++++++++++++-
hw/display/virtio-vga.c     |  4 +++-
hw/display/vmware_vga.c     |  2 +-
11 files changed, 43 insertions(+), 20 deletions(-)
[PATCH-for-6.2 0/2] hw/display: Do not allow multiple (identical) VGA devices
Posted by Philippe Mathieu-Daudé 2 years, 5 months ago
Commit 7852a77f598 fixed creating multiple TYPE_ISA_VGA devices,
generalize the fix to all VGA devices.

See https://gitlab.com/qemu-project/qemu/-/issues/44

Philippe Mathieu-Daudé (2):
  hw/display: Add Error* handle to vga_common_init()
  hw/display: Do not allow multiple identical VGA devices

 hw/display/vga_int.h        |  2 +-
 hw/display/ati.c            |  4 +++-
 hw/display/cirrus_vga.c     |  4 +++-
 hw/display/cirrus_vga_isa.c |  4 +++-
 hw/display/qxl.c            |  4 +++-
 hw/display/vga-isa-mm.c     |  3 ++-
 hw/display/vga-isa.c        | 11 ++---------
 hw/display/vga-pci.c        |  8 ++++++--
 hw/display/vga.c            | 17 ++++++++++++++++-
 hw/display/virtio-vga.c     |  4 +++-
 hw/display/vmware_vga.c     |  2 +-
 11 files changed, 43 insertions(+), 20 deletions(-)

-- 
2.31.1


Re: [PATCH-for-6.2 0/2] hw/display: Do not allow multiple (identical) VGA devices
Posted by Mark Cave-Ayland 2 years, 5 months ago
On 18/11/2021 19:20, Philippe Mathieu-Daudé wrote:

> Commit 7852a77f598 fixed creating multiple TYPE_ISA_VGA devices,
> generalize the fix to all VGA devices.
> 
> See https://gitlab.com/qemu-project/qemu/-/issues/44
> 
> Philippe Mathieu-Daudé (2):
>    hw/display: Add Error* handle to vga_common_init()
>    hw/display: Do not allow multiple identical VGA devices
> 
>   hw/display/vga_int.h        |  2 +-
>   hw/display/ati.c            |  4 +++-
>   hw/display/cirrus_vga.c     |  4 +++-
>   hw/display/cirrus_vga_isa.c |  4 +++-
>   hw/display/qxl.c            |  4 +++-
>   hw/display/vga-isa-mm.c     |  3 ++-
>   hw/display/vga-isa.c        | 11 ++---------
>   hw/display/vga-pci.c        |  8 ++++++--
>   hw/display/vga.c            | 17 ++++++++++++++++-
>   hw/display/virtio-vga.c     |  4 +++-
>   hw/display/vmware_vga.c     |  2 +-
>   11 files changed, 43 insertions(+), 20 deletions(-)

Hi Phil,

I don't think this is correct for non-ISA devices: for example years ago I had a PC 
running Windows 98SE with 2 identical PCI graphics cards configured in dual-head mode.

IIRC the BIOS would bring up the first graphics card and configure it to use the 
legacy ISA VGA ioports for compatibility, and then once the main OS drivers loaded 
both cards were switched to PCI mode and configured using the BARs as normal.


ATB,

Mark.

Re: [PATCH-for-6.2 0/2] hw/display: Do not allow multiple (identical) VGA devices
Posted by Philippe Mathieu-Daudé 2 years, 5 months ago
On 11/19/21 09:21, Mark Cave-Ayland wrote:
> On 18/11/2021 19:20, Philippe Mathieu-Daudé wrote:
> 
>> Commit 7852a77f598 fixed creating multiple TYPE_ISA_VGA devices,
>> generalize the fix to all VGA devices.
>>
>> See https://gitlab.com/qemu-project/qemu/-/issues/44
>>
>> Philippe Mathieu-Daudé (2):
>>    hw/display: Add Error* handle to vga_common_init()
>>    hw/display: Do not allow multiple identical VGA devices
>>
>>   hw/display/vga_int.h        |  2 +-
>>   hw/display/ati.c            |  4 +++-
>>   hw/display/cirrus_vga.c     |  4 +++-
>>   hw/display/cirrus_vga_isa.c |  4 +++-
>>   hw/display/qxl.c            |  4 +++-
>>   hw/display/vga-isa-mm.c     |  3 ++-
>>   hw/display/vga-isa.c        | 11 ++---------
>>   hw/display/vga-pci.c        |  8 ++++++--
>>   hw/display/vga.c            | 17 ++++++++++++++++-
>>   hw/display/virtio-vga.c     |  4 +++-
>>   hw/display/vmware_vga.c     |  2 +-
>>   11 files changed, 43 insertions(+), 20 deletions(-)
> 
> Hi Phil,
> 
> I don't think this is correct for non-ISA devices: for example years ago
> I had a PC running Windows 98SE with 2 identical PCI graphics cards
> configured in dual-head mode.
> 
> IIRC the BIOS would bring up the first graphics card and configure it to
> use the legacy ISA VGA ioports for compatibility, and then once the main
> OS drivers loaded both cards were switched to PCI mode and configured
> using the BARs as normal.

The problem here is QEMU technical debt, not the hardware.

When vga_common_init() calls memory_region_init_ram_nomigrate()
with obj=NULL, "vga.vram" is registered as a QOM singleton.

Updating it would 1/ require non-QOM devices to be QOM'ified
and 2/ break migration unless using HPFM which I don't master.


Re: [PATCH-for-6.2 0/2] hw/display: Do not allow multiple (identical) VGA devices
Posted by Thomas Huth 2 years, 5 months ago
On 19/11/2021 10.49, Philippe Mathieu-Daudé wrote:
> On 11/19/21 09:21, Mark Cave-Ayland wrote:
>> On 18/11/2021 19:20, Philippe Mathieu-Daudé wrote:
>>
>>> Commit 7852a77f598 fixed creating multiple TYPE_ISA_VGA devices,
>>> generalize the fix to all VGA devices.
>>>
>>> See https://gitlab.com/qemu-project/qemu/-/issues/44
>>>
>>> Philippe Mathieu-Daudé (2):
>>>     hw/display: Add Error* handle to vga_common_init()
>>>     hw/display: Do not allow multiple identical VGA devices
>>>
>>>    hw/display/vga_int.h        |  2 +-
>>>    hw/display/ati.c            |  4 +++-
>>>    hw/display/cirrus_vga.c     |  4 +++-
>>>    hw/display/cirrus_vga_isa.c |  4 +++-
>>>    hw/display/qxl.c            |  4 +++-
>>>    hw/display/vga-isa-mm.c     |  3 ++-
>>>    hw/display/vga-isa.c        | 11 ++---------
>>>    hw/display/vga-pci.c        |  8 ++++++--
>>>    hw/display/vga.c            | 17 ++++++++++++++++-
>>>    hw/display/virtio-vga.c     |  4 +++-
>>>    hw/display/vmware_vga.c     |  2 +-
>>>    11 files changed, 43 insertions(+), 20 deletions(-)
>>
>> Hi Phil,
>>
>> I don't think this is correct for non-ISA devices: for example years ago
>> I had a PC running Windows 98SE with 2 identical PCI graphics cards
>> configured in dual-head mode.
>>
>> IIRC the BIOS would bring up the first graphics card and configure it to
>> use the legacy ISA VGA ioports for compatibility, and then once the main
>> OS drivers loaded both cards were switched to PCI mode and configured
>> using the BARs as normal.
> 
> The problem here is QEMU technical debt, not the hardware.
> 
> When vga_common_init() calls memory_region_init_ram_nomigrate()
> with obj=NULL, "vga.vram" is registered as a QOM singleton.
> 
> Updating it would
 > 1/ require non-QOM devices to be QOM'ified

So sounds like that's the right way to go here.

> and 2/ break migration unless using HPFM which I don't master.

What's HPFM?

  Thomas


Re: [PATCH-for-6.2 0/2] hw/display: Do not allow multiple (identical) VGA devices
Posted by Philippe Mathieu-Daudé 2 years, 5 months ago
On 11/19/21 10:58, Thomas Huth wrote:
> On 19/11/2021 10.49, Philippe Mathieu-Daudé wrote:
>> On 11/19/21 09:21, Mark Cave-Ayland wrote:
>>> On 18/11/2021 19:20, Philippe Mathieu-Daudé wrote:
>>>
>>>> Commit 7852a77f598 fixed creating multiple TYPE_ISA_VGA devices,
>>>> generalize the fix to all VGA devices.
>>>>
>>>> See https://gitlab.com/qemu-project/qemu/-/issues/44
>>>>
>>>> Philippe Mathieu-Daudé (2):
>>>>     hw/display: Add Error* handle to vga_common_init()
>>>>     hw/display: Do not allow multiple identical VGA devices
>>>>
>>>>    hw/display/vga_int.h        |  2 +-
>>>>    hw/display/ati.c            |  4 +++-
>>>>    hw/display/cirrus_vga.c     |  4 +++-
>>>>    hw/display/cirrus_vga_isa.c |  4 +++-
>>>>    hw/display/qxl.c            |  4 +++-
>>>>    hw/display/vga-isa-mm.c     |  3 ++-
>>>>    hw/display/vga-isa.c        | 11 ++---------
>>>>    hw/display/vga-pci.c        |  8 ++++++--
>>>>    hw/display/vga.c            | 17 ++++++++++++++++-
>>>>    hw/display/virtio-vga.c     |  4 +++-
>>>>    hw/display/vmware_vga.c     |  2 +-
>>>>    11 files changed, 43 insertions(+), 20 deletions(-)
>>>
>>> Hi Phil,
>>>
>>> I don't think this is correct for non-ISA devices: for example years ago
>>> I had a PC running Windows 98SE with 2 identical PCI graphics cards
>>> configured in dual-head mode.
>>>
>>> IIRC the BIOS would bring up the first graphics card and configure it to
>>> use the legacy ISA VGA ioports for compatibility, and then once the main
>>> OS drivers loaded both cards were switched to PCI mode and configured
>>> using the BARs as normal.
>>
>> The problem here is QEMU technical debt, not the hardware.
>>
>> When vga_common_init() calls memory_region_init_ram_nomigrate()
>> with obj=NULL, "vga.vram" is registered as a QOM singleton.
>>
>> Updating it would
>> 1/ require non-QOM devices to be QOM'ified
> 
> So sounds like that's the right way to go here.
> 
>> and 2/ break migration unless using HPFM which I don't master.
> 
> What's HPFM?

Hocus Pocus Freakin' Magic


Re: [PATCH-for-6.2 0/2] hw/display: Do not allow multiple (identical) VGA devices
Posted by Thomas Huth 2 years, 5 months ago
On 19/11/2021 11.17, Philippe Mathieu-Daudé wrote:
> On 11/19/21 10:58, Thomas Huth wrote:
>> On 19/11/2021 10.49, Philippe Mathieu-Daudé wrote:
>>> On 11/19/21 09:21, Mark Cave-Ayland wrote:
>>>> On 18/11/2021 19:20, Philippe Mathieu-Daudé wrote:
>>>>
>>>>> Commit 7852a77f598 fixed creating multiple TYPE_ISA_VGA devices,
>>>>> generalize the fix to all VGA devices.
>>>>>
>>>>> See https://gitlab.com/qemu-project/qemu/-/issues/44
>>>>>
>>>>> Philippe Mathieu-Daudé (2):
>>>>>      hw/display: Add Error* handle to vga_common_init()
>>>>>      hw/display: Do not allow multiple identical VGA devices
>>>>>
>>>>>     hw/display/vga_int.h        |  2 +-
>>>>>     hw/display/ati.c            |  4 +++-
>>>>>     hw/display/cirrus_vga.c     |  4 +++-
>>>>>     hw/display/cirrus_vga_isa.c |  4 +++-
>>>>>     hw/display/qxl.c            |  4 +++-
>>>>>     hw/display/vga-isa-mm.c     |  3 ++-
>>>>>     hw/display/vga-isa.c        | 11 ++---------
>>>>>     hw/display/vga-pci.c        |  8 ++++++--
>>>>>     hw/display/vga.c            | 17 ++++++++++++++++-
>>>>>     hw/display/virtio-vga.c     |  4 +++-
>>>>>     hw/display/vmware_vga.c     |  2 +-
>>>>>     11 files changed, 43 insertions(+), 20 deletions(-)
>>>>
>>>> Hi Phil,
>>>>
>>>> I don't think this is correct for non-ISA devices: for example years ago
>>>> I had a PC running Windows 98SE with 2 identical PCI graphics cards
>>>> configured in dual-head mode.
>>>>
>>>> IIRC the BIOS would bring up the first graphics card and configure it to
>>>> use the legacy ISA VGA ioports for compatibility, and then once the main
>>>> OS drivers loaded both cards were switched to PCI mode and configured
>>>> using the BARs as normal.
>>>
>>> The problem here is QEMU technical debt, not the hardware.
>>>
>>> When vga_common_init() calls memory_region_init_ram_nomigrate()
>>> with obj=NULL, "vga.vram" is registered as a QOM singleton.
>>>
>>> Updating it would
>>> 1/ require non-QOM devices to be QOM'ified
>>
>> So sounds like that's the right way to go here.
>>
>>> and 2/ break migration unless using HPFM which I don't master.
>>
>> What's HPFM?
> 
> Hocus Pocus Freakin' Magic

LOL, ok, thanks, TIL.

Anyway, IMHO I'd rather fix this issue by properly QOM'ifying the 
vga-isa-mm.c code and breaking migration here (who's migrating such old ISA 
devices anyway?) instead of introducing more kludges
that might cause other trouble (see also 
https://gitlab.com/qemu-project/qemu/-/issues/733 ).

  Thomas