On Thu, Jan 27, 2022 at 1:27 PM Patrick Venture <vent...@google.com> wrote:
>
>
> On Thu, Jan 27, 2022 at 10:37 AM Peter Maydell <peter.mayd...@linaro.org>
> wrote:
>
>> 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.
>>
>
> Hao informed me that we can drop the content bit here, since it's not used
> until a later patch that we'll be sending up with a bit more detail when we
> get a chance. I sent this series up to land some of the groundwork.
>
> I can send out a v2 with that bit removed.
>
>
>>
>> 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().
>>
>
> I think this is what we use it for in the future patches, when we add it
> back, it'll come with the context.
>
>
>>
>> > +#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.
>>
>
> Ack.
>
>
>>
>> thanks
>> -- PMM
>>
>
Peter, just an FYI, this fell off my radar, but I will be circling back
this week to revisit any patches I've sent that are in limbo or waiting on
me, etc. Thanks for your patience.
>
> Thanks!
>
