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?
>
> > 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?
>
> > 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.
Regards,
Nikhil D
>
> > + 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.
--
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/20201029152737.og3yauwbcxu3th2p%40NiksLab.
