Re: [Qemu-devel] [PATCH v3 2/3] hw/intc: Add (new) ASPEED VIC device model

[Andrew Jeffery]

On Fri, 2016-03-11 at 16:03 +0700, Peter Maydell wrote:
> On 5 March 2016 at 11:29, Andrew Jeffery <address@hidden> wrote:
> > Implement a basic ASPEED VIC device model, enough to boot a Linux kernel
> > configured with aspeed_defconfig. The model implements the 'new'
> > (revised) register set and while the hardware exposes both the new and
> > legacy register sets, accesses to the legacy register set will not
> > be serviced (though the access will be logged).
> > 
> > Signed-off-by: Andrew Jeffery <address@hidden>
> 
> > +static void aspeed_vic_write(void *opaque, hwaddr offset, uint64_t data,
> > +                             unsigned size)
> > +{
> > +    const bool high = !!(offset & 0x4);
> > +    hwaddr n_offset = (offset & ~0x4);
> > +    AspeedVICState *s = (AspeedVICState *)opaque;
> > +
> > +    if (offset < AVIC_NEW_BASE_OFFSET) {
> > +        qemu_log_mask(LOG_UNIMP,
> > +                      "%s: Ignoring write to legacy registers at 0x%"
> > +                      HWADDR_PRIx "[%u] <- 0x%" PRIx64 "\n", __func__, 
> > offset,
> > +                      size, data);
> > +        return;
> > +    }
> > +
> > +    n_offset -= AVIC_NEW_BASE_OFFSET;
> > +    trace_aspeed_vic_write(offset, size, data);
> > +
> > +    /* Given we have members using separate enable/clear registers, 
> > deposit64()
> > +     * isn't quite the tool for the job. Instead, relocate the incoming 
> > bits to
> > +     * the required bit offset based on the provided access address
> > +     */
> > +    if (high) {
> > +        data &= AVIC_H_MASK;
> > +        data <<= 32;
> > +    } else {
> > +        data &= AVIC_L_MASK;
> > +    }
> > +
> > +    switch (n_offset) {
> > +    case 0x18: /* Interrupt Selection */
> > +        /* Register has deposit64() semantics - overwrite requested 32 
> > bits */
> > +        if (high) {
> > +            s->select &= AVIC_L_MASK;
> > +        } else {
> > +            s->select &= ((uint64_t) AVIC_H_MASK) << 32;
> > +        }
> > +        s->select |= data;
> > +        break;
> > +    case 0x20: /* Interrupt Enable */
> > +        s->enable |= data;
> > +        break;
> > +    case 0x28: /* Interrupt Enable Clear */
> > +        s->enable &= ~data;
> > +        break;
> > +    case 0x30: /* Software Interrupt */
> > +        qemu_log_mask(LOG_UNIMP, "%s: Software interrupts unavailable. "
> > +                "IRQs requested: 0x%016" PRIx64 "\n", __func__, data);
> > +        break;
> > +    case 0x38: /* Software Interrupt Clear */
> > +        qemu_log_mask(LOG_UNIMP, "%s: Software interrupts unavailable. "
> > +                "IRQs to be cleared: 0x%016" PRIx64 "\n", __func__, data);
> > +        break;
> > +    case 0x50: /* Interrupt Event */
> > +        /* Register has deposit64() semantics - overwrite the top four 
> > valid
> > +         * IRQ bits, as only the top four IRQs (GPIOs) can change their 
> > event
> > +         * type */
> > +        g_assert(high);
> 
> Don't assert on conditions that can be triggered by a guest.

Good point, I'll change it to qemu_log_mask(LOG_GUEST_ERROR, ...)

> 
> > +        s->event &= ~AVIC_EVENT_W_MASK;
> > +        s->event |= (data & AVIC_EVENT_W_MASK);
> > +        break;
> > +    case 0x58: /* Edge Triggered Interrupt Clear */
> > +        s->raw &= ~(data & ~s->sense);
> > +        break;
> > +    case 0x00: /* IRQ Status */
> > +    case 0x08: /* FIQ Status */
> > +    case 0x10: /* Raw Interrupt Status */
> > +    case 0x40: /* Interrupt Sensitivity */
> > +    case 0x48: /* Interrupt Both Edge Trigger Control */
> > +    case 0x60: /* Edge Triggered Interrupt Status */
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: Write of read-only register with offset 0x%"
> > +                      HWADDR_PRIx "\n", __func__, offset);
> > +        break;
> > +
> > +    default:
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: Bad register at offset 0x%" HWADDR_PRIx "\n",
> > +                      __func__, offset);
> > +        break;
> > +    }
> > +    aspeed_vic_update(s);
> > +}
> 
> Otherwise
> Reviewed-by: Peter Maydell <address@hidden>

Thanks,

Andrew
> 
> thanks
> -- PMM

signature.asc
Description: This is a digitally signed message part