linux-kernel - Re: Qemu Guest kernel 4.20-rc1 PCIe hotplug issue

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20181114133014.ge7cy2r2vrrtt6xx@wunner.de>
Date:   Wed, 14 Nov 2018 14:30:14 +0100
From:   Lukas Wunner <lukas@...ner.de>
To:     "mika.westerberg@...ux.intel.com" <mika.westerberg@...ux.intel.com>
Cc:     Shameerali Kolothum Thodi <shameerali.kolothum.thodi@...wei.com>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Wangzhou (B)" <wangzhou1@...ilicon.com>,
        Linuxarm <linuxarm@...wei.com>
Subject: Re: Qemu Guest kernel 4.20-rc1 PCIe hotplug issue

On Wed, Nov 14, 2018 at 11:52:25AM +0200, mika.westerberg@...ux.intel.com wrote:
> On Tue, Nov 13, 2018 at 03:57:47PM +0000, Shameerali Kolothum Thodi wrote:
> > > The smb_mb() thing is not that clear (at least to me) because it is used
> > > in two places in the driver and both seem to be making write to
> > > ctrl->cmd_busy visible to other CPUs but I don't see where we deal with
> > > the read part.
> > > 
> > > I may be missing something, though.
> > 
> > I think the read part is in wait_event_timeout() which evaluates the
> > condition. The wake_up is called from the pciehp_isr().  Since the flag
> > is being updated in both process level and interrupt handler context,
> > smp_mb() is used. I think the same now applies to  ctrl->slot_ctrl now
> > as this being used in process context and interrupt context as well.
> 
> Right, but that would require to use another read/general barrier in the
> pciehp_isr() before we read the variable in case interrupt happens
> immediately on another CPU (at least that's my understanding).

In pcie_do_write_cmd(), please just move the

	ctrl->slot_ctrl = slot_ctrl;

above the call to pcie_capability_write_word().

AFAICS an explicit memory barrier isn't needed here because of the call to
pcie_capability_write_word(), which "will [ordinarily] be guaranteed to be
fully ordered and uncombined" (Documentation/memory-barriers.txt, section
"KERNEL I/O BARRIER EFFECTS").

The memory barrier in pciehp_isr() is also bogus because the following
wake_up() implies a memory barrier if a task was woken.  (And if none
was woken, who cares.)

> Since I'm
> not too comfortable with all these barriers to be honest I would prefer
> reading the slot control register directly in pciehp_isr() :-)

That is an approach I'd strongly object to:  While pciehp itself only
signals very few interrupts (making an additional mmio read appear to
be negligible), it may share its interrupt with other devices.  On my
MacBookPro9,1, a hotplug port of the Thunderbolt controller shares
its interrupt line with the Wifi card and SD card reader, and those
may signal a huge number of interrupts.  On such a machine an additional
mmio read per interrupt becomes a problem.

Thanks,

Lukas