| Message ID | 20210915181049.27597-3-agraf@csgraf.de |
|---|---|
| State | New |
| Headers | show |
| Series | hvf: Implement Apple Silicon Support | expand |
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
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
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 --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); } }
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(-)