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

[Auger Eric]

Hi Peter,

On 06/03/18 13:09, Peter Maydell wrote:
> On 17 February 2018 at 18:46, Eric Auger <address@hidden> 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 <address@hidden>
>> Signed-off-by: Prem Mallappa <address@hidden>
>>
> 
>> + * Author: Prem Mallappa <address@hidden>
>> + *
>> + */
>> +
>> +#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 ?
Yes will fix that.

note that those hash tables are not yet used. However I kept those in
this series to illustrate the split between the generic and smmuv3 part.
configs will be used to cache config data whereas iotlb will be used to
cache IOTLB entries. I can remove them at the moment and keep the
realize function void.
> 
>> +}
>> +
>> +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);
OK. Will do that as well for smmuv3 class. I used the fellow reset
setter but not that one.
> 
>> +    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'.)
OK
> 
>> +} SMMUTransCfg;
> 
> Otherwise
> Reviewed-by: Peter Maydell <address@hidden>
thanks!

Eric
> 
> thanks
> -- PMM
>