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<[email protected]> >>>>> >>>>> 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<[email protected]> >>>>> --- >>>>> >>>>> 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. Jan -- Siemens AG, Corporate Technology, CT RDA IOT SES-DE 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/596d4db3-2af9-df01-3090-dad8c347eb77%40siemens.com.
