Re: [Qemu-devel] [PATCH v3 01/35] ppc/xive: introduce a XIVE interrupt source model

[David Gibson]

On Mon, Apr 23, 2018 at 09:11:28AM +0200, Cédric Le Goater wrote:
> On 04/23/2018 05:59 AM, David Gibson wrote:
> > On Fri, Apr 20, 2018 at 10:27:21AM +0200, Cédric Le Goater wrote:
> >> On 04/20/2018 09:10 AM, David Gibson wrote:
> >>> On Thu, Apr 19, 2018 at 02:42:57PM +0200, Cédric Le Goater wrote:
> >>>> Each XIVE interrupt source is associated with a two bit state machine
> >>>> called an Event State Buffer (ESB) : the first bit "P" means that an
> >>>> interrupt is "pending" and waiting for an EOI and the bit "Q" (queued)
> >>>> means a new interrupt was triggered while another was still pending.
> >>>>
> >>>> When an event is triggered, the associated interrupt state bits are
> >>>> fetched and modified and forwarded to the virtualization engine of the
> >>>> controller doing the routing. These can also be controlled by MMIO, to
> >>>> trigger events or turn off the sources for instance. See code for more
> >>>> details on the states and transitions.
> >>>>
> >>>> On a sPAPR machine, the OS will obtain the address of the MMIO page of
> >>>> the ESB entry associated with a source and its characteristic using
> >>>> the H_INT_GET_SOURCE_INFO hcall. On PowerNV, a similar OPAL call is
> >>>> used.
> >>>>
> >>>> The xive_source_notify() routine is in charge forwarding the source
> >>>> event notification to the routing engine. It will be filled later on.
> >>>>
> >>>> Signed-off-by: Cédric Le Goater <address@hidden>
> >>>> ---
> >>>>  Changes since v2:
> >>>>
> >>>>  - added support for Store EOI
> >>>>  - added support for two page MMIO setting like on KVM
> >>>
> >>> Looks generally sane to me, though I have a few queries.
[snip]
> >>>> + * The default XIVE interrupt source setting for ESB MMIO is two 64k
> >>>> + * pages without Store EOI. This is in sync with KVM.
> >>>> + */
> >>>> +static Property xive_source_properties[] = {
> >>>> +    DEFINE_PROP_UINT64("flags", XiveSource, esb_flags, 0),
> >>>> +    DEFINE_PROP_UINT32("nr-irqs", XiveSource, nr_irqs, 0),
> >>>> +    DEFINE_PROP_UINT64("bar", XiveSource, esb_base, 0),
> >>>
> >>> Isn't this redundant with however the base address is handled through
> >>> the SysBusDevice stuff (I forget the details)?
> >>
> >> Storing the ESB MMIO base address under the XiveSource object is 
> >> convenient later on in the h_int_get_source_info hcall which make
> >> use of the helpers : 
> >>
> >>    hwaddr xive_source_esb_trigger(XiveSource *xsrc, uint32_t srcno)
> >>    hwaddr xive_source_esb_mgmt(XiveSource *xsrc, int srcno)
> >>
> >> But it is only used in that place. So we could just store the ESB 
> >> MMIO base address under the sPAPRXive controller. This makes some
> >> sense in the design, as we have to inform KVM of this address with
> >> a KVM device ioctl.
> > 
> > Well.. I really dislike the idea that you could change the actual MMIO
> > mapping address in one place, but other bits of code would still think
> > it was mapped somewhere else.
> 
> OK. I think that my last proposal of removing the ESB MMIO address 
> from the source and letting the owning device, the sPAPR Xive controller 
> in this case, but this is the same for PoweNV or PSI HB, handle the
> mapping goes in the direction you want to take ?  
> 
> It does looks better in the overall XIVE model and the XiveSource
> object have no need of this address.

Yes, I think that's the way to go.

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