Re: [PATCH 20/20] spapr: Eliminate SpaprIrq::init hook

From: David Gibson <david@gibson.dropbear.id.au>
To: "Cédric Le Goater" <clg@kaod.org>
Cc: "Jason Wang" <jasowang@redhat.com>,
	"Riku Voipio" <riku.voipio@iki.fi>,
	qemu-devel@nongnu.org, groug@kaod.org, qemu-ppc@nongnu.org,
	"Marc-André Lureau" <marcandre.lureau@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	philmd@redhat.com, "Laurent Vivier" <laurent@vivier.eu>
Subject: Re: [PATCH 20/20] spapr: Eliminate SpaprIrq::init hook
Date: Thu, 26 Sep 2019 21:29:13 +1000	[thread overview]
Message-ID: <20190926112913.GY17405@umbus> (raw)
In-Reply-To: <92ce15dd-f7f9-3d2b-4226-9693bd9cfd65@kaod.org>

[-- Attachment #1: Type: text/plain, Size: 4199 bytes --]

On Thu, Sep 26, 2019 at 09:05:56AM +0200, Cédric Le Goater wrote:
> >>> +    if (spapr->irq->xive) {
> >>> +        uint32_t nr_servers = spapr_max_server_number(spapr);
> >>> +        DeviceState *dev;
> >>> +        int i;
> >>> +
> >>> +        dev = qdev_create(NULL, TYPE_SPAPR_XIVE);
> >>> +        qdev_prop_set_uint32(dev, "nr-irqs",
> >>> +                             spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE);
> >>> +        /*
> >>> +         * 8 XIVE END structures per CPU. One for each available
> >>> +         * priority
> >>> +         */
> >>> +        qdev_prop_set_uint32(dev, "nr-ends", nr_servers << 3);
> >>> +        qdev_init_nofail(dev);
> >>> +
> >>> +        spapr->xive = SPAPR_XIVE(dev);
> >>> +
> >>> +        /* Enable the CPU IPIs */
> >>> +        for (i = 0; i < nr_servers; ++i) {
> >>> +            Error *local_err = NULL;
> >>> +
> >>> +            spapr_xive_irq_claim(spapr->xive, SPAPR_IRQ_IPI + i, false, &local_err);
> >>> +            if (local_err) {
> >>> +                goto out;
> >>> +            }
> >>> +        }
> >>
> >> We could move the IPI claim part in the realize routine of SPAPR_XIVE.
> > 
> > Yeah, I know.  I thought about this, but there's a slight complication
> > in that the XIVE part doesn't know nr_servers directly.  There's
> > several possible ways to handle that, but I wasn't 100% happy with any
> > that I came up with yet.
> 
> The "nr-ends" property was inappropriate, "nr-servers" would have been
> better and we would have hidden the calculation of ENDs 'nr_servers << 3'
> in the realize routine of SpaprXive. 

Ah, interesting.

> I don't think we can change that without breaking migration though
> :/

Hm, there might be a way around it, I'll see what I can do, but
probably as a rather later patch.

> 
> C.
> 
> >>
> >>> +        spapr_xive_hcall_init(spapr);
> >>
> >> This also.
> > 
> > Right.
> > 
> >> It can be done later one.
> > 
> > That's my intention.
> > 
> >>
> >> C.
> >>
> >>> +    }
> >>>  
> >>>      spapr->qirqs = qemu_allocate_irqs(spapr->irq->set_irq, spapr,
> >>>                                        spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE);
> >>> +
> >>> +out:
> >>> +    error_propagate(errp, local_err);
> >>>  }
> >>>  
> >>>  void spapr_irq_claim(SpaprMachineState *spapr, int irq, bool lsi, Error **errp)
> >>> @@ -757,7 +744,6 @@ SpaprIrq spapr_irq_xics_legacy = {
> >>>      .xics        = true,
> >>>      .xive        = false,
> >>>  
> >>> -    .init        = spapr_irq_init_xics,
> >>>      .claim       = spapr_irq_claim_xics,
> >>>      .free        = spapr_irq_free_xics,
> >>>      .print_info  = spapr_irq_print_info_xics,
> >>> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> >>> index 6816cb0500..fa862c665b 100644
> >>> --- a/include/hw/ppc/spapr_irq.h
> >>> +++ b/include/hw/ppc/spapr_irq.h
> >>> @@ -42,7 +42,6 @@ typedef struct SpaprIrq {
> >>>      bool        xics;
> >>>      bool        xive;
> >>>  
> >>> -    void (*init)(SpaprMachineState *spapr, Error **errp);
> >>>      void (*claim)(SpaprMachineState *spapr, int irq, bool lsi, Error **errp);
> >>>      void (*free)(SpaprMachineState *spapr, int irq);
> >>>      void (*print_info)(SpaprMachineState *spapr, Monitor *mon);
> >>> diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h
> >>> index 691a6d00f7..267984a97b 100644
> >>> --- a/include/hw/ppc/xics_spapr.h
> >>> +++ b/include/hw/ppc/xics_spapr.h
> >>> @@ -34,6 +34,7 @@
> >>>  #define TYPE_ICS_SPAPR "ics-spapr"
> >>>  #define ICS_SPAPR(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SPAPR)
> >>>  
> >>> +void ics_spapr_create(SpaprMachineState *spapr, int nr_xirqs, Error **errp);
> >>>  void spapr_dt_xics(SpaprMachineState *spapr, uint32_t nr_servers, void *fdt,
> >>>                     uint32_t phandle);
> >>>  int xics_kvm_connect(SpaprMachineState *spapr, Error **errp);
> >>>
> >>
> > 
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]