Re: [PATCH v2 6/6] xen_arm: Add virtual PCIe host bridge support

[Volodymyr Babchuk]

Hi Igor,

Thank you for the review,

Igor Mammedov <imammedo@redhat.com> writes:

> On Tue, 21 Nov 2023 22:10:28 +0000
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> wrote:
>
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> 
>> The bridge is needed for virtio-pci support, as QEMU can emulate the
>> whole bridge with any virtio-pci devices connected to it.
>> 
>> This patch provides a flexible way to configure PCIe brige resources
>> with xenstore. We made this for several reasons:
>> 
>> - We don't want to clash with vPCI devices, so we need information
>>   from Xen toolstack on which PCI bus to use.
>> - The guest memory layout that describes these resources is not stable
>>   and may vary between guests, so we cannot rely on static resources
>>   to be always the same for both ends.
>> - Also the device-models which run in different domains and serve
>>   virtio-pci devices for the same guest should use different host
>>   bridge resources for Xen to distinguish. The rule for the guest
>>   device-tree generation is one PCI host bridge per backend domain.
>> 
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> 
>> ---
>> 
>> Changes from v1:
>> 
>>  - Renamed virtio_pci_host to pcie_host entries in XenStore, because
>>  there is nothing specific to virtio-pci: any PCI device can be
>>  emulated via this newly created bridge.
>> ---
>>  hw/arm/xen_arm.c            | 186 ++++++++++++++++++++++++++++++++++++
>>  hw/xen/xen-hvm-common.c     |   9 +-
>>  include/hw/xen/xen_native.h |   8 +-
>>  3 files changed, 200 insertions(+), 3 deletions(-)
>> 
>> diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c
>> index b9c3ae14b6..d506d55d0f 100644
>> --- a/hw/arm/xen_arm.c
>> +++ b/hw/arm/xen_arm.c
>> @@ -22,6 +22,7 @@
>>   */
>>  
>>  #include "qemu/osdep.h"
>> +#include "qemu/cutils.h"
>>  #include "qemu/error-report.h"
>>  #include "qapi/qapi-commands-migration.h"
>>  #include "qapi/visitor.h"
>> @@ -34,6 +35,9 @@
>>  #include "hw/xen/xen-hvm-common.h"
>>  #include "sysemu/tpm.h"
>>  #include "hw/xen/arch_hvm.h"
>> +#include "exec/address-spaces.h"
>> +#include "hw/pci-host/gpex.h"
>> +#include "hw/virtio/virtio-pci.h"
>>  
>>  #define TYPE_XEN_ARM  MACHINE_TYPE_NAME("xenpvh")
>>  OBJECT_DECLARE_SIMPLE_TYPE(XenArmState, XEN_ARM)
>> @@ -58,6 +62,11 @@ struct XenArmState {
>>      struct {
>>          uint64_t tpm_base_addr;
>>      } cfg;
>> +
>> +    MemMapEntry pcie_mmio;
>> +    MemMapEntry pcie_ecam;
>> +    MemMapEntry pcie_mmio_high;
>> +    int         pcie_irq;
>>  };
>>  
>>  static MemoryRegion ram_lo, ram_hi;
>> @@ -73,6 +82,7 @@ static MemoryRegion ram_lo, ram_hi;
>>  #define NR_VIRTIO_MMIO_DEVICES   \
>>     (GUEST_VIRTIO_MMIO_SPI_LAST - GUEST_VIRTIO_MMIO_SPI_FIRST)
>>  
>> +/* TODO It should be xendevicemodel_set_pci_intx_level() for PCI 
>> interrupts. */
>>  static void xen_set_irq(void *opaque, int irq, int level)
>>  {
>>      if (xendevicemodel_set_irq_level(xen_dmod, xen_domid, irq, level)) {
>> @@ -129,6 +139,176 @@ static void xen_init_ram(MachineState *machine)
>>      }
>>  }
>>  
>> +static void xen_create_pcie(XenArmState *xam)
>> +{
>> +    MemoryRegion *mmio_alias, *mmio_alias_high, *mmio_reg;
>> +    MemoryRegion *ecam_alias, *ecam_reg;
>> +    DeviceState *dev;
>> +    int i;
>> +
>> +    dev = qdev_new(TYPE_GPEX_HOST);
>> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>> +
>> +    /* Map ECAM space */
>> +    ecam_alias = g_new0(MemoryRegion, 1);
>> +    ecam_reg = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
>> +    memory_region_init_alias(ecam_alias, OBJECT(dev), "pcie-ecam",
>> +                             ecam_reg, 0, xam->pcie_ecam.size);
>> +    memory_region_add_subregion(get_system_memory(), xam->pcie_ecam.base,
>> +                                ecam_alias);
>> +
>> +    /* Map the MMIO space */
>> +    mmio_alias = g_new0(MemoryRegion, 1);
>> +    mmio_reg = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 1);
>> +    memory_region_init_alias(mmio_alias, OBJECT(dev), "pcie-mmio",
>> +                             mmio_reg,
>> +                             xam->pcie_mmio.base,
>> +                             xam->pcie_mmio.size);
>> +    memory_region_add_subregion(get_system_memory(), xam->pcie_mmio.base,
>> +                                mmio_alias);
>> +
>> +    /* Map the MMIO_HIGH space */
>> +    mmio_alias_high = g_new0(MemoryRegion, 1);
>> +    memory_region_init_alias(mmio_alias_high, OBJECT(dev), "pcie-mmio-high",
>> +                             mmio_reg,
>> +                             xam->pcie_mmio_high.base,
>> +                             xam->pcie_mmio_high.size);
>> +    memory_region_add_subregion(get_system_memory(), 
>> xam->pcie_mmio_high.base,
>> +                                mmio_alias_high);
>> +
>> +    /* Legacy PCI interrupts (#INTA - #INTD) */
>> +    for (i = 0; i < GPEX_NUM_IRQS; i++) {
>> +        qemu_irq irq = qemu_allocate_irq(xen_set_irq, NULL,
>> +                                         xam->pcie_irq + i);
>> +
>> +        sysbus_connect_irq(SYS_BUS_DEVICE(dev), i, irq);
>> +        gpex_set_irq_num(GPEX_HOST(dev), i, xam->pcie_irq + i);
>> +    }
>> +
>
>> +    DPRINTF("Created PCIe host bridge\n");
> replace DPRINTFs with trace_foo(), see 885f380f7be for example
>
>> +}
>> +
>> +static bool xen_read_pcie_prop(XenArmState *xam, unsigned int xen_domid,
>> +                               const char *prop_name, unsigned long *data)
>> +{
>> +    char path[128], *value = NULL;
>> +    unsigned int len;
>> +    bool ret = true;
>> +
>> +    snprintf(path, sizeof(path), "device-model/%d/pcie_host/%s",
>> +             xen_domid, prop_name);
>
> try to avoid usage of snprintf() in series
> see d2fe2264679 as an example
>

This whole function will be dropped in the next version.

>> +    value = xs_read(xam->state->xenstore, XBT_NULL, path, &len);
> maybe use g_autofree

I am not sure if this is a good idea, as xs_read() is provided by an
external library which uses plain old malloc().

>>      xen_create_virtio_mmio_devices(xam);
>> +    if (!xen_get_pcie_params(xam)) {
>> +        xen_create_pcie(xam);
>> +    } else {
>> +        DPRINTF("PCIe host bridge is not available,"
>> +                "only virtio-mmio can be used\n");
>
> so if something went wrong, it will be just ignored.
> Is it really expected behavior?
>

In the new version I reworked this section. Now we have 3 possible
outcomes:

1. No PCIe config was provided at all. This is fine, as user don't want
to use PCIe.

2. Full PCIe config was provided. We continue to creating the PCIe bridge.

3. Partial config was provided. This is an error and we exit.


-- 
WBR, Volodymyr