Re: [PATCH v2 8/8] x86/ioapic: Generate RTE directly from parent irqchip's MSI message

[Thomas Gleixner <tglx@linutronix.de>]

On Fri, Oct 09 2020 at 11:46, David Woodhouse wrote:

@@ -45,12 +45,11 @@ enum irq_alloc_type {
 };

> +static void mp_swizzle_msi_dest_bits(struct irq_data *irq_data, void *_entry)
> +{
> +	struct msi_msg msg;
> +	u32 *entry = _entry;

Why is this a void * argument and then converting it to a u32 *? Just to
make that function completely unreadable?

> +
> +	irq_chip_compose_msi_msg(irq_data, &msg);

Lacks a comment. Also mp_swizzle... is a misnomer as this invokes the
msi compose function which is not what the function name suggests.

> +	/*
> +	 * They're in a bit of a random order for historical reasons, but
> +	 * the IO/APIC is just a device for turning interrupt lines into
> +	 * MSIs, and various bits of the MSI addr/data are just swizzled
> +	 * into/from the bits of Redirection Table Entry.
> +	 */
> +	entry[0] &= 0xfffff000;
> +	entry[0] |= (msg.data & (MSI_DATA_DELIVERY_MODE_MASK |
> +				 MSI_DATA_VECTOR_MASK));
> +	entry[0] |= (msg.address_lo & MSI_ADDR_DEST_MODE_MASK) << 9;
> +
> +	entry[1] &= 0xffff;
> +	entry[1] |= (msg.address_lo & MSI_ADDR_DEST_ID_MASK) << 12;

Sorry, but this is unreviewable gunk. The whole msi_msg setup sucks with
this unholy macro maze. I have a half finished series which allows
architectures to provide shadow members for data, address_* so this can
be done proper with bitfields.

Aside of that it works magically because polarity,trigger and mask bit
have been set up before. But of course a comment about this is
completely overrated.

Thanks,

        tglx