On Mon, 10 Jan 2022 at 17:56, Patrick Venture <vent...@google.com> wrote: > > From: Hao Wu <wuhao...@google.com> > > The PCI Mailbox Module is a high-bandwidth communcation module > between a Nuvoton BMC and CPU. It features 16KB RAM that are both > accessible by the BMC and core CPU. and supports interrupt for > both sides. > > This patch implements the BMC side of the PCI mailbox module. > Communication with the core CPU is emulated via a chardev and > will be in a follow-up patch. > > Reviewed-by: Patrick Venture <vent...@google.com> > Reviewed-by: Joe Komlodi <koml...@google.com>
Hi; this mostly looks good, but I have a question about s->content. > +static void npcm7xx_pci_mbox_init(Object *obj) > +{ > + NPCM7xxPCIMBoxState *s = NPCM7XX_PCI_MBOX(obj); > + SysBusDevice *sbd = SYS_BUS_DEVICE(obj); > + > + memory_region_init_ram_device_ptr(&s->ram, obj, "pci-mbox-ram", > + NPCM7XX_PCI_MBOX_RAM_SIZE, s->content); What's s->content for? Nothing in the rest of the device emulation touches it, which seems odd. memory_region_init_ram_device_ptr() is intended primarily for "create a MemoryRegion corresponding to something like a bit of a host device (eg a host PCI MMIO BAR). That doesn't seem like what you're doing here. In particular, using it means that you take on responsibility for ensuring that the underlying memory gets migrated, which you're not doing. If you just want a MemoryRegion that's backed by a bit of host memory, use memory_region_init_ram(). > +#define TYPE_NPCM7XX_PCI_MBOX "npcm7xx-pci-mbox" > +#define NPCM7XX_PCI_MBOX(obj) \ > + OBJECT_CHECK(NPCM7xxPCIMBoxState, (obj), TYPE_NPCM7XX_PCI_MBOX) We prefer the OBJECT_DECLARE_SIMPLE_TYPE() macro rather than hand-defining a cast macro these days. thanks -- PMM