Message-ID: <20220427164147.330a0bc8.alex.williamson@redhat.com>
Date:   Wed, 27 Apr 2022 16:41:47 -0600
From:   Alex Williamson <alex.williamson@...hat.com>
To:     Jake Oshins <jakeo@...rosoft.com>
Cc:     Dexuan Cui <decui@...rosoft.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Bjorn Helgaas <helgaas@...nel.org>,
        "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        "wei.liu@...nel.org" <wei.liu@...nel.org>,
        KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Michael Kelley (LINUX)" <mikelley@...rosoft.com>,
        "robh@...nel.org" <robh@...nel.org>, "kw@...ux.com" <kw@...ux.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>
Subject: Re: [PATCH] PCI: hv: Do not set PCI_COMMAND_MEMORY to reduce VM
 boot time

On Tue, 26 Apr 2022 19:25:43 +0000
Jake Oshins <jakeo@...rosoft.com> wrote:

> > -----Original Message-----
> > From: Dexuan Cui <decui@...rosoft.com>
> > Sent: Tuesday, April 26, 2022 11:32 AM
> > To: Lorenzo Pieralisi <lorenzo.pieralisi@....com>
> > Cc: Jake Oshins <jakeo@...rosoft.com>; Bjorn Helgaas <helgaas@...nel.org>;
> > bhelgaas@...gle.com; Alex Williamson <alex.williamson@...hat.com>;
> > wei.liu@...nel.org; KY Srinivasan <kys@...rosoft.com>; Haiyang Zhang
> > <haiyangz@...rosoft.com>; Stephen Hemminger <sthemmin@...rosoft.com>;
> > linux-hyperv@...r.kernel.org; linux-pci@...r.kernel.org; linux-
> > kernel@...r.kernel.org; Michael Kelley (LINUX) <mikelley@...rosoft.com>;
> > robh@...nel.org; kw@...ux.com; kvm@...r.kernel.org
> > Subject: RE: [PATCH] PCI: hv: Do not set PCI_COMMAND_MEMORY to reduce
> > VM boot time
> >   
> > > From: Lorenzo Pieralisi <lorenzo.pieralisi@....com>
> > > Sent: Tuesday, April 26, 2022 9:45 AM  
> > > > ...
> > > > Sorry I don't quite follow. pci-hyperv allocates MMIO for the bridge
> > > > window in hv_pci_allocate_bridge_windows() and registers the MMIO
> > > > ranges to the core PCI driver via pci_add_resource(), and later the
> > > > core PCI driver probes the bus/device(s), validates the BAR sizes
> > > > and the pre-initialized BAR values, and uses the BAR configuration.
> > > > IMO the whole process doesn't require the bit PCI_COMMAND_MEMORY to
> > > > be pre-set, and there should be no issue to delay setting the bit to
> > > > a PCI device device's .probe() -> pci_enable_device().  
> > >
> > > IIUC you want to bootstrap devices with PCI_COMMAND_MEMORY clear
> > > (otherwise PCI core would toggle it on and off for eg BAR sizing).
> > >
> > > Is that correct ?  
> > 
> > Yes, that's the exact purpose of this patch.
> > 
> > Do you see any potential architectural issue with the patch?
> > From my reading of the core PCI code, it looks like this should be safe.
> > 
> > Jake has some concerns that I don't quite follow.
> > @Jake, could you please explain the concerns with more details?
> >   
> 
> First, let me say that I really don't know whether this is an issue.
> I know it's an issue with other operating system kernels.  I'm
> curious whether the Linux kernel / Linux PCI driver would behave in a
> way that has an issue here.
> 
> The VM has a window of address space into which it chooses to put PCI
> device's BARs.  The guest OS will generally pick the value that is
> within the BAR, by default, but it can theoretically place the device
> in any free address space.  The subset of the VM's memory address
> space which can be populated by devices' BARs is finite, and
> generally not particularly large.

AIUI, if the firmware has programmed the BAR addresses, Linux will
generally try to leave them alone, it's only unprogrammed devices or
when using the pci=realloc option that we'll try to shuffle things
around.

If you talk to bare metal system firmware developers, you might find
disagreement regarding whether the OS or system firmware owns the
device address space, which I believe also factors into our handling of
the memory space enable bit of the command register.  Minimally, system
firmware is required to allocate resources and enable boot devices, and
often these are left enabled after the hand-off to the OS.  This might
include some peripherals, for instance legacy emulation on a USB
keyboard might leave the USB host controller enabled.  There are also
more devious use cases, where there might be device monitoring running
across the bus under the OS, perhaps via SMI or other means, where if
we start moving devices around, that could theoretically break.

However, I don't really see any obvious problems with your proposal
that we simply leave the memory enable bit in the hand-off state.

> Imagine a VM that is configured with 25 NVMe controllers, each of
> which requires 64KiB of address space.  (This is just an example.)
> At first boot, all of these NVMe controllers are packed into address
> space, one after the other.
> 
> While that VM is running, one of the 25 NVMe controllers fails and is
> replaced with an NVMe controller from a separate manufacturer, but
> this one requires 128KiB of memory, for some reason.  Perhaps it
> implements the "controller buffer" feature of NVMe.  It doesn't fit
> in the hole that was vacated by the failed NVMe controller, so it
> needs to be placed somewhere else in address space.  This process
> continues over months, with several more failures and replacements.
> Eventually, the address space is very fragmented.
> 
> At some point, there is an attempt to place an NVMe controller into
> the VM but there is no contiguous block of address space free which
> would allow that NVMe controller to operate.  There is, however,
> enough total address space if the other, currently functioning, NVMe
> controllers are moved from the address space that they are using to
> other ranges, consolidating their usage and reducing fragmentation.
> Let's call this a rebalancing of memory resources.
> 
> When the NVMe controllers are moved, a new value is written into
> their BAR.  In general, the PCI spec would require that you clear the
> memory enable bit in the command register (PCI_COMMAND_MEMORY) during
> this move operation, both so that there's never a moment when two
> devices are occupying the same address space and because writing a
> 64-bit BAR atomically isn't possible.  This is the reason that I
> originally wrote the code in this driver to unmap the device from the
> VM's address space when the memory enable bit is cleared.
> 
> What I don't know is whether this sequence of operations can ever
> happen in Linux, or perhaps in a VM running Linux.  Will it rebalance
> resources in order to consolidate address space?  If it will, will
> this involve clearing the memory enable bit to ensure that two
> devices never overlap?

Once the OS is running and drivers are attached to devices, any
reshuffling of resources for those devices would require coordination
of the driver to release the resources and reprogram them.  Even if an
atomic update of the BAR were possible, that can't account for possible
in-flight use cases, such as p2p DMA.

There were a couple sessions from the 2019 Linux Plumbers conference
that might be useful to review regarding these issues.  IIRC the
first[1] was specifically looking at whether we could do partial BAR
allocations for NVMe devices, where we might have functionality but
reduced performance or features with a partial mapping.  In your
example, perhaps we're replacing a device with one that has twice the
BAR space, but is functional with only half that, so we can slide it
into the same slot as the previous device.  This would likely mean
enlightening the PCI core with device or class specific information.
I've not followed whether anything occurred here.

The second[2] (next session, same recording) discusses problems around
resource allocation and dynamic reallocation.  Again, I haven't
followed further discussions here, but I don't expect much has changed.
Thanks,

Alex

[1]https://youtu.be/ozlQ1XQreac?list=PLVsQ_xZBEyN1PDehCCAiztGf45K_D6txS&t=6481
[2]https://youtu.be/ozlQ1XQreac?list=PLVsQ_xZBEyN1PDehCCAiztGf45K_D6txS&t=7980

Powered by blists - more mailing lists