| Message ID | 20180716172712.20294-4-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Bunch of clean-up/improvement | expand |
On Mon, 16 Jul 2018, Julien Grall wrote: > A couple of places in the code will need to clear/set flags in HCR_EL2 > for a given vCPU and then replicate into the hardware. Introduce > helpers and replace open-coded version. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> The macros look good, but I grepped for them in your series and there are no more callers. What places are you referring to that they will need them? > --- > > I haven't find a good place for those new helpers so stick to > processor.h at the moment. This require to use macro rather than inline > helpers given that processor.h is usually the root of all headers. > --- > xen/arch/arm/traps.c | 3 +-- > xen/include/asm-arm/processor.h | 18 ++++++++++++++++++ > 2 files changed, 19 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c > index 9ae64ae6fc..d1bf69b245 100644 > --- a/xen/arch/arm/traps.c > +++ b/xen/arch/arm/traps.c > @@ -681,8 +681,7 @@ static void inject_vabt_exception(struct cpu_user_regs *regs) > break; > } > > - current->arch.hcr_el2 |= HCR_VA; > - WRITE_SYSREG(current->arch.hcr_el2, HCR_EL2); > + vcpu_hcr_set_flags(current, HCR_VA); > } > > /* > diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h > index 222a02dd99..7e695c2418 100644 > --- a/xen/include/asm-arm/processor.h > +++ b/xen/include/asm-arm/processor.h > @@ -843,6 +843,24 @@ void abort_guest_exit_end(void); > : : : "memory"); \ > } while (0) > > +/* > + * Clear/Set flags in HCR_EL2 for a given vCPU. It only supports the current > + * vCPU for now. > + */ > +#define vcpu_hcr_clear_flags(v, flags) \ > + do { \ > + ASSERT((v) == current); \ > + (v)->arch.hcr_el2 &= ~(flags); \ > + WRITE_SYSREG((v)->arch.hcr_el2, HCR_EL2); \ > + } while (0) > + > +#define vcpu_hcr_set_flags(v, flags) \ > + do { \ > + ASSERT((v) == current); \ > + (v)->arch.hcr_el2 |= (flags); \ > + WRITE_SYSREG((v)->arch.hcr_el2, HCR_EL2); \ > + } while (0) > + > #endif /* __ASSEMBLY__ */ > #endif /* __ASM_ARM_PROCESSOR_H */ > /* > -- > 2.11.0 >
Hi Stefano, On 08/14/2018 09:46 PM, Stefano Stabellini wrote: > On Mon, 16 Jul 2018, Julien Grall wrote: >> A couple of places in the code will need to clear/set flags in HCR_EL2 >> for a given vCPU and then replicate into the hardware. Introduce >> helpers and replace open-coded version. >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> > > The macros look good, but I grepped for them in your series and there > are no more callers. What places are you referring to that they will > need them? I split some of my work in 2 part to reduce the size of the series. This will be used in another series where updating HCR for trapping TTBR/SCTLR* will be dynamic. Although in general, this is an improvement compare to the current code as it makes clear what needs to be updated when modifying HCR. Cheers,
On Tue, 14 Aug 2018, Julien Grall wrote: > Hi Stefano, > > On 08/14/2018 09:46 PM, Stefano Stabellini wrote: > > On Mon, 16 Jul 2018, Julien Grall wrote: > > > A couple of places in the code will need to clear/set flags in HCR_EL2 > > > for a given vCPU and then replicate into the hardware. Introduce > > > helpers and replace open-coded version. > > > > > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > > > > The macros look good, but I grepped for them in your series and there > > are no more callers. What places are you referring to that they will > > need them? > > I split some of my work in 2 part to reduce the size of the series. This will > be used in another series where updating HCR for trapping TTBR/SCTLR* will be > dynamic. > > Although in general, this is an improvement compare to the current code as it > makes clear what needs to be updated when modifying HCR. Yes, it is, but normally I would only introduce vcpu_hcr_set_flags in this series, because vcpu_hcr_clear_flags is left completely unused? I also don't mind introducing it anyway if you have a series coming.
Hi Stefano, On 08/14/2018 10:49 PM, Stefano Stabellini wrote: > On Tue, 14 Aug 2018, Julien Grall wrote: >> Hi Stefano, >> >> On 08/14/2018 09:46 PM, Stefano Stabellini wrote: >>> On Mon, 16 Jul 2018, Julien Grall wrote: >>>> A couple of places in the code will need to clear/set flags in HCR_EL2 >>>> for a given vCPU and then replicate into the hardware. Introduce >>>> helpers and replace open-coded version. >>>> >>>> Signed-off-by: Julien Grall <julien.grall@linaro.org> >>> >>> The macros look good, but I grepped for them in your series and there >>> are no more callers. What places are you referring to that they will >>> need them? >> >> I split some of my work in 2 part to reduce the size of the series. This will >> be used in another series where updating HCR for trapping TTBR/SCTLR* will be >> dynamic. >> >> Although in general, this is an improvement compare to the current code as it >> makes clear what needs to be updated when modifying HCR. > > Yes, it is, but normally I would only introduce vcpu_hcr_set_flags in > this series, because vcpu_hcr_clear_flags is left completely unused? Well, it makes sense to keep the pair together. I am happy to move that patch in the other if you prefer. Cheers,
On Tue, 14 Aug 2018, Julien Grall wrote: > Hi Stefano, > > On 08/14/2018 10:49 PM, Stefano Stabellini wrote: > > On Tue, 14 Aug 2018, Julien Grall wrote: > > > Hi Stefano, > > > > > > On 08/14/2018 09:46 PM, Stefano Stabellini wrote: > > > > On Mon, 16 Jul 2018, Julien Grall wrote: > > > > > A couple of places in the code will need to clear/set flags in HCR_EL2 > > > > > for a given vCPU and then replicate into the hardware. Introduce > > > > > helpers and replace open-coded version. > > > > > > > > > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > > > > > > > > The macros look good, but I grepped for them in your series and there > > > > are no more callers. What places are you referring to that they will > > > > need them? > > > > > > I split some of my work in 2 part to reduce the size of the series. This > > > will > > > be used in another series where updating HCR for trapping TTBR/SCTLR* will > > > be > > > dynamic. > > > > > > Although in general, this is an improvement compare to the current code as > > > it > > > makes clear what needs to be updated when modifying HCR. > > > > Yes, it is, but normally I would only introduce vcpu_hcr_set_flags in > > this series, because vcpu_hcr_clear_flags is left completely unused? > > Well, it makes sense to keep the pair together. I am happy to move that patch > in the other if you prefer. Sounds good. When you do that, you can directly add my reviewed-by.
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index 9ae64ae6fc..d1bf69b245 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -681,8 +681,7 @@ static void inject_vabt_exception(struct cpu_user_regs *regs) break; } - current->arch.hcr_el2 |= HCR_VA; - WRITE_SYSREG(current->arch.hcr_el2, HCR_EL2); + vcpu_hcr_set_flags(current, HCR_VA); } /* diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h index 222a02dd99..7e695c2418 100644 --- a/xen/include/asm-arm/processor.h +++ b/xen/include/asm-arm/processor.h @@ -843,6 +843,24 @@ void abort_guest_exit_end(void); : : : "memory"); \ } while (0) +/* + * Clear/Set flags in HCR_EL2 for a given vCPU. It only supports the current + * vCPU for now. + */ +#define vcpu_hcr_clear_flags(v, flags) \ + do { \ + ASSERT((v) == current); \ + (v)->arch.hcr_el2 &= ~(flags); \ + WRITE_SYSREG((v)->arch.hcr_el2, HCR_EL2); \ + } while (0) + +#define vcpu_hcr_set_flags(v, flags) \ + do { \ + ASSERT((v) == current); \ + (v)->arch.hcr_el2 |= (flags); \ + WRITE_SYSREG((v)->arch.hcr_el2, HCR_EL2); \ + } while (0) + #endif /* __ASSEMBLY__ */ #endif /* __ASM_ARM_PROCESSOR_H */ /*
A couple of places in the code will need to clear/set flags in HCR_EL2 for a given vCPU and then replicate into the hardware. Introduce helpers and replace open-coded version. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- I haven't find a good place for those new helpers so stick to processor.h at the moment. This require to use macro rather than inline helpers given that processor.h is usually the root of all headers. --- xen/arch/arm/traps.c | 3 +-- xen/include/asm-arm/processor.h | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-)