[v3] ramfb: Add property to control if load the romfile

[PATCH v3 2/2] hw/arm: Add the romfile compatatibility

Posted by Shaoqin Huang 5 months, 1 week ago

On arm64, it doesn't use the vgabios-ramfb.bin, so set the property
"use-legacy-x86-rom" to false, thus the ramfb won't load the
vgabios-ramfb.bin.

This can mitigate the problem that on release version the qemu can't
find the vgabios-ramfb.bin if it use the ramfb.

Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
---
 hw/arm/virt.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index a96452f17a..5f94f7a2ca 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -38,6 +38,7 @@
 #include "hw/arm/primecell.h"
 #include "hw/arm/virt.h"
 #include "hw/block/flash.h"
+#include "hw/vfio/pci.h"
 #include "hw/vfio/vfio-calxeda-xgmac.h"
 #include "hw/vfio/vfio-amd-xgbe.h"
 #include "hw/display/ramfb.h"
@@ -90,6 +91,8 @@
 
 static GlobalProperty arm_virt_compat[] = {
     { TYPE_VIRTIO_IOMMU_PCI, "aw-bits", "48" },
+    { TYPE_RAMFB_DEVICE, "use-legacy-x86-rom", "false" },
+    { TYPE_VFIO_PCI, "use-legacy-x86-rom", "false" },
 };
 static const size_t arm_virt_compat_len = G_N_ELEMENTS(arm_virt_compat);
 
-- 
2.40.1

Re: [PATCH v3 2/2] hw/arm: Add the romfile compatatibility

Posted by Peter Maydell 5 months, 1 week ago

On Mon, 9 Jun 2025 at 08:34, Shaoqin Huang <shahuang@redhat.com> wrote:
>
> On arm64, it doesn't use the vgabios-ramfb.bin, so set the property
> "use-legacy-x86-rom" to false, thus the ramfb won't load the
> vgabios-ramfb.bin.
>
> This can mitigate the problem that on release version the qemu can't
> find the vgabios-ramfb.bin if it use the ramfb.
>
> Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
> ---
>  hw/arm/virt.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index a96452f17a..5f94f7a2ca 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -38,6 +38,7 @@
>  #include "hw/arm/primecell.h"
>  #include "hw/arm/virt.h"
>  #include "hw/block/flash.h"
> +#include "hw/vfio/pci.h"
>  #include "hw/vfio/vfio-calxeda-xgmac.h"
>  #include "hw/vfio/vfio-amd-xgbe.h"
>  #include "hw/display/ramfb.h"
> @@ -90,6 +91,8 @@
>
>  static GlobalProperty arm_virt_compat[] = {
>      { TYPE_VIRTIO_IOMMU_PCI, "aw-bits", "48" },
> +    { TYPE_RAMFB_DEVICE, "use-legacy-x86-rom", "false" },
> +    { TYPE_VFIO_PCI, "use-legacy-x86-rom", "false" },

I think we should find a way to make this default to "false"
and only be set "true" for x86. Otherwise every single non-x86
board that ever adds support for ramfb and virtio will have
to add these two lines, which is a source of future bugs.
(Whereas if you forget to mark a new x86 board as needing
the legacy rom you'll find out about it pretty quickly.)

thanks
-- PMM

Re: [PATCH v3 2/2] hw/arm: Add the romfile compatatibility

Posted by Daniel P. Berrangé 5 months, 1 week ago

On Mon, Jun 09, 2025 at 09:48:36AM +0100, Peter Maydell wrote:
> On Mon, 9 Jun 2025 at 08:34, Shaoqin Huang <shahuang@redhat.com> wrote:
> >
> > On arm64, it doesn't use the vgabios-ramfb.bin, so set the property
> > "use-legacy-x86-rom" to false, thus the ramfb won't load the
> > vgabios-ramfb.bin.
> >
> > This can mitigate the problem that on release version the qemu can't
> > find the vgabios-ramfb.bin if it use the ramfb.
> >
> > Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
> > ---
> >  hw/arm/virt.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index a96452f17a..5f94f7a2ca 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -38,6 +38,7 @@
> >  #include "hw/arm/primecell.h"
> >  #include "hw/arm/virt.h"
> >  #include "hw/block/flash.h"
> > +#include "hw/vfio/pci.h"
> >  #include "hw/vfio/vfio-calxeda-xgmac.h"
> >  #include "hw/vfio/vfio-amd-xgbe.h"
> >  #include "hw/display/ramfb.h"
> > @@ -90,6 +91,8 @@
> >
> >  static GlobalProperty arm_virt_compat[] = {
> >      { TYPE_VIRTIO_IOMMU_PCI, "aw-bits", "48" },
> > +    { TYPE_RAMFB_DEVICE, "use-legacy-x86-rom", "false" },
> > +    { TYPE_VFIO_PCI, "use-legacy-x86-rom", "false" },
> 
> I think we should find a way to make this default to "false"
> and only be set "true" for x86. Otherwise every single non-x86
> board that ever adds support for ramfb and virtio will have
> to add these two lines, which is a source of future bugs.
> (Whereas if you forget to mark a new x86 board as needing
> the legacy rom you'll find out about it pretty quickly.)

Yes, going forward we this to default to true only on x86.

For non-x86, historical versioned machine types will need
likely it set to true, in order to avoid the memory layout
being changed IIUC.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 2/2] hw/arm: Add the romfile compatatibility

Posted by Shaoqin Huang 5 months ago


On 6/9/25 5:10 PM, Daniel P. Berrangé wrote:
> On Mon, Jun 09, 2025 at 09:48:36AM +0100, Peter Maydell wrote:
>> On Mon, 9 Jun 2025 at 08:34, Shaoqin Huang <shahuang@redhat.com> wrote:
>>>
>>> On arm64, it doesn't use the vgabios-ramfb.bin, so set the property
>>> "use-legacy-x86-rom" to false, thus the ramfb won't load the
>>> vgabios-ramfb.bin.
>>>
>>> This can mitigate the problem that on release version the qemu can't
>>> find the vgabios-ramfb.bin if it use the ramfb.
>>>
>>> Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
>>> ---
>>>   hw/arm/virt.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>> index a96452f17a..5f94f7a2ca 100644
>>> --- a/hw/arm/virt.c
>>> +++ b/hw/arm/virt.c
>>> @@ -38,6 +38,7 @@
>>>   #include "hw/arm/primecell.h"
>>>   #include "hw/arm/virt.h"
>>>   #include "hw/block/flash.h"
>>> +#include "hw/vfio/pci.h"
>>>   #include "hw/vfio/vfio-calxeda-xgmac.h"
>>>   #include "hw/vfio/vfio-amd-xgbe.h"
>>>   #include "hw/display/ramfb.h"
>>> @@ -90,6 +91,8 @@
>>>
>>>   static GlobalProperty arm_virt_compat[] = {
>>>       { TYPE_VIRTIO_IOMMU_PCI, "aw-bits", "48" },
>>> +    { TYPE_RAMFB_DEVICE, "use-legacy-x86-rom", "false" },
>>> +    { TYPE_VFIO_PCI, "use-legacy-x86-rom", "false" },
>>
>> I think we should find a way to make this default to "false"
>> and only be set "true" for x86. Otherwise every single non-x86
>> board that ever adds support for ramfb and virtio will have
>> to add these two lines, which is a source of future bugs.
>> (Whereas if you forget to mark a new x86 board as needing
>> the legacy rom you'll find out about it pretty quickly.)
> 
> Yes, going forward we this to default to true only on x86.
> 
> For non-x86, historical versioned machine types will need
> likely it set to true, in order to avoid the memory layout
> being changed IIUC.

Ok, I will set this to be true by default only on x86.

Thanks,
Shaoqin

> 
> 
> With regards,
> Daniel

-- 
Shaoqin