[PATCHv3,1/2] sun4m: Add Sun CG3 framebuffer and corresponding OpenBIOS FCode ROM

The CG3 framebuffer is a simple 8-bit framebuffer for use with operating
systems such as early Solaris that do not have drivers for TCX.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
CC: Blue Swirl <blauwirbel@gmail.com>
CC: Anthony Liguori <aliguori@amazon.com>
CC: Peter Maydell <peter.maydell@linaro.org>
CC: Bob Breuer <breuerr@mc.net>
CC: Artyom Tarasenko <atar4qemu@gmail.com>
---
 Makefile                          |    2 +-
 default-configs/sparc-softmmu.mak |    1 +
 hw/display/Makefile.objs          |    1 +
 hw/display/cg3.c                  |  384 +++++++++++++++++++++++++++++++++++++
 pc-bios/QEMU,cgthree.bin          |  Bin 0 -> 850 bytes
 pc-bios/README                    |    4 +-
 6 files changed, 389 insertions(+), 3 deletions(-)
 create mode 100644 hw/display/cg3.c
 create mode 100644 pc-bios/QEMU,cgthree.bin

Hi Mark,

On Wed, Feb 19, 2014 at 09:05:19AM +0000, Mark Cave-Ayland wrote:
> The CG3 framebuffer is a simple 8-bit framebuffer for use with operating
> systems such as early Solaris that do not have drivers for TCX.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> CC: Blue Swirl <blauwirbel@gmail.com>
> CC: Anthony Liguori <aliguori@amazon.com>
> CC: Peter Maydell <peter.maydell@linaro.org>
> CC: Bob Breuer <breuerr@mc.net>
> CC: Artyom Tarasenko <atar4qemu@gmail.com>
> ---
>  Makefile                          |    2 +-
>  default-configs/sparc-softmmu.mak |    1 +
>  hw/display/Makefile.objs          |    1 +
>  hw/display/cg3.c                  |  384 +++++++++++++++++++++++++++++++++++++
>  pc-bios/QEMU,cgthree.bin          |  Bin 0 -> 850 bytes
>  pc-bios/README                    |    4 +-
>  6 files changed, 389 insertions(+), 3 deletions(-)
>  create mode 100644 hw/display/cg3.c
>  create mode 100644 pc-bios/QEMU,cgthree.bin
> 
> diff --git a/Makefile b/Makefile
> index 807054b..c3c7ccc 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -293,7 +293,7 @@ ifdef INSTALL_BLOBS
>  BLOBS=bios.bin bios-256k.bin sgabios.bin vgabios.bin vgabios-cirrus.bin \
>  vgabios-stdvga.bin vgabios-vmware.bin vgabios-qxl.bin \
>  acpi-dsdt.aml q35-acpi-dsdt.aml \
> -ppc_rom.bin openbios-sparc32 openbios-sparc64 openbios-ppc QEMU,tcx.bin \
> +ppc_rom.bin openbios-sparc32 openbios-sparc64 openbios-ppc QEMU,tcx.bin QEMU,cgthree.bin \
>  pxe-e1000.rom pxe-eepro100.rom pxe-ne2k_pci.rom \
>  pxe-pcnet.rom pxe-rtl8139.rom pxe-virtio.rom \
>  efi-e1000.rom efi-eepro100.rom efi-ne2k_pci.rom \
> diff --git a/default-configs/sparc-softmmu.mak b/default-configs/sparc-softmmu.mak
> index 8fc93dd..ab796b3 100644
> --- a/default-configs/sparc-softmmu.mak
> +++ b/default-configs/sparc-softmmu.mak
> @@ -10,6 +10,7 @@ CONFIG_EMPTY_SLOT=y
>  CONFIG_PCNET_COMMON=y
>  CONFIG_LANCE=y
>  CONFIG_TCX=y
> +CONFIG_CG3=y
>  CONFIG_SLAVIO=y
>  CONFIG_CS4231=y
>  CONFIG_GRLIB=y
> diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
> index 540df82..7ed76a9 100644
> --- a/hw/display/Makefile.objs
> +++ b/hw/display/Makefile.objs
> @@ -28,6 +28,7 @@ obj-$(CONFIG_OMAP) += omap_lcdc.o
>  obj-$(CONFIG_PXA2XX) += pxa2xx_lcd.o
>  obj-$(CONFIG_SM501) += sm501.o
>  obj-$(CONFIG_TCX) += tcx.o
> +obj-$(CONFIG_CG3) += cg3.o
>  
>  obj-$(CONFIG_VGA) += vga.o
>  
> diff --git a/hw/display/cg3.c b/hw/display/cg3.c
> new file mode 100644
> index 0000000..b6bc0c2
> --- /dev/null
> +++ b/hw/display/cg3.c
> @@ -0,0 +1,384 @@
> +/*
> + * QEMU CG3 Frame buffer
> + *
> + * Copyright (c) 2012 Bob Breuer
> + * Copyright (c) 2013 Mark Cave-Ayland
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "qemu-common.h"
> +#include "qemu/error-report.h"
> +#include "ui/console.h"
> +#include "hw/sysbus.h"
> +#include "hw/loader.h"
> +
> +/* Change to 1 to enable debugging */
> +#define DEBUG_CG3 0
> +
> +#define CG3_ROM_FILE  "QEMU,cgthree.bin"
> +#define FCODE_MAX_ROM_SIZE 0x10000
> +
> +#define CG3_REG_SIZE            0x20
> +
> +#define CG3_REG_BT458_ADDR      0x0
> +#define CG3_REG_BT458_COLMAP    0x4
> +#define CG3_REG_FBC_CTRL        0x10
> +#define CG3_REG_FBC_STATUS      0x11
> +#define CG3_REG_FBC_CURSTART    0x12
> +#define CG3_REG_FBC_CUREND      0x13
> +#define CG3_REG_FBC_VCTRL       0x14
> +
> +/* Control register flags */
> +#define CG3_CR_ENABLE_INTS      0x80
> +
> +/* Status register flags */
> +#define CG3_SR_PENDING_INT      0x80
> +#define CG3_SR_1152_900_76_B    0x60
> +#define CG3_SR_ID_COLOR         0x01
> +
> +#define CG3_VRAM_SIZE 0x100000
> +#define CG3_VRAM_OFFSET 0x800000
> +
> +#define DPRINTF(fmt, ...) do { \
> +    if (DEBUG_CG3) { \
> +        printf("CG3: " fmt , ## __VA_ARGS__); \
> +    } \
> +} while (0);
> +
> +#define TYPE_CG3 "cgthree"
> +#define CG3(obj) OBJECT_CHECK(CG3State, (obj), TYPE_CG3)
> +
> +typedef struct CG3State {
> +    SysBusDevice parent_obj;
> +
> +    QemuConsole *con;
> +    qemu_irq irq;
> +    hwaddr prom_addr;
> +    MemoryRegion vram_mem;
> +    MemoryRegion rom;
> +    MemoryRegion reg;
> +    uint32_t vram_size;
> +    int full_update;
> +    uint8_t regs[16];
> +    uint8_t r[256], g[256], b[256];
> +    uint16_t width, height, depth;
> +    uint8_t dac_index, dac_state;
> +} CG3State;
> +
> +static void cg3_update_display(void *opaque)
> +{
> +    CG3State *s = opaque;
> +    DisplaySurface *surface = qemu_console_surface(s->con);
> +    const uint8_t *pix;
> +    uint32_t *data;
> +    uint32_t dval;
> +    int x, y, y_start;
> +    unsigned int width, height;
> +    ram_addr_t page, page_min, page_max;
> +
> +    if (surface_bits_per_pixel(surface) != 32) {
> +        return;
> +    }
> +    width = s->width;
> +    height = s->height;
> +
> +    y_start = -1;
> +    page_min = -1;
> +    page_max = 0;
> +    page = 0;
> +    pix = memory_region_get_ram_ptr(&s->vram_mem);
> +    data = (uint32_t *)surface_data(surface);
> +
> +    for (y = 0; y < height; y++) {
> +        int update = s->full_update;
> +
> +        page = (y * width) & TARGET_PAGE_MASK;
> +        update |= memory_region_get_dirty(&s->vram_mem, page, page + width,
> +                                          DIRTY_MEMORY_VGA);
> +        if (update) {
> +            if (y_start < 0) {
> +                y_start = y;
> +            }
> +            if (page < page_min) {
> +                page_min = page;
> +            }
> +            if (page > page_max) {
> +                page_max = page;
> +            }
> +
> +            for (x = 0; x < width; x++) {
> +                dval = *pix++;
> +                dval = (s->r[dval] << 16) | (s->g[dval] << 8) | s->b[dval];
> +                *data++ = dval;
> +            }
> +        } else {
> +            if (y_start >= 0) {
> +                dpy_gfx_update(s->con, 0, y_start, s->width, y - y_start);
> +                y_start = -1;
> +            }
> +            pix += width;
> +            data += width;
> +        }
> +    }
> +    s->full_update = 0;
> +    if (y_start >= 0) {
> +        dpy_gfx_update(s->con, 0, y_start, s->width, y - y_start);
> +    }
> +    if (page_max >= page_min) {
> +        memory_region_reset_dirty(&s->vram_mem,
> +                              page_min, page_max - page_min + TARGET_PAGE_SIZE,
> +                              DIRTY_MEMORY_VGA);
> +    }
> +    /* vsync interrupt? */
> +    if (s->regs[0] & CG3_CR_ENABLE_INTS) {
> +        s->regs[1] |= CG3_SR_PENDING_INT;
> +        qemu_irq_raise(s->irq);
> +    }
> +}
> +
> +static void cg3_invalidate_display(void *opaque)
> +{
> +    CG3State *s = opaque;
> +
> +    memory_region_set_dirty(&s->vram_mem, 0, CG3_VRAM_SIZE);
> +}
> +
> +static uint64_t cg3_reg_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    CG3State *s = opaque;
> +    int val;
> +
> +    switch (addr) {
> +    case CG3_REG_BT458_ADDR:
> +    case CG3_REG_BT458_COLMAP:
> +        val = 0;
> +        break;
> +    case CG3_REG_FBC_CTRL:
> +        val = s->regs[0];
> +        break;
> +    case CG3_REG_FBC_STATUS:
> +        /* monitor ID 6, board type = 1 (color) */
> +        val = s->regs[1] | CG3_SR_1152_900_76_B | CG3_SR_ID_COLOR;
> +        break;
> +    case CG3_REG_FBC_CURSTART ... CG3_REG_SIZE:
> +        val = s->regs[addr - 0x10];
> +        break;
> +    default:
> +        qemu_log_mask(LOG_UNIMP,
> +                  "cg3: Unimplemented register read "
> +                  "reg 0x%" HWADDR_PRIx " size 0x%x\n",
> +                  addr, size);
> +        val = 0;
> +        break;
> +    }
> +    DPRINTF("read %02x from reg %x\n", val, (int)addr);
> +    return val;
> +}
> +
> +static void cg3_reg_write(void *opaque, hwaddr addr, uint64_t val,
> +                          unsigned size)
> +{
> +    CG3State *s = opaque;
> +    uint8_t regval;
> +    int i;
> +
> +    DPRINTF("write %02lx to reg %x size %d\n", val, (int)addr, size);
> +
> +    switch (addr) {
> +    case CG3_REG_BT458_ADDR:
> +        s->dac_index = val;
> +        s->dac_state = 0;
> +        break;
> +    case CG3_REG_BT458_COLMAP:
> +        /* This register can be written to as either a long word or a byte */
> +        if (size == 1) {
> +            val <<= 24;
> +        }
> +
> +        for (i = 0; i < size; i++) {
> +            regval = val >> 24;
> +
> +            switch (s->dac_state) {
> +            case 0:
> +                s->r[s->dac_index] = regval;
> +                s->dac_state++;
> +                break;
> +            case 1:
> +                s->g[s->dac_index] = regval;
> +                s->dac_state++;
> +                break;
> +            case 2:
> +                s->b[s->dac_index] = regval;
> +                /* Index autoincrement */
> +                s->dac_index = (s->dac_index + 1) & 0xff;
> +            default:
> +                s->dac_state = 0;
> +                break;
> +            }
> +            val <<= 8;
> +        }
> +        s->full_update = 1;
> +        break;
> +    case CG3_REG_FBC_CTRL:
> +        s->regs[0] = val;
> +        break;
> +    case CG3_REG_FBC_STATUS:
> +        if (s->regs[1] & CG3_SR_PENDING_INT) {
> +            /* clear interrupt */
> +            s->regs[1] &= ~CG3_SR_PENDING_INT;
> +            qemu_irq_lower(s->irq);
> +        }
> +        break;
> +    case CG3_REG_FBC_CURSTART ... CG3_REG_SIZE:
> +        s->regs[addr - 0x10] = val;
> +        break;
> +    default:
> +        qemu_log_mask(LOG_UNIMP,
> +                  "cg3: Unimplemented register write "
> +                  "reg 0x%" HWADDR_PRIx " size 0x%x value 0x%" PRIx64 "\n",
> +                  addr, size, val);
> +        break;
> +    }
> +}
> +
> +static const MemoryRegionOps cg3_reg_ops = {
> +    .read = cg3_reg_read,
> +    .write = cg3_reg_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 1,
> +        .max_access_size = 4,
> +    },
> +};
> +
> +static const GraphicHwOps cg3_ops = {
> +    .invalidate = cg3_invalidate_display,
> +    .gfx_update = cg3_update_display,
> +};
> +
> +static void cg3_realizefn(DeviceState *dev, Error **errp)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    CG3State *s = CG3(dev);
> +    int ret;
> +    char *fcode_filename;
> +
> +    /* FCode ROM */
> +    memory_region_init_ram(&s->rom, NULL, "cg3.prom", FCODE_MAX_ROM_SIZE);
> +    vmstate_register_ram_global(&s->rom);
> +    memory_region_set_readonly(&s->rom, true);
> +    sysbus_init_mmio(sbd, &s->rom);
> +


I think this initialization code could be done in a SysBusDeviceClass init operation,
don't you think?


> +    fcode_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, CG3_ROM_FILE);
> +    if (fcode_filename) {
> +        ret = load_image_targphys(fcode_filename, s->prom_addr,
> +                                  FCODE_MAX_ROM_SIZE);
> +        if (ret < 0 || ret > FCODE_MAX_ROM_SIZE) {
> +            error_report("cg3: could not load prom '%s'", CG3_ROM_FILE);


What happens if we fail to load the rom file? is the framebuffer supposed to work?


> +        }
> +    }
> +
> +    memory_region_init_io(&s->reg, NULL, &cg3_reg_ops, s, "cg3.reg",
> +                          CG3_REG_SIZE);
> +    sysbus_init_mmio(sbd, &s->reg);
> +
> +    memory_region_init_ram(&s->vram_mem, NULL, "cg3.vram", s->vram_size);
> +    vmstate_register_ram_global(&s->vram_mem);
> +    sysbus_init_mmio(sbd, &s->vram_mem);
> +
> +    sysbus_init_irq(sbd, &s->irq);
> +
> +    s->con = graphic_console_init(DEVICE(dev), &cg3_ops, s);
> +    qemu_console_resize(s->con, s->width, s->height);
> +}
> +
> +static int vmstate_cg3_post_load(void *opaque, int version_id)
> +{
> +    CG3State *s = opaque;
> +
> +    cg3_invalidate_display(s);
> +
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_cg3 = {
> +    .name = "cg3",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .post_load = vmstate_cg3_post_load,
> +    .fields    = (VMStateField[]) {
> +        VMSTATE_UINT16(height, CG3State),
> +        VMSTATE_UINT16(width, CG3State),
> +        VMSTATE_UINT16(depth, CG3State),
> +        VMSTATE_BUFFER(r, CG3State),
> +        VMSTATE_BUFFER(g, CG3State),
> +        VMSTATE_BUFFER(b, CG3State),
> +        VMSTATE_UINT8(dac_index, CG3State),
> +        VMSTATE_UINT8(dac_state, CG3State),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void cg3_reset(DeviceState *d)
> +{
> +    CG3State *s = CG3(d);
> +
> +    /* Initialize palette */
> +    memset(s->r, 0, 256);
> +    memset(s->g, 0, 256);
> +    memset(s->b, 0, 256);
> +
> +    s->dac_state = 0;
> +    s->full_update = 1;
> +    qemu_irq_lower(s->irq);
> +}
> +
> +static Property cg3_properties[] = {
> +    DEFINE_PROP_UINT32("vram_size",    CG3State, vram_size, -1),
> +    DEFINE_PROP_UINT16("width",        CG3State, width,     -1),
> +    DEFINE_PROP_UINT16("height",       CG3State, height,    -1),
> +    DEFINE_PROP_UINT16("depth",        CG3State, depth,     -1),
> +    DEFINE_PROP_UINT64("prom_addr",    CG3State, prom_addr, -1),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void cg3_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = cg3_realizefn;
> +    dc->reset = cg3_reset;
> +    dc->vmsd = &vmstate_cg3;
> +    dc->props = cg3_properties;
> +}
> +
> +static const TypeInfo cg3_info = {
> +    .name          = TYPE_CG3,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(CG3State),
> +    .class_init    = cg3_class_init,
> +};
> +
> +static void cg3_register_types(void)
> +{
> +    type_register_static(&cg3_info);
> +}
> +
> +type_init(cg3_register_types)
> diff --git a/pc-bios/QEMU,cgthree.bin b/pc-bios/QEMU,cgthree.bin
> new file mode 100644
> index 0000000000000000000000000000000000000000..6fec9462070bf9a0f88024c7bf66f444d1d11d61
> GIT binary patch
> literal 850
> zcmZ{i&2HL25XX10!NXT-76XB#Rk_y^s6|SoR%&la53Q7_>KX6`yTvxLO)GA>_ch=J
> zP_0Ed5~+vw1^OC&f^?RT1C@IDd*?SZ|5@Af2Y<wjX;&#S`O9L)^D4_Nujb2jiXgca
> zPDC*9!r1=eIU=;bdQRdZo4=yUd6i|Ci{<)%MIyz_ir4;eaD_K=6J(UtR=nVdN#fcA
> zFNrruCp7i~VGm}B*rM#FYA_wy$!sE!rI=f#Xh;N$<uT(|S$=6UrZaVA++l5xwGGbi
> zu)fC(Rdr#9vwOTXDN4*eUYqPSqhX~xGG|XyEYsmuks~^k)Zx)xil*!=AoCkEsCJ<O
> zoLnnXbubf1(BxVqMqm=>vIAO|=luS}_JT~FP*rk6h2b=zc%GuQBB{~))g@X_rt6=%
> zVK@$>Ha6q}>z8kpvySz{2N@kpEMXb>Jz5ksB_3_=s6dTCOX4v$*W4J65;qbe1Ke=D
> zcrxzKpvBAAAKra@*6Vcb?u%{@nkk;p^!ZDR`PfpU9u9?JLjiUu4_oRe`bNojC4ddA
> z-NOJr!DrG6H~Nkfi8uxm4a5uZ+ZQly!#DLmP9;_lsVKMI5>-P{cC&R96e!56_1J6&
> zm}<Z|R2J&PbKMJ)NOcg^Z|U+yl{R+kL90s5Wj_qOB#i7>1hD{<>+03P;w8TyOmF(b
> yWEu%F;rYw!_h)ClbGu8)^3d%^loP5i*^VudTX8sz;xLL`?}lgvPvCTor|d5SYsmWm
> 
> literal 0
> HcmV?d00001
> 
> diff --git a/pc-bios/README b/pc-bios/README
> index f190068..5914200 100644
> --- a/pc-bios/README
> +++ b/pc-bios/README
> @@ -11,8 +11,8 @@
>    firmware implementation. The goal is to implement a 100% IEEE
>    1275-1994 (referred to as Open Firmware) compliant firmware.
>    The included images for PowerPC (for 32 and 64 bit PPC CPUs),
> -  Sparc32 (including QEMU,tcx.bin) and Sparc64 are built from OpenBIOS SVN
> -  revision 1246.
> +  Sparc32 (including QEMU,tcx.bin and QEMU,cgthree.bin) and Sparc64 are built
> +  from OpenBIOS SVN revision 1246.
>  
>  - SLOF (Slimline Open Firmware) is a free IEEE 1275 Open Firmware
>    implementation for certain IBM POWER hardware.  The sources are at
> -- 
> 1.7.10.4
> 
>

On 19/02/14 13:35, Leandro Dorileo wrote:

Hi Leandro,

>> +static void cg3_realizefn(DeviceState *dev, Error **errp)
>> +{
>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>> +    CG3State *s = CG3(dev);
>> +    int ret;
>> +    char *fcode_filename;
>> +
>> +    /* FCode ROM */
>> +    memory_region_init_ram(&s->rom, NULL, "cg3.prom", FCODE_MAX_ROM_SIZE);
>> +    vmstate_register_ram_global(&s->rom);
>> +    memory_region_set_readonly(&s->rom, true);
>> +    sysbus_init_mmio(sbd,&s->rom);
>> +
>
>
> I think this initialization code could be done in a SysBusDeviceClass init operation,
> don't you think?

I think it's possible since these MemoryRegions don't depend upon 
properties, but I leave that to Andres who seems reasonably happy with 
the patchset in its current form.

>
>> +    fcode_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, CG3_ROM_FILE);
>> +    if (fcode_filename) {
>> +        ret = load_image_targphys(fcode_filename, s->prom_addr,
>> +                                  FCODE_MAX_ROM_SIZE);
>> +        if (ret<  0 || ret>  FCODE_MAX_ROM_SIZE) {
>> +            error_report("cg3: could not load prom '%s'", CG3_ROM_FILE);
>
>
> What happens if we fail to load the rom file? is the framebuffer supposed to work?

I guess the framebuffer would still "work" although nothing would be 
able to find its address because the node wouldn't exist in the device tree?


ATB,

Mark.

On Wed, Feb 19, 2014 at 09:39:09PM +0000, Mark Cave-Ayland wrote:
> On 19/02/14 13:35, Leandro Dorileo wrote:
> 
> Hi Leandro,
> 
> >>+static void cg3_realizefn(DeviceState *dev, Error **errp)
> >>+{
> >>+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> >>+    CG3State *s = CG3(dev);
> >>+    int ret;
> >>+    char *fcode_filename;
> >>+
> >>+    /* FCode ROM */
> >>+    memory_region_init_ram(&s->rom, NULL, "cg3.prom", FCODE_MAX_ROM_SIZE);
> >>+    vmstate_register_ram_global(&s->rom);
> >>+    memory_region_set_readonly(&s->rom, true);
> >>+    sysbus_init_mmio(sbd,&s->rom);
> >>+
> >
> >
> >I think this initialization code could be done in a SysBusDeviceClass init operation,
> >don't you think?
> 
> I think it's possible since these MemoryRegions don't depend upon
> properties, but I leave that to Andres who seems reasonably happy with the
> patchset in its current form.


Yes, I just saw his comment in the patch 02...

> 
> >
> >>+    fcode_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, CG3_ROM_FILE);
> >>+    if (fcode_filename) {
> >>+        ret = load_image_targphys(fcode_filename, s->prom_addr,
> >>+                                  FCODE_MAX_ROM_SIZE);
> >>+        if (ret<  0 || ret>  FCODE_MAX_ROM_SIZE) {
> >>+            error_report("cg3: could not load prom '%s'", CG3_ROM_FILE);
> >
> >
> >What happens if we fail to load the rom file? is the framebuffer supposed to work?
> 
> I guess the framebuffer would still "work" although nothing would be able to
> find its address because the node wouldn't exist in the device tree?
> 

Ok, when we move this code to an instance init op we handle it a bit better.

> 
> ATB,
> 
> Mark.

Il 19/02/2014 10:05, Mark Cave-Ayland ha scritto:
> +#define CG3_REG_SIZE            0x20
> +
> +#define CG3_REG_FBC_CTRL        0x10
> +#define CG3_REG_FBC_STATUS      0x11
> +#define CG3_REG_FBC_CURSTART    0x12
> +#define CG3_REG_FBC_CUREND      0x13
> +#define CG3_REG_FBC_VCTRL       0x14
> +
> +typedef struct CG3State {
...

> +    uint8_t regs[16];

...

> +    case CG3_REG_FBC_CURSTART ... CG3_REG_SIZE:
> +        val = s->regs[addr - 0x10];
> +        break;
> +    default:

Something weird here, you can access regs[16] if addr == CG3_REG_SIZE.

The same happens in the write path.

Paolo

Il 05/03/2014 11:05, Paolo Bonzini ha scritto:
> Il 19/02/2014 10:05, Mark Cave-Ayland ha scritto:
>> +#define CG3_REG_SIZE            0x20
>> +
>> +#define CG3_REG_FBC_CTRL        0x10
>> +#define CG3_REG_FBC_STATUS      0x11
>> +#define CG3_REG_FBC_CURSTART    0x12
>> +#define CG3_REG_FBC_CUREND      0x13
>> +#define CG3_REG_FBC_VCTRL       0x14
>> +
>> +typedef struct CG3State {
> ...
>
>> +    uint8_t regs[16];
>
> ...
>
>> +    case CG3_REG_FBC_CURSTART ... CG3_REG_SIZE:
>> +        val = s->regs[addr - 0x10];
>> +        break;
>> +    default:
>
> Something weird here, you can access regs[16] if addr == CG3_REG_SIZE.
>
> The same happens in the write path.

Ping.  I cannot fix it without access to the datasheet, though I suspect 
you want CG3_REG_SIZE - 1.

Paolo

Hi,

Am 19.02.2014 22:39, schrieb Mark Cave-Ayland:
> On 19/02/14 13:35, Leandro Dorileo wrote:
> 
> Hi Leandro,
> 
>>> +static void cg3_realizefn(DeviceState *dev, Error **errp)
>>> +{
>>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>>> +    CG3State *s = CG3(dev);
>>> +    int ret;
>>> +    char *fcode_filename;
>>> +
>>> +    /* FCode ROM */
>>> +    memory_region_init_ram(&s->rom, NULL, "cg3.prom",
>>> FCODE_MAX_ROM_SIZE);
>>> +    vmstate_register_ram_global(&s->rom);
>>> +    memory_region_set_readonly(&s->rom, true);
>>> +    sysbus_init_mmio(sbd,&s->rom);
>>> +
>>
>>
>> I think this initialization code could be done in a SysBusDeviceClass
>> init operation,
>> don't you think?
> 
> I think it's possible since these MemoryRegions don't depend upon
> properties, but I leave that to Andres who seems reasonably happy with
> the patchset in its current form.

Just seeing this...

memory_region_init_ram() and sysbus_init_mmio() could indeed be moved to
an .instance_init function, given that FCODE_MAX_ROM_SIZE is constant.
The others no. It makes a difference when considering reentrancy of the
property code via qom-set (just posted a patchset that makes playing
with that easier), although there's probably more corner cases to
consider. Could either of you follow up with a cleanup?

Regards,
Andreas

>>> +    fcode_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, CG3_ROM_FILE);
>>> +    if (fcode_filename) {
>>> +        ret = load_image_targphys(fcode_filename, s->prom_addr,
>>> +                                  FCODE_MAX_ROM_SIZE);
>>> +        if (ret<  0 || ret>  FCODE_MAX_ROM_SIZE) {
>>> +            error_report("cg3: could not load prom '%s'",
>>> CG3_ROM_FILE);

On 07/05/14 20:56, Paolo Bonzini wrote:

> Il 05/03/2014 11:05, Paolo Bonzini ha scritto:
>> Il 19/02/2014 10:05, Mark Cave-Ayland ha scritto:
>>> +#define CG3_REG_SIZE            0x20
>>> +
>>> +#define CG3_REG_FBC_CTRL        0x10
>>> +#define CG3_REG_FBC_STATUS      0x11
>>> +#define CG3_REG_FBC_CURSTART    0x12
>>> +#define CG3_REG_FBC_CUREND      0x13
>>> +#define CG3_REG_FBC_VCTRL       0x14
>>> +
>>> +typedef struct CG3State {
>> ...
>>
>>> +    uint8_t regs[16];
>>
>> ...
>>
>>> +    case CG3_REG_FBC_CURSTART ... CG3_REG_SIZE:
>>> +        val = s->regs[addr - 0x10];
>>> +        break;
>>> +    default:
>>
>> Something weird here, you can access regs[16] if addr == CG3_REG_SIZE.
>>
>> The same happens in the write path.
>
> Ping.  I cannot fix it without access to the datasheet, though I suspect
> you want CG3_REG_SIZE - 1.

Hi Paolo,

Sorry I didn't think you could access regs[16] since the MemoryRegion 
size is set to CG3_REG_SIZE too (and so I hope should only handle 
accesses from 0 to CG3_REG_SIZE - 1).

Anyway, I've quickly tried a Solaris 8 boot test replacing CG3_REG_SIZE 
with CG3_REG_SIZE - 1 for the case statements in both the read and write 
paths and everything still works, so happy for you to go ahead and fix it.


ATB,

Mark.

Il 08/05/2014 16:44, Mark Cave-Ayland ha scritto:
>>>
>>>> +    case CG3_REG_FBC_CURSTART ... CG3_REG_SIZE:
>>>> +        val = s->regs[addr - 0x10];
>>>> +        break;
>>>> +    default:
>>>
>>> Something weird here, you can access regs[16] if addr == CG3_REG_SIZE.
>>>
>>> The same happens in the write path.
>>
>> Ping.  I cannot fix it without access to the datasheet, though I suspect
>> you want CG3_REG_SIZE - 1.
>
> Hi Paolo,
>
> Sorry I didn't think you could access regs[16] since the MemoryRegion
> size is set to CG3_REG_SIZE too (and so I hope should only handle
> accesses from 0 to CG3_REG_SIZE - 1).
>
> Anyway, I've quickly tried a Solaris 8 boot test replacing CG3_REG_SIZE
> with CG3_REG_SIZE - 1 for the case statements in both the read and write
> paths and everything still works, so happy for you to go ahead and fix it.

Ah okay so it's a false positive.  But yes, it's better to fix it.  I'll 
try to send a patch for qemu-trivial.

Paolo