Re: [Qemu-arm] [Qemu-devel] [PATCH v2 15/15] arm/xlnx-zynqmp: put APUs and RPUs in separate GDB groups

[Philippe Mathieu-Daudé]

On 03/10/2018 13:44, Luc Michel wrote:
> On 10/2/18 1:58 PM, Peter Maydell wrote:
>> On 2 October 2018 at 12:33, Philippe Mathieu-Daudé <address@hidden> wrote:
>>> Cc'ing more QOM involved people.
>>>
>>> On 01/10/2018 13:57, Luc Michel wrote:
>>>> Create two separate QOM containers for APUs and RPUs to indicate to the
>>>> GDB stub that those CPUs should be put in different processes.
>>>>
>>>> Signed-off-by: Luc Michel <address@hidden>
>>>> ---
>>>>  hw/arm/xlnx-zynqmp.c | 7 +++++--
>>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
>>>> index c195040350..5e92adbc71 100644
>>>> --- a/hw/arm/xlnx-zynqmp.c
>>>> +++ b/hw/arm/xlnx-zynqmp.c
>>>> @@ -22,10 +22,11 @@
>>>>  #include "hw/arm/xlnx-zynqmp.h"
>>>>  #include "hw/intc/arm_gic_common.h"
>>>>  #include "exec/address-spaces.h"
>>>>  #include "sysemu/kvm.h"
>>>>  #include "kvm_arm.h"
>>>> +#include "exec/gdbstub.h"
>>>>
>>>>  #define GIC_NUM_SPI_INTR 160
>>>>
>>>>  #define ARM_PHYS_TIMER_PPI  30
>>>>  #define ARM_VIRT_TIMER_PPI  27
>>>> @@ -175,17 +176,18 @@ static void xlnx_zynqmp_create_rpu(XlnxZynqMPState 
>>>> *s, const char *boot_cpu,
>>>>                                     Error **errp)
>>>>  {
>>>>      Error *err = NULL;
>>>>      int i;
>>>>      int num_rpus = MIN(smp_cpus - XLNX_ZYNQMP_NUM_APU_CPUS, 
>>>> XLNX_ZYNQMP_NUM_RPU_CPUS);
>>>> +    Object *rpu_group = gdb_cpu_group_container_get(OBJECT(s));
>>>
>>> I'd rather keep this generic: not involve 'gdb' container name.
>>
>> Yes, I agree. We should structure how we construct our
>> model to follow what the hardware has (two CPU clusters
>> with 4 cores each), and then the gdb code should introspect
>> the system later to decide how it exposes things to the gdb
>> user. GDB specifics should (as far as possible) be kept out
>> of the board code.
>>
>> The fact that there are two clusters here also
>> affects other things, like whether they have the
>> same view of memory, whether they can share translated
>> code (they shouldn't but do at the moment), and so on --
>> it's not just a GDB-relevant distinction. So we should
>> be modelling it somehow, definitely. I don't have a clear
>> view how just yet.
> 
> So for now, maybe it's better to rely on an heuristic such as the one
> suggested by Philippe in the gdb code to group the CPUs. Once QEMU gains
> more supports for such heterogeneous architectures, we can remove the
> heuristic and put the proper QOM introspection code instead.

I'm not sure this is the best approach, just suggested because using
object_resolve_path_type("", TYPE_CPU, NULL) seemed to me the
quicker/easiest approach.

Eduardo: Do you have other thoughts on how to resolve those generic
containers, without involving any gdb-specific tag?

>> This probably ties into the stuff I have somewhere on
>> my todo list about supporting multiple heterogenous
>> systems. The problem with this xilinx board is that it
>> is trying to model that kind of system but we don't actually
>> properly support that in QEMU yet.
>>
>> thanks
>> -- PMM
>>