Hi Aleksandar and Philippe, On 5/16/19 8:04 PM, Aleksandar Markovic wrote: > > On May 16, 2019 6:31 PM, "Philippe Mathieu-Daudé" <phi...@redhat.com > <mailto:phi...@redhat.com>> wrote: >> >> Hi Jakub, >> >> On 5/16/19 3:10 PM, Jakub Jermar wrote: >> > Hi, >> > >> > On 5/3/19 12:02 PM, Jakub Jermar wrote: >> >> Hi, >> >> >> >> On 4/23/19 4:58 PM, Jakub Jermar wrote: >> >>> Hi Philippe! >> >>> >> >>> On 4/23/19 3:48 PM, Philippe Mathieu-Daudé wrote: >> >>>> Hi Jakub, >> >>>> >> >>>> On 4/23/19 1:00 PM, Jakub Jermář wrote: >> >>>>> This commit addresses QEMU Bug #1825311: >> >>>>> >> >>>>> mips_cpu_handle_mmu_fault renders all accessed pages executable >> >>>>> >> >>>>> It allows finer-grained control over whether the accessed page > should be >> >>>>> executable by moving the decision to the underlying map_address >> >>>>> function, which has more information for this. >> >>>>> >> >>>>> As a result, pages that have the XI bit set in the TLB and are > accessed >> >>>>> for read/write, don't suddenly end up being executable. >> >>>>> >> >>>> >> >>>> Fixes: https://bugs.launchpad.net/qemu/+bug/1825311 >> >>>> >> >>>>> Signed-off-by: Jakub Jermář <jakub.jer...@kernkonzept.com > <mailto:jakub.jer...@kernkonzept.com>> >> >>>>> --- >> >>>>> target/mips/helper.c | 17 ++++++++++------- >> >>>>> 1 file changed, 10 insertions(+), 7 deletions(-) >> >>>>> >> >>>>> diff --git a/target/mips/helper.c b/target/mips/helper.c >> >>>>> index c44cdca3b5..132d073fbe 100644 >> >>>>> --- a/target/mips/helper.c >> >>>>> +++ b/target/mips/helper.c >> >>>>> @@ -43,7 +43,7 @@ int no_mmu_map_address (CPUMIPSState *env, > hwaddr *physical, int *prot, >> >>>>> target_ulong address, int rw, int > access_type) >> >>>>> { >> >>>>> *physical = address; >> >>>>> - *prot = PAGE_READ | PAGE_WRITE; >> >>>>> + *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC; >> >>>>> return TLBRET_MATCH; >> >>>>> } >> >>>>> >> >>>>> @@ -61,7 +61,7 @@ int fixed_mmu_map_address (CPUMIPSState *env, > hwaddr *physical, int *prot, >> >>>>> else >> >>>>> *physical = address; >> >>>>> >> >>>>> - *prot = PAGE_READ | PAGE_WRITE; >> >>>>> + *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC; >> >>>>> return TLBRET_MATCH; >> >>>>> } >> >>>>> >> >>>>> @@ -101,6 +101,9 @@ int r4k_map_address (CPUMIPSState *env, > hwaddr *physical, int *prot, >> >>>>> *prot = PAGE_READ; >> >>>>> if (n ? tlb->D1 : tlb->D0) >> >>>>> *prot |= PAGE_WRITE; >> >>>>> + if (!(n ? tlb->XI1 : tlb->XI0)) { >> >>>>> + *prot |= PAGE_EXEC; >> >>>>> + } >> >>>> >> >>>> This was indeed missed in commit 2fb58b73746e. >> >> Aleksandar, if this patch is OK with you, can you amend this comment, >> and add the "Fixes:" tag too when applying? Thanks! > > Yes, definitely, Philippe, that is not a problem. > > Thanks for helping out! > > I tested Jakub's scenario too, it works as expected, but I am not > concerned about it as much as about regression tests. Knowing that you > have many MIPS test kernels and images, may I ask you to test some of > them WITH Jakub's fix (so indepenently of myself anf Jakub), just to > confirm that there are no regressions?
May I suggest to include also the following mips32r2 HelenOS images with the following command lines to the set of test kernels used for verifying new versions of QEMU? I always test HelenOS master on malta when there is a new version of QEMU, but it might be better to spot prospective issues earlier for both projects: http://www.helenos.org/releases/HelenOS-0.9.1-mips32-malta-be.boot qemu-system-mips -cpu 4Kc -kernel HelenOS-0.9.1-mips32-malta-be.boot -nographic http://www.helenos.org/releases/HelenOS-0.9.1-mips32-malta-le.boot qemu-system-mipsel -cpu 4Kc -kernel HelenOS-0.9.1-mips32-malta-le.boot -nographic Cheers, Jakub > > C’est vraiment gentil de votre part. > > Aleksandar > >> Reviewed-by: Philippe Mathieu-Daudé <phi...@redhat.com > <mailto:phi...@redhat.com>> >> >> >>>> >> >>>>> return TLBRET_MATCH; >> >>>>> } >> >>>>> return TLBRET_DIRTY; >> >>>>> @@ -182,7 +185,7 @@ static int > get_seg_physical_address(CPUMIPSState *env, hwaddr *physical, >> >>>>> } else { >> >>>>> /* The segment is unmapped */ >> >>>>> *physical = physical_base | (real_address & segmask); >> >>>>> - *prot = PAGE_READ | PAGE_WRITE; >> >>>>> + *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC; >> >>>>> return TLBRET_MATCH; >> >>>>> } >> >>>>> } >> >>>>> @@ -913,8 +916,8 @@ int mips_cpu_handle_mmu_fault(CPUState *cs, > vaddr address, int size, int rw, >> >>>>> } >> >>>>> if (ret == TLBRET_MATCH) { >> >>>>> tlb_set_page(cs, address & TARGET_PAGE_MASK, >> >>>>> - physical & TARGET_PAGE_MASK, prot | PAGE_EXEC, >> >>>>> - mmu_idx, TARGET_PAGE_SIZE); >> >>>>> + physical & TARGET_PAGE_MASK, prot, mmu_idx, >> >>>>> + TARGET_PAGE_SIZE); >> >>>>> ret = 0; >> >>>>> } else if (ret < 0) >> >>>>> #endif >> >>>>> @@ -936,8 +939,8 @@ int mips_cpu_handle_mmu_fault(CPUState *cs, > vaddr address, int size, int rw, >> >>>>> address, rw, > access_type, mmu_idx); >> >>>>> if (ret == TLBRET_MATCH) { >> >>>>> tlb_set_page(cs, address & TARGET_PAGE_MASK, >> >>>>> - physical & TARGET_PAGE_MASK, prot | > PAGE_EXEC, >> >>>>> - mmu_idx, TARGET_PAGE_SIZE); >> >>>>> + physical & TARGET_PAGE_MASK, prot, > mmu_idx, >> >>>>> + TARGET_PAGE_SIZE); >> >>>>> ret = 0; >> >>>>> return ret; >> >>>>> } >> >>>>> >> >>>> >> >>>> Your patch looks correct, but I'd like to test it. >> >>>> Do you have a reproducer? >> >>>> Can you describe the command line you used? >> >>> >> >>> I've just attached a reproducer image and script to the bug. It's a >> >>> 32-bit little-endian test binary running on top of the L4Re > microkernel. >> >> I can't get the "TAP" output you described on launchpad. >> >> >>> Let me know if you also need a 64-bit version. >> >> 64-bit version is welcomed. >> >> >>> I tested both 32 and 64-bit versions of the reproducer and also > checked >> >>> to see that the the other images I have lying around here (Linux > 2.6.32 >> >>> big endian and HelenOS master little-endian, both 32-bit for 4Kc) >> >>> continue to run without regressions. >> >> Yes, definitively an improvement: >> Tested-by: Philippe Mathieu-Daudé <phi...@redhat.com > <mailto:phi...@redhat.com>> >> >> Regards, >> >> Phil. >> > -- Kernkonzept GmbH at Dresden, Germany, HRB 31129, CEO Dr.-Ing. Michael Hohmuth