Re: [Qemu-devel] [RFC v1 4/7] pci: switch iommu to using the memory API

[Paolo Bonzini <pbonzini@redhat.com>]

Il 11/10/2012 15:27, Avi Kivity ha scritto:
> -static int spapr_tce_translate(DMAContext *dma,
> -                               dma_addr_t addr,
> -                               target_phys_addr_t *paddr,
> -                               target_phys_addr_t *len,
> -                               DMADirection dir)
> +IOMMUTLBEntry spapr_tce_translate(DMAContext *dma, target_phys_addr_t addr,
> +                                  bool is_write)
>  {
>      sPAPRTCETable *tcet = DO_UPCAST(sPAPRTCETable, dma, dma);
> -    enum sPAPRTCEAccess access = (dir == DMA_DIRECTION_FROM_DEVICE)
> -        ? SPAPR_TCE_WO : SPAPR_TCE_RO;
> +    enum sPAPRTCEAccess access = is_write ? SPAPR_TCE_WO : SPAPR_TCE_RO;
>      uint64_t tce;
>  
>  #ifdef DEBUG_TCE
> @@ -84,29 +80,27 @@ static int spapr_tce_translate(DMAContext *dma,
>  #ifdef DEBUG_TCE
>          fprintf(stderr, "spapr_tce_translate out of bounds\n");
>  #endif
> -        return -EFAULT;
> +        return (IOMMUTLBEntry) { .valid = false };
>      }
>  
>      tce = tcet->table[addr >> SPAPR_TCE_PAGE_SHIFT].tce;
>  
>      /* Check TCE */
>      if (!(tce & access)) {
> -        return -EPERM;
> +        return (IOMMUTLBEntry) { .valid = false };
>      }
>  
> -    /* How much til end of page ? */
> -    *len = ((~addr) & SPAPR_TCE_PAGE_MASK) + 1;
> -
> -    /* Translate */
> -    *paddr = (tce & ~SPAPR_TCE_PAGE_MASK) |
> -        (addr & SPAPR_TCE_PAGE_MASK);
> -
>  #ifdef DEBUG_TCE
>      fprintf(stderr, " ->  *paddr=0x" TARGET_FMT_plx ", *len=0x"
> -            TARGET_FMT_plx "\n", *paddr, *len);
> +            TARGET_FMT_plx "\n", (tce & ~SPAPR_TCE_PAGE_MASK), SPAPR_TCE_PAGE_MASK + 1);
>  #endif
>  
> -    return 0;
> +    return (IOMMUTLBEntry) {
> +        .device_addr = addr & SPAPR_TCE_PAGE_MASK,
> +        .translated_addr = (tce & ~SPAPR_TCE_PAGE_MASK),
> +        .addr_mask = SPAPR_TCE_PAGE_MASK,
> +        .valid = true,
> +    };
>  }
>  
>  DMAContext *spapr_tce_new_dma_context(uint32_t liobn, size_t window_size)
> @@ -118,7 +112,7 @@ DMAContext *spapr_tce_new_dma_context(uint32_t liobn, size_t window_size)
>      }
>  
>      tcet = g_malloc0(sizeof(*tcet));
> -    dma_context_init(&tcet->dma, &address_space_memory, spapr_tce_translate, NULL, NULL);
> +    dma_context_init(&tcet->dma, &address_space_memory, NULL, NULL, NULL);
>  
>      tcet->liobn = liobn;
>      tcet->window_size = window_size;
> @@ -253,7 +247,8 @@ int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
>          return 0;
>      }
>  
> -    if (iommu->translate == spapr_tce_translate) {
> +    /* FIXME: WHAT?? */
> +    if (true) {
>          sPAPRTCETable *tcet = DO_UPCAST(sPAPRTCETable, dma, iommu);
>          return spapr_dma_dt(fdt, node_off, propname,
>                  tcet->liobn, 0, tcet->window_size);
> diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
> index 661c05b..24f5b46 100644
> --- a/hw/spapr_pci.c
> +++ b/hw/spapr_pci.c
> @@ -506,14 +506,30 @@ static void spapr_msi_write(void *opaque, target_phys_addr_t addr,
>  /*
>   * PHB PCI device
>   */
> -static DMAContext *spapr_pci_dma_context_fn(PCIBus *bus, void *opaque,
> -                                            int devfn)
> +static MemoryRegion *spapr_pci_dma_iommu_new(PCIBus *bus, void *opaque, int devfn)
>  {
>      sPAPRPHBState *phb = opaque;
>  
> -    return phb->dma;
> +    return &phb->iommu;
>  }
>  
> +static void spapr_pci_dma_iommu_destroy(MemoryRegion *iommu)
> +{
> +    /* iommu is shared among devices, do nothing */
> +}
> +
> +static IOMMUTLBEntry spapr_phb_translate(MemoryRegion *iommu, target_phys_addr_t addr,
> +                                         bool is_write)
> +{
> +    sPAPRPHBState *phb = container_of(iommu, sPAPRPHBState, iommu);
> +
> +    return spapr_tce_translate(phb->dma, addr, is_write);
> +}

IIUC, sPAPRTCETable could drop the DMAContext field and just include the
actual translation info.  spapr_tce_new_dma_context becomes
spapr_tce_new and returns an opaque sPAPRTCETable struct that is passed
back to spapr_tce_translate.

And DMAContext can disappear and will be replaced with just an
AddressSpace *.

I like it!

Paolo