| Message ID | 1446747358-18214-3-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Thu, Nov 05, 2015 at 06:15:44PM +0000, Peter Maydell wrote: > Allow multiple calls to cpu_address_space_init(); each > call adds an entry to the cpu->ases array at the specified > index. It is up to the target-specific CPU code to actually use > these extra address spaces. > > Since this multiple AddressSpace support won't work with > KVM, add an assertion to avoid confusing failures. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > exec.c | 28 ++++++++++++++++++---------- > include/qom/cpu.h | 2 ++ > 2 files changed, 20 insertions(+), 10 deletions(-) > > diff --git a/exec.c b/exec.c > index b5490c8..6a2a694 100644 > --- a/exec.c > +++ b/exec.c > @@ -552,25 +552,32 @@ CPUState *qemu_get_cpu(int index) > #if !defined(CONFIG_USER_ONLY) > void cpu_address_space_init(CPUState *cpu, AddressSpace *as, int asidx) > { > + CPUAddressSpace *newas; > + > if (asidx == 0) { > /* address space 0 gets the convenience alias */ > cpu->as = as; > } > > - /* We only support one address space per cpu at the moment. */ > - assert(cpu->as == as); > + /* KVM cannot currently support multiple address spaces. */ > + assert(asidx == 0 || !kvm_enabled()); > > - if (cpu->cpu_ases) { > - /* We've already registered the listener for our only AS */ > - return; > + if (asidx >= cpu->num_ases) { > + if (cpu->num_ases == 0) { > + cpu->cpu_ases = g_new(CPUAddressSpace, asidx + 1); > + } else { > + cpu->cpu_ases = g_renew(CPUAddressSpace, cpu->cpu_ases, asidx + 1); IIUC, g_renew may move the entire cpu_ases area. The internals of memory_listener_register (called below) seem to put away the pointers to listeners so a renew+move would leave invalid pointers to listeners in memory.c wouldn't it? There are various ways of solving this, (e.g dynamic allocation of the listener, static allocation of the cpu_ases, invalidate all listeners and restore them after each as init and more). I'm sure you'll figure something out. > + } > + cpu->num_ases = asidx + 1; > } > > - cpu->cpu_ases = g_new0(CPUAddressSpace, 1); > - cpu->cpu_ases[0].cpu = cpu; > - cpu->cpu_ases[0].as = as; > + newas = &cpu->cpu_ases[asidx]; > + memset(newas, 0, sizeof(*newas)); > + newas->cpu = cpu; > + newas->as = as; > if (tcg_enabled()) { > - cpu->cpu_ases[0].tcg_as_listener.commit = tcg_commit; > - memory_listener_register(&cpu->cpu_ases[0].tcg_as_listener, as); > + newas->tcg_as_listener.commit = tcg_commit; > + memory_listener_register(&newas->tcg_as_listener, as); > } > } > #endif > @@ -627,6 +634,7 @@ void cpu_exec_init(CPUState *cpu, Error **errp) > Error *local_err = NULL; > > cpu->as = NULL; > + cpu->num_ases = 0; > > #ifndef CONFIG_USER_ONLY > cpu->thread_id = qemu_get_thread_id(); > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index 51a1323..ae17932 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -236,6 +236,7 @@ struct kvm_run; > * so that interrupts take effect immediately. > * @cpu_ases: Pointer to array of CPUAddressSpaces (which define the > * AddressSpaces this CPU has) > + * @num_ases: number of CPUAddressSpaces in @cpu_ases > * @as: Pointer to the first AddressSpace, for the convenience of targets which > * only have a single AddressSpace > * @env_ptr: Pointer to subclass-specific CPUArchState field. > @@ -285,6 +286,7 @@ struct CPUState { > struct qemu_work_item *queued_work_first, *queued_work_last; > > CPUAddressSpace *cpu_ases; > + int num_ases; > AddressSpace *as; > > void *env_ptr; /* CPUArchState */ > -- > 1.9.1 >
On 6 November 2015 at 13:21, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > On Thu, Nov 05, 2015 at 06:15:44PM +0000, Peter Maydell wrote: >> Allow multiple calls to cpu_address_space_init(); each >> call adds an entry to the cpu->ases array at the specified >> index. It is up to the target-specific CPU code to actually use >> these extra address spaces. >> >> Since this multiple AddressSpace support won't work with >> KVM, add an assertion to avoid confusing failures. >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> + if (asidx >= cpu->num_ases) { >> + if (cpu->num_ases == 0) { >> + cpu->cpu_ases = g_new(CPUAddressSpace, asidx + 1); >> + } else { >> + cpu->cpu_ases = g_renew(CPUAddressSpace, cpu->cpu_ases, asidx + 1); > > IIUC, g_renew may move the entire cpu_ases area. The internals of > memory_listener_register (called below) seem to put away the pointers to listeners > so a renew+move would leave invalid pointers to listeners in memory.c wouldn't it? > > There are various ways of solving this, (e.g dynamic allocation of the listener, > static allocation of the cpu_ases, invalidate all listeners and restore them after > each as init and more). I'm sure you'll figure something out. Oops, yes, you're right. Maybe we should just have the target CPU say in advance what the maximum number of AddressSpaces it will have is -- my expectation is that this will be (a) small (b) known in advance anyway. thanks -- PMM
On Fri, Nov 06, 2015 at 01:34:44PM +0000, Peter Maydell wrote: > On 6 November 2015 at 13:21, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > > On Thu, Nov 05, 2015 at 06:15:44PM +0000, Peter Maydell wrote: > >> Allow multiple calls to cpu_address_space_init(); each > >> call adds an entry to the cpu->ases array at the specified > >> index. It is up to the target-specific CPU code to actually use > >> these extra address spaces. > >> > >> Since this multiple AddressSpace support won't work with > >> KVM, add an assertion to avoid confusing failures. > >> > >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > >> + if (asidx >= cpu->num_ases) { > >> + if (cpu->num_ases == 0) { > >> + cpu->cpu_ases = g_new(CPUAddressSpace, asidx + 1); > >> + } else { > >> + cpu->cpu_ases = g_renew(CPUAddressSpace, cpu->cpu_ases, asidx + 1); > > > > IIUC, g_renew may move the entire cpu_ases area. The internals of > > memory_listener_register (called below) seem to put away the pointers to listeners > > so a renew+move would leave invalid pointers to listeners in memory.c wouldn't it? > > > > There are various ways of solving this, (e.g dynamic allocation of the listener, > > static allocation of the cpu_ases, invalidate all listeners and restore them after > > each as init and more). I'm sure you'll figure something out. > > Oops, yes, you're right. > > Maybe we should just have the target CPU say in advance what the > maximum number of AddressSpaces it will have is -- my expectation > is that this will be (a) small (b) known in advance anyway. Yes, that sounds good to me. Cheers, Edgar
On 05/11/2015 19:15, Peter Maydell wrote: > Allow multiple calls to cpu_address_space_init(); each > call adds an entry to the cpu->ases array at the specified > index. It is up to the target-specific CPU code to actually use > these extra address spaces. > > Since this multiple AddressSpace support won't work with > KVM, add an assertion to avoid confusing failures. Actually it won't work _now_ with KVM, but it could. It would be a good idea to map the multiple CPU AddressSpaces to KVM's own multiple address spaces. It's possible to modify i386 to do this, using address space 0 for normal operation and address space 1 for SMM, just like KVM. More on this as I reply to the remainder of the series... Paolo > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
On 06/11/2015 14:34, Peter Maydell wrote: >> > IIUC, g_renew may move the entire cpu_ases area. The internals of >> > memory_listener_register (called below) seem to put away the pointers to listeners >> > so a renew+move would leave invalid pointers to listeners in memory.c wouldn't it? >> > >> > There are various ways of solving this, (e.g dynamic allocation of the listener, >> > static allocation of the cpu_ases, invalidate all listeners and restore them after >> > each as init and more). I'm sure you'll figure something out. > Oops, yes, you're right. > > Maybe we should just have the target CPU say in advance what the > maximum number of AddressSpaces it will have is -- my expectation > is that this will be (a) small (b) known in advance anyway. I agree. Or even just allocate room statically, for the largest amount that all targets in QEMU use. My expectation is that this will be 2. :) Paolo
diff --git a/exec.c b/exec.c index b5490c8..6a2a694 100644 --- a/exec.c +++ b/exec.c @@ -552,25 +552,32 @@ CPUState *qemu_get_cpu(int index) #if !defined(CONFIG_USER_ONLY) void cpu_address_space_init(CPUState *cpu, AddressSpace *as, int asidx) { + CPUAddressSpace *newas; + if (asidx == 0) { /* address space 0 gets the convenience alias */ cpu->as = as; } - /* We only support one address space per cpu at the moment. */ - assert(cpu->as == as); + /* KVM cannot currently support multiple address spaces. */ + assert(asidx == 0 || !kvm_enabled()); - if (cpu->cpu_ases) { - /* We've already registered the listener for our only AS */ - return; + if (asidx >= cpu->num_ases) { + if (cpu->num_ases == 0) { + cpu->cpu_ases = g_new(CPUAddressSpace, asidx + 1); + } else { + cpu->cpu_ases = g_renew(CPUAddressSpace, cpu->cpu_ases, asidx + 1); + } + cpu->num_ases = asidx + 1; } - cpu->cpu_ases = g_new0(CPUAddressSpace, 1); - cpu->cpu_ases[0].cpu = cpu; - cpu->cpu_ases[0].as = as; + newas = &cpu->cpu_ases[asidx]; + memset(newas, 0, sizeof(*newas)); + newas->cpu = cpu; + newas->as = as; if (tcg_enabled()) { - cpu->cpu_ases[0].tcg_as_listener.commit = tcg_commit; - memory_listener_register(&cpu->cpu_ases[0].tcg_as_listener, as); + newas->tcg_as_listener.commit = tcg_commit; + memory_listener_register(&newas->tcg_as_listener, as); } } #endif @@ -627,6 +634,7 @@ void cpu_exec_init(CPUState *cpu, Error **errp) Error *local_err = NULL; cpu->as = NULL; + cpu->num_ases = 0; #ifndef CONFIG_USER_ONLY cpu->thread_id = qemu_get_thread_id(); diff --git a/include/qom/cpu.h b/include/qom/cpu.h index 51a1323..ae17932 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -236,6 +236,7 @@ struct kvm_run; * so that interrupts take effect immediately. * @cpu_ases: Pointer to array of CPUAddressSpaces (which define the * AddressSpaces this CPU has) + * @num_ases: number of CPUAddressSpaces in @cpu_ases * @as: Pointer to the first AddressSpace, for the convenience of targets which * only have a single AddressSpace * @env_ptr: Pointer to subclass-specific CPUArchState field. @@ -285,6 +286,7 @@ struct CPUState { struct qemu_work_item *queued_work_first, *queued_work_last; CPUAddressSpace *cpu_ases; + int num_ases; AddressSpace *as; void *env_ptr; /* CPUArchState */
Allow multiple calls to cpu_address_space_init(); each call adds an entry to the cpu->ases array at the specified index. It is up to the target-specific CPU code to actually use these extra address spaces. Since this multiple AddressSpace support won't work with KVM, add an assertion to avoid confusing failures. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- exec.c | 28 ++++++++++++++++++---------- include/qom/cpu.h | 2 ++ 2 files changed, 20 insertions(+), 10 deletions(-)