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

['Pratyush Yadav' via Jailhouse]

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.
 
> Jan
> 
>> ---
>>   hypervisor/setup.c | 14 +++++++-------
>>   1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/hypervisor/setup.c b/hypervisor/setup.c
>> index e3b1b3c8..c4032f5b 100644
>> --- a/hypervisor/setup.c
>> +++ b/hypervisor/setup.c
>> @@ -174,13 +174,6 @@ static void init_late(void)
>>           return;
>>       }
>>   -    for_each_unit(unit) {
>> -        printk("Initializing unit: %s\n", unit->name);
>> -        error = unit->init();
>> -        if (error)
>> -            return;
>> -    }
>> -
>>       for_each_mem_region(mem, root_cell.config, n) {
>>           if (JAILHOUSE_MEMORY_IS_SUBPAGE(mem))
>>               error = mmio_subpage_register(&root_cell, mem);
>> @@ -190,6 +183,13 @@ static void init_late(void)
>>               return;
>>       }
>>   +    for_each_unit(unit) {
>> +        printk("Initializing unit: %s\n", unit->name);
>> +        error = unit->init();
>> +        if (error)
>> +            return;
>> +    }
>> +
>>       config_commit(&root_cell);
>>         paging_dump_stats("after late setup");
>>
> 

-- 
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/7d39456a-9797-e83c-1cba-63d21b374e51%40ti.com.
For more options, visit https://groups.google.com/d/optout.