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

[Cédric Le Goater]

On 07/25/2017 02:21 PM, David Gibson wrote:
> 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.

To ease the modeling, the ESB states of all IRQs are under the same
bit array and they are exposed to the guest OS through a single memory 
region so that all accesses to the ESB states are centralized. On real 
HW, the ESB cache and the MMIO base depends on the source. 
 
Anyway, that might not be an issue. I will see what I can do to 
decorrelate the XIVE source from the main XIVE object. We can 
probably move the ESB state array below the XIVE source.

C.

>> 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;
>>>>  };
>>>>  
>>>
>>
>