[RFC,v5,13/16] skiboot/imc: Disable IMC node when UV enabled
diff mbox series

Message ID 20200227204023.22125-14-grimm@linux.ibm.com
State New
Headers show
Series
  • Ultravisor support in skiboot
Related show

Checks

Context Check Description
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco success Signed-off-by present
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot success Test snowpatch/job/snowpatch-skiboot on branch master
snowpatch_ozlabs/apply_patch success Successfully applied on branch master (82aed17a5468aff6b600ee1694a10a60f942c018)

Commit Message

Ryan Grimm Feb. 27, 2020, 8:40 p.m. UTC
From: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>

Remove the IMC nodes when the ultravisor is enabled,
since both HOMER and IMC scoms are not accessable in
hypervisor state.

Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
---
 hw/imc.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Alexey Kardashevskiy March 12, 2020, 1:19 a.m. UTC | #1
On 28/02/2020 07:40, Ryan Grimm wrote:
> From: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
> 
> Remove the IMC nodes when the ultravisor is enabled,
> since both HOMER and IMC scoms are not accessable in
> hypervisor state.


They are available via UV_READ_SCOM.


> 
> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
> ---
>  hw/imc.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/hw/imc.c b/hw/imc.c
> index 3a5382c0..576eac87 100644
> --- a/hw/imc.c
> +++ b/hw/imc.c
> @@ -603,6 +603,17 @@ imc_mambo:
>  	if (pause_microcode_at_boot())
>  		goto err;
>  
> +	/*
> +	 * If MSR(S) bit is set, disable IMC nodes.
> +	 * IMC nodes need access to specific scom and HOMER region
> +	 * which are not accessible from hypervisor.
> +	 *
> +	 * At this point uv_present cant be used since uv_init()

s/cant/can't/
s/uv_init/init_uv/


> +	 * is called much later. Hencing checking for the MSR bit here.
> +	 */
> +	if (is_msr_bit_set(MSR_S))
> +		goto err;
> +

Do this at the very beginning of imc_init()? Or do not call it at all in
main_cpu_entry() if (is_msr_bit_set(MSR_S))?


>  	/*
>  	 * If the dt_attach_root() fails, "imc-counters" node will not be
>  	 * seen in the device-tree and hence OS should not make any
>
maddy March 24, 2020, 4:46 a.m. UTC | #2
On 3/12/20 6:49 AM, Alexey Kardashevskiy wrote:
>
> On 28/02/2020 07:40, Ryan Grimm wrote:
>> From: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
>>
>> Remove the IMC nodes when the ultravisor is enabled,
>> since both HOMER and IMC scoms are not accessable in
>> hypervisor state.
>
> They are available via UV_READ_SCOM.

SCOM yes, but the latency will be high and
also there are security concerns, hence to disable it.

>
>
>> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
>> ---
>>   hw/imc.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/hw/imc.c b/hw/imc.c
>> index 3a5382c0..576eac87 100644
>> --- a/hw/imc.c
>> +++ b/hw/imc.c
>> @@ -603,6 +603,17 @@ imc_mambo:
>>   	if (pause_microcode_at_boot())
>>   		goto err;
>>   
>> +	/*
>> +	 * If MSR(S) bit is set, disable IMC nodes.
>> +	 * IMC nodes need access to specific scom and HOMER region
>> +	 * which are not accessible from hypervisor.
>> +	 *
>> +	 * At this point uv_present cant be used since uv_init()
> s/cant/can't/
> s/uv_init/init_uv/
Will fix it.

>
>
>> +	 * is called much later. Hencing checking for the MSR bit here.
>> +	 */
>> +	if (is_msr_bit_set(MSR_S))
>> +		goto err;
>> +
> Do this at the very beginning of imc_init()? Or do not call it at all in
> main_cpu_entry() if (is_msr_bit_set(MSR_S))?

We do couple of call to this file before jumping to imc_init(). And
yes I agree, we could move the check to the beginning. Will fix it


Thanks for review
maddy

>
>
>>   	/*
>>   	 * If the dt_attach_root() fails, "imc-counters" node will not be
>>   	 * seen in the device-tree and hence OS should not make any
>>

Patch
diff mbox series

diff --git a/hw/imc.c b/hw/imc.c
index 3a5382c0..576eac87 100644
--- a/hw/imc.c
+++ b/hw/imc.c
@@ -603,6 +603,17 @@  imc_mambo:
 	if (pause_microcode_at_boot())
 		goto err;
 
+	/*
+	 * If MSR(S) bit is set, disable IMC nodes.
+	 * IMC nodes need access to specific scom and HOMER region
+	 * which are not accessible from hypervisor.
+	 *
+	 * At this point uv_present cant be used since uv_init()
+	 * is called much later. Hencing checking for the MSR bit here.
+	 */
+	if (is_msr_bit_set(MSR_S))
+		goto err;
+
 	/*
 	 * If the dt_attach_root() fails, "imc-counters" node will not be
 	 * seen in the device-tree and hence OS should not make any