Re: [PATCH 1/2] hw/misc: Add Nuvoton's PCI Mailbox Module

[Peter Maydell <peter.maydell@linaro.org>]

On Mon, 10 Jan 2022 at 17:56, Patrick Venture <venture@google.com> wrote:
>
> From: Hao Wu <wuhaotsh@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 <venture@google.com>
> Reviewed-by: Joe Komlodi <komlodi@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