Re: [Qemu-devel] [PATCH v7 09/11] hw/kvm/arm_gic: Implement support for KVM in-kernel ARM GIC

Re: [Qemu-devel] [PATCH v7 09/11] hw/kvm/arm_gic: Implement support for KVM in-kernel ARM GIC

[Andreas Färber]

Am 26.02.2013 18:40, schrieb Peter Maydell:
> Implement support for using the KVM in-kernel GIC for ARM.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Andreas Färber <afaerber@suse.de>

Thanks,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v7 07/11] hw/arm_gic: Convert ARM GIC classes to use init/realize

[Andreas Färber]

Am 26.02.2013 18:40, schrieb Peter Maydell:
> Convert the ARM GIC classes to use init/realize rather than
> SysBusDevice::init. (We have to do them all in one patch to
> avoid unconverted subclasses calling a nonexistent SysBusDevice
> init function in the base class and crashing.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm_gic.c          |   23 +++++++++++++----------
>  hw/arm_gic_common.c   |   26 +++++++++++++++-----------
>  hw/arm_gic_internal.h |    2 +-
>  hw/armv7m_nvic.c      |   15 ++++++++-------
>  4 files changed, 37 insertions(+), 29 deletions(-)
> 
> diff --git a/hw/arm_gic.c b/hw/arm_gic.c
> index 90e43d0..250e720 100644
> --- a/hw/arm_gic.c
> +++ b/hw/arm_gic.c
> @@ -659,14 +659,18 @@ void gic_init_irqs_and_distributor(GICState *s, int num_irq)
>      memory_region_init_io(&s->iomem, &gic_dist_ops, s, "gic_dist", 0x1000);
>  }
>  
> -static int arm_gic_init(SysBusDevice *dev)
> +static void arm_gic_realize(DeviceState *dev, Error **errp)
>  {
> -    /* Device instance init function for the GIC sysbus device */
> +    /* Device instance realize function for the GIC sysbus device */
>      int i;
> -    GICState *s = FROM_SYSBUS(GICState, dev);
> +    GICState *s = ARM_GIC(dev);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>      ARMGICClass *agc = ARM_GIC_GET_CLASS(s);
>  
> -    agc->parent_init(dev);
> +    agc->parent_realize(dev, errp);
> +    if (error_is_set(errp)) {
> +        return;
> +    }
>  
>      gic_init_irqs_and_distributor(s, s->num_irq);
>  
> @@ -686,22 +690,21 @@ static int arm_gic_init(SysBusDevice *dev)
>                                "gic_cpu", 0x100);
>      }
>      /* Distributor */
> -    sysbus_init_mmio(dev, &s->iomem);
> +    sysbus_init_mmio(sbd, &s->iomem);
>      /* cpu interfaces (one for "current cpu" plus one per cpu) */
>      for (i = 0; i <= NUM_CPU(s); i++) {
> -        sysbus_init_mmio(dev, &s->cpuiomem[i]);
> +        sysbus_init_mmio(sbd, &s->cpuiomem[i]);
>      }
> -    return 0;
>  }
>  
>  static void arm_gic_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> -    SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(klass);
>      ARMGICClass *agc = ARM_GIC_CLASS(klass);
> -    agc->parent_init = sbc->init;
> -    sbc->init = arm_gic_init;
> +
>      dc->no_user = 1;
> +    agc->parent_realize = dc->realize;
> +    dc->realize = arm_gic_realize;
>  }
>  
>  static const TypeInfo arm_gic_info = {
> diff --git a/hw/arm_gic_common.c b/hw/arm_gic_common.c
> index 2947622..3b2955c 100644
> --- a/hw/arm_gic_common.c
> +++ b/hw/arm_gic_common.c
> @@ -104,31 +104,35 @@ static int gic_load(QEMUFile *f, void *opaque, int version_id)
>      return 0;
>  }
>  
> -static int arm_gic_common_init(SysBusDevice *dev)
> +static void arm_gic_common_realize(DeviceState *dev, Error **errp)
>  {
> -    GICState *s = FROM_SYSBUS(GICState, dev);
> +    GICState *s = ARM_GIC_COMMON(dev);
>      int num_irq = s->num_irq;
>  
>      if (s->num_cpu > NCPU) {
> -        hw_error("requested %u CPUs exceeds GIC maximum %d\n",
> -                 s->num_cpu, NCPU);
> +        error_setg(errp, "requested %u CPUs exceeds GIC maximum %d\n",

Please drop \n for error_setg(). Probably would be worth adding to a
convert-to-realize section on the Wiki.

> +                   s->num_cpu, NCPU);
> +        return;
>      }
>      s->num_irq += GIC_BASE_IRQ;
>      if (s->num_irq > GIC_MAXIRQ) {
> -        hw_error("requested %u interrupt lines exceeds GIC maximum %d\n",
> -                 num_irq, GIC_MAXIRQ);
> +        error_setg(errp,
> +                   "requested %u interrupt lines exceeds GIC maximum %d\n",
> +                   num_irq, GIC_MAXIRQ);
> +        return;
>      }
>      /* ITLinesNumber is represented as (N / 32) - 1 (see
>       * gic_dist_readb) so this is an implementation imposed
>       * restriction, not an architectural one:
>       */
>      if (s->num_irq < 32 || (s->num_irq % 32)) {
> -        hw_error("%d interrupt lines unsupported: not divisible by 32\n",
> -                 num_irq);
> +        error_setg(errp,
> +                   "%d interrupt lines unsupported: not divisible by 32\n",
> +                   num_irq);
> +        return;
>      }
>  
>      register_savevm(NULL, "arm_gic", -1, 3, gic_save, gic_load, s);
> -    return 0;
>  }
>  
>  static void arm_gic_common_reset(DeviceState *dev)
> @@ -173,12 +177,12 @@ static Property arm_gic_common_properties[] = {
>  
>  static void arm_gic_common_class_init(ObjectClass *klass, void *data)
>  {
> -    SysBusDeviceClass *sc = SYS_BUS_DEVICE_CLASS(klass);
>      DeviceClass *dc = DEVICE_CLASS(klass);
> +
>      dc->reset = arm_gic_common_reset;
> +    dc->realize = arm_gic_common_realize;
>      dc->props = arm_gic_common_properties;
>      dc->no_user = 1;
> -    sc->init = arm_gic_common_init;
>  }
>  
>  static const TypeInfo arm_gic_common_type = {
> diff --git a/hw/arm_gic_internal.h b/hw/arm_gic_internal.h
> index 3640be0..3ba37f3 100644
> --- a/hw/arm_gic_internal.h
> +++ b/hw/arm_gic_internal.h
> @@ -132,7 +132,7 @@ typedef struct ARMGICCommonClass {
>  
>  typedef struct ARMGICClass {
>      ARMGICCommonClass parent_class;
> -    int (*parent_init)(SysBusDevice *dev);
> +    DeviceRealize parent_realize;
>  } ARMGICClass;
>  
>  #endif /* !QEMU_ARM_GIC_INTERNAL_H */
> diff --git a/hw/armv7m_nvic.c b/hw/armv7m_nvic.c
> index d5798d0..3c79674 100644
> --- a/hw/armv7m_nvic.c
> +++ b/hw/armv7m_nvic.c
> @@ -41,7 +41,7 @@ typedef struct NVICClass {
>      /*< private >*/
>      ARMGICClass parent_class;
>      /*< public >*/
> -    int (*parent_init)(SysBusDevice *dev);
> +    DeviceRealize parent_realize;
>      void (*parent_reset)(DeviceState *dev);
>  } NVICClass;
>  
> @@ -465,7 +465,7 @@ static void armv7m_nvic_reset(DeviceState *dev)
>      systick_reset(s);
>  }
>  
> -static int armv7m_nvic_init(SysBusDevice *dev)
> +static void armv7m_nvic_realize(DeviceState *dev, Error **errp)
>  {
>      nvic_state *s = NVIC(dev);
>      NVICClass *nc = NVIC_GET_CLASS(s);
> @@ -475,7 +475,10 @@ static int armv7m_nvic_init(SysBusDevice *dev)
>      /* Tell the common code we're an NVIC */
>      s->gic.revision = 0xffffffff;
>      s->num_irq = s->gic.num_irq;
> -    nc->parent_init(dev);
> +    nc->parent_realize(dev, errp);
> +    if (error_is_set(errp)) {
> +        return;
> +    }
>      gic_init_irqs_and_distributor(&s->gic, s->num_irq);
>      /* The NVIC and system controller register area looks like this:
>       *  0..0xff : system control registers, including systick
> @@ -503,7 +506,6 @@ static int armv7m_nvic_init(SysBusDevice *dev)
>       */
>      memory_region_add_subregion(get_system_memory(), 0xe000e000, &s->container);
>      s->systick.timer = qemu_new_timer_ns(vm_clock, systick_timer_tick, s);
> -    return 0;
>  }
>  
>  static void armv7m_nvic_instance_init(Object *obj)
> @@ -526,13 +528,12 @@ static void armv7m_nvic_class_init(ObjectClass *klass, void *data)
>  {
>      NVICClass *nc = NVIC_CLASS(klass);
>      DeviceClass *dc = DEVICE_CLASS(klass);
> -    SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
>  
>      nc->parent_reset = dc->reset;
> -    nc->parent_init = sdc->init;
> -    sdc->init = armv7m_nvic_init;
> +    nc->parent_realize = dc->realize;
>      dc->vmsd  = &vmstate_nvic;
>      dc->reset = armv7m_nvic_reset;
> +    dc->realize = armv7m_nvic_realize;
>  }
>  
>  static const TypeInfo armv7m_nvic_info = {
> 

Otherwise looks fine, thanks.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v7 03/11] target-arm: Drop CPUARMState* argument from bank_number()

[Andreas Färber]

Am 26.02.2013 18:40, schrieb Peter Maydell:
> Drop the CPUARMState* argument from bank_number(), since we only
> use it for passing to cpu_abort(). Use hw_error() instead.
> This avoids propagating further interfaces using env pointers.
> 
> In the long term this function's callers need auditing to fix
> problems where badly behaved guests can pass invalid bank numbers.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Andreas Färber <afaerber@suse.de>

Thanks,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v7 07/11] hw/arm_gic: Convert ARM GIC classes to use init/realize

[Peter Maydell]

On 4 March 2013 19:10, Andreas Färber <afaerber@suse.de> wrote:
> Am 26.02.2013 18:40, schrieb Peter Maydell:

>>      if (s->num_cpu > NCPU) {
>> -        hw_error("requested %u CPUs exceeds GIC maximum %d\n",
>> -                 s->num_cpu, NCPU);
>> +        error_setg(errp, "requested %u CPUs exceeds GIC maximum %d\n",
>
> Please drop \n for error_setg(). Probably would be worth adding to a
> convert-to-realize section on the Wiki.

Doh. That's such a trivial change I intend to just make it in
passing when I put these changes into target-arm.next rather
than sending out an entire fresh round of patches, unless you
object.

> Otherwise looks fine, thanks.

Should I mark such a fixed-up patch with your reviewed-by tag?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v7 07/11] hw/arm_gic: Convert ARM GIC classes to use init/realize

[Andreas Färber]

Am 04.03.2013 12:50, schrieb Peter Maydell:
> On 4 March 2013 19:10, Andreas Färber <afaerber@suse.de> wrote:
>> Am 26.02.2013 18:40, schrieb Peter Maydell:
> 
>>>      if (s->num_cpu > NCPU) {
>>> -        hw_error("requested %u CPUs exceeds GIC maximum %d\n",
>>> -                 s->num_cpu, NCPU);
>>> +        error_setg(errp, "requested %u CPUs exceeds GIC maximum %d\n",
>>
>> Please drop \n for error_setg(). Probably would be worth adding to a
>> convert-to-realize section on the Wiki.
> 
> Doh. That's such a trivial change I intend to just make it in
> passing when I put these changes into target-arm.next rather
> than sending out an entire fresh round of patches, unless you
> object.
> 
>> Otherwise looks fine, thanks.
> 
> Should I mark such a fixed-up patch with your reviewed-by tag?

Yes, that's fine, thanks.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v7 00/11] QEMU: Support KVM on ARM

[Christoffer Dall]

On Tue, Feb 26, 2013 at 05:40:10PM +0000, Peter Maydell wrote:
> KVM ARM support has just hit Linus' kernel tree, so we can
> finally commit this series to QEMU. Since all the patches got
> reviewed last time round this should be ready to commit.
> I plan to commit this via arm-devs.next.
> 
> NB: the linux-headers sync is against Linus' mainline, and
> so I had to make a few minor tweaks to avoid the QEMU patch
> reverting some s390 and ppc changes which haven't yet hit
> mainline. (Can't sync vs kvm-next kernel tree because ARM
> KVM hasn't got there yet...)
> 
> thanks
> -- PMM
> 
FWIW, I've tested this and the KVM support works just fine.

(There's an issue using newer kernels and SD card emulation on the
vexpress platform, but that's completely unrelated to these patches).

Thanks,
-Christoffer