Re: [Qemu-ppc] [RFC PATCH 07/26] ppc/xive: add MMIO handlers to the XIVE interrupt source

[David Gibson]

On Mon, Jul 24, 2017 at 05:55:42PM +0200, Cédric Le Goater wrote:
> On 07/24/2017 06:29 AM, David Gibson wrote:
> > On Wed, Jul 05, 2017 at 07:13:20PM +0200, Cédric Le Goater wrote:
> >> Each interrupt source is associated with a 2-bit state machine called
> >> an Event State Buffer (ESB). It is controlled by MMIO to trigger
> >> events.
[snip]
> >> +/* TODO: handle second page */
> > 
> > Is this comment still relevent?
> 
> Some HW have a second page to trigger the event. I am not sure we need 
> to model it though. I will make some inquiries.

Ah, ok.  Maybe clarify the comment a bit.

[snip]
> >> +static void xive_esb_write(void *opaque, hwaddr addr,
> >> +                           uint64_t value, unsigned size)
> >> +{
> >> +    XiveICSState *xs = ICS_XIVE(opaque);
> >> +    XIVE *x = xs->xive;
> >> +    uint32_t offset = addr & 0xF00;
> >> +    uint32_t srcno = addr >> xs->esb_shift;
> >> +    uint32_t lisn = srcno + ICS_BASE(xs)->offset;
> >> +    XiveIVE *ive;
> >> +    bool notify = false;
> >> +
> >> +    ive = xive_get_ive(x, lisn);
> >> +    if (!ive || !(ive->w & IVE_VALID))  {
> >> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid LISN %d\n", lisn);
> >> +        return;
> >> +    }
> > 
> > Having this code associated with the individual ICS look directly at
> > the IVE table in the core xive object seems a bit dubious.
> 
> The IVE table holds the validity and mask status of the interrupt 
> entries, so we need that lookup. However, (continues below) ...
> 
> > This also
> > points out another mismatch between the re-used ICS code and the new
> > XIVE code: ICS gathers all the per-source-irq flags/state into the
> > irqstate structure, whereas xive has per-irq information in the
> > centralized ecb and IVE tables.  There can certainly be good reasons
> > for that, but using both at once is kind of clunky.
> 
> I understand that you would rather put the esbs in the source they 
> belong to. That is the case on real HW but it makes the modeling a 
> bit more difficult. We would need to choose a MMIO address to give 
> to the guest OS. I had some issues with the allocator (I need 
> to look at this problem closer).

Uh.. what do MMIO addresses have to do with this?  I'm talking about
the actual ESB state in the packed bit array.

> It might also be an "issue" for KVM. Ben talked about maintaining 
> all the esbs of a guest under a single memory region to be able to 
> map the pages in the host.
> 
> Any how, I agree this is another point to discuss in the sPAPR 
> model.
> 
> Thanks,
> 
> C. 
> 
> 
> >> +    if (srcno >= ICS_BASE(xs)->nr_irqs) {
> >> +        qemu_log_mask(LOG_GUEST_ERROR,
> >> +                      "XIVE: invalid IRQ number: %d/%d lisn: %d\n",
> >> +                      srcno, ICS_BASE(xs)->nr_irqs, lisn);
> >> +        return;
> >> +    }
> >> +
> >> +    switch (offset) {
> >> +    case 0:
> >> +        /* TODO: should we trigger even if the IVE is masked ? */
> >> +        notify = xive_pq_trigger(x, lisn);
> >> +        break;
> >> +    default:
> >> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid ESB write addr 
> >> %d\n",
> >> +                      offset);
> >> +        return;
> >> +    }
> >> +
> >> +    if (notify && !(ive->w & IVE_MASKED)) {
> >> +        qemu_irq_pulse(ICS_BASE(xs)->qirqs[srcno]);
> >> +    }
> >> +}
> >> +
> >> +static const MemoryRegionOps xive_esb_ops = {
> >> +    .read = xive_esb_read,
> >> +    .write = xive_esb_write,
> >> +    .endianness = DEVICE_BIG_ENDIAN,
> >> +    .valid = {
> >> +        .min_access_size = 8,
> >> +        .max_access_size = 8,
> >> +    },
> >> +    .impl = {
> >> +        .min_access_size = 8,
> >> +        .max_access_size = 8,
> >> +    },
> >> +};
> >> +
> >> +/*
> >>   * XIVE Interrupt Source
> >>   */
> >>  static void xive_ics_set_irq_msi(XiveICSState *xs, int srcno, int val)
> >> @@ -106,15 +326,25 @@ static void xive_ics_realize(ICSState *ics, Error 
> >> **errp)
> >>          return;
> >>      }
> >>  
> >> +    if (!xs->esb_shift) {
> >> +        error_setg(errp, "ESB page size needs to be greater 0");
> >> +        return;
> >> +    }
> >> +
> >>      ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
> >>      ics->qirqs = qemu_allocate_irqs(xive_ics_set_irq, xs, ics->nr_irqs);
> >>  
> >> +    memory_region_init_io(&xs->esb_iomem, OBJECT(xs), &xive_esb_ops, xs,
> >> +                          "xive.esb",
> >> +                          (1ull << xs->esb_shift) * 
> >> ICS_BASE(xs)->nr_irqs);
> >> +
> >>      qemu_register_reset(xive_ics_reset, xs);
> >>  }
> >>  
> >>  static Property xive_ics_properties[] = {
> >>      DEFINE_PROP_UINT32("nr-irqs", ICSState, nr_irqs, 0),
> >>      DEFINE_PROP_UINT32("irq-base", ICSState, offset, 0),
> >> +    DEFINE_PROP_UINT32("shift", XiveICSState, esb_shift, 0),
> >>      DEFINE_PROP_END_OF_LIST(),
> >>  };
> >>  
> >> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> >> index 544cc6e0c796..5303d96f5f59 100644
> >> --- a/include/hw/ppc/xive.h
> >> +++ b/include/hw/ppc/xive.h
> >> @@ -33,6 +33,9 @@ typedef struct XiveICSState XiveICSState;
> >>  struct XiveICSState {
> >>      ICSState parent_obj;
> >>  
> >> +    uint32_t     esb_shift;
> >> +    MemoryRegion esb_iomem;
> >> +
> >>      XIVE         *xive;
> >>  };
> >>  
> > 
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature