Re: [PATCH RESEND v6 13/36] multi-process: setup PCI host bridge for remote device

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4753 bytes --]

On Wed, Apr 22, 2020 at 09:13:48PM -0700, elena.ufimtseva@oracle.com wrote:
> diff --git a/include/remote/pcihost.h b/include/remote/pcihost.h
> new file mode 100644
> index 0000000000..7aca9ccaf1
> --- /dev/null
> +++ b/include/remote/pcihost.h
> @@ -0,0 +1,45 @@
> +/*
> + * PCI Host for remote device
> + *
> + * Copyright © 2018, 2020 Oracle and/or its affiliates.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#ifndef REMOTE_PCIHOST_H
> +#define REMOTE_PCIHOST_H
> +
> +#include <stddef.h>
> +#include <stdint.h>
> +
> +#include "exec/memory.h"
> +#include "hw/pci/pcie_host.h"
> +
> +#define TYPE_REMOTE_HOST_DEVICE "remote-pcihost"
> +#define REMOTE_HOST_DEVICE(obj) \
> +    OBJECT_CHECK(RemPCIHost, (obj), TYPE_REMOTE_HOST_DEVICE)
> +
> +typedef struct RemPCIHost {

Hmm...this object has no state or behavior specific to remote device
emulation. Could you use an existing PCIe host instead? It's not clear
to me that a new object is needed.

> +    /*< private >*/
> +    PCIExpressHost parent_obj;
> +    /*< public >*/
> +
> +    /*
> +     * Memory Controller Hub (MCH) may not be necessary for the emulation
> +     * program. The two important reasons for implementing a PCI host in the
> +     * emulation program are:
> +     * - Provide a PCI bus for IO devices
> +     * - Enable translation of guest PA to the PCI bar regions
> +     *
> +     * For both the above mentioned purposes, it doesn't look like we would
> +     * need the MCH
> +     */
> +
> +    MemoryRegion *mr_pci_mem;
> +    MemoryRegion *mr_sys_mem;

Unused?

> +    MemoryRegion *mr_sys_io;
> +} RemPCIHost;

The name "RemotePCIHost" would be consistent with the QOM type and the
filename. It might seem trivial but when reading code that others have
written every time the naming changes you need to figure out why (just
an inconsistency or is this a different concept/abstraction?).

> +
> +#endif
> diff --git a/remote/Makefile.objs b/remote/Makefile.objs
> index a9b2256b2a..2757f5a265 100644
> --- a/remote/Makefile.objs
> +++ b/remote/Makefile.objs
> @@ -1 +1,2 @@
>  remote-pci-obj-$(CONFIG_MPQEMU) += remote-main.o
> +remote-pci-obj-$(CONFIG_MPQEMU) += pcihost.o
> diff --git a/remote/pcihost.c b/remote/pcihost.c
> new file mode 100644
> index 0000000000..dbe081903e
> --- /dev/null
> +++ b/remote/pcihost.c
> @@ -0,0 +1,64 @@
> +/*
> + * Remote PCI host device
> + *
> + * Copyright © 2018, 2020 Oracle and/or its affiliates.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include <sys/types.h>
> +#include <unistd.h>

"qemu/osdep.h" already includes these headers.

> +#include "qemu/osdep.h"
> +#include "hw/pci/pci.h"
> +#include "hw/pci/pci_host.h"
> +#include "hw/pci/pcie_host.h"
> +#include "hw/qdev-properties.h"
> +#include "remote/pcihost.h"
> +#include "exec/memory.h"
> +
> +static const char *remote_host_root_bus_path(PCIHostState *host_bridge,
> +                                             PCIBus *rootbus)
> +{
> +    return "0000:00";
> +}
> +
> +static void remote_host_realize(DeviceState *dev, Error **errp)
> +{
> +    char *busname = g_strdup_printf("remote-pci-%ld", (unsigned long)getpid());
> +    PCIHostState *pci = PCI_HOST_BRIDGE(dev);
> +    RemPCIHost *s = REMOTE_HOST_DEVICE(dev);
> +
> +    pci->bus = pci_root_bus_new(DEVICE(s), busname,
> +                                s->mr_pci_mem, s->mr_sys_io,
> +                                0, TYPE_PCIE_BUS);
> +}
> +
> +static void remote_host_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
> +
> +    hc->root_bus_path = remote_host_root_bus_path;
> +    dc->realize = remote_host_realize;
> +
> +    dc->user_creatable = false;
> +    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> +    dc->fw_name = "pci";
> +}
> +
> +static const TypeInfo remote_host_info = {
> +    .name = TYPE_REMOTE_HOST_DEVICE,
> +    .parent = TYPE_PCIE_HOST_BRIDGE,
> +    .instance_size = sizeof(RemPCIHost),
> +    .class_init = remote_host_class_init,
> +};
> +
> +static void remote_machine_register(void)
> +{
> +    type_register_static(&remote_host_info);
> +}
> +
> +type_init(remote_machine_register)

The naming in this file is inconsistent:

remote_host_root_bus_path -> remote_pcihost_root_bus_path
remote_machine_register -> remote_pcihost_register

I haven't listed all instances.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]