Re: [Qemu-devel] [PATCH 00/12] Refactor get_phys_addr() not to return FSR values

["Edgar E. Iglesias" <edgar.iglesias@xilinx.com>]

On Tue, Dec 05, 2017 at 07:46:20PM +0000, Peter Maydell wrote:
> Currently get_phys_addr() and its various subfunctions return a
> hard-coded fault status register value for translation failures. This
> is awkward because FSR values these days may be either long-descriptor
> format or short-descriptor format.  Worse, the right FSR type to use
> doesn't depend only on the translation table being walked -- some
> cases, like fault info reported to AArch32 EL2 for some kinds of ATS
> operation, must be in long-descriptor format even if the translation
> table being walked was short format. We can't get those cases right
> with our current approach. (We put in a bodge fix so we could at
> least run Xen, in commit 50cd71b0d347c7.)
>     
> This series does a refactoring of get_phys_addr() so that instead
> of returning FSR values it puts information in the existing
> ARMMMUFaultInfo structure that is sufficient to construct an
> FSR value, and the callers then do that using the right condition
> for whatever they're doing. I've included Edgar's patch from back
> in June that fixes the handling of ATS instructions, which is most
> of the point of the refactoring.
> 
> Rather than doing it all in one massive unreviewable patch, the
> series is structured as:
>  * patch 1: add fields to ARMMMUFaultInfo, and the utility functions
>    that return a long or short format FSR given a fault info struct
>  * patch 2: stop passing an fsr argument to arm_ld*_ptw(), since
>    nothing actually looks at the result. (This breaks a cycle
>    arm_ldq_ptw()->S1_ptw_translate()->get_phys_addr_lpae()->arm_ldq_ptw()
>    which would otherwise make a stepwise refactoring awkward.)
>  * patches 3 to 8: for each of the different subfunctions of
>    get_phys_addr(), make them fill in the fault info fields instead
>    of an fsr value. Temporarily we add calls to arm_fi_to_sfsc()
>    and arm_fi_to_lfsc() at the callsites in get_phys_addr(), since
>    the callers of get_phys_addr() still need the fsr values. These
>    will go away again in a later patch.
>  * patches 9 and 10: change the callers of get_phys_addr() to use
>    the info in the fault info struct and ignore the fsr value.
>  * patch 11: remove the now unused fsr argument (and the temporary
>    calls to arm_fi_to_sfsc()/arm_fi_to_lfsc())
>  * patch 12: Edgar's patch for ATS PAR format
> 
> Arguably it would be good to push the "create the FSR value"
> logic further, into the point where the exception is taken.
> (This would let us correct the oddity where for M profile we
> get an A/R profile FSR value in arm_v7m_cpu_do_interrupt()
> which we then have to convert into the appropriate M profile
> fault status bits.) However, migration backcompat makes this
> tricky, because at the moment we migrate env->exception.fsr,
> and so we'd need to have code just to handle inbound migrations
> from old QEMU with an FSR and reconstitute a fault info struct.
> So I've stopped here, at least for now.
> 
> This whole series sits on top of my v8M TT patchset (which also
> had to touch some of the get_phys_addr() code). I've pushed it
> to this git branch if that's more convenient:
> 
>  https://git.linaro.org/people/peter.maydell/qemu-arm.git fsr-in-faultinfo
> 
> I have tested a bit (including a Linux KVM EL2 setup), but more
> testing would definitely be useful. Stefano: in particular it would
> be good to check this hasn't broken Xen again :-)
> 
> Based-on: 1512153879-5291-1-git-send-email-peter.maydell@linaro.org
> ([PATCH 0/7] armv8m: Implement TT, and other bugfixes)


Hi Peter,

Thans for working on this, the series looks good to me!
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Cheers,
Edgar


> 
> thanks
> -- PMM
> 
> 
> Edgar E. Iglesias (1):
>   target/arm: Extend PAR format determination
> 
> Peter Maydell (11):
>   target/arm: Provide fault type enum and FSR conversion functions
>   target/arm: Remove fsr argument from arm_ld*_ptw()
>   target/arm: Convert get_phys_addr_v5() to not return FSC values
>   target/arm: Convert get_phys_addr_v6() to not return FSC values
>   target/arm: Convert get_phys_addr_lpae() to not return FSC values
>   target/arm: Convert get_phys_addr_pmsav5() to not return FSC values
>   target/arm: Convert get_phys_addr_pmsav7() to not return FSC values
>   target/arm: Convert get_phys_addr_pmsav8() to not return FSC values
>   target/arm: Use ARMMMUFaultInfo in deliver_fault()
>   target/arm: Ignore fsr from get_phys_addr() in do_ats_write()
>   target/arm: Remove fsr argument from get_phys_addr() and
>     arm_tlb_fill()
> 
>  target/arm/internals.h | 187 +++++++++++++++++++++++++++++++++++++-
>  target/arm/helper.c    | 238 +++++++++++++++++++++++++++----------------------
>  target/arm/op_helper.c |  82 +++++------------
>  3 files changed, 342 insertions(+), 165 deletions(-)
> 
> -- 
> 2.7.4
>