Re: [PATCH] hw/misc: Add an iBT device model

[Cédric Le Goater]

On 9/24/21 20:43, Titus Rwantare wrote:

On Fri, 24 Sept 2021 at 03:55, Cédric Le Goater  wrote:


Hello Titus,

On 9/24/21 10:42, Philippe Mathieu-Daudé wrote:

On 9/24/21 01:48, Titus Rwantare wrote:

Hello all,

I'd like some clarification on how the following code transfers irqs
back and forth:

b/hw/arm/aspeed_soc.c
+/* iBT */
+if (!sysbus_realize(SYS_BUS_DEVICE(>ibt), errp)) {
+return;
+}
+memory_region_add_subregion(>lpc.iomem,
+   sc->memmap[ASPEED_DEV_IBT] - sc->memmap[ASPEED_DEV_LPC],
+   >ibt.iomem);
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_ibt,



The iBT device IRQ is connected to a subdevice irq of the LPC device.
See aspeed_lpc_realize(). And triggered in aspeed_lpc_set_irq()


Yes, that side makes sense. I tried to get at that irq from
aspeed_ibt.c as follows:

qemu_irq_lower(ibt->lpc->subdevice_irqs[aspeed_lpc_ibt]); // or raise

static void aspeed_ibt_realize(DeviceState *dev, Error **errp)
{
AspeedIBTState *ibt = ASPEED_IBT(dev);
SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
IPMIInterface *ii = IPMI_INTERFACE(dev);
ibt->lpc = ASPEED_LPC(dev);
...

but this doesn't work and maybe I'm misusing the dynamic cast?


+   qdev_get_gpio_in(DEVICE(>lpc), aspeed_lpc_ibt));
}


and


hw/misc/aspeed_ibt.c
+static void aspeed_ibt_realize(DeviceState *dev, Error **errp)
+{
+SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+AspeedIBTState *ibt = ASPEED_IBT(dev);

...

+
+sysbus_init_irq(sbd, >irq);


I ask because the code in aspeed_soc.c seems to connect to the
lpc->subdevice_irqs[aspeed_lpc_ibt], initialised on
hw/misc/aspeed_lpc.c:408.
I noticed that bmc firmware running in qemu was checking the BT_CTRL
register less frequently than I'd like while editing this patch to use
the IPMIInterface.


OK.

This might be a problem in aspeed_ibt_update_irq(). This patch is
an experiment from some few years ago. It still works good enough
for the witherspoon-bmc and powernv9 machines for simple IPMI
commands: fru, sdr, lan, power off (to be checked).

Could you share your BMC and host command line ?



Host:
-chardev socket,id=ipmichr1,host=localhost,port=,reconnect=10 \
-device ipmi-bmc-extern,chardev=ipmichr1,id=bmc0 \
-device isa-ipmi-bt,bmc=bmc0,irq=10 -nodefaults

BMC:
-chardev socket,id=ipmichr1,host=localhost,port=,server=on,wait=off \
-device ipmi-host-extern,chardev=ipmichr1,responder=/machine/soc/ibt


This looks correct.


But for this to work you need Hao's patch as well: [PATCH 7/8]
hw/ipmi: Add an IPMI external host device.
 
Is it based on my aspeed branch which contains the iBT model ?

Could you send the patchset as an RFC maybe ?


Thanks,

C.

Re: [PATCH] hw/misc: Add an iBT device model

[Titus Rwantare]

On Fri, 24 Sept 2021 at 03:55, Cédric Le Goater  wrote:
>
> Hello Titus,
>
> On 9/24/21 10:42, Philippe Mathieu-Daudé wrote:
> > On 9/24/21 01:48, Titus Rwantare wrote:
> >> Hello all,
> >>
> >> I'd like some clarification on how the following code transfers irqs
> >> back and forth:
> >>> b/hw/arm/aspeed_soc.c
> >>> +/* iBT */
> >>> +if (!sysbus_realize(SYS_BUS_DEVICE(>ibt), errp)) {
> >>> +return;
> >>> +}
> >>> +memory_region_add_subregion(>lpc.iomem,
> >>> +   sc->memmap[ASPEED_DEV_IBT] - 
> >>> sc->memmap[ASPEED_DEV_LPC],
> >>> +   >ibt.iomem);
> >>> +sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_ibt,
>
>
> The iBT device IRQ is connected to a subdevice irq of the LPC device.
> See aspeed_lpc_realize(). And triggered in aspeed_lpc_set_irq()

Yes, that side makes sense. I tried to get at that irq from
aspeed_ibt.c as follows:

qemu_irq_lower(ibt->lpc->subdevice_irqs[aspeed_lpc_ibt]); // or raise

static void aspeed_ibt_realize(DeviceState *dev, Error **errp)
{
AspeedIBTState *ibt = ASPEED_IBT(dev);
SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
IPMIInterface *ii = IPMI_INTERFACE(dev);
ibt->lpc = ASPEED_LPC(dev);
...

but this doesn't work and maybe I'm misusing the dynamic cast?

> >>> +   qdev_get_gpio_in(DEVICE(>lpc), 
> >>> aspeed_lpc_ibt));
> >>> }
> >>
> >> and
> >>
> >>> hw/misc/aspeed_ibt.c
> >>> +static void aspeed_ibt_realize(DeviceState *dev, Error **errp)
> >>> +{
> >>> +SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> >>> +AspeedIBTState *ibt = ASPEED_IBT(dev);
> >> ...
> >>> +
> >>> +sysbus_init_irq(sbd, >irq);
> >>
> >> I ask because the code in aspeed_soc.c seems to connect to the
> >> lpc->subdevice_irqs[aspeed_lpc_ibt], initialised on
> >> hw/misc/aspeed_lpc.c:408.
> >> I noticed that bmc firmware running in qemu was checking the BT_CTRL
> >> register less frequently than I'd like while editing this patch to use
> >> the IPMIInterface.
>
> OK.
>
> This might be a problem in aspeed_ibt_update_irq(). This patch is
> an experiment from some few years ago. It still works good enough
> for the witherspoon-bmc and powernv9 machines for simple IPMI
> commands: fru, sdr, lan, power off (to be checked).
>
> Could you share your BMC and host command line ?
>

Host:
-chardev socket,id=ipmichr1,host=localhost,port=,reconnect=10 \
-device ipmi-bmc-extern,chardev=ipmichr1,id=bmc0 \
-device isa-ipmi-bt,bmc=bmc0,irq=10 -nodefaults

BMC:
-chardev socket,id=ipmichr1,host=localhost,port=,server=on,wait=off \
-device ipmi-host-extern,chardev=ipmichr1,responder=/machine/soc/ibt

But for this to work you need Hao's patch as well: [PATCH 7/8]
hw/ipmi: Add an IPMI external host device.

Re: [PATCH] hw/misc: Add an iBT device model

[Cédric Le Goater]

Hello Titus,

On 9/24/21 10:42, Philippe Mathieu-Daudé wrote:

On 9/24/21 01:48, Titus Rwantare wrote:

Hello all,

I'd like some clarification on how the following code transfers irqs
back and forth:

b/hw/arm/aspeed_soc.c
+    /* iBT */
+    if (!sysbus_realize(SYS_BUS_DEVICE(>ibt), errp)) {
+    return;
+    }
+    memory_region_add_subregion(>lpc.iomem,
+   sc->memmap[ASPEED_DEV_IBT] - sc->memmap[ASPEED_DEV_LPC],
+   >ibt.iomem);
+    sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_ibt,



The iBT device IRQ is connected to a subdevice irq of the LPC device.
See aspeed_lpc_realize(). And triggered in aspeed_lpc_set_irq()


Code smell indeed, likely:

     sysbus_connect_irq(SYS_BUS_DEVICE(>ibt), 1 + aspeed_lpc_ibt,


why ? See what's done above for the LPC device. Commit c59f781e3bcc
("hw/misc: Model KCS devices in the Aspeed LPC controller") and comment


/*
 * On the AST2400 and AST2500 the one LPC IRQ is shared between all of the
 * subdevices. Connect the LPC subdevice IRQs to the LPC controller IRQ (by
 * contrast, on the AST2600, the subdevice IRQs are connected straight to
 * the GIC).
 *
 * LPC subdevice IRQ sources are offset from 1 because the shared IRQ output
 * to the VIC is at offset 0.
 */




+   qdev_get_gpio_in(DEVICE(>lpc), aspeed_lpc_ibt));
}


and


hw/misc/aspeed_ibt.c
+static void aspeed_ibt_realize(DeviceState *dev, Error **errp)
+{
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    AspeedIBTState *ibt = ASPEED_IBT(dev);

...

+
+    sysbus_init_irq(sbd, >irq);


I ask because the code in aspeed_soc.c seems to connect to the
lpc->subdevice_irqs[aspeed_lpc_ibt], initialised on
hw/misc/aspeed_lpc.c:408.
I noticed that bmc firmware running in qemu was checking the BT_CTRL
register less frequently than I'd like while editing this patch to use
the IPMIInterface.


OK.

This might be a problem in aspeed_ibt_update_irq(). This patch is
an experiment from some few years ago. It still works good enough
for the witherspoon-bmc and powernv9 machines for simple IPMI
commands: fru, sdr, lan, power off (to be checked).

Could you share your BMC and host command line ?

Thanks,

C.

Re: [PATCH] hw/misc: Add an iBT device model

[Philippe Mathieu-Daudé]

On 9/24/21 01:48, Titus Rwantare wrote:

Hello all,

I'd like some clarification on how the following code transfers irqs
back and forth:


b/hw/arm/aspeed_soc.c
+/* iBT */
+if (!sysbus_realize(SYS_BUS_DEVICE(>ibt), errp)) {
+return;
+}
+memory_region_add_subregion(>lpc.iomem,
+   sc->memmap[ASPEED_DEV_IBT] - sc->memmap[ASPEED_DEV_LPC],
+   >ibt.iomem);
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_ibt,


Code smell indeed, likely:

sysbus_connect_irq(SYS_BUS_DEVICE(>ibt), 1 + aspeed_lpc_ibt,


+   qdev_get_gpio_in(DEVICE(>lpc), aspeed_lpc_ibt));
}


and


hw/misc/aspeed_ibt.c
+static void aspeed_ibt_realize(DeviceState *dev, Error **errp)
+{
+SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+AspeedIBTState *ibt = ASPEED_IBT(dev);

...

+
+sysbus_init_irq(sbd, >irq);


I ask because the code in aspeed_soc.c seems to connect to the
lpc->subdevice_irqs[aspeed_lpc_ibt], initialised on
hw/misc/aspeed_lpc.c:408.
I noticed that bmc firmware running in qemu was checking the BT_CTRL
register less frequently than I'd like while editing this patch to use
the IPMIInterface.

Thanks,
Titus

Re: [PATCH] hw/misc: Add an iBT device model

[Titus Rwantare]

Hello all,

I'd like some clarification on how the following code transfers irqs
back and forth:

> b/hw/arm/aspeed_soc.c
>+/* iBT */
>+if (!sysbus_realize(SYS_BUS_DEVICE(>ibt), errp)) {
>+return;
>+}
>+memory_region_add_subregion(>lpc.iomem,
>+   sc->memmap[ASPEED_DEV_IBT] - sc->memmap[ASPEED_DEV_LPC],
>+   >ibt.iomem);
>+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_ibt,
>+   qdev_get_gpio_in(DEVICE(>lpc), aspeed_lpc_ibt));
> }

and

>hw/misc/aspeed_ibt.c
>+static void aspeed_ibt_realize(DeviceState *dev, Error **errp)
>+{
>+SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>+AspeedIBTState *ibt = ASPEED_IBT(dev);
...
>+
>+sysbus_init_irq(sbd, >irq);

I ask because the code in aspeed_soc.c seems to connect to the
lpc->subdevice_irqs[aspeed_lpc_ibt], initialised on
hw/misc/aspeed_lpc.c:408.
I noticed that bmc firmware running in qemu was checking the BT_CTRL
register less frequently than I'd like while editing this patch to use
the IPMIInterface.

Thanks,
Titus

Re: [PATCH] hw/misc: Add an iBT device model

[Cédric Le Goater]

Hello Corey,

On 3/29/21 2:19 PM, Cédric Le Goater wrote:
> Implement an IPMI BT interface model using a chardev backend to
> communicate with an external PowerNV machine. It uses the OpenIPMI
> simulator protocol for virtual machines described in :
> 
> https://github.com/cminyard/openipmi/blob/master/lanserv/README.vm
> 
> and implemented by the 'ipmi-bmc-extern' model on the host side.
> 
> To use, start the Aspeed BMC machine with :
> 
> -chardev socket,id=ipmi0,host=localhost,port=9002,ipv4,server,nowait \
> -global driver=aspeed.ibt,property=chardev,value=ipmi0
> 
> and the PowerNV machine with :
> 
> -chardev socket,id=ipmi0,host=localhost,port=9002,reconnect=10 \
> -device ipmi-bmc-extern,id=bmc0,chardev=ipmi0 \
> -device isa-ipmi-bt,bmc=bmc0,irq=10 -nodefaults
> 
I prefer waiting for your feedback before including this patch in 
the aspeed queue I will send to Peter. When ever you have time.

Thanks,

C. 

> Cc: Corey Minyard 
> Signed-off-by: Cédric Le Goater 
> ---
>  include/hw/arm/aspeed_soc.h  |   2 +
>  include/hw/misc/aspeed_ibt.h |  47 +++
>  hw/arm/aspeed_ast2600.c  |  12 +
>  hw/arm/aspeed_soc.c  |  12 +
>  hw/misc/aspeed_ibt.c | 596 +++
>  hw/misc/meson.build  |   1 +
>  hw/misc/trace-events |   7 +
>  7 files changed, 677 insertions(+)
>  create mode 100644 include/hw/misc/aspeed_ibt.h
>  create mode 100644 hw/misc/aspeed_ibt.c
> 
> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> index d9161d26d645..f0c36b8f7d35 100644
> --- a/include/hw/arm/aspeed_soc.h
> +++ b/include/hw/arm/aspeed_soc.h
> @@ -30,6 +30,7 @@
>  #include "hw/usb/hcd-ehci.h"
>  #include "qom/object.h"
>  #include "hw/misc/aspeed_lpc.h"
> +#include "hw/misc/aspeed_ibt.h"
>  
>  #define ASPEED_SPIS_NUM  2
>  #define ASPEED_EHCIS_NUM 2
> @@ -65,6 +66,7 @@ struct AspeedSoCState {
>  AspeedSDHCIState sdhci;
>  AspeedSDHCIState emmc;
>  AspeedLPCState lpc;
> +AspeedIBTState ibt;
>  };
>  
>  #define TYPE_ASPEED_SOC "aspeed-soc"
> diff --git a/include/hw/misc/aspeed_ibt.h b/include/hw/misc/aspeed_ibt.h
> new file mode 100644
> index ..a02a57df9ff8
> --- /dev/null
> +++ b/include/hw/misc/aspeed_ibt.h
> @@ -0,0 +1,47 @@
> +/*
> + * ASPEED iBT Device
> + *
> + * Copyright 2016 IBM Corp.
> + *
> + * This code is licensed under the GPL version 2 or later.  See
> + * the COPYING file in the top-level directory.
> + */
> +#ifndef ASPEED_IBT_H
> +#define ASPEED_IBT_H
> +
> +#include "hw/sysbus.h"
> +#include "chardev/char-fe.h"
> +
> +#define TYPE_ASPEED_IBT "aspeed.ibt"
> +#define ASPEED_IBT(obj) OBJECT_CHECK(AspeedIBTState, (obj), TYPE_ASPEED_IBT)
> +
> +#define ASPEED_IBT_NR_REGS (0x1C >> 2)
> +
> +#define ASPEED_IBT_BUFFER_SIZE 64
> +
> +typedef struct AspeedIBTState {
> +/*< private >*/
> +SysBusDevice parent_obj;
> +
> +/*< public >*/
> +CharBackend chr;
> +bool connected;
> +
> +uint8_t recv_msg[ASPEED_IBT_BUFFER_SIZE];
> +uint8_t recv_msg_len;
> +int recv_msg_index;
> +int recv_msg_too_many;
> +bool recv_waiting;
> +int in_escape;
> +
> +uint8_t send_msg[ASPEED_IBT_BUFFER_SIZE];
> +uint8_t send_msg_len;
> +
> +MemoryRegion iomem;
> +qemu_irq irq;
> +
> +uint32_t regs[ASPEED_IBT_NR_REGS];
> +
> +} AspeedIBTState;
> +
> +#endif /* ASPEED_IBT_H */
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index fc81c0d8df06..c30d0f320c2a 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -219,6 +219,8 @@ static void aspeed_soc_ast2600_init(Object *obj)
>  
>  snprintf(typename, sizeof(typename), "aspeed.hace-%s", socname);
>  object_initialize_child(obj, "hace", >hace, typename);
> +
> +object_initialize_child(obj, "ibt", >ibt, TYPE_ASPEED_IBT);
>  }
>  
>  /*
> @@ -510,6 +512,16 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, 
> Error **errp)
>  sysbus_mmio_map(SYS_BUS_DEVICE(>hace), 0, 
> sc->memmap[ASPEED_DEV_HACE]);
>  sysbus_connect_irq(SYS_BUS_DEVICE(>hace), 0,
> aspeed_soc_get_irq(s, ASPEED_DEV_HACE));
> +
> +/* iBT */
> +if (!sysbus_realize(SYS_BUS_DEVICE(>ibt), errp)) {
> +return;
> +}
> +memory_region_add_subregion(>lpc.iomem,
> +   sc->memmap[ASPEED_DEV_IBT] - sc->memmap[ASPEED_DEV_LPC],
> +   >ibt.iomem);
> +sysbus_connect_irq(SYS_BUS_DEVICE(>ibt), 0,
> +   aspeed_soc_get_irq(s, ASPEED_DEV_IBT));
>  }
>  
>  static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index 4a95d27d9d63..5ab4cefc7e8b 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -219,6 +219,8 @@ static void aspeed_soc_init(Object *obj)
>  
>  snprintf(typename, sizeof(typename), "aspeed.hace-%s", socname);
>  object_initialize_child(obj, "hace", >hace, typename);
> +
> +

Re: [PATCH] hw/misc: Add an iBT device model

[Cédric Le Goater]

Hello,

On 4/5/21 6:54 PM, Hao Wu wrote:
> Hi, Cedric and Corey
> 
> When I'm implementing KCS device for nuvoton BMC boards, one of the feedback 
> Corey gave me was to refactor the existing device like ipmi-bmc-extern so 
> that we can reuse some of the common stuff there. I'm in the process of doing 
> that. I'll probably send that as an RFC first to see how you think about that.

OK. 

I wrote this code 5/6 years ago ... Feedback welcome ! 

Thanks,

C.  



>
> On Mon, Mar 29, 2021 at 5:19 AM Cédric Le Goater  > wrote:
> 
> Implement an IPMI BT interface model using a chardev backend to
> communicate with an external PowerNV machine. It uses the OpenIPMI
> simulator protocol for virtual machines described in :
> 
>     https://github.com/cminyard/openipmi/blob/master/lanserv/README.vm 
> 
> 
> and implemented by the 'ipmi-bmc-extern' model on the host side.
> 
> To use, start the Aspeed BMC machine with :
> 
>     -chardev socket,id=ipmi0,host=localhost,port=9002,ipv4,server,nowait \
>     -global driver=aspeed.ibt,property=chardev,value=ipmi0
> 
> and the PowerNV machine with :
> 
>     -chardev socket,id=ipmi0,host=localhost,port=9002,reconnect=10 \
>     -device ipmi-bmc-extern,id=bmc0,chardev=ipmi0 \
>     -device isa-ipmi-bt,bmc=bmc0,irq=10 -nodefaults
> 
> Cc: Corey Minyard mailto:cminy...@mvista.com>>
> Signed-off-by: Cédric Le Goater mailto:c...@kaod.org>>
> ---
>  include/hw/arm/aspeed_soc.h  |   2 +
>  include/hw/misc/aspeed_ibt.h |  47 +++
>  hw/arm/aspeed_ast2600.c      |  12 +
>  hw/arm/aspeed_soc.c          |  12 +
>  hw/misc/aspeed_ibt.c         | 596 +++
>  hw/misc/meson.build          |   1 +
>  hw/misc/trace-events         |   7 +
>  7 files changed, 677 insertions(+)
>  create mode 100644 include/hw/misc/aspeed_ibt.h
>  create mode 100644 hw/misc/aspeed_ibt.c
> 
> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> index d9161d26d645..f0c36b8f7d35 100644
> --- a/include/hw/arm/aspeed_soc.h
> +++ b/include/hw/arm/aspeed_soc.h
> @@ -30,6 +30,7 @@
>  #include "hw/usb/hcd-ehci.h"
>  #include "qom/object.h"
>  #include "hw/misc/aspeed_lpc.h"
> +#include "hw/misc/aspeed_ibt.h"
> 
>  #define ASPEED_SPIS_NUM  2
>  #define ASPEED_EHCIS_NUM 2
> @@ -65,6 +66,7 @@ struct AspeedSoCState {
>      AspeedSDHCIState sdhci;
>      AspeedSDHCIState emmc;
>      AspeedLPCState lpc;
> +    AspeedIBTState ibt;
>  };
> 
>  #define TYPE_ASPEED_SOC "aspeed-soc"
> diff --git a/include/hw/misc/aspeed_ibt.h b/include/hw/misc/aspeed_ibt.h
> new file mode 100644
> index ..a02a57df9ff8
> --- /dev/null
> +++ b/include/hw/misc/aspeed_ibt.h
> @@ -0,0 +1,47 @@
> +/*
> + * ASPEED iBT Device
> + *
> + * Copyright 2016 IBM Corp.
> + *
> + * This code is licensed under the GPL version 2 or later.  See
> + * the COPYING file in the top-level directory.
> + */
> +#ifndef ASPEED_IBT_H
> +#define ASPEED_IBT_H
> +
> +#include "hw/sysbus.h"
> +#include "chardev/char-fe.h"
> +
> +#define TYPE_ASPEED_IBT "aspeed.ibt"
> +#define ASPEED_IBT(obj) OBJECT_CHECK(AspeedIBTState, (obj), 
> TYPE_ASPEED_IBT)
> +
> +#define ASPEED_IBT_NR_REGS (0x1C >> 2)
> +
> +#define ASPEED_IBT_BUFFER_SIZE 64
> +
> +typedef struct AspeedIBTState {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +
> +    /*< public >*/
> +    CharBackend chr;
> +    bool connected;
> +
> +    uint8_t recv_msg[ASPEED_IBT_BUFFER_SIZE];
> +    uint8_t recv_msg_len;
> +    int recv_msg_index;
> +    int recv_msg_too_many;
> +    bool recv_waiting;
> +    int in_escape;
> +
> +    uint8_t send_msg[ASPEED_IBT_BUFFER_SIZE];
> +    uint8_t send_msg_len;
> +
> +    MemoryRegion iomem;
> +    qemu_irq irq;
> +
> +    uint32_t regs[ASPEED_IBT_NR_REGS];
> +
> +} AspeedIBTState;
> +
> +#endif /* ASPEED_IBT_H */
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index fc81c0d8df06..c30d0f320c2a 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -219,6 +219,8 @@ static void aspeed_soc_ast2600_init(Object *obj)
> 
>      snprintf(typename, sizeof(typename), "aspeed.hace-%s", socname);
>      object_initialize_child(obj, "hace", >hace, typename);
> +
> +    object_initialize_child(obj, "ibt", >ibt, TYPE_ASPEED_IBT);
>  }
> 
>  /*
> @@ -510,6 +512,16 @@ static void aspeed_soc_ast2600_realize(DeviceState 
> *dev, Error **errp)
>      sysbus_mmio_map(SYS_BUS_DEVICE(>hace), 0, 
> sc->memmap[ASPEED_DEV_HACE]);
>      sysbus_connect_irq(SYS_BUS_DEVICE(>hace), 

Re: [PATCH] hw/misc: Add an iBT device model

[Hao Wu]

Hi, Cedric and Corey

When I'm implementing KCS device for nuvoton BMC boards, one of the
feedback Corey gave me was to refactor the existing device like
ipmi-bmc-extern so that we can reuse some of the common stuff there. I'm in
the process of doing that. I'll probably send that as an RFC first to see
how you think about that.

On Mon, Mar 29, 2021 at 5:19 AM Cédric Le Goater  wrote:

> Implement an IPMI BT interface model using a chardev backend to
> communicate with an external PowerNV machine. It uses the OpenIPMI
> simulator protocol for virtual machines described in :
>
> https://github.com/cminyard/openipmi/blob/master/lanserv/README.vm
>
> and implemented by the 'ipmi-bmc-extern' model on the host side.
>
> To use, start the Aspeed BMC machine with :
>
> -chardev socket,id=ipmi0,host=localhost,port=9002,ipv4,server,nowait \
> -global driver=aspeed.ibt,property=chardev,value=ipmi0
>
> and the PowerNV machine with :
>
> -chardev socket,id=ipmi0,host=localhost,port=9002,reconnect=10 \
> -device ipmi-bmc-extern,id=bmc0,chardev=ipmi0 \
> -device isa-ipmi-bt,bmc=bmc0,irq=10 -nodefaults
>
> Cc: Corey Minyard 
> Signed-off-by: Cédric Le Goater 
> ---
>  include/hw/arm/aspeed_soc.h  |   2 +
>  include/hw/misc/aspeed_ibt.h |  47 +++
>  hw/arm/aspeed_ast2600.c  |  12 +
>  hw/arm/aspeed_soc.c  |  12 +
>  hw/misc/aspeed_ibt.c | 596 +++
>  hw/misc/meson.build  |   1 +
>  hw/misc/trace-events |   7 +
>  7 files changed, 677 insertions(+)
>  create mode 100644 include/hw/misc/aspeed_ibt.h
>  create mode 100644 hw/misc/aspeed_ibt.c
>
> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> index d9161d26d645..f0c36b8f7d35 100644
> --- a/include/hw/arm/aspeed_soc.h
> +++ b/include/hw/arm/aspeed_soc.h
> @@ -30,6 +30,7 @@
>  #include "hw/usb/hcd-ehci.h"
>  #include "qom/object.h"
>  #include "hw/misc/aspeed_lpc.h"
> +#include "hw/misc/aspeed_ibt.h"
>
>  #define ASPEED_SPIS_NUM  2
>  #define ASPEED_EHCIS_NUM 2
> @@ -65,6 +66,7 @@ struct AspeedSoCState {
>  AspeedSDHCIState sdhci;
>  AspeedSDHCIState emmc;
>  AspeedLPCState lpc;
> +AspeedIBTState ibt;
>  };
>
>  #define TYPE_ASPEED_SOC "aspeed-soc"
> diff --git a/include/hw/misc/aspeed_ibt.h b/include/hw/misc/aspeed_ibt.h
> new file mode 100644
> index ..a02a57df9ff8
> --- /dev/null
> +++ b/include/hw/misc/aspeed_ibt.h
> @@ -0,0 +1,47 @@
> +/*
> + * ASPEED iBT Device
> + *
> + * Copyright 2016 IBM Corp.
> + *
> + * This code is licensed under the GPL version 2 or later.  See
> + * the COPYING file in the top-level directory.
> + */
> +#ifndef ASPEED_IBT_H
> +#define ASPEED_IBT_H
> +
> +#include "hw/sysbus.h"
> +#include "chardev/char-fe.h"
> +
> +#define TYPE_ASPEED_IBT "aspeed.ibt"
> +#define ASPEED_IBT(obj) OBJECT_CHECK(AspeedIBTState, (obj),
> TYPE_ASPEED_IBT)
> +
> +#define ASPEED_IBT_NR_REGS (0x1C >> 2)
> +
> +#define ASPEED_IBT_BUFFER_SIZE 64
> +
> +typedef struct AspeedIBTState {
> +/*< private >*/
> +SysBusDevice parent_obj;
> +
> +/*< public >*/
> +CharBackend chr;
> +bool connected;
> +
> +uint8_t recv_msg[ASPEED_IBT_BUFFER_SIZE];
> +uint8_t recv_msg_len;
> +int recv_msg_index;
> +int recv_msg_too_many;
> +bool recv_waiting;
> +int in_escape;
> +
> +uint8_t send_msg[ASPEED_IBT_BUFFER_SIZE];
> +uint8_t send_msg_len;
> +
> +MemoryRegion iomem;
> +qemu_irq irq;
> +
> +uint32_t regs[ASPEED_IBT_NR_REGS];
> +
> +} AspeedIBTState;
> +
> +#endif /* ASPEED_IBT_H */
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index fc81c0d8df06..c30d0f320c2a 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -219,6 +219,8 @@ static void aspeed_soc_ast2600_init(Object *obj)
>
>  snprintf(typename, sizeof(typename), "aspeed.hace-%s", socname);
>  object_initialize_child(obj, "hace", >hace, typename);
> +
> +object_initialize_child(obj, "ibt", >ibt, TYPE_ASPEED_IBT);
>  }
>
>  /*
> @@ -510,6 +512,16 @@ static void aspeed_soc_ast2600_realize(DeviceState
> *dev, Error **errp)
>  sysbus_mmio_map(SYS_BUS_DEVICE(>hace), 0,
> sc->memmap[ASPEED_DEV_HACE]);
>  sysbus_connect_irq(SYS_BUS_DEVICE(>hace), 0,
> aspeed_soc_get_irq(s, ASPEED_DEV_HACE));
> +
> +/* iBT */
> +if (!sysbus_realize(SYS_BUS_DEVICE(>ibt), errp)) {
> +return;
> +}
> +memory_region_add_subregion(>lpc.iomem,
> +   sc->memmap[ASPEED_DEV_IBT] -
> sc->memmap[ASPEED_DEV_LPC],
> +   >ibt.iomem);
> +sysbus_connect_irq(SYS_BUS_DEVICE(>ibt), 0,
> +   aspeed_soc_get_irq(s, ASPEED_DEV_IBT));
>  }
>
>  static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index 4a95d27d9d63..5ab4cefc7e8b 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -219,6 +219,8 @@ static void 

Re: [PATCH] hw/misc: Add an iBT device model

[Joel Stanley]

On Mon, 29 Mar 2021 at 12:19, Cédric Le Goater  wrote:
>
> Implement an IPMI BT interface model using a chardev backend to
> communicate with an external PowerNV machine. It uses the OpenIPMI
> simulator protocol for virtual machines described in :
>
> https://github.com/cminyard/openipmi/blob/master/lanserv/README.vm
>
> and implemented by the 'ipmi-bmc-extern' model on the host side.
>
> To use, start the Aspeed BMC machine with :
>
> -chardev socket,id=ipmi0,host=localhost,port=9002,ipv4,server,nowait \
> -global driver=aspeed.ibt,property=chardev,value=ipmi0
>
> and the PowerNV machine with :
>
> -chardev socket,id=ipmi0,host=localhost,port=9002,reconnect=10 \
> -device ipmi-bmc-extern,id=bmc0,chardev=ipmi0 \
> -device isa-ipmi-bt,bmc=bmc0,irq=10 -nodefaults
>
> Cc: Corey Minyard 
> Signed-off-by: Cédric Le Goater 

Yay!

Reviewed-by: Joel Stanley 

> ---
>  include/hw/arm/aspeed_soc.h  |   2 +
>  include/hw/misc/aspeed_ibt.h |  47 +++
>  hw/arm/aspeed_ast2600.c  |  12 +
>  hw/arm/aspeed_soc.c  |  12 +
>  hw/misc/aspeed_ibt.c | 596 +++
>  hw/misc/meson.build  |   1 +
>  hw/misc/trace-events |   7 +
>  7 files changed, 677 insertions(+)
>  create mode 100644 include/hw/misc/aspeed_ibt.h
>  create mode 100644 hw/misc/aspeed_ibt.c
>
> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> index d9161d26d645..f0c36b8f7d35 100644
> --- a/include/hw/arm/aspeed_soc.h
> +++ b/include/hw/arm/aspeed_soc.h
> @@ -30,6 +30,7 @@
>  #include "hw/usb/hcd-ehci.h"
>  #include "qom/object.h"
>  #include "hw/misc/aspeed_lpc.h"
> +#include "hw/misc/aspeed_ibt.h"
>
>  #define ASPEED_SPIS_NUM  2
>  #define ASPEED_EHCIS_NUM 2
> @@ -65,6 +66,7 @@ struct AspeedSoCState {
>  AspeedSDHCIState sdhci;
>  AspeedSDHCIState emmc;
>  AspeedLPCState lpc;
> +AspeedIBTState ibt;
>  };
>
>  #define TYPE_ASPEED_SOC "aspeed-soc"
> diff --git a/include/hw/misc/aspeed_ibt.h b/include/hw/misc/aspeed_ibt.h
> new file mode 100644
> index ..a02a57df9ff8
> --- /dev/null
> +++ b/include/hw/misc/aspeed_ibt.h
> @@ -0,0 +1,47 @@
> +/*
> + * ASPEED iBT Device
> + *
> + * Copyright 2016 IBM Corp.
> + *
> + * This code is licensed under the GPL version 2 or later.  See
> + * the COPYING file in the top-level directory.
> + */
> +#ifndef ASPEED_IBT_H
> +#define ASPEED_IBT_H
> +
> +#include "hw/sysbus.h"
> +#include "chardev/char-fe.h"
> +
> +#define TYPE_ASPEED_IBT "aspeed.ibt"
> +#define ASPEED_IBT(obj) OBJECT_CHECK(AspeedIBTState, (obj), TYPE_ASPEED_IBT)
> +
> +#define ASPEED_IBT_NR_REGS (0x1C >> 2)
> +
> +#define ASPEED_IBT_BUFFER_SIZE 64
> +
> +typedef struct AspeedIBTState {
> +/*< private >*/
> +SysBusDevice parent_obj;
> +
> +/*< public >*/
> +CharBackend chr;
> +bool connected;
> +
> +uint8_t recv_msg[ASPEED_IBT_BUFFER_SIZE];
> +uint8_t recv_msg_len;
> +int recv_msg_index;
> +int recv_msg_too_many;
> +bool recv_waiting;
> +int in_escape;
> +
> +uint8_t send_msg[ASPEED_IBT_BUFFER_SIZE];
> +uint8_t send_msg_len;
> +
> +MemoryRegion iomem;
> +qemu_irq irq;
> +
> +uint32_t regs[ASPEED_IBT_NR_REGS];
> +
> +} AspeedIBTState;
> +
> +#endif /* ASPEED_IBT_H */
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index fc81c0d8df06..c30d0f320c2a 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -219,6 +219,8 @@ static void aspeed_soc_ast2600_init(Object *obj)
>
>  snprintf(typename, sizeof(typename), "aspeed.hace-%s", socname);
>  object_initialize_child(obj, "hace", >hace, typename);
> +
> +object_initialize_child(obj, "ibt", >ibt, TYPE_ASPEED_IBT);
>  }
>
>  /*
> @@ -510,6 +512,16 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, 
> Error **errp)
>  sysbus_mmio_map(SYS_BUS_DEVICE(>hace), 0, 
> sc->memmap[ASPEED_DEV_HACE]);
>  sysbus_connect_irq(SYS_BUS_DEVICE(>hace), 0,
> aspeed_soc_get_irq(s, ASPEED_DEV_HACE));
> +
> +/* iBT */
> +if (!sysbus_realize(SYS_BUS_DEVICE(>ibt), errp)) {
> +return;
> +}
> +memory_region_add_subregion(>lpc.iomem,
> +   sc->memmap[ASPEED_DEV_IBT] - sc->memmap[ASPEED_DEV_LPC],
> +   >ibt.iomem);
> +sysbus_connect_irq(SYS_BUS_DEVICE(>ibt), 0,
> +   aspeed_soc_get_irq(s, ASPEED_DEV_IBT));
>  }
>
>  static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index 4a95d27d9d63..5ab4cefc7e8b 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -219,6 +219,8 @@ static void aspeed_soc_init(Object *obj)
>
>  snprintf(typename, sizeof(typename), "aspeed.hace-%s", socname);
>  object_initialize_child(obj, "hace", >hace, typename);
> +
> +object_initialize_child(obj, "ibt", >ibt, TYPE_ASPEED_IBT);
>  }
>
>  static void aspeed_soc_realize(DeviceState *dev, Error **errp)
> @@ 

[PATCH] hw/misc: Add an iBT device model

[Cédric Le Goater]

Implement an IPMI BT interface model using a chardev backend to
communicate with an external PowerNV machine. It uses the OpenIPMI
simulator protocol for virtual machines described in :

https://github.com/cminyard/openipmi/blob/master/lanserv/README.vm

and implemented by the 'ipmi-bmc-extern' model on the host side.

To use, start the Aspeed BMC machine with :

-chardev socket,id=ipmi0,host=localhost,port=9002,ipv4,server,nowait \
-global driver=aspeed.ibt,property=chardev,value=ipmi0

and the PowerNV machine with :

-chardev socket,id=ipmi0,host=localhost,port=9002,reconnect=10 \
-device ipmi-bmc-extern,id=bmc0,chardev=ipmi0 \
-device isa-ipmi-bt,bmc=bmc0,irq=10 -nodefaults

Cc: Corey Minyard 
Signed-off-by: Cédric Le Goater 
---
 include/hw/arm/aspeed_soc.h  |   2 +
 include/hw/misc/aspeed_ibt.h |  47 +++
 hw/arm/aspeed_ast2600.c  |  12 +
 hw/arm/aspeed_soc.c  |  12 +
 hw/misc/aspeed_ibt.c | 596 +++
 hw/misc/meson.build  |   1 +
 hw/misc/trace-events |   7 +
 7 files changed, 677 insertions(+)
 create mode 100644 include/hw/misc/aspeed_ibt.h
 create mode 100644 hw/misc/aspeed_ibt.c

diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index d9161d26d645..f0c36b8f7d35 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -30,6 +30,7 @@
 #include "hw/usb/hcd-ehci.h"
 #include "qom/object.h"
 #include "hw/misc/aspeed_lpc.h"
+#include "hw/misc/aspeed_ibt.h"
 
 #define ASPEED_SPIS_NUM  2
 #define ASPEED_EHCIS_NUM 2
@@ -65,6 +66,7 @@ struct AspeedSoCState {
 AspeedSDHCIState sdhci;
 AspeedSDHCIState emmc;
 AspeedLPCState lpc;
+AspeedIBTState ibt;
 };
 
 #define TYPE_ASPEED_SOC "aspeed-soc"
diff --git a/include/hw/misc/aspeed_ibt.h b/include/hw/misc/aspeed_ibt.h
new file mode 100644
index ..a02a57df9ff8
--- /dev/null
+++ b/include/hw/misc/aspeed_ibt.h
@@ -0,0 +1,47 @@
+/*
+ * ASPEED iBT Device
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This code is licensed under the GPL version 2 or later.  See
+ * the COPYING file in the top-level directory.
+ */
+#ifndef ASPEED_IBT_H
+#define ASPEED_IBT_H
+
+#include "hw/sysbus.h"
+#include "chardev/char-fe.h"
+
+#define TYPE_ASPEED_IBT "aspeed.ibt"
+#define ASPEED_IBT(obj) OBJECT_CHECK(AspeedIBTState, (obj), TYPE_ASPEED_IBT)
+
+#define ASPEED_IBT_NR_REGS (0x1C >> 2)
+
+#define ASPEED_IBT_BUFFER_SIZE 64
+
+typedef struct AspeedIBTState {
+/*< private >*/
+SysBusDevice parent_obj;
+
+/*< public >*/
+CharBackend chr;
+bool connected;
+
+uint8_t recv_msg[ASPEED_IBT_BUFFER_SIZE];
+uint8_t recv_msg_len;
+int recv_msg_index;
+int recv_msg_too_many;
+bool recv_waiting;
+int in_escape;
+
+uint8_t send_msg[ASPEED_IBT_BUFFER_SIZE];
+uint8_t send_msg_len;
+
+MemoryRegion iomem;
+qemu_irq irq;
+
+uint32_t regs[ASPEED_IBT_NR_REGS];
+
+} AspeedIBTState;
+
+#endif /* ASPEED_IBT_H */
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index fc81c0d8df06..c30d0f320c2a 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -219,6 +219,8 @@ static void aspeed_soc_ast2600_init(Object *obj)
 
 snprintf(typename, sizeof(typename), "aspeed.hace-%s", socname);
 object_initialize_child(obj, "hace", >hace, typename);
+
+object_initialize_child(obj, "ibt", >ibt, TYPE_ASPEED_IBT);
 }
 
 /*
@@ -510,6 +512,16 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, 
Error **errp)
 sysbus_mmio_map(SYS_BUS_DEVICE(>hace), 0, sc->memmap[ASPEED_DEV_HACE]);
 sysbus_connect_irq(SYS_BUS_DEVICE(>hace), 0,
aspeed_soc_get_irq(s, ASPEED_DEV_HACE));
+
+/* iBT */
+if (!sysbus_realize(SYS_BUS_DEVICE(>ibt), errp)) {
+return;
+}
+memory_region_add_subregion(>lpc.iomem,
+   sc->memmap[ASPEED_DEV_IBT] - sc->memmap[ASPEED_DEV_LPC],
+   >ibt.iomem);
+sysbus_connect_irq(SYS_BUS_DEVICE(>ibt), 0,
+   aspeed_soc_get_irq(s, ASPEED_DEV_IBT));
 }
 
 static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 4a95d27d9d63..5ab4cefc7e8b 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -219,6 +219,8 @@ static void aspeed_soc_init(Object *obj)
 
 snprintf(typename, sizeof(typename), "aspeed.hace-%s", socname);
 object_initialize_child(obj, "hace", >hace, typename);
+
+object_initialize_child(obj, "ibt", >ibt, TYPE_ASPEED_IBT);
 }
 
 static void aspeed_soc_realize(DeviceState *dev, Error **errp)
@@ -438,6 +440,16 @@ static void aspeed_soc_realize(DeviceState *dev, Error 
**errp)
 sysbus_mmio_map(SYS_BUS_DEVICE(>hace), 0, sc->memmap[ASPEED_DEV_HACE]);
 sysbus_connect_irq(SYS_BUS_DEVICE(>hace), 0,
aspeed_soc_get_irq(s, ASPEED_DEV_HACE));
+
+/* iBT */
+if (!sysbus_realize(SYS_BUS_DEVICE(>ibt), errp)) {
+return;
+}
+