Re: [PATCH v2 1/3] memory: add a sparse memory device for fuzzing

[Alexander Bulekov]

On 210315 1209, Darren Kenny wrote:
> Hi Alex,
> 
> On Saturday, 2021-03-13 at 18:18:57 -05, Alexander Bulekov wrote:
> > For testing, it can be useful to simulate an enormous amount of memory
> > (e.g. 2^64 RAM). This adds an MMIO device that acts as sparse memory.
> > When something writes a nonzero value to a sparse-mem address, we
> > allocate a block of memory. This block is kept around, until all of the
> > bytes within the block are zero-ed. The device has a very low priority
> 
> I don't see code below that actually checks if a block is zero-ed and
> removes it from the hash table, so is this comment correct?

No.. I will update it

> 
> > (so it can be mapped beneath actual RAM, and virtual device MMIO
> > regions).
> >
> > Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> > ---
> >  MAINTAINERS                 |   1 +
> >  hw/mem/meson.build          |   1 +
> >  hw/mem/sparse-mem.c         | 152 ++++++++++++++++++++++++++++++++++++
> >  include/hw/mem/sparse-mem.h |  19 +++++
> >  4 files changed, 173 insertions(+)
> >  create mode 100644 hw/mem/sparse-mem.c
> >  create mode 100644 include/hw/mem/sparse-mem.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index f22d83c178..9e3d8b1401 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2618,6 +2618,7 @@ R: Thomas Huth <thuth@redhat.com>
> >  S: Maintained
> >  F: tests/qtest/fuzz/
> >  F: scripts/oss-fuzz/
> > +F: hw/mem/sparse-mem.c
> >  F: docs/devel/fuzzing.rst
> >  
> >  Register API
> > diff --git a/hw/mem/meson.build b/hw/mem/meson.build
> > index 0d22f2b572..ef79e04678 100644
> > --- a/hw/mem/meson.build
> > +++ b/hw/mem/meson.build
> > @@ -1,5 +1,6 @@
> >  mem_ss = ss.source_set()
> >  mem_ss.add(files('memory-device.c'))
> > +mem_ss.add(when: 'CONFIG_FUZZ', if_true: files('sparse-mem.c'))
> >  mem_ss.add(when: 'CONFIG_DIMM', if_true: files('pc-dimm.c'))
> >  mem_ss.add(when: 'CONFIG_NPCM7XX', if_true: files('npcm7xx_mc.c'))
> >  mem_ss.add(when: 'CONFIG_NVDIMM', if_true: files('nvdimm.c'))
> > diff --git a/hw/mem/sparse-mem.c b/hw/mem/sparse-mem.c
> > new file mode 100644
> > index 0000000000..575a287f59
> > --- /dev/null
> > +++ b/hw/mem/sparse-mem.c
> > @@ -0,0 +1,152 @@
> > +/*
> > + * A sparse memory device. Useful for fuzzing
> > + *
> > + * Copyright Red Hat Inc., 2021
> > + *
> > + * Authors:
> > + *  Alexander Bulekov   <alxndr@bu.edu>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or 
> > later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +
> > +#include "exec/address-spaces.h"
> > +#include "hw/qdev-properties.h"
> > +#include "hw/sysbus.h"
> > +#include "qapi/error.h"
> > +#include "qemu/units.h"
> > +#include "sysemu/qtest.h"
> > +#include "hw/mem/sparse-mem.h"
> > +
> > +#define SPARSE_MEM(obj) OBJECT_CHECK(SparseMemState, (obj), 
> > TYPE_SPARSE_MEM)
> > +#define SPARSE_BLOCK_SIZE 0x1000
> 
> This is assuming a 4K block size, should that be the same as the system
> pagesize is? Or will it not matter w.r.t. how this is being consumed?
> 

It shouldn't make a difference, as long as it is a multiple of the
MMIO region's alignment size.

> > +
> > +typedef struct SparseMemState {
> > +    SysBusDevice parent_obj;
> > +    MemoryRegion mmio;
> > +    uint64_t baseaddr;
> > +    uint64_t length;
> > +    uint64_t size_used;
> > +    uint64_t maxsize;
> > +    GHashTable *mapped;
> > +} SparseMemState;
> > +
> > +typedef struct sparse_mem_block {
> > +    uint8_t data[SPARSE_BLOCK_SIZE];
> > +} sparse_mem_block;
> > +
> > +static uint64_t sparse_mem_read(void *opaque, hwaddr addr, unsigned int 
> > size)
> > +{
> > +    printf("SPARSEREAD %lx\n", addr);
> 
> Should this printf() be a logging/trace call? Or do you really want it to be
> printed all the time? Also seems out of place before the declaration of
> the variables.

No. I will remove it

> 
> Thanks,
> 
> Darren.