Re: [Qemu-devel] [PATCH] hw: Add test device for unittests execution

[Avi Kivity <avi@redhat.com>]

On 08/27/2011 07:22 PM, Blue Swirl wrote:
> >  +
> >  +static void test_device_irq_line(void *opaque, uint32_t addr, uint32_t data)
> >  +{
> >  +    qemu_set_irq(isa_get_irq(addr - 0x2000), !!data);
>
> Where does 0x2000 come from?

The base address of this range.

> >  +
> >  +static uint32_t test_iomem_readw(void *opaque, target_phys_addr_t addr)
> >  +{
> >  +    return *(uint16_t*)(iomem_buf + addr);
> >  +}
>
> This and the other functions assume that the memory is available and
> the guest and the host are of same endianness.
>
> This looks like pure RAM, so MMIO is not the best way to do this.
> Please just map more RAM.

The intent is to get the guest to fault and exercise kvm's instruction 
emulator.  That won't happen with RAM.

(well, with a sub-page mapping, it should)


> What's the use of this anyway, it doesn't affect QEMU in any way? Scratch space?
>
>
> >  +static int init_test_device(ISADevice *isa)
> >  +{
> >  +    struct testdev *dev = DO_UPCAST(struct testdev, dev, isa);
> >  +    int iomem;
> >  +
> >  +    register_ioport_write(0xf1, 1, 1, test_device_serial_write, dev);
> >  +    register_ioport_write(0xf4, 1, 4, test_device_exit, dev);
> >  +    register_ioport_read(0xd1, 1, 4, test_device_memsize_read, dev);
> >  +    register_ioport_read(0xe0, 1, 1, test_device_ioport_read, dev);
> >  +    register_ioport_write(0xe0, 1, 1, test_device_ioport_write, dev);
> >  +    register_ioport_read(0xe0, 1, 2, test_device_ioport_read, dev);
> >  +    register_ioport_write(0xe0, 1, 2, test_device_ioport_write, dev);
> >  +    register_ioport_read(0xe0, 1, 4, test_device_ioport_read, dev);
> >  +    register_ioport_write(0xe0, 1, 4, test_device_ioport_write, dev);
> >  +    register_ioport_write(0xe4, 1, 4, test_device_flush_page, dev);
> >  +    register_ioport_write(0x2000, 24, 1, test_device_irq_line, NULL);
>
> 24? Doesn't ISA have only 16? Enums for all constants would be more readable.

GSI space - the ioapic pins.  It's really a motherboard device.

>
> >  +    iomem_buf = g_malloc0(0x10000);
> >  +    iomem = cpu_register_io_memory(test_iomem_read, test_iomem_write, NULL,
> >  +                                   DEVICE_NATIVE_ENDIAN);
> >  +    cpu_register_physical_memory(0xff000000, 0x10000, iomem);
>
> Devices may not map themselves, this should be done at board level.
> Doesn't this address also conflict with PCI for PC?

Probably.  The whole thing is a hack!

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.