[Qemu-devel] [PATCH v1 0/5] xlnx-zynqmp: Fix various issues with KVM runs

[Qemu-devel] [PATCH v1 0/5] xlnx-zynqmp: Fix various issues with KVM runs

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

(This is a resend of the same v1, I managed to typo my own work email
address in CC in the first send-email :-))

This series fixes a bunch of issues with KVM runs using the ZynqMP
machine.

When using KVM, we don't have support for TrustZone, the RPU subsystem
with Cortex R5s and we also need to use the in-kernel GIC model.

This adds a set of properties to the ZynqMP allowing the creation
of the KVM friendly machine.

I've opted to default the ZynqMP to the KVM friendly machine for
consistensy between KVM and TCG and also because some of the options
are currently of limited use (i.e the RPU) or were actually disabled to
start with (e.g TrustZone sneaked in as we enabled EL3 on the A53s).
I'm OK either way though, so if people think differently we can change
the defaults.

Comments welcome!

Cheers,
Edgar

Edgar E. Iglesias (5):
  xlnx-zynqmp: Add a secure prop to en/disable ARM Security Extensions
  xlnx-zynqmp: Break out RPU setup into a separate function
  xlnx-zynqmp: Make the RPU subsystem optional
  xlnx-zynqmp: Delay realization of GIC until post CPU realization
  xlnx-zynqmp: Use the in kernel GIC model for KVM runs

 hw/arm/xlnx-zynqmp.c         | 156 +++++++++++++++++++++++++++++--------------
 include/hw/arm/xlnx-zynqmp.h |   5 ++
 2 files changed, 110 insertions(+), 51 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH v1 1/5] xlnx-zynqmp: Add a secure prop to en/disable ARM Security Extensions

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Add a secure prop to en/disable ARM Security Extensions.
This is particulary useful for KVM runs.

Default to disabled to match the behavior of KVM.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 hw/arm/xlnx-zynqmp.c         | 3 +++
 include/hw/arm/xlnx-zynqmp.h | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 4d504da..965a250 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -238,6 +238,8 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
         }
         g_free(name);
 
+        object_property_set_bool(OBJECT(&s->apu_cpu[i]),
+                                 s->secure, "has_el3", NULL);
         object_property_set_int(OBJECT(&s->apu_cpu[i]), GIC_BASE_ADDR,
                                 "reset-cbar", &error_abort);
         object_property_set_bool(OBJECT(&s->apu_cpu[i]), true, "realized",
@@ -370,6 +372,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
 
 static Property xlnx_zynqmp_props[] = {
     DEFINE_PROP_STRING("boot-cpu", XlnxZynqMPState, boot_cpu),
+    DEFINE_PROP_BOOL("secure", XlnxZynqMPState, secure, false),
     DEFINE_PROP_END_OF_LIST()
 };
 
diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
index 2332596..38d4c8c 100644
--- a/include/hw/arm/xlnx-zynqmp.h
+++ b/include/hw/arm/xlnx-zynqmp.h
@@ -84,6 +84,9 @@ typedef struct XlnxZynqMPState {
 
     char *boot_cpu;
     ARMCPU *boot_cpu_ptr;
+
+    /* Has the ARM Security extensions?  */
+    bool secure;
 }  XlnxZynqMPState;
 
 #define XLNX_ZYNQMP_H
-- 
2.5.0

Re: [Qemu-devel] [PATCH v1 1/5] xlnx-zynqmp: Add a secure prop to en/disable ARM Security Extensions

[Peter Maydell]

On 19 May 2016 at 23:54, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Add a secure prop to en/disable ARM Security Extensions.
> This is particulary useful for KVM runs.

"particularly"

> Default to disabled to match the behavior of KVM.

This is a change in behaviour, though, right? Is that OK?

> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  hw/arm/xlnx-zynqmp.c         | 3 +++
>  include/hw/arm/xlnx-zynqmp.h | 3 +++
>  2 files changed, 6 insertions(+)
>
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 4d504da..965a250 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -238,6 +238,8 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>          }
>          g_free(name);
>
> +        object_property_set_bool(OBJECT(&s->apu_cpu[i]),
> +                                 s->secure, "has_el3", NULL);
>          object_property_set_int(OBJECT(&s->apu_cpu[i]), GIC_BASE_ADDR,
>                                  "reset-cbar", &error_abort);
>          object_property_set_bool(OBJECT(&s->apu_cpu[i]), true, "realized",
> @@ -370,6 +372,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>
>  static Property xlnx_zynqmp_props[] = {
>      DEFINE_PROP_STRING("boot-cpu", XlnxZynqMPState, boot_cpu),
> +    DEFINE_PROP_BOOL("secure", XlnxZynqMPState, secure, false),
>      DEFINE_PROP_END_OF_LIST()
>  };
>
> diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
> index 2332596..38d4c8c 100644
> --- a/include/hw/arm/xlnx-zynqmp.h
> +++ b/include/hw/arm/xlnx-zynqmp.h
> @@ -84,6 +84,9 @@ typedef struct XlnxZynqMPState {
>
>      char *boot_cpu;
>      ARMCPU *boot_cpu_ptr;
> +
> +    /* Has the ARM Security extensions?  */
> +    bool secure;
>  }  XlnxZynqMPState;

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 1/5] xlnx-zynqmp: Add a secure prop to en/disable ARM Security Extensions

[Edgar E. Iglesias]

On Tue, May 24, 2016 at 05:30:54PM +0100, Peter Maydell wrote:
> On 19 May 2016 at 23:54, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > Add a secure prop to en/disable ARM Security Extensions.
> > This is particulary useful for KVM runs.
> 
> "particularly"
> 
> > Default to disabled to match the behavior of KVM.
> 
> This is a change in behaviour, though, right? Is that OK?

IMO it's OK. But I don't have a very strong opinion. We
didn't have EL3 in this machine to start with, it came in when
we enabled it on the a53. We don't have a big user-base that
really depends on either default I'd say.

Best regards,
Edgar


> 
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  hw/arm/xlnx-zynqmp.c         | 3 +++
> >  include/hw/arm/xlnx-zynqmp.h | 3 +++
> >  2 files changed, 6 insertions(+)
> >
> > diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> > index 4d504da..965a250 100644
> > --- a/hw/arm/xlnx-zynqmp.c
> > +++ b/hw/arm/xlnx-zynqmp.c
> > @@ -238,6 +238,8 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
> >          }
> >          g_free(name);
> >
> > +        object_property_set_bool(OBJECT(&s->apu_cpu[i]),
> > +                                 s->secure, "has_el3", NULL);
> >          object_property_set_int(OBJECT(&s->apu_cpu[i]), GIC_BASE_ADDR,
> >                                  "reset-cbar", &error_abort);
> >          object_property_set_bool(OBJECT(&s->apu_cpu[i]), true, "realized",
> > @@ -370,6 +372,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
> >
> >  static Property xlnx_zynqmp_props[] = {
> >      DEFINE_PROP_STRING("boot-cpu", XlnxZynqMPState, boot_cpu),
> > +    DEFINE_PROP_BOOL("secure", XlnxZynqMPState, secure, false),
> >      DEFINE_PROP_END_OF_LIST()
> >  };
> >
> > diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
> > index 2332596..38d4c8c 100644
> > --- a/include/hw/arm/xlnx-zynqmp.h
> > +++ b/include/hw/arm/xlnx-zynqmp.h
> > @@ -84,6 +84,9 @@ typedef struct XlnxZynqMPState {
> >
> >      char *boot_cpu;
> >      ARMCPU *boot_cpu_ptr;
> > +
> > +    /* Has the ARM Security extensions?  */
> > +    bool secure;
> >  }  XlnxZynqMPState;
> 
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH v1 1/5] xlnx-zynqmp: Add a secure prop to en/disable ARM Security Extensions

[Peter Maydell]

On 24 May 2016 at 17:57, Edgar E. Iglesias <edgar.iglesias@xilinx.com> wrote:
> On Tue, May 24, 2016 at 05:30:54PM +0100, Peter Maydell wrote:
>> On 19 May 2016 at 23:54, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
>> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>> >
>> > Add a secure prop to en/disable ARM Security Extensions.
>> > This is particulary useful for KVM runs.
>>
>> "particularly"
>>
>> > Default to disabled to match the behavior of KVM.
>>
>> This is a change in behaviour, though, right? Is that OK?
>
> IMO it's OK. But I don't have a very strong opinion. We
> didn't have EL3 in this machine to start with, it came in when
> we enabled it on the a53. We don't have a big user-base that
> really depends on either default I'd say.

I'll defer to your preference, but we should mention in the
commit message that we changed the behaviour (and perhaps
also that it was accidental that EL3 got enabled).

thanks
-- PMM

[Qemu-devel] [PATCH v1 2/5] xlnx-zynqmp: Break out RPU setup into a separate function

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Break out RPU setup into a separate function. This is in
preparation for making the RPU optional.

No functional change.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 hw/arm/xlnx-zynqmp.c | 53 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 31 insertions(+), 22 deletions(-)

diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 965a250..250ecc4 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -83,6 +83,36 @@ static inline int arm_gic_ppi_index(int cpu_nr, int ppi_index)
     return GIC_NUM_SPI_INTR + cpu_nr * GIC_INTERNAL + ppi_index;
 }
 
+static void xlnx_zynqmp_setup_rpu(XlnxZynqMPState *s, const char *boot_cpu,
+                                  Error **errp)
+{
+    Error *err = NULL;
+    int i;
+
+    for (i = 0; i < XLNX_ZYNQMP_NUM_RPU_CPUS; i++) {
+        char *name;
+
+        name = object_get_canonical_path_component(OBJECT(&s->rpu_cpu[i]));
+        if (strcmp(name, boot_cpu)) {
+            /* Secondary CPUs start in PSCI powered-down state */
+            object_property_set_bool(OBJECT(&s->rpu_cpu[i]), true,
+                                     "start-powered-off", &error_abort);
+        } else {
+            s->boot_cpu_ptr = &s->rpu_cpu[i];
+        }
+        g_free(name);
+
+        object_property_set_bool(OBJECT(&s->rpu_cpu[i]), true, "reset-hivecs",
+                                 &error_abort);
+        object_property_set_bool(OBJECT(&s->rpu_cpu[i]), true, "realized",
+                                 &err);
+        if (err) {
+            error_propagate(errp, err);
+            return;
+        }
+    }
+}
+
 static void xlnx_zynqmp_init(Object *obj)
 {
     XlnxZynqMPState *s = XLNX_ZYNQMP(obj);
@@ -260,28 +290,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
         qdev_connect_gpio_out(DEVICE(&s->apu_cpu[i]), 1, irq);
     }
 
-    for (i = 0; i < XLNX_ZYNQMP_NUM_RPU_CPUS; i++) {
-        char *name;
-
-        name = object_get_canonical_path_component(OBJECT(&s->rpu_cpu[i]));
-        if (strcmp(name, boot_cpu)) {
-            /* Secondary CPUs start in PSCI powered-down state */
-            object_property_set_bool(OBJECT(&s->rpu_cpu[i]), true,
-                                     "start-powered-off", &error_abort);
-        } else {
-            s->boot_cpu_ptr = &s->rpu_cpu[i];
-        }
-        g_free(name);
-
-        object_property_set_bool(OBJECT(&s->rpu_cpu[i]), true, "reset-hivecs",
-                                 &error_abort);
-        object_property_set_bool(OBJECT(&s->rpu_cpu[i]), true, "realized",
-                                 &err);
-        if (err) {
-            error_propagate(errp, err);
-            return;
-        }
-    }
+    xlnx_zynqmp_setup_rpu(s, boot_cpu, errp);
 
     if (!s->boot_cpu_ptr) {
         error_setg(errp, "ZynqMP Boot cpu %s not found", boot_cpu);
-- 
2.5.0

Re: [Qemu-devel] [PATCH v1 2/5] xlnx-zynqmp: Break out RPU setup into a separate function

[Peter Maydell]

On 19 May 2016 at 23:54, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Break out RPU setup into a separate function. This is in
> preparation for making the RPU optional.
>
> No functional change.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  hw/arm/xlnx-zynqmp.c | 53 ++++++++++++++++++++++++++++++----------------------
>  1 file changed, 31 insertions(+), 22 deletions(-)
>
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 965a250..250ecc4 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -83,6 +83,36 @@ static inline int arm_gic_ppi_index(int cpu_nr, int ppi_index)
>      return GIC_NUM_SPI_INTR + cpu_nr * GIC_INTERNAL + ppi_index;
>  }
>
> +static void xlnx_zynqmp_setup_rpu(XlnxZynqMPState *s, const char *boot_cpu,
> +                                  Error **errp)
> +{
> +    Error *err = NULL;
> +    int i;
> +
> +    for (i = 0; i < XLNX_ZYNQMP_NUM_RPU_CPUS; i++) {
> +        char *name;
> +
> +        name = object_get_canonical_path_component(OBJECT(&s->rpu_cpu[i]));
> +        if (strcmp(name, boot_cpu)) {
> +            /* Secondary CPUs start in PSCI powered-down state */
> +            object_property_set_bool(OBJECT(&s->rpu_cpu[i]), true,
> +                                     "start-powered-off", &error_abort);
> +        } else {
> +            s->boot_cpu_ptr = &s->rpu_cpu[i];
> +        }
> +        g_free(name);
> +
> +        object_property_set_bool(OBJECT(&s->rpu_cpu[i]), true, "reset-hivecs",
> +                                 &error_abort);
> +        object_property_set_bool(OBJECT(&s->rpu_cpu[i]), true, "realized",
> +                                 &err);
> +        if (err) {
> +            error_propagate(errp, err);
> +            return;

This used to return from xlnz_zynqmp_init, but now it only returns
from this setup function, and xlnx_zynqmp_init will blithely continue.

> +        }
> +    }
> +}

thanks
-- PMM

[Qemu-devel] [PATCH v1 3/5] xlnx-zynqmp: Make the RPU subsystem optional

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

The way we currently model the RPU subsystem is of quite
limited use. In addition to that, it causes problems for
KVM and for GDB debugging.

Make the RPU optional by adding a has_rpu property and
default to having it disabled.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 hw/arm/xlnx-zynqmp.c         | 50 +++++++++++++++++++++++++++++++++++++-------
 include/hw/arm/xlnx-zynqmp.h |  2 ++
 2 files changed, 44 insertions(+), 8 deletions(-)

diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 250ecc4..c180206 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -83,6 +83,40 @@ static inline int arm_gic_ppi_index(int cpu_nr, int ppi_index)
     return GIC_NUM_SPI_INTR + cpu_nr * GIC_INTERNAL + ppi_index;
 }
 
+static bool xlnx_zynqmp_get_has_rpu(Object *obj, Error **errp)
+{
+    XlnxZynqMPState *s = XLNX_ZYNQMP(obj);
+
+    return s->has_rpu;
+}
+
+static void xlnx_zynqmp_set_has_rpu(Object *obj, bool value, Error **errp)
+{
+    XlnxZynqMPState *s = XLNX_ZYNQMP(obj);
+    int i;
+
+    if (s->has_rpu == value) {
+        /* Nothing to do.  */
+        return;
+    }
+
+    /* We don't support clearing the flag.  */
+    if (s->has_rpu) {
+        error_setg(errp, "has_rpu is already set to %u",
+                   s->has_rpu);
+        return;
+    }
+
+    /* Create the Cortex R5s.  */
+    for (i = 0; i < XLNX_ZYNQMP_NUM_RPU_CPUS; i++) {
+        object_initialize(&s->rpu_cpu[i], sizeof(s->rpu_cpu[i]),
+                          "cortex-r5-" TYPE_ARM_CPU);
+        object_property_add_child(obj, "rpu-cpu[*]", OBJECT(&s->rpu_cpu[i]),
+                                  &error_abort);
+    }
+    s->has_rpu = value;
+}
+
 static void xlnx_zynqmp_setup_rpu(XlnxZynqMPState *s, const char *boot_cpu,
                                   Error **errp)
 {
@@ -118,6 +152,11 @@ static void xlnx_zynqmp_init(Object *obj)
     XlnxZynqMPState *s = XLNX_ZYNQMP(obj);
     int i;
 
+    object_property_add_bool(obj, "has_rpu",
+                             xlnx_zynqmp_get_has_rpu,
+                             xlnx_zynqmp_set_has_rpu,
+                             &error_abort);
+
     for (i = 0; i < XLNX_ZYNQMP_NUM_APU_CPUS; i++) {
         object_initialize(&s->apu_cpu[i], sizeof(s->apu_cpu[i]),
                           "cortex-a53-" TYPE_ARM_CPU);
@@ -125,13 +164,6 @@ static void xlnx_zynqmp_init(Object *obj)
                                   &error_abort);
     }
 
-    for (i = 0; i < XLNX_ZYNQMP_NUM_RPU_CPUS; i++) {
-        object_initialize(&s->rpu_cpu[i], sizeof(s->rpu_cpu[i]),
-                          "cortex-r5-" TYPE_ARM_CPU);
-        object_property_add_child(obj, "rpu-cpu[*]", OBJECT(&s->rpu_cpu[i]),
-                                  &error_abort);
-    }
-
     object_property_add_link(obj, "ddr-ram", TYPE_MEMORY_REGION,
                              (Object **)&s->ddr_ram,
                              qdev_prop_allow_set_link_before_realize,
@@ -290,7 +322,9 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
         qdev_connect_gpio_out(DEVICE(&s->apu_cpu[i]), 1, irq);
     }
 
-    xlnx_zynqmp_setup_rpu(s, boot_cpu, errp);
+    if (s->has_rpu) {
+        xlnx_zynqmp_setup_rpu(s, boot_cpu, errp);
+    }
 
     if (!s->boot_cpu_ptr) {
         error_setg(errp, "ZynqMP Boot cpu %s not found", boot_cpu);
diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
index 38d4c8c..68f6eb0 100644
--- a/include/hw/arm/xlnx-zynqmp.h
+++ b/include/hw/arm/xlnx-zynqmp.h
@@ -87,6 +87,8 @@ typedef struct XlnxZynqMPState {
 
     /* Has the ARM Security extensions?  */
     bool secure;
+    /* Has the RPU subsystem?  */
+    bool has_rpu;
 }  XlnxZynqMPState;
 
 #define XLNX_ZYNQMP_H
-- 
2.5.0

Re: [Qemu-devel] [PATCH v1 3/5] xlnx-zynqmp: Make the RPU subsystem optional

[Peter Maydell]

On 19 May 2016 at 23:54, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> The way we currently model the RPU subsystem is of quite
> limited use. In addition to that, it causes problems for
> KVM and for GDB debugging.
>
> Make the RPU optional by adding a has_rpu property and
> default to having it disabled.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  hw/arm/xlnx-zynqmp.c         | 50 +++++++++++++++++++++++++++++++++++++-------
>  include/hw/arm/xlnx-zynqmp.h |  2 ++
>  2 files changed, 44 insertions(+), 8 deletions(-)
>
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 250ecc4..c180206 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -83,6 +83,40 @@ static inline int arm_gic_ppi_index(int cpu_nr, int ppi_index)
>      return GIC_NUM_SPI_INTR + cpu_nr * GIC_INTERNAL + ppi_index;
>  }
>
> +static bool xlnx_zynqmp_get_has_rpu(Object *obj, Error **errp)
> +{
> +    XlnxZynqMPState *s = XLNX_ZYNQMP(obj);
> +
> +    return s->has_rpu;
> +}
> +
> +static void xlnx_zynqmp_set_has_rpu(Object *obj, bool value, Error **errp)
> +{
> +    XlnxZynqMPState *s = XLNX_ZYNQMP(obj);
> +    int i;
> +
> +    if (s->has_rpu == value) {
> +        /* Nothing to do.  */
> +        return;
> +    }
> +
> +    /* We don't support clearing the flag.  */
> +    if (s->has_rpu) {
> +        error_setg(errp, "has_rpu is already set to %u",
> +                   s->has_rpu);
> +        return;
> +    }
> +
> +    /* Create the Cortex R5s.  */
> +    for (i = 0; i < XLNX_ZYNQMP_NUM_RPU_CPUS; i++) {
> +        object_initialize(&s->rpu_cpu[i], sizeof(s->rpu_cpu[i]),
> +                          "cortex-r5-" TYPE_ARM_CPU);
> +        object_property_add_child(obj, "rpu-cpu[*]", OBJECT(&s->rpu_cpu[i]),
> +                                  &error_abort);
> +    }

Do we have to create them in the set function so we can
set properties before realize, or could this be deferred
to realize time?


thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 3/5] xlnx-zynqmp: Make the RPU subsystem optional

[Edgar E. Iglesias]

On Tue, May 24, 2016 at 05:26:54PM +0100, Peter Maydell wrote:
> On 19 May 2016 at 23:54, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > The way we currently model the RPU subsystem is of quite
> > limited use. In addition to that, it causes problems for
> > KVM and for GDB debugging.
> >
> > Make the RPU optional by adding a has_rpu property and
> > default to having it disabled.
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  hw/arm/xlnx-zynqmp.c         | 50 +++++++++++++++++++++++++++++++++++++-------
> >  include/hw/arm/xlnx-zynqmp.h |  2 ++
> >  2 files changed, 44 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> > index 250ecc4..c180206 100644
> > --- a/hw/arm/xlnx-zynqmp.c
> > +++ b/hw/arm/xlnx-zynqmp.c
> > @@ -83,6 +83,40 @@ static inline int arm_gic_ppi_index(int cpu_nr, int ppi_index)
> >      return GIC_NUM_SPI_INTR + cpu_nr * GIC_INTERNAL + ppi_index;
> >  }
> >
> > +static bool xlnx_zynqmp_get_has_rpu(Object *obj, Error **errp)
> > +{
> > +    XlnxZynqMPState *s = XLNX_ZYNQMP(obj);
> > +
> > +    return s->has_rpu;
> > +}
> > +
> > +static void xlnx_zynqmp_set_has_rpu(Object *obj, bool value, Error **errp)
> > +{
> > +    XlnxZynqMPState *s = XLNX_ZYNQMP(obj);
> > +    int i;
> > +
> > +    if (s->has_rpu == value) {
> > +        /* Nothing to do.  */
> > +        return;
> > +    }
> > +
> > +    /* We don't support clearing the flag.  */
> > +    if (s->has_rpu) {
> > +        error_setg(errp, "has_rpu is already set to %u",
> > +                   s->has_rpu);
> > +        return;
> > +    }
> > +
> > +    /* Create the Cortex R5s.  */
> > +    for (i = 0; i < XLNX_ZYNQMP_NUM_RPU_CPUS; i++) {
> > +        object_initialize(&s->rpu_cpu[i], sizeof(s->rpu_cpu[i]),
> > +                          "cortex-r5-" TYPE_ARM_CPU);
> > +        object_property_add_child(obj, "rpu-cpu[*]", OBJECT(&s->rpu_cpu[i]),
> > +                                  &error_abort);
> > +    }
> 
> Do we have to create them in the set function so we can
> set properties before realize, or could this be deferred
> to realize time?

Yes, I thought it was recommended to avoid object creation
in realize. But creating the PRU in realize works too.

Cheers,
Edgar

Re: [Qemu-devel] [PATCH v1 3/5] xlnx-zynqmp: Make the RPU subsystem optional

[Peter Maydell]

On 24 May 2016 at 17:32, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> On Tue, May 24, 2016 at 05:26:54PM +0100, Peter Maydell wrote:
>> Do we have to create them in the set function so we can
>> set properties before realize, or could this be deferred
>> to realize time?
>
> Yes, I thought it was recommended to avoid object creation
> in realize. But creating the PRU in realize works too.

Well, it is recommended, but only in the sense of "prefer to
do it in instance init" :-) I think we prefer not to have
complicated behaviours happening on property-set because it's
a bit unexpected (but sometimes it's necessary if later
property-sets wouldn't work otherwise).

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 3/5] xlnx-zynqmp: Make the RPU subsystem optional

[Edgar E. Iglesias]

On Tue, May 24, 2016 at 05:37:14PM +0100, Peter Maydell wrote:
> On 24 May 2016 at 17:32, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > On Tue, May 24, 2016 at 05:26:54PM +0100, Peter Maydell wrote:
> >> Do we have to create them in the set function so we can
> >> set properties before realize, or could this be deferred
> >> to realize time?
> >
> > Yes, I thought it was recommended to avoid object creation
> > in realize. But creating the PRU in realize works too.
> 
> Well, it is recommended, but only in the sense of "prefer to
> do it in instance init" :-) I think we prefer not to have
> complicated behaviours happening on property-set because it's
> a bit unexpected (but sometimes it's necessary if later
> property-sets wouldn't work otherwise).

Allright, I'm happy to change it. It definitely simplifies the code.

Cheers,
Edgar

[Qemu-devel] [PATCH v1 4/5] xlnx-zynqmp: Delay realization of GIC until post CPU realization

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Delay the realization of the GIC until after CPUs are
realized. This is needed for KVM as the in-kernel GIC
model will fail if it is realized with no available CPUs.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 hw/arm/xlnx-zynqmp.c | 56 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 31 insertions(+), 25 deletions(-)

diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index c180206..0648028 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -258,33 +258,9 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
     qdev_prop_set_uint32(DEVICE(&s->gic), "num-irq", GIC_NUM_SPI_INTR + 32);
     qdev_prop_set_uint32(DEVICE(&s->gic), "revision", 2);
     qdev_prop_set_uint32(DEVICE(&s->gic), "num-cpu", XLNX_ZYNQMP_NUM_APU_CPUS);
-    object_property_set_bool(OBJECT(&s->gic), true, "realized", &err);
-    if (err) {
-        error_propagate(errp, err);
-        return;
-    }
-    assert(ARRAY_SIZE(xlnx_zynqmp_gic_regions) == XLNX_ZYNQMP_GIC_REGIONS);
-    for (i = 0; i < XLNX_ZYNQMP_GIC_REGIONS; i++) {
-        SysBusDevice *gic = SYS_BUS_DEVICE(&s->gic);
-        const XlnxZynqMPGICRegion *r = &xlnx_zynqmp_gic_regions[i];
-        MemoryRegion *mr = sysbus_mmio_get_region(gic, r->region_index);
-        uint32_t addr = r->address;
-        int j;
-
-        sysbus_mmio_map(gic, r->region_index, addr);
-
-        for (j = 0; j < XLNX_ZYNQMP_GIC_ALIASES; j++) {
-            MemoryRegion *alias = &s->gic_mr[i][j];
-
-            addr += XLNX_ZYNQMP_GIC_REGION_SIZE;
-            memory_region_init_alias(alias, OBJECT(s), "zynqmp-gic-alias", mr,
-                                     0, XLNX_ZYNQMP_GIC_REGION_SIZE);
-            memory_region_add_subregion(system_memory, addr, alias);
-        }
-    }
 
+    /* Realize APUs before realizing the GIC. KVM requires this.  */
     for (i = 0; i < XLNX_ZYNQMP_NUM_APU_CPUS; i++) {
-        qemu_irq irq;
         char *name;
 
         object_property_set_int(OBJECT(&s->apu_cpu[i]), QEMU_PSCI_CONDUIT_SMC,
@@ -310,6 +286,36 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
             error_propagate(errp, err);
             return;
         }
+    }
+
+    object_property_set_bool(OBJECT(&s->gic), true, "realized", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+
+    assert(ARRAY_SIZE(xlnx_zynqmp_gic_regions) == XLNX_ZYNQMP_GIC_REGIONS);
+    for (i = 0; i < XLNX_ZYNQMP_GIC_REGIONS; i++) {
+        SysBusDevice *gic = SYS_BUS_DEVICE(&s->gic);
+        const XlnxZynqMPGICRegion *r = &xlnx_zynqmp_gic_regions[i];
+        MemoryRegion *mr = sysbus_mmio_get_region(gic, r->region_index);
+        uint32_t addr = r->address;
+        int j;
+
+        sysbus_mmio_map(gic, r->region_index, addr);
+
+        for (j = 0; j < XLNX_ZYNQMP_GIC_ALIASES; j++) {
+            MemoryRegion *alias = &s->gic_mr[i][j];
+
+            addr += XLNX_ZYNQMP_GIC_REGION_SIZE;
+            memory_region_init_alias(alias, OBJECT(s), "zynqmp-gic-alias", mr,
+                                     0, XLNX_ZYNQMP_GIC_REGION_SIZE);
+            memory_region_add_subregion(system_memory, addr, alias);
+        }
+    }
+
+    for (i = 0; i < XLNX_ZYNQMP_NUM_APU_CPUS; i++) {
+        qemu_irq irq;
 
         sysbus_connect_irq(SYS_BUS_DEVICE(&s->gic), i,
                            qdev_get_gpio_in(DEVICE(&s->apu_cpu[i]),
-- 
2.5.0

Re: [Qemu-devel] [PATCH v1 4/5] xlnx-zynqmp: Delay realization of GIC until post CPU realization

[Peter Maydell]

On 19 May 2016 at 23:54, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Delay the realization of the GIC until after CPUs are
> realized. This is needed for KVM as the in-kernel GIC
> model will fail if it is realized with no available CPUs.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

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

thanks
-- PMM

[Qemu-devel] [PATCH v1 5/5] xlnx-zynqmp: Use the in kernel GIC model for KVM runs

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Use the in kernel GIC model when running with KVM enabled.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 hw/arm/xlnx-zynqmp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 0648028..c3878da 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -22,6 +22,8 @@
 #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"
 
 #define GIC_NUM_SPI_INTR 160
 
@@ -169,7 +171,7 @@ static void xlnx_zynqmp_init(Object *obj)
                              qdev_prop_allow_set_link_before_realize,
                              OBJ_PROP_LINK_UNREF_ON_RELEASE, &error_abort);
 
-    object_initialize(&s->gic, sizeof(s->gic), TYPE_ARM_GIC);
+    object_initialize(&s->gic, sizeof(s->gic), gic_class_name());
     qdev_set_parent_bus(DEVICE(&s->gic), sysbus_get_default());
 
     for (i = 0; i < XLNX_ZYNQMP_NUM_GEMS; i++) {
-- 
2.5.0

Re: [Qemu-devel] [PATCH v1 5/5] xlnx-zynqmp: Use the in kernel GIC model for KVM runs

[Peter Maydell]

On 19 May 2016 at 23:54, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Use the in kernel GIC model when running with KVM enabled.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  hw/arm/xlnx-zynqmp.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>

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

thanks
-- PMM

[Qemu-devel] [PATCH v1 0/5] xlnx-zynqmp: Fix various issues with KVM runs

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

This series fixes a bunch of issues with KVM runs using the ZynqMP
machine.

When using KVM, we don't have support for TrustZone, the RPU subsystem
with Cortex R5s and we also need to use the in-kernel GIC model.

This adds a set of properties to the ZynqMP allowing the creation
of the KVM friendly machine.

I've opted to default the ZynqMP to the KVM friendly machine for
consistensy between KVM and TCG and also because some of the options
are currently of limited use (i.e the RPU) or were actually disabled to
start with (e.g TrustZone sneaked in as we enabled EL3 on the A53s).
I'm OK either way though, so if people think differently we can change
the defaults.

Comments welcome!

Cheers,
Edgar

Edgar E. Iglesias (5):
  xlnx-zynqmp: Add a secure prop to en/disable ARM Security Extensions
  xlnx-zynqmp: Break out RPU setup into a separate function
  xlnx-zynqmp: Make the RPU subsystem optional
  xlnx-zynqmp: Delay realization of GIC until post CPU realization
  xlnx-zynqmp: Use the in kernel GIC model for KVM runs

 hw/arm/xlnx-zynqmp.c         | 156 +++++++++++++++++++++++++++++--------------
 include/hw/arm/xlnx-zynqmp.h |   5 ++
 2 files changed, 110 insertions(+), 51 deletions(-)

-- 
2.5.0