Re: [Qemu-devel] [RFC 2/2] arm_gic: Use new __private macro to mark private fields

[Alexander Graf]

On 25.07.2013, at 18:42, Peter Maydell wrote:

> Use the new __private macro infrastructure to mark private fields for
> the arm_gic classes.
> 
> Signed-off-by: Peter Maydell <address@hidden>
> ---
> hw/intc/arm_gic.c         |    3 +++
> hw/intc/arm_gic_common.c  |    2 ++
> hw/intc/arm_gic_kvm.c     |    2 ++
> hw/intc/armv7m_nvic.c     |    2 ++
> include/hw/intc/arm_gic.h |   64 ++++++++++++++++++++++++++++-----------------
> 5 files changed, 49 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 8e34004..cbe7d27 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -18,6 +18,9 @@
>  *  armv7m_nvic device.
>  */
> 
> +#define IMPLEMENTING_ARM_GIC_COMMON
> +#define IMPLEMENTING_ARM_GIC
> +
> #include "hw/sysbus.h"
> #include "gic_internal.h"
> #include "qom/cpu.h"
> diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
> index a89c786..c27c5c1 100644
> --- a/hw/intc/arm_gic_common.c
> +++ b/hw/intc/arm_gic_common.c
> @@ -18,6 +18,8 @@
>  * with this program; if not, see <http://www.gnu.org/licenses/>.
>  */
> 
> +#define IMPLEMENTING_ARM_GIC_COMMON
> +
> #include "gic_internal.h"
> 
> static void gic_pre_save(void *opaque)
> diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
> index f713975..0acd676 100644
> --- a/hw/intc/arm_gic_kvm.c
> +++ b/hw/intc/arm_gic_kvm.c
> @@ -18,6 +18,8 @@
>  * with this program; if not, see <http://www.gnu.org/licenses/>.
>  */
> 
> +#define IMPLEMENTING_ARM_GIC_COMMON
> +
> #include "hw/sysbus.h"
> #include "sysemu/kvm.h"
> #include "kvm_arm.h"
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 178344b..54d62fe 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -10,6 +10,8 @@
>  * NVIC.  Much of that is also implemented here.
>  */
> 
> +#define IMPLEMENTING_ARM_GIC_COMMON
> +
> #include "hw/sysbus.h"
> #include "qemu/timer.h"
> #include "hw/arm/arm.h"
> diff --git a/include/hw/intc/arm_gic.h b/include/hw/intc/arm_gic.h
> index be945ec..d978d72 100644
> --- a/include/hw/intc/arm_gic.h
> +++ b/include/hw/intc/arm_gic.h
> @@ -40,36 +40,42 @@ typedef struct gic_irq_state {
>     bool trigger; /* nonzero = edge triggered.  */
> } gic_irq_state;
> 
> +#ifdef IMPLEMENTING_ARM_GIC_COMMON
> +#define __private
> +#else
> +#define __private QEMU_PRIVATE_ATTR
> +#endif
> +
> typedef struct GICState {
>     /*< private >*/
> -    SysBusDevice busdev;
> +    __private SysBusDevice busdev;
>     /*< public >*/

I've been wondering about this for a while, but why are private fields in the 
public section?


Alex

> 
> -    qemu_irq parent_irq[GIC_NCPU];
> -    bool enabled;
> -    bool cpu_enabled[GIC_NCPU];
> +    __private qemu_irq parent_irq[GIC_NCPU];
> +    __private bool enabled;
> +    __private bool cpu_enabled[GIC_NCPU];
> 
> -    gic_irq_state irq_state[GIC_MAXIRQ];
> -    uint8_t irq_target[GIC_MAXIRQ];
> -    uint8_t priority1[GIC_INTERNAL][GIC_NCPU];
> -    uint8_t priority2[GIC_MAXIRQ - GIC_INTERNAL];
> -    uint16_t last_active[GIC_MAXIRQ][GIC_NCPU];
> +    __private gic_irq_state irq_state[GIC_MAXIRQ];
> +    __private uint8_t irq_target[GIC_MAXIRQ];
> +    __private uint8_t priority1[GIC_INTERNAL][GIC_NCPU];
> +    __private uint8_t priority2[GIC_MAXIRQ - GIC_INTERNAL];
> +    __private uint16_t last_active[GIC_MAXIRQ][GIC_NCPU];
> 
> -    uint16_t priority_mask[GIC_NCPU];
> -    uint16_t running_irq[GIC_NCPU];
> -    uint16_t running_priority[GIC_NCPU];
> -    uint16_t current_pending[GIC_NCPU];
> +    __private uint16_t priority_mask[GIC_NCPU];
> +    __private uint16_t running_irq[GIC_NCPU];
> +    __private uint16_t running_priority[GIC_NCPU];
> +    __private uint16_t current_pending[GIC_NCPU];
> 
> -    uint32_t num_cpu;
> +    __private uint32_t num_cpu;
> 
> -    MemoryRegion iomem; /* Distributor */
> +    __private MemoryRegion iomem; /* Distributor */
>     /* This is just so we can have an opaque pointer which identifies
>      * both this GIC and which CPU interface we should be accessing.
>      */
> -    struct GICState *backref[GIC_NCPU];
> -    MemoryRegion cpuiomem[GIC_NCPU + 1]; /* CPU interfaces */
> -    uint32_t num_irq;
> -    uint32_t revision;
> +    __private struct GICState *backref[GIC_NCPU];
> +    __private MemoryRegion cpuiomem[GIC_NCPU + 1]; /* CPU interfaces */
> +    __private uint32_t num_irq;
> +    __private uint32_t revision;
> } GICState;
> 
> #define TYPE_ARM_GIC_COMMON "arm_gic_common"
> @@ -82,13 +88,21 @@ typedef struct GICState {
> 
> typedef struct ARMGICCommonClass {
>     /*< private >*/
> -    SysBusDeviceClass parent_class;
> +    __private SysBusDeviceClass parent_class;
>     /*< public >*/
> 
> -    void (*pre_save)(GICState *s);
> -    void (*post_load)(GICState *s);
> +    __private void (*pre_save)(GICState *s);
> +    __private void (*post_load)(GICState *s);
> } ARMGICCommonClass;
> 
> +#undef __private
> +
> +#ifdef IMPLEMENTING_ARM_GIC
> +#define __private
> +#else
> +#define __private QEMU_PRIVATE_ATTR
> +#endif
> +
> #define TYPE_ARM_GIC "arm_gic"
> #define ARM_GIC(obj) \
>      OBJECT_CHECK(GICState, (obj), TYPE_ARM_GIC)
> @@ -99,10 +113,12 @@ typedef struct ARMGICCommonClass {
> 
> typedef struct ARMGICClass {
>     /*< private >*/
> -    ARMGICCommonClass parent_class;
> +    __private ARMGICCommonClass parent_class;
>     /*< public >*/
> 
> -    DeviceRealize parent_realize;
> +    __private DeviceRealize parent_realize;
> } ARMGICClass;
> 
> +#undef __private
> +
> #endif
> -- 
> 1.7.9.5
>