* Re: [Qemu-devel] [4/6] ds1225y nvram: Fix some bugs
2008-03-03 10:45 [Qemu-devel] [4/6] ds1225y nvram: Fix some bugs Hervé Poussineau
@ 2008-03-13 1:48 ` Aurelien Jarno
0 siblings, 0 replies; 2+ messages in thread
From: Aurelien Jarno @ 2008-03-13 1:48 UTC (permalink / raw)
To: Hervé Poussineau; +Cc: qemu-devel
On Mon, Mar 03, 2008 at 11:45:28AM +0100, Hervé Poussineau wrote:
> Attached files fixes some problems with nvram emulation:
> - whole nvram was erased in some conditions
> - fix out of range accesses
> - improve speed by keeping contents in memory
>
> Sorry to not provide a patch for ds1225y.c, but it contains mixed
> line-endings and my diff/patch tools doesn't like that.
I have fixed the line-terminators in the CVS. Please find below the
diff, with inline comments.
> diff --git a/hw/ds1225y.c b/hw/ds1225y.c
> index 2b3f02e..a0c52a8 100644
> --- a/hw/ds1225y.c
> +++ b/hw/ds1225y.c
> @@ -1,8 +1,8 @@
> /*
> * QEMU NVRAM emulation for DS1225Y chip
> - *
> - * Copyright (c) 2007 Hervé Poussineau
> - *
> + *
> + * Copyright (c) 2007-2008 Hervé Poussineau
> + *
> * 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
> @@ -26,98 +26,167 @@
> #include "mips.h"
> #include "nvram.h"
>
> -typedef enum
> -{
> - none = 0,
> - readmode,
> - writemode,
> -} nvram_open_mode;
> +//#define DEBUG_NVRAM
>
> -struct ds1225y_t
> +typedef struct ds1225y_t
> {
> target_phys_addr_t mem_base;
> uint32_t capacity;
> - const char *filename;
> QEMUFile *file;
> - nvram_open_mode open_mode;
> -};
> + uint8_t *contents;
> + uint8_t protection;
> +} ds1225y_t;
>
> -static int ds1225y_set_to_mode(ds1225y_t *NVRAM, nvram_open_mode mode, const char *filemode)
> -{
> - if (NVRAM->open_mode != mode)
> - {
> - if (NVRAM->file)
> - qemu_fclose(NVRAM->file);
> - NVRAM->file = qemu_fopen(NVRAM->filename, filemode);
> - NVRAM->open_mode = mode;
> - }
> - return (NVRAM->file != NULL);
> -}
>
> static uint32_t nvram_readb (void *opaque, target_phys_addr_t addr)
> {
> - ds1225y_t *NVRAM = opaque;
> + ds1225y_t *s = opaque;
> int64_t pos;
> + uint32_t val;
> +
> + pos = addr - s->mem_base;
> + if (pos >= s->capacity)
> + pos -= s->capacity;
>
> - pos = addr - NVRAM->mem_base;
> - if (addr >= NVRAM->capacity)
> - addr -= NVRAM->capacity;
> + val = s->contents[pos];
>
> - if (!ds1225y_set_to_mode(NVRAM, readmode, "rb"))
> - return 0;
> - qemu_fseek(NVRAM->file, pos, SEEK_SET);
> - return (uint32_t)qemu_get_byte(NVRAM->file);
> +#ifdef DEBUG_NVRAM
> + printf("nvram: read 0x%x at " TARGET_FMT_lx "\n", val, addr);
> +#endif
> + return val;
> +}
> +
> +static uint32_t nvram_readw (void *opaque, target_phys_addr_t addr)
> +{
> + uint32_t v;
> + v = nvram_readb(opaque, addr);
> + v |= nvram_readb(opaque, addr + 1) << 8;
> + return v;
> }
>
> -static void nvram_writeb (void *opaque, target_phys_addr_t addr, uint32_t value)
> +static uint32_t nvram_readl (void *opaque, target_phys_addr_t addr)
> {
> - ds1225y_t *NVRAM = opaque;
> + uint32_t v;
> + v = nvram_readb(opaque, addr);
> + v |= nvram_readb(opaque, addr + 1) << 8;
> + v |= nvram_readb(opaque, addr + 2) << 16;
> + v |= nvram_readb(opaque, addr + 3) << 24;
> + return v;
> +}
> +
> +static void nvram_writeb (void *opaque, target_phys_addr_t addr, uint32_t val)
> +{
> + ds1225y_t *s = opaque;
> int64_t pos;
>
> - pos = addr - NVRAM->mem_base;
> - if (ds1225y_set_to_mode(NVRAM, writemode, "wb"))
> - {
> - qemu_fseek(NVRAM->file, pos, SEEK_SET);
> - qemu_put_byte(NVRAM->file, (int)value);
> +#ifdef DEBUG_NVRAM
> + printf("nvram: write 0x%x at " TARGET_FMT_lx "\n", val, addr);
> +#endif
> +
> + pos = addr - s->mem_base;
> + s->contents[pos] = val & 0xff;
> + if (s->file) {
> + qemu_fseek(s->file, pos, SEEK_SET);
> + qemu_put_byte(s->file, (int)val);
> + qemu_fflush(s->file);
> }
> }
>
> +static void nvram_writew (void *opaque, target_phys_addr_t addr, uint32_t val)
> +{
> + nvram_writeb(opaque, addr, val & 0xff);
> + nvram_writeb(opaque, addr + 1, (val >> 8) & 0xff);
> +}
> +
> +static void nvram_writel (void *opaque, target_phys_addr_t addr, uint32_t val)
> +{
> + nvram_writeb(opaque, addr, val & 0xff);
> + nvram_writeb(opaque, addr + 1, (val >> 8) & 0xff);
> + nvram_writeb(opaque, addr + 2, (val >> 16) & 0xff);
> + nvram_writeb(opaque, addr + 3, (val >> 24) & 0xff);
> +}
> +
> +static void nvram_writeb_protected (void *opaque, target_phys_addr_t addr, uint32_t val)
> +{
> + ds1225y_t *s = opaque;
> +
> + if (s->protection != 7) {
> +#ifdef DEBUG_NVRAM
> + printf("nvram: prevent write of 0x%x at " TARGET_FMT_lx "\n", val, addr);
> +#endif
> + return;
> + }
> +
> + nvram_writeb(opaque, addr - s->capacity, val);
> +}
> +
> +static void nvram_writew_protected (void *opaque, target_phys_addr_t addr, uint32_t val)
> +{
> + nvram_writeb_protected(opaque, addr, val & 0xff);
> + nvram_writeb_protected(opaque, addr + 1, (val >> 8) & 0xff);
> +}
> +
> +static void nvram_writel_protected (void *opaque, target_phys_addr_t addr, uint32_t val)
> +{
> + nvram_writeb_protected(opaque, addr, val & 0xff);
> + nvram_writeb_protected(opaque, addr + 1, (val >> 8) & 0xff);
> + nvram_writeb_protected(opaque, addr + 2, (val >> 16) & 0xff);
> + nvram_writeb_protected(opaque, addr + 3, (val >> 24) & 0xff);
> +}
> +
> static CPUReadMemoryFunc *nvram_read[] = {
> &nvram_readb,
> - NULL,
> - NULL,
> + &nvram_readw,
> + &nvram_readl,
> };
>
> static CPUWriteMemoryFunc *nvram_write[] = {
> &nvram_writeb,
> - NULL,
> - NULL,
> + &nvram_writew,
> + &nvram_writel,
> };
>
> -static CPUWriteMemoryFunc *nvram_none[] = {
> - NULL,
> - NULL,
> - NULL,
> +static CPUWriteMemoryFunc *nvram_write_protected[] = {
> + &nvram_writeb_protected,
> + &nvram_writew_protected,
> + &nvram_writel_protected,
> };
>
> /* Initialisation routine */
> -ds1225y_t *ds1225y_init(target_phys_addr_t mem_base, const char *filename)
> +void *ds1225y_init(target_phys_addr_t mem_base, const char *filename)
> {
> ds1225y_t *s;
> - int mem_index1, mem_index2;
> + int mem_indexRW, mem_indexRP;
> + QEMUFile *file;
>
> s = qemu_mallocz(sizeof(ds1225y_t));
> if (!s)
> return NULL;
> - s->mem_base = mem_base;
> s->capacity = 0x2000; /* Fixed for ds1225y chip: 8K */
> - s->filename = filename;
> + s->contents = qemu_mallocz(s->capacity);
> + if (!s->contents) {
> + return NULL;
> + }
> + s->mem_base = mem_base;
> + s->protection = 7;
> +
> + /* Read current file */
> + file = qemu_fopen(filename, "rb");
> + if (file) {
> + qemu_get_buffer(file, s->contents, s->capacity);
> + qemu_fclose(file);
> + }
> + s->file = qemu_fopen(filename, "wb");
> + if (s->file) {
> + qemu_put_buffer(s->file, s->contents, s->capacity);
> + qemu_fflush(s->file);
> + }
What's the point of writing the file with the exact content that has
just been read?
BTW "capacity" used that way is probably a frenglish word. "size" or
"chip_size" would probably be more correct here.
> /* Read/write memory */
> - mem_index1 = cpu_register_io_memory(0, nvram_read, nvram_write, s);
> - cpu_register_physical_memory(mem_base, s->capacity, mem_index1);
> - /* Read-only memory */
> - mem_index2 = cpu_register_io_memory(0, nvram_read, nvram_none, s);
> - cpu_register_physical_memory(mem_base + s->capacity, s->capacity, mem_index2);
> + mem_indexRW = cpu_register_io_memory(0, nvram_read, nvram_write, s);
> + cpu_register_physical_memory(mem_base, s->capacity, mem_indexRW);
> + /* Read/write protected memory */
> + mem_indexRP = cpu_register_io_memory(0, nvram_read, nvram_write_protected, s);
> + cpu_register_physical_memory(mem_base + s->capacity, s->capacity, mem_indexRP);
> return s;
> }
> diff --git a/hw/mips.h b/hw/mips.h
> index 0196b6c..f4599a4 100644
> --- a/hw/mips.h
> +++ b/hw/mips.h
> @@ -6,8 +6,8 @@
> PCIBus *pci_gt64120_init(qemu_irq *pic);
>
> /* ds1225y.c */
> -typedef struct ds1225y_t ds1225y_t;
> -ds1225y_t *ds1225y_init(target_phys_addr_t mem_base, const char *filename);
> +void *ds1225y_init(target_phys_addr_t mem_base, const char *filename);
> +void ds1225y_set_protection(void *opaque, int protection);
>
> /* mipsnet.c */
> void mipsnet_init(int base, qemu_irq irq, NICInfo *nd);
--
.''`. Aurelien Jarno | GPG: 1024D/F1BCDB73
: :' : Debian developer | Electrical Engineer
`. `' aurel32@debian.org | aurelien@aurel32.net
`- people.debian.org/~aurel32 | www.aurel32.net
^ permalink raw reply [flat|nested] 2+ messages in thread