On 28.10.20 15:14, 'Nikhil Devshatwar' via Jailhouse wrote: > Current PVU iommu implementation ignores possible failures in the > config_commit part. This would allow inconsistent configuration > to run and may introduce unknown bugs. > > Solve this by making sure that the pvu_iommu_config_commit never > fails. Catch the errors early in the mapping phase. Use > "free_tlb_count" to track available no of TLBs for chaining. > This can be used to check if any mapping causes it to potentially > use more no of TLBs than that are free. This will ensure that > the allocationg for chaining will not fail.
allocating > > Change the return type to void for few functions. Add comments to > explain behavior in case of failure. > > Signed-off-by: Nikhil Devshatwar <[email protected]> > --- > > Notes: > Changes from v1: > * Add a comment to descibe why pvu_tlb_alloc will not fail > * Make return type of pvu_entry_write as void and explain the > behavior in case the constraints are not met > * Remove un necessary else block > > hypervisor/arch/arm64/include/asm/ti-pvu.h | 3 +- > hypervisor/arch/arm64/ti-pvu.c | 68 ++++++++++++++-------- > 2 files changed, 45 insertions(+), 26 deletions(-) > > diff --git a/hypervisor/arch/arm64/include/asm/ti-pvu.h > b/hypervisor/arch/arm64/include/asm/ti-pvu.h > index 2c340b3a..62aec7c0 100644 > --- a/hypervisor/arch/arm64/include/asm/ti-pvu.h > +++ b/hypervisor/arch/arm64/include/asm/ti-pvu.h > @@ -117,6 +117,7 @@ struct pvu_dev { > u16 max_virtid; > > u16 tlb_data[PVU_NUM_TLBS]; > + u16 free_tlb_count; > }; > > int pvu_iommu_map_memory(struct cell *cell, > @@ -125,6 +126,6 @@ int pvu_iommu_map_memory(struct cell *cell, > int pvu_iommu_unmap_memory(struct cell *cell, > const struct jailhouse_memory *mem); > > -int pvu_iommu_config_commit(struct cell *cell); > +void pvu_iommu_config_commit(struct cell *cell); > > #endif /* _IOMMMU_PVU_H_ */ > diff --git a/hypervisor/arch/arm64/ti-pvu.c b/hypervisor/arch/arm64/ti-pvu.c > index 3b9a29ec..c488ce89 100644 > --- a/hypervisor/arch/arm64/ti-pvu.c > +++ b/hypervisor/arch/arm64/ti-pvu.c > @@ -15,7 +15,7 @@ > * There are limitations on the number of available contexts, page sizes, > * number of pages that can be mapped, etc. > * > - * PVU is desgined to be programmed with all the memory mapping at once. > + * PVU is designed to be programmed with all the memory mapping at once. > * Therefore, it defers the actual register programming till config_commit. > * Also, it does not support unmapping of the pages at runtime. > * > @@ -110,9 +110,15 @@ static u32 pvu_tlb_alloc(struct pvu_dev *dev, u16 virtid) > for (i = dev->max_virtid + 1; i < dev->num_tlbs; i++) { > if (dev->tlb_data[i] == 0) { > dev->tlb_data[i] = virtid << dev->num_entries; > + dev->free_tlb_count--; > return i; > } > } > + > + /* > + * We should never reach here, tlb_allocation should not fail > + * pvu_iommu_map_memory ensures that there are enough free TLBs > + */ If we never get here, the test for "i < dev->num_tlbs" is pointless. If we could get here, we should not return. > return 0; > } > > @@ -138,10 +144,13 @@ static void pvu_tlb_flush(struct pvu_dev *dev, u16 > tlbnum) > > mmio_write32(&tlb->chain, 0x0); > > - if (i < dev->max_virtid) > + if (i < dev->max_virtid) { > dev->tlb_data[tlbnum] = 0x0 | i << dev->num_entries; > - else > + } else { > + /* This was a chained TLB */ > dev->tlb_data[tlbnum] = 0x0; > + dev->free_tlb_count++; > + } > > } > > @@ -159,7 +168,7 @@ static void pvu_entry_enable(struct pvu_dev *dev, u16 > tlbnum, u8 index) > dev->tlb_data[tlbnum] |= (1 << index); > } > > -static int pvu_entry_write(struct pvu_dev *dev, u16 tlbnum, u8 index, > +static void pvu_entry_write(struct pvu_dev *dev, u16 tlbnum, u8 index, > struct pvu_tlb_entry *ent) > { > struct pvu_hw_tlb_entry *entry; > @@ -174,17 +183,21 @@ static int pvu_entry_write(struct pvu_dev *dev, u16 > tlbnum, u8 index, > break; > } > > + /* > + * If the hardware constraints for page size and address alignment > + * are not met, print out an error, return without writing any entry > + */ That's lacking reasoning *why* we can continue then. Again: Can the user trigger this by providing an invalid config? I suspect so. Can we catch that earlier? > if (pgsz >= ARRAY_SIZE(pvu_page_size_bytes)) { > printk("ERROR: PVU: %s: Unsupported page size %llx\n", > __func__, ent->size); > - return -EINVAL; > + return; > } > > if (!is_aligned(ent->virt_addr, ent->size) || > !is_aligned(ent->phys_addr, ent->size)) { > printk("ERROR: PVU: %s: Address %llx => %llx is not aligned > with size %llx\n", > __func__, ent->virt_addr, ent->phys_addr, ent->size); > - return -EINVAL; > + return; > } > > mmio_write32(&entry->reg0, ent->virt_addr & 0xffffffff); > @@ -198,9 +211,8 @@ static int pvu_entry_write(struct pvu_dev *dev, u16 > tlbnum, u8 index, > mmio_write32_field(&entry->reg2, PVU_TLB_ENTRY_PGSIZE_MASK, pgsz); > mmio_write32_field(&entry->reg2, PVU_TLB_ENTRY_FLAG_MASK, ent->flags); > > - /* Do we need "DSB NSH" here to make sure all writes are finised? */ > + /* Do we need "DSB NSH" here to make sure all writes are finished? */ > pvu_entry_enable(dev, tlbnum, index); > - return 0; > } > > static u32 pvu_init_device(struct pvu_dev *dev, u16 max_virtid) > @@ -221,6 +233,8 @@ static u32 pvu_init_device(struct pvu_dev *dev, u16 > max_virtid) > } > > dev->max_virtid = max_virtid; > + dev->free_tlb_count = dev->num_tlbs - (max_virtid + 1); > + > mmio_write32(&cfg->virtid_map1, 0); > mmio_write32_field(&cfg->virtid_map2, PVU_MAX_VIRTID_MASK, max_virtid); > > @@ -328,17 +342,17 @@ static void pvu_entrylist_sort(struct pvu_tlb_entry > *entlist, u32 num_entries) > } > } > > -static int pvu_iommu_program_entries(struct cell *cell, u8 virtid) > +static void pvu_iommu_program_entries(struct cell *cell, u8 virtid) > { > unsigned int inst, i, tlbnum, idx, ent_count; > struct pvu_tlb_entry *ent, *cell_entries; > struct pvu_dev *dev; > - int ret, tlb_next; > + int tlb_next; > > cell_entries = cell->arch.iommu_pvu.entries; > ent_count = cell->arch.iommu_pvu.ent_count; > if (ent_count == 0 || cell_entries == NULL) > - return 0; > + return; > > /* Program same memory mapping for all of the instances */ > for (inst = 0; inst < pvu_count; inst++) { > @@ -356,20 +370,15 @@ static int pvu_iommu_program_entries(struct cell *cell, > u8 virtid) > if (idx == 0 && i >= dev->num_entries) { > /* Find next available TLB and chain to it */ > tlb_next = pvu_tlb_alloc(dev, virtid); > - if (tlb_next < 0) > - return -ENOMEM; > pvu_tlb_chain(dev, tlbnum, tlb_next); > pvu_tlb_enable(dev, tlbnum); > tlbnum = tlb_next; > } > > - ret = pvu_entry_write(dev, tlbnum, idx, ent); > - if (ret) > - return ret; > + pvu_entry_write(dev, tlbnum, idx, ent); > } > pvu_tlb_enable(dev, tlbnum); > } > - return 0; > } > > /* > @@ -380,8 +389,9 @@ int pvu_iommu_map_memory(struct cell *cell, > const struct jailhouse_memory *mem) > { > struct pvu_tlb_entry *ent; > + struct pvu_dev *dev; > unsigned int size; > - u32 flags = 0; > + u32 tlb_count, flags = 0; > int ret; > > if (pvu_count == 0 || (mem->flags & JAILHOUSE_MEM_DMA) == 0) > @@ -408,6 +418,18 @@ int pvu_iommu_map_memory(struct cell *cell, > if (ret < 0) > return ret; > > + /* > + * Check if there are enough TLBs left for *chaining* to ensure that > + * pvu_tlb_alloc called from config_commit never fails > + */ > + tlb_count = (cell->arch.iommu_pvu.ent_count + ret - 1) / 8; Can you explain this math? Without reasioning how that prevents the overflow exactly, dropping a check from pvu_tlb_alloc() would be hard to argue. > + dev = &pvu_units[0]; > + > + if (tlb_count > dev->free_tlb_count) { > + printk("ERROR: PVU: Mapping this memory needs more TLBs than > that are available\n"); > + return -EINVAL; > + } > + > cell->arch.iommu_pvu.ent_count += ret; > return 0; > } > @@ -434,13 +456,12 @@ int pvu_iommu_unmap_memory(struct cell *cell, > return 0; > } > > -int pvu_iommu_config_commit(struct cell *cell) > +void pvu_iommu_config_commit(struct cell *cell) > { > unsigned int i, virtid; > - int ret = 0; > > if (pvu_count == 0 || !cell) > - return 0; > + return; > > /* > * Chaining the TLB entries adds extra latency to translate those > @@ -455,13 +476,10 @@ int pvu_iommu_config_commit(struct cell *cell) > if (virtid > MAX_VIRTID) > continue; > > - ret = pvu_iommu_program_entries(cell, virtid); > - if (ret) > - return ret; > + pvu_iommu_program_entries(cell, virtid); > } > > cell->arch.iommu_pvu.ent_count = 0; > - return ret; > } > > static int pvu_iommu_cell_init(struct cell *cell) > Jan -- Siemens AG, T RDA IOT Corporate Competence Center Embedded Linux -- You received this message because you are subscribed to the Google Groups "Jailhouse" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/jailhouse-dev/372c4fcc-f707-955b-ec3f-c800dc948557%40siemens.com.
