[01/18] xen/arm: Introduce a helper to get default HCR_EL2 flags
diff mbox

Message ID 1489402563-4978-2-git-send-email-Wei.Chen@arm.com
State New, archived
Headers show

Commit Message

Wei Chen March 13, 2017, 10:55 a.m. UTC
We want to add HCR_EL2 register to Xen context switch. And each copy
of HCR_EL2 in vcpu structure will be initialized with the same set
of trap flags as the HCR_EL2 register. We introduce a helper here to
represent these flags to be reused easily.

Signed-off-by: Wei Chen <Wei.Chen@arm.com>
---
 xen/arch/arm/traps.c            | 11 ++++++++---
 xen/include/asm-arm/processor.h |  2 ++
 2 files changed, 10 insertions(+), 3 deletions(-)

Comments

Stefano Stabellini March 15, 2017, 12:24 a.m. UTC | #1
On Mon, 13 Mar 2017, Wei Chen wrote:
> We want to add HCR_EL2 register to Xen context switch. And each copy
> of HCR_EL2 in vcpu structure will be initialized with the same set
> of trap flags as the HCR_EL2 register. We introduce a helper here to
> represent these flags to be reused easily.
> 
> Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> ---
>  xen/arch/arm/traps.c            | 11 ++++++++---
>  xen/include/asm-arm/processor.h |  2 ++
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 614501f..d343c66 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -115,6 +115,13 @@ static void __init parse_vwfi(const char *s)
>  }
>  custom_param("vwfi", parse_vwfi);
>  
> +register_t get_default_hcr_flags(void)
> +{
> +    return  (HCR_PTW|HCR_BSU_INNER|HCR_AMO|HCR_IMO|HCR_FMO|HCR_VM|
> +             (vwfi != NATIVE ? (HCR_TWI|HCR_TWE) : 0) |
> +             HCR_TSC|HCR_TAC|HCR_SWIO|HCR_TIDCP|HCR_FB);
> +}

I haven't finished reading this series yet, but I would make this a
static inline function if possible


>  void init_traps(void)
>  {
>      /* Setup Hyp vector base */
> @@ -139,9 +146,7 @@ void init_traps(void)
>                   CPTR_EL2);
>  
>      /* Setup hypervisor traps */
> -    WRITE_SYSREG(HCR_PTW|HCR_BSU_INNER|HCR_AMO|HCR_IMO|HCR_FMO|HCR_VM|
> -                 (vwfi != NATIVE ? (HCR_TWI|HCR_TWE) : 0) |
> -                 HCR_TSC|HCR_TAC|HCR_SWIO|HCR_TIDCP|HCR_FB,HCR_EL2);
> +    WRITE_SYSREG(get_default_hcr_flags(), HCR_EL2);
>      isb();
>  }
>  
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index afc0e9a..4b6338b 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -708,6 +708,8 @@ int call_smc(register_t function_id, register_t arg0, register_t arg1,
>  
>  void do_trap_guest_error(struct cpu_user_regs *regs);
>  
> +register_t get_default_hcr_flags(void);
> +
>  #endif /* __ASSEMBLY__ */
>  #endif /* __ASM_ARM_PROCESSOR_H */
>  /*
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
>
Wei Chen March 15, 2017, 7:19 a.m. UTC | #2
Hi Stefano,

On 2017/3/15 8:24, Stefano Stabellini wrote:
> On Mon, 13 Mar 2017, Wei Chen wrote:
>> We want to add HCR_EL2 register to Xen context switch. And each copy
>> of HCR_EL2 in vcpu structure will be initialized with the same set
>> of trap flags as the HCR_EL2 register. We introduce a helper here to
>> represent these flags to be reused easily.
>>
>> Signed-off-by: Wei Chen <Wei.Chen@arm.com>
>> ---
>>  xen/arch/arm/traps.c            | 11 ++++++++---
>>  xen/include/asm-arm/processor.h |  2 ++
>>  2 files changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 614501f..d343c66 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -115,6 +115,13 @@ static void __init parse_vwfi(const char *s)
>>  }
>>  custom_param("vwfi", parse_vwfi);
>>
>> +register_t get_default_hcr_flags(void)
>> +{
>> +    return  (HCR_PTW|HCR_BSU_INNER|HCR_AMO|HCR_IMO|HCR_FMO|HCR_VM|
>> +             (vwfi != NATIVE ? (HCR_TWI|HCR_TWE) : 0) |
>> +             HCR_TSC|HCR_TAC|HCR_SWIO|HCR_TIDCP|HCR_FB);
>> +}
>
> I haven't finished reading this series yet, but I would make this a
> static inline function if possible
>

I had considered to use static inline before. But it must move the

static enum {
	TRAP,
	NATIVE,
} vwfi;

to the header file at the same time. But get_default_hcr_flags would
not be used frequently. So I thought it didn't have enough value to
change a less relevant code to make this function become static inline.

>
>>  void init_traps(void)
>>  {
>>      /* Setup Hyp vector base */
>> @@ -139,9 +146,7 @@ void init_traps(void)
>>                   CPTR_EL2);
>>
>>      /* Setup hypervisor traps */
>> -    WRITE_SYSREG(HCR_PTW|HCR_BSU_INNER|HCR_AMO|HCR_IMO|HCR_FMO|HCR_VM|
>> -                 (vwfi != NATIVE ? (HCR_TWI|HCR_TWE) : 0) |
>> -                 HCR_TSC|HCR_TAC|HCR_SWIO|HCR_TIDCP|HCR_FB,HCR_EL2);
>> +    WRITE_SYSREG(get_default_hcr_flags(), HCR_EL2);
>>      isb();
>>  }
>>
>> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
>> index afc0e9a..4b6338b 100644
>> --- a/xen/include/asm-arm/processor.h
>> +++ b/xen/include/asm-arm/processor.h
>> @@ -708,6 +708,8 @@ int call_smc(register_t function_id, register_t arg0, register_t arg1,
>>
>>  void do_trap_guest_error(struct cpu_user_regs *regs);
>>
>> +register_t get_default_hcr_flags(void);
>> +
>>  #endif /* __ASSEMBLY__ */
>>  #endif /* __ASM_ARM_PROCESSOR_H */
>>  /*
>> --
>> 2.7.4
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> https://lists.xen.org/xen-devel
>>
>
Julien Grall March 15, 2017, 11:01 a.m. UTC | #3
On 15/03/17 07:19, Wei Chen wrote:
> Hi Stefano,

Hello,

> On 2017/3/15 8:24, Stefano Stabellini wrote:
>> On Mon, 13 Mar 2017, Wei Chen wrote:
>>> We want to add HCR_EL2 register to Xen context switch. And each copy
>>> of HCR_EL2 in vcpu structure will be initialized with the same set
>>> of trap flags as the HCR_EL2 register. We introduce a helper here to
>>> represent these flags to be reused easily.
>>>
>>> Signed-off-by: Wei Chen <Wei.Chen@arm.com>
>>> ---
>>>  xen/arch/arm/traps.c            | 11 ++++++++---
>>>  xen/include/asm-arm/processor.h |  2 ++
>>>  2 files changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>> index 614501f..d343c66 100644
>>> --- a/xen/arch/arm/traps.c
>>> +++ b/xen/arch/arm/traps.c
>>> @@ -115,6 +115,13 @@ static void __init parse_vwfi(const char *s)
>>>  }
>>>  custom_param("vwfi", parse_vwfi);
>>>
>>> +register_t get_default_hcr_flags(void)
>>> +{
>>> +    return  (HCR_PTW|HCR_BSU_INNER|HCR_AMO|HCR_IMO|HCR_FMO|HCR_VM|
>>> +             (vwfi != NATIVE ? (HCR_TWI|HCR_TWE) : 0) |
>>> +             HCR_TSC|HCR_TAC|HCR_SWIO|HCR_TIDCP|HCR_FB);
>>> +}
>>
>> I haven't finished reading this series yet, but I would make this a
>> static inline function if possible
>>
>
> I had considered to use static inline before. But it must move the
>
> static enum {
>         TRAP,
>         NATIVE,
> } vwfi;
>
> to the header file at the same time. But get_default_hcr_flags would
> not be used frequently. So I thought it didn't have enough value to
> change a less relevant code to make this function become static inline.

Looking at the spec, HCR_EL2 is controlling the behavior of the VM. We 
only need to ensure this to be set before going to EL1/EL0. Note that 
the reset value of some register are architecturally UNKNOWN, but I 
don't think we care here.

So I would suggest to drop the setting in init_traps and only rely on 
the one in when returning to the guest. And therefore this function 
could be moved in domain.c

Any opinions?

Cheers,
Stefano Stabellini March 15, 2017, 10:31 p.m. UTC | #4
On Wed, 15 Mar 2017, Julien Grall wrote:
> On 15/03/17 07:19, Wei Chen wrote:
> > Hi Stefano,
> 
> Hello,
> 
> > On 2017/3/15 8:24, Stefano Stabellini wrote:
> > > On Mon, 13 Mar 2017, Wei Chen wrote:
> > > > We want to add HCR_EL2 register to Xen context switch. And each copy
> > > > of HCR_EL2 in vcpu structure will be initialized with the same set
> > > > of trap flags as the HCR_EL2 register. We introduce a helper here to
> > > > represent these flags to be reused easily.
> > > > 
> > > > Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> > > > ---
> > > >  xen/arch/arm/traps.c            | 11 ++++++++---
> > > >  xen/include/asm-arm/processor.h |  2 ++
> > > >  2 files changed, 10 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > > > index 614501f..d343c66 100644
> > > > --- a/xen/arch/arm/traps.c
> > > > +++ b/xen/arch/arm/traps.c
> > > > @@ -115,6 +115,13 @@ static void __init parse_vwfi(const char *s)
> > > >  }
> > > >  custom_param("vwfi", parse_vwfi);
> > > > 
> > > > +register_t get_default_hcr_flags(void)
> > > > +{
> > > > +    return  (HCR_PTW|HCR_BSU_INNER|HCR_AMO|HCR_IMO|HCR_FMO|HCR_VM|
> > > > +             (vwfi != NATIVE ? (HCR_TWI|HCR_TWE) : 0) |
> > > > +             HCR_TSC|HCR_TAC|HCR_SWIO|HCR_TIDCP|HCR_FB);
> > > > +}
> > > 
> > > I haven't finished reading this series yet, but I would make this a
> > > static inline function if possible
> > > 
> > 
> > I had considered to use static inline before. But it must move the
> > 
> > static enum {
> >         TRAP,
> >         NATIVE,
> > } vwfi;
> > 
> > to the header file at the same time. But get_default_hcr_flags would
> > not be used frequently. So I thought it didn't have enough value to
> > change a less relevant code to make this function become static inline.
> 
> Looking at the spec, HCR_EL2 is controlling the behavior of the VM. We only
> need to ensure this to be set before going to EL1/EL0. Note that the reset
> value of some register are architecturally UNKNOWN, but I don't think we care
> here.
> 
> So I would suggest to drop the setting in init_traps and only rely on the one
> in when returning to the guest. And therefore this function could be moved in
> domain.c
> 
> Any opinions?

Fine by me
Wei Chen March 16, 2017, 7:44 a.m. UTC | #5
Hi Julien,

On 2017/3/15 19:01, Julien Grall wrote:
> On 15/03/17 07:19, Wei Chen wrote:
>> Hi Stefano,
>
> Hello,
>
>> On 2017/3/15 8:24, Stefano Stabellini wrote:
>>> On Mon, 13 Mar 2017, Wei Chen wrote:
>>>> We want to add HCR_EL2 register to Xen context switch. And each copy
>>>> of HCR_EL2 in vcpu structure will be initialized with the same set
>>>> of trap flags as the HCR_EL2 register. We introduce a helper here to
>>>> represent these flags to be reused easily.
>>>>
>>>> Signed-off-by: Wei Chen <Wei.Chen@arm.com>
>>>> ---
>>>>  xen/arch/arm/traps.c            | 11 ++++++++---
>>>>  xen/include/asm-arm/processor.h |  2 ++
>>>>  2 files changed, 10 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>>> index 614501f..d343c66 100644
>>>> --- a/xen/arch/arm/traps.c
>>>> +++ b/xen/arch/arm/traps.c
>>>> @@ -115,6 +115,13 @@ static void __init parse_vwfi(const char *s)
>>>>  }
>>>>  custom_param("vwfi", parse_vwfi);
>>>>
>>>> +register_t get_default_hcr_flags(void)
>>>> +{
>>>> +    return  (HCR_PTW|HCR_BSU_INNER|HCR_AMO|HCR_IMO|HCR_FMO|HCR_VM|
>>>> +             (vwfi != NATIVE ? (HCR_TWI|HCR_TWE) : 0) |
>>>> +             HCR_TSC|HCR_TAC|HCR_SWIO|HCR_TIDCP|HCR_FB);
>>>> +}
>>>
>>> I haven't finished reading this series yet, but I would make this a
>>> static inline function if possible
>>>
>>
>> I had considered to use static inline before. But it must move the
>>
>> static enum {
>>         TRAP,
>>         NATIVE,
>> } vwfi;
>>
>> to the header file at the same time. But get_default_hcr_flags would
>> not be used frequently. So I thought it didn't have enough value to
>> change a less relevant code to make this function become static inline.
>
> Looking at the spec, HCR_EL2 is controlling the behavior of the VM. We
> only need to ensure this to be set before going to EL1/EL0. Note that
> the reset value of some register are architecturally UNKNOWN, but I
> don't think we care here.
>
> So I would suggest to drop the setting in init_traps and only rely on
> the one in when returning to the guest. And therefore this function
> could be moved in domain.c
>
> Any opinions?
>

It seems good. I will do some test, if everything pass, I will do this
change in next version.

> Cheers,
>

Patch
diff mbox

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 614501f..d343c66 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -115,6 +115,13 @@  static void __init parse_vwfi(const char *s)
 }
 custom_param("vwfi", parse_vwfi);
 
+register_t get_default_hcr_flags(void)
+{
+    return  (HCR_PTW|HCR_BSU_INNER|HCR_AMO|HCR_IMO|HCR_FMO|HCR_VM|
+             (vwfi != NATIVE ? (HCR_TWI|HCR_TWE) : 0) |
+             HCR_TSC|HCR_TAC|HCR_SWIO|HCR_TIDCP|HCR_FB);
+}
+
 void init_traps(void)
 {
     /* Setup Hyp vector base */
@@ -139,9 +146,7 @@  void init_traps(void)
                  CPTR_EL2);
 
     /* Setup hypervisor traps */
-    WRITE_SYSREG(HCR_PTW|HCR_BSU_INNER|HCR_AMO|HCR_IMO|HCR_FMO|HCR_VM|
-                 (vwfi != NATIVE ? (HCR_TWI|HCR_TWE) : 0) |
-                 HCR_TSC|HCR_TAC|HCR_SWIO|HCR_TIDCP|HCR_FB,HCR_EL2);
+    WRITE_SYSREG(get_default_hcr_flags(), HCR_EL2);
     isb();
 }
 
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index afc0e9a..4b6338b 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -708,6 +708,8 @@  int call_smc(register_t function_id, register_t arg0, register_t arg1,
 
 void do_trap_guest_error(struct cpu_user_regs *regs);
 
+register_t get_default_hcr_flags(void);
+
 #endif /* __ASSEMBLY__ */
 #endif /* __ASM_ARM_PROCESSOR_H */
 /*