On 29.10.20 08:14, 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. > >> 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 catching earlier is not easily possible, you could also consider moving things to jailhouse-config-check. 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/64aeedd9-53cc-150b-cea0-f62d8ee7a402%40siemens.com.
