Re: [Xen-devel] [PATCH v2 02/19] xen/arm: Remove vwfi while setting HCR_EL2 in init_traps

[Julien Grall <julien.grall@xxxxxxx>]

Hi Wei,

On 30/03/17 10:13, Wei Chen wrote:
> We will set HCR_EL2 for each domain individually at the place where each
> domain is created. vwfi will affect the behavior of VM trap. Initialize
> the HCR_EL2 in init_traps is a generic setting for AT translations while
> creating dom0.
This statement looks wrong. Any AT s1s2 translations (i.e on behalf of 
the guest) should be done after a call to p2m_restore_state that restore 
HCR_EL2, SCTLR_EL1, VTTBR_EL1. If it is not the case, then there is an 
error.
> After dom0 has been created, the HCR_EL2 will use the saved
> value in dom0's context. So checking vwfi in init_trap is pointless.
>
> Signed-off-by: Wei Chen <Wei.Chen@xxxxxxx>
This is a nack from me. We don't remove feature even temporarily without 
any strong reasons. This is making more difficult to track the history 
of a feature and a call to forget to restore it correctly later on or 
removing the feature if we ever decide to revert the patch which adds 
back the feature (see patch #4).
I would prefer to see patch #2, #3 squashed into #4, the patch will not 
be that big (50 lines) and avoid to drop a feature temporarily. But I am 
not convinced about your reasons.
> ---
> I have tried to remove HCR_EL2 setting from init_traps, but the Xen will
> hang at the place of creating domain0. The console hot key can work, so
> the Xen is alive, not panic.
With the limited description you provided it is hard to know what's 
going on. In the future please try to provide meaningful details 
(platform used, debugging you have done...). Anyway, I have done some 
debug and I don't end up to the same conclusion as you.
I tried on different boards, and it gets stuck when initializing stage-2 
page table configuration on each CPU (see setup_virt_paging). The 
secondary CPUs don't receive the SGI.
Reading a bit more the ARM ARM (D7.2.33 in ARM DDI 0487A.k_iss10775), 
HCR_EL2 is unknown at reset. Whilst I already knew that, I would have 
expected to have no impact on EL2 (at least in the non-VHE case). 
However, the value of the {A,I,F}MO bits will affect the routing of 
physical IRQs to Xen.
I have only gone quickly through the spec, so it might be possible to 
have other necessary bits. It might be good to keep initialization here 
until someone sit in front of the spec for few hours and check everything.
So in this case I would prefer to keep the helper avoiding to have 
declared twice the same flags. Stefano any opinions?
Also, whilst I was debugging the problem I noticed the vwfi option does 
not work properly on CPU0. Indeed, the command line has not been parsed 
when init_traps is called on CPU0. This is should be fixed by patch #4 
in this series, but I don't think we want to backport it. Stefano, would 
you be up to write a patch for this?
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel