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

[Blue Swirl]

On Mon, Aug 29, 2011 at 5:57 AM, Avi Kivity <address@hidden> wrote:
> 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!

The use case is valid, to help with testing and debugging. But this is
already a bit baroque and I foresee an endless stream of hacks in this
area.

Maybe the device should be as simple as possible but then connect to
some kind of external test program or plugin? Then that one could
include a beowulf cluster of PDF test report generators or even
Eclipse and we wouldn't care less here.

Even better, taking the instrumentation approach, could the test
device be left out completely, just use guest invisible methods (like
watchpoints) to interact with the guest?