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
