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
>
>>
>>> 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.
>
>>
>>> 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
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... ;)
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/0dccb943-2777-b646-de90-1124bc6d9530%40siemens.com.
