linux-kernel - Re: [patch 2/8] PCI/MSI: Mask all unused MSI-X entries

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87zgufnuks.ffs@nanos.tec.linutronix.de>
Date:   Thu, 22 Jul 2021 00:57:55 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     "Raj\, Ashok" <ashok.raj@...el.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Alex Williamson <alex.williamson@...hat.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>, linux-pci@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>,
        Kevin Tian <kevin.tian@...el.com>,
        Marc Zyngier <maz@...nel.org>, Ingo Molnar <mingo@...nel.org>,
        x86@...nel.org, Ashok Raj <ashok.raj@...el.com>
Subject: Re: [patch 2/8] PCI/MSI: Mask all unused MSI-X entries

Ashok,

On Wed, Jul 21 2021 at 15:23, Ashok Raj wrote:
> On Wed, Jul 21, 2021 at 09:11:28PM +0200, Thomas Gleixner wrote:
>>  
>> +		addr = pci_msix_desc_addr(entry);
>> +		if (addr)
>> +			entry->masked = readl(addr + PCI_MSIX_ENTRY_VECTOR_CTRL);
>
> Silly question:
> Do we have to read what the HW has to set this entry->masked? Shouldn't
> this be all masked before we start the setup?

msix_mask_all() is invoked before the msi descriptors are
allocated. msi_desc::masked is actually a misnomer because it's not like
the name suggests a boolean representing the masked state. It's caching
the content of the PCI_MSIX_ENTRY_VECTOR_CTRL part of the corresponding
table entry. Right now this is just using bit 0 (the mask bit), but is
that true forever? So we actually should rename that member to
vector_ctrl or such.

>> +static void msix_mask_all(void __iomem *base, int tsize)
>> +{
>> +	u32 ctrl = PCI_MSIX_ENTRY_CTRL_MASKBIT;
>> +	int i;
>>  
>> -		msix_mask_irq(entry, 1);
>> -	}
>> +	for (i = 0; i < tsize; i++, base += PCI_MSIX_ENTRY_SIZE)
>> +		writel(ctrl, base + PCI_MSIX_ENTRY_VECTOR_CTRL);
>
> shouldn't we initialize entry->masked here?

How so?

>> +	/* Ensure that all table entries are masked. */
>> +	msix_mask_all(base, tsize);
>> +
>>  	ret = msix_setup_entries(dev, base, entries, nvec, affd);

It's invoked before the msi descriptors are set up. We can of course
setup the descriptors first and then do the masking, but notice that
@nvec is not necessarily the same as the table size.

Thanks,

        tglx