diff mbox series

[v11,02/10] hvf: Add execute to dirty log permission bitmap

Message ID 20210915181049.27597-3-agraf@csgraf.de (mailing list archive)
State New, archived
Headers show
Series hvf: Implement Apple Silicon Support | expand

Commit Message

Alexander Graf Sept. 15, 2021, 6:10 p.m. UTC
Hvf's permission bitmap during and after dirty logging does not include
the HV_MEMORY_EXEC permission. At least on Apple Silicon, this leads to
instruction faults once dirty logging was enabled.

Add the bit to make it work properly.

Signed-off-by: Alexander Graf <agraf@csgraf.de>
---
 accel/hvf/hvf-accel-ops.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Peter Maydell Sept. 16, 2021, 11:59 a.m. UTC | #1
On Wed, 15 Sept 2021 at 19:10, Alexander Graf <agraf@csgraf.de> wrote:
>
> Hvf's permission bitmap during and after dirty logging does not include
> the HV_MEMORY_EXEC permission. At least on Apple Silicon, this leads to
> instruction faults once dirty logging was enabled.
>
> Add the bit to make it work properly.
>
> Signed-off-by: Alexander Graf <agraf@csgraf.de>
> ---
>  accel/hvf/hvf-accel-ops.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/accel/hvf/hvf-accel-ops.c b/accel/hvf/hvf-accel-ops.c
> index d1691be989..71cc2fa70f 100644
> --- a/accel/hvf/hvf-accel-ops.c
> +++ b/accel/hvf/hvf-accel-ops.c
> @@ -239,12 +239,12 @@ static void hvf_set_dirty_tracking(MemoryRegionSection *section, bool on)
>      if (on) {
>          slot->flags |= HVF_SLOT_LOG;
>          hv_vm_protect((uintptr_t)slot->start, (size_t)slot->size,
> -                      HV_MEMORY_READ);
> +                      HV_MEMORY_READ | HV_MEMORY_EXEC);
>      /* stop tracking region*/
>      } else {
>          slot->flags &= ~HVF_SLOT_LOG;
>          hv_vm_protect((uintptr_t)slot->start, (size_t)slot->size,
> -                      HV_MEMORY_READ | HV_MEMORY_WRITE);
> +                      HV_MEMORY_READ | HV_MEMORY_WRITE | HV_MEMORY_EXEC);
>      }
>  }

Makes sense -- this matches the premissions we set initially
for memory regions in hvf_set_phys_mem().

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Should we change also the hv_vm_protect() call in
target/i386/hvf/hvf.c:ept_emulation_fault(), for consistency ?

thanks
-- PMM
Alexander Graf Sept. 16, 2021, 2:04 p.m. UTC | #2
On 16.09.21 13:59, Peter Maydell wrote:
> On Wed, 15 Sept 2021 at 19:10, Alexander Graf <agraf@csgraf.de> wrote:
>> Hvf's permission bitmap during and after dirty logging does not include
>> the HV_MEMORY_EXEC permission. At least on Apple Silicon, this leads to
>> instruction faults once dirty logging was enabled.
>>
>> Add the bit to make it work properly.
>>
>> Signed-off-by: Alexander Graf <agraf@csgraf.de>
>> ---
>>  accel/hvf/hvf-accel-ops.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/accel/hvf/hvf-accel-ops.c b/accel/hvf/hvf-accel-ops.c
>> index d1691be989..71cc2fa70f 100644
>> --- a/accel/hvf/hvf-accel-ops.c
>> +++ b/accel/hvf/hvf-accel-ops.c
>> @@ -239,12 +239,12 @@ static void hvf_set_dirty_tracking(MemoryRegionSection *section, bool on)
>>      if (on) {
>>          slot->flags |= HVF_SLOT_LOG;
>>          hv_vm_protect((uintptr_t)slot->start, (size_t)slot->size,
>> -                      HV_MEMORY_READ);
>> +                      HV_MEMORY_READ | HV_MEMORY_EXEC);
>>      /* stop tracking region*/
>>      } else {
>>          slot->flags &= ~HVF_SLOT_LOG;
>>          hv_vm_protect((uintptr_t)slot->start, (size_t)slot->size,
>> -                      HV_MEMORY_READ | HV_MEMORY_WRITE);
>> +                      HV_MEMORY_READ | HV_MEMORY_WRITE | HV_MEMORY_EXEC);
>>      }
>>  }
> Makes sense -- this matches the premissions we set initially
> for memory regions in hvf_set_phys_mem().
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> Should we change also the hv_vm_protect() call in
> target/i386/hvf/hvf.c:ept_emulation_fault(), for consistency ?


The x86 hvf code seems to deal just fine with !X mappings, so I'd leave
it as is as part of the arm enablement series - especially because I
have limited testing capabilities for x86 hvf.

Roman, Cameron, would you like to pick up the ept_emulation_fault() part?


Alex
Peter Maydell Sept. 16, 2021, 2:05 p.m. UTC | #3
On Thu, 16 Sept 2021 at 15:04, Alexander Graf <agraf@csgraf.de> wrote:
>
>
> On 16.09.21 13:59, Peter Maydell wrote:
> > On Wed, 15 Sept 2021 at 19:10, Alexander Graf <agraf@csgraf.de> wrote:
> >> Hvf's permission bitmap during and after dirty logging does not include
> >> the HV_MEMORY_EXEC permission. At least on Apple Silicon, this leads to
> >> instruction faults once dirty logging was enabled.
> >>
> >> Add the bit to make it work properly.
> >>
> >> Signed-off-by: Alexander Graf <agraf@csgraf.de>
> >> ---
> >>  accel/hvf/hvf-accel-ops.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/accel/hvf/hvf-accel-ops.c b/accel/hvf/hvf-accel-ops.c
> >> index d1691be989..71cc2fa70f 100644
> >> --- a/accel/hvf/hvf-accel-ops.c
> >> +++ b/accel/hvf/hvf-accel-ops.c
> >> @@ -239,12 +239,12 @@ static void hvf_set_dirty_tracking(MemoryRegionSection *section, bool on)
> >>      if (on) {
> >>          slot->flags |= HVF_SLOT_LOG;
> >>          hv_vm_protect((uintptr_t)slot->start, (size_t)slot->size,
> >> -                      HV_MEMORY_READ);
> >> +                      HV_MEMORY_READ | HV_MEMORY_EXEC);
> >>      /* stop tracking region*/
> >>      } else {
> >>          slot->flags &= ~HVF_SLOT_LOG;
> >>          hv_vm_protect((uintptr_t)slot->start, (size_t)slot->size,
> >> -                      HV_MEMORY_READ | HV_MEMORY_WRITE);
> >> +                      HV_MEMORY_READ | HV_MEMORY_WRITE | HV_MEMORY_EXEC);
> >>      }
> >>  }
> > Makes sense -- this matches the premissions we set initially
> > for memory regions in hvf_set_phys_mem().
> >
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> >
> > Should we change also the hv_vm_protect() call in
> > target/i386/hvf/hvf.c:ept_emulation_fault(), for consistency ?
>
>
> The x86 hvf code seems to deal just fine with !X mappings, so I'd leave
> it as is as part of the arm enablement series - especially because I
> have limited testing capabilities for x86 hvf.

Yeah, I should have been clearer -- that would be best as
a separate x86-specific patch.

-- PMM
diff mbox series

Patch

diff --git a/accel/hvf/hvf-accel-ops.c b/accel/hvf/hvf-accel-ops.c
index d1691be989..71cc2fa70f 100644
--- a/accel/hvf/hvf-accel-ops.c
+++ b/accel/hvf/hvf-accel-ops.c
@@ -239,12 +239,12 @@  static void hvf_set_dirty_tracking(MemoryRegionSection *section, bool on)
     if (on) {
         slot->flags |= HVF_SLOT_LOG;
         hv_vm_protect((uintptr_t)slot->start, (size_t)slot->size,
-                      HV_MEMORY_READ);
+                      HV_MEMORY_READ | HV_MEMORY_EXEC);
     /* stop tracking region*/
     } else {
         slot->flags &= ~HVF_SLOT_LOG;
         hv_vm_protect((uintptr_t)slot->start, (size_t)slot->size,
-                      HV_MEMORY_READ | HV_MEMORY_WRITE);
+                      HV_MEMORY_READ | HV_MEMORY_WRITE | HV_MEMORY_EXEC);
     }
 }