Re: [Qemu-devel] [PATCH 21/25] spapr: introduce a helper to map the XIVE memory regions

[David Gibson]

On Mon, Dec 04, 2017 at 04:30:36PM +0100, Cédric Le Goater wrote:
> On 12/04/2017 08:52 AM, David Gibson wrote:
> > On Thu, Nov 23, 2017 at 02:29:51PM +0100, Cédric Le Goater wrote:
> >> When the XIVE interrupt mode is activated, the machine needs to expose
> >> to the guest the MMIO regions use by the controller :
> >>
> >>   - Event State Buffer (ESB)
> >>   - Thread Interrupt Management Area (TIMA)
> >>
> >> Migration will also need to reflect the current interrupt mode in use.
> >>
> >> Signed-off-by: Cédric Le Goater <address@hidden>
> >> ---
> >>  hw/intc/spapr_xive_hcall.c  | 14 ++++++++++++++
> >>  hw/ppc/spapr.c              |  5 +++++
> >>  include/hw/ppc/spapr_xive.h |  1 +
> >>  3 files changed, 20 insertions(+)
> >>
> >> diff --git a/hw/intc/spapr_xive_hcall.c b/hw/intc/spapr_xive_hcall.c
> >> index 60c6c9f4be8f..ba217144878e 100644
> >> --- a/hw/intc/spapr_xive_hcall.c
> >> +++ b/hw/intc/spapr_xive_hcall.c
> >> @@ -933,3 +933,17 @@ void spapr_xive_populate(sPAPRMachineState *spapr, 
> >> int nr_servers,
> >>      _FDT(fdt_setprop(fdt, 0, "ibm,plat-res-int-priorities",
> >>                       plat_res_int_priorities, 
> >> sizeof(plat_res_int_priorities)));
> >>  }
> >> +
> >> +void spapr_xive_mmio_map(sPAPRMachineState *spapr)
> >> +{
> >> +    sPAPRXive *xive = spapr->xive;
> >> +
> >> +    /* ESBs */
> >> +    sysbus_mmio_map(SYS_BUS_DEVICE(xive), 0, xive->esb_base);
> >> +
> >> +    /* Thread Management Interrupt Areas */
> >> +    /* TODO: Only map the OS TIMA for the moment. Mapping the whole
> >> +     * region needs some rework in the handlers */
> >> +    sysbus_mmio_map(SYS_BUS_DEVICE(xive), 1,
> >> +                    xive->tm_base + (1 << xive->tm_shift));
> > 
> > You probably shouldn't be exposing the user TIMA in the DT if you're
> > only allowing the OS TIME to be mapped.
> 
> The specs requires to map both Uset and OS TIMA.

Ok.

> > 
> >> +}
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index 3a62369883cc..734706c18cb3 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -1132,6 +1132,7 @@ static void *spapr_build_fdt(sPAPRMachineState 
> >> *spapr,
> >>      } else {
> >>          /* Populate device tree for XIVE */
> >>          spapr_xive_populate(spapr, xics_max_server_number(), fdt, 
> >> PHANDLE_XICP);
> >> +        spapr_xive_mmio_map(spapr);
> > 
> > This doesn't belong here, spapr_build_fdt() should _just_ build the
> > fdt, not have side effects on the actual device state.
> 
> Yes. I will move the rest of the XIVE setup in the reset handler
> before the device tree is built.

Ok.

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