Re: [PATCH RESEND v6 14/36] multi-process: setup a machine object for remote device process

[Jag Raman]

> On May 12, 2020, at 6:43 AM, Stefan Hajnoczi <address@hidden> wrote:
> 
> On Wed, Apr 22, 2020 at 09:13:49PM -0700, address@hidden wrote:
>> From: Jagannathan Raman <address@hidden>
>> 
>> remote-machine object sets up various subsystems of the remote device
>> process. Instantiate PCI host bridge object and initialize RAM, IO &
>> PCI memory regions.
>> 
>> Signed-off-by: John G Johnson <address@hidden>
>> Signed-off-by: Jagannathan Raman <address@hidden>
>> Signed-off-by: Elena Ufimtseva <address@hidden>
>> ---
>> MAINTAINERS                   |  2 +
>> Makefile.objs                 |  1 +
>> exec.c                        |  3 +-
>> include/exec/address-spaces.h |  2 +
>> include/remote/machine.h      | 30 +++++++++++++
>> remote/Makefile.objs          |  2 +
>> remote/machine.c              | 84 +++++++++++++++++++++++++++++++++++
>> remote/remote-main.c          |  7 +++
> 
> Now that the separate remote emulation program is going away I think it
> makes sense to move the PCIe host and machine type into hw/:
> 
>  hw/pci-host/remote.c <-- PCIe host
>  hw/i386/remote.c     <-- machine type (could be moved again later if
>                           other architectures are supported)

OK, got it.

> 
>> diff --git a/exec.c b/exec.c
>> index d0ac9545f4..5b1e414099 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -161,7 +161,6 @@ typedef struct subpage_t {
>> #define PHYS_SECTION_UNASSIGNED 0
>> 
>> static void io_mem_init(void);
>> -static void memory_map_init(void);
> 
> The memory_map_init() change is unnecessary once a softmmu target is
> used since it will be called from cpu_exec_init_all().

OK.

> 
>> +static void remote_machine_init(Object *obj)
>> +{
>> +    RemMachineState *s = REMOTE_MACHINE(obj);
>> +    RemPCIHost *rem_host;
>> +    MemoryRegion *system_memory, *system_io, *pci_memory;
>> +
>> +    Error *error_abort = NULL;
>> +
>> +    object_property_add_child(object_get_root(), "machine", obj, 
>> &error_abort);
>> +    if (error_abort) {
> 
> error_abort aborts the program so handling it is not necessary.

OK, thanks!

> 
>> +        error_report_err(error_abort);
>> +    }
>> +
>> +    memory_map_init();
>> +
>> +    system_memory = get_system_memory();
>> +    system_io = get_system_io();
>> +
>> +    pci_memory = g_new(MemoryRegion, 1);
>> +    memory_region_init(pci_memory, NULL, "pci", UINT64_MAX);
>> +
>> +    rem_host = REMOTE_HOST_DEVICE(qdev_create(NULL, 
>> TYPE_REMOTE_HOST_DEVICE));
>> +
>> +    rem_host->mr_pci_mem = pci_memory;
>> +    rem_host->mr_sys_mem = system_memory;
>> +    rem_host->mr_sys_io = system_io;
>> +
>> +    s->host = rem_host;
>> +
>> +    object_property_add_child(OBJECT(s), "remote-device", OBJECT(rem_host),
>> +                              &error_abort);
>> +    if (error_abort) {
> 
> error_abort aborts the program so handling it is not necessary.
> 
>> +        error_report_err(error_abort);
>> +        return;
>> +    }
>> +
>> +    qemu_mutex_lock_iothread();
> 
> This will be executed with the iothread lock held. There is no need to
> acquire it.

Yes, this wouldn’t be necessary from QEMU’s main loop.

Thanks!
--
Jag

> 
>> +    memory_region_add_subregion_overlap(system_memory, 0x0, pci_memory, -1);
>> +    qemu_mutex_unlock_iothread();
>> +
>> +    qdev_init_nofail(DEVICE(rem_host));
>> +}