Re: [Qemu-devel] [PATCH v2 3/9] hw: arm: SMMUv3 emulation model

[Prem Mallappa]

Hi Edger,

I'm going to look at the PCI parts and get back to you with
> comments on that.
>
> Please do, by the time, I'll address your and Eric's comments.


> I've put another round of comments inline:
>
> Thanks



> > +inline void
> > +smmu_write_sysmem(hwaddr addr, void *buf, int len, bool secure)
> > +{
> > +    MemTxAttrs attrs = {.unspecified = 1, .secure = secure};
> > +
> > +    switch (len) {
> > +    case 4:
> > +        stl_le_phys(&address_space_memory, addr, *(uint32_t *)buf);
> > +        break;
> > +    case 8:
> > +        stq_le_phys(&address_space_memory, addr, *(uint64_t *)buf);
> > +        break;
> > +    default:
> > +        address_space_rw(&address_space_memory, addr,
> > +                         attrs, buf, len, true);
> > +    }
> > +}
>
> Thinking about this, I think you should just remove these functions and
> always call dma_memory_read/write directly.
>
> It would be nice if you could add a property/link so that machine code
> can specify the MemoryRegion/address space to be used. You'll need a
> link to allow setup of the MemoryRegion and also some code to create
> an address space from the selected MR.
>
> You can have a look at the following code to see how it's done:
> exec.c cpu_exec_init()    see object_property_add_link
> cpus.c qemu_init_vcpu()   see address_space_init_shareable
>
>
Sure, will do.


> > +#define smmu_evt_irq_enabled(s)                   \
> > +    __smmu_irq_enabled(s, SMMU_IRQ_CTRL_EVENT_EN)
> > +#define smmu_gerror_irq_enabled(s)                  \
> > +    __smmu_irq_enabled(s, SMMU_IRQ_CTRL_GERROR_EN)
> > +#define smmu_pri_irq_enabled(s)                 \
> > +    __smmu_irq_enabled(s, SMMU_IRQ_CTRL_PRI_EN)
>
> Please drop the __ prefix on functions. _ prefixed functions are reserved
> and
> we usually avoid them.
>
> I don't think smmu_evt_irq_enabled() is very useful,
> smmu_irq_enabled(s, SMMU_IRQ_CTRL_EVENT_EN) is readable enough.
>
>
Got it.


> > +
> > +/*****************************
> > + * MMIO Register
> > + *****************************/
> > +enum {
> > +    SMMU_REG_IDR0            = 0x0,
>
> For all regs, I think you should prefix regs with R_.
> And also do / 4, e.g:
>
> R_SMMU_REG_IDR1            = 0x4 / 4,
>
> That way you can do s->regs[R_SMMU_REG_IDR1] and remove smmu_read32_reg.
> If you use the REG32 and FIELD macros from the register API you'll
> also be able to use the FIELD_ family of macros (e.g ARRAY_FIELD_EX32)
> to extract fields from regs.
>
> Will change this


> > +struct SMMUQueue {
> > +     hwaddr base;
> > +     uint32_t prod;
> > +     uint32_t cons;
> > +     union {
> > +          struct {
> > +               uint8_t prod:1;
> > +               uint8_t cons:1;
>
> Hi, Peter generally doesn't like bitfields. I'd stay away form
> them unless you have a good case. Just change them too bool.
>
>
This a wrap field, and used as a circular buffer full/empty indicator.
changing it to bool would loose its meaning, I'll change if its too much
off the
coding standard.



>
> > +
> > +typedef struct __smmu_data2  STEDesc; /* STE Level 1 Descriptor */
> > +typedef struct __smmu_data16 Ste;     /* Stream Table Entry(STE) */
> > +typedef struct __smmu_data2  CDDesc;  /* CD Level 1 Descriptor */
> > +typedef struct __smmu_data16 Cd;      /* Context Descriptor(CD) */
> > +
> > +typedef struct __smmu_data4  Cmd; /* Command Entry */
> > +typedef struct __smmu_data8  Evt; /* Event Entry */
> > +typedef struct __smmu_data4  Pri; /* PRI entry */
>
>
> For all of these, I think it would be more useful if you would declare
> structs with actual fields representing the data structures.
> You can then declare load functions that load the STE from memory and
> decode the fields.
>
> E.g:
>
> typedef struct SMMUv3_STEDesc {
>     bool valid;
>     .... etc...
> } SMMUv3_STEDesc;
>
> void smmuv3_load_ste(AddressSpace *as, dma_addr_t addr, SMMUv3_STEDesc
> *ste)
> {
>     uint32_t buf[16];
>     dma_memory_read(as, addr, buf, sizeof(*buf));
>
>     ste->valid = extract32(buf[0], 0, 1);
> }
>
>
> Then, instead of for example doing STE_VALID(x), you can do ste->valid.
>
>
Thanks
I'll change it to appropriate names and delete where possible.


>
> > +#endif
> > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > index e51ed3a..96da537 100644
> > --- a/hw/vfio/common.c
> > +++ b/hw/vfio/common.c
> > @@ -412,10 +412,10 @@ static void vfio_listener_region_add(MemoryListener
> *listener,
> >
> >      ret = vfio_dma_map(container, iova, int128_get64(llsize),
> >                         vaddr, section->readonly);
> > -    if (ret) {
> >          error_report("vfio_dma_map(%p, 0x%"HWADDR_PRIx", "
> >                       "0x%"HWADDR_PRIx", %p) = %d (%m)",
> >                       container, iova, int128_get64(llsize), vaddr, ret);
> > +    if (ret) {
> >          goto fail;
> >      }
>
>
> Shouldn't this be in a separate patch?
>
>
>
Will do this, thanks for your time


-- 
Cheers,
/Prem