Re: Record AS in full tlb

["Alex Bennée" <alex.bennee@linaro.org>]

Alex Bennée <alex.bennee@linaro.org> writes:

> Mark Burton <mburton@qti.qualcomm.com> writes:
>
>> Just posting this here for the discussion this afternoon.
>>
>> Cheers
>> Mark.
>>
>>
>> [2. 0001-Record-AddressSpace-in-full-tlb-so-access-to-MMIO-vi.patch --- text/x-diff; 0001-Record-AddressSpace-in-full-tlb-so-access-to-MMIO-vi.patch]...
>
> We were discussing last week how we should break the dependency on
> CPUState for AddressSpaces while Gustavo was looking at adding the
> FEAT_MEC code.
>
> They are really a function of the machine where most CPUs will share the
> same set of views. However we still need a way to resolve the AS from
> the CPUs perspective.
>
> Copying inline for easier commenting:
>
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index b09229dae8..4b1aa9df71 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -1073,7 +1073,8 @@ void tlb_set_page_full(CPUState *cpu, int mmu_idx,
>      prot = full->prot;
>      asidx = cpu_asidx_from_attrs(cpu, full->attrs);
>      section = address_space_translate_for_iotlb(cpu, asidx, paddr_page,
> -                                                &xlat, &sz, full->attrs, &prot);
> +                                                &xlat, &sz, &full->attrs,
> +                                                &full->as, &prot);
>      assert(sz >= TARGET_PAGE_SIZE);
>  
>      tlb_debug("vaddr=%016" VADDR_PRIx " paddr=0x" HWADDR_FMT_plx
> @@ -1294,13 +1295,13 @@ static inline void cpu_unaligned_access(CPUState *cpu, vaddr addr,
>  }
>  
>  static MemoryRegionSection *
> -io_prepare(hwaddr *out_offset, CPUState *cpu, hwaddr xlat,
> +io_prepare(hwaddr *out_offset, CPUState *cpu, AddressSpace *as, hwaddr xlat,
>             MemTxAttrs attrs, vaddr addr, uintptr_t retaddr)
>  {
>      MemoryRegionSection *section;
>      hwaddr mr_offset;
>  
> -    section = iotlb_to_section(cpu, xlat, attrs);
> +    section = iotlb_to_section(as, xlat, attrs);
>      mr_offset = (xlat & TARGET_PAGE_MASK) + addr;
>      cpu->mem_io_pc = retaddr;
>      if (!cpu->neg.can_do_io) {
> @@ -1618,7 +1619,7 @@ bool tlb_plugin_lookup(CPUState *cpu, vaddr addr, int mmu_idx,
>      /* We must have an iotlb entry for MMIO */
>      if (tlb_addr & TLB_MMIO) {
>          MemoryRegionSection *section =
> -            iotlb_to_section(cpu, full->xlat_section & ~TARGET_PAGE_MASK,
> +            iotlb_to_section(full->as, full->xlat_section & ~TARGET_PAGE_MASK,
>                               full->attrs);
>          data->is_io = true;
>          data->mr = section->mr;
> @@ -2028,7 +2029,8 @@ static uint64_t do_ld_mmio_beN(CPUState *cpu, CPUTLBEntryFull *full,
>      tcg_debug_assert(size > 0 && size <= 8);
>  
>      attrs = full->attrs;
> -    section = io_prepare(&mr_offset, cpu, full->xlat_section, attrs, addr, ra);
> +    section = io_prepare(&mr_offset, cpu, full->as,
> +                         full->xlat_section, attrs, addr, ra);
>      mr = section->mr;
>  
>      BQL_LOCK_GUARD();
> @@ -2049,7 +2051,8 @@ static Int128 do_ld16_mmio_beN(CPUState *cpu, CPUTLBEntryFull *full,
>      tcg_debug_assert(size > 8 && size <= 16);
>  
>      attrs = full->attrs;
> -    section = io_prepare(&mr_offset, cpu, full->xlat_section, attrs, addr, ra);
> +    section = io_prepare(&mr_offset, cpu, full->as,
> +                         full->xlat_section, attrs, addr, ra);
>      mr = section->mr;
>  
>      BQL_LOCK_GUARD();
> @@ -2593,7 +2596,8 @@ static uint64_t do_st_mmio_leN(CPUState *cpu, CPUTLBEntryFull *full,
>      tcg_debug_assert(size > 0 && size <= 8);
>  
>      attrs = full->attrs;
> -    section = io_prepare(&mr_offset, cpu, full->xlat_section, attrs, addr, ra);
> +    section = io_prepare(&mr_offset, cpu, full->as,
> +                         full->xlat_section, attrs, addr, ra);
>      mr = section->mr;
>  
>      BQL_LOCK_GUARD();
> @@ -2613,7 +2617,8 @@ static uint64_t do_st16_mmio_leN(CPUState *cpu, CPUTLBEntryFull *full,
>      tcg_debug_assert(size > 8 && size <= 16);
>  
>      attrs = full->attrs;
> -    section = io_prepare(&mr_offset, cpu, full->xlat_section, attrs, addr, ra);
> +    section = io_prepare(&mr_offset, cpu, full->as,
> +                         full->xlat_section, attrs, addr, ra);
>      mr = section->mr;
>  
>      BQL_LOCK_GUARD();
> diff --git a/include/accel/tcg/iommu.h b/include/accel/tcg/iommu.h
> index 90cfd6c0ed..ac50e50601 100644
> --- a/include/accel/tcg/iommu.h
> +++ b/include/accel/tcg/iommu.h
> @@ -16,22 +16,23 @@
>  
>  /**
>   * iotlb_to_section:
> - * @cpu: CPU performing the access
> + * @as: Address space to access
>   * @index: TCG CPU IOTLB entry
>   *
>   * Given a TCG CPU IOTLB entry, return the MemoryRegionSection that
>   * it refers to. @index will have been initially created and returned
>   * by memory_region_section_get_iotlb().
>   */
> -MemoryRegionSection *iotlb_to_section(CPUState *cpu,
> -                                      hwaddr index, MemTxAttrs attrs);
> +struct MemoryRegionSection *iotlb_to_section(AddressSpace *as,
> +                                             hwaddr index, MemTxAttrs attrs);
>  
>  MemoryRegionSection *address_space_translate_for_iotlb(CPUState *cpu,
>                                                         int asidx,
>                                                         hwaddr addr,
>                                                         hwaddr *xlat,
>                                                         hwaddr *plen,
> -                                                       MemTxAttrs attrs,
> +                                                       MemTxAttrs *attrs,
> +                                                       AddressSpace **as,
>                                                         int *prot);
>  
>  hwaddr memory_region_section_get_iotlb(CPUState *cpu,
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index c0ca4b6905..a27d8feefc 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -269,6 +269,8 @@ struct CPUTLBEntryFull {
>              bool guarded;
>          } arm;
>      } extra;
> +
> +    AddressSpace *as;
>  };
>  
>  /*
> diff --git a/system/physmem.c b/system/physmem.c
> index cf7146b224..52156325d9 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -688,7 +688,8 @@ void tcg_iommu_init_notifier_list(CPUState *cpu)
>  MemoryRegionSection *
>  address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr orig_addr,
>                                    hwaddr *xlat, hwaddr *plen,
> -                                  MemTxAttrs attrs, int *prot)
> +                                  MemTxAttrs *attrs, AddressSpace **as,
> +                                  int *prot)
>  {
>      MemoryRegionSection *section;
>      IOMMUMemoryRegion *iommu_mr;
> @@ -696,7 +697,8 @@ address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr orig_addr,
>      IOMMUTLBEntry iotlb;
>      int iommu_idx;
>      hwaddr addr = orig_addr;
> -    AddressSpaceDispatch *d = address_space_to_dispatch(cpu->cpu_ases[asidx].as);
> +    *as = cpu->cpu_ases[asidx].as;
> +    AddressSpaceDispatch *d = address_space_to_dispatch(*as);

Should address_space_translate_for_iotlb be decoupled from CPUs entirely
and just be passed the computed address space from cputlb? The only
other reason we need cpu here is for the tcg_register_iommu_notifier()
call which could be passed the notifier directly.

Do we have notifiers for IOMMU's themselves to track changes or is this
something we only care about for cputlb because of the fast/slowpath handling?

>  
>      for (;;) {
>          section = address_space_translate_internal(d, addr, &addr, plen, false);
> @@ -708,13 +710,13 @@ address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr orig_addr,
>  
>          imrc = memory_region_get_iommu_class_nocheck(iommu_mr);
>  
> -        iommu_idx = imrc->attrs_to_index(iommu_mr, attrs);
> +        iommu_idx = imrc->attrs_to_index(iommu_mr, *attrs);
>          tcg_register_iommu_notifier(cpu, iommu_mr, iommu_idx);
>          /* We need all the permissions, so pass IOMMU_NONE so the IOMMU
>           * doesn't short-cut its translation table walk.
>           */
>          if (imrc->translate_attr) {
> -            iotlb = imrc->translate_attr(iommu_mr, addr, IOMMU_NONE, &attrs);
> +            iotlb = imrc->translate_attr(iommu_mr, addr, IOMMU_NONE, attrs);
>          } else {
>              iotlb = imrc->translate(iommu_mr, addr, IOMMU_NONE, iommu_idx);
>          }
> @@ -735,7 +737,8 @@ address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr orig_addr,
>              goto translate_fail;
>          }
>  
> -        d = flatview_to_dispatch(address_space_to_flatview(iotlb.target_as));
> +        *as = iotlb.target_as;
> +        d = flatview_to_dispatch(address_space_to_flatview(*as));
>      }
>  
>      assert(!memory_region_is_iommu(section->mr));
> @@ -756,12 +759,12 @@ translate_fail:
>      return &d->map.sections[PHYS_SECTION_UNASSIGNED];
>  }
>  
> -MemoryRegionSection *iotlb_to_section(CPUState *cpu,
> +
> +MemoryRegionSection *iotlb_to_section(AddressSpace *as,
>                                        hwaddr index, MemTxAttrs attrs)
>  {
> -    int asidx = cpu_asidx_from_attrs(cpu, attrs);
> -    CPUAddressSpace *cpuas = &cpu->cpu_ases[asidx];
> -    AddressSpaceDispatch *d = address_space_to_dispatch(cpuas->as);
> +    assert(as);
> +    AddressSpaceDispatch *d = address_space_to_dispatch(as);
>      int section_index = index & ~TARGET_PAGE_MASK;
>      MemoryRegionSection *ret;
>  
> @@ -3102,6 +3105,9 @@ static void tcg_commit(MemoryListener *listener)
>       * That said, the listener is also called during realize, before
>       * all of the tcg machinery for run-on is initialized: thus halt_cond.
>       */
> +// Why are these removed   ?        
> +//     cpu_reloading_memory_map();
> +//     tlb_flush(cpuas->cpu);
>      if (cpu->halt_cond) {
>          async_run_on_cpu(cpu, tcg_commit_cpu, RUN_ON_CPU_HOST_PTR(cpuas));
>      } else {
> -- 
> 2.51.1

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro