Re: [Qemu-devel] [PATCH v2] mips: Decide to map PAGE_EXEC in map_address

[Jakub Jermar]

Hi Aleksandar and Philippe,

On 5/16/19 8:04 PM, Aleksandar Markovic wrote:
> 
> On May 16, 2019 6:31 PM, "Philippe Mathieu-Daudé" <address@hidden
> <mailto:address@hidden>> 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ář <address@hidden
> <mailto:address@hidden>>
>> >>>>> ---
>> >>>>>  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é <address@hidden
> <mailto:address@hidden>>
>>
>> >>>>
>> >>>>>                  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é <address@hidden
> <mailto:address@hidden>>
>>
>> Regards,
>>
>> Phil.
>>
> 

-- 
Kernkonzept GmbH at Dresden, Germany, HRB 31129, CEO Dr.-Ing. Michael
Hohmuth