Linux-HyperV Archive on lore.kernel.org
 help / color / Atom feed
From: David Woodhouse <dwmw2@infradead.org>
To: Thomas Gleixner <tglx@linutronix.de>, x86@kernel.org
Cc: kvm <kvm@vger.kernel.org>, Paolo Bonzini <pbonzini@redhat.com>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	linux-hyperv@vger.kernel.org
Subject: Re: [PATCH v2 8/8] x86/ioapic: Generate RTE directly from parent irqchip's MSI message
Date: Fri, 23 Oct 2020 11:10:39 +0100
Message-ID: <C53CAD52-38F8-47D7-A5BE-4F470532EF20@infradead.org> (raw)
In-Reply-To: <87y2jy542v.fsf@nanos.tec.linutronix.de>



On 22 October 2020 22:43:52 BST, Thomas Gleixner <tglx@linutronix.de> wrote:
>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?

It makes the callers slightly more readable, not having to cast to uint32_t* from the struct.

I did ponder defining a new struct with bitfields named along the lines of 'msi_addr_bits_19_to_4', but that seemed like overkill.

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

It's got a four-line comment right there after it as we use the bits we got from it.

>> +	/*
>> +	 * 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.

Crap. Sure, want to look at the I/OAPIC and MSI documentation and observe that it's just shifting bits around and "these bits go there, those bits go here..." but there's no magic which will make that go away. At some point you end up swizzling bits around with seemingly random bitmasks and shifts which you have to have worked out from the docs.

Now that I killed off the various IOMMU bits which also horrifically touch the RTE directly, perhaps there is more scope for faffing around with it differently, but it won't really change much. It's just urinating into the atmospheric disturbance.


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

Well yes, there's a lot about the I/OAPIC code which is a bit horrid but this patch is doing just one thing: making the bits get from e.g. cfg->dest_apicid and cfg->vector into the RTE in a generic fashion which does the right thing for IR too. Other cleanups are somewhat orthogonal, but yes it's a good spot that one of these was somewhat redundant in the first place. We could fix that up in a separate patch which comes first perhaps.

If we're starting to clean up I/OAPIC code I'd like to take a long hard look at the level ack code paths, and the fact that we have separate ack_apic_irq and apic_ack_irq functions (or something like that; on phone now) which do different things. Perhaps the order (or the capitals on APIC which one of them has) makes it sane and meaningful for them both to exist and do different things?

I also note the Hyper-V "remapping" takes the IR code path and I'm not sure that's OK. Because of the complete lack of overall documentation on what it's all doing and why.

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

  parent reply index

Thread overview: 66+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <803bb6b2212e65c568c84ff6882c2aa8a0ee03d5.camel@infradead.org>
2020-10-09 10:46 ` [PATCH v2 0/8] Fix x2apic enablement and allow up to 32768 CPUs without IR where supported David Woodhouse
2020-10-09 10:46   ` [PATCH v2 1/8] x86/apic: Fix x2apic enablement without interrupt remapping David Woodhouse
2020-10-09 10:46   ` [PATCH v2 2/8] x86/msi: Only use high bits of MSI address for DMAR unit David Woodhouse
2020-10-09 10:46   ` [PATCH v2 3/8] x86/apic: Always provide irq_compose_msi_msg() method for vector domain David Woodhouse
2020-10-09 10:46   ` [PATCH v2 4/8] x86/ioapic: Handle Extended Destination ID field in RTE David Woodhouse
2020-10-09 10:46   ` [PATCH v2 5/8] x86/apic: Support 15 bits of APIC ID in MSI where available David Woodhouse
2020-10-09 10:46   ` [PATCH v2 6/8] x86/kvm: Add KVM_FEATURE_MSI_EXT_DEST_ID David Woodhouse
2020-10-09 10:46   ` [PATCH v2 7/8] x86/hpet: Move MSI support into hpet.c David Woodhouse
2020-10-09 10:46   ` [PATCH v2 8/8] x86/ioapic: Generate RTE directly from parent irqchip's MSI message David Woodhouse
2020-10-22 21:43     ` Thomas Gleixner
2020-10-22 22:10       ` Thomas Gleixner
2020-10-23 17:04         ` David Woodhouse
2020-10-23 10:10       ` David Woodhouse [this message]
2020-10-23 21:28         ` Thomas Gleixner
2020-10-24  8:26           ` David Woodhouse
2020-10-24  8:41             ` David Woodhouse
2020-10-24  9:13             ` Paolo Bonzini
2020-10-24 10:13               ` David Woodhouse
2020-10-24 12:44                 ` David Woodhouse
2020-10-24 21:35                   ` [PATCH v3 00/35] Fix x2apic enablement and allow more CPUs, clean up I/OAPIC and MSI bitfields David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 01/35] x86/apic: Fix x2apic enablement without interrupt remapping David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 02/35] x86/msi: Only use high bits of MSI address for DMAR unit David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 03/35] x86/apic/uv: Fix inconsistent destination mode David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 04/35] x86/devicetree: Fix the ioapic interrupt type table David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 05/35] x86/apic: Cleanup delivery mode defines David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 06/35] x86/apic: Replace pointless apic::dest_logical usage David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 07/35] x86/apic: Get rid of apic::dest_logical David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 08/35] x86/apic: Cleanup destination mode David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 09/35] x86/apic: Always provide irq_compose_msi_msg() method for vector domain David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 10/35] x86/hpet: Move MSI support into hpet.c David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 11/35] genirq/msi: Allow shadow declarations of msi_msg::$member David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 12/35] x86/msi: Provide msi message shadow structs David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 13/35] iommu/intel: Use msi_msg " David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 14/35] iommu/amd: " David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 15/35] PCI: vmd: " David Woodhouse
2020-10-28 20:49                       ` Kees Cook
2020-10-28 21:13                         ` Thomas Gleixner
2020-10-28 23:22                           ` Kees Cook
2020-10-24 21:35                     ` [PATCH v3 16/35] x86/kvm: " David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 17/35] x86/pci/xen: " David Woodhouse
2020-10-25  9:49                       ` David Laight
2020-10-25 10:26                         ` David Woodhouse
2020-10-25 13:20                           ` David Laight
2020-10-24 21:35                     ` [PATCH v3 18/35] x86/msi: Remove msidef.h David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 19/35] x86/io_apic: Cleanup trigger/polarity helpers David Woodhouse
2020-11-10  6:31                       ` Qian Cai
2020-11-10  8:59                         ` David Woodhouse
2020-11-10 16:26                           ` Paolo Bonzini
2020-10-24 21:35                     ` [PATCH v3 20/35] x86/ioapic: Cleanup IO/APIC route entry structs David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 21/35] x86/ioapic: Generate RTE directly from parent irqchip's MSI message David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 22/35] genirq/irqdomain: Implement get_name() method on irqchip fwnodes David Woodhouse
2020-10-25  9:41                       ` Marc Zyngier
2020-10-24 21:35                     ` [PATCH v3 23/35] x86/apic: Add select() method on vector irqdomain David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 24/35] iommu/amd: Implement select() method on remapping irqdomain David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 25/35] iommu/vt-d: " David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 26/35] iommu/hyper-v: " David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 27/35] x86/hpet: Use irq_find_matching_fwspec() to find " David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 28/35] x86/ioapic: " David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 29/35] x86: Kill all traces of irq_remapping_get_irq_domain() David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 30/35] iommu/vt-d: Simplify intel_irq_remapping_select() David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 31/35] x86/ioapic: Handle Extended Destination ID field in RTE David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 32/35] x86/apic: Support 15 bits of APIC ID in MSI where available David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 33/35] iommu/hyper-v: Disable IRQ pseudo-remapping if 15 bit APIC IDs are available David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 34/35] x86/kvm: Reserve KVM_FEATURE_MSI_EXT_DEST_ID David Woodhouse
2020-10-24 21:35                     ` [PATCH v3 35/35] x86/kvm: Enable 15-bit extension when KVM_FEATURE_MSI_EXT_DEST_ID detected David Woodhouse
2020-10-25  8:12                     ` [PATCH v3 00/35] Fix x2apic enablement and allow more CPUs, clean up I/OAPIC and MSI bitfields David Woodhouse

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=C53CAD52-38F8-47D7-A5BE-4F470532EF20@infradead.org \
    --to=dwmw2@infradead.org \
    --cc=kvm@vger.kernel.org \
    --cc=linux-hyperv@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=tglx@linutronix.de \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-HyperV Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hyperv/0 linux-hyperv/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-hyperv linux-hyperv/ https://lore.kernel.org/linux-hyperv \
		linux-hyperv@vger.kernel.org
	public-inbox-index linux-hyperv

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-hyperv


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git