On 02/06/20 8:16 pm, Jan Kiszka wrote:
On 11.04.20 20:55, Jan Kiszka wrote:
On 11.04.20 20:37, Nikhil Devshatwar wrote:
Hi Jan,
On 11/04/20 11:28 pm, Jan Kiszka wrote:
On 11.04.20 19:25, 'Nikhil Devshatwar' via Jailhouse wrote:
Hi Jan,
On 11/04/20 4:31 pm, Jan Kiszka wrote:
From: Jan Kiszka<jan.kis...@siemens.com>
So far, any error returned by a function called by
pvu_iommu_config_commit was ignored, only reported to the console.
That
would have resulted in an inconsistent configuration being run. Also,
pvu_tlb_alloc and pvu_tlb_chain didn't even report an errors at all,
and
the former also returned an incorrect code then.
This is because I have implemented all the functions with return codes.
I have implemented the pvu_iommu_config_commit with int return type.
From the design perspective, the pvu_xxx APIs are written from any
place.
Nope, they aren't, I checked that.
It's the config_commit call which is nont returnable, So that function
should panic.
So, the only place for panic should be either in the
pvu_iommu_config_commit
Or, in the iommu_config_commit
Regards,
Nikhil D
We rather need to panic-stop the system in case some configuration bit
cannot be programmed because there is no way to roll back from a
config_commit by design.
Signed-off-by: Jan Kiszka<jan.kis...@siemens.com>
---
I wonder if there isn't a way - provided it's not too complex - to
build
up the programming during cell_init/exit, validate it at that chance,
cache it, and only apply that state on config_commit. Would mean less
panic.
I gave some thought to create the data structures before config_commit.
But as of now, there is no way that exist.
Following is the sequence of calls
pvu_iommu_map_memory
pvu_iommu_map_memory
..
pvu_iommu_map_memory
pvu_iommu_config_commit
There is nothing that gets called from framework, and there is no
way to
figure out if the pvu_iommu_map_memory is the last call
OK, so none of the error conditions can be predicted from the given
config and current configuration due to the unknown upcoming mappings? I
don't thinks so. E.g. the alignment checks in pvu_entry_write(). The
entries checked here come from a a cache, and that is built in
pvu_iommu_map_memory where we are able to return a proper error.
All the alignment check errors are defensive programming only.
So it can be guaranteed that these functions will succeed when called
from the
config_commit, except the alloc_tlb failure.
I can maintain a free_entries count per device and check if the entry
count
exceeds that. Assuming that the last memory_map and config_commit for a
cell happen
in order before any other cell's map_memory is called.
This way, we can track the errors and fail the cell creation.
That sounds good.
I would still recommend a return value check and panic to track bugs in
the entry creation logic.
If there is a real risk that invalid configs we can't catch earlier or
unexpected hardware state can get us there, yes. For all other cases, I
would rather recommend to sit down, check the code more than once again
and exclude programming errors upfront.
I hope this issue is not forgotten, just in the backlog.
I will send v2 for this fixes
Regards,
Nikhil D
--
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 jailhouse-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/jailhouse-dev/2255653f-e329-7515-17cc-ed44eba962ce%40ti.com.
