Re: [PATCH 5/6] core: Move unit initialization after memory initialization

['Pratyush Yadav' via Jailhouse]

On 03/07/19 3:16 PM, Jan Kiszka wrote:
> On 03.07.19 11:12, Pratyush Yadav wrote:
>>
>>
>> On 02/07/19 9:18 PM, Jan Kiszka wrote:
>>> On 02.07.19 16:36, Pratyush Yadav wrote:
>>>> The SMMU driver needs access to guest paging structures, so they need to
>>>> be initialized before we can initialize the driver.
>>>
>>> No signed-off - because you were not sure if this is ready? ;)
>>
>> Quite the opposite. I was so sure this simple change was correct, I didn't 
>> double-check the patch and missed the signed-off.
>>
>>> The bad news: it isn't. The x86 IOMMUs have to be initialized prior to 
>>> calling arch_map_memory_region because that calls iommu_map_memory_region.
>>>
>>> Can you describe in more details why the SMMU driver needs that guest 
>>> paging access during init, and why that can't be solved by hooking into 
>>> ARM's arch_map_memory_region?
>>
>> SMMU's init doesn't need guest paging access, but arm_smmuv3_cell_init() 
>> does. Linux might have already initialised some stream table entries before 
>> Jailhouse was enabled. We need to copy those entries in the hypervisor's 
>> stream table. This is done in arm_smmuv3_cell_init(). But for some reason, 
>> cell_init() is not called for the root cell. So I call it from 
>> arm_smmuv3_init().
>>
>> This seemed like a simple enough change to work around the problem. Calling 
>> cell_init() from a later point works just as fine. Any particular reason why 
>> cell_init() is not called for the root cell? I see a lot of drivers (ioapic, 
>> vtd, etc) calling it in their init functions.
> 
> I suppose you mean the cell_init unit callback (cell_init() is called already 
> during early_init()): That is called on cell_create only. We could also call 
> the cell_init callbacks after unit initialization from init_late(), but that 
> requires some careful checks if there are ordering issue. However, that 
> wouldn't solve your issue.
> 
> But your problem is not that different from vtd.c e.g. It also needs to 
> access the guest pages in order to transfer interrupt remapping information 
> into the new IR table. That is done during config_commit, and you should just 
> do the same.

config_commit() is called at both cell init and destroy. I don't see a way to 
differentiate between the two. I need it to run only one time when the cell 
comes up. The cleanup is handled by the unit's cell_exit() callback. Any other 
way to do this?

> Jan
> 

-- 
Regards,
Pratyush Yadav

-- 
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/47bf5f44-f36c-1aad-3ff4-d4e315d4c837%40ti.com.
For more options, visit https://groups.google.com/d/optout.