Re: [PATCH 1/5] Revert "hw/display/ramfb: initialize fw-config space with xres/ yres"

[Laszlo Ersek]

On 04/22/20 12:02, Gerd Hoffmann wrote:
> This reverts commit f79081b4b71b72640bedd40a7cd76f864c8287f1.
> 
> Patch has broken byteorder handling: RAMFBCfg fields are in bigendian
> byteorder, the reset function doesn't care so native byteorder is used
> instead.  Given this went unnoticed so far the feature is obviously
> unused, so just revert the patch.
> 
> Cc: Hou Qiming <address@hidden>
> Signed-off-by: Gerd Hoffmann <address@hidden>
> ---
>  include/hw/display/ramfb.h    |  2 +-
>  hw/display/ramfb-standalone.c | 12 +-----------
>  hw/display/ramfb.c            | 16 +---------------
>  hw/vfio/display.c             |  4 ++--
>  stubs/ramfb.c                 |  2 +-
>  5 files changed, 6 insertions(+), 30 deletions(-)

Acked-by: Laszlo Ersek <address@hidden>


> diff --git a/include/hw/display/ramfb.h b/include/hw/display/ramfb.h
> index f6c2de93b222..b33a2c467b28 100644
> --- a/include/hw/display/ramfb.h
> +++ b/include/hw/display/ramfb.h
> @@ -4,7 +4,7 @@
>  /* ramfb.c */
>  typedef struct RAMFBState RAMFBState;
>  void ramfb_display_update(QemuConsole *con, RAMFBState *s);
> -RAMFBState *ramfb_setup(DeviceState *dev, Error **errp);
> +RAMFBState *ramfb_setup(Error **errp);
>  
>  /* ramfb-standalone.c */
>  #define TYPE_RAMFB_DEVICE "ramfb"
> diff --git a/hw/display/ramfb-standalone.c b/hw/display/ramfb-standalone.c
> index d76a9d0fe2c9..b18db97eeb1b 100644
> --- a/hw/display/ramfb-standalone.c
> +++ b/hw/display/ramfb-standalone.c
> @@ -3,7 +3,6 @@
>  #include "qemu/module.h"
>  #include "hw/loader.h"
>  #include "hw/qdev-properties.h"
> -#include "hw/isa/isa.h"
>  #include "hw/display/ramfb.h"
>  #include "ui/console.h"
>  
> @@ -13,8 +12,6 @@ typedef struct RAMFBStandaloneState {
>      SysBusDevice parent_obj;
>      QemuConsole *con;
>      RAMFBState *state;
> -    uint32_t xres;
> -    uint32_t yres;
>  } RAMFBStandaloneState;
>  
>  static void display_update_wrapper(void *dev)
> @@ -37,22 +34,15 @@ static void ramfb_realizefn(DeviceState *dev, Error 
> **errp)
>      RAMFBStandaloneState *ramfb = RAMFB(dev);
>  
>      ramfb->con = graphic_console_init(dev, 0, &wrapper_ops, dev);
> -    ramfb->state = ramfb_setup(dev, errp);
> +    ramfb->state = ramfb_setup(errp);
>  }
>  
> -static Property ramfb_properties[] = {
> -    DEFINE_PROP_UINT32("xres", RAMFBStandaloneState, xres, 0),
> -    DEFINE_PROP_UINT32("yres", RAMFBStandaloneState, yres, 0),
> -    DEFINE_PROP_END_OF_LIST(),
> -};
> -
>  static void ramfb_class_initfn(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
>      dc->realize = ramfb_realizefn;
> -    device_class_set_props(dc, ramfb_properties);
>      dc->desc = "ram framebuffer standalone device";
>      dc->user_creatable = true;
>  }
> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
> index 7ba07c80f6e1..bd4746dc1768 100644
> --- a/hw/display/ramfb.c
> +++ b/hw/display/ramfb.c
> @@ -13,7 +13,6 @@
>  
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
> -#include "qemu/option.h"
>  #include "hw/loader.h"
>  #include "hw/display/ramfb.h"
>  #include "ui/console.h"
> @@ -31,7 +30,6 @@ struct QEMU_PACKED RAMFBCfg {
>  struct RAMFBState {
>      DisplaySurface *ds;
>      uint32_t width, height;
> -    uint32_t starting_width, starting_height;
>      struct RAMFBCfg cfg;
>      bool locked;
>  };
> @@ -117,11 +115,9 @@ static void ramfb_reset(void *opaque)
>      RAMFBState *s = (RAMFBState *)opaque;
>      s->locked = false;
>      memset(&s->cfg, 0, sizeof(s->cfg));
> -    s->cfg.width = s->starting_width;
> -    s->cfg.height = s->starting_height;
>  }
>  
> -RAMFBState *ramfb_setup(DeviceState* dev, Error **errp)
> +RAMFBState *ramfb_setup(Error **errp)
>  {
>      FWCfgState *fw_cfg = fw_cfg_find();
>      RAMFBState *s;
> @@ -133,16 +129,6 @@ RAMFBState *ramfb_setup(DeviceState* dev, Error **errp)
>  
>      s = g_new0(RAMFBState, 1);
>  
> -    const char *s_fb_width = qemu_opt_get(dev->opts, "xres");
> -    const char *s_fb_height = qemu_opt_get(dev->opts, "yres");
> -    if (s_fb_width) {
> -        s->cfg.width = atoi(s_fb_width);
> -        s->starting_width = s->cfg.width;
> -    }
> -    if (s_fb_height) {
> -        s->cfg.height = atoi(s_fb_height);
> -        s->starting_height = s->cfg.height;
> -    }
>      s->locked = false;
>  
>      rom_add_vga("vgabios-ramfb.bin");
> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> index f4977c66e1b5..a57a22674d62 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -353,7 +353,7 @@ static int vfio_display_dmabuf_init(VFIOPCIDevice *vdev, 
> Error **errp)
>                                            &vfio_display_dmabuf_ops,
>                                            vdev);
>      if (vdev->enable_ramfb) {
> -        vdev->dpy->ramfb = ramfb_setup(DEVICE(vdev), errp);
> +        vdev->dpy->ramfb = ramfb_setup(errp);
>      }
>      vfio_display_edid_init(vdev);
>      return 0;
> @@ -479,7 +479,7 @@ static int vfio_display_region_init(VFIOPCIDevice *vdev, 
> Error **errp)
>                                            &vfio_display_region_ops,
>                                            vdev);
>      if (vdev->enable_ramfb) {
> -        vdev->dpy->ramfb = ramfb_setup(DEVICE(vdev), errp);
> +        vdev->dpy->ramfb = ramfb_setup(errp);
>      }
>      return 0;
>  }
> diff --git a/stubs/ramfb.c b/stubs/ramfb.c
> index 0799093a5d6e..48143f33542f 100644
> --- a/stubs/ramfb.c
> +++ b/stubs/ramfb.c
> @@ -6,7 +6,7 @@ void ramfb_display_update(QemuConsole *con, RAMFBState *s)
>  {
>  }
>  
> -RAMFBState *ramfb_setup(DeviceState* dev, Error **errp)
> +RAMFBState *ramfb_setup(Error **errp)
>  {
>      error_setg(errp, "ramfb support not available");
>      return NULL;
>