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

2019-05-17 Thread Philippe Mathieu-Daudé 
On Fri, May 17, 2019 at 11:30 AM Jakub Jermar
 wrote:
>
> Hi Aleksandar and Philippe,
>
> On 5/16/19 8:04 PM, Aleksandar Markovic wrote:
[...]
> > 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

Yes, I added both ;)

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

2019-05-17 Thread 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é"  > 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ář  >
>> > ---
>> >  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é  >
>>
>> 
>> >                  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,
>> > +           

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

2019-05-17 Thread Jakub Jermar 
Hi Philippe,

On 5/16/19 6:31 PM, Philippe Mathieu-Daudé 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ář 
>> ---
>>  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!
> 
> Reviewed-by: Philippe Mathieu-Daudé 
> 
>
>>  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.

Rarely the entire guest gets stuck early during boot at the following line:

Calibrating timer loop...

This is an unrelated problem, not sure why that happens.

If that is the case, just rerun the reproducer until you get either

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

2019-05-16 Thread Aleksandar Markovic 
On May 16, 2019 10:05 PM, "Philippe Mathieu-Daudé"  wrote:
>
> On 5/16/19 8:04 PM, Aleksandar Markovic wrote:
> > On May 16, 2019 6:31 PM, "Philippe Mathieu-Daudé" 
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ář 
> >>> ---
> >>>  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?
>
> Yes, I can do that (during next WE).
>
> FYI I try to test all QEMU MIPS boards at least once a month (except the
> Jazz board, I don't have handy setup and think Hervé does test it).
> It is time consuming so I'm investing time to offload that testing with
> Avocado slowly. This will take me some months (at least 2 QEMU releases).
>

Avocado MIPS testing was one of the best news for QEMU for MIPS in years.
Even my bosses nodded with approval and interest, which doesn't happen too
often. In other words, it was received very well, excellent.

> I'm also regularly testing embedded firmwares on boards not yet
> upstreamed (which are of my interest) and am working on a Avocado test
> setup too. I wish I can upstream the whole work this year...
>
> > C’est vraiment gentil de votre part.
>
> ;)
>
> >
> > Aleksandar
> >
> >> Reviewed-by: Philippe Mathieu-Daudé 
> >>
> >>
> >>>  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,
> >>> - physica

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

2019-05-16 Thread Philippe Mathieu-Daudé 
On 5/16/19 8:04 PM, Aleksandar Markovic wrote:
> On May 16, 2019 6:31 PM, "Philippe Mathieu-Daudé"  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ář 
>>> ---
>>>  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?

Yes, I can do that (during next WE).

FYI I try to test all QEMU MIPS boards at least once a month (except the
Jazz board, I don't have handy setup and think Hervé does test it).
It is time consuming so I'm investing time to offload that testing with
Avocado slowly. This will take me some months (at least 2 QEMU releases).

I'm also regularly testing embedded firmwares on boards not yet
upstreamed (which are of my interest) and am working on a Avocado test
setup too. I wish I can upstream the whole work this year...

> C’est vraiment gentil de votre part.

;)

> 
> Aleksandar
> 
>> Reviewed-by: Philippe Mathieu-Daudé 
>>
>>
>>>  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);
>>>

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

2019-05-16 Thread Aleksandar Markovic 
On May 16, 2019 6:31 PM, "Philippe Mathieu-Daudé"  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ář 
> > ---
> >  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?

C’est vraiment gentil de votre part.

Aleksandar

> Reviewed-by: Philippe Mathieu-Daudé 
>
> 
> >  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

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

2019-05-16 Thread Philippe Mathieu-Daudé 
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ář 
> ---
>  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!

Reviewed-by: Philippe Mathieu-Daudé 


>  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é 

Regards,

Phi

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

2019-05-16 Thread Aleksandar Markovic 
On May 16, 2019 3:11 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ář 
>  ---
>   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.
> >>>
>   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.
> >> Let me know if you also need a 64-bit version.
> >>
> >> 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.
> >>
> >> Best regards,
> >> Jakub
> >
> > (ping)
> >
> > Is there anything else I can do to help to get this merged?
> >
> >
https://patchew.org/QEMU/20190423110034.1260142-1-jakub.jer...@kernkonzept.com/
>
> Has anyone managed to have a look at this?
>

Sorry for the delay, but there

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

2019-05-16 Thread Jakub Jermar 
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ář 
 ---
  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.
>>>
  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.
>> Let me know if you also need a 64-bit version.
>>
>> 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.
>>
>> Best regards,
>> Jakub
> 
> (ping)
> 
> Is there anything else I can do to help to get this merged?
> 
> https://patchew.org/QEMU/20190423110034.1260142-1-jakub.jer...@kernkonzept.com/

Has anyone managed to have a look at this?

Thanks,
Jakub

> 
> Thanks,
> Jakub
> 

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

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

2019-05-03 Thread Aleksandar Markovic 
> (ping)
> 
> Is there anything else I can do to help to get this merged?
> 
> https://patchew.org/QEMU/20190423110034.1260142-1-jakub.jer...@kernkonzept.com/

Hello, Jakub.

I will be reviewing your patch next week, please be patient. In any case, 
thanks for
your involving in solving this issue!

Aleksandar

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

2019-05-03 Thread Jakub Jermar 
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ář 
>>> ---
>>>  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.
>>
>>>  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.
> Let me know if you also need a 64-bit version.
> 
> 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.
> 
> Best regards,
> Jakub

(ping)

Is there anything else I can do to help to get this merged?

https://patchew.org/QEMU/20190423110034.1260142-1-jakub.jer...@kernkonzept.com/

Thanks,
Jakub

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

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

2019-04-23 Thread Jakub Jermar 
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ář 
>> ---
>>  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.
> 
>>  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.
Let me know if you also need a 64-bit version.

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.

Best regards,
Jakub

> 
> Thanks,
> 
> Phil.
> 

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

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

2019-04-23 Thread Philippe Mathieu-Daudé 
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ář 
> ---
>  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.

>  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?

Thanks,

Phil.

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

2019-04-23 Thread Jakub Jermář 
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.

Signed-off-by: Jakub Jermář 
---
 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;
+}
 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;
 }
-- 
2.20.1