Re: [Qemu-devel] [Qemu-arm] [PATCH v7 13/20] hw/arm/smmuv3: Implement IOMMU memory region replay callback

[Tomasz Nowicki]

Hi Eric,

On 14.09.2017 11:27, Linu Cherian wrote:
> Hi Eric,
>
> On Fri Sep 01, 2017 at 07:21:16PM +0200, Eric Auger wrote:
> > memory_region_iommu_replay() is used for VFIO integration.
> >
> > However its default implementation is not adapted to SMMUv3
> > IOMMU memory region. Indeed the input address range is too
> > huge and its execution is too slow as it calls the translate()
> > callback on each granule.
> >
> > Let's implement the replay callback which hierarchically walk
> > over the page table structure and notify only the segments
> > that are populated with valid entries.
> >
> > Signed-off-by: Eric Auger <address@hidden>
> > ---
> >   hw/arm/smmuv3.c     | 36 ++++++++++++++++++++++++++++++++++++
> >   hw/arm/trace-events |  1 +
> >   2 files changed, 37 insertions(+)
> >
> > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> > index 8e7d10d..c43bd93 100644
> > --- a/hw/arm/smmuv3.c
> > +++ b/hw/arm/smmuv3.c
> > @@ -657,6 +657,41 @@ static int smmuv3_notify_entry(IOMMUTLBEntry *entry, void 
> > *private)
> >       return 0;
> >   }
> >   
> > +/* Unmap the whole notifier's range */
> > +static void smmuv3_unmap_notifier_range(IOMMUNotifier *n)
> > +{
> > +    IOMMUTLBEntry entry;
> > +    hwaddr size = n->end - n->start + 1;
> > +
> > +    entry.target_as = &address_space_memory;
> > +    entry.iova = n->start & ~(size - 1);
> > +    entry.perm = IOMMU_NONE;
> > +    entry.addr_mask = size - 1;
> > +
> > +    memory_region_notify_one(n, &entry);
> > +}
> > +
> > +static void smmuv3_replay(IOMMUMemoryRegion *mr, IOMMUNotifier *n)
> > +{
> > +    SMMUTransCfg cfg = {};
> > +    int ret;
> > +
> > +    trace_smmuv3_replay(mr->parent_obj.name, n, n->start, n->end);
> > +    smmuv3_unmap_notifier_range(n);
> > +
> > +    ret = smmuv3_decode_config(mr, &cfg);
> > +    if (ret) {
> > +        error_report("%s error decoding the configuration for iommu mr=%s",
> > +                     __func__, mr->parent_obj.name);
> > +    }
>
> On an invalid config being found, shouldnt we return rather than proceeding with
> page table walk. For example on an invalid Stream table entry.

Indeed, without return here vhost case is not working for me.

Thanks,
Tomasz

> +
> > +    if (cfg.disabled || cfg.bypassed) {
> > +        return;
> > +    }
> > +    /* walk the page tables and replay valid entries */
> > +    smmu_page_walk(&cfg, 0, (1ULL << (64 - cfg.tsz)) - 1, false,
> > +                   smmuv3_notify_entry, n);
> > +}
> >   static void smmuv3_notify_iova_range(IOMMUMemoryRegion *mr, IOMMUNotifier *n,
> >                                        uint64_t iova, size_t size)
> >   {
> > @@ -1095,6 +1130,7 @@ static void 
> > smmuv3_iommu_memory_region_class_init(ObjectClass *klass,
> >   
> >       imrc->translate = smmuv3_translate;
> >       imrc->notify_flag_changed = smmuv3_notify_flag_changed;
> > +    imrc->replay = smmuv3_replay;
> >   }
> >   
> >   static const TypeInfo smmuv3_type_info = {
> > diff --git a/hw/arm/trace-events b/hw/arm/trace-events
> > index 4ac264d..15f84d6 100644
> > --- a/hw/arm/trace-events
> > +++ b/hw/arm/trace-events
> > @@ -46,5 +46,6 @@ smmuv3_cfg_stage(int s, uint32_t oas, uint32_t tsz, uint64_t 
> > ttbr, bool aa64, ui
> >   smmuv3_notify_flag_add(const char *iommu) "ADD SMMUNotifier node for iommu 
> > mr=%s"
> >   smmuv3_notify_flag_del(const char *iommu) "DEL SMMUNotifier node for iommu 
> > mr=%s"
> >   smmuv3_replay_mr(const char *name) "iommu mr=%s"
> > +smmuv3_replay(const char *name, void *n, hwaddr start, hwaddr end) "iommu mr=%s notifier=%p 
> > [0x%"PRIx64",0x%"PRIx64"]"
> >   smmuv3_notify_entry(hwaddr iova, hwaddr pa, hwaddr mask, int perm) "iova=0x%"PRIx64" pa=0x%" 
> > PRIx64" mask=0x%"PRIx64" perm=%d"
> >   smmuv3_notify_iova_range(const char *name, uint64_t iova, size_t size, void *n) "iommu mr=%s 
> > iova=0x%"PRIx64" size=0x%lx n=%p"
> > --
> > 2.5.5