Re: [PATCH v3 0/2] accel/tcg/tcg-accel-ops-rr: ensure fairness with icount

[Jamie Iles]

Hi Richard,

On Sat, Apr 29, 2023 at 10:28:03AM +0100, Richard Henderson wrote:
> On 4/27/23 03:09, Jamie Iles wrote:
> > From: Jamie Iles 
> > 
> > The round-robin scheduler will iterate over the CPU list with an
> > assigned budget until the next timer expiry and may exit early because
> > of a TB exit.  This is fine under normal operation but with icount
> > enabled and SMP it is possible for a CPU to be starved of run time and
> > the system live-locks.
> > 
> > For example, booting a riscv64 platform with '-icount
> > shift=0,align=off,sleep=on -smp 2' we observe a livelock once the kernel
> > has timers enabled and starts performing TLB shootdowns.  In this case
> > we have CPU 0 in M-mode with interrupts disabled sending an IPI to CPU
> > 1.  As we enter the TCG loop, we assign the icount budget to next timer
> > interrupt to CPU 0 and begin executing where the guest is sat in a busy
> > loop exhausting all of the budget before we try to execute CPU 1 which
> > is the target of the IPI but CPU 1 is left with no budget with which to
> > execute and the process repeats.
> > 
> > We try here to add some fairness by splitting the budget across all of
> > the CPUs on the thread fairly before entering each one.  The CPU count
> > is cached on CPU list generation ID to avoid iterating the list on each
> > loop iteration.  With this change it is possible to boot an SMP rv64
> > guest with icount enabled and no hangs.
> > 
> > New in v3 (address feedback from Richard Henderson):
> >   - Additional patch to use QEMU_LOCK_GUARD with qemu_cpu_list_lock where
> > appropriate
> >   - Move rr_cpu_count() call to be conditional on icount_enabled()
> >   - Initialize cpu_budget to 0
> > 
> > Jamie Iles (2):
> >cpu: expose qemu_cpu_list_lock for lock-guard use
> >accel/tcg/tcg-accel-ops-rr: ensure fairness with icount
> 
> It appears as if one of these two patches causes a failure in replay, e.g.
> 
>   https://gitlab.com/rth7680/qemu/-/jobs/4200609234#L4162
> 
> Would you have a look, please?

I was out on vacation and it looks like this job got cleaned up, but was 
this a mutex check failing for the replay mutex and/or iothread mutex?  
If so then the following patch fixes it for me which I can include in a 
v4

Thanks,

Jamie


diff --git i/accel/tcg/tcg-accel-ops-icount.c w/accel/tcg/tcg-accel-ops-icount.c
index e1e8afaf2f99..3d2cfbbc9778 100644
--- i/accel/tcg/tcg-accel-ops-icount.c
+++ w/accel/tcg/tcg-accel-ops-icount.c
@@ -114,13 +114,13 @@ void icount_prepare_for_run(CPUState *cpu, int64_t 
cpu_budget)
 g_assert(cpu_neg(cpu)->icount_decr.u16.low == 0);
 g_assert(cpu->icount_extra == 0);
 
+replay_mutex_lock();
+
 cpu->icount_budget = MIN(icount_get_limit(), cpu_budget);
 insns_left = MIN(0x, cpu->icount_budget);
 cpu_neg(cpu)->icount_decr.u16.low = insns_left;
 cpu->icount_extra = cpu->icount_budget - insns_left;
 
-replay_mutex_lock();
-
 if (cpu->icount_budget == 0) {
 /*
  * We're called without the iothread lock, so must take it while
diff --git i/replay/replay.c w/replay/replay.c
index c39156c52221..0f7d766efe81 100644
--- i/replay/replay.c
+++ w/replay/replay.c
@@ -74,7 +74,7 @@ uint64_t replay_get_current_icount(void)
 int replay_get_instructions(void)
 {
 int res = 0;
-replay_mutex_lock();
+g_assert(replay_mutex_locked());
 if (replay_next_event_is(EVENT_INSTRUCTION)) {
 res = replay_state.instruction_count;
 if (replay_break_icount != -1LL) {
@@ -85,7 +85,6 @@ int replay_get_instructions(void)
 }
 }
 }
-replay_mutex_unlock();
 return res;
 }
 

[PATCH v3 2/2] accel/tcg/tcg-accel-ops-rr: ensure fairness with icount

[Jamie Iles]

The round-robin scheduler will iterate over the CPU list with an
assigned budget until the next timer expiry and may exit early because
of a TB exit.  This is fine under normal operation but with icount
enabled and SMP it is possible for a CPU to be starved of run time and
the system live-locks.

For example, booting a riscv64 platform with '-icount
shift=0,align=off,sleep=on -smp 2' we observe a livelock once the kernel
has timers enabled and starts performing TLB shootdowns.  In this case
we have CPU 0 in M-mode with interrupts disabled sending an IPI to CPU
1.  As we enter the TCG loop, we assign the icount budget to next timer
interrupt to CPU 0 and begin executing where the guest is sat in a busy
loop exhausting all of the budget before we try to execute CPU 1 which
is the target of the IPI but CPU 1 is left with no budget with which to
execute and the process repeats.

We try here to add some fairness by splitting the budget across all of
the CPUs on the thread fairly before entering each one.  The CPU count
is cached on CPU list generation ID to avoid iterating the list on each
loop iteration.  With this change it is possible to boot an SMP rv64
guest with icount enabled and no hangs.

Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Peter Maydell 
Signed-off-by: Jamie Iles 
---
 accel/tcg/tcg-accel-ops-icount.c | 17 +--
 accel/tcg/tcg-accel-ops-icount.h |  3 ++-
 accel/tcg/tcg-accel-ops-rr.c | 37 +++-
 3 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/accel/tcg/tcg-accel-ops-icount.c b/accel/tcg/tcg-accel-ops-icount.c
index 84cc7421be88..e1e8afaf2f99 100644
--- a/accel/tcg/tcg-accel-ops-icount.c
+++ b/accel/tcg/tcg-accel-ops-icount.c
@@ -89,7 +89,20 @@ void icount_handle_deadline(void)
 }
 }
 
-void icount_prepare_for_run(CPUState *cpu)
+/* Distribute the budget evenly across all CPUs */
+int64_t icount_percpu_budget(int cpu_count)
+{
+int64_t limit = icount_get_limit();
+int64_t timeslice = limit / cpu_count;
+
+if (timeslice == 0) {
+timeslice = limit;
+}
+
+return timeslice;
+}
+
+void icount_prepare_for_run(CPUState *cpu, int64_t cpu_budget)
 {
 int insns_left;
 
@@ -101,7 +114,7 @@ void icount_prepare_for_run(CPUState *cpu)
 g_assert(cpu_neg(cpu)->icount_decr.u16.low == 0);
 g_assert(cpu->icount_extra == 0);
 
-cpu->icount_budget = icount_get_limit();
+cpu->icount_budget = MIN(icount_get_limit(), cpu_budget);
 insns_left = MIN(0x, cpu->icount_budget);
 cpu_neg(cpu)->icount_decr.u16.low = insns_left;
 cpu->icount_extra = cpu->icount_budget - insns_left;
diff --git a/accel/tcg/tcg-accel-ops-icount.h b/accel/tcg/tcg-accel-ops-icount.h
index 1b6fd9c60751..16a301b6dc0b 100644
--- a/accel/tcg/tcg-accel-ops-icount.h
+++ b/accel/tcg/tcg-accel-ops-icount.h
@@ -11,7 +11,8 @@
 #define TCG_ACCEL_OPS_ICOUNT_H
 
 void icount_handle_deadline(void);
-void icount_prepare_for_run(CPUState *cpu);
+void icount_prepare_for_run(CPUState *cpu, int64_t cpu_budget);
+int64_t icount_percpu_budget(int cpu_count);
 void icount_process_data(CPUState *cpu);
 
 void icount_handle_interrupt(CPUState *cpu, int mask);
diff --git a/accel/tcg/tcg-accel-ops-rr.c b/accel/tcg/tcg-accel-ops-rr.c
index 290833a37fb2..5788efa5ff4d 100644
--- a/accel/tcg/tcg-accel-ops-rr.c
+++ b/accel/tcg/tcg-accel-ops-rr.c
@@ -24,6 +24,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/lockable.h"
 #include "sysemu/tcg.h"
 #include "sysemu/replay.h"
 #include "sysemu/cpu-timers.h"
@@ -139,6 +140,33 @@ static void rr_force_rcu(Notifier *notify, void *data)
 rr_kick_next_cpu();
 }
 
+/*
+ * Calculate the number of CPUs that we will process in a single iteration of
+ * the main CPU thread loop so that we can fairly distribute the instruction
+ * count across CPUs.
+ *
+ * The CPU count is cached based on the CPU list generation ID to avoid
+ * iterating the list every time.
+ */
+static int rr_cpu_count(void)
+{
+static unsigned int last_gen_id = ~0;
+static int cpu_count;
+CPUState *cpu;
+
+QEMU_LOCK_GUARD(_cpu_list_lock);
+
+if (cpu_list_generation_id_get() != last_gen_id) {
+cpu_count = 0;
+CPU_FOREACH(cpu) {
+++cpu_count;
+}
+last_gen_id = cpu_list_generation_id_get();
+}
+
+return cpu_count;
+}
+
 /*
  * In the single-threaded case each vCPU is simulated in turn. If
  * there is more than a single vCPU we create a simple timer to kick
@@ -185,11 +213,16 @@ static void *rr_cpu_thread_fn(void *arg)
 cpu->exit_request = 1;
 
 while (1) {
+/* Only used for icount_enabled() */
+int64_t cpu_budget = 0;
+
 qemu_mutex_unlock_iothread();
 replay_mutex_lock();
 qemu_mutex_lock_iothread();
 
 if (icount_enabled()) {
+int cpu_count = rr_cpu_count();
+
 /* Account partial waits to QEMU_CLOCK_VIRTUAL.  

[PATCH v3 0/2] accel/tcg/tcg-accel-ops-rr: ensure fairness with icount

[Jamie Iles]

From: Jamie Iles 

The round-robin scheduler will iterate over the CPU list with an
assigned budget until the next timer expiry and may exit early because
of a TB exit.  This is fine under normal operation but with icount
enabled and SMP it is possible for a CPU to be starved of run time and
the system live-locks.

For example, booting a riscv64 platform with '-icount
shift=0,align=off,sleep=on -smp 2' we observe a livelock once the kernel
has timers enabled and starts performing TLB shootdowns.  In this case
we have CPU 0 in M-mode with interrupts disabled sending an IPI to CPU
1.  As we enter the TCG loop, we assign the icount budget to next timer
interrupt to CPU 0 and begin executing where the guest is sat in a busy
loop exhausting all of the budget before we try to execute CPU 1 which
is the target of the IPI but CPU 1 is left with no budget with which to
execute and the process repeats.

We try here to add some fairness by splitting the budget across all of
the CPUs on the thread fairly before entering each one.  The CPU count
is cached on CPU list generation ID to avoid iterating the list on each
loop iteration.  With this change it is possible to boot an SMP rv64
guest with icount enabled and no hangs.

New in v3 (address feedback from Richard Henderson):
 - Additional patch to use QEMU_LOCK_GUARD with qemu_cpu_list_lock where 
   appropriate
 - Move rr_cpu_count() call to be conditional on icount_enabled()
 - Initialize cpu_budget to 0

Jamie Iles (2):
  cpu: expose qemu_cpu_list_lock for lock-guard use
  accel/tcg/tcg-accel-ops-rr: ensure fairness with icount

 accel/tcg/tcg-accel-ops-icount.c | 17 +--
 accel/tcg/tcg-accel-ops-icount.h |  3 ++-
 accel/tcg/tcg-accel-ops-rr.c | 37 +++-
 cpus-common.c|  2 +-
 include/exec/cpu-common.h|  1 +
 linux-user/elfload.c | 12 +--
 migration/dirtyrate.c| 26 +++---
 trace/control-target.c   |  9 
 8 files changed, 78 insertions(+), 29 deletions(-)

-- 
2.25.1

[PATCH v3 1/2] cpu: expose qemu_cpu_list_lock for lock-guard use

[Jamie Iles]

Expose qemu_cpu_list_lock globally so that we can use
WITH_QEMU_LOCK_GUARD and QEMU_LOCK_GUARD to simplify a few code paths
now and in future.

Signed-off-by: Jamie Iles 
---
 cpus-common.c |  2 +-
 include/exec/cpu-common.h |  1 +
 linux-user/elfload.c  | 12 ++--
 migration/dirtyrate.c | 26 +-
 trace/control-target.c|  9 -
 5 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/cpus-common.c b/cpus-common.c
index b0047e456f93..82d439add5c1 100644
--- a/cpus-common.c
+++ b/cpus-common.c
@@ -25,7 +25,7 @@
 #include "qemu/lockable.h"
 #include "trace/trace-root.h"
 
-static QemuMutex qemu_cpu_list_lock;
+QemuMutex qemu_cpu_list_lock;
 static QemuCond exclusive_cond;
 static QemuCond exclusive_resume;
 static QemuCond qemu_work_cond;
diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index 6feaa40ca7b0..0c833d6ac9c6 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -32,6 +32,7 @@ extern intptr_t qemu_host_page_mask;
 #define REAL_HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_real_host_page_size())
 
 /* The CPU list lock nests outside page_(un)lock or mmap_(un)lock */
+extern QemuMutex qemu_cpu_list_lock;
 void qemu_init_cpu_list(void);
 void cpu_list_lock(void);
 void cpu_list_unlock(void);
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 1dbc1f0f9baa..3ff16b163382 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -4238,14 +4238,14 @@ static int fill_note_info(struct elf_note_info *info,
 info->notes_size += note_size(>notes[i]);
 
 /* read and fill status of all threads */
-cpu_list_lock();
-CPU_FOREACH(cpu) {
-if (cpu == thread_cpu) {
-continue;
+WITH_QEMU_LOCK_GUARD(_cpu_list_lock) {
+CPU_FOREACH(cpu) {
+if (cpu == thread_cpu) {
+continue;
+}
+fill_thread_info(info, cpu->env_ptr);
 }
-fill_thread_info(info, cpu->env_ptr);
 }
-cpu_list_unlock();
 
 return (0);
 }
diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 180ba38c7a80..388337a33249 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -150,25 +150,25 @@ int64_t vcpu_calculate_dirtyrate(int64_t calc_time_ms,
 retry:
 init_time_ms = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
 
-cpu_list_lock();
-gen_id = cpu_list_generation_id_get();
-records = vcpu_dirty_stat_alloc(stat);
-vcpu_dirty_stat_collect(stat, records, true);
-cpu_list_unlock();
+WITH_QEMU_LOCK_GUARD(_cpu_list_lock) {
+gen_id = cpu_list_generation_id_get();
+records = vcpu_dirty_stat_alloc(stat);
+vcpu_dirty_stat_collect(stat, records, true);
+}
 
 duration = dirty_stat_wait(calc_time_ms, init_time_ms);
 
 global_dirty_log_sync(flag, one_shot);
 
-cpu_list_lock();
-if (gen_id != cpu_list_generation_id_get()) {
-g_free(records);
-g_free(stat->rates);
-cpu_list_unlock();
-goto retry;
+WITH_QEMU_LOCK_GUARD(_cpu_list_lock) {
+if (gen_id != cpu_list_generation_id_get()) {
+g_free(records);
+g_free(stat->rates);
+cpu_list_unlock();
+goto retry;
+}
+vcpu_dirty_stat_collect(stat, records, false);
 }
-vcpu_dirty_stat_collect(stat, records, false);
-cpu_list_unlock();
 
 for (i = 0; i < stat->nvcpu; i++) {
 dirtyrate = do_calculate_dirtyrate(records[i], duration);
diff --git a/trace/control-target.c b/trace/control-target.c
index 232c97a4a183..c0c1e2310a51 100644
--- a/trace/control-target.c
+++ b/trace/control-target.c
@@ -8,6 +8,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/lockable.h"
 #include "cpu.h"
 #include "trace/trace-root.h"
 #include "trace/control.h"
@@ -116,11 +117,9 @@ static bool adding_first_cpu1(void)
 
 static bool adding_first_cpu(void)
 {
-bool res;
-cpu_list_lock();
-res = adding_first_cpu1();
-cpu_list_unlock();
-return res;
+QEMU_LOCK_GUARD(_cpu_list_lock);
+
+return adding_first_cpu1();
 }
 
 void trace_init_vcpu(CPUState *vcpu)
-- 
2.25.1

Re: [PATCH v2] accel/tcg/tcg-accel-ops-rr: ensure fairness with icount

[Jamie Iles]

Hi Richard,

On Mon, Apr 24, 2023 at 02:06:18PM +0100, Richard Henderson wrote:
> WARNING: This email originated from outside of Qualcomm. Please be wary of 
> any links or attachments, and do not enable macros.
> 
> On 4/24/23 12:29, Jamie Iles wrote:
> > +/*
> > + * Calculate the number of CPUs that we will process in a single iteration 
> > of
> > + * the main CPU thread loop so that we can fairly distribute the 
> > instruction
> > + * count across CPUs.
> > + *
> > + * The CPU count is cached based on the CPU list generation ID to avoid
> > + * iterating the list every time.
> > + */
> > +static int rr_cpu_count(void)
> > +{
> > +static unsigned int last_gen_id = ~0;
> > +static int cpu_count;
> > +CPUState *cpu;
> > +
> > +cpu_list_lock();
> > +if (cpu_list_generation_id_get() != last_gen_id) {
> > +cpu_count = 0;
> > +CPU_FOREACH(cpu) {
> > +++cpu_count;
> > +}
> > +last_gen_id = cpu_list_generation_id_get();
> > +}
> > +cpu_list_unlock();
> > +
> > +return cpu_count;
> > +}
> 
> The read of cpu_count should be in the lock.
> 
> (Ideally we'd expose QEMU_LOCK_GUARD(_cpu_list_lock) which would make 
> the read+return
> trivial.)

Sure, can do that, I can add that as a first patch and update other 
users.

> 
> >   /*
> >* In the single-threaded case each vCPU is simulated in turn. If
> >* there is more than a single vCPU we create a simple timer to kick
> > @@ -185,6 +212,9 @@ static void *rr_cpu_thread_fn(void *arg)
> >   cpu->exit_request = 1;
> > 
> >   while (1) {
> > +int cpu_count = rr_cpu_count();
> > +int64_t cpu_budget = INT64_MAX;
> > +
> >   qemu_mutex_unlock_iothread();
> >   replay_mutex_lock();
> >   qemu_mutex_lock_iothread();
> > @@ -197,6 +227,8 @@ static void *rr_cpu_thread_fn(void *arg)
> >* waking up the I/O thread and waiting for completion.
> >*/
> >   icount_handle_deadline();
> > +
> > +cpu_budget = icount_percpu_budget(cpu_count);
> 
> I think you can move the call to rr_cpu_count() here, so that it only happens 
> if icount is
> in use.
> 
> Why cpu_budget = INT64_MAX as opposed to 0 or 0xdeadbeef?  It's not set or 
> used except for
> icount_enabled.

That's left over from an earlier version, I can leave it uninitialized.

Thanks,

Jamie

[PATCH v2] accel/tcg/tcg-accel-ops-rr: ensure fairness with icount

[Jamie Iles]

The round-robin scheduler will iterate over the CPU list with an
assigned budget until the next timer expiry and may exit early because
of a TB exit.  This is fine under normal operation but with icount
enabled and SMP it is possible for a CPU to be starved of run time and
the system live-locks.

For example, booting a riscv64 platform with '-icount
shift=0,align=off,sleep=on -smp 2' we observe a livelock once the kernel
has timers enabled and starts performing TLB shootdowns.  In this case
we have CPU 0 in M-mode with interrupts disabled sending an IPI to CPU
1.  As we enter the TCG loop, we assign the icount budget to next timer
interrupt to CPU 0 and begin executing where the guest is sat in a busy
loop exhausting all of the budget before we try to execute CPU 1 which
is the target of the IPI but CPU 1 is left with no budget with which to
execute and the process repeats.

We try here to add some fairness by splitting the budget across all of
the CPUs on the thread fairly before entering each one.  The CPU count
is cached on CPU list generation ID to avoid iterating the list on each
loop iteration.  With this change it is possible to boot an SMP rv64
guest with icount enabled and no hangs.

Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Peter Maydell 
Signed-off-by: Jamie Iles 
---

Changes in v2:
 - Rename icount_cpu_timeslice to icount_percpu_budget
 - Add a clarifying comment about caching to rr_cpu_count()

 accel/tcg/tcg-accel-ops-icount.c | 17 ++--
 accel/tcg/tcg-accel-ops-icount.h |  3 ++-
 accel/tcg/tcg-accel-ops-rr.c | 34 +++-
 3 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/accel/tcg/tcg-accel-ops-icount.c b/accel/tcg/tcg-accel-ops-icount.c
index 84cc7421be88..e1e8afaf2f99 100644
--- a/accel/tcg/tcg-accel-ops-icount.c
+++ b/accel/tcg/tcg-accel-ops-icount.c
@@ -89,7 +89,20 @@ void icount_handle_deadline(void)
 }
 }
 
-void icount_prepare_for_run(CPUState *cpu)
+/* Distribute the budget evenly across all CPUs */
+int64_t icount_percpu_budget(int cpu_count)
+{
+int64_t limit = icount_get_limit();
+int64_t timeslice = limit / cpu_count;
+
+if (timeslice == 0) {
+timeslice = limit;
+}
+
+return timeslice;
+}
+
+void icount_prepare_for_run(CPUState *cpu, int64_t cpu_budget)
 {
 int insns_left;
 
@@ -101,7 +114,7 @@ void icount_prepare_for_run(CPUState *cpu)
 g_assert(cpu_neg(cpu)->icount_decr.u16.low == 0);
 g_assert(cpu->icount_extra == 0);
 
-cpu->icount_budget = icount_get_limit();
+cpu->icount_budget = MIN(icount_get_limit(), cpu_budget);
 insns_left = MIN(0x, cpu->icount_budget);
 cpu_neg(cpu)->icount_decr.u16.low = insns_left;
 cpu->icount_extra = cpu->icount_budget - insns_left;
diff --git a/accel/tcg/tcg-accel-ops-icount.h b/accel/tcg/tcg-accel-ops-icount.h
index 1b6fd9c60751..16a301b6dc0b 100644
--- a/accel/tcg/tcg-accel-ops-icount.h
+++ b/accel/tcg/tcg-accel-ops-icount.h
@@ -11,7 +11,8 @@
 #define TCG_ACCEL_OPS_ICOUNT_H
 
 void icount_handle_deadline(void);
-void icount_prepare_for_run(CPUState *cpu);
+void icount_prepare_for_run(CPUState *cpu, int64_t cpu_budget);
+int64_t icount_percpu_budget(int cpu_count);
 void icount_process_data(CPUState *cpu);
 
 void icount_handle_interrupt(CPUState *cpu, int mask);
diff --git a/accel/tcg/tcg-accel-ops-rr.c b/accel/tcg/tcg-accel-ops-rr.c
index 290833a37fb2..7114210173df 100644
--- a/accel/tcg/tcg-accel-ops-rr.c
+++ b/accel/tcg/tcg-accel-ops-rr.c
@@ -139,6 +139,33 @@ static void rr_force_rcu(Notifier *notify, void *data)
 rr_kick_next_cpu();
 }
 
+/*
+ * Calculate the number of CPUs that we will process in a single iteration of
+ * the main CPU thread loop so that we can fairly distribute the instruction
+ * count across CPUs.
+ *
+ * The CPU count is cached based on the CPU list generation ID to avoid
+ * iterating the list every time.
+ */
+static int rr_cpu_count(void)
+{
+static unsigned int last_gen_id = ~0;
+static int cpu_count;
+CPUState *cpu;
+
+cpu_list_lock();
+if (cpu_list_generation_id_get() != last_gen_id) {
+cpu_count = 0;
+CPU_FOREACH(cpu) {
+++cpu_count;
+}
+last_gen_id = cpu_list_generation_id_get();
+}
+cpu_list_unlock();
+
+return cpu_count;
+}
+
 /*
  * In the single-threaded case each vCPU is simulated in turn. If
  * there is more than a single vCPU we create a simple timer to kick
@@ -185,6 +212,9 @@ static void *rr_cpu_thread_fn(void *arg)
 cpu->exit_request = 1;
 
 while (1) {
+int cpu_count = rr_cpu_count();
+int64_t cpu_budget = INT64_MAX;
+
 qemu_mutex_unlock_iothread();
 replay_mutex_lock();
 qemu_mutex_lock_iothread();
@@ -197,6 +227,8 @@ static void *rr_cpu_thread_fn(void *arg)
  * waking up the I/O thread and waiting for completion.
  */
 icount_handle_deadline();
+
+cpu_budget = ico

[PATCH] accel/tcg/tcg-accel-ops-rr: ensure fairness with icount

[Jamie Iles]

The round-robin scheduler will iterate over the CPU list with an
assigned budget until the next timer expiry and may exit early because
of a TB exit.  This is fine under normal operation but with icount
enabled and SMP it is possible for a CPU to be starved of run time and
the system live-locks.

For example, booting a riscv64 platform with '-icount
shift=0,align=off,sleep=on -smp 2' we observe a livelock once the kernel
has timers enabled and starts performing TLB shootdowns.  In this case
we have CPU 0 in M-mode with interrupts disabled sending an IPI to CPU
1.  As we enter the TCG loop, we assign the icount budget to next timer
interrupt to CPU 0 and begin executing where the guest is sat in a busy
loop exhausting all of the budget before we try to execute CPU 1 which
is the target of the IPI but CPU 1 is left with no budget with which to
execute and the process repeats.

We try here to add some fairness by splitting the budget across all of
the CPUs on the thread fairly before entering each one.  The CPU count
is cached on CPU list generation ID to avoid iterating the list on each
loop iteration.  With this change it is possible to boot an SMP rv64
guest with icount enabled and no hangs.

Signed-off-by: Jamie Iles 
---
 accel/tcg/tcg-accel-ops-icount.c | 16 ++--
 accel/tcg/tcg-accel-ops-icount.h |  3 ++-
 accel/tcg/tcg-accel-ops-rr.c | 26 +-
 3 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/accel/tcg/tcg-accel-ops-icount.c b/accel/tcg/tcg-accel-ops-icount.c
index 84cc7421be88..a7ffc8a68bad 100644
--- a/accel/tcg/tcg-accel-ops-icount.c
+++ b/accel/tcg/tcg-accel-ops-icount.c
@@ -89,7 +89,19 @@ void icount_handle_deadline(void)
 }
 }
 
-void icount_prepare_for_run(CPUState *cpu)
+int64_t icount_cpu_timeslice(int cpu_count)
+{
+int64_t limit = icount_get_limit();
+int64_t timeslice = limit / cpu_count;
+
+if (timeslice == 0) {
+timeslice = limit;
+}
+
+return timeslice;
+}
+
+void icount_prepare_for_run(CPUState *cpu, int64_t timeslice)
 {
 int insns_left;
 
@@ -101,7 +113,7 @@ void icount_prepare_for_run(CPUState *cpu)
 g_assert(cpu_neg(cpu)->icount_decr.u16.low == 0);
 g_assert(cpu->icount_extra == 0);
 
-cpu->icount_budget = icount_get_limit();
+cpu->icount_budget = MIN(icount_get_limit(), timeslice);
 insns_left = MIN(0x, cpu->icount_budget);
 cpu_neg(cpu)->icount_decr.u16.low = insns_left;
 cpu->icount_extra = cpu->icount_budget - insns_left;
diff --git a/accel/tcg/tcg-accel-ops-icount.h b/accel/tcg/tcg-accel-ops-icount.h
index 1b6fd9c60751..e8785a0e196d 100644
--- a/accel/tcg/tcg-accel-ops-icount.h
+++ b/accel/tcg/tcg-accel-ops-icount.h
@@ -11,7 +11,8 @@
 #define TCG_ACCEL_OPS_ICOUNT_H
 
 void icount_handle_deadline(void);
-void icount_prepare_for_run(CPUState *cpu);
+void icount_prepare_for_run(CPUState *cpu, int64_t timeslice);
+int64_t icount_cpu_timeslice(int cpu_count);
 void icount_process_data(CPUState *cpu);
 
 void icount_handle_interrupt(CPUState *cpu, int mask);
diff --git a/accel/tcg/tcg-accel-ops-rr.c b/accel/tcg/tcg-accel-ops-rr.c
index 290833a37fb2..bccb3670a656 100644
--- a/accel/tcg/tcg-accel-ops-rr.c
+++ b/accel/tcg/tcg-accel-ops-rr.c
@@ -139,6 +139,25 @@ static void rr_force_rcu(Notifier *notify, void *data)
 rr_kick_next_cpu();
 }
 
+static int rr_cpu_count(void)
+{
+static unsigned int last_gen_id = ~0;
+static int cpu_count;
+CPUState *cpu;
+
+cpu_list_lock();
+if (cpu_list_generation_id_get() != last_gen_id) {
+cpu_count = 0;
+CPU_FOREACH(cpu) {
+++cpu_count;
+}
+last_gen_id = cpu_list_generation_id_get();
+}
+cpu_list_unlock();
+
+return cpu_count;
+}
+
 /*
  * In the single-threaded case each vCPU is simulated in turn. If
  * there is more than a single vCPU we create a simple timer to kick
@@ -185,6 +204,9 @@ static void *rr_cpu_thread_fn(void *arg)
 cpu->exit_request = 1;
 
 while (1) {
+int cpu_count = rr_cpu_count();
+int64_t icount_timeslice = INT64_MAX;
+
 qemu_mutex_unlock_iothread();
 replay_mutex_lock();
 qemu_mutex_lock_iothread();
@@ -197,6 +219,8 @@ static void *rr_cpu_thread_fn(void *arg)
  * waking up the I/O thread and waiting for completion.
  */
 icount_handle_deadline();
+
+icount_timeslice = icount_cpu_timeslice(cpu_count);
 }
 
 replay_mutex_unlock();
@@ -218,7 +242,7 @@ static void *rr_cpu_thread_fn(void *arg)
 
 qemu_mutex_unlock_iothread();
 if (icount_enabled()) {
-icount_prepare_for_run(cpu);
+icount_prepare_for_run(cpu, icount_timeslice);
 }
 r = tcg_cpus_exec(cpu);
 if (icount_enabled()) {
-- 
2.25.1

Re: [PATCH 2/2] hw/core/loader: workaround read() size limit.

[Jamie Iles]

Hi Philippe,

On Thu, Nov 11, 2021 at 05:04:56PM +, Jamie Iles wrote:
> On Thu, Nov 11, 2021 at 04:55:35PM +0100, Philippe Mathieu-Daudé wrote:
> > On 11/11/21 16:43, Philippe Mathieu-Daudé wrote:
> > > On 11/11/21 16:36, Jamie Iles wrote:
> > >> Hi Philippe,
> > >>
> > >> On Thu, Nov 11, 2021 at 03:55:48PM +0100, Philippe Mathieu-Daudé wrote:
> > >>> Hi Jamie,
> > >>>
> > >>> On 11/11/21 15:11, Jamie Iles wrote:
> > >>>> On Linux, read() will only ever read a maximum of 0x7000 bytes
> > >>>> regardless of what is asked.  If the file is larger than 0x7ffff000
> > >>>> bytes the read will need to be broken up into multiple chunks.
> > >>>>
> > >>>> Cc: Luc Michel 
> > >>>> Signed-off-by: Jamie Iles 
> > >>>> ---
> > >>>>  hw/core/loader.c | 40 ++--
> > >>>>  1 file changed, 34 insertions(+), 6 deletions(-)
> > >>>>
> > >>>> diff --git a/hw/core/loader.c b/hw/core/loader.c
> > >>>> index 348bbf535bd9..16ca9b99cf0f 100644
> > >>>> --- a/hw/core/loader.c
> > >>>> +++ b/hw/core/loader.c
> > >>>> @@ -80,6 +80,34 @@ int64_t get_image_size(const char *filename)
> > >>>>  return size;
> > >>>>  }
> > >>>>  
> > >>>> +static ssize_t read_large(int fd, void *dst, size_t len)
> > >>>> +{
> > >>>> +/*
> > >>>> + * man 2 read says:
> > >>>> + *
> > >>>> + * On Linux, read() (and similar system calls) will transfer at 
> > >>>> most
> > >>>> + * 0x7000 (2,147,479,552) bytes, returning the number of bytes
> > >>>
> > >>> Could you mention MAX_RW_COUNT from linux/fs.h?
> > >>>
> > >>>> + * actually transferred.  (This is true on both 32-bit and 64-bit
> > >>>> + * systems.)
> > >>>
> > >>> Maybe "This is true for both ILP32 and LP64 data models used by Linux"?
> > >>> (because that would not be the case for the ILP64 model).
> > >>>
> > >>> Otherwise s/systems/Linux variants/?
> > >>>
> > >>>> + *
> > >>>> + * So read in chunks no larger than 0x7000 bytes.
> > >>>> + */
> > >>>> +size_t max_chunk_size = 0x7000;
> > >>>
> > >>> We can declare it static const.
> > >>
> > >> Ack, can fix all of those up.
> > >>
> > >>>> +size_t offset = 0;
> > >>>> +
> > >>>> +while (offset < len) {
> > >>>> +size_t chunk_len = MIN(max_chunk_size, len - offset);
> > >>>> +ssize_t br = read(fd, dst + offset, chunk_len);
> > >>>> +
> > >>>> +if (br < 0) {
> > >>>> +return br;
> > >>>> +}
> > >>>> +offset += br;
> > >>>> +}
> > >>>> +
> > >>>> +return (ssize_t)len;
> > >>>> +}
> > >>>
> > >>> I see other read()/pread() calls:
> > >>>
> > >>> hw/9pfs/9p-local.c:472:tsize = read(fd, (void *)buf, bufsz);
> > >>> hw/vfio/common.c:269:if (pread(vbasedev->fd, , size,
> > >>> region->fd_offset + addr) != size) {
> > >>> ...
> > >>>
> > >>> Maybe the read_large() belongs to "sysemu/os-xxx.h"?
> > >>
> > >> I think util/osdep.c would be a good fit for this.  To make sure we're 
> > > 
> > > Yes.
> > > 
> > >> on the same page though are you proposing converting all pread/read 
> > >> calls to a qemu variant or auditing for ones that could potentially take 
> > >> a larger size?
> > > 
> > > Yes, I took some time wondering beside loading blob in guest memory,
> > > what would be the other issues you might encounter. I couldn't find
> > > many cases. Eventually hw/vfio/. I haven't audit much, only noticed
> > > hw/9pfs/9p-local.c and qga/commands-*.c (not sure if relevant), but
> > > since we want to fix this, I'd rather try to fix it globally.
> > 
> > Actually what you suggest is simpler, add qemu_read() / qemu_pread()
> > in util/osdep.c, convert all uses without caring about any audit.
> 
> Okay, this hasn't worked out too badly - I'll do the same for 
> write/pwrite too and then switch all of the callers over with a 
> coccinelle patch so it'll be a fairly large diff but simple.
> 
> We could elect to keep any calls with a compile-time constant length 
> with the unwrapped variants but I think that's probably more confusing 
> in the long-run.

Coming back to this I think this is probably a non-starter because of 
non-blocking file descriptors.  There is already a qemu_write_full so 
I'm inclined to add qemu_read_full following the same pattern and then 
convert all of the read calls in the loader to use that.

Thanks,

Jamie

Re: [PATCH 2/2] hw/core/loader: workaround read() size limit.

[Jamie Iles]

On Thu, Nov 11, 2021 at 04:55:35PM +0100, Philippe Mathieu-Daudé wrote:
> On 11/11/21 16:43, Philippe Mathieu-Daudé wrote:
> > On 11/11/21 16:36, Jamie Iles wrote:
> >> Hi Philippe,
> >>
> >> On Thu, Nov 11, 2021 at 03:55:48PM +0100, Philippe Mathieu-Daudé wrote:
> >>> Hi Jamie,
> >>>
> >>> On 11/11/21 15:11, Jamie Iles wrote:
> >>>> On Linux, read() will only ever read a maximum of 0x7000 bytes
> >>>> regardless of what is asked.  If the file is larger than 0x7000
> >>>> bytes the read will need to be broken up into multiple chunks.
> >>>>
> >>>> Cc: Luc Michel 
> >>>> Signed-off-by: Jamie Iles 
> >>>> ---
> >>>>  hw/core/loader.c | 40 ++--
> >>>>  1 file changed, 34 insertions(+), 6 deletions(-)
> >>>>
> >>>> diff --git a/hw/core/loader.c b/hw/core/loader.c
> >>>> index 348bbf535bd9..16ca9b99cf0f 100644
> >>>> --- a/hw/core/loader.c
> >>>> +++ b/hw/core/loader.c
> >>>> @@ -80,6 +80,34 @@ int64_t get_image_size(const char *filename)
> >>>>  return size;
> >>>>  }
> >>>>  
> >>>> +static ssize_t read_large(int fd, void *dst, size_t len)
> >>>> +{
> >>>> +/*
> >>>> + * man 2 read says:
> >>>> + *
> >>>> + * On Linux, read() (and similar system calls) will transfer at most
> >>>> + * 0x7000 (2,147,479,552) bytes, returning the number of bytes
> >>>
> >>> Could you mention MAX_RW_COUNT from linux/fs.h?
> >>>
> >>>> + * actually transferred.  (This is true on both 32-bit and 64-bit
> >>>> + * systems.)
> >>>
> >>> Maybe "This is true for both ILP32 and LP64 data models used by Linux"?
> >>> (because that would not be the case for the ILP64 model).
> >>>
> >>> Otherwise s/systems/Linux variants/?
> >>>
> >>>> + *
> >>>> + * So read in chunks no larger than 0x7000 bytes.
> >>>> + */
> >>>> +size_t max_chunk_size = 0x7000;
> >>>
> >>> We can declare it static const.
> >>
> >> Ack, can fix all of those up.
> >>
> >>>> +size_t offset = 0;
> >>>> +
> >>>> +while (offset < len) {
> >>>> +size_t chunk_len = MIN(max_chunk_size, len - offset);
> >>>> +ssize_t br = read(fd, dst + offset, chunk_len);
> >>>> +
> >>>> +if (br < 0) {
> >>>> +return br;
> >>>> +}
> >>>> +offset += br;
> >>>> +}
> >>>> +
> >>>> +return (ssize_t)len;
> >>>> +}
> >>>
> >>> I see other read()/pread() calls:
> >>>
> >>> hw/9pfs/9p-local.c:472:tsize = read(fd, (void *)buf, bufsz);
> >>> hw/vfio/common.c:269:if (pread(vbasedev->fd, , size,
> >>> region->fd_offset + addr) != size) {
> >>> ...
> >>>
> >>> Maybe the read_large() belongs to "sysemu/os-xxx.h"?
> >>
> >> I think util/osdep.c would be a good fit for this.  To make sure we're 
> > 
> > Yes.
> > 
> >> on the same page though are you proposing converting all pread/read 
> >> calls to a qemu variant or auditing for ones that could potentially take 
> >> a larger size?
> > 
> > Yes, I took some time wondering beside loading blob in guest memory,
> > what would be the other issues you might encounter. I couldn't find
> > many cases. Eventually hw/vfio/. I haven't audit much, only noticed
> > hw/9pfs/9p-local.c and qga/commands-*.c (not sure if relevant), but
> > since we want to fix this, I'd rather try to fix it globally.
> 
> Actually what you suggest is simpler, add qemu_read() / qemu_pread()
> in util/osdep.c, convert all uses without caring about any audit.

Okay, this hasn't worked out too badly - I'll do the same for 
write/pwrite too and then switch all of the callers over with a 
coccinelle patch so it'll be a fairly large diff but simple.

We could elect to keep any calls with a compile-time constant length 
with the unwrapped variants but I think that's probably more confusing 
in the long-run.

Thanks,

Jamie

Re: [PATCH 2/2] hw/core/loader: workaround read() size limit.

[Jamie Iles]

Hi Philippe,

On Thu, Nov 11, 2021 at 03:55:48PM +0100, Philippe Mathieu-Daudé wrote:
> Hi Jamie,
> 
> On 11/11/21 15:11, Jamie Iles wrote:
> > On Linux, read() will only ever read a maximum of 0x7000 bytes
> > regardless of what is asked.  If the file is larger than 0x7000
> > bytes the read will need to be broken up into multiple chunks.
> > 
> > Cc: Luc Michel 
> > Signed-off-by: Jamie Iles 
> > ---
> >  hw/core/loader.c | 40 ++--
> >  1 file changed, 34 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/core/loader.c b/hw/core/loader.c
> > index 348bbf535bd9..16ca9b99cf0f 100644
> > --- a/hw/core/loader.c
> > +++ b/hw/core/loader.c
> > @@ -80,6 +80,34 @@ int64_t get_image_size(const char *filename)
> >  return size;
> >  }
> >  
> > +static ssize_t read_large(int fd, void *dst, size_t len)
> > +{
> > +/*
> > + * man 2 read says:
> > + *
> > + * On Linux, read() (and similar system calls) will transfer at most
> > + * 0x7000 (2,147,479,552) bytes, returning the number of bytes
> 
> Could you mention MAX_RW_COUNT from linux/fs.h?
> 
> > + * actually transferred.  (This is true on both 32-bit and 64-bit
> > + * systems.)
> 
> Maybe "This is true for both ILP32 and LP64 data models used by Linux"?
> (because that would not be the case for the ILP64 model).
> 
> Otherwise s/systems/Linux variants/?
> 
> > + *
> > + * So read in chunks no larger than 0x7000 bytes.
> > + */
> > +size_t max_chunk_size = 0x7000;
> 
> We can declare it static const.

Ack, can fix all of those up.

> > +size_t offset = 0;
> > +
> > +while (offset < len) {
> > +size_t chunk_len = MIN(max_chunk_size, len - offset);
> > +ssize_t br = read(fd, dst + offset, chunk_len);
> > +
> > +if (br < 0) {
> > +return br;
> > +}
> > +offset += br;
> > +}
> > +
> > +return (ssize_t)len;
> > +}
> 
> I see other read()/pread() calls:
> 
> hw/9pfs/9p-local.c:472:tsize = read(fd, (void *)buf, bufsz);
> hw/vfio/common.c:269:if (pread(vbasedev->fd, , size,
> region->fd_offset + addr) != size) {
> ...
> 
> Maybe the read_large() belongs to "sysemu/os-xxx.h"?

I think util/osdep.c would be a good fit for this.  To make sure we're 
on the same page though are you proposing converting all pread/read 
calls to a qemu variant or auditing for ones that could potentially take 
a larger size?

Thanks,

Jamie

[PATCH 2/2] hw/core/loader: workaround read() size limit.

[Jamie Iles]

On Linux, read() will only ever read a maximum of 0x7000 bytes
regardless of what is asked.  If the file is larger than 0x7000
bytes the read will need to be broken up into multiple chunks.

Cc: Luc Michel 
Signed-off-by: Jamie Iles 
---
 hw/core/loader.c | 40 ++--
 1 file changed, 34 insertions(+), 6 deletions(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 348bbf535bd9..16ca9b99cf0f 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -80,6 +80,34 @@ int64_t get_image_size(const char *filename)
 return size;
 }
 
+static ssize_t read_large(int fd, void *dst, size_t len)
+{
+/*
+ * man 2 read says:
+ *
+ * On Linux, read() (and similar system calls) will transfer at most
+ * 0x7000 (2,147,479,552) bytes, returning the number of bytes
+ * actually transferred.  (This is true on both 32-bit and 64-bit
+ * systems.)
+ *
+ * So read in chunks no larger than 0x7000 bytes.
+ */
+size_t max_chunk_size = 0x7000;
+size_t offset = 0;
+
+while (offset < len) {
+size_t chunk_len = MIN(max_chunk_size, len - offset);
+ssize_t br = read(fd, dst + offset, chunk_len);
+
+if (br < 0) {
+return br;
+}
+offset += br;
+}
+
+return (ssize_t)len;
+}
+
 /* return the size or -1 if error */
 ssize_t load_image_size(const char *filename, void *addr, size_t size)
 {
@@ -91,7 +119,7 @@ ssize_t load_image_size(const char *filename, void *addr, 
size_t size)
 return -1;
 }
 
-while ((actsize = read(fd, addr + l, size - l)) > 0) {
+while ((actsize = read_large(fd, addr + l, size - l)) > 0) {
 l += actsize;
 }
 
@@ -108,7 +136,7 @@ ssize_t read_targphys(const char *name,
 ssize_t did;
 
 buf = g_malloc(nbytes);
-did = read(fd, buf, nbytes);
+did = read_large(fd, buf, nbytes);
 if (did > 0)
 rom_add_blob_fixed("read", buf, did, dst_addr);
 g_free(buf);
@@ -235,7 +263,7 @@ ssize_t load_aout(const char *filename, hwaddr addr, int 
max_sz,
 if (fd < 0)
 return -1;
 
-size = read(fd, , sizeof(e));
+size = read_large(fd, , sizeof(e));
 if (size < 0)
 goto fail;
 
@@ -286,7 +314,7 @@ static void *load_at(int fd, off_t offset, size_t size)
 if (lseek(fd, offset, SEEK_SET) < 0)
 return NULL;
 ptr = g_malloc(size);
-if (read(fd, ptr, size) != size) {
+if (read_large(fd, ptr, size) != size) {
 g_free(ptr);
 return NULL;
 }
@@ -714,7 +742,7 @@ static ssize_t load_uboot_image(const char *filename, 
hwaddr *ep,
 
 data = g_malloc(hdr->ih_size);
 
-if (read(fd, data, hdr->ih_size) != hdr->ih_size) {
+if (read_large(fd, data, hdr->ih_size) != hdr->ih_size) {
 fprintf(stderr, "Error reading file\n");
 goto out;
 }
@@ -1005,7 +1033,7 @@ ssize_t rom_add_file(const char *file, const char *fw_dir,
 rom->datasize = rom->romsize;
 rom->data = g_malloc0(rom->datasize);
 lseek(fd, 0, SEEK_SET);
-rc = read(fd, rom->data, rom->datasize);
+rc = read_large(fd, rom->data, rom->datasize);
 if (rc != rom->datasize) {
 fprintf(stderr, "rom: file %-20s: read error: rc=%zd (expected %zd)\n",
 rom->name, rc, rom->datasize);
-- 
2.30.2

[PATCH 1/2] hw/core/loader: return image sizes as ssize_t

[Jamie Iles]

Various loader functions return an int which limits images to 2GB which
is fine for things like a BIOS/kernel image, but if we want to be able
to load memory images or large ramdisks then any file over 2GB would
silently fail to load.

Cc: Luc Michel 
Signed-off-by: Jamie Iles 
---
 hw/arm/armv7m.c  |  2 +-
 hw/arm/boot.c|  8 ++--
 hw/core/generic-loader.c |  2 +-
 hw/core/loader.c | 81 +---
 hw/i386/x86.c|  2 +-
 hw/riscv/boot.c  |  5 ++-
 include/hw/loader.h  | 55 +--
 7 files changed, 80 insertions(+), 75 deletions(-)

diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index 8d08db80be83..a6393dce7276 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -552,7 +552,7 @@ static void armv7m_reset(void *opaque)
 
 void armv7m_load_kernel(ARMCPU *cpu, const char *kernel_filename, int mem_size)
 {
-int image_size;
+ssize_t image_size;
 uint64_t entry;
 int big_endian;
 AddressSpace *as;
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 74ad397b1ff9..3853203438ba 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -876,7 +876,7 @@ static int do_arm_linux_init(Object *obj, void *opaque)
 return 0;
 }
 
-static int64_t arm_load_elf(struct arm_boot_info *info, uint64_t *pentry,
+static ssize_t arm_load_elf(struct arm_boot_info *info, uint64_t *pentry,
 uint64_t *lowaddr, uint64_t *highaddr,
 int elf_machine, AddressSpace *as)
 {
@@ -887,7 +887,7 @@ static int64_t arm_load_elf(struct arm_boot_info *info, 
uint64_t *pentry,
 } elf_header;
 int data_swab = 0;
 bool big_endian;
-int64_t ret = -1;
+ssize_t ret = -1;
 Error *err = NULL;
 
 
@@ -1009,7 +1009,7 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
 /* Set up for a direct boot of a kernel image file. */
 CPUState *cs;
 AddressSpace *as = arm_boot_address_space(cpu, info);
-int kernel_size;
+ssize_t kernel_size;
 int initrd_size;
 int is_linux = 0;
 uint64_t elf_entry;
@@ -1098,7 +1098,7 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
 
 if (kernel_size > info->ram_size) {
 error_report("kernel '%s' is too large to fit in RAM "
- "(kernel size %d, RAM size %" PRId64 ")",
+ "(kernel size %zd, RAM size %" PRId64 ")",
  info->kernel_filename, kernel_size, info->ram_size);
 exit(1);
 }
diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
index d14f932eea2e..bc1451da8f55 100644
--- a/hw/core/generic-loader.c
+++ b/hw/core/generic-loader.c
@@ -66,7 +66,7 @@ static void generic_loader_realize(DeviceState *dev, Error 
**errp)
 GenericLoaderState *s = GENERIC_LOADER(dev);
 hwaddr entry;
 int big_endian;
-int size = 0;
+ssize_t size = 0;
 
 s->set_pc = false;
 
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 052a0fd7198b..348bbf535bd9 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -115,17 +115,17 @@ ssize_t read_targphys(const char *name,
 return did;
 }
 
-int load_image_targphys(const char *filename,
-hwaddr addr, uint64_t max_sz)
+ssize_t load_image_targphys(const char *filename,
+hwaddr addr, uint64_t max_sz)
 {
 return load_image_targphys_as(filename, addr, max_sz, NULL);
 }
 
 /* return the size or -1 if error */
-int load_image_targphys_as(const char *filename,
-   hwaddr addr, uint64_t max_sz, AddressSpace *as)
+ssize_t load_image_targphys_as(const char *filename,
+   hwaddr addr, uint64_t max_sz, AddressSpace *as)
 {
-int size;
+ssize_t size;
 
 size = get_image_size(filename);
 if (size < 0 || size > max_sz) {
@@ -139,9 +139,9 @@ int load_image_targphys_as(const char *filename,
 return size;
 }
 
-int load_image_mr(const char *filename, MemoryRegion *mr)
+ssize_t load_image_mr(const char *filename, MemoryRegion *mr)
 {
-int size;
+ssize_t size;
 
 if (!memory_access_is_direct(mr, false)) {
 /* Can only load an image into RAM or ROM */
@@ -223,8 +223,8 @@ static void bswap_ahdr(struct exec *e)
  : (_N_SEGMENT_ROUND (_N_TXTENDADDR(x, target_page_size), 
target_page_size)))
 
 
-int load_aout(const char *filename, hwaddr addr, int max_sz,
-  int bswap_needed, hwaddr target_page_size)
+ssize_t load_aout(const char *filename, hwaddr addr, int max_sz,
+  int bswap_needed, hwaddr target_page_size)
 {
 int fd;
 ssize_t size, ret;
@@ -618,13 +618,14 @@ toosmall:
 }
 
 /* Load a U-Boot image.  */
-static int load_uboot_image(const char *filename, hwaddr *ep, hwaddr *loadaddr,
-int *is_linux, uint8_t image_type,
-uint64_t (*translate_fn)(void *, uint64_t),
-  

[PATCH 0/2] Fix integer overflows in loading of large images

[Jamie Iles]

Most of the loader code currently uses a ssize_t or 64 bit integer type  
to store image lengths, but many functions that handle loading return an 
int with a negative value on error or length on success.  Once an image 
exceeds 2GB this will cause an integer overflow and so can end up 
loading truncated images, silently failing to load an image (a 4GB image 
would be interpreted as 0 bytes long).

This is unlikely to affect many deployments, but can manifest when 
preloading RAM disks for example.

This builds upon 8975eb891fb6 ("hw/elf_ops.h: switch to ssize_t for elf 
loader return type") to cover more of the generic loader.

Jamie Iles (2):
  hw/core/loader: return image sizes as ssize_t
  hw/core/loader: workaround read() size limit.

 hw/arm/armv7m.c  |   2 +-
 hw/arm/boot.c|   8 +--
 hw/core/generic-loader.c |   2 +-
 hw/core/loader.c | 121 ---
 hw/i386/x86.c|   2 +-
 hw/riscv/boot.c  |   5 +-
 include/hw/loader.h  |  55 +-
 7 files changed, 114 insertions(+), 81 deletions(-)

-- 
2.30.2

[PATCHv2 4/4] target/arm: use raise_exception_ra for stack limit exception

[Jamie Iles]

Now that raise_exception_ra restores the state before raising the
exception we can use restore_exception_ra to perform the state restore +
exception raising without clobbering the PC/condbits.

Cc: Richard Henderson 
Cc: Peter Maydell 
Signed-off-by: Jamie Iles 
---
 target/arm/m_helper.c  |  5 +
 target/arm/op_helper.c | 10 +-
 2 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
index d63ae465e1e7..c793dc486f0d 100644
--- a/target/arm/m_helper.c
+++ b/target/arm/m_helper.c
@@ -2600,10 +2600,7 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, 
uint32_t val)
 limit = is_psp ? env->v7m.psplim[false] : env->v7m.msplim[false];
 
 if (val < limit) {
-CPUState *cs = env_cpu(env);
-
-cpu_restore_state(cs, GETPC(), true);
-raise_exception(env, EXCP_STKOF, 0, 1);
+raise_exception_ra(env, EXCP_STKOF, 0, 1, GETPC());
 }
 
 if (is_psp) {
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 3b3daa955598..23365b33feac 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -90,15 +90,7 @@ void HELPER(v8m_stackcheck)(CPUARMState *env, uint32_t 
newvalue)
  * raising an exception if the limit is breached.
  */
 if (newvalue < v7m_sp_limit(env)) {
-CPUState *cs = env_cpu(env);
-
-/*
- * Stack limit exceptions are a rare case, so rather than syncing
- * PC/condbits before the call, we use cpu_restore_state() to
- * get them right before raising the exception.
- */
-cpu_restore_state(cs, GETPC(), true);
-raise_exception(env, EXCP_STKOF, 0, 1);
+raise_exception_ra(env, EXCP_STKOF, 0, 1, GETPC());
 }
 }
 
-- 
2.30.2

[PATCHv2 0/4] target/arm: fix missing exception class

[Jamie Iles]

Thanks Peter for the suggestions, I also added a patch to switch a 
couple of cpu_restore_state+raise_exception pairs in stack limit 
exception handling for both v7m and v8m.

v2:
 - fix raise_exception_ra to restore state before raising exception
 - remove redundant do_raise_exception
 - remove now redundant open coded raise_exception_ra from MTE and stack 
   limit exception handling

Jamie Iles (4):
  target/arm: fix missing exception class
  target/arm: fold do_raise_exception into raise_exception
  target/arm: use raise_exception_ra for MTE check failure
  target/arm: use raise_exception_ra for stack limit exception

 target/arm/m_helper.c   |  5 +
 target/arm/mte_helper.c | 11 ++-
 target/arm/op_helper.c  | 28 +++-
 3 files changed, 10 insertions(+), 34 deletions(-)

-- 
2.30.2

[PATCHv2 3/4] target/arm: use raise_exception_ra for MTE check failure

[Jamie Iles]

Now that raise_exception_ra restores the state before raising the
exception we can use restore_exception_ra to perform the state restore +
exception raising without clobbering the syndrome.

Cc: Richard Henderson 
Cc: Peter Maydell 
Signed-off-by: Jamie Iles 
---
 target/arm/mte_helper.c | 11 ++-
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/target/arm/mte_helper.c b/target/arm/mte_helper.c
index a6fccc6e69e2..d48419583747 100644
--- a/target/arm/mte_helper.c
+++ b/target/arm/mte_helper.c
@@ -563,20 +563,13 @@ static void mte_check_fail(CPUARMState *env, uint32_t 
desc,
 
 switch (tcf) {
 case 1:
-/*
- * Tag check fail causes a synchronous exception.
- *
- * In restore_state_to_opc, we set the exception syndrome
- * for the load or store operation.  Unwind first so we
- * may overwrite that with the syndrome for the tag check.
- */
-cpu_restore_state(env_cpu(env), ra, true);
 env->exception.vaddress = dirty_ptr;
 
 is_write = FIELD_EX32(desc, MTEDESC, WRITE);
 syn = syn_data_abort_no_iss(arm_current_el(env) != 0, 0, 0, 0, 0,
 is_write, 0x11);
-raise_exception(env, EXCP_DATA_ABORT, syn, exception_target_el(env));
+raise_exception_ra(env, EXCP_DATA_ABORT, syn,
+   exception_target_el(env), ra);
 /* noreturn, but fall through to the assert anyway */
 
 case 0:
-- 
2.30.2

[PATCHv2 1/4] target/arm: fix missing exception class

[Jamie Iles]

The DAIF and PAC checks used raise_exception_ra to raise an exception
and unwind CPU state but raise_exception_ra is currently designed for
handling data aborts as the syndrome is partially precomputed and
encoded in the TB and then merged in merge_syn_data_abort when handling
the data abort.  Using raise_exception_ra for DAIF and PAC checks
results in an empty syndrome being retrieved from data[2] in
restore_state_to_opc and setting ESR to 0.  This manifested as:

  kvm [571]: Unknown exception class: esr: 0x00 –
  Unknown/Uncategorized

when launching a KVM guest when the host qemu used a CPU supporting
EL2+pointer authentication and enabling pointer authentication in the
guest.

Rework raise_exception_ra such that the state is restored before raising
the exception so that the exception is not clobbered by
restore_state_to_opc.

Fixes: 0d43e1a2d29a ("target/arm: Add PAuth helpers")
Cc: Richard Henderson 
Cc: Peter Maydell 
Signed-off-by: Jamie Iles 
---
 target/arm/op_helper.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index efcb60099277..078ed74ab962 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -63,8 +63,10 @@ void raise_exception(CPUARMState *env, uint32_t excp,
 void raise_exception_ra(CPUARMState *env, uint32_t excp, uint32_t syndrome,
 uint32_t target_el, uintptr_t ra)
 {
-CPUState *cs = do_raise_exception(env, excp, syndrome, target_el);
-cpu_loop_exit_restore(cs, ra);
+CPUState *cs = env_cpu(env);
+
+cpu_restore_state(cs, ra, true);
+raise_exception(env, excp, syndrome, target_el);
 }
 
 uint64_t HELPER(neon_tbl)(CPUARMState *env, uint32_t desc,
-- 
2.30.2

[PATCHv2 2/4] target/arm: fold do_raise_exception into raise_exception

[Jamie Iles]

Now that there are no other users of do_raise_exception, fold it into
raise_exception.

Cc: Richard Henderson 
Cc: Peter Maydell 
Signed-off-by: Jamie Iles 
---
 target/arm/op_helper.c | 12 ++--
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 078ed74ab962..3b3daa955598 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -27,8 +27,8 @@
 #define SIGNBIT (uint32_t)0x8000
 #define SIGNBIT64 ((uint64_t)1 << 63)
 
-static CPUState *do_raise_exception(CPUARMState *env, uint32_t excp,
-uint32_t syndrome, uint32_t target_el)
+void raise_exception(CPUARMState *env, uint32_t excp,
+ uint32_t syndrome, uint32_t target_el)
 {
 CPUState *cs = env_cpu(env);
 
@@ -49,14 +49,6 @@ static CPUState *do_raise_exception(CPUARMState *env, 
uint32_t excp,
 cs->exception_index = excp;
 env->exception.syndrome = syndrome;
 env->exception.target_el = target_el;
-
-return cs;
-}
-
-void raise_exception(CPUARMState *env, uint32_t excp,
- uint32_t syndrome, uint32_t target_el)
-{
-CPUState *cs = do_raise_exception(env, excp, syndrome, target_el);
 cpu_loop_exit(cs);
 }
 
-- 
2.30.2

[PATCHv2] target/arm: make pointer authentication a switchable feature

[Jamie Iles]

Rather than having pointer authentication properties be specific to the
max CPU type, turn this into a generic feature that can be set for each
CPU model.  This means that for future CPU types the feature can be set
without having the ID_AA64ISAR1 bits clobbered in
arm_cpu_pauth_finalize.  This also makes it possible for real CPU models
to use the impdef algorithm for improved performance by setting
pauth-impdef=on on the command line.

Cc: Richard Henderson 
Cc: Peter Maydell 
Signed-off-by: Jamie Iles 
---

Following Richard's suggestion to make impdef selectable for all CPUs 
where pointer auth is supported, I've moved this up to a full feature 
and then any future CPUs supporting pointer auth can just set 
ARM_FEATURE_PAUTH.

 target/arm/cpu.h   |  2 ++
 target/arm/cpu64.c | 13 +
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 272fde83ca4e..f724744c4f2b 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -969,6 +969,7 @@ struct ARMCPU {
  */
 bool prop_pauth;
 bool prop_pauth_impdef;
+bool has_pauth;
 
 /* DCZ blocksize, in log_2(words), ie low 4 bits of DCZID_EL0 */
 uint32_t dcz_blocksize;
@@ -2115,6 +2116,7 @@ enum arm_features {
 ARM_FEATURE_M_SECURITY, /* M profile Security Extension */
 ARM_FEATURE_M_MAIN, /* M profile Main Extension */
 ARM_FEATURE_V8_1M, /* M profile extras only in v8.1M and later */
+ARM_FEATURE_PAUTH, /* has pointer authentication support */
 };
 
 static inline int arm_feature(CPUARMState *env, int feature)
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index f42803ecaf1d..5a4386ce9218 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -760,10 +760,7 @@ static void aarch64_max_initfn(Object *obj)
 cpu->ctr = 0x80038003; /* 32 byte I and D cacheline size, VIPT icache 
*/
 cpu->dcz_blocksize = 7; /*  512 bytes */
 #endif
-
-/* Default to PAUTH on, with the architected algorithm. */
-qdev_property_add_static(DEVICE(obj), _cpu_pauth_property);
-qdev_property_add_static(DEVICE(obj), _cpu_pauth_impdef_property);
+set_feature(>env, ARM_FEATURE_PAUTH);
 }
 
 aarch64_add_sve_properties(obj);
@@ -835,8 +832,16 @@ static void aarch64_cpu_class_init(ObjectClass *oc, void 
*data)
 static void aarch64_cpu_instance_init(Object *obj)
 {
 ARMCPUClass *acc = ARM_CPU_GET_CLASS(obj);
+ARMCPU *cpu = ARM_CPU(obj);
 
 acc->info->initfn(obj);
+
+/* Default to PAUTH on, with the architected algorithm. */
+if (arm_feature(>env, ARM_FEATURE_PAUTH)) {
+qdev_property_add_static(DEVICE(obj), _cpu_pauth_property);
+qdev_property_add_static(DEVICE(obj), _cpu_pauth_impdef_property);
+}
+
 arm_cpu_post_init(obj);
 }
 
-- 
2.30.2

Re: [PATCH] target/arm: fix missing exception class

[Jamie Iles]

Hi Peter,

On Mon, May 24, 2021 at 10:41:58AM +0100, Peter Maydell wrote:
> On Mon, 24 May 2021 at 09:42, Jamie Iles  wrote:
> >
> > The DAIF and PAC checks used raise_exception_ra to raise an exception
> > and unwind CPU state but raise_exception_ra is currently designed for
> > handling data aborts as the syndrome is partially precomputed and
> > encoded in the TB and then merged in merge_syn_data_abort when handling
> > the data abort.  Using raise_exception_ra for DAIF and PAC checks
> > results in an empty syndrome being retrieved from data[2] in
> > restore_state_to_opc and setting ESR to 0.  This manifested as:
> >
> >   kvm [571]: Unknown exception class: esr: 0x00 –
> >   Unknown/Uncategorized
> >
> > when launching a KVM guest when the host qemu used a CPU supporting
> > EL2+pointer authentication and enabling pointer authentication in the
> > guest.
> 
> raise_exception() and raise_exception_ra() are supposed to have
> the same semantics apart from one of them being passed a return
> address. So perhaps we should look at trying to fix this by
> making raise_exception_ra() not first carefully set and then
> very opaquely unconditionally trash env->exception.syndrome...

The simplest fix for this would be the patch below and that is 
consistent with the TLB fault handling code where we first restore state 
then raise the exception, is this the sort of thing you were thinking 
of?

Thanks,

Jamie

diff --git i/target/arm/op_helper.c w/target/arm/op_helper.c
index efcb60099277..0b85403cb9f4 100644
--- i/target/arm/op_helper.c
+++ w/target/arm/op_helper.c
@@ -63,8 +63,11 @@ void raise_exception(CPUARMState *env, uint32_t excp,
 void raise_exception_ra(CPUARMState *env, uint32_t excp, uint32_t syndrome,
 uint32_t target_el, uintptr_t ra)
 {
-CPUState *cs = do_raise_exception(env, excp, syndrome, target_el);
-cpu_loop_exit_restore(cs, ra);
+CPUState *cs = env_cpu(env);
+
+cpu_restore_state(cs, ra, true);
+do_raise_exception(env, excp, syndrome, target_el);
+cpu_loop_exit(cs);
 }
 
 uint64_t HELPER(neon_tbl)(CPUARMState *env, uint32_t desc,

[PATCH] target/arm: fix missing exception class

[Jamie Iles]

The DAIF and PAC checks used raise_exception_ra to raise an exception
and unwind CPU state but raise_exception_ra is currently designed for
handling data aborts as the syndrome is partially precomputed and
encoded in the TB and then merged in merge_syn_data_abort when handling
the data abort.  Using raise_exception_ra for DAIF and PAC checks
results in an empty syndrome being retrieved from data[2] in
restore_state_to_opc and setting ESR to 0.  This manifested as:

  kvm [571]: Unknown exception class: esr: 0x00 –
  Unknown/Uncategorized

when launching a KVM guest when the host qemu used a CPU supporting
EL2+pointer authentication and enabling pointer authentication in the
guest.

As the syndrome and exception class is not encoded in the TB, use
cpu_restore_state and raise_exception to restore the CPU state and raise
the exception with the correct syndrome.

Fixes: 0d43e1a2d29a ("target/arm: Add PAuth helpers")
Cc: Richard Henderson 
Cc: Peter Maydell 
Signed-off-by: Jamie Iles 
---
 target/arm/helper-a64.c   | 12 +++-
 target/arm/pauth_helper.c |  4 +++-
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/target/arm/helper-a64.c b/target/arm/helper-a64.c
index 9cc3b066e28b..83991d783fc1 100644
--- a/target/arm/helper-a64.c
+++ b/target/arm/helper-a64.c
@@ -72,11 +72,13 @@ static void daif_check(CPUARMState *env, uint32_t op,
 {
 /* DAIF update to PSTATE. This is OK from EL0 only if UMA is set.  */
 if (arm_current_el(env) == 0 && !(arm_sctlr(env, 0) & SCTLR_UMA)) {
-raise_exception_ra(env, EXCP_UDEF,
-   syn_aa64_sysregtrap(0, extract32(op, 0, 3),
-   extract32(op, 3, 3), 4,
-   imm, 0x1f, 0),
-   exception_target_el(env), ra);
+CPUState *cs = env_cpu(env);
+cpu_restore_state(cs, ra, true);
+raise_exception(env, EXCP_UDEF,
+syn_aa64_sysregtrap(0, extract32(op, 0, 3),
+extract32(op, 3, 3), 4,
+imm, 0x1f, 0),
+exception_target_el(env));
 }
 }
 
diff --git a/target/arm/pauth_helper.c b/target/arm/pauth_helper.c
index cd6df18150bf..c90a3f765b9c 100644
--- a/target/arm/pauth_helper.c
+++ b/target/arm/pauth_helper.c
@@ -385,7 +385,9 @@ static uint64_t pauth_strip(CPUARMState *env, uint64_t ptr, 
bool data)
 static void QEMU_NORETURN pauth_trap(CPUARMState *env, int target_el,
  uintptr_t ra)
 {
-raise_exception_ra(env, EXCP_UDEF, syn_pactrap(), target_el, ra);
+CPUState *cs = env_cpu(env);
+cpu_restore_state(cs, ra, true);
+raise_exception(env, EXCP_UDEF, syn_pactrap(), target_el);
 }
 
 static void pauth_check_trap(CPUARMState *env, int el, uintptr_t ra)
-- 
2.30.2

[PATCH] target/arm: don't clobber ID_AA64ISAR1 pointer auth

[Jamie Iles]

The pointer auth properties are added to the max CPU type but the
finalization happens for all CPUs.  It makes sense to be able to disable
pointer authentication for the max CPU type, but for future CPUs that
implement pointer authentication and have bits set in ID_AA64ISAR1,
don't clobber them unless there is a property registered that can
disable them.

Cc: Richard Henderson 
Cc: Peter Maydell 
Signed-off-by: Jamie Iles 
---
 target/arm/cpu64.c | 33 +
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index f0a9e968c9c1..81c9e494acb6 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -575,26 +575,31 @@ void aarch64_add_sve_properties(Object *obj)
 
 void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp)
 {
-int arch_val = 0, impdef_val = 0;
+int apa = FIELD_EX64(cpu->isar.id_aa64isar1, ID_AA64ISAR1, APA);
+int gpa = FIELD_EX64(cpu->isar.id_aa64isar1, ID_AA64ISAR1, GPA);
+int api = FIELD_EX64(cpu->isar.id_aa64isar1, ID_AA64ISAR1, API);
+int gpi = FIELD_EX64(cpu->isar.id_aa64isar1, ID_AA64ISAR1, GPI);
 uint64_t t;
 
+if (object_property_find(OBJECT(cpu), "pauth-impdef")) {
+api = gpi = cpu->prop_pauth_impdef;
+}
+
+if (object_property_find(OBJECT(cpu), "pauth")) {
+apa = gpa = cpu->prop_pauth;
+}
+
 /* TODO: Handle HaveEnhancedPAC, HaveEnhancedPAC2, HaveFPAC. */
-if (cpu->prop_pauth) {
-if (cpu->prop_pauth_impdef) {
-impdef_val = 1;
-} else {
-arch_val = 1;
-}
-} else if (cpu->prop_pauth_impdef) {
+if (cpu->prop_pauth_impdef && !cpu->prop_pauth) {
 error_setg(errp, "cannot enable pauth-impdef without pauth");
 error_append_hint(errp, "Add pauth=on to the CPU property list.\n");
 }
 
 t = cpu->isar.id_aa64isar1;
-t = FIELD_DP64(t, ID_AA64ISAR1, APA, arch_val);
-t = FIELD_DP64(t, ID_AA64ISAR1, GPA, arch_val);
-t = FIELD_DP64(t, ID_AA64ISAR1, API, impdef_val);
-t = FIELD_DP64(t, ID_AA64ISAR1, GPI, impdef_val);
+t = FIELD_DP64(t, ID_AA64ISAR1, APA, apa);
+t = FIELD_DP64(t, ID_AA64ISAR1, GPA, gpa);
+t = FIELD_DP64(t, ID_AA64ISAR1, API, api);
+t = FIELD_DP64(t, ID_AA64ISAR1, GPI, gpi);
 cpu->isar.id_aa64isar1 = t;
 }
 
@@ -662,6 +667,10 @@ static void aarch64_max_initfn(Object *obj)
 t = FIELD_DP64(t, ID_AA64ISAR1, SPECRES, 1);
 t = FIELD_DP64(t, ID_AA64ISAR1, FRINTTS, 1);
 t = FIELD_DP64(t, ID_AA64ISAR1, LRCPC, 2); /* ARMv8.4-RCPC */
+t = FIELD_DP64(t, ID_AA64ISAR1, APA, 1);
+t = FIELD_DP64(t, ID_AA64ISAR1, GPA, 1);
+t = FIELD_DP64(t, ID_AA64ISAR1, API, 0);
+t = FIELD_DP64(t, ID_AA64ISAR1, GPI, 0);
 cpu->isar.id_aa64isar1 = t;
 
 t = cpu->isar.id_aa64pfr0;
-- 
2.30.2

[Qemu-devel] [PATCH] monitor: fix build breakage for !CONFIG_VNC

[Jamie Iles]

Commit c62f6d1 (monitor: fix build breakage with --disable-vnc)
conditionalised some VNC setup code but left an unused variable.  Move
the variable into the conditional code to fix the build breakage.

Cc: Luiz Capitulino lcapitul...@redhat.com
Cc: Markus Armbruster arm...@redhat.com
Signed-off-by: Jamie Iles ja...@jamieiles.com
---
 monitor.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/monitor.c b/monitor.c
index 1b8ba2c..fee572c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1189,7 +1189,6 @@ static int add_graphics_client(Monitor *mon, const QDict 
*qdict, QObject **ret_d
 {
 const char *protocol  = qdict_get_str(qdict, protocol);
 const char *fdname = qdict_get_str(qdict, fdname);
-int skipauth = qdict_get_try_bool(qdict, skipauth, 0);
 CharDriverState *s;
 
 if (strcmp(protocol, spice) == 0) {
@@ -1203,6 +1202,7 @@ static int add_graphics_client(Monitor *mon, const QDict 
*qdict, QObject **ret_d
 #ifdef CONFIG_VNC
 } else if (strcmp(protocol, vnc) == 0) {
int fd = monitor_get_fd(mon, fdname);
+int skipauth = qdict_get_try_bool(qdict, skipauth, 0);
vnc_display_add_client(NULL, fd, skipauth);
return 0;
 #endif
-- 
1.7.4.1

Re: [Qemu-devel] [PATCHv2] target-arm: support for ARM1176JZF-s cores

[Jamie Iles]

On Wed, Jul 20, 2011 at 03:35:19PM +0100, Peter Maydell wrote:
 On 19 July 2011 13:32, Jamie Iles ja...@jamieiles.com wrote:
  Add support for v6K ARM1176JZF-S.  This core includes the VA-PA
  translation capability and security extensions.
 
  +static uint32_t arm1176_cp15_c0_c1[8] =
  +{ 0x111, 0x11, 0x33, 0x01130003, 0x01130003, 0x10030302, 0x01222100, 0 };
 
 This seems to be the wrong value for the AUXFR0 (0,c0,c1,3 : fourth item
 in this list). The 1176JZF-S TRM says its value is 0x0, not 0x01130003
 (which I think you've miscopied from the 0,c0,c1,4 MMFR0 entry).

Ahh, yes.  Thanks for that, a good spot.

Jamie

8--

From af92ff0cd28077ff187f2ac1d952f71297b7d026 Mon Sep 17 00:00:00 2001
From: Jamie Iles ja...@jamieiles.com
Date: Thu, 23 Jun 2011 11:37:19 +0100
Subject: [PATCHv3] target-arm: support for ARM1176JZF-s cores

Add support for v6K ARM1176JZF-S.  This core includes the VA-PA
translation capability and security extensions.

v2: Model the version with the VFP
v3: fix up the AFR0 value to be in line with TRM (0x0)

Cc: Peter Maydell peter.mayd...@linaro.org
Cc: Paul Brook p...@codesourcery.com
Cc: Aurelien Jarno aurel...@aurel32.net
Signed-off-by: Jamie Iles ja...@jamieiles.com
---
 target-arm/cpu.h|1 +
 target-arm/helper.c |   23 +++
 2 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 01f5b57..8708f9e 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -414,6 +414,7 @@ void cpu_arm_set_cp_io(CPUARMState *env, int cpnum,
 #define ARM_CPUID_PXA270_C5   0x69054117
 #define ARM_CPUID_ARM1136 0x4117b363
 #define ARM_CPUID_ARM1136_R2  0x4107b362
+#define ARM_CPUID_ARM1176 0x410fb767
 #define ARM_CPUID_ARM11MPCORE 0x410fb022
 #define ARM_CPUID_CORTEXA80x410fc080
 #define ARM_CPUID_CORTEXA90x410fc090
diff --git a/target-arm/helper.c b/target-arm/helper.c
index eda881b..2050653 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -36,6 +36,12 @@ static uint32_t arm1136_cp15_c0_c1[8] =
 static uint32_t arm1136_cp15_c0_c2[8] =
 { 0x00140011, 0x12002111, 0x1123, 0x01102131, 0x141, 0, 0, 0 };
 
+static uint32_t arm1176_cp15_c0_c1[8] =
+{ 0x111, 0x11, 0x33, 0, 0x01130003, 0x10030302, 0x01222100, 0 };
+
+static uint32_t arm1176_cp15_c0_c2[8] =
+{ 0x0140011, 0x12002111, 0x11231121, 0x01102131, 0x01141, 0, 0, 0 };
+
 static uint32_t cpu_arm_find_by_name(const char *name);
 
 static inline void set_feature(CPUARMState *env, int feature)
@@ -86,6 +92,21 @@ static void cpu_reset_model_id(CPUARMState *env, uint32_t id)
 env-cp15.c0_cachetype = 0x1dd20d2;
 env-cp15.c1_sys = 0x00050078;
 break;
+case ARM_CPUID_ARM1176:
+set_feature(env, ARM_FEATURE_V4T);
+set_feature(env, ARM_FEATURE_V5);
+set_feature(env, ARM_FEATURE_V6);
+set_feature(env, ARM_FEATURE_V6K);
+set_feature(env, ARM_FEATURE_VFP);
+set_feature(env, ARM_FEATURE_AUXCR);
+env-vfp.xregs[ARM_VFP_FPSID] = 0x410120b5;
+env-vfp.xregs[ARM_VFP_MVFR0] = 0x;
+env-vfp.xregs[ARM_VFP_MVFR1] = 0x;
+memcpy(env-cp15.c0_c1, arm1176_cp15_c0_c1, 8 * sizeof(uint32_t));
+memcpy(env-cp15.c0_c2, arm1176_cp15_c0_c2, 8 * sizeof(uint32_t));
+env-cp15.c0_cachetype = 0x1dd20d2;
+env-cp15.c1_sys = 0x00050078;
+break;
 case ARM_CPUID_ARM11MPCORE:
 set_feature(env, ARM_FEATURE_V4T);
 set_feature(env, ARM_FEATURE_V5);
@@ -377,6 +398,7 @@ static const struct arm_cpu_t arm_cpu_names[] = {
 { ARM_CPUID_ARM1026, arm1026},
 { ARM_CPUID_ARM1136, arm1136},
 { ARM_CPUID_ARM1136_R2, arm1136-r2},
+{ ARM_CPUID_ARM1176, arm1176},
 { ARM_CPUID_ARM11MPCORE, arm11mpcore},
 { ARM_CPUID_CORTEXM3, cortex-m3},
 { ARM_CPUID_CORTEXA8, cortex-a8},
@@ -1770,6 +1792,7 @@ uint32_t HELPER(get_cp15)(CPUState *env, uint32_t insn)
 return 1;
 case ARM_CPUID_ARM1136:
 case ARM_CPUID_ARM1136_R2:
+case ARM_CPUID_ARM1176:
 return 7;
 case ARM_CPUID_ARM11MPCORE:
 return 1;
-- 
1.7.4.1

Re: [Qemu-devel] [PATCHv2] target-arm: support for ARM1176JZF-s cores

[Jamie Iles]

On Wed, Jul 20, 2011 at 04:59:29PM +0100, Peter Maydell wrote:
 On 20 July 2011 16:36, Jamie Iles ja...@jamieiles.com wrote:
  On Wed, Jul 20, 2011 at 03:35:19PM +0100, Peter Maydell wrote:
  On 19 July 2011 13:32, Jamie Iles ja...@jamieiles.com wrote:
   Add support for v6K ARM1176JZF-S.  This core includes the VA-PA
   translation capability and security extensions.
 
   +static uint32_t arm1176_cp15_c0_c1[8] =
   +{ 0x111, 0x11, 0x33, 0x01130003, 0x01130003, 0x10030302, 0x01222100, 0 
   };
 
  This seems to be the wrong value for the AUXFR0 (0,c0,c1,3 : fourth item
  in this list). The 1176JZF-S TRM says its value is 0x0, not 0x01130003
  (which I think you've miscopied from the 0,c0,c1,4 MMFR0 entry).
 
  Ahh, yes.  Thanks for that, a good spot.
 
 Thanks for the quick patch refresh. This version is
 Reviewed-by: Peter Maydell peter.mayd...@linaro.org
 
  8--
 
 Incidentally, this is a bit of an unfortunate format for patches, as it
 doesn't apply with the usual git am filename. Even if you add -c git
 still puts these headers:
 
  From af92ff0cd28077ff187f2ac1d952f71297b7d026 Mon Sep 17 00:00:00 2001
  From: Jamie Iles ja...@jamieiles.com
  Date: Thu, 23 Jun 2011 11:37:19 +0100
  Subject: [PATCHv3] target-arm: support for ARM1176JZF-s cores
 
 into the commit log.

Hmm, I misunderstood the scissors option, I guess a plain patch reply 
would have been best.

Thanks for the pointer (and Reviewed-by)!

Jamie

[Qemu-devel] [PATCHv2] target-arm: support for ARM1176JZF-s cores

[Jamie Iles]

Add support for v6K ARM1176JZF-S.  This core includes the VA-PA
translation capability and security extensions.

v2: Model the version with the VFP

Cc: Peter Maydell peter.mayd...@linaro.org
Cc: Paul Brook p...@codesourcery.com
Cc: Aurelien Jarno aurel...@aurel32.net
Signed-off-by: Jamie Iles ja...@jamieiles.com
---
 target-arm/cpu.h|1 +
 target-arm/helper.c |   23 +++
 2 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 01f5b57..8708f9e 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -414,6 +414,7 @@ void cpu_arm_set_cp_io(CPUARMState *env, int cpnum,
 #define ARM_CPUID_PXA270_C5   0x69054117
 #define ARM_CPUID_ARM1136 0x4117b363
 #define ARM_CPUID_ARM1136_R2  0x4107b362
+#define ARM_CPUID_ARM1176 0x410fb767
 #define ARM_CPUID_ARM11MPCORE 0x410fb022
 #define ARM_CPUID_CORTEXA80x410fc080
 #define ARM_CPUID_CORTEXA90x410fc090
diff --git a/target-arm/helper.c b/target-arm/helper.c
index eda881b..c5ba5a6 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -36,6 +36,12 @@ static uint32_t arm1136_cp15_c0_c1[8] =
 static uint32_t arm1136_cp15_c0_c2[8] =
 { 0x00140011, 0x12002111, 0x1123, 0x01102131, 0x141, 0, 0, 0 };
 
+static uint32_t arm1176_cp15_c0_c1[8] =
+{ 0x111, 0x11, 0x33, 0x01130003, 0x01130003, 0x10030302, 0x01222100, 0 };
+
+static uint32_t arm1176_cp15_c0_c2[8] =
+{ 0x0140011, 0x12002111, 0x11231121, 0x01102131, 0x01141, 0, 0, 0 };
+
 static uint32_t cpu_arm_find_by_name(const char *name);
 
 static inline void set_feature(CPUARMState *env, int feature)
@@ -86,6 +92,21 @@ static void cpu_reset_model_id(CPUARMState *env, uint32_t id)
 env-cp15.c0_cachetype = 0x1dd20d2;
 env-cp15.c1_sys = 0x00050078;
 break;
+case ARM_CPUID_ARM1176:
+set_feature(env, ARM_FEATURE_V4T);
+set_feature(env, ARM_FEATURE_V5);
+set_feature(env, ARM_FEATURE_V6);
+set_feature(env, ARM_FEATURE_V6K);
+set_feature(env, ARM_FEATURE_VFP);
+set_feature(env, ARM_FEATURE_AUXCR);
+env-vfp.xregs[ARM_VFP_FPSID] = 0x410120b5;
+env-vfp.xregs[ARM_VFP_MVFR0] = 0x;
+env-vfp.xregs[ARM_VFP_MVFR1] = 0x;
+memcpy(env-cp15.c0_c1, arm1176_cp15_c0_c1, 8 * sizeof(uint32_t));
+memcpy(env-cp15.c0_c2, arm1176_cp15_c0_c2, 8 * sizeof(uint32_t));
+env-cp15.c0_cachetype = 0x1dd20d2;
+env-cp15.c1_sys = 0x00050078;
+break;
 case ARM_CPUID_ARM11MPCORE:
 set_feature(env, ARM_FEATURE_V4T);
 set_feature(env, ARM_FEATURE_V5);
@@ -377,6 +398,7 @@ static const struct arm_cpu_t arm_cpu_names[] = {
 { ARM_CPUID_ARM1026, arm1026},
 { ARM_CPUID_ARM1136, arm1136},
 { ARM_CPUID_ARM1136_R2, arm1136-r2},
+{ ARM_CPUID_ARM1176, arm1176},
 { ARM_CPUID_ARM11MPCORE, arm11mpcore},
 { ARM_CPUID_CORTEXM3, cortex-m3},
 { ARM_CPUID_CORTEXA8, cortex-a8},
@@ -1770,6 +1792,7 @@ uint32_t HELPER(get_cp15)(CPUState *env, uint32_t insn)
 return 1;
 case ARM_CPUID_ARM1136:
 case ARM_CPUID_ARM1136_R2:
+case ARM_CPUID_ARM1176:
 return 7;
 case ARM_CPUID_ARM11MPCORE:
 return 1;
-- 
1.7.4.1

[Qemu-devel] [PATCHv2 2/2] target-arm: support for ARM1176JZ-s cores

[Jamie Iles]

Add support for v6K ARM1176JZ-S.  This core includes the VA-PA
translation capability and security extensions.

Cc: Peter Maydell peter.mayd...@linaro.org
Cc: Paul Brook p...@codesourcery.com
Cc: Aurelien Jarno aurel...@aurel32.net
Signed-off-by: Jamie Iles ja...@jamieiles.com
---
 target-arm/cpu.h|1 +
 target-arm/helper.c |   19 +++
 2 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 01f5b57..8708f9e 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -414,6 +414,7 @@ void cpu_arm_set_cp_io(CPUARMState *env, int cpnum,
 #define ARM_CPUID_PXA270_C5   0x69054117
 #define ARM_CPUID_ARM1136 0x4117b363
 #define ARM_CPUID_ARM1136_R2  0x4107b362
+#define ARM_CPUID_ARM1176 0x410fb767
 #define ARM_CPUID_ARM11MPCORE 0x410fb022
 #define ARM_CPUID_CORTEXA80x410fc080
 #define ARM_CPUID_CORTEXA90x410fc090
diff --git a/target-arm/helper.c b/target-arm/helper.c
index eda881b..aee1456 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -36,6 +36,12 @@ static uint32_t arm1136_cp15_c0_c1[8] =
 static uint32_t arm1136_cp15_c0_c2[8] =
 { 0x00140011, 0x12002111, 0x1123, 0x01102131, 0x141, 0, 0, 0 };
 
+static uint32_t arm1176_cp15_c0_c1[8] =
+{ 0x111, 0x11, 0x33, 0x01130003, 0x01130003, 0x10030302, 0x01222100, 0 };
+
+static uint32_t arm1176_cp15_c0_c2[8] =
+{ 0x0140011, 0x12002111, 0x11231121, 0x01102131, 0x01141, 0, 0, 0 };
+
 static uint32_t cpu_arm_find_by_name(const char *name);
 
 static inline void set_feature(CPUARMState *env, int feature)
@@ -86,6 +92,17 @@ static void cpu_reset_model_id(CPUARMState *env, uint32_t id)
 env-cp15.c0_cachetype = 0x1dd20d2;
 env-cp15.c1_sys = 0x00050078;
 break;
+case ARM_CPUID_ARM1176:
+set_feature(env, ARM_FEATURE_V4T);
+set_feature(env, ARM_FEATURE_V5);
+set_feature(env, ARM_FEATURE_V6);
+set_feature(env, ARM_FEATURE_V6K);
+set_feature(env, ARM_FEATURE_AUXCR);
+memcpy(env-cp15.c0_c1, arm1176_cp15_c0_c1, 8 * sizeof(uint32_t));
+memcpy(env-cp15.c0_c2, arm1176_cp15_c0_c2, 8 * sizeof(uint32_t));
+env-cp15.c0_cachetype = 0x1dd20d2;
+env-cp15.c1_sys = 0x00050078;
+break;
 case ARM_CPUID_ARM11MPCORE:
 set_feature(env, ARM_FEATURE_V4T);
 set_feature(env, ARM_FEATURE_V5);
@@ -377,6 +394,7 @@ static const struct arm_cpu_t arm_cpu_names[] = {
 { ARM_CPUID_ARM1026, arm1026},
 { ARM_CPUID_ARM1136, arm1136},
 { ARM_CPUID_ARM1136_R2, arm1136-r2},
+{ ARM_CPUID_ARM1176, arm1176},
 { ARM_CPUID_ARM11MPCORE, arm11mpcore},
 { ARM_CPUID_CORTEXM3, cortex-m3},
 { ARM_CPUID_CORTEXA8, cortex-a8},
@@ -1770,6 +1788,7 @@ uint32_t HELPER(get_cp15)(CPUState *env, uint32_t insn)
 return 1;
 case ARM_CPUID_ARM1136:
 case ARM_CPUID_ARM1136_R2:
+case ARM_CPUID_ARM1176:
 return 7;
 case ARM_CPUID_ARM11MPCORE:
 return 1;
-- 
1.7.4.1

[Qemu-devel] [PATCHv2 1/2] target-arm: make VMSAv7 remapping and AP dependent on V6K

[Jamie Iles]

The VMSAv7 remapping and access permissions were introduced in ARMv6K
and not ARMv7.

Cc: Peter Maydell peter.mayd...@linaro.org
Cc: Aurelien Jarno aurel...@aurel32.net
Cc: Paul Brook p...@codesourcery.com
Signed-off-by: Jamie Iles ja...@jamieiles.com
---
 target-arm/helper.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 1208416..eda881b 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -945,7 +945,7 @@ static inline int check_ap(CPUState *env, int ap, int 
domain, int access_type,
   case 6:
   return prot_ro;
   case 7:
-  if (!arm_feature (env, ARM_FEATURE_V7))
+  if (!arm_feature (env, ARM_FEATURE_V6K))
   return 0;
   return prot_ro;
   default:
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH] target-arm: support for ARM1176JZ-s cores

[Jamie Iles]

On Wed, Jun 22, 2011 at 10:40:12AM +0100, Peter Maydell wrote:
 On 22 June 2011 00:42, Jamie Iles ja...@jamieiles.com wrote:
  On 21 June 2011 23:13, Peter Maydell peter.mayd...@linaro.org wrote:
  Ah yes, sorry, I misread the TRM there. So it does have those, it's
  just the SEV/WFI/WFE it is missing. I guess we'll want an
  ARM_FEATURE_VAPA too.
 
  Could we perhaps infer and detect some of these features?  For example,
  my reading of the ARM ARM says that the VA-PA translation registers
  exist for v7 or v6k if the security extensions exist.  We can detect
  the security extensions from the cpuid registers so we could
  automatically set that feature.
 
 I thought about that, but there are a couple of reasons I'd rather
 not detect things from the cpuid/feature registers:
  * older cores don't have them, so you need to cope without them anyway
  * there's a tension between emulate the same feature regs as the
h/w and emulate feature regs matching what we implement -- some
guest OSes will actually refuse to boot unless they get exact matches
on the feature reg values...
(At the moment we tend to the former, so we probably advertise the
security extensions even though we don't implement them, for instance)
  * at the moment the feature regs are just random hex values in helper.c;
if we wanted to drive things from them we'd need to set up a lot of
enumerations and constants anyway in order to have something maintainable
 
 Inferring ARM_FEATURE_foo flags from other ARM_FEATURE_foo flags is fine,
 though.
 
 Mostly what I'd like is for the actual code implementing things to
 be gated on a fairly fine-grained set of flags, so that we can confine
 the what does this core have? what things imply what other things?
 code to a single place where it's easy to tweak if we get it wrong.

OK, I don't think I can object to that!  I'll submit a patch to fix up 
the v7 VMSA ap/remap dependency to be v6K rather than v7.  Given that, 
do you have any objection to adding 1167 as a v6K?  I'm happy to help 
with/test some of the feature cleanup.

Jamie

Re: [Qemu-devel] [PATCH] target-arm: support for ARM1176JZ-s cores

[Jamie Iles]

On Wed, Jun 22, 2011 at 05:01:30PM +0100, Peter Maydell wrote:
 On 22 June 2011 16:45, Jamie Iles ja...@jamieiles.com wrote:
  On Wed, Jun 22, 2011 at 10:40:12AM +0100, Peter Maydell wrote:
  Mostly what I'd like is for the actual code implementing things to
  be gated on a fairly fine-grained set of flags, so that we can confine
  the what does this core have? what things imply what other things?
  code to a single place where it's easy to tweak if we get it wrong.
 
  OK, I don't think I can object to that!  I'll submit a patch to fix up
  the v7 VMSA ap/remap dependency to be v6K rather than v7.  Given that,
  do you have any objection to adding 1167 as a v6K?  I'm happy to help
  with/test some of the feature cleanup.
 
 I think the question is, if you mark the 1176 as a v6K then how do
 you gate the working WFI from WFI instruction, which otherwise
 could reasonably be marked as one of the features implied by v6K?

The ARM1176 technically is a v6K core, but the actual definition of v6K 
seems a bit vague on required features.

As wfi is a valid encoding on 1176 I personally don't see this as being 
a blocking issue (though technically incorrect, though most code 
targeted for this CPU presumably wouldn't emit a wfi instruction?).

Jamie

[Qemu-devel] [PATCH] target-arm: support for ARM1176JZ-s cores

[Jamie Iles]

Add support for the ARM1176JZ-s cores.  The ARM1176JZ-s is a v6K core
but uses the v7 VMSA for remapping and access permissions and there is
no way to identify these VMSA extensions from the cpuid feature
registers.

Cc: Paul Brook p...@codesourcery.com
Cc: Aurelien Jarno aurel...@aurel32.net
Signed-off-by: Jamie Iles jamie.i...@picochip.com
---
 target-arm/cpu.h|1 +
 target-arm/helper.c |   23 ++-
 2 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 01f5b57..8708f9e 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -414,6 +414,7 @@ void cpu_arm_set_cp_io(CPUARMState *env, int cpnum,
 #define ARM_CPUID_PXA270_C5   0x69054117
 #define ARM_CPUID_ARM1136 0x4117b363
 #define ARM_CPUID_ARM1136_R2  0x4107b362
+#define ARM_CPUID_ARM1176 0x410fb767
 #define ARM_CPUID_ARM11MPCORE 0x410fb022
 #define ARM_CPUID_CORTEXA80x410fc080
 #define ARM_CPUID_CORTEXA90x410fc090
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 1208416..63df576 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -36,6 +36,12 @@ static uint32_t arm1136_cp15_c0_c1[8] =
 static uint32_t arm1136_cp15_c0_c2[8] =
 { 0x00140011, 0x12002111, 0x1123, 0x01102131, 0x141, 0, 0, 0 };
 
+static uint32_t arm1176_cp15_c0_c1[8] =
+{ 0x111, 0x11, 0x33, 0x01130003, 0x01130003, 0x10030302, 0x01222100, 0 };
+
+static uint32_t arm1176_cp15_c0_c2[8] =
+{ 0x0140011, 0x12002111, 0x11231121, 0x01102131, 0x01141, 0, 0, 0 };
+
 static uint32_t cpu_arm_find_by_name(const char *name);
 
 static inline void set_feature(CPUARMState *env, int feature)
@@ -86,6 +92,17 @@ static void cpu_reset_model_id(CPUARMState *env, uint32_t id)
 env-cp15.c0_cachetype = 0x1dd20d2;
 env-cp15.c1_sys = 0x00050078;
 break;
+case ARM_CPUID_ARM1176:
+set_feature(env, ARM_FEATURE_V4T);
+set_feature(env, ARM_FEATURE_V5);
+set_feature(env, ARM_FEATURE_V6);
+set_feature(env, ARM_FEATURE_V6K);
+set_feature(env, ARM_FEATURE_AUXCR);
+memcpy(env-cp15.c0_c1, arm1176_cp15_c0_c1, 8 * sizeof(uint32_t));
+memcpy(env-cp15.c0_c2, arm1176_cp15_c0_c2, 8 * sizeof(uint32_t));
+env-cp15.c0_cachetype = 0x1dd20d2;
+env-cp15.c1_sys = 0x00050078;
+break;
 case ARM_CPUID_ARM11MPCORE:
 set_feature(env, ARM_FEATURE_V4T);
 set_feature(env, ARM_FEATURE_V5);
@@ -377,6 +394,7 @@ static const struct arm_cpu_t arm_cpu_names[] = {
 { ARM_CPUID_ARM1026, arm1026},
 { ARM_CPUID_ARM1136, arm1136},
 { ARM_CPUID_ARM1136_R2, arm1136-r2},
+{ ARM_CPUID_ARM1176, arm1176},
 { ARM_CPUID_ARM11MPCORE, arm11mpcore},
 { ARM_CPUID_CORTEXM3, cortex-m3},
 { ARM_CPUID_CORTEXA8, cortex-a8},
@@ -945,7 +963,9 @@ static inline int check_ap(CPUState *env, int ap, int 
domain, int access_type,
   case 6:
   return prot_ro;
   case 7:
-  if (!arm_feature (env, ARM_FEATURE_V7))
+  /* ARM1176 uses VMSAv7 remapping and access flag. */
+  if (!arm_feature (env, ARM_FEATURE_V7) 
+  ARM_CPUID(env) != ARM_CPUID_ARM1176)
   return 0;
   return prot_ro;
   default:
@@ -1770,6 +1790,7 @@ uint32_t HELPER(get_cp15)(CPUState *env, uint32_t insn)
 return 1;
 case ARM_CPUID_ARM1136:
 case ARM_CPUID_ARM1136_R2:
+case ARM_CPUID_ARM1176:
 return 7;
 case ARM_CPUID_ARM11MPCORE:
 return 1;
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH] target-arm: support for ARM1176JZ-s cores

[Jamie Iles]

Hi Peter,

On Tue, Jun 21, 2011 at 04:43:44PM +0100, Peter Maydell wrote:
 On 21 June 2011 13:55, Jamie Iles ja...@jamieiles.com wrote:
  +    case ARM_CPUID_ARM1176:
  +        set_feature(env, ARM_FEATURE_V4T);
  +        set_feature(env, ARM_FEATURE_V5);
  +        set_feature(env, ARM_FEATURE_V6);
  +        set_feature(env, ARM_FEATURE_V6K);
  +        set_feature(env, ARM_FEATURE_AUXCR);
 
 This isn't quite right -- the 1176 isn't a v6K. In particular it
 is missing the VA-PA translation cp15 regs, and SEV, WFI and WFE
 have to no-op rather than doing anything.

Hmm, perhaps I should have specified the 1176JZ-S?  According to the TRM 
it does have the VA-PA translation operations in the cache operations 
cp15 register.  We have a platform with an ARM1176JZ-S that had 
previously been confirmed by ARM has having the K extensions.

Also it does support wait-for-interrupt stalling, but only through a 
cp15 access and not the wfi instruction (which is a nop).

 It does have the v6K byte/halfword/double ld/st exclusives, CLREX, 
 YIELD and NOP. (It's like the 1136r1 in this regard, another case that 
 came up on the list recently.)
 
 I think we really need to split some of these subcases out into
 their own ARM_FEATURE_ bits; see below.
 
  @@ -945,7 +963,9 @@ static inline int check_ap(CPUState *env, int ap, int 
  domain, int access_type,
    case 6:
        return prot_ro;
    case 7:
  -      if (!arm_feature (env, ARM_FEATURE_V7))
  +      /* ARM1176 uses VMSAv7 remapping and access flag. */
  +      if (!arm_feature (env, ARM_FEATURE_V7) 
  +          ARM_CPUID(env) != ARM_CPUID_ARM1176)
            return 0;
        return prot_ro;
 
 Turning on features based on specific CPUs is a bit of a wart;
 I don't think this condition was quite right anyway. This
 (the additional access permission encoding AP[2:0] = 0b111)
 was introduced in ARMv6K, not ARMv7. It's also present in
 1176 and in 1136r1 and above. (The same is true of the other
 v6K VMSA features, TEX remapping and the SCTLR access flag.)

Yes, that is a bit ugly.  If that's wrong anyway I'll submit a patch to 
change this to just be v6k for now before cleaning up the feature bits.

 [We don't currently implement TEX remapping at all...]
 
 Anyway, feature bit cleanup. Here's a concrete proposal,
 starting with the ones we already have. Rather than every
 core setting every feature flag it needs, I suggest that we
 have a little bit of common code which knows which features
 imply which others. So each core picks one core architecture
 version and then a hopefully small set of extra specific
 feature flags for things not implied by the core arch version.
 
 core architecture version flags: these imply all the
 preceding arch versions, and also various specific features
 which are mandatory in that arch version.
 
  V4T
  V5 implies V4T
  V6 implies V5, V4T, AUXCR
  V6K implies V6, V5, V4T
  V7 implies V6K, V6, V5, V4T, THUMB2EE, THUMB2
  XSCALE implies V5
  IWMMXT implies XSCALE
 
 Existing specific feature flags: these indicate particular
 well-defined features which may exist either as an
 optional part of an arch version, or which might appear
 in earlier cores before being rolled into the formal
 architecture spec:
 
  THUMB2
  THUMB2EE
  V7MP  # ie the multiprocessing extensions
  VFP
  VFP3 implies VFP
  VFP_FP16  # ie half-precision support
  NEON
  DIV
  MPU
  M  # a bit of a special case since it's a whole other
 # architecture variant, but it could be v6 or v7.
 # M  V7 implies DIV, THUMB2.
 
 Existing feature flags which are really trying to
 get device-specific cp15 registers right; I think we
 could clean up our cp15 support to the point where these
 weren't actually needed, but that's a different topic and
 for now I'm happy to leave them be:
  AUXCR

We should be able to get the presence of the ACR from memory model 
feature register 0 if my understanding of the ARM ARM is correct, but as 
the contents are implementation defined I guess we need a way to handle 
the accesses.

  STRONGARM
  OMAPCP
 
 Which puts us in a position to define some new feature
 flags to account for 1136/1176:
  V6K_EXCLUSIVES  # CLREX, LDREXB, LDREXH, LDREXD,
  # STREXB, STREXD, STREXH
 
  V6K_VMSA # TEX remapping, SCTLR AFE, AP[2:0] = 0b111 encoding
 
  NOPHINTS # enable the nop/yield/wfi/wfe space
   # (WFI and WFE are only non-NOPs if FEATURE_V6K.)

I thought the only thing that made them non-NOPs were if the system was 
multi-core v6K.

 (V6K implies V6K_EXCLUSIVES, V6K_VMSA, NOPHINTS. THUMB2
 implies NOPHINTS. V6K implies NOPHINTS. 1136r1 and 1176
 set V6, V6K_EXCLUSIVES, V6K_VMSA, NOPHINTS.)

Jamie

Re: [Qemu-devel] [PATCH] target-arm: support for ARM1176JZ-s cores

[Jamie Iles]

On 21 June 2011 23:13, Peter Maydell peter.mayd...@linaro.org wrote:
 On 21 June 2011 17:13, Jamie Iles ja...@jamieiles.com wrote:
 Hi Peter,

 On Tue, Jun 21, 2011 at 04:43:44PM +0100, Peter Maydell wrote:
 On 21 June 2011 13:55, Jamie Iles ja...@jamieiles.com wrote:
  +    case ARM_CPUID_ARM1176:
  +        set_feature(env, ARM_FEATURE_V4T);
  +        set_feature(env, ARM_FEATURE_V5);
  +        set_feature(env, ARM_FEATURE_V6);
  +        set_feature(env, ARM_FEATURE_V6K);
  +        set_feature(env, ARM_FEATURE_AUXCR);

 This isn't quite right -- the 1176 isn't a v6K. In particular it
 is missing the VA-PA translation cp15 regs, and SEV, WFI and WFE
 have to no-op rather than doing anything.

 Hmm, perhaps I should have specified the 1176JZ-S?  According to the TRM
 it does have the VA-PA translation operations in the cache operations
 cp15 register.

 Ah yes, sorry, I misread the TRM there. So it does have those, it's
 just the SEV/WFI/WFE it is missing. I guess we'll want an
 ARM_FEATURE_VAPA too.

Could we perhaps infer and detect some of these features?  For example,
my reading of the ARM ARM says that the VA-PA translation registers
exist for v7 or v6k if the security extensions exist.  We can detect
the security extensions from the cpuid registers so we could
automatically set that feature.

 Which puts us in a position to define some new feature
 flags to account for 1136/1176:
  V6K_EXCLUSIVES  # CLREX, LDREXB, LDREXH, LDREXD,
                  # STREXB, STREXD, STREXH

  V6K_VMSA # TEX remapping, SCTLR AFE, AP[2:0] = 0b111 encoding

Incidentally, the ARM ARM says that ID_MMFR0[3:0] = 0b0011 means
VMSAv7 with the above remapping, but 1136 appears to have this too
(I think it's only 1156 out of the v6 cores that doesn't).

  NOPHINTS # enable the nop/yield/wfi/wfe space
           # (WFI and WFE are only non-NOPs if FEATURE_V6K.)

 Whoops, I forgot TLS_REGISTERS (1136r1 has those as does 1176).

Ahh, yes.  I can't see any way to detect this - though the security
extensions so you could infer that for 1176, but not 1136.

 I thought the only thing that made them non-NOPs were if the system was
 multi-core v6K.

 I think you end up in about the same place if you say the
 1176 is a v6 with all the v6K extras except non-nop WFI,
 as if you say 1176 is a v6K but v6K WFI is only non-nop on
 multicore. And it's easier to define the 1176 as v6 + lots
 than to try to have a means for cpus to remove feature flags
 that V6K would otherwise imply, just for this one corner case.

Agreed.  Again, could we autodetect this though, or would having
the features explicitly set be better?  We should be able to do
things like ARM_FEATURE_THUMB2EE from the cpuid registers too right?

Jamie