Re: [PATCH v1 3/6] xen/arm: create dom0less virtio-pci DT node

["Edgar E. Iglesias" <edgar.iglesias@xxxxxxx>]

On Tue, Sep 24, 2024 at 04:55:15PM -0700, Stefano Stabellini wrote:
> On Tue, 24 Sep 2024, Edgar E. Iglesias wrote:
> > From: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
> > 
> > When virtio-pci is specified in the dom0less domU properties, create a
> > virtio-pci node in the guest's device tree. Set up an mmio handler with
> > a register for the guest to poll when the backend has connected and
> > virtio-pci bus is ready to be probed. Grant tables may be used by
> > specifying virtio-pci = "grants";.
> > 
> > [Edgar: Use GPEX PCI INTX interrupt swizzling (from PCI specs).
> >  Make grants iommu-map cover the entire PCI bus.
> >  Add virtio-pci-ranges to specify memory-map for direct-mapped guests.
> >  Document virtio-pci dom0less fdt bindings.]
> > Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xxxxxxx>
> > ---
> >  docs/misc/arm/device-tree/booting.txt |  21 +++
> >  xen/arch/arm/dom0less-build.c         | 238 ++++++++++++++++++++++++++
> >  xen/arch/arm/include/asm/kernel.h     |  15 ++
> >  3 files changed, 274 insertions(+)
> > 
> > diff --git a/docs/misc/arm/device-tree/booting.txt 
> > b/docs/misc/arm/device-tree/booting.txt
> > index 3a04f5c57f..82f3bd7026 100644
> > --- a/docs/misc/arm/device-tree/booting.txt
> > +++ b/docs/misc/arm/device-tree/booting.txt
> > @@ -276,6 +276,27 @@ with the following properties:
> >      passed through. This option is the default if this property is missing
> >      and the user does not provide the device partial device tree for the 
> > domain.
> >  
> > +- virtio-pci
> > +
> > +    A string property specifying whether virtio-pci is enabled for the
> > +    domain and if grant table mappings should be used. If no value is set
> > +    this property is treated as a boolean and the same way as if set to
> > +    "enabled".
> > +    Possible property values are:
> > +
> > +    - "enabled"
> > +    Virtio-pci is enabled for the domain.
> > +
> > +    - "grants"
> > +    Virtio-pci is enabled for the domain and an grants IOMMU node will be
> > +    generated in the domains device-tree.
> > +
> > +- virtio-pci-ranges
> > +
> > +    An optional array of 6 u32 values specifying the 2 cells base 
> > addresses of
> > +    the ECAM, Memory and Prefetchable-Memory regions for virtio-pci. This 
> > is
> > +    useful to avoid memory-map collisions when using direct-mapped guests.
> > +
> >  Under the "xen,domain" compatible node, one or more sub-nodes are present
> >  for the DomU kernel and ramdisk.
> >  
> > diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> > index 09b65e44ae..dab24fa9e2 100644
> > --- a/xen/arch/arm/dom0less-build.c
> > +++ b/xen/arch/arm/dom0less-build.c
> > @@ -586,6 +586,189 @@ static int __init domain_handle_dtb_bootmodule(struct 
> > domain *d,
> >      return res;
> >  }
> >  
> > +static int __init make_virtio_pci_domU_node(const struct kernel_info 
> > *kinfo)
> > +{
> > +    void *fdt = kinfo->fdt;
> > +    /* reg is sized to be used for all the needed properties below */
> > +    __be32 reg[(1 + (GUEST_ROOT_ADDRESS_CELLS * 2) + GUEST_ROOT_SIZE_CELLS)
> > +               * 2];
> > +    __be32 irq_map[4 * 4 * 8];
> > +    __be32 *cells;
> > +    char buf[22]; /* pcie@ + max 16 char address + '\0' */
> > +    int res;
> > +    int devfn, intx_pin;
> > +    static const char compat[] = "pci-host-ecam-generic";
> > +    static const char reg_names[] = "ecam";
> > +
> > +    if ( p2m_ipa_bits <= 40 ) {
> > +        printk("PA bits %d is too small!\nvirtio-pci is only supported "
> > +               "on platforms with PA bits > 40\n", p2m_ipa_bits);
> > +        return -EINVAL;
> > +    }
> > +
> > +    snprintf(buf, sizeof(buf), "pcie@%lx", kinfo->virtio_pci.ecam.base);
> > +    dt_dprintk("Create virtio-pci node\n");
> > +    res = fdt_begin_node(fdt, buf);
> > +    if ( res )
> > +        return res;
> > +
> > +    res = fdt_property(fdt, "compatible", compat, sizeof(compat));
> > +    if ( res )
> > +        return res;
> > +
> > +    res = fdt_property_string(fdt, "device_type", "pci");
> > +    if ( res )
> > +        return res;
> > +
> > +    /* Create reg property */
> > +    cells = &reg[0];
> > +    dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, 
> > GUEST_ROOT_SIZE_CELLS,
> > +                       kinfo->virtio_pci.ecam.base, 
> > GUEST_VIRTIO_PCI_ECAM_SIZE);
> > +
> > +    res = fdt_property(fdt, "reg", reg,
> > +                       (GUEST_ROOT_ADDRESS_CELLS +
> > +                        GUEST_ROOT_SIZE_CELLS) * sizeof(*reg));
> > +    if ( res )
> > +        return res;
> > +
> > +    res = fdt_property(fdt, "reg-names", reg_names, sizeof(reg_names));
> > +    if ( res )
> > +        return res;
> > +
> > +    /* Create bus-range property */
> > +    cells = &reg[0];
> > +    dt_set_cell(&cells, 1, 0);
> > +    dt_set_cell(&cells, 1, 0xff);
> > +    res = fdt_property(fdt, "bus-range", reg, 2 * sizeof(*reg));
> > +    if ( res )
> > +        return res;
> > +
> > +    res = fdt_property_cell(fdt, "#address-cells", 3);
> > +    if ( res )
> > +        return res;
> > +
> > +    res = fdt_property_cell(fdt, "#size-cells", 2);
> > +    if ( res )
> > +        return res;
> > +
> > +    res = fdt_property_string(fdt, "status", "okay");
> > +    if ( res )
> > +        return res;
> > +
> > +    /*
> > +     * Create ranges property as:
> > +     * <(PCI bitfield) (PCI address) (CPU address) (Size)>
> > +     */
> > +    cells = &reg[0];
> > +    dt_set_cell(&cells, 1, GUEST_VIRTIO_PCI_MEM_TYPE);
> > +    dt_set_cell(&cells, GUEST_ROOT_ADDRESS_CELLS, 
> > kinfo->virtio_pci.mem.base);
> > +    dt_set_cell(&cells, GUEST_ROOT_ADDRESS_CELLS, 
> > kinfo->virtio_pci.mem.base);
> > +    dt_set_cell(&cells, GUEST_ROOT_SIZE_CELLS, GUEST_VIRTIO_PCI_MEM_SIZE);
> > +    dt_set_cell(&cells, 1, GUEST_VIRTIO_PCI_PREFETCH_MEM_TYPE);
> > +    dt_set_cell(&cells, GUEST_ROOT_ADDRESS_CELLS, 
> > kinfo->virtio_pci.pf_mem.base);
> > +    dt_set_cell(&cells, GUEST_ROOT_ADDRESS_CELLS, 
> > kinfo->virtio_pci.pf_mem.base);
> > +    dt_set_cell(&cells, GUEST_ROOT_SIZE_CELLS, 
> > GUEST_VIRTIO_PCI_PREFETCH_MEM_SIZE);
> > +    res = fdt_property(fdt, "ranges", reg, 14 * sizeof(*reg));
> > +    if ( res )
> > +        return res;
> > +
> > +    res = fdt_property(fdt, "dma-coherent", "", 0);
> > +    if ( res )
> > +        return res;
> > +
> > +    res = fdt_property_cell(fdt, "#interrupt-cells", 1);
> > +    if ( res )
> > +        return res;
> > +
> > +    /*
> > +     * PCI-to-PCI bridge specification
> > +     * 9.1: Interrupt routing. Table 9-1
> > +     *
> > +     * the PCI Express Base Specification, Revision 2.1
> > +     * 2.2.8.1: INTx interrupt signaling - Rules
> > +     *          the Implementation Note
> > +     *          Table 2-20
> > +     */
> > +    cells = &irq_map[0];
> > +    for (devfn = 0; devfn <= 0x18; devfn += 8) {
> > +        for (intx_pin = 0; intx_pin < 4; intx_pin++) {
> > +            int irq = GUEST_VIRTIO_PCI_SPI_FIRST - 32;
> > +            irq += ((intx_pin + PCI_SLOT(devfn)) % 4);
> > +
> > +            dt_set_cell(&cells, 1, devfn << 8);
> > +            dt_set_cell(&cells, 1, 0);
> > +            dt_set_cell(&cells, 1, 0);
> > +            dt_set_cell(&cells, 1, intx_pin + 1);
> > +            dt_set_cell(&cells, 1, kinfo->phandle_gic);
> > +            /* 3 GIC cells.  */
> > +            dt_set_cell(&cells, 1, 0);
> > +            dt_set_cell(&cells, 1, irq);
> > +            dt_set_cell(&cells, 1, DT_IRQ_TYPE_LEVEL_HIGH);
> > +        }
> > +    }
> > +
> > +    /* Assert we've sized irq_map correctly.  */
> > +    BUG_ON(cells - &irq_map[0] != ARRAY_SIZE(irq_map));
> > +
> > +    res = fdt_property(fdt, "interrupt-map", irq_map, sizeof(irq_map));
> > +    if ( res )
> > +        return res;
> > +
> > +    cells = &reg[0];
> > +    dt_set_cell(&cells, 1, cpu_to_be16(PCI_DEVFN(3, 0)));
> 
> Hi Edgar, what is this PCI_DEVFN(3, 0)  device?


Hi Stefano,

This is for the interrupt-map-mask, since the swizzling pattern
repeats itself for every fourth device, we only need to describe
entries 0 - 3 and can mask out the rest.

I the next version (which will be quite different) I'll try to make
this clearer.


> 
> 
> > +    dt_set_cell(&cells, 1, 0x0);
> > +    dt_set_cell(&cells, 1, 0x0);
> > +    dt_set_cell(&cells, 1, 0x7);
> > +    res = fdt_property(fdt, "interrupt-map-mask", reg, 4 * sizeof(*reg));
> > +    if ( res )
> > +        return res;
> > +
> > +    if ( kinfo->virtio_pci.mode == VIRTIO_PCI_GRANTS )
> > +    {
> > +        cells = &reg[0];
> > +        dt_set_cell(&cells, 1, 0x0);
> > +        dt_set_cell(&cells, 1, GUEST_PHANDLE_IOMMU);
> > +        dt_set_cell(&cells, 1, 0x0);
> > +        dt_set_cell(&cells, 1, 0x10000);
> > +        res = fdt_property(fdt, "iommu-map", reg, 4 * sizeof(*reg));
> > +        if ( res )
> > +            return res;
> > +    }
> > +
> > +    res = fdt_property_cell(fdt, "linux,pci-domain", 1);
> > +    if ( res )
> > +        return res;
> > +
> > +    res = fdt_end_node(fdt);
> > +    if ( res )
> > +        return res;
> > +
> > +    if ( kinfo->virtio_pci.mode == VIRTIO_PCI_GRANTS )
> > +    {
> > +        snprintf(buf, sizeof(buf), "xen_iommu");
> 
> I don't think underscores are allowed in device tree node names


Will fix, thanks.


> 
> 
> 
> > +        res = fdt_begin_node(fdt, buf);
> > +        if ( res )
> > +            return res;
> > +
> > +        res = fdt_property_string(fdt, "compatible", "xen,grant-dma");
> > +        if ( res )
> > +            return res;
> > +
> > +        res = fdt_property_cell(fdt, "#iommu-cells", 1);
> > +        if ( res )
> > +            return res;
> > +
> > +        res = fdt_property_cell(fdt, "phandle", GUEST_PHANDLE_IOMMU);
> > +        if ( res )
> > +            return res;
> > +
> > +        res = fdt_end_node(fdt);
> > +    }
> > +
> > +    return res;
> > +}
> > +
> >  /*
> >   * The max size for DT is 2MB. However, the generated DT is small (not 
> > including
> >   * domU passthrough DT nodes whose size we account separately), 4KB are 
> > enough
> > @@ -693,6 +876,13 @@ static int __init prepare_dtb_domU(struct domain *d, 
> > struct kernel_info *kinfo)
> >              goto err;
> >      }
> >  
> > +    if ( kinfo->virtio_pci.mode )
> > +    {
> > +        ret = make_virtio_pci_domU_node(kinfo);
> > +        if ( ret )
> > +            goto err;
> > +    }
> > +
> >      ret = fdt_end_node(kinfo->fdt);
> >      if ( ret < 0 )
> >          goto err;
> > @@ -744,11 +934,24 @@ static int __init alloc_xenstore_evtchn(struct domain 
> > *d)
> >      return 0;
> >  }
> >  
> > +static u64 combine_u64(u32 v[2])
> 
> Let's make this a static inline or a macro. I can't believe we don't
> have one already.

Yes, I'll try what Stewart suggested.

Cheers,
Edgar