Re: [Qemu-devel] [PATCH 2/6] [RFC] Emulation of GRLIB IRQMP as defined in GRLIB IP Core User's Manual.

[Blue Swirl]

On Tue, Dec 7, 2010 at 10:43 AM, Fabien Chouteau <address@hidden> wrote:
> On 12/06/2010 06:25 PM, Blue Swirl wrote:
>>
>> On Mon, Dec 6, 2010 at 9:26 AM, Fabien Chouteau<address@hidden>
>>  wrote:
>>>
>>> Signed-off-by: Fabien Chouteau<address@hidden>
>>> ---
>>>  hw/grlib_irqmp.c |  416
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 files changed, 416 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/hw/grlib_irqmp.c b/hw/grlib_irqmp.c
>>> new file mode 100644
>>> index 0000000..69e1553
>>> --- /dev/null
>>> +++ b/hw/grlib_irqmp.c
>>> @@ -0,0 +1,416 @@
>>> +/*
>>> + * QEMU GRLIB IRQMP Emulator
>>> + *
>>> + * (Multiprocessor and extended interrupt not supported)
>>> + *
>>> + * Copyright (c) 2010 AdaCore
>>> + *
>>> + * 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 "sysbus.h"
>>> +#include "cpu.h"
>>> +
>>> +#include "grlib.h"
>>> +
>>> +/* #define DEBUG_IRQ */
>>> +
>>> +#ifdef DEBUG_IRQ
>>> +#define DPRINTF(fmt, ...)                                       \
>>> +    do { printf("IRQMP: " fmt , ## __VA_ARGS__); } while (0)
>>> +#else
>>> +#define DPRINTF(fmt, ...)
>>> +#endif
>>> +
>>> +#define IRQMP_MAX_CPU 16
>>> +#define IRQMP_REG_SIZE 256      /* Size of memory mapped registers */
>>> +
>>> +/* Memory mapped register offsets */
>>> +#define LEVEL_OFFSET     0x00
>>> +#define PENDING_OFFSET   0x04
>>> +#define FORCE0_OFFSET    0x08
>>> +#define CLEAR_OFFSET     0x0C
>>> +#define MP_STATUS_OFFSET 0x10
>>> +#define BROADCAST_OFFSET 0x14
>>> +#define MASK_OFFSET      0x40
>>> +#define FORCE_OFFSET     0x80
>>> +#define EXTENDED_OFFSET  0xC0
>>> +
>>> +typedef struct IRQMP
>>> +{
>>> +    SysBusDevice busdev;
>>> +
>>> +    CPUSPARCState *env;
>>
>> Devices should never access CPUState directly. Instead, board level
>> should create CPU irqs and these should then be passed here.
>>
>
> This case is special, Leon3 is a System-On-Chip and some of the components
> are very close to the processor.
> IRQMP is not really a peripheral nor a part of the CPU, it's both...

It's not a special case, it could be easily implemented separately.
MMUs, FPUs or co-processors could be special even if they have been
implemented as separate chips with real hardware. But we are actually
not looking at the (historical or current) chip boundaries but more
like what makes sense from QEMU architecture point of view.

>>> +} IRQMP;
>>> +
>>> +typedef struct IRQMPState
>>> +{
>>> +    uint32_t level;
>>> +    uint32_t pending;
>>> +    uint32_t clear;
>>> +    uint32_t broadcast;
>>> +
>>> +    uint32_t mask[IRQMP_MAX_CPU];
>>> +    uint32_t force[IRQMP_MAX_CPU];
>>> +    uint32_t extended[IRQMP_MAX_CPU];
>>> +
>>> +    IRQMP    *parent;
>>> +} IRQMPState;
>>> +
>>> +IRQMPState grlib_irqmp_state;
>>
>> Global state indicates poor design. Why separate IRQMP and IRQMPState?
>
> I have to access IRQMPState in grlib_irqmp_ack and grlib_irqmp_check_irqs,
> but I don't see how I can do it without a global variable.
> Again, I think that it's related to the special case of IRQMP.

Adding another set of signals for ack, going from board level to the
device should solve the problem cleanly.

>>> +
>>> +void grlib_irqmp_set_irq(void *opaque, int irq, int level);
>>
>> This should not be global. Again, creating qemu_irqs or moving some of
>> the code to board level should help.
>
> This one should be static indeed.
>
>>> +
>>> +DeviceState *grlib_irqmp_create(target_phys_addr_t   base,
>>> +                                CPUState            *env,
>>> +                                qemu_irq           **cpu_irqs,
>>> +                                uint32_t             nr_irqs)
>>> +{
>>> +    DeviceState *dev;
>>> +
>>> +    assert(cpu_irqs != NULL);
>>> +
>>> +    dev = qdev_create(NULL, "grlib,irqmp");
>>> +    qdev_prop_set_ptr(dev, "cpustate", env);
>>> +
>>> +    if (qdev_init(dev)) {
>>> +        return NULL;
>>> +    }
>>> +
>>> +    sysbus_mmio_map(sysbus_from_qdev(dev), 0, base);
>>> +
>>> +    *cpu_irqs = qemu_allocate_irqs(grlib_irqmp_set_irq,
>>> +&grlib_irqmp_state,
>>> +                                   nr_irqs);
>>> +
>>> +    return dev;
>>> +}
>>> +
>>> +static void grlib_irqmp_check_irqs(CPUState *env)
>>> +{
>>> +    uint32_t pend   = 0;
>>> +    uint32_t level0 = 0;
>>> +    uint32_t level1 = 0;
>>> +
>>> +    assert(env != NULL);
>>> +
>>> +    /* IRQ for CPU 0 (no SMP support) */
>>> +    pend = (grlib_irqmp_state.pending | grlib_irqmp_state.force[0])
>>> +&  grlib_irqmp_state.mask[0];
>>> +
>>> +
>>> +    level0 = pend&  ~grlib_irqmp_state.level;
>>> +    level1 = pend&    grlib_irqmp_state.level;
>>> +
>>> +    DPRINTF("pend:0x%04x force:0x%04x mask:0x%04x lvl1:0x%04x
>>> lvl0:0x%04x\n",
>>> +            grlib_irqmp_state.pending, grlib_irqmp_state.force[0],
>>> +            grlib_irqmp_state.mask[0], level1, level0);
>>
>> The above should stay here, but code below should to go to board level
>> (leon3.c). Then you need to separate device IRQ handling from CPU PIL
>> handling.
>
> If I want to use IRQMP for another machine I will have to duplicate the
> code.
> So I think it is the right place for this this code.

Maybe with the ack signals you can also keep this here.