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

[Cédric Le Goater]

On 07/20/2018 04:38 AM, David Gibson wrote:
> 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 <address@hidden> 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);

And what about the proposal below ? May be the QEMU/libvirt split could 
be rebalanced to favor one or the other. 

Thanks,

C.


+#define SPAPR_VIO_REG_BASE 0x71000000
+
+/*
+ * 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
+ *
+ * For the VIO devices with a user defined "reg", we extract bits
+ * [29-28] and [16-12] to compose an index on 7 bits. It should fit
+ * all libvirt usage.
+ *
+ * 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 | 0x80 | (reg & 0x7f);
+    } else {
+        return SPAPR_IRQ_VIO | (((reg >> 28) & 0x3) << 5) |
+            ((reg >> 12) & 0x1f);
+    }
+}
+