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

[Avi Kivity]

On 10/11/2012 03:53 PM, Paolo Bonzini wrote:
> 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.

Maybe, I just typed in code until it compiled, then forgot to copy Alex.
 I have no idea how to test it, so I kept the changes minimal.

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

Indeed that's the plan.  address_space_map() and dma_memory_map() simply
duplicate each other.  The only thing remaining is dma_memory_set().

> I like it!

Me too, except when I'm coding it.

-- 
error compiling committee.c: too many arguments to function