Re: [Qemu-arm] [PATCH v9 01/14] hw/arm/smmu-common: smmu base device and datatypes

[Peter Maydell <peter.maydell@linaro.org>]

On 17 February 2018 at 18:46, Eric Auger <eric.auger@redhat.com> wrote:
> The patch introduces the smmu base device and class for the ARM
> smmu. Devices for specific versions will be derived from this
> base device.
>
> We also introduce some important datatypes.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Prem Mallappa <prem.mallappa@broadcom.com>
>

> + * Author: Prem Mallappa <pmallapp@broadcom.com>
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "sysemu/sysemu.h"
> +#include "exec/address-spaces.h"
> +#include "trace.h"
> +#include "exec/target_page.h"
> +#include "qom/cpu.h"
> +#include "hw/qdev-properties.h"
> +#include "qapi/error.h"
> +
> +#include "qemu/error-report.h"
> +#include "hw/arm/smmu-common.h"
> +
> +static void smmu_base_realize(DeviceState *dev, Error **errp)
> +{
> +    SMMUState *s = ARM_SMMU(dev);
> +
> +    s->configs = g_hash_table_new_full(NULL, NULL, NULL, g_free);
> +    s->iotlb = g_hash_table_new_full(NULL, NULL, NULL, g_free);

Shouldn't we also invoke the parent_realize ?

> +}
> +
> +static void smmu_base_reset(DeviceState *dev)
> +{
> +    SMMUState *s = ARM_SMMU(dev);
> +
> +    g_hash_table_remove_all(s->configs);
> +    g_hash_table_remove_all(s->iotlb);
> +}
> +
> +static Property smmu_dev_properties[] = {
> +    DEFINE_PROP_UINT8("bus_num", SMMUState, bus_num, 0),
> +    DEFINE_PROP_LINK("primary-bus", SMMUState, primary_bus, "PCI", PCIBus *),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void smmu_base_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    SMMUBaseClass *sbc = ARM_SMMU_CLASS(klass);
> +
> +    dc->props = smmu_dev_properties;
> +    sbc->parent_realize = dc->realize;
> +    dc->realize = smmu_base_realize;

There's a device_class_set_parent_realize() in the tree now that
we should probably use instead of these 2 lines:
       device_class_set_parent_realize(dc, smmu_base_realize,
                                       &sbc->parent_realize);

> +    dc->reset = smmu_base_reset;
> +}
> +
> +static const TypeInfo smmu_base_info = {
> +    .name          = TYPE_ARM_SMMU,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(SMMUState),
> +    .class_data    = NULL,
> +    .class_size    = sizeof(SMMUBaseClass),
> +    .class_init    = smmu_base_class_init,
> +    .abstract      = true,
> +};
> +
> +static void smmu_base_register_types(void)
> +{
> +    type_register_static(&smmu_base_info);
> +}
> +
> +type_init(smmu_base_register_types)
> +
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> new file mode 100644
> index 0000000..8a9d931
> --- /dev/null
> +++ b/include/hw/arm/smmu-common.h
> @@ -0,0 +1,124 @@
> +/*
> + * ARM SMMU Support
> + *
> + * Copyright (C) 2015-2016 Broadcom Corporation
> + * Copyright (c) 2017 Red Hat, Inc.
> + * Written by Prem Mallappa, Eric Auger
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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.
> + *
> + */
> +
> +#ifndef HW_ARM_SMMU_COMMON_H
> +#define HW_ARM_SMMU_COMMON_H
> +
> +#include <hw/sysbus.h>

QEMU headers should be included using "", not <>.

> +#include "hw/pci/pci.h"
> +
> +#define SMMU_PCI_BUS_MAX      256
> +#define SMMU_PCI_DEVFN_MAX    256
> +
> +#define SMMU_MAX_VA_BITS      48
> +
> +/*
> + * Page table walk error types
> + */
> +typedef enum {
> +    SMMU_PTW_ERR_NONE,
> +    SMMU_PTW_ERR_WALK_EABT,   /* Translation walk external abort */
> +    SMMU_PTW_ERR_TRANSLATION, /* Translation fault */
> +    SMMU_PTW_ERR_ADDR_SIZE,   /* Address Size fault */
> +    SMMU_PTW_ERR_ACCESS,      /* Access fault */
> +    SMMU_PTW_ERR_PERMISSION,  /* Permission fault */
> +} SMMUPTWEventType;
> +
> +typedef struct SMMUPTWEventInfo {
> +    SMMUPTWEventType type;
> +    dma_addr_t addr; /* fetched address that induced an abort, if any */
> +} SMMUPTWEventInfo;
> +
> +typedef struct SMMUTransTableInfo {
> +    bool disabled;             /* is the translation table disabled? */
> +    uint64_t ttb;              /* TT base address */
> +    uint8_t tsz;               /* input range, ie. 2^(64 -tsz)*/
> +    uint8_t granule_sz;        /* granule page shift */
> +    uint8_t initial_level;     /* initial lookup level */
> +} SMMUTransTableInfo;
> +
> +/*
> + * Generic structure populated by derived SMMU devices
> + * after decoding the configuration information and used as
> + * input to the page table walk
> + */
> +typedef struct SMMUTransCfg {
> +    int      stage;            /* translation stage */
> +    bool     aa64;             /* arch64 or aarch32 translation table */
> +    bool     disabled;         /* smmu is disabled */
> +    bool     bypassed;         /* translation is bypassed */
> +    bool     aborted;          /* translation is aborted */
> +    uint64_t ttb;              /* TT base address */
> +    uint8_t oas;               /* output address width */
> +    uint8_t  tbi;              /* Top Byte Ignore */
> +    uint16_t asid;
> +    SMMUTransTableInfo tt[2];

Can you be consistent about either lining up the field names or not doing so,
please? (I would suggest going for 'not'.)

> +} SMMUTransCfg;

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM