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

[David Gibson]

On Mon, Jul 30, 2018 at 11:05:58AM +0200, Cédric Le Goater wrote:
> On 07/27/2018 05:56 AM, David Gibson wrote:
> > On Thu, Jul 26, 2018 at 03:37:21PM +0200, Cédric Le Goater wrote:
> >> This proposal introduces a new IRQ number space layout using static
> >> numbers for all devices, depending on a device index, and a bitmap
> >> allocator for the MSI IRQ numbers which are negotiated by the guest at
> >> runtime.
> >>
> >> As the VIO device model does not have a device index but a "reg"
> >> property, we introduce a formula to compute an IRQ number from a "reg"
> >> value. It should minimize most of the collisions.
> >>
> >> The previous layout is kept in pre-3.1 machines raising the
> >> 'legacy_irq_allocation' machine class flag.
> >>
> >> Signed-off-by: Cédric Le Goater <address@hidden>
> > 
> > One nit left..
> > 
> > [snip]
> >> +static inline uint32_t spapr_vio_reg_to_irq(uint32_t reg)
> >> +{
> >> +    uint32_t irq;
> >> +
> >> +    if (reg >= SPAPR_VIO_REG_BASE) {
> >> +        /*
> >> +         * VIO device register values when allocated by QEMU. For
> >> +         * these, we simply mask the high bits to fit the overall
> >> +         * range: [0x00 - 0xff].
> >> +         *
> >> +         * The nvram VIO device (reg=0x71000000) is a static device of
> >> +         * the pseries machine and so is always allocated by QEMU. Its
> >> +         * IRQ number is 0x0.
> >> +         */
> >> +        irq = reg & 0xff;
> >> +
> >> +    } else if (reg >= 0x30000000) {
> >> +        /*
> >> +         * VIO tty devices register values, when allocated by livirt,
> >> +         * are mapped in range [0xf0 - 0xff], gives us a maximum of 16
> >> +         * vtys.
> >> +         */
> >> +        irq = 0xf0 | ((reg >> 12) & 0xf);
> >> +
> >> +    } else {
> >> +        /*
> >> +         * Other VIO devices register values, when allocated by
> >> +         * livirt, are mapped in range [0x00 - 0xef].
> >> +         */
> >> +        irq = (reg >> 12) & 0xef;
> > 
> > This mask doesn't do what you intend - it will map 0x10 to 0, for
> > example.  You could use % 0xf0, but actually you might as well just
> > use & 0xff.  Yes, it could collide with the vty devices, but either
> > way you can still have collisions if you try hard enough.  And, either
> > way, they'll get detected later.
> > 
> 
> 
> David,
> 
> Shall I resend a v6 with this fix or should I wait for the patch adding 
> 3.1. I could also send a 3.1 pseries machine also if you prefer.

We'll need a pseries-3.1 at some point, so if you can send that, it
would be great.  Then I can stage the lot in ppc-for-3.1.  It might
need some tweaking before final merge, but that shouldn't be too hard.

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

signature.asc
Description: PGP signature