Re: [RFC PATCH] cpu: system_ops: move to cpu-system-ops.h, keep a pointer in CPUClass

From: Richard Henderson <richard.henderson@linaro.org>
To: "Philippe Mathieu-Daudé" <f4bug@amsat.org>, qemu-devel@nongnu.org
Cc: "Peter Maydell" <peter.maydell@linaro.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Daniel P. Berrangé" <berrange@redhat.com>,
	"Eduardo Habkost" <ehabkost@redhat.com>,
	"Paul Durrant" <paul@xen.org>,
	"Dr. David Alan Gilbert" <dgilbert@redhat.com>,
	"Markus Armbruster" <armbru@redhat.com>,
	qemu-arm@nongnu.org, "Claudio Fontana" <cfontana@suse.de>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Anthony Perard" <anthony.perard@citrix.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [RFC PATCH] cpu: system_ops: move to cpu-system-ops.h, keep a pointer in CPUClass
Date: Sat, 27 Feb 2021 10:59:23 -0800	[thread overview]
Message-ID: <516603e7-d3ba-6958-41ec-ebcc52530d37@linaro.org> (raw)
In-Reply-To: <20210226164001.4102868-1-f4bug@amsat.org>

On 2/26/21 8:40 AM, Philippe Mathieu-Daudé wrote:
> +++ b/include/hw/core/cpu-system-ops.h
> @@ -0,0 +1,89 @@
> +/*
> + * CPU operations specific to system emulation
> + *
> + * Copyright (c) 2012 SUSE LINUX Products GmbH
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef CPU_SYSTEM_OPS_H
> +#define CPU_SYSTEM_OPS_H

I think you should create this header from the start, so that you don't have to
move the structure later.

> +/* see cpu-system-ops.h */
> +struct CPUSystemOperations;

In the beginning, you'd actually #include the new header here, and in this
final patch you'd remove the include and insert the declaration.

>      /* when system emulation is not available, this pointer is NULL */
> -    struct CPUSystemOperations system_ops;
> +    struct CPUSystemOperations *system_ops;

Insert the comment here, since the structure can't be null.  ;-)
Also, const.

>      /* when TCG is not available, this pointer is NULL */
>      struct TCGCPUOps *tcg_ops;

The only reason this one isn't const is hw/mips/jazz.  And I'm very tempted to
hack around that one.

> +static struct CPUSystemOperations arm_sysemu_ops = {
> +    .vmsd = &vmstate_arm_cpu,
> +    .get_phys_page_attrs_debug = arm_cpu_get_phys_page_attrs_debug,
> +    .asidx_from_attrs = arm_asidx_from_attrs,
> +    .virtio_is_big_endian = arm_cpu_virtio_is_big_endian,
> +    .write_elf64_note = arm_cpu_write_elf64_note,
> +    .write_elf32_note = arm_cpu_write_elf32_note,
> +};

const.

r~