Re: [Qemu-devel] [PATCH v4 1/3] spapr: introduce a fixed IRQ number space

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6053 bytes --]

On Fri, Jul 06, 2018 at 03:36:24PM +0200, Greg Kurz wrote:
> On Fri,  6 Jul 2018 11:07:11 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
[snip]
> > +/*
> > + * The register property of a VIO device is defined in livirt using a
> > + * base number + 0x1000 increment and in QEMU by incrementing the base
> > + * register number 0x71000000.
> > + *
> > + * The formula below tries to compute a unique index number from the
> > + * register value that will be used to define the IRQ number of the
> > + * VIO device. A maximum of 256 (0x100) VIO devices is covered.
> > + *
> > + * To minimize collisions, we define two distinct ranges depending on
> > + * the "reg" value definition:
> > + *
> > + *     [0x00 - 0x7f]    user/libvirt
> > + *     [0x80 - 0xff]    QEMU VIO model
> > + *
> > + * Collisions will be detected when the IRQ is claimed.
> > + */
> > +static inline uint32_t spapr_vio_reg_to_irq(uint32_t reg)
> > +{
> > +    if (reg >= SPAPR_VIO_REG_BASE) {
> > +        return SPAPR_IRQ_VIO | (reg & 0x7f) | 0x80;
> > +    } else {
> > +        return SPAPR_IRQ_VIO | ((reg & 0x7f) ^ ((reg >> 12) & 0x7f));
> 
> Hmm... if you put a VIO net and two VIO vty in the domain XML, libvirt
> will generate reg == 0x1000 for the VIO net and reg == 0x30001000 for the
> second VIO vty... this will necessarily collide, won't it ?

Ah, drat, yeah, this will still need some tweaking.

> With a 256 VIO devices limit, libvirt can only add 255 devices since
> the nvram is created by QEMU by default (libvirt can only change its
> reg using -global).

Note that the 256 limit is basically arbitrary.  I don't think it
exists in the code now, but it should be way more than sufficient.

> As David mentioned in another mail:
> 
> 	VIO net devices start at reg=0x1000
> 	VIO scsi devices start at reg=0x2000
> 	VIO nvram devices start at reg=0x3000
> 	VIO vty devices start at reg=0x30000000
> 	    and increment by 0x1000 each type
> 
> 
> The values for net, scsi and nvram overlap... which makes me wonder why
> do we even care to have per-type base value !?!

We don't, really, I'm not sure why it ended up like that.

> Anyway, I don't think
> it's important for what you're trying to achieve.
> 
> Basically libvirt can generate regs in two distinct ranges:
> 
> - one for scsi/net/nvram:
> 
> smallest possible reg: 0x1000
> largest possible reg: 0x2000 + 254 * 0x1000 = 0x100000
> 
> ie, 254 scsi devices starting at 0x2000 and 1 nvram
> 
> - one for vty 
> 
> smallest possible reg: 0x30000000
> largest possible reg: 0x30000000 + 253 * 0x1000 = 0x300fd000
> 
> ie, 254 vty devices
> 
> Thinking about the bit shifting magic that is needed to convert
> reg into a usable index makes my brain hurt, but I'll happily
> review anything you propose :)

Considering just the libvirt assigned addresses how about:

	if (reg >= 0x30000000)
		irq = VIO_BASE + 240 + (reg >> 12); // up to 16 vtys
	else
		irq = VIO_BASE + (reg >> 12);       // up to 238 others

(+ some masking to enforce the limits).

I think it's ok to overlap the "qemu" and "libvirt" ranges, since
(with the exception of the nvram device) I don't see any natural way
you'd get both schemes in use at once.  If the user overrides things,
either on the qemu command line or in the libvirt XML to combine the
two schemes, then we're back to "fix your own mess by manually
allocating irqs as well".

Note that the above formula doesn't use VIO_BASE+0 itself, so I think
we can then factor in the qemu assigned devices with just a:
	irq ^= (reg & 0xff);

> 
> > +    }
> > +}
> > +
> >  static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
> >  {
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > @@ -485,10 +512,18 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
> >      }
> >  
> >      if (!dev->irq) {
> > -        dev->irq = spapr_irq_findone(spapr, &local_err);
> > -        if (local_err) {
> > -            error_propagate(errp, local_err);
> > -            return;
> > +        if (SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
> > +            dev->irq = spapr_irq_findone(spapr, &local_err);
> > +            if (local_err) {
> > +                error_propagate(errp, local_err);
> > +                return;
> > +            }
> > +        } else {
> > +            dev->irq = spapr_vio_reg_to_irq(dev->reg);
> > +            if (dev->irq == SPAPR_IRQ_PCI_LSI) {
> > +                error_setg(errp, "Too many VIO devices");
> > +                return;
> > +            }
> >          }
> >      }
> >  
> > @@ -557,7 +592,7 @@ VIOsPAPRBus *spapr_vio_bus_init(void)
> >      /* Create bus on bridge device */
> >      qbus = qbus_create(TYPE_SPAPR_VIO_BUS, dev, "spapr-vio");
> >      bus = SPAPR_VIO_BUS(qbus);
> > -    bus->next_reg = 0x71000000;
> > +    bus->next_reg = SPAPR_VIO_REG_BASE;
> >  
> >      /* hcall-vio */
> >      spapr_register_hypercall(H_VIO_SIGNAL, h_vio_signal);
> > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> > index bcab6323b7ed..4ab556467289 100644
> > --- a/hw/ppc/Makefile.objs
> > +++ b/hw/ppc/Makefile.objs
> > @@ -4,7 +4,7 @@ obj-y += ppc.o ppc_booke.o fdt.o
> >  obj-$(CONFIG_PSERIES) += spapr.o spapr_caps.o spapr_vio.o spapr_events.o
> >  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> >  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> > -obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o
> > +obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o spapr_irq.o
> >  # IBM PowerNV
> >  obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.o pnv_core.o pnv_lpc.o pnv_psi.o pnv_occ.o pnv_bmc.o
> >  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]