On 17:09-20201029, Jan Kiszka wrote:
> On 29.10.20 16:27, Nikhil Devshatwar wrote:
> > On 08:14-20201029, Jan Kiszka wrote:
> >> 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.
> >
> > I would like to add some kind of BUG()
> > Do we have something like this?
>
> Not yet. Could be simply defined as
>
> #define BUG() *(int *)0 = 0xdead
okay, I will add this.
>
> >
> >>
> >>> 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?
> > No. It's not possible. As I explained, it was just defensive
> > programming. I can get rid of the checks and still no bad config will
> > trigger this.
> >
> >> I suspect so. Can we catch that earlier?
> > The original intent was to write the PVU part such that it can be
> > easily ported to other hypervisors. That's why these checks.
> > Shall I remove?
>
> Yes, then it can go.
Will do
>
> >
> >>
> >>> 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
> >>> + */
> >
> > Looks like I will need better comment here
> >
> >>> + 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.
> >
> > I am counting how many TLBs you need for chaining.
> > Total TLBs = (no of entries + 7 ) / 8
> > Total TLBs for chaining = (no of entries + 7 ) / 8 - 1
> > simplified, it becomes (no of entries - 1 ) / 8
> >
> > Let's say the user asks to map a region, which after breakdown, results
> > into 12 entries. Since this call is saving entries for all the mappings,
> > it will add with previous count and find out that total of 76 entries
> > are needed.
> >
> > That means (76 -1 ) / 8 = 9 new allocations are needed.
> > If there are at least 9 free_tlb available, we are okay.
> > If not, we flag the failure early by failing the map call.
> >
>
> OK, there is a magic REGS_PER_TLB = 8, spread in hard-coded manner. I
Good catch.
Actually in all other places I am using dev->num_entries
This is the only place I used magic 8
I will change that as well.
> didn't look as close at this code yet as I did for the smmu-v2. I
> suspect there is also some room for cleanups... ;)
Thanks!
Nikhil D
>
> 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/20201029221907.357r23w3q7rozsxa%40NiksLab.
