Re: [Qemu-devel] [PATCH v4 15/20] ppc/xics: Add "native" XICS subclass

2016-10-16 Thread David Gibson 
On Fri, Oct 14, 2016 at 11:40:30AM +0200, Cédric Le Goater wrote:
> On 10/14/2016 08:10 AM, David Gibson wrote:
> > On Mon, Oct 03, 2016 at 09:24:51AM +0200, Cédric Le Goater wrote:
> >> From: Benjamin Herrenschmidt 
> >>
> >> This provides access to the MMIO based Interrupt Presentation
> >> Controllers (ICP) as found on a POWER8 system.
> >>
> >> A new XICSNative class is introduced to hold the MMIO region of the
> >> ICPs. It also makes use of a hash table to associate the ICPState of a
> >> CPU with a HW processor id, as this is the server number presented in
> >> the XIVEs.
> >>
> >> The class routine 'find_icp' provide the way to do the lookups when
> >> needed in the XICS base class.
> >>
> >> Signed-off-by: Benjamin Herrenschmidt 
> >> [clg: - naming cleanups
> >>   - replaced the use of xics_get_cpu_index_by_dt_id() by 
> >> xics_find_icp()
> >>   - added some qemu logging in case of error  
> >>   - introduced a xics_native_find_icp routine to map icp index to
> >> cpu index  
> >>   - moved sysbus mapping to chip ]
> >> Signed-off-by: Cédric Le Goater 
> >> ---
> >>
> >>  checkpatch complains on this one, but it seems to be a false positive :
> >>  
> >>  ERROR: spaces required around that '&' (ctx:WxV)
> >>  #314: FILE: hw/intc/xics_native.c:246:
> >>  +(gpointer) &xics->ss[cs->cpu_index]);
> >>
> >>  default-configs/ppc64-softmmu.mak |   3 +-
> >>  hw/intc/Makefile.objs |   1 +
> >>  hw/intc/xics_native.c | 327 
> >> ++
> >>  include/hw/ppc/pnv.h  |  19 +++
> >>  include/hw/ppc/xics.h |  24 +++
> >>  5 files changed, 373 insertions(+), 1 deletion(-)
> >>  create mode 100644 hw/intc/xics_native.c
> >>
> >> diff --git a/default-configs/ppc64-softmmu.mak 
> >> b/default-configs/ppc64-softmmu.mak
> >> index 67a9bcaa67fa..a22c93a48686 100644
> >> --- a/default-configs/ppc64-softmmu.mak
> >> +++ b/default-configs/ppc64-softmmu.mak
> >> @@ -48,8 +48,9 @@ CONFIG_PLATFORM_BUS=y
> >>  CONFIG_ETSEC=y
> >>  CONFIG_LIBDECNUMBER=y
> >>  # For pSeries
> >> -CONFIG_XICS=$(CONFIG_PSERIES)
> >> +CONFIG_XICS=$(or $(CONFIG_PSERIES),$(CONFIG_POWERNV))
> >>  CONFIG_XICS_SPAPR=$(CONFIG_PSERIES)
> >> +CONFIG_XICS_NATIVE=$(CONFIG_POWERNV)
> >>  CONFIG_XICS_KVM=$(and $(CONFIG_PSERIES),$(CONFIG_KVM))
> >>  # For PReP
> >>  CONFIG_MC146818RTC=y
> >> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
> >> index 05ec21b21e0e..7be5dfc8347b 100644
> >> --- a/hw/intc/Makefile.objs
> >> +++ b/hw/intc/Makefile.objs
> >> @@ -31,6 +31,7 @@ obj-$(CONFIG_RASPI) += bcm2835_ic.o bcm2836_control.o
> >>  obj-$(CONFIG_SH4) += sh_intc.o
> >>  obj-$(CONFIG_XICS) += xics.o
> >>  obj-$(CONFIG_XICS_SPAPR) += xics_spapr.o
> >> +obj-$(CONFIG_XICS_NATIVE) += xics_native.o
> >>  obj-$(CONFIG_XICS_KVM) += xics_kvm.o
> >>  obj-$(CONFIG_ALLWINNER_A10_PIC) += allwinner-a10-pic.o
> >>  obj-$(CONFIG_S390_FLIC) += s390_flic.o
> >> diff --git a/hw/intc/xics_native.c b/hw/intc/xics_native.c
> >> new file mode 100644
> >> index ..16413d807f65
> >> --- /dev/null
> >> +++ b/hw/intc/xics_native.c
> >> @@ -0,0 +1,327 @@
> >> +/*
> >> + * QEMU PowerPC PowerNV machine model
> >> + *
> >> + * Native version of ICS/ICP
> >> + *
> >> + * Copyright (c) 2016, IBM Corporation.
> >> + *
> >> + * This library is free software; you can redistribute it and/or
> >> + * modify it under the terms of the GNU Lesser General Public
> >> + * License as published by the Free Software Foundation; either
> >> + * version 2 of the License, or (at your option) any later version.
> >> + *
> >> + * This library is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >> + * Lesser General Public License for more details.
> >> + *
> >> + * You should have received a copy of the GNU Lesser General Public
> >> + * License along with this library; if not, see 
> >> .
> >> + */
> >> +
> >> +#include "qemu/osdep.h"
> >> +#include "qapi/error.h"
> >> +#include "qemu-common.h"
> >> +#include "cpu.h"
> >> +#include "hw/hw.h"
> >> +#include "qemu/log.h"
> >> +#include "qapi/error.h"
> >> +
> >> +#include "hw/ppc/fdt.h"
> >> +#include "hw/ppc/xics.h"
> >> +#include "hw/ppc/pnv.h"
> >> +
> >> +#include 
> >> +
> >> +static void xics_native_reset(void *opaque)
> >> +{
> >> +device_reset(DEVICE(opaque));
> >> +}
> >> +
> >> +static void xics_native_initfn(Object *obj)
> >> +{
> >> +XICSState *xics = XICS_COMMON(obj);
> >> +
> >> +QLIST_INIT(&xics->ics);
> > 
> > This shouldn't be necessary, because it's done in the base class
> > initfn (which is called before the subclass initfns).
> 
> True. This is a left over. 
> 
> >> +/*
> >> + * Let's not forget to register a reset handler else the ICPs
> >> + * won't be initialized with the correct values.

Re: [Qemu-devel] [PATCH v4 15/20] ppc/xics: Add "native" XICS subclass

2016-10-14 Thread Cédric Le Goater 
On 10/14/2016 08:10 AM, David Gibson wrote:
> On Mon, Oct 03, 2016 at 09:24:51AM +0200, Cédric Le Goater wrote:
>> From: Benjamin Herrenschmidt 
>>
>> This provides access to the MMIO based Interrupt Presentation
>> Controllers (ICP) as found on a POWER8 system.
>>
>> A new XICSNative class is introduced to hold the MMIO region of the
>> ICPs. It also makes use of a hash table to associate the ICPState of a
>> CPU with a HW processor id, as this is the server number presented in
>> the XIVEs.
>>
>> The class routine 'find_icp' provide the way to do the lookups when
>> needed in the XICS base class.
>>
>> Signed-off-by: Benjamin Herrenschmidt 
>> [clg: - naming cleanups
>>   - replaced the use of xics_get_cpu_index_by_dt_id() by xics_find_icp()
>>   - added some qemu logging in case of error  
>>   - introduced a xics_native_find_icp routine to map icp index to
>> cpu index  
>>   - moved sysbus mapping to chip ]
>> Signed-off-by: Cédric Le Goater 
>> ---
>>
>>  checkpatch complains on this one, but it seems to be a false positive :
>>  
>>  ERROR: spaces required around that '&' (ctx:WxV)
>>  #314: FILE: hw/intc/xics_native.c:246:
>>  +(gpointer) &xics->ss[cs->cpu_index]);
>>
>>  default-configs/ppc64-softmmu.mak |   3 +-
>>  hw/intc/Makefile.objs |   1 +
>>  hw/intc/xics_native.c | 327 
>> ++
>>  include/hw/ppc/pnv.h  |  19 +++
>>  include/hw/ppc/xics.h |  24 +++
>>  5 files changed, 373 insertions(+), 1 deletion(-)
>>  create mode 100644 hw/intc/xics_native.c
>>
>> diff --git a/default-configs/ppc64-softmmu.mak 
>> b/default-configs/ppc64-softmmu.mak
>> index 67a9bcaa67fa..a22c93a48686 100644
>> --- a/default-configs/ppc64-softmmu.mak
>> +++ b/default-configs/ppc64-softmmu.mak
>> @@ -48,8 +48,9 @@ CONFIG_PLATFORM_BUS=y
>>  CONFIG_ETSEC=y
>>  CONFIG_LIBDECNUMBER=y
>>  # For pSeries
>> -CONFIG_XICS=$(CONFIG_PSERIES)
>> +CONFIG_XICS=$(or $(CONFIG_PSERIES),$(CONFIG_POWERNV))
>>  CONFIG_XICS_SPAPR=$(CONFIG_PSERIES)
>> +CONFIG_XICS_NATIVE=$(CONFIG_POWERNV)
>>  CONFIG_XICS_KVM=$(and $(CONFIG_PSERIES),$(CONFIG_KVM))
>>  # For PReP
>>  CONFIG_MC146818RTC=y
>> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
>> index 05ec21b21e0e..7be5dfc8347b 100644
>> --- a/hw/intc/Makefile.objs
>> +++ b/hw/intc/Makefile.objs
>> @@ -31,6 +31,7 @@ obj-$(CONFIG_RASPI) += bcm2835_ic.o bcm2836_control.o
>>  obj-$(CONFIG_SH4) += sh_intc.o
>>  obj-$(CONFIG_XICS) += xics.o
>>  obj-$(CONFIG_XICS_SPAPR) += xics_spapr.o
>> +obj-$(CONFIG_XICS_NATIVE) += xics_native.o
>>  obj-$(CONFIG_XICS_KVM) += xics_kvm.o
>>  obj-$(CONFIG_ALLWINNER_A10_PIC) += allwinner-a10-pic.o
>>  obj-$(CONFIG_S390_FLIC) += s390_flic.o
>> diff --git a/hw/intc/xics_native.c b/hw/intc/xics_native.c
>> new file mode 100644
>> index ..16413d807f65
>> --- /dev/null
>> +++ b/hw/intc/xics_native.c
>> @@ -0,0 +1,327 @@
>> +/*
>> + * QEMU PowerPC PowerNV machine model
>> + *
>> + * Native version of ICS/ICP
>> + *
>> + * Copyright (c) 2016, IBM Corporation.
>> + *
>> + * This library is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2 of the License, or (at your option) any later version.
>> + *
>> + * This library is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with this library; if not, see 
>> .
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "qemu-common.h"
>> +#include "cpu.h"
>> +#include "hw/hw.h"
>> +#include "qemu/log.h"
>> +#include "qapi/error.h"
>> +
>> +#include "hw/ppc/fdt.h"
>> +#include "hw/ppc/xics.h"
>> +#include "hw/ppc/pnv.h"
>> +
>> +#include 
>> +
>> +static void xics_native_reset(void *opaque)
>> +{
>> +device_reset(DEVICE(opaque));
>> +}
>> +
>> +static void xics_native_initfn(Object *obj)
>> +{
>> +XICSState *xics = XICS_COMMON(obj);
>> +
>> +QLIST_INIT(&xics->ics);
> 
> This shouldn't be necessary, because it's done in the base class
> initfn (which is called before the subclass initfns).

True. This is a left over. 

>> +/*
>> + * Let's not forget to register a reset handler else the ICPs
>> + * won't be initialized with the correct values. Trouble ahead !
>> + */
>> +qemu_register_reset(xics_native_reset, xics);
> 
> And. this shouldn't be necessary.  If you set dc->reset to the right
> thing in class_init (which the xics base class should already do) then
> the device model will automatically reset the device, you shouldn't
> need an extra reset handler.

Re: [Qemu-devel] [PATCH v4 15/20] ppc/xics: Add "native" XICS subclass

2016-10-14 Thread David Gibson 
On Mon, Oct 03, 2016 at 09:24:51AM +0200, Cédric Le Goater wrote:
> From: Benjamin Herrenschmidt 
> 
> This provides access to the MMIO based Interrupt Presentation
> Controllers (ICP) as found on a POWER8 system.
> 
> A new XICSNative class is introduced to hold the MMIO region of the
> ICPs. It also makes use of a hash table to associate the ICPState of a
> CPU with a HW processor id, as this is the server number presented in
> the XIVEs.
> 
> The class routine 'find_icp' provide the way to do the lookups when
> needed in the XICS base class.
> 
> Signed-off-by: Benjamin Herrenschmidt 
> [clg: - naming cleanups
>   - replaced the use of xics_get_cpu_index_by_dt_id() by xics_find_icp()
>   - added some qemu logging in case of error  
>   - introduced a xics_native_find_icp routine to map icp index to
> cpu index  
>   - moved sysbus mapping to chip ]
> Signed-off-by: Cédric Le Goater 
> ---
> 
>  checkpatch complains on this one, but it seems to be a false positive :
>  
>  ERROR: spaces required around that '&' (ctx:WxV)
>  #314: FILE: hw/intc/xics_native.c:246:
>  +(gpointer) &xics->ss[cs->cpu_index]);
> 
>  default-configs/ppc64-softmmu.mak |   3 +-
>  hw/intc/Makefile.objs |   1 +
>  hw/intc/xics_native.c | 327 
> ++
>  include/hw/ppc/pnv.h  |  19 +++
>  include/hw/ppc/xics.h |  24 +++
>  5 files changed, 373 insertions(+), 1 deletion(-)
>  create mode 100644 hw/intc/xics_native.c
> 
> diff --git a/default-configs/ppc64-softmmu.mak 
> b/default-configs/ppc64-softmmu.mak
> index 67a9bcaa67fa..a22c93a48686 100644
> --- a/default-configs/ppc64-softmmu.mak
> +++ b/default-configs/ppc64-softmmu.mak
> @@ -48,8 +48,9 @@ CONFIG_PLATFORM_BUS=y
>  CONFIG_ETSEC=y
>  CONFIG_LIBDECNUMBER=y
>  # For pSeries
> -CONFIG_XICS=$(CONFIG_PSERIES)
> +CONFIG_XICS=$(or $(CONFIG_PSERIES),$(CONFIG_POWERNV))
>  CONFIG_XICS_SPAPR=$(CONFIG_PSERIES)
> +CONFIG_XICS_NATIVE=$(CONFIG_POWERNV)
>  CONFIG_XICS_KVM=$(and $(CONFIG_PSERIES),$(CONFIG_KVM))
>  # For PReP
>  CONFIG_MC146818RTC=y
> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
> index 05ec21b21e0e..7be5dfc8347b 100644
> --- a/hw/intc/Makefile.objs
> +++ b/hw/intc/Makefile.objs
> @@ -31,6 +31,7 @@ obj-$(CONFIG_RASPI) += bcm2835_ic.o bcm2836_control.o
>  obj-$(CONFIG_SH4) += sh_intc.o
>  obj-$(CONFIG_XICS) += xics.o
>  obj-$(CONFIG_XICS_SPAPR) += xics_spapr.o
> +obj-$(CONFIG_XICS_NATIVE) += xics_native.o
>  obj-$(CONFIG_XICS_KVM) += xics_kvm.o
>  obj-$(CONFIG_ALLWINNER_A10_PIC) += allwinner-a10-pic.o
>  obj-$(CONFIG_S390_FLIC) += s390_flic.o
> diff --git a/hw/intc/xics_native.c b/hw/intc/xics_native.c
> new file mode 100644
> index ..16413d807f65
> --- /dev/null
> +++ b/hw/intc/xics_native.c
> @@ -0,0 +1,327 @@
> +/*
> + * QEMU PowerPC PowerNV machine model
> + *
> + * Native version of ICS/ICP
> + *
> + * Copyright (c) 2016, IBM Corporation.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> .
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu-common.h"
> +#include "cpu.h"
> +#include "hw/hw.h"
> +#include "qemu/log.h"
> +#include "qapi/error.h"
> +
> +#include "hw/ppc/fdt.h"
> +#include "hw/ppc/xics.h"
> +#include "hw/ppc/pnv.h"
> +
> +#include 
> +
> +static void xics_native_reset(void *opaque)
> +{
> +device_reset(DEVICE(opaque));
> +}
> +
> +static void xics_native_initfn(Object *obj)
> +{
> +XICSState *xics = XICS_COMMON(obj);
> +
> +QLIST_INIT(&xics->ics);

This shouldn't be necessary, because it's done in the base class
initfn (which is called before the subclass initfns).

> +/*
> + * Let's not forget to register a reset handler else the ICPs
> + * won't be initialized with the correct values. Trouble ahead !
> + */
> +qemu_register_reset(xics_native_reset, xics);

And. this shouldn't be necessary.  If you set dc->reset to the right
thing in class_init (which the xics base class should already do) then
the device model will automatically reset the device, you shouldn't
need an extra reset handler.

> +}
> +
> +static uint64_t xics_native_read(void *opaque, hwaddr addr, unsigned width)
> +{
> +XICSState *s = opaque;
> +uint32_t cpu_id = (addr & (PNV_XICS_SIZE - 1)) >> 12;
> +bool byte0 = (w