Re: [Qemu-devel] [PATCH] target-arm: Extract some external ARM CPU API

[Peter Maydell]

On 22 October 2015 at 14:09, Pavel Fedin <address@hidden> wrote:
> Includes, which reside in target-arm, are very problematic to use from code
> which is considered target-independent by the build system (for example,
> hw/intc/something.c). It happens because they depend on config-target.h,
> which is unreachable in this case. This creates problems for example for
> GICv3 implementation, which needs to call define_arm_cp_regs_with_opaque()
> in order to add system registers to the processor model, as well as play
> with affinity IDs.
>
> This patch solves the problem by extracting some self-sufficient
> definitions into public area (include/hw/cpu).

Yes, this makes sense. I have one minor questions below.

> Additionally, the patch introduces useful "mp-affinity" property.

Please don't do two things in one patch. (I actually read this patch
code-first and thought you'd accidentally inserted this change due
to a rebasing mishap...) It's particularly bad in patches which are
otherwise almost entirely moving code from one file to another,
because it's easy to overlook the substantive change in the resulting
large patch.

> Signed-off-by: Pavel Fedin <address@hidden>
> ---
>  include/hw/cpu/arm.h | 295 
> +++++++++++++++++++++++++++++++++++++++++++++++++++
>  target-arm/cpu-qom.h |  40 +------
>  target-arm/cpu.c     |   1 +
>  target-arm/cpu.h     | 226 +--------------------------------------
>  4 files changed, 299 insertions(+), 263 deletions(-)
>  create mode 100644 include/hw/cpu/arm.h
>
> diff --git a/include/hw/cpu/arm.h b/include/hw/cpu/arm.h
> new file mode 100644
> index 0000000..17544c9
> --- /dev/null
> +++ b/include/hw/cpu/arm.h
> @@ -0,0 +1,295 @@
> +/*
> + * QEMU ARM CPU
> + *
> + * Copyright (c) 2003 Fabrice Bellard
> + * Copyright (c) 2012 SUSE LINUX Products GmbH
> + * Copyright (c) 2015 Samsung Electronics Co. Ltd.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program 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 General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see
> + * <http://www.gnu.org/licenses/gpl-2.0.html>
> + */
> +
> +#ifndef HW_CPU_ARM_H
> +#define HW_CPU_ARM_H
> +
> +#include "qom/cpu.h"
> +
> +#define TYPE_ARM_CPU "arm-cpu"
> +
> +#define ARM_CPU_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(ARMCPUClass, (klass), TYPE_ARM_CPU)
> +#define ARM_CPU(obj) \
> +    OBJECT_CHECK(ARMCPU, (obj), TYPE_ARM_CPU)
> +#define ARM_CPU_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(ARMCPUClass, (obj), TYPE_ARM_CPU)
> +
> +/**
> + * ARMCPUClass:
> + * @parent_realize: The parent class' realize handler.
> + * @parent_reset: The parent class' reset handler.
> + *
> + * An ARM CPU model.
> + */
> +typedef struct ARMCPUClass {
> +    /*< private >*/
> +    CPUClass parent_class;
> +    /*< public >*/
> +
> +    DeviceRealize parent_realize;
> +    void (*parent_reset)(CPUState *cpu);
> +} ARMCPUClass;

I'm slightly surprised we need to define the ARMCPUClass struct
here -- what needs the definition rather than just the typedef,
or is this just to follow other header files? (I guess in theory
you could have a subclass of ARMCPU which wasn't target-dependent.)

thanks
-- PMM