Re: [PATCH V8 2/2] libxl: Introduce basic virtio-mmio support on Arm

[Oleksandr <olekstysh@xxxxxxxxx>]

On 18.05.22 14:05, Anthony PERARD wrote:

Hello Anthony


> On Tue, May 03, 2022 at 08:26:03PM +0300, Oleksandr Tyshchenko wrote:
> > From: Julien Grall <julien.grall@xxxxxxx>
> >
> > This patch introduces helpers to allocate Virtio MMIO params
> > (IRQ and memory region) and create specific device node in
> > the Guest device-tree with allocated params. In order to deal
> > with multiple Virtio devices, reserve corresponding ranges.
> > For now, we reserve 1MB for memory regions and 10 SPIs.
> >
> > As these helpers should be used for every Virtio device attached
> > to the Guest, call them for Virtio disk(s).
> >
> > Please note, with statically allocated Virtio IRQs there is
> > a risk of a clash with a physical IRQs of passthrough devices.
> > For the first version, it's fine, but we should consider allocating
> > the Virtio IRQs automatically. Thankfully, we know in advance which
> > IRQs will be used for passthrough to be able to choose non-clashed
> > ones.
> >
> > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > ---
> > diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> > index eef1de0..37403a2 100644
> > --- a/tools/libs/light/libxl_arm.c
> > +++ b/tools/libs/light/libxl_arm.c
> > @@ -8,6 +8,46 @@
> >   #include <assert.h>
> >   #include <xen/device_tree_defs.h>
> >   
> > +/*
> > + * There is no clear requirements for the total size of Virtio MMIO region.
> > + * The size of control registers is 0x100 and device-specific configuration
> > + * registers starts at the offset 0x100, however it's size depends on the 
> > device
> > + * and the driver. Pick the biggest known size at the moment to cover most
> > + * of the devices (also consider allowing the user to configure the size via
> > + * config file for the one not conforming with the proposed value).
> > + */
> > +#define VIRTIO_MMIO_DEV_SIZE   xen_mk_ullong(0x200)
> > +
> > +static uint64_t alloc_virtio_mmio_base(libxl__gc *gc, uint64_t 
> > *virtio_mmio_base)
> > +{
> > +    uint64_t base = *virtio_mmio_base;
> > +
> > +    /* Make sure we have enough reserved resources */
> > +    if ((base + VIRTIO_MMIO_DEV_SIZE >
> > +        GUEST_VIRTIO_MMIO_BASE + GUEST_VIRTIO_MMIO_SIZE)) {
> Could you remove the second set of parentheses? I'd like the compiler to
> warn us if there's an assignment.
ok


> > @@ -26,8 +66,10 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> >   {
> >       uint32_t nr_spis = 0;
> >       unsigned int i;
> > -    uint32_t vuart_irq;
> > -    bool vuart_enabled = false;
> > +    uint32_t vuart_irq, virtio_irq = 0;
> > +    bool vuart_enabled = false, virtio_enabled = false;
> > +    uint64_t virtio_mmio_base = GUEST_VIRTIO_MMIO_BASE;
> > +    uint32_t virtio_mmio_irq = GUEST_VIRTIO_MMIO_SPI_FIRST;
> >   
> >       /*
> >        * If pl011 vuart is enabled then increment the nr_spis to allow 
> > allocation
> > @@ -39,6 +81,30 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> >           vuart_enabled = true;
> >       }
> >   
> > +    for (i = 0; i < d_config->num_disks; i++) {
> > +        libxl_device_disk *disk = &d_config->disks[i];
> > +
> > +        if (disk->specification == LIBXL_DISK_SPECIFICATION_VIRTIO) {
> > +            disk->base = alloc_virtio_mmio_base(gc, &virtio_mmio_base);
> > +            if (!disk->base)
> > +                return ERROR_FAIL;
> > +
> > +            disk->irq = alloc_virtio_mmio_irq(gc, &virtio_mmio_irq);
> > +            if (!disk->irq)
> > +                return ERROR_FAIL;
> > +
> > +            if (virtio_irq < disk->irq)
> > +                virtio_irq = disk->irq;
> > +            virtio_enabled = true;
> > +
> > +            LOG(DEBUG, "Allocate Virtio MMIO params for Vdev %s: IRQ %u BASE 
> > 0x%"PRIx64,
> > +                disk->vdev, disk->irq, disk->base);
> > +        }
> > +    }
> > +
> > +    if (virtio_enabled)
> > +        nr_spis += (virtio_irq - 32) + 1;
> Is it possible to update "nr_spis" inside the loop?
yes, but ...


>   The added value
> seems to be "number of virtio device + 1",
   ... not really ...


>   so updating "nr_spis" and
> adding +1 after the loop could work, right?
   ... from my understanding, we cannot just increment nr_spis by "one" 
inside a loop, we need to calculate it.

Something like that (not tested):

       uint32_t spi;

       ...

       spi = irq - 32;
       if (nr_spis <= spi)
           nr_spis = spi + 1;


Shall I update "nr_spis" inside the loop?

Are you asking because of eliminating "virtio_enabled" and/or 
"virtio_irq" locals? They are used down the code.

> Also, what is "32"? Is it "GUEST_VIRTIO_MMIO_SPI_FIRST - 1" ?

Although currently "GUEST_VIRTIO_MMIO_SPI_FIRST - 1" = "32", we cannot 
rely on this (I mean to use "GUEST_VIRTIO_MMIO_SPI_FIRST - 1"
instead of "32"),  because "32" has yet another meaning. This is an 
offset for SPI, the values before 32 are for private IRQs (PPI, SGI).


> > +
> >       for (i = 0; i < d_config->b_info.num_irqs; i++) {
> >           uint32_t irq = d_config->b_info.irqs[i];
> >           uint32_t spi;
> > @@ -787,6 +860,39 @@ static int make_vpci_node(libxl__gc *gc, void *fdt,
> >       return 0;
> >   }
> >   
> > +
> > +static int make_virtio_mmio_node(libxl__gc *gc, void *fdt,
> > +                                 uint64_t base, uint32_t irq)
> > +{
> > +    int res;
> > +    gic_interrupt intr;
> > +    /* Placeholder for virtio@ + a 64-bit number + \0 */
> > +    char buf[24];
> > +
> > +    snprintf(buf, sizeof(buf), "virtio@%"PRIx64, base);
> Could you use GCSPRINTF() here instead of using a buffer of a static
> size calculated by hand which is potentially wrong? Also, the return
> value of snprintf isn't checked so the string could be truncated without
> warning. So I think GCSPRINTF is better than a static buffer.
I got it, thank you for detailed explanation, will use.


> The rest of the patch looks fine.
Thank you.


> Thanks,
--
Regards,

Oleksandr Tyshchenko