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

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

On Wed, Sep 25, 2024 at 08:44:41AM +0100, Julien Grall wrote:
> Hi,
> 

Hi Julien,


> On 24/09/2024 17:23, 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
> 
> Similar question to the other patches, why is this specific to virtio PCI?
> QEMU (or another device module) is free to emulate whatever it wants behind
> the PCI hosbtridge.

There's no hard limitatino to only virtio-pci devices it's more of a
recommendation that PVH guests should not use "emulated" devices but
there's nothing stopping it.

Perhaps the names of these properties are missleading, I'm happy to
change them if there are better suggestions!

I can also clarify it in the documentation.


> 
> > +
> > +    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;
> > +    }
> 
> Please add a comment explaining where does this requires come from. If this
> is the Address layout, then probably be to avoid relying on hardcoded number
> of bits.
>

Oops, sorry, I had actually removed this part from my git branch but I
forgot to regenerated the patch series.

I'll remove it.

Best regards,
Edgar