Re: [PATCH v2 2/2] hw/sparc/leon3: add second uart with extended interrupt usage
On Sat, Sep 21, 2024 at 6:43 PM Nikita Shushura wrote:
>
> Signed-off-by: Nikita Shushura
Mostly OK just a few comments.
> ---
> hw/sparc/leon3.c | 63 +++-
> 1 file changed, 46 insertions(+), 17 deletions(-)
>
> diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
> index 6aaa04cb19..c559854e5e 100644
> --- a/hw/sparc/leon3.c
> +++ b/hw/sparc/leon3.c
> @@ -54,10 +54,14 @@
> #define LEON3_PROM_OFFSET(0x)
> #define LEON3_RAM_OFFSET (0x4000)
>
> -#define MAX_CPUS 4
> +#define MAX_CPUS (4)
> +#define LEON3_EIRQ (12)
>
> -#define LEON3_UART_OFFSET (0x8100)
> -#define LEON3_UART_IRQ (3)
> +#define LEON3_UART0_OFFSET (0x8100)
> +#define LEON3_UART0_IRQ (2)
> +
> +#define LEON3_UART1_OFFSET (0x80100100)
> +#define LEON3_UART1_IRQ (17)
>
> #define LEON3_IRQMP_OFFSET (0x8200)
>
> @@ -65,7 +69,8 @@
> #define LEON3_TIMER_IRQ(6)
> #define LEON3_TIMER_COUNT (2)
>
> -#define LEON3_APB_PNP_OFFSET (0x800FF000)
> +#define LEON3_APB1_PNP_OFFSET (0x800FF000)
> +#define LEON3_APB2_PNP_OFFSET (0x801FF000)
> #define LEON3_AHB_PNP_OFFSET (0xF000)
>
> typedef struct ResetData {
> @@ -122,7 +127,8 @@ static void write_bootloader(void *ptr, hwaddr
> kernel_addr)
>
> /* Initialize the UARTs*/
> /* *UART_CONTROL = UART_RECEIVE_ENABLE | UART_TRANSMIT_ENABLE; */
> -p = gen_store_u32(p, 0x8108, 3);
> +p = gen_store_u32(p, LEON3_UART0_OFFSET + 0x8, 3);
> +p = gen_store_u32(p, LEON3_UART1_OFFSET + 0x8, 3);
>
> /* Initialize the TIMER 0 */
> /* *GPTIMER_SCALER_RELOAD = 40 - 1;*/
> @@ -271,7 +277,8 @@ static void leon3_generic_hw_init(MachineState *machine)
> DeviceState *dev, *irqmpdev;
> int i;
> AHBPnp *ahb_pnp;
> -APBPnp *apb_pnp;
> +APBPnp *apb1_pnp;
> +APBPnp *apb2_pnp;
>
> reset_info = g_malloc0(sizeof(ResetData));
>
> @@ -298,10 +305,19 @@ static void leon3_generic_hw_init(MachineState *machine)
> GRLIB_LEON3_DEV, GRLIB_AHB_MASTER,
> GRLIB_CPU_AREA);
>
> -apb_pnp = GRLIB_APB_PNP(qdev_new(TYPE_GRLIB_APB_PNP));
> -sysbus_realize_and_unref(SYS_BUS_DEVICE(apb_pnp), &error_fatal);
> -sysbus_mmio_map(SYS_BUS_DEVICE(apb_pnp), 0, LEON3_APB_PNP_OFFSET);
> -grlib_ahb_pnp_add_entry(ahb_pnp, LEON3_APB_PNP_OFFSET, 0xFFF,
> +/* Initialize APB1 */
> +apb1_pnp = GRLIB_APB_PNP(qdev_new(TYPE_GRLIB_APB_PNP));
> +sysbus_realize_and_unref(SYS_BUS_DEVICE(apb1_pnp), &error_fatal);
> +sysbus_mmio_map(SYS_BUS_DEVICE(apb1_pnp), 0, LEON3_APB1_PNP_OFFSET);
> +grlib_ahb_pnp_add_entry(ahb_pnp, LEON3_APB1_PNP_OFFSET, 0xFFF,
> +GRLIB_VENDOR_GAISLER, GRLIB_APBMST_DEV,
> +GRLIB_AHB_SLAVE, GRLIB_AHBMEM_AREA);
> +
> +/* Initialize APB2 */
> +apb2_pnp = GRLIB_APB_PNP(qdev_new(TYPE_GRLIB_APB_PNP));
> +sysbus_realize_and_unref(SYS_BUS_DEVICE(apb2_pnp), &error_fatal);
> +sysbus_mmio_map(SYS_BUS_DEVICE(apb2_pnp), 0, LEON3_APB2_PNP_OFFSET);
> +grlib_ahb_pnp_add_entry(ahb_pnp, LEON3_APB2_PNP_OFFSET, 0xFFF,
> GRLIB_VENDOR_GAISLER, GRLIB_APBMST_DEV,
> GRLIB_AHB_SLAVE, GRLIB_AHBMEM_AREA);
You could embed that within a loop.
> @@ -309,6 +325,8 @@ static void leon3_generic_hw_init(MachineState *machine)
> irqmpdev = qdev_new(TYPE_GRLIB_IRQMP);
> object_property_set_int(OBJECT(irqmpdev), "ncpus", machine->smp.cpus,
> &error_fatal);
> +/*object_property_set_int(OBJECT(irqmpdev), "eirq", LEON3_EIRQ,*/
> +/*&error_fatal);*/
Why is this commented ?
> sysbus_realize_and_unref(SYS_BUS_DEVICE(irqmpdev), &error_fatal);
>
> for (i = 0; i < machine->smp.cpus; i++) {
> @@ -325,7 +343,7 @@ static void leon3_generic_hw_init(MachineState *machine)
> }
>
> sysbus_mmio_map(SYS_BUS_DEVICE(irqmpdev), 0, LEON3_IRQMP_OFFSET);
> -grlib_apb_pnp_add_entry(apb_pnp, LEON3_IRQMP_OFFSET, 0xFFF,
> +grlib_apb_pnp_add_entry(apb1_pnp, LEON3_IRQMP_OFFSET, 0xFFF,
> GRLIB_VENDOR_GAISLER, GRLIB_IRQMP_DEV,
> 2, 0, GRLIB_APBIO_AREA);
>
> @@ -417,20 +435,31 @@ static void leon3_generic_hw_init(MachineState *machine)
> qdev_get_gpio_in(irqmpdev, LEON3_TIMER_IRQ + i));
> }
>
> -grlib_apb_pnp_add_entry(apb_pnp, LEON3_TIMER_OFFSET, 0xFFF,
> +grlib_apb_pnp_add_entry(apb1_pnp, LEON3_TIMER_OFFSET, 0xFFF,
> GRLIB_VENDOR_GAISLER, GRLIB_GPTIMER_DEV,
> 0, LEON3_TIMER_IRQ, GRLIB_APBIO_AREA);
>
> -/* Allocate uart */
> +/* Allocate UART0 */
> dev = qdev_new(TYPE_GRLIB_APB_UART);
> qdev_prop_set_chr(dev, "chrdev", serial_hd(0));
> sysbus_realize_and_un
Re: [PATCH v2 1/2] hw/intc/grlib_irqmp: add support for extended interrupts
On Sat, Sep 21, 2024 at 6:43 PM Nikita Shushura wrote:
>
> Signed-off-by: Nikita Shushura
Additionally to inlined comments:
- there are a few "extended (not supported)" you can now remove.
- I think the extended part in "grlib_irqmp_write" is still wrong,
the extended register being read-only.
> ---
> hw/intc/grlib_irqmp.c | 69 +++
> 1 file changed, 50 insertions(+), 19 deletions(-)
>
> diff --git a/hw/intc/grlib_irqmp.c b/hw/intc/grlib_irqmp.c
> index 37ac63fd80..e9414c054a 100644
> --- a/hw/intc/grlib_irqmp.c
> +++ b/hw/intc/grlib_irqmp.c
> @@ -1,8 +1,6 @@
> /*
> * QEMU GRLIB IRQMP Emulator
> *
> - * (Extended interrupt not supported)
> - *
> * SPDX-License-Identifier: MIT
> *
> * Copyright (c) 2010-2024 AdaCore
> @@ -38,25 +36,29 @@
> #include "qemu/module.h"
> #include "qom/object.h"
>
> -#define IRQMP_MAX_CPU 16
> -#define IRQMP_REG_SIZE 256 /* Size of memory mapped registers */
> +#define IRQMP_MAX_CPU (16)
> +#define IRQMP_REG_SIZE (256)/* Size of memory mapped registers */
>
> /* Memory mapped register offsets */
> -#define LEVEL_OFFSET 0x00
> -#define PENDING_OFFSET 0x04
> -#define FORCE0_OFFSET0x08
> -#define CLEAR_OFFSET 0x0C
> -#define MP_STATUS_OFFSET 0x10
> -#define BROADCAST_OFFSET 0x14
> -#define MASK_OFFSET 0x40
> -#define FORCE_OFFSET 0x80
> -#define EXTENDED_OFFSET 0xC0
> +#define LEVEL_OFFSET (0x00)
> +#define PENDING_OFFSET (0x04)
> +#define FORCE0_OFFSET(0x08)
> +#define CLEAR_OFFSET (0x0C)
> +#define MP_STATUS_OFFSET (0x10)
> +#define BROADCAST_OFFSET (0x14)
> +#define MASK_OFFSET (0x40)
> +#define FORCE_OFFSET (0x80)
> +#define EXTENDED_OFFSET (0xC0)
>
> /* Multiprocessor Status Register */
> #define MP_STATUS_CPU_STATUS_MASK ((1 << IRQMP_MAX_CPU)-2)
> -#define MP_STATUS_NCPU_SHIFT 28
> +#define MP_STATUS_NCPU_SHIFT (28)
> +#define MP_STATUS_EIRQ_OFFSET (16)
> +
> +#define MAX_PILS_STD (16)
> +#define MAX_PILS_EXT (32)
>
> -#define MAX_PILS 16
> +#define DEFAULT_EIRQ (12)
>
> OBJECT_DECLARE_SIMPLE_TYPE(IRQMP, GRLIB_IRQMP)
>
> @@ -68,6 +70,7 @@ struct IRQMP {
> MemoryRegion iomem;
>
> unsigned int ncpus;
> +unsigned int eirq;
> IRQMPState *state;
> qemu_irq start_signal[IRQMP_MAX_CPU];
> qemu_irq irq[IRQMP_MAX_CPU];
> @@ -89,13 +92,26 @@ struct IRQMPState {
>
> static void grlib_irqmp_check_irqs(IRQMPState *state)
> {
> -int i;
> +int i, j;
>
> assert(state != NULL);
> assert(state->parent != NULL);
>
> for (i = 0; i < state->parent->ncpus; i++) {
> uint32_t pend = (state->pending | state->force[i]) & state->mask[i];
> +
> +/*
> + * Checks is pending interrupt extended.
s/is/if
> + * If so, sets pending to EIRQ and acknowledges
> + * extended interrupt
> + */
> +for (j = MAX_PILS_STD; j < MAX_PILS_EXT; j++) {
> +if ((pend & (1 << j)) != 0) {
> +pend = (1 << state->parent->eirq);
> +state->extended[i] = (j & 0x);
You're writing 1 bit too far. It should be ((j>>1) & 0xf).
Moreover, what's happening when two extended interrupts are both
pending ? Here, only the last one is taken into account
> +}
> +}
> +
> uint32_t level0 = pend & ~state->level;
> uint32_t level1 = pend & state->level;
>
> @@ -110,6 +126,10 @@ static void grlib_irqmp_check_irqs(IRQMPState *state)
> static void grlib_irqmp_ack_mask(IRQMPState *state, unsigned int cpu,
> uint32_t mask)
> {
> +if ((mask & (1 << state->parent->eirq)) != 0) {
> +mask |= (1 << state->extended[cpu]);
> +}
> +
> /* Clear registers */
> state->pending &= ~mask;
> state->force[cpu] &= ~mask;
> @@ -144,7 +164,6 @@ static void grlib_irqmp_set_irq(void *opaque, int irq,
> int level)
> assert(s != NULL);
> assert(s->parent != NULL);
>
> -
> if (level) {
> trace_grlib_irqmp_set_irq(irq);
>
> @@ -278,6 +297,9 @@ static void grlib_irqmp_write(void *opaque, hwaddr addr,
> state->mpstatus &= ~(1 << i);
> }
> }
> +
> +/* Writing EIRQ number */
> +state->mpstatus |= (state->parent->eirq << MP_STATUS_EIRQ_OFFSET);
> return;
>
> case BROADCAST_OFFSET:
> @@ -345,7 +367,8 @@ static void grlib_irqmp_reset(DeviceState *d)
> memset(irqmp->state, 0, sizeof *irqmp->state);
> irqmp->state->parent = irqmp;
> irqmp->state->mpstatus = ((irqmp->ncpus - 1) << MP_STATUS_NCPU_SHIFT) |
> -((1 << irqmp->ncpus) - 2);
> +((1 << irqmp->ncpus) - 2) |
> +(irqmp->eirq << MP_STATUS_EIRQ_OFFSET);
> }
>
> static void grlib_irqmp_realize(DeviceState *dev, Error **errp)
> @@ -359,7 +382,14 @@ static void grlib_irqmp_realize(DeviceState *dev, Error
> **errp)
> return;
> }
>
> -qdev_init_gpio_in(dev, grlib_irqmp_set_irq, MAX_PILS
Re: [PATCH] hw/ppc: fix decrementer with BookE timers
Hi Cédric,
On Tue, Aug 27, 2024 at 7:40 PM Cédric Le Goater wrote:
>
> Hello Clément,
>
> On 7/15/24 10:46, Clément Chigot wrote:
> > The BookE decrementer stops at 0, meaning that it won't decremented
> > towards "negative" values.
> > However, the current logic is inverted: decr is updated solely when
> > the resulting value would be negative.
>
> How did you hit the issue ? which machine ? I didn't see any error
> when booting Linux 6.6.3 on mpc8544ds, e500mc, e5500 and e6500.
>
> > Signed-off-by: Clément Chigot
> > Fixed: 8e0a5ac87800 ("hw/ppc: Avoid decrementer rounding errors")
>
> LGTM,
>
> Reviewed-by: Cédric Le Goater
Unless I'm wrong this patch has not been queued yet. Is there any
reason for this ?
I just want to make sure it hasn't been forgotten.
Thanks,
Clément
> We have some automated tests with the ppce500 machine which it would be
> interesting to extend to have a better coverage of booke.
>
> Thanks,
>
> C.
>
>
>
> > ---
> > hw/ppc/ppc.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> > index e6fa5580c0..9fc85c7de0 100644
> > --- a/hw/ppc/ppc.c
> > +++ b/hw/ppc/ppc.c
> > @@ -729,7 +729,9 @@ static inline int64_t __cpu_ppc_load_decr(CPUPPCState
> > *env, int64_t now,
> > int64_t decr;
> >
> > n = ns_to_tb(tb_env->decr_freq, now);
> > -if (next > n && tb_env->flags & PPC_TIMER_BOOKE) {
> > +
> > +/* BookE timers stop when reaching 0. */
> > +if (next < n && tb_env->flags & PPC_TIMER_BOOKE) {
> > decr = 0;
> > } else {
> > decr = next - n;
>
>
Re: [PATCH] hw/ppc: fix decrementer with BookE timers
On Thu, Aug 29, 2024 at 2:33 PM Cédric Le Goater wrote:
>
> On 8/28/24 09:21, Clément Chigot wrote:
> > On Tue, Aug 27, 2024 at 7:40 PM Cédric Le Goater wrote:
> >>
> >> Hello Clément,
> >>
> >> On 7/15/24 10:46, Clément Chigot wrote:
> >>> The BookE decrementer stops at 0, meaning that it won't decremented
> >>> towards "negative" values.
> >>> However, the current logic is inverted: decr is updated solely when
> >>> the resulting value would be negative.
> >>
> >> How did you hit the issue ? which machine ? I didn't see any error
> >> when booting Linux 6.6.3 on mpc8544ds, e500mc, e5500 and e6500.
> >
> > I hit this issue while running some version of VxWorks on a custom
> > machine: p3041ds (description [1] and our local implementation [2]).
> > So, I'm not that surprised you were not able to reproduce.
> >
> >>> Signed-off-by: Clément Chigot
> >>> Fixed: 8e0a5ac87800 ("hw/ppc: Avoid decrementer rounding errors")
> >>
> >> LGTM,
> >>
> >> Reviewed-by: Cédric Le Goater
> >>
> >> We have some automated tests with the ppce500 machine which it would be
> >> interesting to extend to have a better coverage of booke.
> >
> > Thanks for the pointer, I'll see if I can extend them.
> >
> >> Thanks,
> >>
> >> C.
> >>
> >
> > [1]
> > https://www.nxp.com/design/design-center/software/qoriq-developer-resources/p3041-qoriq-development-system:P3041DS
> > [2] https://github.com/AdaCore/qemu/blob/qemu-stable-9.0.0/hw/ppc/p3041ds.c
>
> Nice. Do you have any plans to upstream the machine ?
That was the plan. However, we are using that emulation in order to
test VxWorks on PPC. Recent versions have now officially provided a
BSP for QEmu using ppce500 board. We are planning to transition to it,
mainly to avoid having to maintain that P3041DS home-made
implementation. Thus, I'm not sure we'll spend time on upstreaming it
:(
That being said, if you heard people being interested in that board,
please do warn me.
Thanks,
Clément
> Thanks,
>
> C.
>
Re: [PATCH] hw/ppc: fix decrementer with BookE timers
On Tue, Aug 27, 2024 at 7:40 PM Cédric Le Goater wrote:
>
> Hello Clément,
>
> On 7/15/24 10:46, Clément Chigot wrote:
> > The BookE decrementer stops at 0, meaning that it won't decremented
> > towards "negative" values.
> > However, the current logic is inverted: decr is updated solely when
> > the resulting value would be negative.
>
> How did you hit the issue ? which machine ? I didn't see any error
> when booting Linux 6.6.3 on mpc8544ds, e500mc, e5500 and e6500.
I hit this issue while running some version of VxWorks on a custom
machine: p3041ds (description [1] and our local implementation [2]).
So, I'm not that surprised you were not able to reproduce.
> > Signed-off-by: Clément Chigot
> > Fixed: 8e0a5ac87800 ("hw/ppc: Avoid decrementer rounding errors")
>
> LGTM,
>
> Reviewed-by: Cédric Le Goater
>
> We have some automated tests with the ppce500 machine which it would be
> interesting to extend to have a better coverage of booke.
Thanks for the pointer, I'll see if I can extend them.
> Thanks,
>
> C.
>
[1]
https://www.nxp.com/design/design-center/software/qoriq-developer-resources/p3041-qoriq-development-system:P3041DS
[2] https://github.com/AdaCore/qemu/blob/qemu-stable-9.0.0/hw/ppc/p3041ds.c
>
> > ---
> > hw/ppc/ppc.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> > index e6fa5580c0..9fc85c7de0 100644
> > --- a/hw/ppc/ppc.c
> > +++ b/hw/ppc/ppc.c
> > @@ -729,7 +729,9 @@ static inline int64_t __cpu_ppc_load_decr(CPUPPCState
> > *env, int64_t now,
> > int64_t decr;
> >
> > n = ns_to_tb(tb_env->decr_freq, now);
> > -if (next > n && tb_env->flags & PPC_TIMER_BOOKE) {
> > +
> > +/* BookE timers stop when reaching 0. */
> > +if (next < n && tb_env->flags & PPC_TIMER_BOOKE) {
> > decr = 0;
> > } else {
> > decr = next - n;
>
>
Re: [PATCH] hw/ppc: fix decrementer with BookE timers
Hey,
Gentle ping
Thanks Clément
On Mon, Jul 29, 2024 at 10:33 AM Clément Chigot wrote:
>
> Hi,
>
> Gentle ping + CC missing maintainers.
>
> Thanks Clément
>
> On Mon, Jul 15, 2024 at 10:46 AM Clément Chigot wrote:
> >
> > The BookE decrementer stops at 0, meaning that it won't decremented
> > towards "negative" values.
> > However, the current logic is inverted: decr is updated solely when
> > the resulting value would be negative.
> >
> > Signed-off-by: Clément Chigot
> > Fixed: 8e0a5ac87800 ("hw/ppc: Avoid decrementer rounding errors")
> > ---
> > hw/ppc/ppc.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> > index e6fa5580c0..9fc85c7de0 100644
> > --- a/hw/ppc/ppc.c
> > +++ b/hw/ppc/ppc.c
> > @@ -729,7 +729,9 @@ static inline int64_t __cpu_ppc_load_decr(CPUPPCState
> > *env, int64_t now,
> > int64_t decr;
> >
> > n = ns_to_tb(tb_env->decr_freq, now);
> > -if (next > n && tb_env->flags & PPC_TIMER_BOOKE) {
> > +
> > +/* BookE timers stop when reaching 0. */
> > +if (next < n && tb_env->flags & PPC_TIMER_BOOKE) {
> > decr = 0;
> > } else {
> > decr = next - n;
> > --
> > 2.25.1
> >
Re: [PATCH] hw/ppc: fix decrementer with BookE timers
Hi,
Gentle ping + CC missing maintainers.
Thanks Clément
On Mon, Jul 15, 2024 at 10:46 AM Clément Chigot wrote:
>
> The BookE decrementer stops at 0, meaning that it won't decremented
> towards "negative" values.
> However, the current logic is inverted: decr is updated solely when
> the resulting value would be negative.
>
> Signed-off-by: Clément Chigot
> Fixed: 8e0a5ac87800 ("hw/ppc: Avoid decrementer rounding errors")
> ---
> hw/ppc/ppc.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index e6fa5580c0..9fc85c7de0 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -729,7 +729,9 @@ static inline int64_t __cpu_ppc_load_decr(CPUPPCState
> *env, int64_t now,
> int64_t decr;
>
> n = ns_to_tb(tb_env->decr_freq, now);
> -if (next > n && tb_env->flags & PPC_TIMER_BOOKE) {
> +
> +/* BookE timers stop when reaching 0. */
> +if (next < n && tb_env->flags & PPC_TIMER_BOOKE) {
> decr = 0;
> } else {
> decr = next - n;
> --
> 2.25.1
>
[PATCH] hw/ppc: fix decrementer with BookE timers
The BookE decrementer stops at 0, meaning that it won't decremented
towards "negative" values.
However, the current logic is inverted: decr is updated solely when
the resulting value would be negative.
Signed-off-by: Clément Chigot
Fixed: 8e0a5ac87800 ("hw/ppc: Avoid decrementer rounding errors")
---
hw/ppc/ppc.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index e6fa5580c0..9fc85c7de0 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -729,7 +729,9 @@ static inline int64_t __cpu_ppc_load_decr(CPUPPCState *env,
int64_t now,
int64_t decr;
n = ns_to_tb(tb_env->decr_freq, now);
-if (next > n && tb_env->flags & PPC_TIMER_BOOKE) {
+
+/* BookE timers stop when reaching 0. */
+if (next < n && tb_env->flags & PPC_TIMER_BOOKE) {
decr = 0;
} else {
decr = next - n;
--
2.25.1
Re: [PATCH] tests/avocado: Remove non-working sparc leon3 test
On Wed, Jul 10, 2024 at 5:25 PM Alex Bennée wrote: > > Thomas Huth writes: > > > The test has been marked as broken more than 4 years ago, and > > so far nobody ever cared to fix it. Thus let's simply remove it > > now ... if somebody ever needs it again, they can restore the > > file from an older version of QEMU. > > > > Signed-off-by: Thomas Huth > > Acked-by: Alex Bennée Yeah, it's been on my todo list for far too long but I still didn't find time to work on it. Reviewed-by: Clément Chigot
Re: [PULL 13/42] target/i386: use gen_writeback() within gen_POP()
On Wed, Jul 10, 2024 at 12:43 PM Paolo Bonzini wrote:
>
> On 7/10/24 11:42, Clément Chigot wrote:
> > Hi Mark,
> >
> > This patch introduces regressions in our x86_64 VxWorks kernels
> > running over qemu. Some page faults are triggered randomly.
> >
> > Earlier to this patch, the MemOp `ot` passed to `gen_op_st_v` was the
> > `gen_pop_T0` created a few lines above.
> > Now, this is `op->ot` which comes from elsewhere.
> >
> > Adding `op->ot = ot` just before calling `gen_writeback` fixes my
> > regressions. But I'm wondering if there could be some unexpected
> > fallbacks, `op->ot` possibly being used afterwards.
>
> Mark's patch is correct and the (previously latent) mistake is in
> the decoding table.
>
> The manual correctly says that POP has "default 64-bit" operand;
> that is, it is 64-bit even without a REX.W prefix. It must be
> marked as "d64" rather than "v".
>
> Can you test this patch?
Yes, it does work. Thanks a lot for it !
> diff --git a/target/i386/tcg/decode-new.c.inc
> b/target/i386/tcg/decode-new.c.inc
> index 0d846c32c22..d2da1d396d5 100644
> --- a/target/i386/tcg/decode-new.c.inc
> +++ b/target/i386/tcg/decode-new.c.inc
> @@ -1717,7 +1717,7 @@ static const X86OpEntry opcodes_root[256] = {
> [0x8C] = X86_OP_ENTRYwr(MOV, E,v, S,w, op0_Mw),
> [0x8D] = X86_OP_ENTRYwr(LEA, G,v, M,v, nolea),
> [0x8E] = X86_OP_ENTRYwr(MOV, S,w, E,w),
> -[0x8F] = X86_OP_GROUPw(group1A, E,v),
> +[0x8F] = X86_OP_GROUPw(group1A, E,d64),
>
> [0x98] = X86_OP_ENTRY1(CBW,0,v), /* rAX */
> [0x99] = X86_OP_ENTRYwr(CWD, 2,v, 0,v), /* rDX, rAX */
> diff --git a/target/i386/tcg/emit.c.inc b/target/i386/tcg/emit.c.inc
> index fc7477833bc..36bb308e449 100644
> --- a/target/i386/tcg/emit.c.inc
> +++ b/target/i386/tcg/emit.c.inc
> @@ -2788,6 +2788,8 @@ static void gen_POP(DisasContext *s, X86DecodedInsn
> *decode)
> X86DecodedOp *op = &decode->op[0];
> MemOp ot = gen_pop_T0(s);
>
> +/* Only 16/32-bit access in 32-bit mode, 16/64-bit access in long mode.
> */
> +assert(ot == op->ot);
> if (op->has_ea || op->unit == X86_OP_SEG) {
> /* NOTE: order is important for MMU exceptions */
> gen_writeback(s, decode, 0, s->T0);
>
> Thanks (and it's reassuring that everything else has worked fine
> for you!),
>
> Paolo
>
> > Thanks,
> > Clément
> >
> > On Sat, Jun 8, 2024 at 10:36 AM Paolo Bonzini wrote:
> >>
> >> From: Mark Cave-Ayland
> >>
> >> Instead of directly implementing the writeback using gen_op_st_v(), use the
> >> existing gen_writeback() function.
> >>
> >> Suggested-by: Paolo Bonzini
> >> Signed-off-by: Mark Cave-Ayland
> >> Message-ID: <20240606095319.229650-3-mark.cave-ayl...@ilande.co.uk>
> >> Signed-off-by: Paolo Bonzini
> >> ---
> >> target/i386/tcg/emit.c.inc | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/target/i386/tcg/emit.c.inc b/target/i386/tcg/emit.c.inc
> >> index ca78504b6e4..6123235c000 100644
> >> --- a/target/i386/tcg/emit.c.inc
> >> +++ b/target/i386/tcg/emit.c.inc
> >> @@ -2580,9 +2580,9 @@ static void gen_POP(DisasContext *s, CPUX86State
> >> *env, X86DecodedInsn *decode)
> >>
> >> if (op->has_ea) {
> >> /* NOTE: order is important for MMU exceptions */
> >> -gen_op_st_v(s, ot, s->T0, s->A0);
> >> -op->unit = X86_OP_SKIP;
> >> +gen_writeback(s, decode, 0, s->T0);
> >> }
> >> +
> >> /* NOTE: writing back registers after update is important for pop
> >> %sp */
> >> gen_pop_update(s, ot);
> >> }
> >> --
> >> 2.45.1
> >>
> >>
> >
> >
>
Re: [PULL 13/42] target/i386: use gen_writeback() within gen_POP()
Hi Mark,
This patch introduces regressions in our x86_64 VxWorks kernels
running over qemu. Some page faults are triggered randomly.
Earlier to this patch, the MemOp `ot` passed to `gen_op_st_v` was the
`gen_pop_T0` created a few lines above.
Now, this is `op->ot` which comes from elsewhere.
Adding `op->ot = ot` just before calling `gen_writeback` fixes my
regressions. But I'm wondering if there could be some unexpected
fallbacks, `op->ot` possibly being used afterwards.
Thanks,
Clément
On Sat, Jun 8, 2024 at 10:36 AM Paolo Bonzini wrote:
>
> From: Mark Cave-Ayland
>
> Instead of directly implementing the writeback using gen_op_st_v(), use the
> existing gen_writeback() function.
>
> Suggested-by: Paolo Bonzini
> Signed-off-by: Mark Cave-Ayland
> Message-ID: <20240606095319.229650-3-mark.cave-ayl...@ilande.co.uk>
> Signed-off-by: Paolo Bonzini
> ---
> target/i386/tcg/emit.c.inc | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/i386/tcg/emit.c.inc b/target/i386/tcg/emit.c.inc
> index ca78504b6e4..6123235c000 100644
> --- a/target/i386/tcg/emit.c.inc
> +++ b/target/i386/tcg/emit.c.inc
> @@ -2580,9 +2580,9 @@ static void gen_POP(DisasContext *s, CPUX86State *env,
> X86DecodedInsn *decode)
>
> if (op->has_ea) {
> /* NOTE: order is important for MMU exceptions */
> -gen_op_st_v(s, ot, s->T0, s->A0);
> -op->unit = X86_OP_SKIP;
> +gen_writeback(s, decode, 0, s->T0);
> }
> +
> /* NOTE: writing back registers after update is important for pop %sp */
> gen_pop_update(s, ot);
> }
> --
> 2.45.1
>
>
[PATCH] target/sparc: use signed denominator in sdiv helper
The result has to be done with the signed denominator (b32) instead of
the unsigned value passed in argument (b).
Fixes: 1326010322d6 ("target/sparc: Remove CC_OP_DIV")
Signed-off-by: Clément Chigot
---
target/sparc/helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/sparc/helper.c b/target/sparc/helper.c
index 2247e243b5..7846ddd6f6 100644
--- a/target/sparc/helper.c
+++ b/target/sparc/helper.c
@@ -121,7 +121,7 @@ uint64_t helper_sdiv(CPUSPARCState *env, target_ulong a,
target_ulong b)
return (uint32_t)(b32 < 0 ? INT32_MAX : INT32_MIN) | (-1ull << 32);
}
-a64 /= b;
+a64 /= b32;
r = a64;
if (unlikely(r != a64)) {
return (uint32_t)(a64 < 0 ? INT32_MIN : INT32_MAX) | (-1ull << 32);
--
2.25.1
Re: [PATCH] target/arm: Restrict translation disabled alignment check to VMSA
On Mon, Apr 22, 2024 at 11:02 PM Philippe Mathieu-Daudé
wrote:
>
> On 22/4/24 19:09, Richard Henderson wrote:
> > On 4/22/24 10:07, Richard Henderson wrote:
> >> For cpus using PMSA, when the MPU is disabled, the default memory
> >> type is Normal, Non-cachable.
> >>
> >> Fixes: 59754f85ed3 ("target/arm: Do memory type alignment check when
> >> translation disabled")
> >> Reported-by: Clément Chigot
> >> Signed-off-by: Richard Henderson
> >> ---
> >>
> >> Since v9 will likely be tagged tomorrow without this fixed,
> >> Cc: qemu-sta...@nongnu.org
> >>
> >> ---
> >> target/arm/tcg/hflags.c | 12 ++--
> >> 1 file changed, 10 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/target/arm/tcg/hflags.c b/target/arm/tcg/hflags.c
> >> index 5da1b0fc1d..66de30b828 100644
> >> --- a/target/arm/tcg/hflags.c
> >> +++ b/target/arm/tcg/hflags.c
> >> @@ -38,8 +38,16 @@ static bool aprofile_require_alignment(CPUARMState
> >> *env, int el, uint64_t sctlr)
> >> }
> >> /*
> >> - * If translation is disabled, then the default memory type is
> >> - * Device(-nGnRnE) instead of Normal, which requires that alignment
> >> + * With PMSA, when the MPU is disabled, all memory types in the
> >> + * default map is Normal.
> >> + */
> >> +if (arm_feature(env, ARM_FEATURE_PMSA)) {
> >> +return false;
> >> +}
> >> +
> >> +/*
> >> + * With VMSA, if translation is disabled, then the default memory
> >> type
> >> + * is Device(-nGnRnE) instead of Normal, which requires that
> >> alignment
> >>* be enforced. Since this affects all ram, it is most efficient
> >>* to handle this during translation.
> >>*/
> >
> > Oh, I meant to add: since the armv7 manual has both VMSA and PMSA
> > sections, and the language about default Device type and alignment
> > traps, is in the VMSA section.
>
> To the best of my knowledge,
> Reviewed-by: Philippe Mathieu-Daudé
Thanks for the patch.
Tested-by: Clément Chigot
Re: [PATCH v3 5/6] target/arm: Do memory type alignment check when translation disabled
Hi Richard,
While testing the future V9, I've some regressions on a custom board
using cortex-R5 CPUs.
Unaligned data accesses are no longer allowed because of that patch.
I've dug into the various documentation and it seems that R-profile
CPUs don't have the same default memory type as A-profile. It depends
on a default memory map provided in the R-Profile RM in C1.3 [1], even
when PMU is disabled.
> Each PMSAv8-32 MPU has an associated default memory map which is used when
> the MPU is not enabled.
> ...
> Table C1-4 and Table C1-5 describe the default memory map defined for the EL1
> MPU.
For our case, Table C1-5 can be simplified as:
| 0x – 0x7FFF Normal
| 0x8000 – 0xBFFF Device-nGnRE
| 0xC000 – 0x Device-nGnRnE
Therefore, we can't blindly enable strict alignment checking solely on
SCTLR bits. We should make it depend on the address targeted. But is
it possible to know that address in `aprofile_require_alignment` ?
with `mmu_idx` ?
By the way, are R-Profile CPUs the same as those having the `PMSA`
feature ? That could mean we can use the `ARM_FEATURE_PMSA` to deal
with that, instead of create a new `ARM_FEATURE_R`
Note that the RM I've linked is for ARMv8. But this other link [2]
seems to show a similar behavior for arm-v7.
cc Jonathan and Ard, though not sure this is the same bug you've
reported earlier.
Thanks,
Clément
[1] https://developer.arm.com/documentation/ddi0568/a-c/?lang=en
[2]
https://developer.arm.com/documentation/ddi0406/cb/System-Level-Architecture/Protected-Memory-System-Architecture--PMSA-/About-the-PMSA/Enabling-and-disabling-the-MPU?lang=en#BEIJEFCJ
On Fri, Mar 1, 2024 at 9:43 PM Richard Henderson
wrote:
>
> If translation is disabled, the default memory type is Device, which
> requires alignment checking. This is more optimally done early via
> the MemOp given to the TCG memory operation.
>
> Reviewed-by: Philippe Mathieu-Daudé
> Reported-by: Idan Horowitz
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1204
> Signed-off-by: Richard Henderson
> ---
> target/arm/tcg/hflags.c | 34 --
> 1 file changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/tcg/hflags.c b/target/arm/tcg/hflags.c
> index 8e5d35d922..5da1b0fc1d 100644
> --- a/target/arm/tcg/hflags.c
> +++ b/target/arm/tcg/hflags.c
> @@ -26,6 +26,35 @@ static inline bool fgt_svc(CPUARMState *env, int el)
> FIELD_EX64(env->cp15.fgt_exec[FGTREG_HFGITR], HFGITR_EL2, SVC_EL1);
> }
>
> +/* Return true if memory alignment should be enforced. */
> +static bool aprofile_require_alignment(CPUARMState *env, int el, uint64_t
> sctlr)
> +{
> +#ifdef CONFIG_USER_ONLY
> +return false;
> +#else
> +/* Check the alignment enable bit. */
> +if (sctlr & SCTLR_A) {
> +return true;
> +}
> +
> +/*
> + * If translation is disabled, then the default memory type is
> + * Device(-nGnRnE) instead of Normal, which requires that alignment
> + * be enforced. Since this affects all ram, it is most efficient
> + * to handle this during translation.
> + */
> +if (sctlr & SCTLR_M) {
> +/* Translation enabled: memory type in PTE via MAIR_ELx. */
> +return false;
> +}
> +if (el < 2 && (arm_hcr_el2_eff(env) & (HCR_DC | HCR_VM))) {
> +/* Stage 2 translation enabled: memory type in PTE. */
> +return false;
> +}
> +return true;
> +#endif
> +}
> +
> static CPUARMTBFlags rebuild_hflags_common(CPUARMState *env, int fp_el,
> ARMMMUIdx mmu_idx,
> CPUARMTBFlags flags)
> @@ -121,8 +150,9 @@ static CPUARMTBFlags rebuild_hflags_a32(CPUARMState *env,
> int fp_el,
> {
> CPUARMTBFlags flags = {};
> int el = arm_current_el(env);
> +uint64_t sctlr = arm_sctlr(env, el);
>
> -if (arm_sctlr(env, el) & SCTLR_A) {
> +if (aprofile_require_alignment(env, el, sctlr)) {
> DP_TBFLAG_ANY(flags, ALIGN_MEM, 1);
> }
>
> @@ -223,7 +253,7 @@ static CPUARMTBFlags rebuild_hflags_a64(CPUARMState *env,
> int el, int fp_el,
>
> sctlr = regime_sctlr(env, stage1);
>
> -if (sctlr & SCTLR_A) {
> +if (aprofile_require_alignment(env, el, sctlr)) {
> DP_TBFLAG_ANY(flags, ALIGN_MEM, 1);
> }
>
> --
> 2.34.1
>
>
Re: [PATCH for-9.0] target/riscv: do not enable all named features by default
On Tue, Mar 12, 2024 at 9:32 PM Daniel Henrique Barboza
wrote:
>
> Commit 3b8022269c added the capability of named features/profile
> extensions to be added in riscv,isa. To do that we had to assign priv
> versions for each one of them in isa_edata_arr[]. But this resulted in a
> side-effect: vendor CPUs that aren't running priv_version_latest started
> to experience warnings for these profile extensions [1]:
>
> | $ qemu-system-riscv32 -M sifive_e
> | qemu-system-riscv32: warning: disabling zic64b extension for hart
> 0x because privilege spec version does not match
> | qemu-system-riscv32: warning: disabling ziccamoa extension for
> hart 0x because privilege spec version does not match
>
> This is benign as far as the CPU behavior is concerned since disabling
> both extensions is a no-op (aside from riscv,isa). But the warnings are
> unpleasant to deal with, especially because we're sending user warnings
> for extensions that users can't enable/disable.
>
> Instead of enabling all named features all the time, separate them by
> priv version. During finalize() time, after we decided which
> priv_version the CPU is running, enable/disable all the named extensions
> based on the priv spec chosen. This will be enough for a bug fix, but as
> a future work we should look into how we can name these extensions in a
> way that we don't need an explicit ext_name => priv_ver as we're doing
> here.
>
> The named extensions being added in isa_edata_arr[] that will be
> enabled/disabled based solely on priv version can be removed from
> riscv_cpu_named_features[]. 'zic64b' is an extension that can be
> disabled based on block sizes so it'll retain its own flag and entry.
>
> [1] https://lists.gnu.org/archive/html/qemu-devel/2024-03/msg02592.html
>
> Reported-by: Clément Chigot
> Fixes: 3b8022269c ("target/riscv: add riscv,isa to named features")
> Suggested-by: Andrew Jones
> Signed-off-by: Daniel Henrique Barboza
> ---
> target/riscv/cpu.c | 40 +-
> target/riscv/cpu_cfg.h | 8 +---
> target/riscv/tcg/tcg-cpu.c | 14 ++---
> 3 files changed, 25 insertions(+), 37 deletions(-)
Thanks for the quick patch !
Tested-by: Clément Chigot
Re: [PULL 09/34] target/riscv: add remaining named features
Hi Alistair,
Since this series, I'm getting warnings when using a CPU not
supporting the latest ISA, such as the SIFIVE_E series.
| $ qemu-system-riscv32 -M sifive_e
| qemu-system-riscv32: warning: disabling zic64b extension for hart
0x because privilege spec version does not match
| qemu-system-riscv32: warning: disabling ziccamoa extension for
hart 0x because privilege spec version does not match
Those are always enabled during the initialization but
riscv_cpu_disable_priv_spec_isa_exts is detecting them as unsupported
by the CPU and thus disabling them.
However, are those extensions different from zicnptr and zihpm
extensions ? As they are not enabled by the same mean, I'm wondering
about that ? Or do we want to skip their ISA verification as well ?
Thanks,
Clément
On Fri, Mar 8, 2024 at 12:13 PM Alistair Francis wrote:
>
> From: Daniel Henrique Barboza
>
> The RVA22U64 and RVA22S64 profiles mandates certain extensions that,
> until now, we were implying that they were available.
>
> We can't do this anymore since named features also has a riscv,isa
> entry. Let's add them to riscv_cpu_named_features[].
>
> Instead of adding one bool for each named feature that we'll always
> implement, i.e. can't be turned off, add a 'ext_always_enabled' bool in
> cpu->cfg. This bool will be set to 'true' in TCG accel init, and all
> named features will point to it. This also means that KVM won't see
> these features as always enable, which is our intention.
>
> If any accelerator adds support to disable one of these features, we'll
> have to promote them to regular extensions and allow users to disable it
> via command line.
>
> After this patch, here's the riscv,isa from a buildroot using the
> 'rva22s64' CPU:
>
> # cat /proc/device-tree/cpus/cpu@0/riscv,isa
> rv64imafdc_zic64b_zicbom_zicbop_zicboz_ziccamoa_ziccif_zicclsm_ziccrse_
> zicntr_zicsr_zifencei_zihintpause_zihpm_za64rs_zfhmin_zca_zcd_zba_zbb_
> zbs_zkt_ssccptr_sscounterenw_sstvala_sstvecd_svade_svinval_svpbmt#
>
> Signed-off-by: Daniel Henrique Barboza
> Reviewed-by: Alistair Francis
> Reviewed-by: Andrew Jones
> Message-ID: <20240215223955.969568-4-dbarb...@ventanamicro.com>
> Signed-off-by: Alistair Francis
> ---
> target/riscv/cpu_cfg.h | 6 ++
> target/riscv/cpu.c | 42 +++---
> target/riscv/tcg/tcg-cpu.c | 2 ++
> 3 files changed, 43 insertions(+), 7 deletions(-)
>
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index e68a4ddb92..be39870691 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -128,6 +128,12 @@ struct RISCVCPUConfig {
> bool ext_svade;
> bool ext_zic64b;
>
> +/*
> + * Always 'true' boolean for named features
> + * TCG always implement/can't be disabled.
> + */
> +bool ext_always_enabled;
> +
> /* Vendor-specific custom extensions */
> bool ext_xtheadba;
> bool ext_xtheadbb;
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index f0cd408237..4c4fa79145 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -102,6 +102,10 @@ const RISCVIsaExtData isa_edata_arr[] = {
> ISA_EXT_DATA_ENTRY(zicbom, PRIV_VERSION_1_12_0, ext_zicbom),
> ISA_EXT_DATA_ENTRY(zicbop, PRIV_VERSION_1_12_0, ext_zicbop),
> ISA_EXT_DATA_ENTRY(zicboz, PRIV_VERSION_1_12_0, ext_zicboz),
> +ISA_EXT_DATA_ENTRY(ziccamoa, PRIV_VERSION_1_11_0, ext_always_enabled),
> +ISA_EXT_DATA_ENTRY(ziccif, PRIV_VERSION_1_11_0, ext_always_enabled),
> +ISA_EXT_DATA_ENTRY(zicclsm, PRIV_VERSION_1_11_0, ext_always_enabled),
> +ISA_EXT_DATA_ENTRY(ziccrse, PRIV_VERSION_1_11_0, ext_always_enabled),
> ISA_EXT_DATA_ENTRY(zicond, PRIV_VERSION_1_12_0, ext_zicond),
> ISA_EXT_DATA_ENTRY(zicntr, PRIV_VERSION_1_12_0, ext_zicntr),
> ISA_EXT_DATA_ENTRY(zicsr, PRIV_VERSION_1_10_0, ext_zicsr),
> @@ -110,6 +114,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
> ISA_EXT_DATA_ENTRY(zihintpause, PRIV_VERSION_1_10_0, ext_zihintpause),
> ISA_EXT_DATA_ENTRY(zihpm, PRIV_VERSION_1_12_0, ext_zihpm),
> ISA_EXT_DATA_ENTRY(zmmul, PRIV_VERSION_1_12_0, ext_zmmul),
> +ISA_EXT_DATA_ENTRY(za64rs, PRIV_VERSION_1_12_0, ext_always_enabled),
> ISA_EXT_DATA_ENTRY(zaamo, PRIV_VERSION_1_12_0, ext_zaamo),
> ISA_EXT_DATA_ENTRY(zacas, PRIV_VERSION_1_12_0, ext_zacas),
> ISA_EXT_DATA_ENTRY(zalrsc, PRIV_VERSION_1_12_0, ext_zalrsc),
> @@ -173,8 +178,12 @@ const RISCVIsaExtData isa_edata_arr[] = {
> ISA_EXT_DATA_ENTRY(smepmp, PRIV_VERSION_1_12_0, ext_smepmp),
> ISA_EXT_DATA_ENTRY(smstateen, PRIV_VERSION_1_12_0, ext_smstateen),
> ISA_EXT_DATA_ENTRY(ssaia, PRIV_VERSION_1_12_0, ext_ssaia),
> +ISA_EXT_DATA_ENTRY(ssccptr, PRIV_VERSION_1_11_0, ext_always_enabled),
> ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf),
> +ISA_EXT_DATA_ENTRY(sscounterenw, PRIV_VERSION_1_12_0,
> ext_always_enabled),
> ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, e
[PATCH] hw/intc/grlib_irqmp: abort realize when ncpus value is out of range
Even if the error is set, the build is not aborted when the ncpus value is wrong, the return is missing. Signed-off-by: Clément Chigot --- hw/intc/grlib_irqmp.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/intc/grlib_irqmp.c b/hw/intc/grlib_irqmp.c index 144b121d48..c6c51a349c 100644 --- a/hw/intc/grlib_irqmp.c +++ b/hw/intc/grlib_irqmp.c @@ -356,6 +356,7 @@ static void grlib_irqmp_realize(DeviceState *dev, Error **errp) error_setg(errp, "Invalid ncpus properties: " "%u, must be 0 < ncpus =< %u.", irqmp->ncpus, IRQMP_MAX_CPU); +return; } qdev_init_gpio_in(dev, grlib_irqmp_set_irq, MAX_PILS); -- 2.25.1
Re: [PULL 34/56] hw/intc/grlib_irqmp: add ncpus property
On Fri, Mar 8, 2024 at 2:27 PM Peter Maydell wrote:
>
> On Thu, 15 Feb 2024 at 18:04, Philippe Mathieu-Daudé
> wrote:
> >
> > From: Clément Chigot
> >
> > This adds a "ncpus" property to the "grlib-irqmp" device to be used
> > later, this required a little refactoring of how we initialize the
> > device (ie: use realize instead of init).
> >
> > Co-developed-by: Frederic Konrad
> > Signed-off-by: Clément Chigot
> > Reviewed-by: Philippe Mathieu-Daudé
> > Message-ID: <20240131085047.18458-3-chi...@adacore.com>
> > Signed-off-by: Philippe Mathieu-Daudé
>
> Hi; Coverity points out a bug in this commit (CID 1534914):
>
>
> > -static void grlib_irqmp_init(Object *obj)
> > +static void grlib_irqmp_realize(DeviceState *dev, Error **errp)
> > {
> > -IRQMP *irqmp = GRLIB_IRQMP(obj);
> > -SysBusDevice *dev = SYS_BUS_DEVICE(obj);
> > +IRQMP *irqmp = GRLIB_IRQMP(dev);
> >
> > -qdev_init_gpio_in(DEVICE(obj), grlib_irqmp_set_irq, MAX_PILS);
> > -qdev_init_gpio_out_named(DEVICE(obj), &irqmp->irq, "grlib-irq", 1);
> > -memory_region_init_io(&irqmp->iomem, obj, &grlib_irqmp_ops, irqmp,
> > +if ((!irqmp->ncpus) || (irqmp->ncpus > IRQMP_MAX_CPU)) {
> > +error_setg(errp, "Invalid ncpus properties: "
> > + "%u, must be 0 < ncpus =< %u.", irqmp->ncpus,
> > + IRQMP_MAX_CPU);
> > +}
>
> We detect the out-of-range 'ncpus' value, but forget the "return"
> statement, so execution will continue onward regardless, and
> overrun the irqmp->irq[] array when we call qdev_init_gpio_out_named().
Indeed, I'll send a patch.
Thanks for pointing that out.
Clément
> > +
> > +qdev_init_gpio_in(dev, grlib_irqmp_set_irq, MAX_PILS);
> > +qdev_init_gpio_out_named(dev, &irqmp->irq, "grlib-irq", 1);
> > +memory_region_init_io(&irqmp->iomem, OBJECT(dev), &grlib_irqmp_ops,
> > irqmp,
> >"irqmp", IRQMP_REG_SIZE);
> >
> > irqmp->state = g_malloc0(sizeof *irqmp->state);
> >
> > -sysbus_init_mmio(dev, &irqmp->iomem);
> > +sysbus_init_mmio(SYS_BUS_DEVICE(dev), &irqmp->iomem);
> > }
>
> thanks
> -- PMM
Re: QNX VM hang on Qemu
On Fri, Mar 1, 2024 at 6:48 PM Faiq Ali Sayed wrote: > > Hi Clément, > > So the output of the command > | $ qemu-system-aarch64 -M xlnx-zcu102 -m 4G -no-reboot -nographic > -kernel qnx.img > is > > $ pulseaudio: set_sink_input_volume() failed > $ pulseaudio: Reason: Invalid argument > $ pulseaudio: set_sink_input_mute() failed > $ pulseaudio: Reason: Invalid argument > > I am not using the "mkifs" rather I am using the command below.. I guess we disabled some options when we built our kernels to allow a simple command line. However, I don't know which ones. > $ mkqnximage --type=qemu --arch=aarch64le --build --ssh-ident=none > > if I use the --run option with the command it creates a VM which is working > fine. > but when I use this image, with qemu command terminal is getting stuck. I wasn't aware that such a command exists. However, the --run option seems to call the script "runimage". You might want to take a look at the options being set or maybe simply retrieve the qemu command line created by it (if that's not how you created your first command line). Sorry to not be more helpful. Clément > BR! > Faiq > > > > On Fri, Mar 1, 2024 at 4:29 PM Clément Chigot wrote: >> >> Hi Faiq, >> >> On Fri, Feb 23, 2024 at 3:55 PM Faiq Ali Sayed >> wrote: >> > >> > So as far as my understanding, we provide these binaries using Qemu >> > command as depicted in the example you provided and there is no way I >> > found to put them into a single image. >> > Regarding the overlapping space, I don't have much idea but I think we >> > could provide a starting address separately to these images something like >> > addr=0x0010. >> >> Where is this 0x0010 address coming from ? Could you confirm with >> "readelf -h" that this is the entry point of your image ? >> >> Alternatively and that's what we used locally, qemu is able to guess >> the entry point of an image, when passed from -kernel. Therefore, our >> command simply looks like: >> | $ qemu-system-aarch64 -M xlnx-zcu102 -m 4G -no-reboot -nographic >> -kernel qnx.img >> >> I'm not the one having built the qnx.img we're using. But it looks >> pretty standard at the first look, made with "mkifs" and the kernel >> specs from zcu102 evaluation kit. >> >> Hope it helps, >> Clément >> >> > So as per your suggestion, I compared my images and I found that the image >> > does not show a virtual disk, and other commands like mkdir, do not have >> > these binaries. >> > So these binaries are not included at the time of image creation and I >> > don't exactly know that how can we add these binaries into the QNX image. >> > >> > The Image that is currently installed in real hardware does not have a >> > debugging symbol, so I can't use GDB to debug that. >> > Now I am looking for a way to create the correct QNX OS image for Qemu. >> > >> > Any lead in this regard will be really helpful :) >> > > > > > -- > Kind Regard- > Faiq Ali Sayed > >
Re: QNX VM hang on Qemu
Hi Faiq, On Fri, Feb 23, 2024 at 3:55 PM Faiq Ali Sayed wrote: > > So as far as my understanding, we provide these binaries using Qemu command > as depicted in the example you provided and there is no way I found to put > them into a single image. > Regarding the overlapping space, I don't have much idea but I think we could > provide a starting address separately to these images something like > addr=0x0010. Where is this 0x0010 address coming from ? Could you confirm with "readelf -h" that this is the entry point of your image ? Alternatively and that's what we used locally, qemu is able to guess the entry point of an image, when passed from -kernel. Therefore, our command simply looks like: | $ qemu-system-aarch64 -M xlnx-zcu102 -m 4G -no-reboot -nographic -kernel qnx.img I'm not the one having built the qnx.img we're using. But it looks pretty standard at the first look, made with "mkifs" and the kernel specs from zcu102 evaluation kit. Hope it helps, Clément > So as per your suggestion, I compared my images and I found that the image > does not show a virtual disk, and other commands like mkdir, do not have > these binaries. > So these binaries are not included at the time of image creation and I don't > exactly know that how can we add these binaries into the QNX image. > > The Image that is currently installed in real hardware does not have a > debugging symbol, so I can't use GDB to debug that. > Now I am looking for a way to create the correct QNX OS image for Qemu. > > Any lead in this regard will be really helpful :) >
Re: [PATCH] hw/sparc/leon3: Fix wrong usage of DO_UPCAST macro
Hi Philippe, Thomas
Thanks for handling that !
And I do confirm that it does not trigger any obvious regression on our side.
Thanks,
Clément
On Thu, Feb 22, 2024 at 8:46 AM Philippe Mathieu-Daudé
wrote:
>
> On 21/2/24 19:49, Philippe Mathieu-Daudé wrote:
> > On 21/2/24 19:47, Philippe Mathieu-Daudé wrote:
> >> On 21/2/24 19:07, Thomas Huth wrote:
> >>> leon3.c currently fails to compile with some compilers when the -Wvla
> >>> option has been enabled:
> >>>
> >>> ../hw/sparc/leon3.c: In function ‘leon3_cpu_reset’:
> >>> ../hw/sparc/leon3.c:153:5: error: ISO C90 forbids variable length
> >>> array
> >>>‘offset_must_be_zero’ [-Werror=vla]
> >>> 153 | ResetData *s = (ResetData *)DO_UPCAST(ResetData,
> >>> info[id], info);
> >>> | ^
> >>> cc1: all warnings being treated as errors
> >>>
> >>> Looking at this code, the DO_UPCAST macro is indeed used in a wrong way
> >>> here: DO_UPCAST is supposed to check that the second parameter is the
> >>> first entry of the struct that the first parameter indicates, but since
> >>> we use and index into the info[] array, this of course cannot work.
> >>>
> >>> The intention here was likely rather to use the container_of() macro
> >>> instead, so switch the code accordingly.
> >
> > Fixes: d65aba8286 ("hw/sparc/leon3: implement multiprocessor")
> >
> >>> Signed-off-by: Thomas Huth
> >>> ---
> >>> hw/sparc/leon3.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> Reviewed-by: Philippe Mathieu-Daudé
> >>
>
> Patch queued!
>
Re: [PATCH 3/3] meson: Enable -Wvla
On Wed, Feb 21, 2024 at 6:27 PM Philippe Mathieu-Daudé
wrote:
>
> On 21/2/24 17:59, Thomas Huth wrote:
> > On 21/02/2024 17.26, Thomas Huth wrote:
> >> From: Peter Maydell
> >>
> >> QEMU has historically used variable length arrays only very rarely.
> >> Variable length arrays are a potential security issue where an
> >> on-stack dynamic allocation isn't correctly size-checked, especially
> >> when the size comes from the guest. (An example problem of this kind
> >> from the past is CVE-2021-3527). Forbidding them entirely is a
> >> defensive measure against further bugs of this kind.
> >>
> >> Enable -Wvla to prevent any new uses from sneaking into the codebase.
> >>
> >> Signed-off-by: Peter Maydell
> >> Message-ID: <20240125173211.1786196-3-peter.mayd...@linaro.org>
> >> [thuth: rebased to current master branch]
> >> Signed-off-by: Thomas Huth
> >> ---
> >> meson.build | 1 +
> >> 1 file changed, 1 insertion(+)
>
> Reviewed-by: Philippe Mathieu-Daudé
> Tested-by: Philippe Mathieu-Daudé
>
> >> diff --git a/meson.build b/meson.build
> >> index c1dc83e4c0..0ef1654e86 100644
> >> --- a/meson.build
> >> +++ b/meson.build
> >> @@ -592,6 +592,7 @@ warn_flags = [
> >> '-Wstrict-prototypes',
> >> '-Wtype-limits',
> >> '-Wundef',
> >> + '-Wvla',
> >> '-Wwrite-strings',
> >> # Then disable some undesirable warnings
> >
> > Sigh, there's a new warning in the latest master branch:
> >
> > https://gitlab.com/thuth/qemu/-/jobs/6225992174
> >
> > Caused by commit d65aba828 ("hw/sparc/leon3: implement multiprocessor")...
> > Clément, Philippe, could this maybe be written in a different way that
> > does not trigger a -Wvla warning?
>
> Clément, ResetData::entry isn't used, so we can simplify removing
> the whole ResetData structure, but I'm not sure this is intended:
AFAICT, it used to take the kernel entry point before the bootloader
part was implemented. But I'm wondering if it could not be one of the
issues being the avocado failure. I do want to resurrect it but I
missed the time at the moment...
I'll do some testing with your patch and see how it goes anyway.
> -- >8 --
> diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
> index 4873b59b6c..1ff6b5d63d 100644
> --- a/hw/sparc/leon3.c
> +++ b/hw/sparc/leon3.c
> @@ -68,14 +68,6 @@
> #define LEON3_APB_PNP_OFFSET (0x800FF000)
> #define LEON3_AHB_PNP_OFFSET (0xF000)
>
> -typedef struct ResetData {
> -struct CPUResetData {
> -int id;
> -SPARCCPU *cpu;
> -} info[MAX_CPUS];
> -uint32_t entry; /* save kernel entry in case of reset */
> -} ResetData;
> -
> static uint32_t *gen_store_u32(uint32_t *code, hwaddr addr, uint32_t val)
> {
> stl_p(code++, 0x8210); /* mov %g0, %g1*/
> @@ -148,17 +140,14 @@ static void write_bootloader(void *ptr, hwaddr
> kernel_addr)
>
> static void leon3_cpu_reset(void *opaque)
> {
> -struct CPUResetData *info = (struct CPUResetData *) opaque;
> -int id = info->id;
> -ResetData *s = (ResetData *)DO_UPCAST(ResetData, info[id], info);
> -CPUState *cpu = CPU(s->info[id].cpu);
> +CPUState *cpu = opaque;
> CPUSPARCState *env = cpu_env(cpu);
>
> cpu_reset(cpu);
>
> cpu->halted = cpu->cpu_index != 0;
> -env->pc = s->entry;
> -env->npc = s->entry + 4;
> +env->pc = LEON3_PROM_OFFSET;
> +env->npc = LEON3_PROM_OFFSET + 4;
> }
>
> static void leon3_cache_control_int(CPUSPARCState *env)
> @@ -259,7 +248,7 @@ static void leon3_generic_hw_init(MachineState *machine)
> ram_addr_t ram_size = machine->ram_size;
> const char *bios_name = machine->firmware ?: LEON3_PROM_FILENAME;
> const char *kernel_filename = machine->kernel_filename;
> -SPARCCPU *cpu;
> +SPARCCPU *cpu[MAX_CPUS];
> CPUSPARCState *env;
> MemoryRegion *address_space_mem = get_system_memory();
> MemoryRegion *prom = g_new(MemoryRegion, 1);
> @@ -267,28 +256,22 @@ static void leon3_generic_hw_init(MachineState
> *machine)
> char *filename;
> int bios_size;
> int prom_size;
> -ResetData *reset_info;
> DeviceState *dev, *irqmpdev;
> int i;
> AHBPnp *ahb_pnp;
> APBPnp *apb_pnp;
>
> -reset_info = g_malloc0(sizeof(ResetData));
> -
> for (i = 0; i < machine->smp.cpus; i++) {
> /* Init CPU */
> -cpu = SPARC_CPU(object_new(machine->cpu_type));
> -qdev_init_gpio_in_named(DEVICE(cpu), leon3_start_cpu,
> "start_cpu", 1);
> -qdev_init_gpio_in_named(DEVICE(cpu), leon3_set_pil_in, "pil", 1);
> -qdev_realize(DEVICE(cpu), NULL, &error_fatal);
> -env = &cpu->env;
> +cpu[i] = SPARC_CPU(object_new(machine->cpu_type));
> +qdev_init_gpio_in_named(DEVICE(cpu[i]), leon3_start_cpu,
> "start_cpu", 1);
> +qdev_init_gpio_in_named(DEVICE(cpu[i]), leon3_set_pil_in,
> "pil", 1);
> +qdev_realize(DEVICE(cpu[i]), NULL, &error_fatal);
> +env = &cpu[i]->en
Re: [PATCH v3 0/9] sparc/leon3: Add support for -smp
Hi Philippe On Thu, Feb 15, 2024 at 10:02 AM Philippe Mathieu-Daudé wrote: > > Hi Clément, > > On 31/1/24 09:50, Clément Chigot wrote: > > > This series allows leon3 emulations to record up 4 CPUs. > > > Clément Chigot (9): > >sparc/grlib: split out the headers for each peripherals > >intc/grlib_irqmp: add ncpus property > >intc/grlib_irqmp: implements the multiprocessor status register > >intc/grlib_irqmp: implements multicore irq > >target/sparc: implement asr17 feature for smp > >leon3: remove SP initialization > >leon3: implement multiprocessor > >leon3: check cpu_id in the tiny bootloader > >MAINTAINERS: replace Fabien by myself as Leon3 maintainer > What is your base commit to apply this series? It's commit 11be70677c70fdccd452a3233653949b79e97908 Merge tag 'pull-vfio-20240129' of https://github.com/legoater/qemu into staging Clément
[PATCH v3 2/9] intc/grlib_irqmp: add ncpus property
This adds a "ncpus" property to the "grlib-irqmp" device to be used later,
this required a little refactoring of how we initialize the device (ie: use
realize instead of init).
Co-developed-by: Frederic Konrad
Signed-off-by: Clément Chigot
Reviewed-by: Philippe Mathieu-Daudé
---
hw/intc/grlib_irqmp.c | 30 +-
hw/sparc/leon3.c | 2 +-
2 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/hw/intc/grlib_irqmp.c b/hw/intc/grlib_irqmp.c
index 11eef62457..744cd64c58 100644
--- a/hw/intc/grlib_irqmp.c
+++ b/hw/intc/grlib_irqmp.c
@@ -1,7 +1,7 @@
/*
* QEMU GRLIB IRQMP Emulator
*
- * (Multiprocessor and extended interrupt not supported)
+ * (Extended interrupt not supported)
*
* SPDX-License-Identifier: MIT
*
@@ -63,6 +63,7 @@ struct IRQMP {
MemoryRegion iomem;
+unsigned int ncpus;
IRQMPState *state;
qemu_irq irq;
};
@@ -326,33 +327,44 @@ static void grlib_irqmp_reset(DeviceState *d)
irqmp->state->parent = irqmp;
}
-static void grlib_irqmp_init(Object *obj)
+static void grlib_irqmp_realize(DeviceState *dev, Error **errp)
{
-IRQMP *irqmp = GRLIB_IRQMP(obj);
-SysBusDevice *dev = SYS_BUS_DEVICE(obj);
+IRQMP *irqmp = GRLIB_IRQMP(dev);
-qdev_init_gpio_in(DEVICE(obj), grlib_irqmp_set_irq, MAX_PILS);
-qdev_init_gpio_out_named(DEVICE(obj), &irqmp->irq, "grlib-irq", 1);
-memory_region_init_io(&irqmp->iomem, obj, &grlib_irqmp_ops, irqmp,
+if ((!irqmp->ncpus) || (irqmp->ncpus > IRQMP_MAX_CPU)) {
+error_setg(errp, "Invalid ncpus properties: "
+ "%u, must be 0 < ncpus =< %u.", irqmp->ncpus,
+ IRQMP_MAX_CPU);
+}
+
+qdev_init_gpio_in(dev, grlib_irqmp_set_irq, MAX_PILS);
+qdev_init_gpio_out_named(dev, &irqmp->irq, "grlib-irq", 1);
+memory_region_init_io(&irqmp->iomem, OBJECT(dev), &grlib_irqmp_ops, irqmp,
"irqmp", IRQMP_REG_SIZE);
irqmp->state = g_malloc0(sizeof *irqmp->state);
-sysbus_init_mmio(dev, &irqmp->iomem);
+sysbus_init_mmio(SYS_BUS_DEVICE(dev), &irqmp->iomem);
}
+static Property grlib_irqmp_properties[] = {
+DEFINE_PROP_UINT32("ncpus", IRQMP, ncpus, 1),
+DEFINE_PROP_END_OF_LIST(),
+};
+
static void grlib_irqmp_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
+dc->realize = grlib_irqmp_realize;
dc->reset = grlib_irqmp_reset;
+device_class_set_props(dc, grlib_irqmp_properties);
}
static const TypeInfo grlib_irqmp_info = {
.name = TYPE_GRLIB_IRQMP,
.parent= TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(IRQMP),
-.instance_init = grlib_irqmp_init,
.class_init= grlib_irqmp_class_init,
};
diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index b7d81c76f3..b72761b959 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -265,11 +265,11 @@ static void leon3_generic_hw_init(MachineState *machine)
/* Allocate IRQ manager */
irqmpdev = qdev_new(TYPE_GRLIB_IRQMP);
+sysbus_realize_and_unref(SYS_BUS_DEVICE(irqmpdev), &error_fatal);
qdev_init_gpio_in_named_with_opaque(DEVICE(cpu), leon3_set_pil_in,
env, "pil", 1);
qdev_connect_gpio_out_named(irqmpdev, "grlib-irq", 0,
qdev_get_gpio_in_named(DEVICE(cpu), "pil", 0));
-sysbus_realize_and_unref(SYS_BUS_DEVICE(irqmpdev), &error_fatal);
sysbus_mmio_map(SYS_BUS_DEVICE(irqmpdev), 0, LEON3_IRQMP_OFFSET);
env->irq_manager = irqmpdev;
env->qemu_irq_ack = leon3_irq_manager;
--
2.25.1
[PATCH v3 6/9] leon3: remove SP initialization
According to the doc (see §4.2.15 in [1]), the reset operation should
not impact %SP.
[1] https://gaisler.com/doc/gr712rc-usermanual.pdf
Signed-off-by: Clément Chigot
---
hw/sparc/leon3.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index 7866f0a049..317eb57336 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -69,7 +69,6 @@
typedef struct ResetData {
SPARCCPU *cpu;
uint32_t entry;/* save kernel entry in case of reset */
-target_ulong sp;/* initial stack pointer */
} ResetData;
static uint32_t *gen_store_u32(uint32_t *code, hwaddr addr, uint32_t val)
@@ -136,7 +135,6 @@ static void main_cpu_reset(void *opaque)
cpu->halted = 0;
env->pc = s->entry;
env->npc= s->entry + 4;
-env->regbase[6] = s->sp;
}
static void leon3_cache_control_int(CPUSPARCState *env)
@@ -247,7 +245,6 @@ static void leon3_generic_hw_init(MachineState *machine)
/* Reset data */
reset_info= g_new0(ResetData, 1);
reset_info->cpu = cpu;
-reset_info->sp= LEON3_RAM_OFFSET + ram_size;
qemu_register_reset(main_cpu_reset, reset_info);
ahb_pnp = GRLIB_AHB_PNP(qdev_new(TYPE_GRLIB_AHB_PNP));
--
2.25.1
[PATCH v3 4/9] intc/grlib_irqmp: implements multicore irq
Now there is an ncpus property, use it in order to deliver the IRQ to
multiple CPU.
Co-developed-by: Frederic Konrad
Signed-off-by: Clément Chigot
Reviewed-by: Philippe Mathieu-Daudé
---
hw/intc/grlib_irqmp.c | 41 +--
hw/sparc/leon3.c | 3 ++-
include/hw/intc/grlib_irqmp.h | 2 +-
3 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/hw/intc/grlib_irqmp.c b/hw/intc/grlib_irqmp.c
index 1e073bd232..144b121d48 100644
--- a/hw/intc/grlib_irqmp.c
+++ b/hw/intc/grlib_irqmp.c
@@ -70,7 +70,7 @@ struct IRQMP {
unsigned int ncpus;
IRQMPState *state;
qemu_irq start_signal[IRQMP_MAX_CPU];
-qemu_irq irq;
+qemu_irq irq[IRQMP_MAX_CPU];
};
struct IRQMPState {
@@ -89,37 +89,35 @@ struct IRQMPState {
static void grlib_irqmp_check_irqs(IRQMPState *state)
{
-uint32_t pend = 0;
-uint32_t level0 = 0;
-uint32_t level1 = 0;
+int i;
assert(state != NULL);
assert(state->parent != NULL);
-/* IRQ for CPU 0 (no SMP support) */
-pend = (state->pending | state->force[0])
-& state->mask[0];
-
-level0 = pend & ~state->level;
-level1 = pend & state->level;
+for (i = 0; i < state->parent->ncpus; i++) {
+uint32_t pend = (state->pending | state->force[i]) & state->mask[i];
+uint32_t level0 = pend & ~state->level;
+uint32_t level1 = pend & state->level;
-trace_grlib_irqmp_check_irqs(state->pending, state->force[0],
- state->mask[0], level1, level0);
+trace_grlib_irqmp_check_irqs(state->pending, state->force[i],
+ state->mask[i], level1, level0);
-/* Trigger level1 interrupt first and level0 if there is no level1 */
-qemu_set_irq(state->parent->irq, level1 ?: level0);
+/* Trigger level1 interrupt first and level0 if there is no level1 */
+qemu_set_irq(state->parent->irq[i], level1 ?: level0);
+}
}
-static void grlib_irqmp_ack_mask(IRQMPState *state, uint32_t mask)
+static void grlib_irqmp_ack_mask(IRQMPState *state, unsigned int cpu,
+ uint32_t mask)
{
/* Clear registers */
state->pending &= ~mask;
-state->force[0] &= ~mask; /* Only CPU 0 (No SMP support) */
+state->force[cpu] &= ~mask;
grlib_irqmp_check_irqs(state);
}
-void grlib_irqmp_ack(DeviceState *dev, int intno)
+void grlib_irqmp_ack(DeviceState *dev, unsigned int cpu, int intno)
{
IRQMP*irqmp = GRLIB_IRQMP(dev);
IRQMPState *state;
@@ -133,7 +131,7 @@ void grlib_irqmp_ack(DeviceState *dev, int intno)
trace_grlib_irqmp_ack(intno);
-grlib_irqmp_ack_mask(state, mask);
+grlib_irqmp_ack_mask(state, cpu, mask);
}
static void grlib_irqmp_set_irq(void *opaque, int irq, int level)
@@ -159,7 +157,6 @@ static void grlib_irqmp_set_irq(void *opaque, int irq, int
level)
s->pending |= 1 << irq;
}
grlib_irqmp_check_irqs(s);
-
}
}
@@ -263,7 +260,9 @@ static void grlib_irqmp_write(void *opaque, hwaddr addr,
case CLEAR_OFFSET:
value &= ~1; /* clean up the value */
-grlib_irqmp_ack_mask(state, value);
+for (i = 0; i < irqmp->ncpus; i++) {
+grlib_irqmp_ack_mask(state, i, value);
+}
return;
case MP_STATUS_OFFSET:
@@ -367,7 +366,7 @@ static void grlib_irqmp_realize(DeviceState *dev, Error
**errp)
*/
qdev_init_gpio_out_named(dev, irqmp->start_signal, "grlib-start-cpu",
IRQMP_MAX_CPU);
-qdev_init_gpio_out_named(dev, &irqmp->irq, "grlib-irq", 1);
+qdev_init_gpio_out_named(dev, irqmp->irq, "grlib-irq", irqmp->ncpus);
memory_region_init_io(&irqmp->iomem, OBJECT(dev), &grlib_irqmp_ops, irqmp,
"irqmp", IRQMP_REG_SIZE);
diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index b72761b959..7866f0a049 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -170,7 +170,8 @@ static void leon3_cache_control_int(CPUSPARCState *env)
static void leon3_irq_ack(CPUSPARCState *env, int intno)
{
-grlib_irqmp_ack(env->irq_manager, intno);
+/* No SMP support yet, only CPU #0 available so far. */
+grlib_irqmp_ack(env->irq_manager, 0, intno);
}
/*
diff --git a/include/hw/intc/grlib_irqmp.h b/include/hw/intc/grlib_irqmp.h
index c5a90cbb3e..a76acbf940 100644
--- a/include/hw/intc/grlib_irqmp.h
+++ b/include/hw/intc/grlib_irqmp.h
@@ -36,6 +36,6 @@
/* IRQMP */
#define TYPE_GRLIB_IRQMP "grlib-irqmp"
-void grlib_irqmp_ack(DeviceState *dev, int intno);
+void grlib_irqmp_ack(DeviceState *dev, unsigned int cpu, int intno);
#endif /* GRLIB_IRQMP_H */
--
2.25.1
[PATCH v3 7/9] leon3: implement multiprocessor
This allows to register more than one CPU on the leon3_generic machine.
Co-developed-by: Frederic Konrad
Signed-off-by: Clément Chigot
---
hw/sparc/leon3.c | 98 ++--
1 file changed, 70 insertions(+), 28 deletions(-)
diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index 317eb57336..252aff72cd 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -54,6 +54,8 @@
#define LEON3_PROM_OFFSET(0x)
#define LEON3_RAM_OFFSET (0x4000)
+#define MAX_CPUS 4
+
#define LEON3_UART_OFFSET (0x8100)
#define LEON3_UART_IRQ (3)
@@ -67,8 +69,11 @@
#define LEON3_AHB_PNP_OFFSET (0xF000)
typedef struct ResetData {
-SPARCCPU *cpu;
-uint32_t entry;/* save kernel entry in case of reset */
+struct CPUResetData {
+int id;
+SPARCCPU *cpu;
+} info[MAX_CPUS];
+uint32_t entry; /* save kernel entry in case of reset */
} ResetData;
static uint32_t *gen_store_u32(uint32_t *code, hwaddr addr, uint32_t val)
@@ -124,17 +129,19 @@ static void write_bootloader(CPUSPARCState *env, uint8_t
*base,
stl_p(p++, 0x0100); /* nop */
}
-static void main_cpu_reset(void *opaque)
+static void leon3_cpu_reset(void *opaque)
{
-ResetData *s = (ResetData *)opaque;
-CPUState *cpu = CPU(s->cpu);
-CPUSPARCState *env = &s->cpu->env;
+struct CPUResetData *info = (struct CPUResetData *) opaque;
+int id = info->id;
+ResetData *s = (ResetData *)DO_UPCAST(ResetData, info[id], info);
+CPUState *cpu = CPU(s->info[id].cpu);
+CPUSPARCState *env = cpu_env(cpu);
cpu_reset(cpu);
-cpu->halted = 0;
-env->pc = s->entry;
-env->npc= s->entry + 4;
+cpu->halted = cpu->cpu_index != 0;
+env->pc = s->entry;
+env->npc = s->entry + 4;
}
static void leon3_cache_control_int(CPUSPARCState *env)
@@ -168,8 +175,8 @@ static void leon3_cache_control_int(CPUSPARCState *env)
static void leon3_irq_ack(CPUSPARCState *env, int intno)
{
-/* No SMP support yet, only CPU #0 available so far. */
-grlib_irqmp_ack(env->irq_manager, 0, intno);
+CPUState *cpu = CPU(env_cpu(env));
+grlib_irqmp_ack(env->irq_manager, cpu->cpu_index, intno);
}
/*
@@ -211,6 +218,19 @@ static void leon3_set_pil_in(void *opaque, int n, int
level)
}
}
+static void leon3_start_cpu_async_work(CPUState *cpu, run_on_cpu_data data)
+{
+cpu->halted = 0;
+}
+
+static void leon3_start_cpu(void *opaque, int n, int level)
+{
+CPUState *cs = CPU(opaque);
+
+assert(level == 1);
+async_run_on_cpu(cs, leon3_start_cpu_async_work, RUN_ON_CPU_NULL);
+}
+
static void leon3_irq_manager(CPUSPARCState *env, int intno)
{
leon3_irq_ack(env, intno);
@@ -236,16 +256,20 @@ static void leon3_generic_hw_init(MachineState *machine)
AHBPnp *ahb_pnp;
APBPnp *apb_pnp;
-/* Init CPU */
-cpu = SPARC_CPU(cpu_create(machine->cpu_type));
-env = &cpu->env;
+reset_info = g_malloc0(sizeof(ResetData));
-cpu_sparc_set_id(env, 0);
+for (i = 0; i < machine->smp.cpus; i++) {
+/* Init CPU */
+cpu = SPARC_CPU(cpu_create(machine->cpu_type));
+env = &cpu->env;
-/* Reset data */
-reset_info= g_new0(ResetData, 1);
-reset_info->cpu = cpu;
-qemu_register_reset(main_cpu_reset, reset_info);
+cpu_sparc_set_id(env, i);
+
+/* Reset data */
+reset_info->info[i].id = i;
+reset_info->info[i].cpu = cpu;
+qemu_register_reset(leon3_cpu_reset, &reset_info->info[i]);
+}
ahb_pnp = GRLIB_AHB_PNP(qdev_new(TYPE_GRLIB_AHB_PNP));
sysbus_realize_and_unref(SYS_BUS_DEVICE(ahb_pnp), &error_fatal);
@@ -263,14 +287,28 @@ static void leon3_generic_hw_init(MachineState *machine)
/* Allocate IRQ manager */
irqmpdev = qdev_new(TYPE_GRLIB_IRQMP);
+object_property_set_int(OBJECT(irqmpdev), "ncpus", machine->smp.cpus,
+&error_fatal);
sysbus_realize_and_unref(SYS_BUS_DEVICE(irqmpdev), &error_fatal);
-qdev_init_gpio_in_named_with_opaque(DEVICE(cpu), leon3_set_pil_in,
-env, "pil", 1);
-qdev_connect_gpio_out_named(irqmpdev, "grlib-irq", 0,
-qdev_get_gpio_in_named(DEVICE(cpu), "pil", 0));
+
+for (i = 0; i < machine->smp.cpus; i++) {
+cpu = reset_info->info[i].cpu;
+env = &cpu->env;
+qdev_init_gpio_in_named_with_opaque(DEVICE(cpu), leon3_start_cpu,
+cpu, "start_cpu", 1);
+qdev_connect_gpio_out_named(irqmpdev, "grlib-start-cpu", i,
+qdev_get_gpio_in_named(DEVICE(cpu),
+ "start_cpu
[PATCH v3 0/9] sparc/leon3: Add support for -smp
V3 modifications
- Patch 3: Fix indentation
- Patch 4: Fix types and improves variable declarations
- Patch 6 (NEW): Remove SP initialization in leon3
- Patch 7: Add assert in leon3_start_cpu
- Patch 8: Improve comment
---
V2 modifications
- Patch1: Add SPDX copyright tags.
- Patch3: Add defines for MP_STATUS fields. Improve comments.
- Patch4: Improve a comment.
- Patch6: Dropped as already merged.
---
This series allows leon3 emulations to record up 4 CPUs.
It requires some enhancements in the grlib_irqmp device and adding the
cpu_index field in the asr17 instruction.
It has been tested locally with various bareboard runtimes.
CC: Frederic Konrad (maintainer:Leon3)
CC: Mark Cave-Ayland (maintainer:SPARC TCG CPUs)
CC: Artyom Tarasenko (maintainer:SPARC TCG CPUs)
CC: Philippe Mathieu-Daudé
Clément Chigot (9):
sparc/grlib: split out the headers for each peripherals
intc/grlib_irqmp: add ncpus property
intc/grlib_irqmp: implements the multiprocessor status register
intc/grlib_irqmp: implements multicore irq
target/sparc: implement asr17 feature for smp
leon3: remove SP initialization
leon3: implement multiprocessor
leon3: check cpu_id in the tiny bootloader
MAINTAINERS: replace Fabien by myself as Leon3 maintainer
MAINTAINERS | 2 +-
hw/char/grlib_apbuart.c | 6 +-
hw/intc/grlib_irqmp.c | 110 ++-
hw/sparc/leon3.c | 130 +-
hw/timer/grlib_gptimer.c | 6 +-
include/hw/char/grlib_uart.h | 32 +
.../hw/{sparc/grlib.h => intc/grlib_irqmp.h} | 18 +--
include/hw/timer/grlib_gptimer.h | 32 +
target/sparc/helper.c | 16 +++
target/sparc/helper.h | 1 +
target/sparc/translate.c | 13 +-
11 files changed, 272 insertions(+), 94 deletions(-)
create mode 100644 include/hw/char/grlib_uart.h
rename include/hw/{sparc/grlib.h => intc/grlib_irqmp.h} (83%)
create mode 100644 include/hw/timer/grlib_gptimer.h
--
2.25.1
[PATCH v3 5/9] target/sparc: implement asr17 feature for smp
This allows the guest program to know its cpu id.
Co-developed-by: Frederic Konrad
Signed-off-by: Clément Chigot
Reviewed-by: Richard Henderson
Reviewed-by: Philippe Mathieu-Daudé
---
target/sparc/helper.c| 16
target/sparc/helper.h| 1 +
target/sparc/translate.c | 13 +++--
3 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/target/sparc/helper.c b/target/sparc/helper.c
index bd10b60e4b..2247e243b5 100644
--- a/target/sparc/helper.c
+++ b/target/sparc/helper.c
@@ -212,4 +212,20 @@ void helper_power_down(CPUSPARCState *env)
env->npc = env->pc + 4;
cpu_loop_exit(cs);
}
+
+target_ulong helper_rdasr17(CPUSPARCState *env)
+{
+CPUState *cs = env_cpu(env);
+target_ulong val;
+
+/*
+ * TODO: There are many more fields to be filled,
+ * some of which are writable.
+ */
+val = env->def.nwindows - 1;/* [4:0] NWIN */
+val |= 1 << 8; /* [8] V8*/
+val |= (cs->cpu_index) << 28; /* [31:28] INDEX */
+
+return val;
+}
#endif
diff --git a/target/sparc/helper.h b/target/sparc/helper.h
index 55eff66283..fc818b8678 100644
--- a/target/sparc/helper.h
+++ b/target/sparc/helper.h
@@ -2,6 +2,7 @@
DEF_HELPER_1(rett, void, env)
DEF_HELPER_2(wrpsr, void, env, tl)
DEF_HELPER_1(rdpsr, tl, env)
+DEF_HELPER_1(rdasr17, tl, env)
DEF_HELPER_1(power_down, void, env)
#else
DEF_HELPER_FLAGS_2(wrpil, TCG_CALL_NO_RWG, void, env, tl)
diff --git a/target/sparc/translate.c b/target/sparc/translate.c
index 9387299559..1cabda9565 100644
--- a/target/sparc/translate.c
+++ b/target/sparc/translate.c
@@ -37,6 +37,7 @@
#ifdef TARGET_SPARC64
# define gen_helper_rdpsr(D, E) qemu_build_not_reached()
+# define gen_helper_rdasr17(D, E) qemu_build_not_reached()
# define gen_helper_rett(E) qemu_build_not_reached()
# define gen_helper_power_down(E) qemu_build_not_reached()
# define gen_helper_wrpsr(E, S) qemu_build_not_reached()
@@ -2681,16 +2682,8 @@ static bool trans_RDY(DisasContext *dc, arg_RDY *a)
static TCGv do_rd_leon3_config(DisasContext *dc, TCGv dst)
{
-uint32_t val;
-
-/*
- * TODO: There are many more fields to be filled,
- * some of which are writable.
- */
-val = dc->def->nwindows - 1; /* [4:0] NWIN */
-val |= 1 << 8; /* [8] V8 */
-
-return tcg_constant_tl(val);
+gen_helper_rdasr17(dst, tcg_env);
+return dst;
}
TRANS(RDASR17, ASR17, do_rd_special, true, a->rd, do_rd_leon3_config)
--
2.25.1
[PATCH v3 3/9] intc/grlib_irqmp: implements the multiprocessor status register
This implements the multiprocessor status register in grlib-irqmp and bind
it to a start signal, which will be later wired in leon3-generic to
start a cpu.
The EIRQ and BA bits are not implemented.
Based on https://gaisler.com/doc/gr712rc-usermanual.pdf, §8.3.5.
Co-developed-by: Frederic Konrad
Signed-off-by: Clément Chigot
Reviewed-by: Philippe Mathieu-Daudé
---
hw/intc/grlib_irqmp.c | 35 ---
1 file changed, 32 insertions(+), 3 deletions(-)
diff --git a/hw/intc/grlib_irqmp.c b/hw/intc/grlib_irqmp.c
index 744cd64c58..1e073bd232 100644
--- a/hw/intc/grlib_irqmp.c
+++ b/hw/intc/grlib_irqmp.c
@@ -52,6 +52,10 @@
#define FORCE_OFFSET 0x80
#define EXTENDED_OFFSET 0xC0
+/* Multiprocessor Status Register */
+#define MP_STATUS_CPU_STATUS_MASK ((1 << IRQMP_MAX_CPU)-2)
+#define MP_STATUS_NCPU_SHIFT 28
+
#define MAX_PILS 16
OBJECT_DECLARE_SIMPLE_TYPE(IRQMP, GRLIB_IRQMP)
@@ -65,6 +69,7 @@ struct IRQMP {
unsigned int ncpus;
IRQMPState *state;
+qemu_irq start_signal[IRQMP_MAX_CPU];
qemu_irq irq;
};
@@ -72,6 +77,7 @@ struct IRQMPState {
uint32_t level;
uint32_t pending;
uint32_t clear;
+uint32_t mpstatus;
uint32_t broadcast;
uint32_t mask[IRQMP_MAX_CPU];
@@ -182,10 +188,12 @@ static uint64_t grlib_irqmp_read(void *opaque, hwaddr
addr,
return state->force[0];
case CLEAR_OFFSET:
-case MP_STATUS_OFFSET:
/* Always read as 0 */
return 0;
+case MP_STATUS_OFFSET:
+return state->mpstatus;
+
case BROADCAST_OFFSET:
return state->broadcast;
@@ -224,8 +232,9 @@ static uint64_t grlib_irqmp_read(void *opaque, hwaddr addr,
static void grlib_irqmp_write(void *opaque, hwaddr addr,
uint64_t value, unsigned size)
{
-IRQMP *irqmp = opaque;
+IRQMP *irqmp = opaque;
IRQMPState *state;
+int i;
assert(irqmp != NULL);
state = irqmp->state;
@@ -258,7 +267,18 @@ static void grlib_irqmp_write(void *opaque, hwaddr addr,
return;
case MP_STATUS_OFFSET:
-/* Read Only (no SMP support) */
+/*
+ * Writing and reading operations are reversed for the CPU status.
+ * Writing "1" will start the CPU, but reading "1" means that the CPU
+ * is power-down.
+ */
+value &= MP_STATUS_CPU_STATUS_MASK;
+for (i = 0; i < irqmp->ncpus; i++) {
+if ((value >> i) & 1) {
+qemu_set_irq(irqmp->start_signal[i], 1);
+state->mpstatus &= ~(1 << i);
+}
+}
return;
case BROADCAST_OFFSET:
@@ -325,6 +345,8 @@ static void grlib_irqmp_reset(DeviceState *d)
memset(irqmp->state, 0, sizeof *irqmp->state);
irqmp->state->parent = irqmp;
+irqmp->state->mpstatus = ((irqmp->ncpus - 1) << MP_STATUS_NCPU_SHIFT) |
+((1 << irqmp->ncpus) - 2);
}
static void grlib_irqmp_realize(DeviceState *dev, Error **errp)
@@ -338,6 +360,13 @@ static void grlib_irqmp_realize(DeviceState *dev, Error
**errp)
}
qdev_init_gpio_in(dev, grlib_irqmp_set_irq, MAX_PILS);
+
+/*
+ * Transitionning from 0 to 1 starts the CPUs. The opposite can't
+ * happen.
+ */
+qdev_init_gpio_out_named(dev, irqmp->start_signal, "grlib-start-cpu",
+ IRQMP_MAX_CPU);
qdev_init_gpio_out_named(dev, &irqmp->irq, "grlib-irq", 1);
memory_region_init_io(&irqmp->iomem, OBJECT(dev), &grlib_irqmp_ops, irqmp,
"irqmp", IRQMP_REG_SIZE);
--
2.25.1
[PATCH v3 8/9] leon3: check cpu_id in the tiny bootloader
Now that SMP is possible, the asr17 must be checked in the little boot code
or the secondary CPU will reinitialize the Timer and the Uart.
Co-developed-by: Frederic Konrad
Signed-off-by: Clément Chigot
Reviewed-by: Philippe Mathieu-Daudé
---
hw/sparc/leon3.c | 22 --
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index 252aff72cd..798dba1e5a 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -99,13 +99,27 @@ static uint32_t *gen_store_u32(uint32_t *code, hwaddr addr,
uint32_t val)
/*
* When loading a kernel in RAM the machine is expected to be in a different
- * state (eg: initialized by the bootloader). This little code reproduces
- * this behavior.
+ * state (eg: initialized by the bootloader). This little code reproduces
+ * this behavior. Also this code can be executed by the secondary cpus as
+ * well since it looks at the %asr17 register before doing any
+ * initialization, it allows to use the same reset address for all the
+ * cpus.
*/
static void write_bootloader(CPUSPARCState *env, uint8_t *base,
hwaddr kernel_addr)
{
uint32_t *p = (uint32_t *) base;
+uint32_t *sec_cpu_branch_p = NULL;
+
+/* If we are running on a secondary CPU, jump directly to the kernel. */
+
+stl_p(p++, 0x85444000); /* rd %asr17, %g2 */
+stl_p(p++, 0x8530a01c); /* srl %g2, 0x1c, %g2 */
+stl_p(p++, 0x80908000); /* tst %g2*/
+/* Filled below. */
+sec_cpu_branch_p = p;
+stl_p(p++, 0x0BADC0DE); /* bne xxx */
+stl_p(p++, 0x0100); /* nop */
/* Initialize the UARTs*/
/* *UART_CONTROL = UART_RECEIVE_ENABLE | UART_TRANSMIT_ENABLE; */
@@ -119,6 +133,10 @@ static void write_bootloader(CPUSPARCState *env, uint8_t
*base,
/* *GPTIMER0_CONFIG = GPTIMER_ENABLE | GPTIMER_RESTART;*/
p = gen_store_u32(p, 0x8318, 3);
+/* Now, the relative branch above can be computed. */
+stl_p(sec_cpu_branch_p, 0x1280
+ + (p - sec_cpu_branch_p));
+
/* JUMP to the entry point */
stl_p(p++, 0x8210); /* mov %g0, %g1 */
stl_p(p++, 0x0300 + extract32(kernel_addr, 10, 22));
--
2.25.1
[PATCH v3 9/9] MAINTAINERS: replace Fabien by myself as Leon3 maintainer
CC: Fabien Chouteau Signed-off-by: Clément Chigot Reviewed-by: Fabien Chouteau --- MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index dfaca8323e..f076c97fcb 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1708,7 +1708,7 @@ F: hw/rtc/sun4v-rtc.c F: include/hw/rtc/sun4v-rtc.h Leon3 -M: Fabien Chouteau +M: Clément Chigot M: Frederic Konrad S: Maintained F: hw/sparc/leon3.c -- 2.25.1
[PATCH v3 1/9] sparc/grlib: split out the headers for each peripherals
... and move them in their right hardware directory.
Update Copyright and add SPDX-License-Identifier at the same time.
Co-developed-by: Frederic Konrad
Signed-off-by: Clément Chigot
Reviewed-by: Philippe Mathieu-Daudé
---
hw/char/grlib_apbuart.c | 6 ++--
hw/intc/grlib_irqmp.c | 6 ++--
hw/sparc/leon3.c | 8 +++--
hw/timer/grlib_gptimer.c | 6 ++--
include/hw/char/grlib_uart.h | 32 +++
.../hw/{sparc/grlib.h => intc/grlib_irqmp.h} | 16 --
include/hw/timer/grlib_gptimer.h | 32 +++
7 files changed, 88 insertions(+), 18 deletions(-)
create mode 100644 include/hw/char/grlib_uart.h
rename include/hw/{sparc/grlib.h => intc/grlib_irqmp.h} (86%)
create mode 100644 include/hw/timer/grlib_gptimer.h
diff --git a/hw/char/grlib_apbuart.c b/hw/char/grlib_apbuart.c
index 82ff40a530..515b65bc07 100644
--- a/hw/char/grlib_apbuart.c
+++ b/hw/char/grlib_apbuart.c
@@ -1,7 +1,9 @@
/*
* QEMU GRLIB APB UART Emulator
*
- * Copyright (c) 2010-2019 AdaCore
+ * SPDX-License-Identifier: MIT
+ *
+ * Copyright (c) 2010-2024 AdaCore
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to
deal
@@ -26,7 +28,7 @@
#include "hw/irq.h"
#include "hw/qdev-properties.h"
#include "hw/qdev-properties-system.h"
-#include "hw/sparc/grlib.h"
+#include "hw/char/grlib_uart.h"
#include "hw/sysbus.h"
#include "qemu/module.h"
#include "chardev/char-fe.h"
diff --git a/hw/intc/grlib_irqmp.c b/hw/intc/grlib_irqmp.c
index 3bfe2544b7..11eef62457 100644
--- a/hw/intc/grlib_irqmp.c
+++ b/hw/intc/grlib_irqmp.c
@@ -3,7 +3,9 @@
*
* (Multiprocessor and extended interrupt not supported)
*
- * Copyright (c) 2010-2019 AdaCore
+ * SPDX-License-Identifier: MIT
+ *
+ * Copyright (c) 2010-2024 AdaCore
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to
deal
@@ -29,7 +31,7 @@
#include "hw/sysbus.h"
#include "hw/qdev-properties.h"
-#include "hw/sparc/grlib.h"
+#include "hw/intc/grlib_irqmp.h"
#include "trace.h"
#include "qapi/error.h"
diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index 2dfb742566..b7d81c76f3 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -1,7 +1,9 @@
/*
* QEMU Leon3 System Emulator
*
- * Copyright (c) 2010-2019 AdaCore
+ * SPDX-License-Identifier: MIT
+ *
+ * Copyright (c) 2010-2024 AdaCore
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to
deal
@@ -40,7 +42,9 @@
#include "elf.h"
#include "trace.h"
-#include "hw/sparc/grlib.h"
+#include "hw/timer/grlib_gptimer.h"
+#include "hw/char/grlib_uart.h"
+#include "hw/intc/grlib_irqmp.h"
#include "hw/misc/grlib_ahb_apb_pnp.h"
/* Default system clock. */
diff --git a/hw/timer/grlib_gptimer.c b/hw/timer/grlib_gptimer.c
index 5c4923c1e0..4990885451 100644
--- a/hw/timer/grlib_gptimer.c
+++ b/hw/timer/grlib_gptimer.c
@@ -1,7 +1,9 @@
/*
* QEMU GRLIB GPTimer Emulator
*
- * Copyright (c) 2010-2019 AdaCore
+ * SPDX-License-Identifier: MIT
+ *
+ * Copyright (c) 2010-2024 AdaCore
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to
deal
@@ -23,7 +25,7 @@
*/
#include "qemu/osdep.h"
-#include "hw/sparc/grlib.h"
+#include "hw/timer/grlib_gptimer.h"
#include "hw/sysbus.h"
#include "qemu/timer.h"
#include "hw/irq.h"
diff --git a/include/hw/char/grlib_uart.h b/include/hw/char/grlib_uart.h
new file mode 100644
index 00..7496f8fd5e
--- /dev/null
+++ b/include/hw/char/grlib_uart.h
@@ -0,0 +1,32 @@
+/*
+ * QEMU GRLIB UART
+ *
+ * SPDX-License-Identifier: MIT
+ *
+ * Copyright (c) 2024 AdaCore
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS&
Re: [PATCH 2/2] hw/sparc/leon3: Remove duplicated code
On Tue, Jan 30, 2024 at 12:31 PM Philippe Mathieu-Daudé
wrote:
>
> Since commit b04d989054 ("SPARC: Emulation of Leon3") the
> main_cpu_reset() handler sets both pc/npc when the CPU is
> reset, after the machine is realized. It is pointless to
> set it in leon3_generic_hw_init().
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/sparc/leon3.c | 2 --
> 1 file changed, 2 deletions(-)
Thanks for those cleanups !
Reviewed-by: Clément Chigot
> diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
> index 2dfb742566..1ae9a37583 100644
> --- a/hw/sparc/leon3.c
> +++ b/hw/sparc/leon3.c
> @@ -343,8 +343,6 @@ static void leon3_generic_hw_init(MachineState *machine)
>
> bootloader_entry = memory_region_get_ram_ptr(prom);
> write_bootloader(env, bootloader_entry, entry);
> -env->pc = LEON3_PROM_OFFSET;
> -env->npc = LEON3_PROM_OFFSET + 4;
> reset_info->entry = LEON3_PROM_OFFSET;
> }
> }
> --
> 2.41.0
>
Re: [PATCH 1/2] target/sparc: Provide hint about CPUSPARCState::irq_manager member
On Tue, Jan 30, 2024 at 12:31 PM Philippe Mathieu-Daudé
wrote:
>
> CPUSPARCState::irq_manager holds a pointer to a QDev,
> so declare it as DeviceState instead of void.
>
> Move the comment about Leon3 fields.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> target/sparc/cpu.h | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
Reviewed-by: Clément Chigot
> diff --git a/target/sparc/cpu.h b/target/sparc/cpu.h
> index 12a11ecb26..d02684569a 100644
> --- a/target/sparc/cpu.h
> +++ b/target/sparc/cpu.h
> @@ -548,10 +548,9 @@ struct CPUArchState {
> #endif
> sparc_def_t def;
>
> -void *irq_manager;
> +/* Leon3 */
> +DeviceState *irq_manager;
> void (*qemu_irq_ack)(CPUSPARCState *env, int intno);
> -
> -/* Leon3 cache control */
> uint32_t cache_control;
> };
>
> --
> 2.41.0
>
Re: [PATCH v2 6/8] leon3: implement multiprocessor
On Tue, Jan 30, 2024 at 12:43 PM Philippe Mathieu-Daudé
wrote:
>
> Hi Clément,
>
> On 16/1/24 14:02, Clément Chigot wrote:
> > This allows to register more than one CPU on the leon3_generic machine.
> >
> > Co-developed-by: Frederic Konrad
> > Signed-off-by: Clément Chigot
> > ---
> > hw/sparc/leon3.c | 106 +--
> > 1 file changed, 74 insertions(+), 32 deletions(-)
> >
> > diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
> > index 7866f0a049..eacd85ee4f 100644
> > --- a/hw/sparc/leon3.c
> > +++ b/hw/sparc/leon3.c
> > @@ -54,6 +54,8 @@
> > #define LEON3_PROM_OFFSET(0x)
> > #define LEON3_RAM_OFFSET (0x4000)
> >
> > +#define MAX_CPUS 4
> > +
> > #define LEON3_UART_OFFSET (0x8100)
> > #define LEON3_UART_IRQ (3)
> >
> > @@ -67,9 +69,12 @@
> > #define LEON3_AHB_PNP_OFFSET (0xF000)
> >
> > typedef struct ResetData {
> > -SPARCCPU *cpu;
> > -uint32_t entry;/* save kernel entry in case of reset */
> > -target_ulong sp;/* initial stack pointer */
> > +struct CPUResetData {
> > +int id;
> > +SPARCCPU *cpu;
> > +target_ulong sp; /* initial stack pointer */
> > +} info[MAX_CPUS];
> > +uint32_t entry; /* save kernel entry in case of reset */
> > } ResetData;
> >
> > static uint32_t *gen_store_u32(uint32_t *code, hwaddr addr, uint32_t val)
> > @@ -125,18 +130,19 @@ static void write_bootloader(CPUSPARCState *env,
> > uint8_t *base,
> > stl_p(p++, 0x0100); /* nop */
> > }
> >
> > -static void main_cpu_reset(void *opaque)
> > +static void leon3_cpu_reset(void *opaque)
> > {
> > -ResetData *s = (ResetData *)opaque;
> > -CPUState *cpu = CPU(s->cpu);
> > -CPUSPARCState *env = &s->cpu->env;
> > +struct CPUResetData *info = (struct CPUResetData *) opaque;
> > +int id = info->id;
> > +ResetData *s = (ResetData *)DO_UPCAST(ResetData, info[id], info);
> > +CPUState *cpu = CPU(s->info[id].cpu);
> > +CPUSPARCState *env = cpu_env(cpu);
> >
> > cpu_reset(cpu);
> > -
> > -cpu->halted = 0;
> > -env->pc = s->entry;
> > -env->npc= s->entry + 4;
> > -env->regbase[6] = s->sp;
> > +cpu->halted = cpu->cpu_index != 0;
> > +env->pc = s->entry;
> > +env->npc = s->entry + 4;
> > +env->regbase[6] = s->info[id].sp;
>
> You take care to initialize with different stack, ...
>
> > }
> >
> > static void leon3_cache_control_int(CPUSPARCState *env)
> > @@ -170,8 +176,8 @@ static void leon3_cache_control_int(CPUSPARCState *env)
> >
> > static void leon3_irq_ack(CPUSPARCState *env, int intno)
> > {
> > -/* No SMP support yet, only CPU #0 available so far. */
> > -grlib_irqmp_ack(env->irq_manager, 0, intno);
> > +CPUState *cpu = CPU(env_cpu(env));
> > +grlib_irqmp_ack(env->irq_manager, cpu->cpu_index, intno);
> > }
> >
> > /*
> > @@ -213,6 +219,20 @@ static void leon3_set_pil_in(void *opaque, int n, int
> > level)
> > }
> > }
> >
> > +static void leon3_start_cpu_async_work(CPUState *cpu, run_on_cpu_data data)
> > +{
> > +cpu->halted = 0;
> > +}
> > +
> > +static void leon3_start_cpu(void *opaque, int n, int level)
> > +{
> > +CPUState *cs = CPU(opaque);
> > +
> > +if (level) {
> > +async_run_on_cpu(cs, leon3_start_cpu_async_work, RUN_ON_CPU_NULL);
> > +}
> > +}
> > +
> > static void leon3_irq_manager(CPUSPARCState *env, int intno)
> > {
> > leon3_irq_ack(env, intno);
> > @@ -238,17 +258,21 @@ static void leon3_generic_hw_init(MachineState
> > *machine)
> > AHBPnp *ahb_pnp;
> > APBPnp *apb_pnp;
> >
> > -/* Init CPU */
> > -cpu = SPARC_CPU(cpu_create(machine->cpu_type));
> > -env = &cpu->env;
> > +reset_info = g_malloc0(sizeof(ResetData));
> >
> > -cpu_sparc_set_id(env, 0);
> > +for (i = 0; i < machine->smp.cpus; i++) {
> > +/* Init CPU */
> > +cpu = SPARC_CPU(cpu_create(machine->cpu_type));
> > +env = &cpu->env;
> >
> > -/* Reset data */
> > -reset_info= g_new0(ResetData, 1);
> > -reset_info->
Re: [PATCH v2 7/8] leon3: check cpu_id in the tiny bootloader
On Tue, Jan 30, 2024 at 10:15 AM Philippe Mathieu-Daudé
wrote:
>
> On 16/1/24 14:02, Clément Chigot wrote:
> > Now that SMP is possible, the asr17 must be checked in the little boot code
> > or the secondary CPU will reinitialize the Timer and the Uart.
> >
> > Co-developed-by: Frederic Konrad
> > Signed-off-by: Clément Chigot
> > ---
> > hw/sparc/leon3.c | 22 --
> > 1 file changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
> > index eacd85ee4f..87a8044a3e 100644
> > --- a/hw/sparc/leon3.c
> > +++ b/hw/sparc/leon3.c
> > @@ -100,13 +100,27 @@ static uint32_t *gen_store_u32(uint32_t *code, hwaddr
> > addr, uint32_t val)
> >
> > /*
> >* When loading a kernel in RAM the machine is expected to be in a
> > different
> > - * state (eg: initialized by the bootloader). This little code reproduces
> > - * this behavior.
> > + * state (eg: initialized by the bootloader). This little code reproduces
> > + * this behavior. Also this code can be executed by the secondary cpus as
> > + * well since it looks at the %asr17 register before doing any
> > + * initialization, it allows to use the same reset address for all the
> > + * cpus.
> >*/
> > static void write_bootloader(CPUSPARCState *env, uint8_t *base,
> >hwaddr kernel_addr)
> > {
> > uint32_t *p = (uint32_t *) base;
> > +uint32_t *sec_cpu_branch_p = NULL;
> > +
> > +/* If we are running on a secondary CPU, jump directly to the kernel.
> > */
> > +
> > +stl_p(p++, 0x85444000); /* rd %asr17, %g2 */
> > +stl_p(p++, 0x8530a01c); /* srl %g2, 0x1c, %g2 */
> > +stl_p(p++, 0x80908000); /* tst %g2*/
> > +/* Fill that later. */
>
>/* Filled below. */
>
> > +sec_cpu_branch_p = p;
> > +stl_p(p++, 0x0BADC0DE); /* bne xxx */
> > +stl_p(p++, 0x0100); /* nop */
> >
> > /* Initialize the UARTs*/
> > /* *UART_CONTROL = UART_RECEIVE_ENABLE | UART_TRANSMIT_ENABLE; */
> > @@ -120,6 +134,10 @@ static void write_bootloader(CPUSPARCState *env,
> > uint8_t *base,
> > /* *GPTIMER0_CONFIG = GPTIMER_ENABLE | GPTIMER_RESTART;*/
> > p = gen_store_u32(p, 0x8318, 3);
> >
> > +/* Now, the relative branch above can be computed. */
> > +stl_p(sec_cpu_branch_p, 0x1280
> > + + (p - sec_cpu_branch_p));
> > +
> > /* JUMP to the entry point */
> > stl_p(p++, 0x8210); /* mov %g0, %g1 */
> > stl_p(p++, 0x0300 + extract32(kernel_addr, 10, 22));
>
> Alternatively have main_cpu_reset / secondary_cpu_reset handlers.
> You could split BL in HWINIT / JUMP, have HWINIT() return # instr
> used and adjust secondary_cpu_reset entry.
Indeed that would make this code a bit easier to maintain. I'll keep
it for a future MR if you don't mind.
Thanks for the idea anyway.
> Anyway,
> Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v2 0/8] sparc/leon3: Add support for -smp
Hey Philippe,
Gentle ping on this serie
Thanks,
Clément
On Tue, Jan 16, 2024 at 2:02 PM Clément Chigot wrote:
>
> V2 modifications
> - Patch1: Add SPDX copyright tags.
> - Patch3: Add defines for MP_STATUS fields. Improve comments.
> - Patch4: Improve a comment.
> - Patch6: Dropped as already merged.
>
> ---
>
> This series allows leon3 emulations to record up 4 CPUs.
>
> It requires some enhancements in the grlib_irqmp device and adding the
> cpu_index field in the asr17 instruction.
>
> It has been tested locally with various bareboard runtimes.
>
>
> Clément Chigot (8):
> sparc/grlib: split out the headers for each peripherals
> intc/grlib_irqmp: add ncpus property
> intc/grlib_irqmp: implements the multiprocessor status register
> intc/grlib_irqmp: implements multicore irq
> target/sparc: implement asr17 feature for smp
> leon3: implement multiprocessor
> leon3: check cpu_id in the tiny bootloader
> MAINTAINERS: replace Fabien by myself as Leon3 maintainer
>
> MAINTAINERS | 2 +-
> hw/char/grlib_apbuart.c | 6 +-
> hw/intc/grlib_irqmp.c | 112 ++-
> hw/sparc/leon3.c | 135 +-
> hw/timer/grlib_gptimer.c | 6 +-
> include/hw/char/grlib_uart.h | 32 +
> .../hw/{sparc/grlib.h => intc/grlib_irqmp.h} | 18 +--
> include/hw/timer/grlib_gptimer.h | 32 +
> target/sparc/helper.c | 16 +++
> target/sparc/helper.h | 1 +
> target/sparc/translate.c | 13 +-
> 11 files changed, 278 insertions(+), 95 deletions(-)
> create mode 100644 include/hw/char/grlib_uart.h
> rename include/hw/{sparc/grlib.h => intc/grlib_irqmp.h} (83%)
> create mode 100644 include/hw/timer/grlib_gptimer.h
>
> --
> 2.25.1
>
[PATCH v2 5/8] target/sparc: implement asr17 feature for smp
This allows the guest program to know its cpu id.
Co-developed-by: Frederic Konrad
Signed-off-by: Clément Chigot
Reviewed-by: Richard Henderson
---
target/sparc/helper.c| 16
target/sparc/helper.h| 1 +
target/sparc/translate.c | 13 +++--
3 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/target/sparc/helper.c b/target/sparc/helper.c
index bd10b60e4b..2247e243b5 100644
--- a/target/sparc/helper.c
+++ b/target/sparc/helper.c
@@ -212,4 +212,20 @@ void helper_power_down(CPUSPARCState *env)
env->npc = env->pc + 4;
cpu_loop_exit(cs);
}
+
+target_ulong helper_rdasr17(CPUSPARCState *env)
+{
+CPUState *cs = env_cpu(env);
+target_ulong val;
+
+/*
+ * TODO: There are many more fields to be filled,
+ * some of which are writable.
+ */
+val = env->def.nwindows - 1;/* [4:0] NWIN */
+val |= 1 << 8; /* [8] V8*/
+val |= (cs->cpu_index) << 28; /* [31:28] INDEX */
+
+return val;
+}
#endif
diff --git a/target/sparc/helper.h b/target/sparc/helper.h
index 55eff66283..fc818b8678 100644
--- a/target/sparc/helper.h
+++ b/target/sparc/helper.h
@@ -2,6 +2,7 @@
DEF_HELPER_1(rett, void, env)
DEF_HELPER_2(wrpsr, void, env, tl)
DEF_HELPER_1(rdpsr, tl, env)
+DEF_HELPER_1(rdasr17, tl, env)
DEF_HELPER_1(power_down, void, env)
#else
DEF_HELPER_FLAGS_2(wrpil, TCG_CALL_NO_RWG, void, env, tl)
diff --git a/target/sparc/translate.c b/target/sparc/translate.c
index 9387299559..1cabda9565 100644
--- a/target/sparc/translate.c
+++ b/target/sparc/translate.c
@@ -37,6 +37,7 @@
#ifdef TARGET_SPARC64
# define gen_helper_rdpsr(D, E) qemu_build_not_reached()
+# define gen_helper_rdasr17(D, E) qemu_build_not_reached()
# define gen_helper_rett(E) qemu_build_not_reached()
# define gen_helper_power_down(E) qemu_build_not_reached()
# define gen_helper_wrpsr(E, S) qemu_build_not_reached()
@@ -2681,16 +2682,8 @@ static bool trans_RDY(DisasContext *dc, arg_RDY *a)
static TCGv do_rd_leon3_config(DisasContext *dc, TCGv dst)
{
-uint32_t val;
-
-/*
- * TODO: There are many more fields to be filled,
- * some of which are writable.
- */
-val = dc->def->nwindows - 1; /* [4:0] NWIN */
-val |= 1 << 8; /* [8] V8 */
-
-return tcg_constant_tl(val);
+gen_helper_rdasr17(dst, tcg_env);
+return dst;
}
TRANS(RDASR17, ASR17, do_rd_special, true, a->rd, do_rd_leon3_config)
--
2.25.1
[PATCH v2 8/8] MAINTAINERS: replace Fabien by myself as Leon3 maintainer
CC: Fabien Chouteau Signed-off-by: Clément Chigot Reviewed-by: Fabien Chouteau --- MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index b406fb20c0..b4e78e7748 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1708,7 +1708,7 @@ F: hw/rtc/sun4v-rtc.c F: include/hw/rtc/sun4v-rtc.h Leon3 -M: Fabien Chouteau +M: Clément Chigot M: Frederic Konrad S: Maintained F: hw/sparc/leon3.c -- 2.25.1
[PATCH v2 7/8] leon3: check cpu_id in the tiny bootloader
Now that SMP is possible, the asr17 must be checked in the little boot code
or the secondary CPU will reinitialize the Timer and the Uart.
Co-developed-by: Frederic Konrad
Signed-off-by: Clément Chigot
---
hw/sparc/leon3.c | 22 --
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index eacd85ee4f..87a8044a3e 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -100,13 +100,27 @@ static uint32_t *gen_store_u32(uint32_t *code, hwaddr
addr, uint32_t val)
/*
* When loading a kernel in RAM the machine is expected to be in a different
- * state (eg: initialized by the bootloader). This little code reproduces
- * this behavior.
+ * state (eg: initialized by the bootloader). This little code reproduces
+ * this behavior. Also this code can be executed by the secondary cpus as
+ * well since it looks at the %asr17 register before doing any
+ * initialization, it allows to use the same reset address for all the
+ * cpus.
*/
static void write_bootloader(CPUSPARCState *env, uint8_t *base,
hwaddr kernel_addr)
{
uint32_t *p = (uint32_t *) base;
+uint32_t *sec_cpu_branch_p = NULL;
+
+/* If we are running on a secondary CPU, jump directly to the kernel. */
+
+stl_p(p++, 0x85444000); /* rd %asr17, %g2 */
+stl_p(p++, 0x8530a01c); /* srl %g2, 0x1c, %g2 */
+stl_p(p++, 0x80908000); /* tst %g2*/
+/* Fill that later. */
+sec_cpu_branch_p = p;
+stl_p(p++, 0x0BADC0DE); /* bne xxx */
+stl_p(p++, 0x0100); /* nop */
/* Initialize the UARTs*/
/* *UART_CONTROL = UART_RECEIVE_ENABLE | UART_TRANSMIT_ENABLE; */
@@ -120,6 +134,10 @@ static void write_bootloader(CPUSPARCState *env, uint8_t
*base,
/* *GPTIMER0_CONFIG = GPTIMER_ENABLE | GPTIMER_RESTART;*/
p = gen_store_u32(p, 0x8318, 3);
+/* Now, the relative branch above can be computed. */
+stl_p(sec_cpu_branch_p, 0x1280
+ + (p - sec_cpu_branch_p));
+
/* JUMP to the entry point */
stl_p(p++, 0x8210); /* mov %g0, %g1 */
stl_p(p++, 0x0300 + extract32(kernel_addr, 10, 22));
--
2.25.1
[PATCH v2 6/8] leon3: implement multiprocessor
This allows to register more than one CPU on the leon3_generic machine.
Co-developed-by: Frederic Konrad
Signed-off-by: Clément Chigot
---
hw/sparc/leon3.c | 106 +--
1 file changed, 74 insertions(+), 32 deletions(-)
diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index 7866f0a049..eacd85ee4f 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -54,6 +54,8 @@
#define LEON3_PROM_OFFSET(0x)
#define LEON3_RAM_OFFSET (0x4000)
+#define MAX_CPUS 4
+
#define LEON3_UART_OFFSET (0x8100)
#define LEON3_UART_IRQ (3)
@@ -67,9 +69,12 @@
#define LEON3_AHB_PNP_OFFSET (0xF000)
typedef struct ResetData {
-SPARCCPU *cpu;
-uint32_t entry;/* save kernel entry in case of reset */
-target_ulong sp;/* initial stack pointer */
+struct CPUResetData {
+int id;
+SPARCCPU *cpu;
+target_ulong sp; /* initial stack pointer */
+} info[MAX_CPUS];
+uint32_t entry; /* save kernel entry in case of reset */
} ResetData;
static uint32_t *gen_store_u32(uint32_t *code, hwaddr addr, uint32_t val)
@@ -125,18 +130,19 @@ static void write_bootloader(CPUSPARCState *env, uint8_t
*base,
stl_p(p++, 0x0100); /* nop */
}
-static void main_cpu_reset(void *opaque)
+static void leon3_cpu_reset(void *opaque)
{
-ResetData *s = (ResetData *)opaque;
-CPUState *cpu = CPU(s->cpu);
-CPUSPARCState *env = &s->cpu->env;
+struct CPUResetData *info = (struct CPUResetData *) opaque;
+int id = info->id;
+ResetData *s = (ResetData *)DO_UPCAST(ResetData, info[id], info);
+CPUState *cpu = CPU(s->info[id].cpu);
+CPUSPARCState *env = cpu_env(cpu);
cpu_reset(cpu);
-
-cpu->halted = 0;
-env->pc = s->entry;
-env->npc= s->entry + 4;
-env->regbase[6] = s->sp;
+cpu->halted = cpu->cpu_index != 0;
+env->pc = s->entry;
+env->npc = s->entry + 4;
+env->regbase[6] = s->info[id].sp;
}
static void leon3_cache_control_int(CPUSPARCState *env)
@@ -170,8 +176,8 @@ static void leon3_cache_control_int(CPUSPARCState *env)
static void leon3_irq_ack(CPUSPARCState *env, int intno)
{
-/* No SMP support yet, only CPU #0 available so far. */
-grlib_irqmp_ack(env->irq_manager, 0, intno);
+CPUState *cpu = CPU(env_cpu(env));
+grlib_irqmp_ack(env->irq_manager, cpu->cpu_index, intno);
}
/*
@@ -213,6 +219,20 @@ static void leon3_set_pil_in(void *opaque, int n, int
level)
}
}
+static void leon3_start_cpu_async_work(CPUState *cpu, run_on_cpu_data data)
+{
+cpu->halted = 0;
+}
+
+static void leon3_start_cpu(void *opaque, int n, int level)
+{
+CPUState *cs = CPU(opaque);
+
+if (level) {
+async_run_on_cpu(cs, leon3_start_cpu_async_work, RUN_ON_CPU_NULL);
+}
+}
+
static void leon3_irq_manager(CPUSPARCState *env, int intno)
{
leon3_irq_ack(env, intno);
@@ -238,17 +258,21 @@ static void leon3_generic_hw_init(MachineState *machine)
AHBPnp *ahb_pnp;
APBPnp *apb_pnp;
-/* Init CPU */
-cpu = SPARC_CPU(cpu_create(machine->cpu_type));
-env = &cpu->env;
+reset_info = g_malloc0(sizeof(ResetData));
-cpu_sparc_set_id(env, 0);
+for (i = 0; i < machine->smp.cpus; i++) {
+/* Init CPU */
+cpu = SPARC_CPU(cpu_create(machine->cpu_type));
+env = &cpu->env;
-/* Reset data */
-reset_info= g_new0(ResetData, 1);
-reset_info->cpu = cpu;
-reset_info->sp= LEON3_RAM_OFFSET + ram_size;
-qemu_register_reset(main_cpu_reset, reset_info);
+cpu_sparc_set_id(env, i);
+
+/* Reset data */
+reset_info->info[i].id = i;
+reset_info->info[i].cpu = cpu;
+reset_info->info[i].sp = LEON3_RAM_OFFSET + ram_size;
+qemu_register_reset(leon3_cpu_reset, &reset_info->info[i]);
+}
ahb_pnp = GRLIB_AHB_PNP(qdev_new(TYPE_GRLIB_AHB_PNP));
sysbus_realize_and_unref(SYS_BUS_DEVICE(ahb_pnp), &error_fatal);
@@ -266,14 +290,28 @@ static void leon3_generic_hw_init(MachineState *machine)
/* Allocate IRQ manager */
irqmpdev = qdev_new(TYPE_GRLIB_IRQMP);
+object_property_set_int(OBJECT(irqmpdev), "ncpus", machine->smp.cpus,
+&error_fatal);
sysbus_realize_and_unref(SYS_BUS_DEVICE(irqmpdev), &error_fatal);
-qdev_init_gpio_in_named_with_opaque(DEVICE(cpu), leon3_set_pil_in,
-env, "pil", 1);
-qdev_connect_gpio_out_named(irqmpdev, "grlib-irq", 0,
-qdev_get_gpio_in_named(DEVICE(cpu), "pil", 0));
+
+for (i = 0; i < machine->smp.cpus; i++) {
+cpu = reset_info->info[i].cpu;
+env = &cpu->env;
+qdev_
[PATCH v2 3/8] intc/grlib_irqmp: implements the multiprocessor status register
This implements the multiprocessor status register in grlib-irqmp and bind
it to a start signal, which will be later wired in leon3-generic to
start a cpu.
The EIRQ and BA bits are not implemented.
Based on https://gaisler.com/doc/gr712rc-usermanual.pdf, §8.3.5.
Co-developed-by: Frederic Konrad
Signed-off-by: Clément Chigot
---
hw/intc/grlib_irqmp.c | 35 ---
1 file changed, 32 insertions(+), 3 deletions(-)
diff --git a/hw/intc/grlib_irqmp.c b/hw/intc/grlib_irqmp.c
index 744cd64c58..fa689e7a1f 100644
--- a/hw/intc/grlib_irqmp.c
+++ b/hw/intc/grlib_irqmp.c
@@ -52,6 +52,10 @@
#define FORCE_OFFSET 0x80
#define EXTENDED_OFFSET 0xC0
+/* Multiprocessor Status Register */
+#define MP_STATUS_CPU_STATUS_MASK ((1 << IRQMP_MAX_CPU)-2)
+#define MP_STATUS_NCPU_SHIFT 28
+
#define MAX_PILS 16
OBJECT_DECLARE_SIMPLE_TYPE(IRQMP, GRLIB_IRQMP)
@@ -65,6 +69,7 @@ struct IRQMP {
unsigned int ncpus;
IRQMPState *state;
+qemu_irq start_signal[IRQMP_MAX_CPU];
qemu_irq irq;
};
@@ -72,6 +77,7 @@ struct IRQMPState {
uint32_t level;
uint32_t pending;
uint32_t clear;
+uint32_t mpstatus;
uint32_t broadcast;
uint32_t mask[IRQMP_MAX_CPU];
@@ -182,10 +188,12 @@ static uint64_t grlib_irqmp_read(void *opaque, hwaddr
addr,
return state->force[0];
case CLEAR_OFFSET:
-case MP_STATUS_OFFSET:
/* Always read as 0 */
return 0;
+case MP_STATUS_OFFSET:
+return state->mpstatus;
+
case BROADCAST_OFFSET:
return state->broadcast;
@@ -224,8 +232,9 @@ static uint64_t grlib_irqmp_read(void *opaque, hwaddr addr,
static void grlib_irqmp_write(void *opaque, hwaddr addr,
uint64_t value, unsigned size)
{
-IRQMP *irqmp = opaque;
+IRQMP *irqmp = opaque;
IRQMPState *state;
+int i;
assert(irqmp != NULL);
state = irqmp->state;
@@ -258,7 +267,18 @@ static void grlib_irqmp_write(void *opaque, hwaddr addr,
return;
case MP_STATUS_OFFSET:
-/* Read Only (no SMP support) */
+/*
+ * Writing and reading operations are reversed for the CPU status.
+ * Writing "1" will start the CPU, but reading "1" means that the CPU
+ * is power-down.
+ */
+value &= MP_STATUS_CPU_STATUS_MASK;
+for (i = 0; i < irqmp->ncpus; i++) {
+if ((value >> i) & 1) {
+qemu_set_irq(irqmp->start_signal[i], 1);
+state->mpstatus &= ~(1 << i);
+}
+}
return;
case BROADCAST_OFFSET:
@@ -325,6 +345,8 @@ static void grlib_irqmp_reset(DeviceState *d)
memset(irqmp->state, 0, sizeof *irqmp->state);
irqmp->state->parent = irqmp;
+irqmp->state->mpstatus = ((irqmp->ncpus - 1) << MP_STATUS_NCPU_SHIFT)
+| ((1 << irqmp->ncpus) - 2);
}
static void grlib_irqmp_realize(DeviceState *dev, Error **errp)
@@ -338,6 +360,13 @@ static void grlib_irqmp_realize(DeviceState *dev, Error
**errp)
}
qdev_init_gpio_in(dev, grlib_irqmp_set_irq, MAX_PILS);
+
+/*
+ * Transitionning from 0 to 1 starts the CPUs. The opposite can't
+ * happen.
+ */
+qdev_init_gpio_out_named(dev, irqmp->start_signal, "grlib-start-cpu",
+ IRQMP_MAX_CPU);
qdev_init_gpio_out_named(dev, &irqmp->irq, "grlib-irq", 1);
memory_region_init_io(&irqmp->iomem, OBJECT(dev), &grlib_irqmp_ops, irqmp,
"irqmp", IRQMP_REG_SIZE);
--
2.25.1
[PATCH v2 4/8] intc/grlib_irqmp: implements multicore irq
Now there is an ncpus property, use it in order to deliver the IRQ to
multiple CPU.
Co-developed-by: Frederic Konrad
Signed-off-by: Clément Chigot
---
hw/intc/grlib_irqmp.c | 43 ++-
hw/sparc/leon3.c | 3 ++-
include/hw/intc/grlib_irqmp.h | 2 +-
3 files changed, 25 insertions(+), 23 deletions(-)
diff --git a/hw/intc/grlib_irqmp.c b/hw/intc/grlib_irqmp.c
index 8299ac183e..91237e6c44 100644
--- a/hw/intc/grlib_irqmp.c
+++ b/hw/intc/grlib_irqmp.c
@@ -70,7 +70,7 @@ struct IRQMP {
unsigned int ncpus;
IRQMPState *state;
qemu_irq start_signal[IRQMP_MAX_CPU];
-qemu_irq irq;
+qemu_irq irq[IRQMP_MAX_CPU];
};
struct IRQMPState {
@@ -89,37 +89,37 @@ struct IRQMPState {
static void grlib_irqmp_check_irqs(IRQMPState *state)
{
-uint32_t pend = 0;
-uint32_t level0 = 0;
-uint32_t level1 = 0;
+uint32_t pend = 0;
+uint32_t level0 = 0;
+uint32_t level1 = 0;
+int i;
assert(state != NULL);
assert(state->parent != NULL);
-/* IRQ for CPU 0 (no SMP support) */
-pend = (state->pending | state->force[0])
-& state->mask[0];
-
-level0 = pend & ~state->level;
-level1 = pend & state->level;
+for (i = 0; i < state->parent->ncpus; i++) {
+pend = (state->pending | state->force[i]) & state->mask[i];
+level0 = pend & ~state->level;
+level1 = pend & state->level;
-trace_grlib_irqmp_check_irqs(state->pending, state->force[0],
- state->mask[0], level1, level0);
+trace_grlib_irqmp_check_irqs(state->pending, state->force[i],
+ state->mask[i], level1, level0);
-/* Trigger level1 interrupt first and level0 if there is no level1 */
-qemu_set_irq(state->parent->irq, level1 ?: level0);
+/* Trigger level1 interrupt first and level0 if there is no level1 */
+qemu_set_irq(state->parent->irq[i], level1 ?: level0);
+}
}
-static void grlib_irqmp_ack_mask(IRQMPState *state, uint32_t mask)
+static void grlib_irqmp_ack_mask(IRQMPState *state, int cpu, uint32_t mask)
{
/* Clear registers */
state->pending &= ~mask;
-state->force[0] &= ~mask; /* Only CPU 0 (No SMP support) */
+state->force[cpu] &= ~mask;
grlib_irqmp_check_irqs(state);
}
-void grlib_irqmp_ack(DeviceState *dev, int intno)
+void grlib_irqmp_ack(DeviceState *dev, int cpu, int intno)
{
IRQMP*irqmp = GRLIB_IRQMP(dev);
IRQMPState *state;
@@ -133,7 +133,7 @@ void grlib_irqmp_ack(DeviceState *dev, int intno)
trace_grlib_irqmp_ack(intno);
-grlib_irqmp_ack_mask(state, mask);
+grlib_irqmp_ack_mask(state, cpu, mask);
}
static void grlib_irqmp_set_irq(void *opaque, int irq, int level)
@@ -159,7 +159,6 @@ static void grlib_irqmp_set_irq(void *opaque, int irq, int
level)
s->pending |= 1 << irq;
}
grlib_irqmp_check_irqs(s);
-
}
}
@@ -263,7 +262,9 @@ static void grlib_irqmp_write(void *opaque, hwaddr addr,
case CLEAR_OFFSET:
value &= ~1; /* clean up the value */
-grlib_irqmp_ack_mask(state, value);
+for (i = 0; i < irqmp->ncpus; i++) {
+grlib_irqmp_ack_mask(state, i, value);
+}
return;
case MP_STATUS_OFFSET:
@@ -367,7 +368,7 @@ static void grlib_irqmp_realize(DeviceState *dev, Error
**errp)
*/
qdev_init_gpio_out_named(dev, irqmp->start_signal, "grlib-start-cpu",
IRQMP_MAX_CPU);
-qdev_init_gpio_out_named(dev, &irqmp->irq, "grlib-irq", 1);
+qdev_init_gpio_out_named(dev, irqmp->irq, "grlib-irq", irqmp->ncpus);
memory_region_init_io(&irqmp->iomem, OBJECT(dev), &grlib_irqmp_ops, irqmp,
"irqmp", IRQMP_REG_SIZE);
diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index b72761b959..7866f0a049 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -170,7 +170,8 @@ static void leon3_cache_control_int(CPUSPARCState *env)
static void leon3_irq_ack(CPUSPARCState *env, int intno)
{
-grlib_irqmp_ack(env->irq_manager, intno);
+/* No SMP support yet, only CPU #0 available so far. */
+grlib_irqmp_ack(env->irq_manager, 0, intno);
}
/*
diff --git a/include/hw/intc/grlib_irqmp.h b/include/hw/intc/grlib_irqmp.h
index c5a90cbb3e..b564a0009f 100644
--- a/include/hw/intc/grlib_irqmp.h
+++ b/include/hw/intc/grlib_irqmp.h
@@ -36,6 +36,6 @@
/* IRQMP */
#define TYPE_GRLIB_IRQMP "grlib-irqmp"
-void grlib_irqmp_ack(DeviceState *dev, int intno);
+void grlib_irqmp_ack(DeviceState *dev, int cpu, int intno);
#endif /* GRLIB_IRQMP_H */
--
2.25.1
[PATCH v2 2/8] intc/grlib_irqmp: add ncpus property
This adds a "ncpus" property to the "grlib-irqmp" device to be used later,
this required a little refactoring of how we initialize the device (ie: use
realize instead of init).
Co-developed-by: Frederic Konrad
Signed-off-by: Clément Chigot
Reviewed-by: Philippe Mathieu-Daudé
---
hw/intc/grlib_irqmp.c | 30 +-
hw/sparc/leon3.c | 2 +-
2 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/hw/intc/grlib_irqmp.c b/hw/intc/grlib_irqmp.c
index 11eef62457..744cd64c58 100644
--- a/hw/intc/grlib_irqmp.c
+++ b/hw/intc/grlib_irqmp.c
@@ -1,7 +1,7 @@
/*
* QEMU GRLIB IRQMP Emulator
*
- * (Multiprocessor and extended interrupt not supported)
+ * (Extended interrupt not supported)
*
* SPDX-License-Identifier: MIT
*
@@ -63,6 +63,7 @@ struct IRQMP {
MemoryRegion iomem;
+unsigned int ncpus;
IRQMPState *state;
qemu_irq irq;
};
@@ -326,33 +327,44 @@ static void grlib_irqmp_reset(DeviceState *d)
irqmp->state->parent = irqmp;
}
-static void grlib_irqmp_init(Object *obj)
+static void grlib_irqmp_realize(DeviceState *dev, Error **errp)
{
-IRQMP *irqmp = GRLIB_IRQMP(obj);
-SysBusDevice *dev = SYS_BUS_DEVICE(obj);
+IRQMP *irqmp = GRLIB_IRQMP(dev);
-qdev_init_gpio_in(DEVICE(obj), grlib_irqmp_set_irq, MAX_PILS);
-qdev_init_gpio_out_named(DEVICE(obj), &irqmp->irq, "grlib-irq", 1);
-memory_region_init_io(&irqmp->iomem, obj, &grlib_irqmp_ops, irqmp,
+if ((!irqmp->ncpus) || (irqmp->ncpus > IRQMP_MAX_CPU)) {
+error_setg(errp, "Invalid ncpus properties: "
+ "%u, must be 0 < ncpus =< %u.", irqmp->ncpus,
+ IRQMP_MAX_CPU);
+}
+
+qdev_init_gpio_in(dev, grlib_irqmp_set_irq, MAX_PILS);
+qdev_init_gpio_out_named(dev, &irqmp->irq, "grlib-irq", 1);
+memory_region_init_io(&irqmp->iomem, OBJECT(dev), &grlib_irqmp_ops, irqmp,
"irqmp", IRQMP_REG_SIZE);
irqmp->state = g_malloc0(sizeof *irqmp->state);
-sysbus_init_mmio(dev, &irqmp->iomem);
+sysbus_init_mmio(SYS_BUS_DEVICE(dev), &irqmp->iomem);
}
+static Property grlib_irqmp_properties[] = {
+DEFINE_PROP_UINT32("ncpus", IRQMP, ncpus, 1),
+DEFINE_PROP_END_OF_LIST(),
+};
+
static void grlib_irqmp_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
+dc->realize = grlib_irqmp_realize;
dc->reset = grlib_irqmp_reset;
+device_class_set_props(dc, grlib_irqmp_properties);
}
static const TypeInfo grlib_irqmp_info = {
.name = TYPE_GRLIB_IRQMP,
.parent= TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(IRQMP),
-.instance_init = grlib_irqmp_init,
.class_init= grlib_irqmp_class_init,
};
diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index b7d81c76f3..b72761b959 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -265,11 +265,11 @@ static void leon3_generic_hw_init(MachineState *machine)
/* Allocate IRQ manager */
irqmpdev = qdev_new(TYPE_GRLIB_IRQMP);
+sysbus_realize_and_unref(SYS_BUS_DEVICE(irqmpdev), &error_fatal);
qdev_init_gpio_in_named_with_opaque(DEVICE(cpu), leon3_set_pil_in,
env, "pil", 1);
qdev_connect_gpio_out_named(irqmpdev, "grlib-irq", 0,
qdev_get_gpio_in_named(DEVICE(cpu), "pil", 0));
-sysbus_realize_and_unref(SYS_BUS_DEVICE(irqmpdev), &error_fatal);
sysbus_mmio_map(SYS_BUS_DEVICE(irqmpdev), 0, LEON3_IRQMP_OFFSET);
env->irq_manager = irqmpdev;
env->qemu_irq_ack = leon3_irq_manager;
--
2.25.1
[PATCH v2 1/8] sparc/grlib: split out the headers for each peripherals
... and move them in their right hardware directory.
Update Copyright and add SPDX-License-Identifier at the same time.
Co-developed-by: Frederic Konrad
Signed-off-by: Clément Chigot
---
hw/char/grlib_apbuart.c | 6 ++--
hw/intc/grlib_irqmp.c | 6 ++--
hw/sparc/leon3.c | 8 +++--
hw/timer/grlib_gptimer.c | 6 ++--
include/hw/char/grlib_uart.h | 32 +++
.../hw/{sparc/grlib.h => intc/grlib_irqmp.h} | 16 --
include/hw/timer/grlib_gptimer.h | 32 +++
7 files changed, 88 insertions(+), 18 deletions(-)
create mode 100644 include/hw/char/grlib_uart.h
rename include/hw/{sparc/grlib.h => intc/grlib_irqmp.h} (86%)
create mode 100644 include/hw/timer/grlib_gptimer.h
diff --git a/hw/char/grlib_apbuart.c b/hw/char/grlib_apbuart.c
index 82ff40a530..515b65bc07 100644
--- a/hw/char/grlib_apbuart.c
+++ b/hw/char/grlib_apbuart.c
@@ -1,7 +1,9 @@
/*
* QEMU GRLIB APB UART Emulator
*
- * Copyright (c) 2010-2019 AdaCore
+ * SPDX-License-Identifier: MIT
+ *
+ * Copyright (c) 2010-2024 AdaCore
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to
deal
@@ -26,7 +28,7 @@
#include "hw/irq.h"
#include "hw/qdev-properties.h"
#include "hw/qdev-properties-system.h"
-#include "hw/sparc/grlib.h"
+#include "hw/char/grlib_uart.h"
#include "hw/sysbus.h"
#include "qemu/module.h"
#include "chardev/char-fe.h"
diff --git a/hw/intc/grlib_irqmp.c b/hw/intc/grlib_irqmp.c
index 3bfe2544b7..11eef62457 100644
--- a/hw/intc/grlib_irqmp.c
+++ b/hw/intc/grlib_irqmp.c
@@ -3,7 +3,9 @@
*
* (Multiprocessor and extended interrupt not supported)
*
- * Copyright (c) 2010-2019 AdaCore
+ * SPDX-License-Identifier: MIT
+ *
+ * Copyright (c) 2010-2024 AdaCore
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to
deal
@@ -29,7 +31,7 @@
#include "hw/sysbus.h"
#include "hw/qdev-properties.h"
-#include "hw/sparc/grlib.h"
+#include "hw/intc/grlib_irqmp.h"
#include "trace.h"
#include "qapi/error.h"
diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index 2dfb742566..b7d81c76f3 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -1,7 +1,9 @@
/*
* QEMU Leon3 System Emulator
*
- * Copyright (c) 2010-2019 AdaCore
+ * SPDX-License-Identifier: MIT
+ *
+ * Copyright (c) 2010-2024 AdaCore
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to
deal
@@ -40,7 +42,9 @@
#include "elf.h"
#include "trace.h"
-#include "hw/sparc/grlib.h"
+#include "hw/timer/grlib_gptimer.h"
+#include "hw/char/grlib_uart.h"
+#include "hw/intc/grlib_irqmp.h"
#include "hw/misc/grlib_ahb_apb_pnp.h"
/* Default system clock. */
diff --git a/hw/timer/grlib_gptimer.c b/hw/timer/grlib_gptimer.c
index 5c4923c1e0..4990885451 100644
--- a/hw/timer/grlib_gptimer.c
+++ b/hw/timer/grlib_gptimer.c
@@ -1,7 +1,9 @@
/*
* QEMU GRLIB GPTimer Emulator
*
- * Copyright (c) 2010-2019 AdaCore
+ * SPDX-License-Identifier: MIT
+ *
+ * Copyright (c) 2010-2024 AdaCore
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to
deal
@@ -23,7 +25,7 @@
*/
#include "qemu/osdep.h"
-#include "hw/sparc/grlib.h"
+#include "hw/timer/grlib_gptimer.h"
#include "hw/sysbus.h"
#include "qemu/timer.h"
#include "hw/irq.h"
diff --git a/include/hw/char/grlib_uart.h b/include/hw/char/grlib_uart.h
new file mode 100644
index 00..7496f8fd5e
--- /dev/null
+++ b/include/hw/char/grlib_uart.h
@@ -0,0 +1,32 @@
+/*
+ * QEMU GRLIB UART
+ *
+ * SPDX-License-Identifier: MIT
+ *
+ * Copyright (c) 2024 AdaCore
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRES
[PATCH v2 0/8] sparc/leon3: Add support for -smp
V2 modifications
- Patch1: Add SPDX copyright tags.
- Patch3: Add defines for MP_STATUS fields. Improve comments.
- Patch4: Improve a comment.
- Patch6: Dropped as already merged.
---
This series allows leon3 emulations to record up 4 CPUs.
It requires some enhancements in the grlib_irqmp device and adding the
cpu_index field in the asr17 instruction.
It has been tested locally with various bareboard runtimes.
Clément Chigot (8):
sparc/grlib: split out the headers for each peripherals
intc/grlib_irqmp: add ncpus property
intc/grlib_irqmp: implements the multiprocessor status register
intc/grlib_irqmp: implements multicore irq
target/sparc: implement asr17 feature for smp
leon3: implement multiprocessor
leon3: check cpu_id in the tiny bootloader
MAINTAINERS: replace Fabien by myself as Leon3 maintainer
MAINTAINERS | 2 +-
hw/char/grlib_apbuart.c | 6 +-
hw/intc/grlib_irqmp.c | 112 ++-
hw/sparc/leon3.c | 135 +-
hw/timer/grlib_gptimer.c | 6 +-
include/hw/char/grlib_uart.h | 32 +
.../hw/{sparc/grlib.h => intc/grlib_irqmp.h} | 18 +--
include/hw/timer/grlib_gptimer.h | 32 +
target/sparc/helper.c | 16 +++
target/sparc/helper.h | 1 +
target/sparc/translate.c | 13 +-
11 files changed, 278 insertions(+), 95 deletions(-)
create mode 100644 include/hw/char/grlib_uart.h
rename include/hw/{sparc/grlib.h => intc/grlib_irqmp.h} (83%)
create mode 100644 include/hw/timer/grlib_gptimer.h
--
2.25.1
Re: [PATCH 1/9] sparc/grlib: split out the headers for each peripherals
On Fri, Jan 5, 2024 at 3:00 PM Philippe Mathieu-Daudé wrote:
>
> On 5/1/24 11:24, Clément Chigot wrote:
> > ... and move them in their right hardware directory.
> >
> > Co-developed-by: Frederic Konrad
> > Signed-off-by: Clément Chigot
> > ---
> > hw/char/grlib_apbuart.c | 4 +--
> > hw/intc/grlib_irqmp.c | 4 +--
> > hw/sparc/leon3.c | 6 ++--
> > hw/timer/grlib_gptimer.c | 4 +--
> > include/hw/char/grlib_uart.h | 30 +++
> > .../hw/{sparc/grlib.h => intc/grlib_irqmp.h} | 14 +++--
> > include/hw/timer/grlib_gptimer.h | 30 +++
> > 7 files changed, 74 insertions(+), 18 deletions(-)
> > create mode 100644 include/hw/char/grlib_uart.h
> > rename include/hw/{sparc/grlib.h => intc/grlib_irqmp.h} (86%)
> > create mode 100644 include/hw/timer/grlib_gptimer.h
>
> This still matches the MAINTAINERS patterns, good.
>
> > diff --git a/include/hw/char/grlib_uart.h b/include/hw/char/grlib_uart.h
> > new file mode 100644
> > index 00..b67da6c62a
> > --- /dev/null
> > +++ b/include/hw/char/grlib_uart.h
> > @@ -0,0 +1,30 @@
> > +/*
> > + * QEMU GRLIB UART
> > + *
> > + * Copyright (c) 2024 AdaCore
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > copy
> > + * of this software and associated documentation files (the "Software"),
> > to deal
> > + * in the Software without restriction, including without limitation the
> > rights
> > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or
> > sell
> > + * copies of the Software, and to permit persons to whom the Software is
> > + * furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included
> > in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> > OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> > OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > FROM,
> > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> > IN
> > + * THE SOFTWARE.
>
> When adding license, SPDX tag is prefered (although not enforced)
> because it eases tools parsing.
Should it be something like this ?
* SPDX-FileCopyrightText: 20xx-2024 Adacore
* SPDX-License-Identifier: MIT
Would updating, now, all those files make it better ?
> > + */
> > +
> > +#ifndef GRLIB_UART_H
> > +#define GRLIB_UART_H
> > +
> > +#define TYPE_GRLIB_APB_UART "grlib-apbuart"
> > +
> > +#endif
>
> Reviewed-by: Philippe Mathieu-Daudé
>
Re: [PATCH 3/9] intc/grlib_irqmp: implements the multiprocessor status register
On Fri, Jan 5, 2024 at 2:37 PM Philippe Mathieu-Daudé wrote:
>
> On 5/1/24 14:23, Clément Chigot wrote:
> > On Fri, Jan 5, 2024 at 12:32 PM Philippe Mathieu-Daudé
> > wrote:
> >>
> >> Hi Clément,
> >>
> >> On 5/1/24 11:24, Clément Chigot wrote:
> >>> This implements the multiprocessor status register in grlib-irqmp and bind
> >>> it to a start signal, which will be later wired in leon3-generic to
> >>> start a cpu.
> >>>
> >>> Co-developed-by: Frederic Konrad
> >>> Signed-off-by: Clément Chigot
> >>> ---
> >>>hw/intc/grlib_irqmp.c | 22 +++---
> >>>1 file changed, 19 insertions(+), 3 deletions(-)
> >>
> >>
> >>> @@ -323,6 +334,8 @@ static void grlib_irqmp_reset(DeviceState *d)
> >>>
> >>>memset(irqmp->state, 0, sizeof *irqmp->state);
> >>>irqmp->state->parent = irqmp;
> >>> +irqmp->state->mpstatus = ((irqmp->ncpus - 1) << 28)
> >>
> >> Can you #define this magic '28' number?
> >>
> >>> +| ((1 << irqmp->ncpus) - 2);
> >>>}
> >>>
> >>>static void grlib_irqmp_realize(DeviceState *dev, Error **errp)
> >>> @@ -336,6 +349,9 @@ static void grlib_irqmp_realize(DeviceState *dev,
> >>> Error **errp)
> >>>}
> >>>
> >>>qdev_init_gpio_in(dev, grlib_irqmp_set_irq, MAX_PILS);
> >>> +/* Transitionning from 0 to 1 starts the CPUs. */
> >>
> >> What about 1 -> 0?
> >
> > It does nothing. I have updated the comment to mention it.
> > For the doc (also mention it in the commit message now).
> >| [15:1] Power-down status of CPU [n]: reads ‘1’ = power-down, ‘0’ =
> > running.
> >| Write to start processor n: ‘1’=to start ‘0'=has no effect.
>
> Then grlib_irqmp_write() could be simplified as:
>
> case MP_STATUS_OFFSET:
> -/* Read Only (no SMP support) */
> +state->mpstatus = deposit32(state->mpstatus,
> +value, 0, IRQMP_MAX_CPU);
> +for (unsigned i = 0; i < irqmp->ncpus; i++) {
> +qemu_set_irq(irqmp->start_signal[i],
> + extract32(value, i, 1));
> +}
> return;
No, because the logic between write and read is reversed.
Writing "1" starts the CPU but reading 1 means that the CPU is not
started. Therefore, when writing 1, we must trigger the start signal
and then write 0 in mpstatus.
Re: [PATCH 3/9] intc/grlib_irqmp: implements the multiprocessor status register
On Fri, Jan 5, 2024 at 12:32 PM Philippe Mathieu-Daudé wrote: > > Hi Clément, > > On 5/1/24 11:24, Clément Chigot wrote: > > This implements the multiprocessor status register in grlib-irqmp and bind > > it to a start signal, which will be later wired in leon3-generic to > > start a cpu. > > > > Co-developed-by: Frederic Konrad > > Signed-off-by: Clément Chigot > > --- > > hw/intc/grlib_irqmp.c | 22 +++--- > > 1 file changed, 19 insertions(+), 3 deletions(-) > > > > @@ -323,6 +334,8 @@ static void grlib_irqmp_reset(DeviceState *d) > > > > memset(irqmp->state, 0, sizeof *irqmp->state); > > irqmp->state->parent = irqmp; > > +irqmp->state->mpstatus = ((irqmp->ncpus - 1) << 28) > > Can you #define this magic '28' number? > > > +| ((1 << irqmp->ncpus) - 2); > > } > > > > static void grlib_irqmp_realize(DeviceState *dev, Error **errp) > > @@ -336,6 +349,9 @@ static void grlib_irqmp_realize(DeviceState *dev, Error > > **errp) > > } > > > > qdev_init_gpio_in(dev, grlib_irqmp_set_irq, MAX_PILS); > > +/* Transitionning from 0 to 1 starts the CPUs. */ > > What about 1 -> 0? It does nothing. I have updated the comment to mention it. For the doc (also mention it in the commit message now). | [15:1] Power-down status of CPU [n]: reads ‘1’ = power-down, ‘0’ = running. | Write to start processor n: ‘1’=to start ‘0'=has no effect. > > +qdev_init_gpio_out_named(dev, irqmp->start_signal, "grlib-start-cpu", > > + IRQMP_MAX_CPU); > > qdev_init_gpio_out_named(dev, &irqmp->irq, "grlib-irq", 1); > > memory_region_init_io(&irqmp->iomem, OBJECT(dev), &grlib_irqmp_ops, > > irqmp, > > "irqmp", IRQMP_REG_SIZE); >
Re: [PATCH 9/9] MAINTAINERS: replace Fabien by myself as Leon3 maintainer
On Fri, Jan 5, 2024 at 11:24 AM Clément Chigot wrote: > > CC: Fabien Chouteau Typo here... Should have been chout...@adacore.com... I'll update it in v2 if one is needed. Otherwise, could the one pushing these patches make the change for me ? Thanks and sorry for that. > Signed-off-by: Clément Chigot > --- > MAINTAINERS | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 395f26ba86..a065e0b21f 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1694,7 +1694,7 @@ F: hw/rtc/sun4v-rtc.c > F: include/hw/rtc/sun4v-rtc.h > > Leon3 > -M: Fabien Chouteau > +M: Clément Chigot > M: Frederic Konrad > S: Maintained > F: hw/sparc/leon3.c > -- > 2.25.1 >
[PATCH 5/9] target/sparc: implement asr17 feature for smp
This allows the guest program to know its cpu id.
Co-developed-by: Frederic Konrad
Signed-off-by: Clément Chigot
---
target/sparc/helper.c| 16
target/sparc/helper.h| 1 +
target/sparc/translate.c | 13 +++--
3 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/target/sparc/helper.c b/target/sparc/helper.c
index bd10b60e4b..2247e243b5 100644
--- a/target/sparc/helper.c
+++ b/target/sparc/helper.c
@@ -212,4 +212,20 @@ void helper_power_down(CPUSPARCState *env)
env->npc = env->pc + 4;
cpu_loop_exit(cs);
}
+
+target_ulong helper_rdasr17(CPUSPARCState *env)
+{
+CPUState *cs = env_cpu(env);
+target_ulong val;
+
+/*
+ * TODO: There are many more fields to be filled,
+ * some of which are writable.
+ */
+val = env->def.nwindows - 1;/* [4:0] NWIN */
+val |= 1 << 8; /* [8] V8*/
+val |= (cs->cpu_index) << 28; /* [31:28] INDEX */
+
+return val;
+}
#endif
diff --git a/target/sparc/helper.h b/target/sparc/helper.h
index 55eff66283..fc818b8678 100644
--- a/target/sparc/helper.h
+++ b/target/sparc/helper.h
@@ -2,6 +2,7 @@
DEF_HELPER_1(rett, void, env)
DEF_HELPER_2(wrpsr, void, env, tl)
DEF_HELPER_1(rdpsr, tl, env)
+DEF_HELPER_1(rdasr17, tl, env)
DEF_HELPER_1(power_down, void, env)
#else
DEF_HELPER_FLAGS_2(wrpil, TCG_CALL_NO_RWG, void, env, tl)
diff --git a/target/sparc/translate.c b/target/sparc/translate.c
index 9387299559..1cabda9565 100644
--- a/target/sparc/translate.c
+++ b/target/sparc/translate.c
@@ -37,6 +37,7 @@
#ifdef TARGET_SPARC64
# define gen_helper_rdpsr(D, E) qemu_build_not_reached()
+# define gen_helper_rdasr17(D, E) qemu_build_not_reached()
# define gen_helper_rett(E) qemu_build_not_reached()
# define gen_helper_power_down(E) qemu_build_not_reached()
# define gen_helper_wrpsr(E, S) qemu_build_not_reached()
@@ -2681,16 +2682,8 @@ static bool trans_RDY(DisasContext *dc, arg_RDY *a)
static TCGv do_rd_leon3_config(DisasContext *dc, TCGv dst)
{
-uint32_t val;
-
-/*
- * TODO: There are many more fields to be filled,
- * some of which are writable.
- */
-val = dc->def->nwindows - 1; /* [4:0] NWIN */
-val |= 1 << 8; /* [8] V8 */
-
-return tcg_constant_tl(val);
+gen_helper_rdasr17(dst, tcg_env);
+return dst;
}
TRANS(RDASR17, ASR17, do_rd_special, true, a->rd, do_rd_leon3_config)
--
2.25.1
[PATCH 7/9] leon3: implement multiprocessor
This allows to register more than one CPU on the leon3_generic machine.
Co-developed-by: Frederic Konrad
Signed-off-by: Clément Chigot
---
hw/sparc/leon3.c | 106 +--
1 file changed, 74 insertions(+), 32 deletions(-)
diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index 6019fc4c2d..38fb8d9af1 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -52,6 +52,8 @@
#define LEON3_PROM_OFFSET(0x)
#define LEON3_RAM_OFFSET (0x4000)
+#define MAX_CPUS 4
+
#define LEON3_UART_OFFSET (0x8100)
#define LEON3_UART_IRQ (3)
@@ -65,9 +67,12 @@
#define LEON3_AHB_PNP_OFFSET (0xF000)
typedef struct ResetData {
-SPARCCPU *cpu;
-uint32_t entry;/* save kernel entry in case of reset */
-target_ulong sp;/* initial stack pointer */
+struct CPUResetData {
+int id;
+SPARCCPU *cpu;
+target_ulong sp; /* initial stack pointer */
+} info[MAX_CPUS];
+uint32_t entry; /* save kernel entry in case of reset */
} ResetData;
static uint32_t *gen_store_u32(uint32_t *code, hwaddr addr, uint32_t val)
@@ -123,18 +128,19 @@ static void write_bootloader(CPUSPARCState *env, uint8_t
*base,
stl_p(p++, 0x0100); /* nop */
}
-static void main_cpu_reset(void *opaque)
+static void leon3_cpu_reset(void *opaque)
{
-ResetData *s = (ResetData *)opaque;
-CPUState *cpu = CPU(s->cpu);
-CPUSPARCState *env = &s->cpu->env;
+struct CPUResetData *info = (struct CPUResetData *) opaque;
+int id = info->id;
+ResetData *s = (ResetData *)DO_UPCAST(ResetData, info[id], info);
+CPUState *cpu = CPU(s->info[id].cpu);
+CPUSPARCState *env = cpu_env(cpu);
cpu_reset(cpu);
-
-cpu->halted = 0;
-env->pc = s->entry;
-env->npc= s->entry + 4;
-env->regbase[6] = s->sp;
+cpu->halted = cpu->cpu_index != 0;
+env->pc = s->entry;
+env->npc = s->entry + 4;
+env->regbase[6] = s->info[id].sp;
}
static void leon3_cache_control_int(CPUSPARCState *env)
@@ -168,8 +174,8 @@ static void leon3_cache_control_int(CPUSPARCState *env)
static void leon3_irq_ack(CPUSPARCState *env, int intno)
{
-/* No SMP support yet. */
-grlib_irqmp_ack(env->irq_manager, 0, intno);
+CPUState *cpu = CPU(env_cpu(env));
+grlib_irqmp_ack(env->irq_manager, cpu->cpu_index, intno);
}
/*
@@ -211,6 +217,20 @@ static void leon3_set_pil_in(void *opaque, int n, int
level)
}
}
+static void leon3_start_cpu_async_work(CPUState *cpu, run_on_cpu_data data)
+{
+cpu->halted = 0;
+}
+
+static void leon3_start_cpu(void *opaque, int n, int level)
+{
+CPUState *cs = CPU(opaque);
+
+if (level) {
+async_run_on_cpu(cs, leon3_start_cpu_async_work, RUN_ON_CPU_NULL);
+}
+}
+
static void leon3_irq_manager(CPUSPARCState *env, int intno)
{
leon3_irq_ack(env, intno);
@@ -236,17 +256,21 @@ static void leon3_generic_hw_init(MachineState *machine)
AHBPnp *ahb_pnp;
APBPnp *apb_pnp;
-/* Init CPU */
-cpu = SPARC_CPU(cpu_create(machine->cpu_type));
-env = &cpu->env;
+reset_info = g_malloc0(sizeof(ResetData));
-cpu_sparc_set_id(env, 0);
+for (i = 0; i < machine->smp.cpus; i++) {
+/* Init CPU */
+cpu = SPARC_CPU(cpu_create(machine->cpu_type));
+env = &cpu->env;
-/* Reset data */
-reset_info= g_new0(ResetData, 1);
-reset_info->cpu = cpu;
-reset_info->sp= LEON3_RAM_OFFSET + ram_size;
-qemu_register_reset(main_cpu_reset, reset_info);
+cpu_sparc_set_id(env, i);
+
+/* Reset data */
+reset_info->info[i].id = i;
+reset_info->info[i].cpu = cpu;
+reset_info->info[i].sp = LEON3_RAM_OFFSET + ram_size;
+qemu_register_reset(leon3_cpu_reset, &reset_info->info[i]);
+}
ahb_pnp = GRLIB_AHB_PNP(qdev_new(TYPE_GRLIB_AHB_PNP));
sysbus_realize_and_unref(SYS_BUS_DEVICE(ahb_pnp), &error_fatal);
@@ -264,14 +288,28 @@ static void leon3_generic_hw_init(MachineState *machine)
/* Allocate IRQ manager */
irqmpdev = qdev_new(TYPE_GRLIB_IRQMP);
+object_property_set_int(OBJECT(irqmpdev), "ncpus", machine->smp.cpus,
+&error_fatal);
sysbus_realize_and_unref(SYS_BUS_DEVICE(irqmpdev), &error_fatal);
-qdev_init_gpio_in_named_with_opaque(DEVICE(cpu), leon3_set_pil_in,
-env, "pil", 1);
-qdev_connect_gpio_out_named(irqmpdev, "grlib-irq", 0,
-qdev_get_gpio_in_named(DEVICE(cpu), "pil", 0));
+
+for (i = 0; i < machine->smp.cpus; i++) {
+cpu = reset_info->info[i].cpu;
+env = &cpu->env;
+qdev_init_gpio_in_named_with_opaque(DEVICE(cpu), leon3_start_cpu,
+
[PATCH 2/9] intc/grlib_irqmp: add ncpus property
This adds a "ncpus" property to the "grlib-irqmp" device to be used later,
this required a little refactoring of how we initialize the device (ie: use
realize instead of init).
Co-developed-by: Frederic Konrad
Signed-off-by: Clément Chigot
---
hw/intc/grlib_irqmp.c | 30 +-
hw/sparc/leon3.c | 2 +-
2 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/hw/intc/grlib_irqmp.c b/hw/intc/grlib_irqmp.c
index c994d5dacc..2bacc0ff56 100644
--- a/hw/intc/grlib_irqmp.c
+++ b/hw/intc/grlib_irqmp.c
@@ -1,7 +1,7 @@
/*
* QEMU GRLIB IRQMP Emulator
*
- * (Multiprocessor and extended interrupt not supported)
+ * (Extended interrupt not supported)
*
* Copyright (c) 2010-2024 AdaCore
*
@@ -61,6 +61,7 @@ struct IRQMP {
MemoryRegion iomem;
+unsigned int ncpus;
IRQMPState *state;
qemu_irq irq;
};
@@ -324,33 +325,44 @@ static void grlib_irqmp_reset(DeviceState *d)
irqmp->state->parent = irqmp;
}
-static void grlib_irqmp_init(Object *obj)
+static void grlib_irqmp_realize(DeviceState *dev, Error **errp)
{
-IRQMP *irqmp = GRLIB_IRQMP(obj);
-SysBusDevice *dev = SYS_BUS_DEVICE(obj);
+IRQMP *irqmp = GRLIB_IRQMP(dev);
-qdev_init_gpio_in(DEVICE(obj), grlib_irqmp_set_irq, MAX_PILS);
-qdev_init_gpio_out_named(DEVICE(obj), &irqmp->irq, "grlib-irq", 1);
-memory_region_init_io(&irqmp->iomem, obj, &grlib_irqmp_ops, irqmp,
+if ((!irqmp->ncpus) || (irqmp->ncpus > IRQMP_MAX_CPU)) {
+error_setg(errp, "Invalid ncpus properties: "
+ "%u, must be 0 < ncpus =< %u.", irqmp->ncpus,
+ IRQMP_MAX_CPU);
+}
+
+qdev_init_gpio_in(dev, grlib_irqmp_set_irq, MAX_PILS);
+qdev_init_gpio_out_named(dev, &irqmp->irq, "grlib-irq", 1);
+memory_region_init_io(&irqmp->iomem, OBJECT(dev), &grlib_irqmp_ops, irqmp,
"irqmp", IRQMP_REG_SIZE);
irqmp->state = g_malloc0(sizeof *irqmp->state);
-sysbus_init_mmio(dev, &irqmp->iomem);
+sysbus_init_mmio(SYS_BUS_DEVICE(dev), &irqmp->iomem);
}
+static Property grlib_irqmp_properties[] = {
+DEFINE_PROP_UINT32("ncpus", IRQMP, ncpus, 1),
+DEFINE_PROP_END_OF_LIST(),
+};
+
static void grlib_irqmp_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
+dc->realize = grlib_irqmp_realize;
dc->reset = grlib_irqmp_reset;
+device_class_set_props(dc, grlib_irqmp_properties);
}
static const TypeInfo grlib_irqmp_info = {
.name = TYPE_GRLIB_IRQMP,
.parent= TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(IRQMP),
-.instance_init = grlib_irqmp_init,
.class_init= grlib_irqmp_class_init,
};
diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index 58784c099a..7b9809b81f 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -263,11 +263,11 @@ static void leon3_generic_hw_init(MachineState *machine)
/* Allocate IRQ manager */
irqmpdev = qdev_new(TYPE_GRLIB_IRQMP);
+sysbus_realize_and_unref(SYS_BUS_DEVICE(irqmpdev), &error_fatal);
qdev_init_gpio_in_named_with_opaque(DEVICE(cpu), leon3_set_pil_in,
env, "pil", 1);
qdev_connect_gpio_out_named(irqmpdev, "grlib-irq", 0,
qdev_get_gpio_in_named(DEVICE(cpu), "pil", 0));
-sysbus_realize_and_unref(SYS_BUS_DEVICE(irqmpdev), &error_fatal);
sysbus_mmio_map(SYS_BUS_DEVICE(irqmpdev), 0, LEON3_IRQMP_OFFSET);
env->irq_manager = irqmpdev;
env->qemu_irq_ack = leon3_irq_manager;
--
2.25.1
[PATCH 0/9] sparc/leon3: Add support for -smp
This series allows leon3 emulations to record up 4 CPUs.
It requires some enhancements in the grlib_irqmp device and adding the
cpu_index field in the asr17 instruction.
It has been tested locally with various bareboard runtimes and through
the Gitlab CI: https://gitlab.com/Helflym/qemu/-/pipelines/1127834623.
Clément Chigot (9):
sparc/grlib: split out the headers for each peripherals
intc/grlib_irqmp: add ncpus property
intc/grlib_irqmp: implements the multiprocessor status register
intc/grlib_irqmp: implements multicore irq
target/sparc: implement asr17 feature for smp
target/sparc: simplify qemu_irq_ack
leon3: implement multiprocessor
leon3: check cpu_id in the tiny bootloader
MAINTAINERS: replace Fabien by myself as Leon3 maintainer
MAINTAINERS | 2 +-
hw/char/grlib_apbuart.c | 4 +-
hw/intc/grlib_irqmp.c | 97
hw/sparc/leon3.c | 145 +-
hw/timer/grlib_gptimer.c | 4 +-
include/hw/char/grlib_uart.h | 30
.../hw/{sparc/grlib.h => intc/grlib_irqmp.h} | 16 +-
include/hw/timer/grlib_gptimer.h | 30
target/sparc/cpu.h| 2 +-
target/sparc/helper.c | 16 ++
target/sparc/helper.h | 1 +
target/sparc/int32_helper.c | 2 +-
target/sparc/translate.c | 13 +-
13 files changed, 258 insertions(+), 104 deletions(-)
create mode 100644 include/hw/char/grlib_uart.h
rename include/hw/{sparc/grlib.h => intc/grlib_irqmp.h} (83%)
create mode 100644 include/hw/timer/grlib_gptimer.h
--
2.25.1
[PATCH 3/9] intc/grlib_irqmp: implements the multiprocessor status register
This implements the multiprocessor status register in grlib-irqmp and bind
it to a start signal, which will be later wired in leon3-generic to
start a cpu.
Co-developed-by: Frederic Konrad
Signed-off-by: Clément Chigot
---
hw/intc/grlib_irqmp.c | 22 +++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/hw/intc/grlib_irqmp.c b/hw/intc/grlib_irqmp.c
index 2bacc0ff56..be0e840181 100644
--- a/hw/intc/grlib_irqmp.c
+++ b/hw/intc/grlib_irqmp.c
@@ -63,6 +63,7 @@ struct IRQMP {
unsigned int ncpus;
IRQMPState *state;
+qemu_irq start_signal[IRQMP_MAX_CPU];
qemu_irq irq;
};
@@ -70,6 +71,7 @@ struct IRQMPState {
uint32_t level;
uint32_t pending;
uint32_t clear;
+uint32_t mpstatus;
uint32_t broadcast;
uint32_t mask[IRQMP_MAX_CPU];
@@ -180,10 +182,12 @@ static uint64_t grlib_irqmp_read(void *opaque, hwaddr
addr,
return state->force[0];
case CLEAR_OFFSET:
-case MP_STATUS_OFFSET:
/* Always read as 0 */
return 0;
+case MP_STATUS_OFFSET:
+return state->mpstatus;
+
case BROADCAST_OFFSET:
return state->broadcast;
@@ -222,8 +226,9 @@ static uint64_t grlib_irqmp_read(void *opaque, hwaddr addr,
static void grlib_irqmp_write(void *opaque, hwaddr addr,
uint64_t value, unsigned size)
{
-IRQMP *irqmp = opaque;
+IRQMP *irqmp = opaque;
IRQMPState *state;
+int i;
assert(irqmp != NULL);
state = irqmp->state;
@@ -256,7 +261,13 @@ static void grlib_irqmp_write(void *opaque, hwaddr addr,
return;
case MP_STATUS_OFFSET:
-/* Read Only (no SMP support) */
+value &= 0x;
+for (i = 0; i < irqmp->ncpus; i++) {
+if ((value >> i) & 1) {
+qemu_set_irq(irqmp->start_signal[i], 1);
+state->mpstatus &= ~(1 << i);
+}
+}
return;
case BROADCAST_OFFSET:
@@ -323,6 +334,8 @@ static void grlib_irqmp_reset(DeviceState *d)
memset(irqmp->state, 0, sizeof *irqmp->state);
irqmp->state->parent = irqmp;
+irqmp->state->mpstatus = ((irqmp->ncpus - 1) << 28)
+| ((1 << irqmp->ncpus) - 2);
}
static void grlib_irqmp_realize(DeviceState *dev, Error **errp)
@@ -336,6 +349,9 @@ static void grlib_irqmp_realize(DeviceState *dev, Error
**errp)
}
qdev_init_gpio_in(dev, grlib_irqmp_set_irq, MAX_PILS);
+/* Transitionning from 0 to 1 starts the CPUs. */
+qdev_init_gpio_out_named(dev, irqmp->start_signal, "grlib-start-cpu",
+ IRQMP_MAX_CPU);
qdev_init_gpio_out_named(dev, &irqmp->irq, "grlib-irq", 1);
memory_region_init_io(&irqmp->iomem, OBJECT(dev), &grlib_irqmp_ops, irqmp,
"irqmp", IRQMP_REG_SIZE);
--
2.25.1
[PATCH 1/9] sparc/grlib: split out the headers for each peripherals
... and move them in their right hardware directory.
Co-developed-by: Frederic Konrad
Signed-off-by: Clément Chigot
---
hw/char/grlib_apbuart.c | 4 +--
hw/intc/grlib_irqmp.c | 4 +--
hw/sparc/leon3.c | 6 ++--
hw/timer/grlib_gptimer.c | 4 +--
include/hw/char/grlib_uart.h | 30 +++
.../hw/{sparc/grlib.h => intc/grlib_irqmp.h} | 14 +++--
include/hw/timer/grlib_gptimer.h | 30 +++
7 files changed, 74 insertions(+), 18 deletions(-)
create mode 100644 include/hw/char/grlib_uart.h
rename include/hw/{sparc/grlib.h => intc/grlib_irqmp.h} (86%)
create mode 100644 include/hw/timer/grlib_gptimer.h
diff --git a/hw/char/grlib_apbuart.c b/hw/char/grlib_apbuart.c
index 82ff40a530..2c9ab70b85 100644
--- a/hw/char/grlib_apbuart.c
+++ b/hw/char/grlib_apbuart.c
@@ -1,7 +1,7 @@
/*
* QEMU GRLIB APB UART Emulator
*
- * Copyright (c) 2010-2019 AdaCore
+ * Copyright (c) 2010-2024 AdaCore
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to
deal
@@ -26,7 +26,7 @@
#include "hw/irq.h"
#include "hw/qdev-properties.h"
#include "hw/qdev-properties-system.h"
-#include "hw/sparc/grlib.h"
+#include "hw/char/grlib_uart.h"
#include "hw/sysbus.h"
#include "qemu/module.h"
#include "chardev/char-fe.h"
diff --git a/hw/intc/grlib_irqmp.c b/hw/intc/grlib_irqmp.c
index 3bfe2544b7..c994d5dacc 100644
--- a/hw/intc/grlib_irqmp.c
+++ b/hw/intc/grlib_irqmp.c
@@ -3,7 +3,7 @@
*
* (Multiprocessor and extended interrupt not supported)
*
- * Copyright (c) 2010-2019 AdaCore
+ * Copyright (c) 2010-2024 AdaCore
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to
deal
@@ -29,7 +29,7 @@
#include "hw/sysbus.h"
#include "hw/qdev-properties.h"
-#include "hw/sparc/grlib.h"
+#include "hw/intc/grlib_irqmp.h"
#include "trace.h"
#include "qapi/error.h"
diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index 1e39d2e2d0..58784c099a 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -1,7 +1,7 @@
/*
* QEMU Leon3 System Emulator
*
- * Copyright (c) 2010-2019 AdaCore
+ * Copyright (c) 2010-2024 AdaCore
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to
deal
@@ -40,7 +40,9 @@
#include "elf.h"
#include "trace.h"
-#include "hw/sparc/grlib.h"
+#include "hw/timer/grlib_gptimer.h"
+#include "hw/char/grlib_uart.h"
+#include "hw/intc/grlib_irqmp.h"
#include "hw/misc/grlib_ahb_apb_pnp.h"
/* Default system clock. */
diff --git a/hw/timer/grlib_gptimer.c b/hw/timer/grlib_gptimer.c
index 5c4923c1e0..5c2cddcec3 100644
--- a/hw/timer/grlib_gptimer.c
+++ b/hw/timer/grlib_gptimer.c
@@ -1,7 +1,7 @@
/*
* QEMU GRLIB GPTimer Emulator
*
- * Copyright (c) 2010-2019 AdaCore
+ * Copyright (c) 2010-2024 AdaCore
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to
deal
@@ -23,7 +23,7 @@
*/
#include "qemu/osdep.h"
-#include "hw/sparc/grlib.h"
+#include "hw/timer/grlib_gptimer.h"
#include "hw/sysbus.h"
#include "qemu/timer.h"
#include "hw/irq.h"
diff --git a/include/hw/char/grlib_uart.h b/include/hw/char/grlib_uart.h
new file mode 100644
index 00..b67da6c62a
--- /dev/null
+++ b/include/hw/char/grlib_uart.h
@@ -0,0 +1,30 @@
+/*
+ * QEMU GRLIB UART
+ *
+ * Copyright (c) 2024 AdaCore
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETH
[PATCH 9/9] MAINTAINERS: replace Fabien by myself as Leon3 maintainer
CC: Fabien Chouteau Signed-off-by: Clément Chigot --- MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index 395f26ba86..a065e0b21f 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1694,7 +1694,7 @@ F: hw/rtc/sun4v-rtc.c F: include/hw/rtc/sun4v-rtc.h Leon3 -M: Fabien Chouteau +M: Clément Chigot M: Frederic Konrad S: Maintained F: hw/sparc/leon3.c -- 2.25.1
[PATCH 6/9] target/sparc: simplify qemu_irq_ack
This is a simple cleanup, since env is passed to qemu_irq_ack it can be
accessed from inside qemu_irq_ack. Just drop this parameter.
Co-developed-by: Frederic Konrad
Signed-off-by: Clément Chigot
---
hw/sparc/leon3.c| 8
target/sparc/cpu.h | 2 +-
target/sparc/int32_helper.c | 2 +-
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index 94d8ec94b0..6019fc4c2d 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -166,10 +166,10 @@ static void leon3_cache_control_int(CPUSPARCState *env)
}
}
-static void leon3_irq_ack(void *irq_manager, int intno)
+static void leon3_irq_ack(CPUSPARCState *env, int intno)
{
/* No SMP support yet. */
-grlib_irqmp_ack((DeviceState *)irq_manager, 0, intno);
+grlib_irqmp_ack(env->irq_manager, 0, intno);
}
/*
@@ -211,9 +211,9 @@ static void leon3_set_pil_in(void *opaque, int n, int level)
}
}
-static void leon3_irq_manager(CPUSPARCState *env, void *irq_manager, int intno)
+static void leon3_irq_manager(CPUSPARCState *env, int intno)
{
-leon3_irq_ack(irq_manager, intno);
+leon3_irq_ack(env, intno);
leon3_cache_control_int(env);
}
diff --git a/target/sparc/cpu.h b/target/sparc/cpu.h
index 6999a10a40..12a11ecb26 100644
--- a/target/sparc/cpu.h
+++ b/target/sparc/cpu.h
@@ -549,7 +549,7 @@ struct CPUArchState {
sparc_def_t def;
void *irq_manager;
-void (*qemu_irq_ack)(CPUSPARCState *env, void *irq_manager, int intno);
+void (*qemu_irq_ack)(CPUSPARCState *env, int intno);
/* Leon3 cache control */
uint32_t cache_control;
diff --git a/target/sparc/int32_helper.c b/target/sparc/int32_helper.c
index 1563613582..8f4e08ed09 100644
--- a/target/sparc/int32_helper.c
+++ b/target/sparc/int32_helper.c
@@ -160,7 +160,7 @@ void sparc_cpu_do_interrupt(CPUState *cs)
#if !defined(CONFIG_USER_ONLY)
/* IRQ acknowledgment */
if ((intno & ~15) == TT_EXTINT && env->qemu_irq_ack != NULL) {
-env->qemu_irq_ack(env, env->irq_manager, intno);
+env->qemu_irq_ack(env, intno);
}
#endif
}
--
2.25.1
[PATCH 4/9] intc/grlib_irqmp: implements multicore irq
Now there is an ncpus property, use it in order to deliver the IRQ to
multiple CPU.
Co-developed-by: Frederic Konrad
Signed-off-by: Clément Chigot
---
hw/intc/grlib_irqmp.c | 43 ++-
hw/sparc/leon3.c | 3 ++-
include/hw/intc/grlib_irqmp.h | 2 +-
3 files changed, 25 insertions(+), 23 deletions(-)
diff --git a/hw/intc/grlib_irqmp.c b/hw/intc/grlib_irqmp.c
index be0e840181..4df0293064 100644
--- a/hw/intc/grlib_irqmp.c
+++ b/hw/intc/grlib_irqmp.c
@@ -64,7 +64,7 @@ struct IRQMP {
unsigned int ncpus;
IRQMPState *state;
qemu_irq start_signal[IRQMP_MAX_CPU];
-qemu_irq irq;
+qemu_irq irq[IRQMP_MAX_CPU];
};
struct IRQMPState {
@@ -83,37 +83,37 @@ struct IRQMPState {
static void grlib_irqmp_check_irqs(IRQMPState *state)
{
-uint32_t pend = 0;
-uint32_t level0 = 0;
-uint32_t level1 = 0;
+uint32_t pend = 0;
+uint32_t level0 = 0;
+uint32_t level1 = 0;
+int i;
assert(state != NULL);
assert(state->parent != NULL);
-/* IRQ for CPU 0 (no SMP support) */
-pend = (state->pending | state->force[0])
-& state->mask[0];
-
-level0 = pend & ~state->level;
-level1 = pend & state->level;
+for (i = 0; i < state->parent->ncpus; i++) {
+pend = (state->pending | state->force[i]) & state->mask[i];
+level0 = pend & ~state->level;
+level1 = pend & state->level;
-trace_grlib_irqmp_check_irqs(state->pending, state->force[0],
- state->mask[0], level1, level0);
+trace_grlib_irqmp_check_irqs(state->pending, state->force[i],
+ state->mask[i], level1, level0);
-/* Trigger level1 interrupt first and level0 if there is no level1 */
-qemu_set_irq(state->parent->irq, level1 ?: level0);
+/* Trigger level1 interrupt first and level0 if there is no level1 */
+qemu_set_irq(state->parent->irq[i], level1 ?: level0);
+}
}
-static void grlib_irqmp_ack_mask(IRQMPState *state, uint32_t mask)
+static void grlib_irqmp_ack_mask(IRQMPState *state, int cpu, uint32_t mask)
{
/* Clear registers */
state->pending &= ~mask;
-state->force[0] &= ~mask; /* Only CPU 0 (No SMP support) */
+state->force[cpu] &= ~mask;
grlib_irqmp_check_irqs(state);
}
-void grlib_irqmp_ack(DeviceState *dev, int intno)
+void grlib_irqmp_ack(DeviceState *dev, int cpu, int intno)
{
IRQMP*irqmp = GRLIB_IRQMP(dev);
IRQMPState *state;
@@ -127,7 +127,7 @@ void grlib_irqmp_ack(DeviceState *dev, int intno)
trace_grlib_irqmp_ack(intno);
-grlib_irqmp_ack_mask(state, mask);
+grlib_irqmp_ack_mask(state, cpu, mask);
}
static void grlib_irqmp_set_irq(void *opaque, int irq, int level)
@@ -153,7 +153,6 @@ static void grlib_irqmp_set_irq(void *opaque, int irq, int
level)
s->pending |= 1 << irq;
}
grlib_irqmp_check_irqs(s);
-
}
}
@@ -257,7 +256,9 @@ static void grlib_irqmp_write(void *opaque, hwaddr addr,
case CLEAR_OFFSET:
value &= ~1; /* clean up the value */
-grlib_irqmp_ack_mask(state, value);
+for (i = 0; i < irqmp->ncpus; i++) {
+grlib_irqmp_ack_mask(state, i, value);
+}
return;
case MP_STATUS_OFFSET:
@@ -352,7 +353,7 @@ static void grlib_irqmp_realize(DeviceState *dev, Error
**errp)
/* Transitionning from 0 to 1 starts the CPUs. */
qdev_init_gpio_out_named(dev, irqmp->start_signal, "grlib-start-cpu",
IRQMP_MAX_CPU);
-qdev_init_gpio_out_named(dev, &irqmp->irq, "grlib-irq", 1);
+qdev_init_gpio_out_named(dev, irqmp->irq, "grlib-irq", irqmp->ncpus);
memory_region_init_io(&irqmp->iomem, OBJECT(dev), &grlib_irqmp_ops, irqmp,
"irqmp", IRQMP_REG_SIZE);
diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index 7b9809b81f..94d8ec94b0 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -168,7 +168,8 @@ static void leon3_cache_control_int(CPUSPARCState *env)
static void leon3_irq_ack(void *irq_manager, int intno)
{
-grlib_irqmp_ack((DeviceState *)irq_manager, intno);
+/* No SMP support yet. */
+grlib_irqmp_ack((DeviceState *)irq_manager, 0, intno);
}
/*
diff --git a/include/hw/intc/grlib_irqmp.h b/include/hw/intc/grlib_irqmp.h
index b9cc584168..776a2508e1 100644
--- a/include/hw/intc/grlib_irqmp.h
+++ b/include/hw/intc/grlib_irqmp.h
@@ -34,6 +34,6 @@
/* IRQMP */
#define TYPE_GRLIB_IRQMP "grlib-irqmp"
-void grlib_irqmp_ack(DeviceState *dev, int intno);
+void grlib_irqmp_ack(DeviceState *dev, int cpu, int intno);
#endif /* GRLIB_IRQMP_H */
--
2.25.1
[PATCH 8/9] leon3: check cpu_id in the tiny bootloader
Now that SMP is possible, the asr17 must be checked in the little boot code
or the secondary CPU will reinitialize the Timer and the Uart.
Co-developed-by: Frederic Konrad
Signed-off-by: Clément Chigot
---
hw/sparc/leon3.c | 22 --
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index 38fb8d9af1..7498eaa827 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -98,13 +98,27 @@ static uint32_t *gen_store_u32(uint32_t *code, hwaddr addr,
uint32_t val)
/*
* When loading a kernel in RAM the machine is expected to be in a different
- * state (eg: initialized by the bootloader). This little code reproduces
- * this behavior.
+ * state (eg: initialized by the bootloader). This little code reproduces
+ * this behavior. Also this code can be executed by the secondary cpus as
+ * well since it looks at the %asr17 register before doing any
+ * initialization, it allows to use the same reset address for all the
+ * cpus.
*/
static void write_bootloader(CPUSPARCState *env, uint8_t *base,
hwaddr kernel_addr)
{
uint32_t *p = (uint32_t *) base;
+uint32_t *sec_cpu_branch_p = NULL;
+
+/* If we are running on a secondary CPU, jump directly to the kernel. */
+
+stl_p(p++, 0x85444000); /* rd %asr17, %g2 */
+stl_p(p++, 0x8530a01c); /* srl %g2, 0x1c, %g2 */
+stl_p(p++, 0x80908000); /* tst %g2*/
+/* Fill that later. */
+sec_cpu_branch_p = p;
+stl_p(p++, 0x0BADC0DE); /* bne xxx */
+stl_p(p++, 0x0100); /* nop */
/* Initialize the UARTs*/
/* *UART_CONTROL = UART_RECEIVE_ENABLE | UART_TRANSMIT_ENABLE; */
@@ -118,6 +132,10 @@ static void write_bootloader(CPUSPARCState *env, uint8_t
*base,
/* *GPTIMER0_CONFIG = GPTIMER_ENABLE | GPTIMER_RESTART;*/
p = gen_store_u32(p, 0x8318, 3);
+/* Now, the relative branch above can be computed. */
+stl_p(sec_cpu_branch_p, 0x1280
+ + (p - sec_cpu_branch_p));
+
/* JUMP to the entry point */
stl_p(p++, 0x8210); /* mov %g0, %g1 */
stl_p(p++, 0x0300 + extract32(kernel_addr, 10, 22));
--
2.25.1
Re: [PATCH for-8.2] accel/tcg: Remove CF_LAST_IO
Tested-by: Clément Chigot
On Fri, Nov 10, 2023 at 6:08 PM Richard Henderson
wrote:
>
> In cpu_exec_step_atomic, we did not set CF_LAST_IO, which can
> lead to a loop with cpu_io_recompile.
>
> But since 18a536f1f8 ("Always require can_do_io") we no longer need
> a flag to indicate when the last insn should have can_do_io set, so
> remove the flag entirely.
>
> Reported-by: Clément Chigot
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1961
> Signed-off-by: Richard Henderson
> ---
> docs/devel/tcg-icount.rst| 6 --
> include/exec/translation-block.h | 13 ++---
> accel/tcg/cpu-exec.c | 2 +-
> accel/tcg/tb-maint.c | 6 ++
> accel/tcg/translate-all.c| 4 ++--
> accel/tcg/translator.c | 22 +-
> system/watchpoint.c | 6 ++
> 7 files changed, 22 insertions(+), 37 deletions(-)
>
> diff --git a/docs/devel/tcg-icount.rst b/docs/devel/tcg-icount.rst
> index 50c8e8dabc..7df883446a 100644
> --- a/docs/devel/tcg-icount.rst
> +++ b/docs/devel/tcg-icount.rst
> @@ -62,12 +62,6 @@ To deal with this case, when an I/O access is made we:
>- re-compile a single [1]_ instruction block for the current PC
>- exit the cpu loop and execute the re-compiled block
>
> -The new block is created with the CF_LAST_IO compile flag which
> -ensures the final instruction translation starts with a call to
> -gen_io_start() so we don't enter a perpetual loop constantly
> -recompiling a single instruction block. For translators using the
> -common translator_loop this is done automatically.
> -
> .. [1] sometimes two instructions if dealing with delay slots
>
> Other I/O operations
> diff --git a/include/exec/translation-block.h
> b/include/exec/translation-block.h
> index b785751774..e2b26e16da 100644
> --- a/include/exec/translation-block.h
> +++ b/include/exec/translation-block.h
> @@ -71,13 +71,12 @@ struct TranslationBlock {
> #define CF_NO_GOTO_TB0x0200 /* Do not chain with goto_tb */
> #define CF_NO_GOTO_PTR 0x0400 /* Do not chain with goto_ptr */
> #define CF_SINGLE_STEP 0x0800 /* gdbstub single-step in effect */
> -#define CF_LAST_IO 0x8000 /* Last insn may be an IO access. */
> -#define CF_MEMI_ONLY 0x0001 /* Only instrument memory ops */
> -#define CF_USE_ICOUNT0x0002
> -#define CF_INVALID 0x0004 /* TB is stale. Set with @jmp_lock held
> */
> -#define CF_PARALLEL 0x0008 /* Generate code for a parallel context
> */
> -#define CF_NOIRQ 0x0010 /* Generate an uninterruptible TB */
> -#define CF_PCREL 0x0020 /* Opcodes in TB are PC-relative */
> +#define CF_MEMI_ONLY 0x1000 /* Only instrument memory ops */
> +#define CF_USE_ICOUNT0x2000
> +#define CF_INVALID 0x4000 /* TB is stale. Set with @jmp_lock held
> */
> +#define CF_PARALLEL 0x8000 /* Generate code for a parallel context
> */
> +#define CF_NOIRQ 0x0001 /* Generate an uninterruptible TB */
> +#define CF_PCREL 0x0002 /* Opcodes in TB are PC-relative */
> #define CF_CLUSTER_MASK 0xff00 /* Top 8 bits are cluster ID */
> #define CF_CLUSTER_SHIFT 24
>
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 1a5bc90220..c938eb96f8 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -721,7 +721,7 @@ static inline bool cpu_handle_exception(CPUState *cpu,
> int *ret)
> && cpu->neg.icount_decr.u16.low + cpu->icount_extra == 0) {
> /* Execute just one insn to trigger exception pending in the log
> */
> cpu->cflags_next_tb = (curr_cflags(cpu) & ~CF_USE_ICOUNT)
> -| CF_LAST_IO | CF_NOIRQ | 1;
> +| CF_NOIRQ | 1;
> }
> #endif
> return false;
> diff --git a/accel/tcg/tb-maint.c b/accel/tcg/tb-maint.c
> index e678d20dc2..3d2a896220 100644
> --- a/accel/tcg/tb-maint.c
> +++ b/accel/tcg/tb-maint.c
> @@ -1083,8 +1083,7 @@ bool tb_invalidate_phys_page_unwind(tb_page_addr_t
> addr, uintptr_t pc)
> if (current_tb_modified) {
> /* Force execution of one insn next time. */
> CPUState *cpu = current_cpu;
> -cpu->cflags_next_tb =
> -1 | CF_LAST_IO | CF_NOIRQ | curr_cflags(current_cpu);
> +cpu->cflags_next_tb = 1 | CF_NOIRQ | curr_cflags(current_cpu);
> return true;
> }
> return false;
> @@ -1154,8 +1153,7 @@ tb_invalidate_phys_page_range__locked(struct
> page_collection *pages,
> if (current_tb_modified) {
> page_collection_unlock(pages);
> /* Force execut
[PATCH v2] target/riscv: don't verify ISA compatibility for zicntr and zihpm
The extensions zicntr and zihpm were officially added in the privilege
instruction set specification 1.12. However, QEMU has been implemented
them long before it and thus they are forced to be on during the cpu
initialization to ensure compatibility (see riscv_cpu_init).
riscv_cpu_disable_priv_spec_isa_exts was not updated when the above
behavior was introduced, resulting in these extensions to be disabled
after all.
Signed-off-by: Clément Chigot
Fixes: c004099330 ("target/riscv: add zicntr extension flag for TCG")
Fixes: 0824121660 ("target/riscv: add zihpm extension flag for TCG")
Reviewed-by: Daniel Henrique Barboza
---
target/riscv/tcg/tcg-cpu.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 08adad304d..8a35683a34 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -250,6 +250,15 @@ static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU
*cpu)
for (edata = isa_edata_arr; edata && edata->name; edata++) {
if (isa_ext_is_enabled(cpu, edata->ext_enable_offset) &&
(env->priv_ver < edata->min_version)) {
+/*
+ * These two extensions are always enabled as they were supported
+ * by QEMU before they were added as extensions in the ISA.
+ */
+if (!strcmp(edata->name, "zicntr") ||
+!strcmp(edata->name, "zihpm")) {
+continue;
+}
+
isa_ext_update_enabled(cpu, edata->ext_enable_offset, false);
#ifndef CONFIG_USER_ONLY
warn_report("disabling %s extension for hart 0x" TARGET_FMT_lx
--
2.25.1
[PATCH] target/riscv: don't verify ISA compatibility for zicntr and zihpm
The extensions zicntr and zihpm were officially added in the privilege
instruction set specification 1.12. However, QEMU has been implemented
them long before it and thus they are forced to be on during the cpu
initialization to ensure compatibility (see riscv_cpu_init).
riscv_cpu_disable_priv_spec_isa_exts was not updated when the above
behavior was introduced, resulting in these extensions to be disabled
after all.
Signed-off-by: Clément Chigot
---
target/riscv/tcg/tcg-cpu.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 08adad304d..8a35683a34 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -250,6 +250,15 @@ static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU
*cpu)
for (edata = isa_edata_arr; edata && edata->name; edata++) {
if (isa_ext_is_enabled(cpu, edata->ext_enable_offset) &&
(env->priv_ver < edata->min_version)) {
+/*
+ * These two extensions are always enabled as they were supported
+ * by QEMU before they were added as extensions in the ISA.
+ */
+if (!strcmp(edata->name, "zicntr") ||
+!strcmp(edata->name, "zihpm")) {
+continue;
+}
+
isa_ext_update_enabled(cpu, edata->ext_enable_offset, false);
#ifndef CONFIG_USER_ONLY
warn_report("disabling %s extension for hart 0x" TARGET_FMT_lx
--
2.25.1
Re: [PATCH v3 0/4] riscv: zicntr/zihpm flags and disable support
Hi Daniel, This series is triggering warnings when instantiating a CPU having a spec version older than 1.12. | $ qemu-system-riscv32 -M sifive_e | qemu-system-riscv32: warning: disabling zicntr extension for hart 0x because privilege spec version does not match | qemu-system-riscv32: warning: disabling zihpm extension for hart 0x because privilege spec version does not match And IIUC cpu-tcg.c:riscv_cpu_disable_priv_spec_isa_exts(), they will end up being disabled as a result of these warnings. I think these two extensions should be skipped in the above function. Though we can also disable them on purpose in those old CPUs. WDYT ? Thanks, Clément On Mon, Oct 23, 2023 at 5:40 PM Daniel Henrique Barboza wrote: > > Hi, > > In this v3 the patches that added the extensions flags were squashed > with the patches that handled the disablement of the extensions in TCG, > as suggested by Alistair in v2. > > No other change made. Patches based on Alistair's riscv-to-apply.next. > > Patches missing acks: patch 3 > > Changes from v2: > - patch 2: squashed with patch 1 > - patch 5: squashed with patch 4 > - v2 link: > https://lore.kernel.org/qemu-riscv/20231017221226.136764-1-dbarb...@ventanamicro.com/ > > Daniel Henrique Barboza (4): > target/riscv: add zicntr extension flag for TCG > target/riscv/kvm: add zicntr reg > target/riscv: add zihpm extension flag for TCG > target/riscv/kvm: add zihpm reg > > target/riscv/cpu.c | 15 +++ > target/riscv/cpu_cfg.h | 2 ++ > target/riscv/csr.c | 4 > target/riscv/kvm/kvm-cpu.c | 2 ++ > target/riscv/tcg/tcg-cpu.c | 21 + > 5 files changed, 44 insertions(+) > > -- > 2.41.0 > >
Re: [PATCH 6/6] accel/tcg: Always require can_do_io
On Thu, Oct 26, 2023 at 2:44 AM Richard Henderson
wrote:
>
> On 10/24/23 02:50, Clément Chigot wrote:
> > Hi Richard,
> >
> > This commit has broken some of our internal bareboard testing on
> > Risc-V 64. At some point in our programs, there is an AMOSWAP (=
> > atomic swap) instruction on I/O. But since this commit, can_do_io is
> > set to false triggering an infinite loop.
> > IIUC the doc (cf [1]), atomic operations on I/O are allowed.
> >
> > I think there is a CF_LAST_IO flag missing somewhere to allow it, but
> > I'm not sure where this should be. Do you have any ideas ?
> >
> > Sadly I cannot provide a reproducer that easily, mainly because our
> > microchip has a few patches not yet merged making our binaries not
> > running on the upstream master.
> > But here is a bit of the in_asm backtrace:
> >
> >| IN: system__bb__riscv_plic__initialize
> >| Priv: 3; Virt: 0
> >| 0x8eb4: 1141 addisp,sp,-16
> >| 0x8eb6: 0c0027b7 lui a5,49154
> >| 0x8eba: e406 sd ra,8(sp)
> >| 0x8ebc: 00010597 auipc a1,16
> > # 0x80010ebc
> >| 0x8ec0: 47458593 addia1,a1,1140
> >| 0x8ec4: f3ffe637 lui a2,-49154
> >| 0x8ec8: 01878693 addia3,a5,24
> >| 0x8ecc: 00f58733 add a4,a1,a5
> >| 0x8ed0: 9732 add a4,a4,a2
> >| 0x8ed2: 4318 lw a4,0(a4)
> >| 0x8ed4: 2701 sext.w a4,a4
> >| 0x8ed6: 08e7a02f amoswap.w zero,a4,(a5)
> >| 0x8eda: 0791 addia5,a5,4
> >| 0x8edc: fed798e3 bne a5,a3,-16
> > # 0x8ecc
> >|
> >|
> >| IN: system__bb__riscv_plic__initialize
> >| Priv: 3; Virt: 0
> >| 0x8ed6: 08e7a02f amoswap.w zero,a4,(a5)
> >|
> >|
> >| IN: system__bb__riscv_plic__initialize
> >| Priv: 3; Virt: 0
> >| 0x8ed6: 08e7a02f amoswap.w zero,a4,(a5)
> >| * Freeze *
>
> I would expect two translations:
>
> (1) with the original TB, aborts execution on !can_do_io.
> (2) with the second TB, we get further into the actual execution and abort
> execution on
> TLB_MMIO.
> (3) with the third TB, we clear CF_PARALLEL and execute under
> cpu_exec_step_atomic.
>
> Both 2 and 3 should have had CF_LAST_IO set.
> You can verify this with '-d exec' output.
>
> As a trivial example from qemu-system-alpha bios startup:
>
> > Trace 0: 0x7f2584008380 [/fc003ee4/0100/ff00]
> > uart_init_line
> > cpu_io_recompile: rewound execution of TB to fc003ee4
> >
> > IN: uart_init_line
> > 0xfc003f20: stbt8,0(t6)
> >
> > Trace 0: 0x7f2584008a00 [/fc003f20/0100/ff018001]
> > uart_init_line
>
> Note that the final "/" field is cflags. The first "Trace" corresponds to
> (1), where the
> store is in the middle of the TB. You can see the io_recompile abort, then
> the second
> "Trace" contains {CF_COUNT=1, CF_LAST_IO, CF_MEMI_ONLY}.
With the exec, it's indeed a bit clearer.
I do have a new translation made by cpu_io_recompile which sets
CF_LAST_IO (2). But afterward, it somehow loops back to the previous
translation which doesn't have CF_LAST_IO which calls cpu_io_recompile
again, etc. This triggers an infinite loop between these two
translations
|
| IN: system__bb__riscv_plic__initialize
| 0x8eb4: 1141 addisp,sp,-16
| ...
| 0x8ed4: 2701 sext.w a4,a4
| 0x8ed6: 08e7a02f amoswap.w zero,a4,(a5)
| 0x8eda: 0791 addia5,a5,4
| 0x8edc: fed798e3 bne a5,a3,-16
# 0x8ecc
|
| Linking TBs 0x7f0d3199db40 index 0 -> 0x7f0d3199dd00
| Trace 1: 0x7f0d3199dd00
[/8eb4/0b02401b/ff28]
system__bb__riscv_plic__initialize
(1) First exec of amoswap without CF_LAST_IO.
|
| IN: system__bb__riscv_plic__initialize
| Priv: 3; Virt: 0
| 0x8ed6: 08e7a02f amoswap.w zero,a4,
Re: [PATCH 6/6] accel/tcg: Always require can_do_io
Hi Richard,
This commit has broken some of our internal bareboard testing on
Risc-V 64. At some point in our programs, there is an AMOSWAP (=
atomic swap) instruction on I/O. But since this commit, can_do_io is
set to false triggering an infinite loop.
IIUC the doc (cf [1]), atomic operations on I/O are allowed.
I think there is a CF_LAST_IO flag missing somewhere to allow it, but
I'm not sure where this should be. Do you have any ideas ?
Sadly I cannot provide a reproducer that easily, mainly because our
microchip has a few patches not yet merged making our binaries not
running on the upstream master.
But here is a bit of the in_asm backtrace:
| IN: system__bb__riscv_plic__initialize
| Priv: 3; Virt: 0
| 0x8eb4: 1141 addisp,sp,-16
| 0x8eb6: 0c0027b7 lui a5,49154
| 0x8eba: e406 sd ra,8(sp)
| 0x8ebc: 00010597 auipc a1,16
# 0x80010ebc
| 0x8ec0: 47458593 addia1,a1,1140
| 0x8ec4: f3ffe637 lui a2,-49154
| 0x8ec8: 01878693 addia3,a5,24
| 0x8ecc: 00f58733 add a4,a1,a5
| 0x8ed0: 9732 add a4,a4,a2
| 0x8ed2: 4318 lw a4,0(a4)
| 0x8ed4: 2701 sext.w a4,a4
| 0x8ed6: 08e7a02f amoswap.w zero,a4,(a5)
| 0x8eda: 0791 addia5,a5,4
| 0x8edc: fed798e3 bne a5,a3,-16
# 0x8ecc
|
|
| IN: system__bb__riscv_plic__initialize
| Priv: 3; Virt: 0
| 0x8ed6: 08e7a02f amoswap.w zero,a4,(a5)
|
|
| IN: system__bb__riscv_plic__initialize
| Priv: 3; Virt: 0
| 0x8ed6: 08e7a02f amoswap.w zero,a4,(a5)
| * Freeze *
a5 (= 0xc002000) above is targeting an address inside sifive_plic HW.
Note IIUC the doc (cf [1]), atomic operations on I/O are allowed.
[1]
https://github.com/riscv/riscv-isa-manual/blob/main/src/a-st-ext.adoc#atomic-memory-operations
Thanks in advance.
On Thu, Sep 14, 2023 at 7:45 PM Richard Henderson
wrote:
>
> Require i/o as the last insn of a TranslationBlock always,
> not only with icount. This is required for i/o that alters
> the address space, such as a pci config space write.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1866
> Signed-off-by: Richard Henderson
> ---
> accel/tcg/translator.c | 20 +++-
> target/mips/tcg/translate.c | 1 -
> 2 files changed, 7 insertions(+), 14 deletions(-)
>
> diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
> index dd507cd471..358214d526 100644
> --- a/accel/tcg/translator.c
> +++ b/accel/tcg/translator.c
> @@ -28,12 +28,6 @@ static void set_can_do_io(DisasContextBase *db, bool val)
>
> bool translator_io_start(DisasContextBase *db)
> {
> -uint32_t cflags = tb_cflags(db->tb);
> -
> -if (!(cflags & CF_USE_ICOUNT)) {
> -return false;
> -}
> -
> set_can_do_io(db, true);
>
> /*
> @@ -86,15 +80,15 @@ static TCGOp *gen_tb_start(DisasContextBase *db, uint32_t
> cflags)
> tcg_gen_st16_i32(count, cpu_env,
> offsetof(ArchCPU, neg.icount_decr.u16.low) -
> offsetof(ArchCPU, env));
> -/*
> - * cpu->can_do_io is set automatically here at the beginning of
> - * each translation block. The cost is minimal and only paid for
> - * -icount, plus it would be very easy to forget doing it in the
> - * translator.
> - */
> -set_can_do_io(db, db->max_insns == 1 && (cflags & CF_LAST_IO));
> }
>
> +/*
> + * cpu->can_do_io is set automatically here at the beginning of
> + * each translation block. The cost is minimal, plus it would be
> + * very easy to forget doing it in the translator.
> + */
> +set_can_do_io(db, db->max_insns == 1 && (cflags & CF_LAST_IO));
> +
> return icount_start_insn;
> }
>
> diff --git a/target/mips/tcg/translate.c b/target/mips/tcg/translate.c
> index 9bb40f1849..593fc80458 100644
> --- a/target/mips/tcg/translate.c
> +++ b/target/mips/tcg/translate.c
> @@ -11212,7 +11212,6 @@ static void gen_branch(DisasContext *ctx, int
> insn_bytes)
> /* Branches completion */
> clear_branch_hflags(ctx);
> ctx->base.is_jmp = DISAS_NORETURN;
> -/* FIXME: Need to clear can_do_io. */
> switch (proc_hflags & MIPS_HFLAG_BMASK_BASE) {
> case MIPS_HFLAG_FBNSLOT:
> gen_goto_tb(ctx, 0, ctx->base.pc_next + insn_bytes);
> --
> 2.34.1
>
>
[PATCH v4 2/5] softmmu: pass the main loop status to gdb "Wxx" packet
gdb_exit function aims to close gdb sessions and sends the exit code of
the current execution. It's being called by qemu_cleanup once the main
loop is over.
Until now, the exit code sent was always 0. Now that hardware can
shutdown this main loop with custom exit codes, these codes must be
transfered to gdb as well.
Signed-off-by: Clément Chigot
Reviewed-by: Alistair Francis
---
include/sysemu/sysemu.h | 2 +-
softmmu/main.c | 2 +-
softmmu/runstate.c | 4 ++--
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 25be2a692e..73a37949c2 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -101,7 +101,7 @@ bool defaults_enabled(void);
void qemu_init(int argc, char **argv);
int qemu_main_loop(void);
-void qemu_cleanup(void);
+void qemu_cleanup(int);
extern QemuOptsList qemu_legacy_drive_opts;
extern QemuOptsList qemu_common_drive_opts;
diff --git a/softmmu/main.c b/softmmu/main.c
index 694388bd7f..9b91d21ea8 100644
--- a/softmmu/main.c
+++ b/softmmu/main.c
@@ -35,7 +35,7 @@ int qemu_default_main(void)
int status;
status = qemu_main_loop();
-qemu_cleanup();
+qemu_cleanup(status);
return status;
}
diff --git a/softmmu/runstate.c b/softmmu/runstate.c
index 363a5ea8dd..ea9d6c2a32 100644
--- a/softmmu/runstate.c
+++ b/softmmu/runstate.c
@@ -834,9 +834,9 @@ void qemu_init_subsystems(void)
}
-void qemu_cleanup(void)
+void qemu_cleanup(int status)
{
-gdb_exit(0);
+gdb_exit(status);
/*
* cleaning up the migration object cancels any existing migration
--
2.25.1
[PATCH v4 0/5] Risc-V/gdb: replace exit calls with proper shutdown
This series replaces some of the call to exit in hardware used by Risc-V boards. Otherwise, the gdb connection can be abruptly disconnected resulting in the last gdb packet "Wxx" being not sent. For the gdbstub modification, gdb_exit calls ensure that the "Wxx" packet is sent before exiting. However, some features (see net/vhost-vdpa.c: vhost_vdpa_cleanup for example) are expecting that a cleanup is being made before exiting. This, it's probably safer to follow the same logic here as well. Difference with v3: - Rebase on riscv-to-apply Clément Chigot (5): softmmu: add means to pass an exit code when requesting a shutdown softmmu: pass the main loop status to gdb "Wxx" packet hw/misc/sifive_test.c: replace exit calls with proper shutdown hw/char: riscv_htif: replace exit calls with proper shutdown gdbstub: replace exit calls with proper shutdown for softmmu gdbstub/gdbstub.c | 5 +++-- gdbstub/softmmu.c | 6 ++ gdbstub/user.c | 6 ++ hw/char/riscv_htif.c | 5 - hw/misc/sifive_test.c | 9 +++-- include/gdbstub/syscalls.h | 9 + include/sysemu/runstate.h | 2 ++ include/sysemu/sysemu.h| 2 +- softmmu/main.c | 2 +- softmmu/runstate.c | 16 +--- 10 files changed, 52 insertions(+), 10 deletions(-) -- 2.25.1
[PATCH v4 5/5] gdbstub: replace exit calls with proper shutdown for softmmu
This replaces the exit calls by shutdown requests, ensuring a proper
cleanup of Qemu. Features like net/vhost-vdpa.c are expecting
qemu_cleanup to be called to remove their last residuals.
Signed-off-by: Clément Chigot
Reviewed-by: Alistair Francis
---
gdbstub/gdbstub.c | 5 +++--
gdbstub/softmmu.c | 6 ++
gdbstub/user.c | 6 ++
include/gdbstub/syscalls.h | 9 +
4 files changed, 24 insertions(+), 2 deletions(-)
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 349d348c7b..1cb6d65306 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -1327,7 +1327,7 @@ static void handle_v_kill(GArray *params, void *user_ctx)
gdb_put_packet("OK");
error_report("QEMU: Terminated via GDBstub");
gdb_exit(0);
-exit(0);
+gdb_qemu_exit(0);
}
static const GdbCmdParseEntry gdb_v_commands_table[] = {
@@ -1846,7 +1846,8 @@ static int gdb_handle_packet(const char *line_buf)
/* Kill the target */
error_report("QEMU: Terminated via GDBstub");
gdb_exit(0);
-exit(0);
+gdb_qemu_exit(0);
+break;
case 'D':
{
static const GdbCmdParseEntry detach_cmd_desc = {
diff --git a/gdbstub/softmmu.c b/gdbstub/softmmu.c
index 9f0b8b5497..a5d6e04c79 100644
--- a/gdbstub/softmmu.c
+++ b/gdbstub/softmmu.c
@@ -435,6 +435,12 @@ void gdb_exit(int code)
qemu_chr_fe_deinit(&gdbserver_system_state.chr, true);
}
+void gdb_qemu_exit(int code)
+{
+qemu_system_shutdown_request_with_code(SHUTDOWN_CAUSE_GUEST_SHUTDOWN,
+ code);
+}
+
/*
* Memory access
*/
diff --git a/gdbstub/user.c b/gdbstub/user.c
index 7ab6e5d975..dbe1d9b887 100644
--- a/gdbstub/user.c
+++ b/gdbstub/user.c
@@ -113,6 +113,12 @@ void gdb_exit(int code)
gdb_put_packet(buf);
gdbserver_state.allow_stop_reply = false;
}
+
+}
+
+void gdb_qemu_exit(int code)
+{
+exit(code);
}
int gdb_handlesig(CPUState *cpu, int sig)
diff --git a/include/gdbstub/syscalls.h b/include/gdbstub/syscalls.h
index 243eaf8ce4..54ff7245a1 100644
--- a/include/gdbstub/syscalls.h
+++ b/include/gdbstub/syscalls.h
@@ -110,4 +110,13 @@ int use_gdb_syscalls(void);
*/
void gdb_exit(int code);
+/**
+ * gdb_qemu_exit: ask qemu to exit
+ * @code: exit code reported
+ *
+ * This requests qemu to exit. This function is allowed to return as
+ * the exit request might be processed asynchronously by qemu backend.
+ */
+void gdb_qemu_exit(int code);
+
#endif /* _SYSCALLS_H_ */
--
2.25.1
[PATCH v4 3/5] hw/misc/sifive_test.c: replace exit calls with proper shutdown
This replaces the exit calls by shutdown requests, ensuring a proper
cleanup of Qemu. Otherwise, some connections like gdb could be broken
before its final packet ("Wxx") is being sent. This part, being done
inside qemu_cleanup function, can be reached only when the main loop
exits after a shutdown request.
Signed-off-by: Clément Chigot
Reviewed-by: Alistair Francis
---
hw/misc/sifive_test.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/hw/misc/sifive_test.c b/hw/misc/sifive_test.c
index 56df45bfe5..ad688079c4 100644
--- a/hw/misc/sifive_test.c
+++ b/hw/misc/sifive_test.c
@@ -25,6 +25,7 @@
#include "qemu/module.h"
#include "sysemu/runstate.h"
#include "hw/misc/sifive_test.h"
+#include "sysemu/sysemu.h"
static uint64_t sifive_test_read(void *opaque, hwaddr addr, unsigned int size)
{
@@ -39,9 +40,13 @@ static void sifive_test_write(void *opaque, hwaddr addr,
int code = (val64 >> 16) & 0x;
switch (status) {
case FINISHER_FAIL:
-exit(code);
+qemu_system_shutdown_request_with_code(
+SHUTDOWN_CAUSE_GUEST_PANIC, code);
+return;
case FINISHER_PASS:
-exit(0);
+qemu_system_shutdown_request_with_code(
+SHUTDOWN_CAUSE_GUEST_SHUTDOWN, code);
+return;
case FINISHER_RESET:
qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
return;
--
2.25.1
[PATCH v4 4/5] hw/char: riscv_htif: replace exit calls with proper shutdown
This replaces the exit calls by shutdown requests, ensuring a proper
cleanup of Qemu. Otherwise, some connections like gdb could be broken
before its final packet ("Wxx") is being sent. This part, being done
inside qemu_cleanup function, can be reached only when the main loop
exits after a shutdown request.
Signed-off-by: Clément Chigot
Reviewed-by: Alistair Francis
---
hw/char/riscv_htif.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
index 40de6b8b77..9bef60def1 100644
--- a/hw/char/riscv_htif.c
+++ b/hw/char/riscv_htif.c
@@ -32,6 +32,7 @@
#include "exec/address-spaces.h"
#include "exec/tswap.h"
#include "sysemu/dma.h"
+#include "sysemu/runstate.h"
#define RISCV_DEBUG_HTIF 0
#define HTIF_DEBUG(fmt, ...)
\
@@ -206,7 +207,9 @@ static void htif_handle_tohost_write(HTIFState *s, uint64_t
val_written)
g_free(sig_data);
}
-exit(exit_code);
+qemu_system_shutdown_request_with_code(
+SHUTDOWN_CAUSE_GUEST_SHUTDOWN, exit_code);
+return;
} else {
uint64_t syscall[8];
cpu_physical_memory_read(payload, syscall, sizeof(syscall));
--
2.25.1
[PATCH v4 1/5] softmmu: add means to pass an exit code when requesting a shutdown
As of now, the exit code was either EXIT_FAILURE when a panic shutdown
was requested or EXIT_SUCCESS otherwise.
However, some hardware could want to pass more complex exit codes. Thus,
introduce a new shutdown request function allowing that.
Signed-off-by: Clément Chigot
Reviewed-by: Alistair Francis
---
include/sysemu/runstate.h | 2 ++
softmmu/runstate.c| 12 +++-
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
index 08afb97695..c8c2bd8a61 100644
--- a/include/sysemu/runstate.h
+++ b/include/sysemu/runstate.h
@@ -68,6 +68,8 @@ void qemu_system_wakeup_request(WakeupReason reason, Error
**errp);
void qemu_system_wakeup_enable(WakeupReason reason, bool enabled);
void qemu_register_wakeup_notifier(Notifier *notifier);
void qemu_register_wakeup_support(void);
+void qemu_system_shutdown_request_with_code(ShutdownCause reason,
+int exit_code);
void qemu_system_shutdown_request(ShutdownCause reason);
void qemu_system_powerdown_request(void);
void qemu_register_powerdown_notifier(Notifier *notifier);
diff --git a/softmmu/runstate.c b/softmmu/runstate.c
index 1652ed0439..363a5ea8dd 100644
--- a/softmmu/runstate.c
+++ b/softmmu/runstate.c
@@ -385,6 +385,7 @@ void vm_state_notify(bool running, RunState state)
static ShutdownCause reset_requested;
static ShutdownCause shutdown_requested;
+static int shutdown_exit_code = EXIT_SUCCESS;
static int shutdown_signal;
static pid_t shutdown_pid;
static int powerdown_requested;
@@ -664,6 +665,13 @@ void qemu_system_killed(int signal, pid_t pid)
qemu_notify_event();
}
+void qemu_system_shutdown_request_with_code(ShutdownCause reason,
+int exit_code)
+{
+shutdown_exit_code = exit_code;
+qemu_system_shutdown_request(reason);
+}
+
void qemu_system_shutdown_request(ShutdownCause reason)
{
trace_qemu_system_shutdown_request(reason);
@@ -725,7 +733,9 @@ static bool main_loop_should_exit(int *status)
if (shutdown_action == SHUTDOWN_ACTION_PAUSE) {
vm_stop(RUN_STATE_SHUTDOWN);
} else {
-if (request == SHUTDOWN_CAUSE_GUEST_PANIC &&
+if (shutdown_exit_code != EXIT_SUCCESS) {
+*status = shutdown_exit_code;
+} else if (request == SHUTDOWN_CAUSE_GUEST_PANIC &&
panic_action == PANIC_ACTION_EXIT_FAILURE) {
*status = EXIT_FAILURE;
}
--
2.25.1
Re: [PATCH v3 4/5] hw/char: riscv_htif: replace exit calls with proper shutdown
On Fri, Sep 22, 2023 at 7:20 AM Alistair Francis wrote:
>
> On Thu, Sep 7, 2023 at 9:26 PM Clément Chigot wrote:
> >
> > This replaces the exit calls by shutdown requests, ensuring a proper
> > cleanup of Qemu. Otherwise, some connections like gdb could be broken
> > before its final packet ("Wxx") is being sent. This part, being done
> > inside qemu_cleanup function, can be reached only when the main loop
> > exits after a shutdown request.
> >
> > Signed-off-by: Clément Chigot
>
> Do you mind rebasing this on:
> https://github.com/alistair23/qemu/tree/riscv-to-apply.next
Thanks for the review.
Just a quick question on the procedure side, is there any special tag
or something to say in the cover letter to state that it has been
rebased on riscv-to-apply instead of the usual master?
Clément
> Alistair
>
> > ---
> > hw/char/riscv_htif.c | 5 -
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
> > index 37d3ccc76b..7e9b6fcc98 100644
> > --- a/hw/char/riscv_htif.c
> > +++ b/hw/char/riscv_htif.c
> > @@ -31,6 +31,7 @@
> > #include "qemu/error-report.h"
> > #include "exec/address-spaces.h"
> > #include "sysemu/dma.h"
> > +#include "sysemu/runstate.h"
> >
> > #define RISCV_DEBUG_HTIF 0
> > #define HTIF_DEBUG(fmt, ...)
> > \
> > @@ -205,7 +206,9 @@ static void htif_handle_tohost_write(HTIFState *s,
> > uint64_t val_written)
> > g_free(sig_data);
> > }
> >
> > -exit(exit_code);
> > +qemu_system_shutdown_request_with_code(
> > +SHUTDOWN_CAUSE_GUEST_SHUTDOWN, exit_code);
> > +return;
> > } else {
> > uint64_t syscall[8];
> > cpu_physical_memory_read(payload, syscall,
> > sizeof(syscall));
> > --
> > 2.25.1
> >
Re: [PATCH v2 3/3] gdbstub: replace exit(0) with proper shutdown
Hi Peter,
On Mon, Sep 4, 2023 at 2:46 PM Clément Chigot wrote:
>
> On Mon, Sep 4, 2023 at 11:42 AM Peter Maydell
> wrote:
> >
> > On Mon, 4 Sept 2023 at 10:36, Clément Chigot wrote:
> > >
> > > On Mon, Sep 4, 2023 at 11:23 AM Peter Maydell
> > > wrote:
> > > >
> > > > On Wed, 23 Aug 2023 at 08:07, Clément Chigot wrote:
> > > > >
> > > > > This replaces the exit(0) call by a shutdown request, ensuring a
> > > > > proper
> > > > > cleanup of Qemu. Otherwise, some connections could be broken without
> > > > > being correctly flushed.
> > > > >
> > > > > Signed-off-by: Clément Chigot
> >
> > > > > +/*
> > > > > + * Shutdown request is a clean way to stop the QEMU, compared
> > > > > + * to a direct call to exit(). But we can't pass the exit code
> > > > > + * through it so avoid doing that when it can matter.
> > > > > + * As this function is also called during the cleanup process,
> > > > > + * avoid sending the request if one is already set.
> > > > > + */
> > > > > +if (code) {
> > > > > +exit(code);
> > > > > +} else if (!qemu_shutdown_requested_get()) {
> > > > > +qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> > > > > +}
> > > > > }
> > > >
> > > > This definitely doesn't look right. Either exit() is OK
> > > > to call, or it is not. We shouldn't be exiting one way
> > > > if the exit status is 0 and another way if it is non-0.
> > >
> > > I do agree but AFAIK, this isn't possible to pass the exit code using
> > > qemu_system_shutdown_request.
> >
> > That would mean that we should add a mechanism to do so.
> >
> > But my opinion is still what I said about the first version
> > of this patchset: we should fix whatever the problem is
> > that means that gdb_exit() is not correctly ensuring that
> > gdb gets the packet response, not paper over it like this.
>
> The main issue is that calling exit(0) bypasses the call of qemu_cleanup().
> For the two other patches, the wrong behavior is obvious: qemu_cleanup
> being not called so is gdb_exit and then the gdb packet is never even
> created, let alone being sent. Replacing exit by a shutdown request
> ensures that the softmmu main loop terminates and that
> qemu_cleanup/gdb_exit is being called.
>
> For this one, I have to verify a bit further. Honestly, I did include
> it for the sake of coherence and because we used to need it. However,
> I've realized that this was earlier to commit b9e10c6c (which adds
> explicit calls to gdb_exit). It might not be mandatory after all, even
> if I still think that this is an improvement as bypassing qemu_cleanup
> could lead to many leaks if contributors expect it to be called once
> the softmmu main loop has started.
For the cases I've tried, there was indeed no leak if qemu_cleanup was
not called.
However, digging a bit in the qemu, I found that net/vhost-vdpa.c is
mentioning that its cleanup function has to be called through
qemu_cleanup in some cases to perform its final sanitation.
Thus, I've improved and kept the commit dealing with gdbstub (in v3
just sent). If you really think it's safer to avoid that, I'm ok to
drop it until I find a leak/issue (if any) caused by qemu_cleanup not
being called here.
Thanks,
Clément
[PATCH v3 5/5] gdbstub: replace exit calls with proper shutdown for softmmu
This replaces the exit calls by shutdown requests, ensuring a proper
cleanup of Qemu. Features like net/vhost-vdpa.c are expecting
qemu_cleanup to be called to remove their last residuals.
Signed-off-by: Clément Chigot
---
gdbstub/gdbstub.c | 5 +++--
gdbstub/softmmu.c | 6 ++
gdbstub/user.c | 6 ++
include/gdbstub/syscalls.h | 9 +
4 files changed, 24 insertions(+), 2 deletions(-)
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 349d348c7b..1cb6d65306 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -1327,7 +1327,7 @@ static void handle_v_kill(GArray *params, void *user_ctx)
gdb_put_packet("OK");
error_report("QEMU: Terminated via GDBstub");
gdb_exit(0);
-exit(0);
+gdb_qemu_exit(0);
}
static const GdbCmdParseEntry gdb_v_commands_table[] = {
@@ -1846,7 +1846,8 @@ static int gdb_handle_packet(const char *line_buf)
/* Kill the target */
error_report("QEMU: Terminated via GDBstub");
gdb_exit(0);
-exit(0);
+gdb_qemu_exit(0);
+break;
case 'D':
{
static const GdbCmdParseEntry detach_cmd_desc = {
diff --git a/gdbstub/softmmu.c b/gdbstub/softmmu.c
index 9f0b8b5497..a5d6e04c79 100644
--- a/gdbstub/softmmu.c
+++ b/gdbstub/softmmu.c
@@ -435,6 +435,12 @@ void gdb_exit(int code)
qemu_chr_fe_deinit(&gdbserver_system_state.chr, true);
}
+void gdb_qemu_exit(int code)
+{
+qemu_system_shutdown_request_with_code(SHUTDOWN_CAUSE_GUEST_SHUTDOWN,
+ code);
+}
+
/*
* Memory access
*/
diff --git a/gdbstub/user.c b/gdbstub/user.c
index 7ab6e5d975..dbe1d9b887 100644
--- a/gdbstub/user.c
+++ b/gdbstub/user.c
@@ -113,6 +113,12 @@ void gdb_exit(int code)
gdb_put_packet(buf);
gdbserver_state.allow_stop_reply = false;
}
+
+}
+
+void gdb_qemu_exit(int code)
+{
+exit(code);
}
int gdb_handlesig(CPUState *cpu, int sig)
diff --git a/include/gdbstub/syscalls.h b/include/gdbstub/syscalls.h
index 243eaf8ce4..54ff7245a1 100644
--- a/include/gdbstub/syscalls.h
+++ b/include/gdbstub/syscalls.h
@@ -110,4 +110,13 @@ int use_gdb_syscalls(void);
*/
void gdb_exit(int code);
+/**
+ * gdb_qemu_exit: ask qemu to exit
+ * @code: exit code reported
+ *
+ * This requests qemu to exit. This function is allowed to return as
+ * the exit request might be processed asynchronously by qemu backend.
+ */
+void gdb_qemu_exit(int code);
+
#endif /* _SYSCALLS_H_ */
--
2.25.1
[PATCH v3 0/5] Risc-V/gdb: replace exit calls with proper shutdown
This series replaces some of the call to exit in hardware used by Risc-V boards or made when gdb is requested to exit by shutdown requests. Otherwise, the gdb connection can be abruptly disconnected resulting in the last gdb packet "Wxx" being not sent. For the gdbstub modification, gdb_exit calls ensure that the "Wxx" packet is sent before exiting. However, some features (see net/vhost-vdpa.c: vhost_vdpa_cleanup for example) are expecting that a cleanup is being made before exiting. This, it's probably safer to follow the same logic here as well. Difference with v2: - Add support to request a shutdown with a specific exit code. - Pass the exit code of the main loop to gdb_exit call in qemu_cleanup - gdbstub: move the request shutdown in a new new function to avoid having to worry about the request having already been sent. Clément Chigot (5): softmmu: add means to pass an exit code when requesting a shutdown softmmu: pass the main loop status to gdb "Wxx" packet hw/misc/sifive_test.c: replace exit calls with proper shutdown hw/char: riscv_htif: replace exit calls with proper shutdown gdbstub: replace exit calls with proper shutdown for softmmu gdbstub/gdbstub.c | 5 +++-- gdbstub/softmmu.c | 6 ++ gdbstub/user.c | 6 ++ hw/char/riscv_htif.c | 5 - hw/misc/sifive_test.c | 9 +++-- include/gdbstub/syscalls.h | 9 + include/sysemu/runstate.h | 2 ++ include/sysemu/sysemu.h| 2 +- softmmu/main.c | 2 +- softmmu/runstate.c | 16 +--- 10 files changed, 52 insertions(+), 10 deletions(-) -- 2.25.1
[PATCH v3 4/5] hw/char: riscv_htif: replace exit calls with proper shutdown
This replaces the exit calls by shutdown requests, ensuring a proper
cleanup of Qemu. Otherwise, some connections like gdb could be broken
before its final packet ("Wxx") is being sent. This part, being done
inside qemu_cleanup function, can be reached only when the main loop
exits after a shutdown request.
Signed-off-by: Clément Chigot
---
hw/char/riscv_htif.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
index 37d3ccc76b..7e9b6fcc98 100644
--- a/hw/char/riscv_htif.c
+++ b/hw/char/riscv_htif.c
@@ -31,6 +31,7 @@
#include "qemu/error-report.h"
#include "exec/address-spaces.h"
#include "sysemu/dma.h"
+#include "sysemu/runstate.h"
#define RISCV_DEBUG_HTIF 0
#define HTIF_DEBUG(fmt, ...)
\
@@ -205,7 +206,9 @@ static void htif_handle_tohost_write(HTIFState *s, uint64_t
val_written)
g_free(sig_data);
}
-exit(exit_code);
+qemu_system_shutdown_request_with_code(
+SHUTDOWN_CAUSE_GUEST_SHUTDOWN, exit_code);
+return;
} else {
uint64_t syscall[8];
cpu_physical_memory_read(payload, syscall, sizeof(syscall));
--
2.25.1
[PATCH v3 3/5] hw/misc/sifive_test.c: replace exit calls with proper shutdown
This replaces the exit calls by shutdown requests, ensuring a proper
cleanup of Qemu. Otherwise, some connections like gdb could be broken
before its final packet ("Wxx") is being sent. This part, being done
inside qemu_cleanup function, can be reached only when the main loop
exits after a shutdown request.
Signed-off-by: Clément Chigot
---
hw/misc/sifive_test.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/hw/misc/sifive_test.c b/hw/misc/sifive_test.c
index 56df45bfe5..ad688079c4 100644
--- a/hw/misc/sifive_test.c
+++ b/hw/misc/sifive_test.c
@@ -25,6 +25,7 @@
#include "qemu/module.h"
#include "sysemu/runstate.h"
#include "hw/misc/sifive_test.h"
+#include "sysemu/sysemu.h"
static uint64_t sifive_test_read(void *opaque, hwaddr addr, unsigned int size)
{
@@ -39,9 +40,13 @@ static void sifive_test_write(void *opaque, hwaddr addr,
int code = (val64 >> 16) & 0x;
switch (status) {
case FINISHER_FAIL:
-exit(code);
+qemu_system_shutdown_request_with_code(
+SHUTDOWN_CAUSE_GUEST_PANIC, code);
+return;
case FINISHER_PASS:
-exit(0);
+qemu_system_shutdown_request_with_code(
+SHUTDOWN_CAUSE_GUEST_SHUTDOWN, code);
+return;
case FINISHER_RESET:
qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
return;
--
2.25.1
[PATCH v3 1/5] softmmu: add means to pass an exit code when requesting a shutdown
As of now, the exit code was either EXIT_FAILURE when a panic shutdown
was requested or EXIT_SUCCESS otherwise.
However, some hardware could want to pass more complex exit codes. Thus,
introduce a new shutdown request function allowing that.
Signed-off-by: Clément Chigot
---
include/sysemu/runstate.h | 2 ++
softmmu/runstate.c| 12 +++-
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
index 7beb29c2e2..1e59e0b12b 100644
--- a/include/sysemu/runstate.h
+++ b/include/sysemu/runstate.h
@@ -61,6 +61,8 @@ void qemu_system_wakeup_request(WakeupReason reason, Error
**errp);
void qemu_system_wakeup_enable(WakeupReason reason, bool enabled);
void qemu_register_wakeup_notifier(Notifier *notifier);
void qemu_register_wakeup_support(void);
+void qemu_system_shutdown_request_with_code(ShutdownCause reason,
+int exit_code);
void qemu_system_shutdown_request(ShutdownCause reason);
void qemu_system_powerdown_request(void);
void qemu_register_powerdown_notifier(Notifier *notifier);
diff --git a/softmmu/runstate.c b/softmmu/runstate.c
index f3bd862818..ee27e26048 100644
--- a/softmmu/runstate.c
+++ b/softmmu/runstate.c
@@ -345,6 +345,7 @@ void vm_state_notify(bool running, RunState state)
static ShutdownCause reset_requested;
static ShutdownCause shutdown_requested;
+static int shutdown_exit_code = EXIT_SUCCESS;
static int shutdown_signal;
static pid_t shutdown_pid;
static int powerdown_requested;
@@ -624,6 +625,13 @@ void qemu_system_killed(int signal, pid_t pid)
qemu_notify_event();
}
+void qemu_system_shutdown_request_with_code(ShutdownCause reason,
+int exit_code)
+{
+shutdown_exit_code = exit_code;
+qemu_system_shutdown_request(reason);
+}
+
void qemu_system_shutdown_request(ShutdownCause reason)
{
trace_qemu_system_shutdown_request(reason);
@@ -685,7 +693,9 @@ static bool main_loop_should_exit(int *status)
if (shutdown_action == SHUTDOWN_ACTION_PAUSE) {
vm_stop(RUN_STATE_SHUTDOWN);
} else {
-if (request == SHUTDOWN_CAUSE_GUEST_PANIC &&
+if (shutdown_exit_code != EXIT_SUCCESS) {
+*status = shutdown_exit_code;
+} else if (request == SHUTDOWN_CAUSE_GUEST_PANIC &&
panic_action == PANIC_ACTION_EXIT_FAILURE) {
*status = EXIT_FAILURE;
}
--
2.25.1
[PATCH v3 2/5] softmmu: pass the main loop status to gdb "Wxx" packet
gdb_exit function aims to close gdb sessions and sends the exit code of
the current execution. It's being called by qemu_cleanup once the main
loop is over.
Until now, the exit code sent was always 0. Now that hardware can
shutdown this main loop with custom exit codes, these codes must be
transfered to gdb as well.
Signed-off-by: Clément Chigot
---
include/sysemu/sysemu.h | 2 +-
softmmu/main.c | 2 +-
softmmu/runstate.c | 4 ++--
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 25be2a692e..73a37949c2 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -101,7 +101,7 @@ bool defaults_enabled(void);
void qemu_init(int argc, char **argv);
int qemu_main_loop(void);
-void qemu_cleanup(void);
+void qemu_cleanup(int);
extern QemuOptsList qemu_legacy_drive_opts;
extern QemuOptsList qemu_common_drive_opts;
diff --git a/softmmu/main.c b/softmmu/main.c
index 694388bd7f..9b91d21ea8 100644
--- a/softmmu/main.c
+++ b/softmmu/main.c
@@ -35,7 +35,7 @@ int qemu_default_main(void)
int status;
status = qemu_main_loop();
-qemu_cleanup();
+qemu_cleanup(status);
return status;
}
diff --git a/softmmu/runstate.c b/softmmu/runstate.c
index ee27e26048..d4e2e59e45 100644
--- a/softmmu/runstate.c
+++ b/softmmu/runstate.c
@@ -794,9 +794,9 @@ void qemu_init_subsystems(void)
}
-void qemu_cleanup(void)
+void qemu_cleanup(int status)
{
-gdb_exit(0);
+gdb_exit(status);
/*
* cleaning up the migration object cancels any existing migration
--
2.25.1
Re: [PATCH v2 3/3] gdbstub: replace exit(0) with proper shutdown
On Mon, Sep 4, 2023 at 11:42 AM Peter Maydell wrote:
>
> On Mon, 4 Sept 2023 at 10:36, Clément Chigot wrote:
> >
> > On Mon, Sep 4, 2023 at 11:23 AM Peter Maydell
> > wrote:
> > >
> > > On Wed, 23 Aug 2023 at 08:07, Clément Chigot wrote:
> > > >
> > > > This replaces the exit(0) call by a shutdown request, ensuring a proper
> > > > cleanup of Qemu. Otherwise, some connections could be broken without
> > > > being correctly flushed.
> > > >
> > > > Signed-off-by: Clément Chigot
>
> > > > +/*
> > > > + * Shutdown request is a clean way to stop the QEMU, compared
> > > > + * to a direct call to exit(). But we can't pass the exit code
> > > > + * through it so avoid doing that when it can matter.
> > > > + * As this function is also called during the cleanup process,
> > > > + * avoid sending the request if one is already set.
> > > > + */
> > > > +if (code) {
> > > > +exit(code);
> > > > +} else if (!qemu_shutdown_requested_get()) {
> > > > +qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> > > > +}
> > > > }
> > >
> > > This definitely doesn't look right. Either exit() is OK
> > > to call, or it is not. We shouldn't be exiting one way
> > > if the exit status is 0 and another way if it is non-0.
> >
> > I do agree but AFAIK, this isn't possible to pass the exit code using
> > qemu_system_shutdown_request.
>
> That would mean that we should add a mechanism to do so.
>
> But my opinion is still what I said about the first version
> of this patchset: we should fix whatever the problem is
> that means that gdb_exit() is not correctly ensuring that
> gdb gets the packet response, not paper over it like this.
The main issue is that calling exit(0) bypasses the call of qemu_cleanup().
For the two other patches, the wrong behavior is obvious: qemu_cleanup
being not called so is gdb_exit and then the gdb packet is never even
created, let alone being sent. Replacing exit by a shutdown request
ensures that the softmmu main loop terminates and that
qemu_cleanup/gdb_exit is being called.
For this one, I have to verify a bit further. Honestly, I did include
it for the sake of coherence and because we used to need it. However,
I've realized that this was earlier to commit b9e10c6c (which adds
explicit calls to gdb_exit). It might not be mandatory after all, even
if I still think that this is an improvement as bypassing qemu_cleanup
could lead to many leaks if contributors expect it to be called once
the softmmu main loop has started.
Clément
Re: [PATCH v2 3/3] gdbstub: replace exit(0) with proper shutdown
On Mon, Sep 4, 2023 at 11:23 AM Peter Maydell wrote:
>
> On Wed, 23 Aug 2023 at 08:07, Clément Chigot wrote:
> >
> > This replaces the exit(0) call by a shutdown request, ensuring a proper
> > cleanup of Qemu. Otherwise, some connections could be broken without
> > being correctly flushed.
> >
> > Signed-off-by: Clément Chigot
> > ---
> > gdbstub/gdbstub.c | 3 +--
> > gdbstub/softmmu.c | 13 +
> > gdbstub/user.c| 2 ++
> > 3 files changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> > index 5f28d5cf57..358eed1935 100644
> > --- a/gdbstub/gdbstub.c
> > +++ b/gdbstub/gdbstub.c
> > @@ -1298,7 +1298,6 @@ static void handle_v_kill(GArray *params, void
> > *user_ctx)
> > gdb_put_packet("OK");
> > error_report("QEMU: Terminated via GDBstub");
> > gdb_exit(0);
> > -exit(0);
> > }
> >
> > static const GdbCmdParseEntry gdb_v_commands_table[] = {
> > @@ -1818,7 +1817,7 @@ static int gdb_handle_packet(const char *line_buf)
> > /* Kill the target */
> > error_report("QEMU: Terminated via GDBstub");
> > gdb_exit(0);
> > -exit(0);
> > +break;
> > case 'D':
> > {
> > static const GdbCmdParseEntry detach_cmd_desc = {
> > diff --git a/gdbstub/softmmu.c b/gdbstub/softmmu.c
> > index f509b7285d..fa9b09537d 100644
> > --- a/gdbstub/softmmu.c
> > +++ b/gdbstub/softmmu.c
> > @@ -434,6 +434,19 @@ void gdb_exit(int code)
> > }
> >
> > qemu_chr_fe_deinit(&gdbserver_system_state.chr, true);
> > +
> > +/*
> > + * Shutdown request is a clean way to stop the QEMU, compared
> > + * to a direct call to exit(). But we can't pass the exit code
> > + * through it so avoid doing that when it can matter.
> > + * As this function is also called during the cleanup process,
> > + * avoid sending the request if one is already set.
> > + */
> > +if (code) {
> > +exit(code);
> > +} else if (!qemu_shutdown_requested_get()) {
> > +qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> > +}
> > }
>
> This definitely doesn't look right. Either exit() is OK
> to call, or it is not. We shouldn't be exiting one way
> if the exit status is 0 and another way if it is non-0.
I do agree but AFAIK, this isn't possible to pass the exit code using
qemu_system_shutdown_request.
Would it make more sense to trigger a SHUTDOWN_CAUSE_GUEST_PANIC
instead ? This would result in a non-0 exit code and the already
available gdb_trace would show the real exit code if needed.
Clément
[PATCH v2 3/3] gdbstub: replace exit(0) with proper shutdown
This replaces the exit(0) call by a shutdown request, ensuring a proper
cleanup of Qemu. Otherwise, some connections could be broken without
being correctly flushed.
Signed-off-by: Clément Chigot
---
gdbstub/gdbstub.c | 3 +--
gdbstub/softmmu.c | 13 +
gdbstub/user.c| 2 ++
3 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 5f28d5cf57..358eed1935 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -1298,7 +1298,6 @@ static void handle_v_kill(GArray *params, void *user_ctx)
gdb_put_packet("OK");
error_report("QEMU: Terminated via GDBstub");
gdb_exit(0);
-exit(0);
}
static const GdbCmdParseEntry gdb_v_commands_table[] = {
@@ -1818,7 +1817,7 @@ static int gdb_handle_packet(const char *line_buf)
/* Kill the target */
error_report("QEMU: Terminated via GDBstub");
gdb_exit(0);
-exit(0);
+break;
case 'D':
{
static const GdbCmdParseEntry detach_cmd_desc = {
diff --git a/gdbstub/softmmu.c b/gdbstub/softmmu.c
index f509b7285d..fa9b09537d 100644
--- a/gdbstub/softmmu.c
+++ b/gdbstub/softmmu.c
@@ -434,6 +434,19 @@ void gdb_exit(int code)
}
qemu_chr_fe_deinit(&gdbserver_system_state.chr, true);
+
+/*
+ * Shutdown request is a clean way to stop the QEMU, compared
+ * to a direct call to exit(). But we can't pass the exit code
+ * through it so avoid doing that when it can matter.
+ * As this function is also called during the cleanup process,
+ * avoid sending the request if one is already set.
+ */
+if (code) {
+exit(code);
+} else if (!qemu_shutdown_requested_get()) {
+qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
+}
}
/*
diff --git a/gdbstub/user.c b/gdbstub/user.c
index 5b375be1d9..f3d97d621f 100644
--- a/gdbstub/user.c
+++ b/gdbstub/user.c
@@ -113,6 +113,8 @@ void gdb_exit(int code)
gdb_put_packet(buf);
gdbserver_state.allow_stop_reply = false;
}
+
+exit(code);
}
int gdb_handlesig(CPUState *cpu, int sig)
--
2.25.1
[PATCH v2 1/3] hw/misc/sifive_test.c: replace exit(0) with proper shutdown
This replaces the exit(0) call by a shutdown request, ensuring a proper
cleanup of Qemu. Otherwise, some connections like gdb could be broken
without being correctly flushed.
Signed-off-by: Clément Chigot
Reviewed-by: Alistair Francis
---
hw/misc/sifive_test.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/misc/sifive_test.c b/hw/misc/sifive_test.c
index 56df45bfe5..ab0674f8fe 100644
--- a/hw/misc/sifive_test.c
+++ b/hw/misc/sifive_test.c
@@ -25,6 +25,7 @@
#include "qemu/module.h"
#include "sysemu/runstate.h"
#include "hw/misc/sifive_test.h"
+#include "sysemu/sysemu.h"
static uint64_t sifive_test_read(void *opaque, hwaddr addr, unsigned int size)
{
@@ -41,7 +42,8 @@ static void sifive_test_write(void *opaque, hwaddr addr,
case FINISHER_FAIL:
exit(code);
case FINISHER_PASS:
-exit(0);
+qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
+return;
case FINISHER_RESET:
qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
return;
--
2.25.1
[PATCH v2 2/3] hw/char: riscv_htif: replace exit(0) with proper shutdown
This replaces the exit(0) call by a shutdown request, ensuring a proper
cleanup of Qemu. Otherwise, some connections like gdb could be broken
without being correctly flushed.
Signed-off-by: Clément Chigot
Reviewed-by: Alistair Francis
---
hw/char/riscv_htif.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
index 37d3ccc76b..c49d20a221 100644
--- a/hw/char/riscv_htif.c
+++ b/hw/char/riscv_htif.c
@@ -31,6 +31,7 @@
#include "qemu/error-report.h"
#include "exec/address-spaces.h"
#include "sysemu/dma.h"
+#include "sysemu/runstate.h"
#define RISCV_DEBUG_HTIF 0
#define HTIF_DEBUG(fmt, ...)
\
@@ -205,7 +206,16 @@ static void htif_handle_tohost_write(HTIFState *s,
uint64_t val_written)
g_free(sig_data);
}
-exit(exit_code);
+/*
+ * Shutdown request is a clean way to stop the QEMU, compared
+ * to a direct call to exit(). But we can't pass the exit code
+ * through it so avoid doing that when it can matter.
+ */
+if (exit_code) {
+exit(exit_code);
+} else {
+
qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
+}
} else {
uint64_t syscall[8];
cpu_physical_memory_read(payload, syscall, sizeof(syscall));
--
2.25.1
[PATCH v2 0/3] Risc-V/gdb: replace exit(0) with proper shutdown
This serie replaces some of the call to exit(0) in hardware used by Risc-V boards or made when gdb is requested to exit. Otherwise, the gdb connection can be too abruptly disconnected resulting in the last gdb packet "Wxx" being not sent. As qemu_system_shutdown_request doesn't allow to pass the exit code, only perform the above modification on a sucessful exit. Difference with v1: - avoid sending a shutdown request in gdb_exit if one has already been set. Clément Chigot (3): hw/misc/sifive_test.c: replace exit(0) with proper shutdown hw/char: riscv_htif: replace exit(0) with proper shutdown gdbstub: replace exit(0) with proper shutdown gdbstub/gdbstub.c | 3 +-- gdbstub/softmmu.c | 13 + gdbstub/user.c| 2 ++ hw/char/riscv_htif.c | 12 +++- hw/misc/sifive_test.c | 4 +++- 5 files changed, 30 insertions(+), 4 deletions(-) -- 2.25.1
Re: [PATCH 3/3] gdbstub: replace exit(0) with proper shutdown
On Fri, Aug 18, 2023 at 11:10 AM Peter Maydell wrote:
>
> On Fri, 18 Aug 2023 at 10:03, Clément Chigot wrote:
> >
> > This replaces the exit(0) call by a shutdown request, ensuring a proper
> > cleanup of Qemu. Otherwise, some connections could be broken without
> > being correctly flushed.
> >
> > Signed-off-by: Clément Chigot
> > ---
> > gdbstub/gdbstub.c | 3 +--
> > gdbstub/softmmu.c | 11 +++
> > gdbstub/user.c| 2 ++
> > 3 files changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> > index 5f28d5cf57..358eed1935 100644
> > --- a/gdbstub/gdbstub.c
> > +++ b/gdbstub/gdbstub.c
> > @@ -1298,7 +1298,6 @@ static void handle_v_kill(GArray *params, void
> > *user_ctx)
> > gdb_put_packet("OK");
> > error_report("QEMU: Terminated via GDBstub");
> > gdb_exit(0);
> > -exit(0);
> > }
> >
> > static const GdbCmdParseEntry gdb_v_commands_table[] = {
> > @@ -1818,7 +1817,7 @@ static int gdb_handle_packet(const char *line_buf)
> > /* Kill the target */
> > error_report("QEMU: Terminated via GDBstub");
> > gdb_exit(0);
> > -exit(0);
> > +break;
> > case 'D':
> > {
> > static const GdbCmdParseEntry detach_cmd_desc = {
> > diff --git a/gdbstub/softmmu.c b/gdbstub/softmmu.c
> > index f509b7285d..9ca7ae10bc 100644
> > --- a/gdbstub/softmmu.c
> > +++ b/gdbstub/softmmu.c
> > @@ -434,6 +434,17 @@ void gdb_exit(int code)
> > }
> >
> > qemu_chr_fe_deinit(&gdbserver_system_state.chr, true);
> > +
> > +/*
> > + * Shutdown request is a clean way to stop the QEMU, compared
> > + * to a direct call to exit(). But we can't pass the exit code
> > + * through it so avoid doing that when it can matter.
> > + */
> > +if (code) {
> > +exit(code);
> > +} else {
> > +qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> > +}
> > }
> >
> > /*
> > diff --git a/gdbstub/user.c b/gdbstub/user.c
> > index 5b375be1d9..f3d97d621f 100644
> > --- a/gdbstub/user.c
> > +++ b/gdbstub/user.c
> > @@ -113,6 +113,8 @@ void gdb_exit(int code)
> > gdb_put_packet(buf);
> > gdbserver_state.allow_stop_reply = false;
> > }
> > +
> > +exit(code);
> > }
>
> These are not the only places that call gdb_exit().
> Notably, qemu_cleanup() calls it, and I'm pretty sure
> it does not expect that gdb_exit() will either call
> exit() or qemu_system_shutdown_request(), because it's
> already in the process of cleaning up and stopping
> the system.
Indeed, I did miss that. I used to have it directly in
gdb_handle_packet and in handle_v_kill. But now that the support of
softmmu and user has been splitted, I thought putting it in gdb_exit
was a solution.
However, IIUC the code, the second request will simply be ignored, the
main loop (where the requests matter) have been already exited.
I see what I can do anyway to avoid this double request.
> If we send the "we're exiting" report to the gdb process,
> that ought to be sufficient. If we're not actually managing
> to send that last packet to gdb because we don't flush
> the data out to the file descriptor, then it seems to me
> that the fix ought to be to ensure we do do that flush
> as part of gdb_exit() not to rearrange or try to avoid
> all the subsequent calls to exit().
Here, I'm seeing the symptoms with a gdb connection being closed too
sharply and a fd not being flush. But looking at the qemu_cleanup(),
many things could require a proper cleanup which will be skipped by a
simple exit(0). This could lead to many unexpected side effects.
Thanks,
Clément
[PATCH 3/3] gdbstub: replace exit(0) with proper shutdown
This replaces the exit(0) call by a shutdown request, ensuring a proper
cleanup of Qemu. Otherwise, some connections could be broken without
being correctly flushed.
Signed-off-by: Clément Chigot
---
gdbstub/gdbstub.c | 3 +--
gdbstub/softmmu.c | 11 +++
gdbstub/user.c| 2 ++
3 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 5f28d5cf57..358eed1935 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -1298,7 +1298,6 @@ static void handle_v_kill(GArray *params, void *user_ctx)
gdb_put_packet("OK");
error_report("QEMU: Terminated via GDBstub");
gdb_exit(0);
-exit(0);
}
static const GdbCmdParseEntry gdb_v_commands_table[] = {
@@ -1818,7 +1817,7 @@ static int gdb_handle_packet(const char *line_buf)
/* Kill the target */
error_report("QEMU: Terminated via GDBstub");
gdb_exit(0);
-exit(0);
+break;
case 'D':
{
static const GdbCmdParseEntry detach_cmd_desc = {
diff --git a/gdbstub/softmmu.c b/gdbstub/softmmu.c
index f509b7285d..9ca7ae10bc 100644
--- a/gdbstub/softmmu.c
+++ b/gdbstub/softmmu.c
@@ -434,6 +434,17 @@ void gdb_exit(int code)
}
qemu_chr_fe_deinit(&gdbserver_system_state.chr, true);
+
+/*
+ * Shutdown request is a clean way to stop the QEMU, compared
+ * to a direct call to exit(). But we can't pass the exit code
+ * through it so avoid doing that when it can matter.
+ */
+if (code) {
+exit(code);
+} else {
+qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
+}
}
/*
diff --git a/gdbstub/user.c b/gdbstub/user.c
index 5b375be1d9..f3d97d621f 100644
--- a/gdbstub/user.c
+++ b/gdbstub/user.c
@@ -113,6 +113,8 @@ void gdb_exit(int code)
gdb_put_packet(buf);
gdbserver_state.allow_stop_reply = false;
}
+
+exit(code);
}
int gdb_handlesig(CPUState *cpu, int sig)
--
2.25.1
[PATCH 2/3] hw/char: riscv_htif: replace exit(0) with proper shutdown
This replaces the exit(0) call by a shutdown request, ensuring a proper
cleanup of Qemu. Otherwise, some connections like gdb could be broken
without being correctly flushed.
Signed-off-by: Clément Chigot
---
hw/char/riscv_htif.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
index 37d3ccc76b..c49d20a221 100644
--- a/hw/char/riscv_htif.c
+++ b/hw/char/riscv_htif.c
@@ -31,6 +31,7 @@
#include "qemu/error-report.h"
#include "exec/address-spaces.h"
#include "sysemu/dma.h"
+#include "sysemu/runstate.h"
#define RISCV_DEBUG_HTIF 0
#define HTIF_DEBUG(fmt, ...)
\
@@ -205,7 +206,16 @@ static void htif_handle_tohost_write(HTIFState *s,
uint64_t val_written)
g_free(sig_data);
}
-exit(exit_code);
+/*
+ * Shutdown request is a clean way to stop the QEMU, compared
+ * to a direct call to exit(). But we can't pass the exit code
+ * through it so avoid doing that when it can matter.
+ */
+if (exit_code) {
+exit(exit_code);
+} else {
+
qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
+}
} else {
uint64_t syscall[8];
cpu_physical_memory_read(payload, syscall, sizeof(syscall));
--
2.25.1
[PATCH 1/3] hw/misc/sifive_test.c: replace exit(0) with proper shutdown
This replaces the exit(0) call by a shutdown request, ensuring a proper
cleanup of Qemu. Otherwise, some connections like gdb could be broken
without being correctly flushed.
Signed-off-by: Clément Chigot
---
hw/misc/sifive_test.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/misc/sifive_test.c b/hw/misc/sifive_test.c
index 56df45bfe5..ab0674f8fe 100644
--- a/hw/misc/sifive_test.c
+++ b/hw/misc/sifive_test.c
@@ -25,6 +25,7 @@
#include "qemu/module.h"
#include "sysemu/runstate.h"
#include "hw/misc/sifive_test.h"
+#include "sysemu/sysemu.h"
static uint64_t sifive_test_read(void *opaque, hwaddr addr, unsigned int size)
{
@@ -41,7 +42,8 @@ static void sifive_test_write(void *opaque, hwaddr addr,
case FINISHER_FAIL:
exit(code);
case FINISHER_PASS:
-exit(0);
+qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
+return;
case FINISHER_RESET:
qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
return;
--
2.25.1
[PATCH 0/3] Risc-V/gdb: replace exit(0) with proper shutdown
This serie replaces some of the call to exit(0) in hardware used by Risc-V boards or made when gdb is requested to exit. Otherwise, the gdb connection can be too abruptly disconnected resulting in the last gdb packet "Wxx" being not sent. As qemu_system_shutdown_request doesn't allow to pass the exit code, only perform the above modification on a sucessful exit. Clément Chigot (3): hw/misc/sifive_test.c: replace exit(0) with proper shutdown hw/char: riscv_htif: replace exit(0) with proper shutdown gdbstub: replace exit(0) with proper shutdown gdbstub/gdbstub.c | 3 +-- gdbstub/softmmu.c | 11 +++ gdbstub/user.c| 2 ++ hw/char/riscv_htif.c | 12 +++- hw/misc/sifive_test.c | 4 +++- 5 files changed, 28 insertions(+), 4 deletions(-) -- 2.25.1
Re: [PATCH] hw/arm/xlnx-zynqmp: fix unsigned error when checking the RPUs number
On Wed, May 24, 2023 at 5:06 PM Philippe Mathieu-Daudé
wrote:
>
> Hi Clément,
>
> On 24/5/23 16:37, Clément Chigot wrote:
> > When passing --smp with a number lower than XLNX_ZYNQMP_NUM_APU_CPUS,
> > the expression (ms->smp.cpus - XLNX_ZYNQMP_NUM_APU_CPUS) will result
> > in a positive number as ms->smp.cpus is a unsigned int.
> > This will raise the following error afterwards, as Qemu will try to
> > instantiate some additional RPUs.
> >| $ qemu-system-aarch64 --smp 1 -M xlnx-zcu102
> >| **
> >| ERROR:../src/tcg/tcg.c:777:tcg_register_thread:
> > | assertion failed: (n < tcg_max_ctxs)
> >
> > Signed-off-by: Clément Chigot
> > ---
> > hw/arm/xlnx-zynqmp.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> > index 335cfc417d..5905a33015 100644
> > --- a/hw/arm/xlnx-zynqmp.c
> > +++ b/hw/arm/xlnx-zynqmp.c
> > @@ -213,7 +213,7 @@ static void xlnx_zynqmp_create_rpu(MachineState *ms,
> > XlnxZynqMPState *s,
> > const char *boot_cpu, Error **errp)
> > {
> > int i;
> > -int num_rpus = MIN(ms->smp.cpus - XLNX_ZYNQMP_NUM_APU_CPUS,
> > +int num_rpus = MIN((int)(ms->smp.cpus - XLNX_ZYNQMP_NUM_APU_CPUS),
> > XLNX_ZYNQMP_NUM_RPU_CPUS);
> >
> > if (num_rpus <= 0) {
>
> Can we set mc->min_cpus in xlnx_zcu102_machine_class_init() instead?
>
> -- >8 --
> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
> index 4c84bb932a..60a2710e21 100644
> --- a/hw/arm/xlnx-zcu102.c
> +++ b/hw/arm/xlnx-zcu102.c
> @@ -269,6 +269,7 @@ static void
> xlnx_zcu102_machine_class_init(ObjectClass *oc, void *data)
> mc->block_default_type = IF_IDE;
> mc->units_per_default_bus = 1;
> mc->ignore_memory_transaction_failures = true;
> +mc->min_cpus = XLNX_ZYNQMP_NUM_APU_CPUS;
> mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS + XLNX_ZYNQMP_NUM_RPU_CPUS;
> mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS;
> mc->default_ram_id = "ddr-ram";
> ---
>
> $ qemu-system-aarch64 -M xlnx-zcu102 -smp 1
> qemu-system-aarch64: Invalid SMP CPUs 1. The min CPUs supported by
> machine 'xlnx-zcu102' is 4
We encountered this issue when we were trying to emulate a zynqmp with
only two cortex-a53 cores. I don't have the full context but this was
probably to debug a race condition or something like that.
Thus, I would like to keep the possibility of lowering the CPU number,
even if it doesn't match the real board.
Clément
[PATCH] hw/arm/xlnx-zynqmp: fix unsigned error when checking the RPUs number
When passing --smp with a number lower than XLNX_ZYNQMP_NUM_APU_CPUS,
the expression (ms->smp.cpus - XLNX_ZYNQMP_NUM_APU_CPUS) will result
in a positive number as ms->smp.cpus is a unsigned int.
This will raise the following error afterwards, as Qemu will try to
instantiate some additional RPUs.
| $ qemu-system-aarch64 --smp 1 -M xlnx-zcu102
| **
| ERROR:../src/tcg/tcg.c:777:tcg_register_thread:
| assertion failed: (n < tcg_max_ctxs)
Signed-off-by: Clément Chigot
---
hw/arm/xlnx-zynqmp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 335cfc417d..5905a33015 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -213,7 +213,7 @@ static void xlnx_zynqmp_create_rpu(MachineState *ms,
XlnxZynqMPState *s,
const char *boot_cpu, Error **errp)
{
int i;
-int num_rpus = MIN(ms->smp.cpus - XLNX_ZYNQMP_NUM_APU_CPUS,
+int num_rpus = MIN((int)(ms->smp.cpus - XLNX_ZYNQMP_NUM_APU_CPUS),
XLNX_ZYNQMP_NUM_RPU_CPUS);
if (num_rpus <= 0) {
--
2.25.1
Re: [PATCH v3 2/2] hw/intc: sifive_plic: change interrupt priority register to WARL field
On Mon, Oct 3, 2022 at 6:14 AM Jim Shu wrote:
>
> PLIC spec [1] requires interrupt source priority registers are WARL
> field and the number of supported priority is power-of-2 to simplify SW
> discovery.
>
> Existing QEMU RISC-V machine (e.g. shakti_c) don't strictly follow PLIC
> spec, whose number of supported priority is not power-of-2. Just change
> each bit of interrupt priority register to WARL field when the number of
> supported priority is power-of-2.
>
> [1]
> https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc#interrupt-priorities
>
> Signed-off-by: Jim Shu
> ---
> hw/intc/sifive_plic.c | 21 +++--
> 1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
> index f864efa761..c2dfacf028 100644
> --- a/hw/intc/sifive_plic.c
> +++ b/hw/intc/sifive_plic.c
> @@ -180,7 +180,15 @@ static void sifive_plic_write(void *opaque, hwaddr addr,
> uint64_t value,
> if (addr_between(addr, plic->priority_base, plic->num_sources << 2)) {
> uint32_t irq = ((addr - plic->priority_base) >> 2) + 1;
>
> -if (value <= plic->num_priorities) {
> +if (((plic->num_priorities + 1) & plic->num_priorities) == 0) {
> +/*
> + * if "num_priorities + 1" is power-of-2, make each register bit
> of
> + * interrupt priority WARL (Write-Any-Read-Legal). Just filter
> + * out the access to unsupported priority bits.
> + */
> +plic->source_priority[irq] = value % (plic->num_priorities + 1);
> +sifive_plic_update(plic);
> +} else if (value <= plic->num_priorities) {
> plic->source_priority[irq] = value;
> sifive_plic_update(plic);
> }
> @@ -207,7 +215,16 @@ static void sifive_plic_write(void *opaque, hwaddr addr,
> uint64_t value,
> uint32_t contextid = (addr & (plic->context_stride - 1));
>
> if (contextid == 0) {
> -if (value <= plic->num_priorities) {
> +if (((plic->num_priorities + 1) & plic->num_priorities) == 0) {
> +/*
> + * if "num_priorities + 1" is power-of-2, each register bit
> of
> + * interrupt priority is WARL (Write-Any-Read-Legal). Just
> + * filter out the access to unsupported priority bits.
> + */
> +plic->target_priority[addrid] = value %
> +(plic->num_priorities + 1);
> +sifive_plic_update(plic);
> +} else if (value <= plic->num_priorities) {
> plic->target_priority[addrid] = value;
> sifive_plic_update(plic);
> }
> --
> 2.17.1
Reviewed-by: Clément Chigot
Re: [PATCH v2 2/2] hw/intc: sifive_plic: change interrupt priority register to WARL field
Hi Jim,
On Fri, Sep 30, 2022 at 2:32 PM Jim Shu wrote:
>
> PLIC spec [1] requires interrupt source priority registers are WARL
> field and the number of supported priority is power-of-2 to simplify SW
> discovery.
>
> Existing QEMU RISC-V machine (e.g. shakti_c) don't strictly follow PLIC
> spec, whose number of supported priority is not power-of-2. Just change
> each bit of interrupt priority register to WARL field when the number of
> supported priority is power-of-2.
>
> [1]
> https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc#interrupt-priorities
>
> Signed-off-by: Jim Shu
> ---
> hw/intc/sifive_plic.c | 21 +++--
> 1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
> index f864efa761..218ccff8bd 100644
> --- a/hw/intc/sifive_plic.c
> +++ b/hw/intc/sifive_plic.c
> @@ -180,7 +180,15 @@ static void sifive_plic_write(void *opaque, hwaddr addr,
> uint64_t value,
> if (addr_between(addr, plic->priority_base, plic->num_sources << 2)) {
> uint32_t irq = ((addr - plic->priority_base) >> 2) + 1;
>
> -if (value <= plic->num_priorities) {
> +if ((plic->num_priorities + 1) & (plic->num_priorities)) {
That's the opposite. If n is a power of 2, n & (n-1) == 0 (eg 8 & 7 ==
0, 9 & 8 == 8).
Note that n must be positive too. But I'm not sure it matters here.
I'll let you decide.
> +/*
> + * if "num_priorities + 1" is power-of-2, make each register bit
> of
> + * interrupt priority WARL (Write-Any-Read-Legal). Just filter
> + * out the access to unsupported priority bits.
> + */
> +plic->source_priority[irq] = value % (plic->num_priorities + 1);
> +sifive_plic_update(plic);
> +} else if (value <= plic->num_priorities) {
> plic->source_priority[irq] = value;
> sifive_plic_update(plic);
> }
> @@ -207,7 +215,16 @@ static void sifive_plic_write(void *opaque, hwaddr addr,
> uint64_t value,
> uint32_t contextid = (addr & (plic->context_stride - 1));
>
> if (contextid == 0) {
> -if (value <= plic->num_priorities) {
> +if ((plic->num_priorities + 1) & (plic->num_priorities)) {
Same.
> +/*
> + * if "num_priorities + 1" is power-of-2, each register bit
> of
> + * interrupt priority is WARL (Write-Any-Read-Legal). Just
> + * filter out the access to unsupported priority bits.
> + */
> +plic->target_priority[addrid] = value %
> +(plic->num_priorities + 1);
> +sifive_plic_update(plic);
> +} else if (value <= plic->num_priorities) {
> plic->target_priority[addrid] = value;
> sifive_plic_update(plic);
> }
> --
> 2.17.1
Clément
Re: [PATCH] hw/intc: sifive_plic: fix hard-coded max priority level
Hi Jim,
On Sun, Sep 25, 2022 at 3:26 PM Jim Shu wrote:
>
> The maximum priority level is hard-coded when writing to interrupt
> priority register. However, when writing to priority threshold register,
> the maximum priority level is from num_priorities Property which is
> configured by platform.
>
> Also change interrupt priority register to use num_priorities Property
> in maximum priority level.
>
> Signed-off-by: Emmanuel Blot
> Signed-off-by: Jim Shu
> Reviewed-by: Frank Chang
> ---
> hw/intc/sifive_plic.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
> index af4ae3630e..f864efa761 100644
> --- a/hw/intc/sifive_plic.c
> +++ b/hw/intc/sifive_plic.c
> @@ -180,8 +180,10 @@ static void sifive_plic_write(void *opaque, hwaddr addr,
> uint64_t value,
> if (addr_between(addr, plic->priority_base, plic->num_sources << 2)) {
> uint32_t irq = ((addr - plic->priority_base) >> 2) + 1;
>
> -plic->source_priority[irq] = value & 7;
> -sifive_plic_update(plic);
> +if (value <= plic->num_priorities) {
> +plic->source_priority[irq] = value;
> +sifive_plic_update(plic);
> +}
If I'm not mistaking the documentation [1], these registers are WARL
(Write-Any-Read-Legal). However, in your case, any value above
"num_priorities" will be ignored.
We had an issue related to that and ended up making a local patch.
However, we are assuming that "num_priorities+1" is a power of 2
(which is currently the case)
-plic->source_priority[irq] = value & 7;
+/* Interrupt Priority registers are Write-Any Read-Legal. Cleanup
+ * incoming values before storing them.
+ */
+plic->source_priority[irq] = value % (plic->num_priorities + 1);
Note that it should also be done for target_priority a bit below.
-if (value <= plic->num_priorities) {
-plic->target_priority[addrid] = value;
-sifive_plic_update(plic);
-}
+/* Priority Thresholds registers are Write-Any Read-Legal. Cleanup
+ * incoming values before storing them.
+ */
+plic->target_priority[addrid] = value % (plic->num_priorities + 1);
+sifive_plic_update(plic);
[1] https://static.dev.sifive.com/FE310-G000.pdf
Thanks,
Clément
[PATCH] target/arm: Fix alignment for VLD4.32
When requested, the alignment for VLD4.32 is 8 and not 16.
See ARM documentation about VLD4 encoding:
ebytes = 1 << UInt(size);
if size == '10' then
alignment = if a == '0' then 1 else 8;
else
alignment = if a == '0' then 1 else 4*ebytes;
Signed-off-by: Clément Chigot
---
target/arm/translate-neon.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/target/arm/translate-neon.c b/target/arm/translate-neon.c
index 321c17e2c7..4016339d46 100644
--- a/target/arm/translate-neon.c
+++ b/target/arm/translate-neon.c
@@ -584,7 +584,11 @@ static bool trans_VLD_all_lanes(DisasContext *s,
arg_VLD_all_lanes *a)
case 3:
return false;
case 4:
-align = pow2_align(size + 2);
+if (size == 2) {
+align = pow2_align(3);
+} else {
+align = pow2_align(size + 2);
+}
break;
default:
g_assert_not_reached();
--
2.25.1
Re: Question about loading bare metal firmware
> > Hi all, > > > > I'm wondering if there is an official way to load bare metal software > > within qemu emulations. > > I've seen a lot of people (including us) using -kernel. However, the > > doc seems to imply that the generic loader would be a better approach > > (cf [1]). I know that the compatibility with older Qemus is one of the > > reasons why -kernel is still highly used. I've also seen that the > > reset vector can be initialized automatically by -kernel unlike with > > the generic loader (this is the case with RiscV AFAICT). > > But is there any kind of official recommendation on that topic ? > > The recommendation is in the document you linked. For bare metal use the > generic loader and make sure you put the blob in the right place so the > architectural reset vector will jump to it. Alright. I should have missed something when I tried with the generic loader. Thanks for the inputs and the confirmation that we were doing something wrong ! Thanks, Clément
Question about loading bare metal firmware
Hi all, I'm wondering if there is an official way to load bare metal software within qemu emulations. I've seen a lot of people (including us) using -kernel. However, the doc seems to imply that the generic loader would be a better approach (cf [1]). I know that the compatibility with older Qemus is one of the reasons why -kernel is still highly used. I've also seen that the reset vector can be initialized automatically by -kernel unlike with the generic loader (this is the case with RiscV AFAICT). But is there any kind of official recommendation on that topic ? I'm asking that because a recent change in RiscV Polarfire Soc is forcing -dtb to be passed along -kernel. But in case of bare board software, -dtb isn't needed (at least in our use case). I've a patch that allows "-dtb" to be missing with "-kernel" only if "-bios none" is provided. But I'm not sure if this is the right way to say "it's a bare board software". @Bin Meng you're the one that added this -kernel support in PolarFire Soc. Thus, is my approach looking good for you or do you have a better one in mind ? [1] https://www.qemu.org/docs/master/system/qemu-manpage.html#hxtool-8 Thanks, Clément
