Re: [PATCH V3 3/6] libxl: arm: Create alloc_virtio_mmio_params()

[Viresh Kumar <viresh.kumar@xxxxxxxxxx>]

On 08-08-22, 21:39, Oleksandr wrote:
> 
> On 04.08.22 10:01, Viresh Kumar wrote:
> 
> Hello Viresh
> 
> 
> > Create a separate routine to allocate base and irq for a device as the
> > same code will be required for each device type.
> > 
> > Suggested-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> > ---
> >   tools/libs/light/libxl_arm.c | 38 ++++++++++++++++++++++++------------
> >   1 file changed, 25 insertions(+), 13 deletions(-)
> > 
> > diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> > index 1a3ac1646e94..2f64b9f0ebee 100644
> > --- a/tools/libs/light/libxl_arm.c
> > +++ b/tools/libs/light/libxl_arm.c
> > @@ -48,6 +48,24 @@ static uint32_t alloc_virtio_mmio_irq(libxl__gc *gc, 
> > uint32_t *virtio_mmio_irq)
> >       return irq;
> >   }
> > +static int alloc_virtio_mmio_params(libxl__gc *gc, uint64_t *base,
> > +                                    uint32_t *irq, uint64_t 
> > *virtio_mmio_base,
> > +                                    uint32_t *virtio_mmio_irq)
> > +{
> > +    *base = alloc_virtio_mmio_base(gc, virtio_mmio_base);
> > +    if (!*base)
> > +        return ERROR_FAIL;
> > +
> > +    *irq = alloc_virtio_mmio_irq(gc, virtio_mmio_irq);
> > +    if (!*irq)
> > +        return ERROR_FAIL;
> > +
> > +    LOG(DEBUG, "Allocate Virtio MMIO params: IRQ %u BASE 0x%"PRIx64, *irq,
> > +        *base);
> > +
> > +    return 0;
> > +}
> > +
> >   static const char *gicv_to_string(libxl_gic_version gic_version)
> >   {
> >       switch (gic_version) {
> > @@ -85,20 +103,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> >           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;
> > +            int rc = alloc_virtio_mmio_params(gc, &disk->base, &disk->irq,
> > +                    &virtio_mmio_base, &virtio_mmio_irq);
> > +            if (rc)
> > +                return rc;
> > -            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;
> 
>    I think that "virtio_enabled = true;" should be moved ...
> 
> 
> > -
> > -            LOG(DEBUG, "Allocate Virtio MMIO params for Vdev %s: IRQ %u 
> > BASE 0x%"PRIx64,
> > -                disk->vdev, disk->irq, disk->base);
> >           }
> >       }
> > @@ -107,8 +117,10 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> >        * present, make sure that we allocate enough SPIs for them.
> >        * The resulting "nr_spis" needs to cover the highest possible SPI.
> >        */
> > -    if (virtio_enabled)
> > +    if (virtio_mmio_irq != GUEST_VIRTIO_MMIO_SPI_FIRST) {
> > +        virtio_irq = virtio_mmio_irq - 1;
> >           nr_spis = max(nr_spis, virtio_irq - 32 + 1);
> 
>  ... here. Otherwise after applying all patches "virtio_enabled" only gets
> set if there is a virtio disk device. So it won't be set for virtio i2c,
> etc.
> 
> I see that in V2 it was there, but in V3 is not. Or maybe there is some
> reason for doing that which I am not aware of?

There is only one use of virtio_enabled after this patch, i.e. do
check for vpl011. Maybe we can drop the variable and use
virtio_mmio_irq != GUEST_VIRTIO_MMIO_SPI_FIRST ?

-- 
viresh