Re: [Qemu-devel] [PATCH v1 1/8] hw: arm: SMMUv3 emulation model

[Prem Mallappa]

Hi Edger,
>
> A few first pass comments inline
>

Thanks for review.


>> +typedef struct SMMUTransCfg {
>> +    union {
>> +        hwaddr va;              /* Input to S1 */
>> +        hwaddr ipa;             /* Input to S2 */
>> +    };
>
> I think we could just use va here.
>
Agreed

>> +    union {
>> +        hwaddr opa;             /* Output from S2 */
>> +        hwaddr pa;              /* Output from S1, Final PA */
>> +    };
>
> ... and pa here.
Agreed

>
>> +
>> +    bool    s2_needed;
>> +    struct SMMUTransCfg *s2cfg;
>
> Can't we instead of having these fields tell the translate function
> what stage we are requesting translation for (and call translate
> twice for S1+S2)?
>
> That's more in line with what we do in the CPU models. Ultimately,
> it would be nice if we can gradually make the translation code
> more and more similar with the CPU one to the point where we
> can start sharing it.

I'll look into this, and change accordingly.

>
>> +} SMMUTransCfg;
>> +
>> +typedef struct {
>> +    /* <private> */
>> +    SysBusDeviceClass parent_class;
>> +
>> +    /* public */
>> +    SMMUTransErr (*translate)(SMMUTransCfg *cfg, uint32_t *pagesize,
>> +                              uint32_t *perm, bool is_write);
>> +    SMMUTransErr (*translate_lpae)(SMMUTransCfg *cfg, uint32_t 
*pagesize,
>> +                                   uint32_t *perm, bool is_write);
>
> I'm not sure about the naming here. In terms of SMMU, I've mostly heard
> the distinction between 32 and 64 bit page-tables and not really the term
> LPAE being used...
>
Will change it to translate_32, and translate_64, ARMv7 refers to it as 
LPAE in spec, hence the name.


>
>> +} SMMUBaseClass;
>> +
>> +#define SMMU_DEVICE_GET_CLASS(obj)                              \
>> +    OBJECT_GET_CLASS(SMMUBaseClass, (obj), TYPE_SMMU_DEV_BASE)
>> +
>> +/* #define ARM_SMMU_DEBUG */
>> +#ifdef ARM_SMMU_DEBUG
>> +
>> +extern uint32_t  dbg_bits;
>
>
> Can you please have a look at reusing qemu_log for this?
> There's a CPU_LOG_MMU classifier already, not sure if may
> want to introduce a CPU_LOG_IOMMU one. I'd be OK either
> way.
>
Sure, will make necessary changes.


>> +    RegInfo      regs[SMMU_NREGS];
>
> This should be uint32_t I think. You can have a different
> RegisterInfo regs_info array if using the reg API.
>

My intensions were to use RegisterInfo, but it looked overkill for this 
particular case.

>> +
>> +static void smmu_write_reg(SMMUV3State *s, uint32_t addr, uint64_t val)
>> +{
>> +    RegInfo *reg = &s->regs[addr >> 2];
>
> I think you should use the register API fully or not use it at all.
> You are kind of reimplementing some of it's features it seems.
> Have a look at hw/dma/xlnx-zynq-devcfg.c for an example on how it's done.
>
> IMO, this device doesn't benefit much from the register API though
> so I'd just probably have an array of uint32_t regs[SMMU_REGS] and
> deal with it. Either way is OK with me.
>
uint32_t was initial design, but it looked clumsy below is the code from 
earlier version (RFC)
Please see smmu_write_mmio() in
http://lists.nongnu.org/archive/html/qemu-devel/2016-01/msg01547.html

>
>> +
>> +    reg->data = val;
>> +
>> +    if (reg->post) {
>> +        reg->post(reg, addr, val, s);
>> +    }
>> +}
>> +
>> +#define smmu_write32_reg smmu_write_reg
>> +
>> +static inline uint32_t smmu_read32_reg(SMMUV3State *s, uint32_t addr)
>> +{
>> +    RegInfo *reg = &s->regs[addr >> 2];
>> +
>> +    return (uint32_t)reg->data;
>> +}
>> +
>> +static inline uint64_t smmu_read64_reg(SMMUV3State *s, uint32_t addr)
>> +{
>> +    RegInfo *reg = &s->regs[addr >> 2];
>> +
>> +    return reg->data;
>> +}
>
> You don't need this stuff with or without the register API. You can
> internally always access the regs directly by looking at 
s->regs[R_SOME_REG].

Probably not needed for the read path, but write() path is definitely 
made much cleaner. I'll see what I can do for direct access to registers.

Thanks
/Prem

<<snipped>>