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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Thu, May 19, 2022 at 08:16:16PM +0300, Oleksandr wrote:
> On 18.05.22 14:05, Anthony PERARD wrote:
> > On Tue, May 03, 2022 at 08:26:03PM +0300, Oleksandr Tyshchenko wrote:
> > > +    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.

I'm asking because the calculation doesn't really make sense to me yet. At the
moment "virtio_irq-32+1" happen to be the "number of disk + 1" and we
have "nr_spis += " which I don't think makes sense with the "+1".

Doesn't "nr_spis" only need to be the highest irq value for the devices
we're adding? (Maybe with +1) (also -32 because I think I understand
what 32 stand for now) (also, the "num_irqs" loop just after this loop
seems to do exactly that)

But I think what this line of code needs the most is a comment.

> > 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).

Do you think it could be possible to name that value? I've seen other
use of 32 in the same function that have probably the same meaning. But
if you don't have a good name, I guess we can also live a bit longer
with a plain "32".

Cheers,

-- 
Anthony PERARD