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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 20.05.22 18:22, Anthony PERARD wrote:

Hello Anthony

> 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".
I see


> 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)
I also think the same, the "nr_spis" needs to cover the highest SPI.


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


> > > 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.
yes, all uses of "32" within current function have 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".
Except here in toolstack, this plain "32" is used in a few places in 
hypervisor code also.
I don't have a plan to convert this value into appropriate #define 
everywhere.
But, I can introduce local to this file #define NR_LOCAL_IRQS 32 and 
change in current function.
Shall I?


> Cheers,
--
Regards,

Oleksandr Tyshchenko