Re: [PATCH 3/6] PCI: restore memory decoding after reallocation

[Stewart Hildebrand <stewart.hildebrand@amd.com>]

On 7/9/24 12:16, Bjorn Helgaas wrote:
> On Tue, Jul 09, 2024 at 09:36:00AM -0400, Stewart Hildebrand wrote:
>> Currently, the PCI subsystem unconditionally clears the memory decoding
>> bit of devices with resource alignment specified. Unfortunately, some
>> drivers assume memory decoding was enabled by firmware. Restore the
>> memory decoding bit after the resource has been reallocated.
> 
> Which drivers have you tripped over?  Those drivers apparently don't
> call pci_enable_device() and assume the firmware has left memory
> decoding enabled.  I don't think there's any guarantee that firmware
> must do that, so the drivers are probably broken on some platforms,
> and we could improve things overall by adding the pci_enable_device()
> to them.

Agreed. Well, it would be vgacon, but lspci -v doesn't actually show any
driver attached to the device in my test case. Presumably because vgacon
just uses the 0xb8000 buffer directly without any sort of PCI
involvement. Memory decoding must be enabled on this particular VGA
device for vgacon to properly display a console on the display. If
memory decoding becomes disabled on the VGA device (or fails to be
reenabled), the console ceases to display properly.

>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>> ---
>> Relevant prior discussion at [1]
>>
>> [1] https://lore.kernel.org/linux-pci/20160906165652.GE1214@localhost/
>> ---
>>  drivers/pci/pci.c       |  1 +
>>  drivers/pci/setup-bus.c | 25 +++++++++++++++++++++++++
>>  include/linux/pci.h     |  2 ++
>>  3 files changed, 28 insertions(+)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index f017e7a8f028..7953e75b9129 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -6633,6 +6633,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
>>  
>>  	pci_read_config_word(dev, PCI_COMMAND, &command);
>>  	if (command & PCI_COMMAND_MEMORY) {
>> +		dev->dev_flags |= PCI_DEV_FLAGS_MEMORY_ENABLE;
>>  		command &= ~PCI_COMMAND_MEMORY;
>>  		pci_write_config_word(dev, PCI_COMMAND, command);
>>  	}
> 
> It would be nice if this could be contained to
> pci_reassigndev_resource_alignment() so the clear and restore could be
> in the same function.  But I suppose the concern is that re-enabling
> decoding too early could be an issue in a hierarchy where bridge
> windows are also reassigned?

Well, yes, and, even if the bridge windows are sufficient and we're just
allocating a new a BAR, that happens later on. If we were to return from
pci_reassigndev_resource_alignment() with memory decoding enabled, we'd
have the situation described in [1] with our knowledge of what the BAR
contains thrown away while memory decoding is enabled. We'd also
potentially be writing the new BAR while memory decoding is enabled.

>> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
>> index ab7510ce6917..6847b251e19a 100644
>> --- a/drivers/pci/setup-bus.c
>> +++ b/drivers/pci/setup-bus.c
>> @@ -2131,6 +2131,29 @@ pci_root_bus_distribute_available_resources(struct pci_bus *bus,
>>  	}
>>  }
>>  
>> +static void restore_memory_decoding(struct pci_bus *bus)
>> +{
>> +	struct pci_dev *dev;
>> +
>> +	list_for_each_entry(dev, &bus->devices, bus_list) {
>> +		struct pci_bus *b;
>> +
>> +		if (dev->dev_flags & PCI_DEV_FLAGS_MEMORY_ENABLE) {
>> +			u16 command;
>> +
>> +			pci_read_config_word(dev, PCI_COMMAND, &command);
>> +			command |= PCI_COMMAND_MEMORY;
>> +			pci_write_config_word(dev, PCI_COMMAND, command);
>> +		}
>> +
>> +		b = dev->subordinate;
>> +		if (!b)
>> +			continue;
>> +
>> +		restore_memory_decoding(b);
>> +	}
>> +}
>> +
>>  /*
>>   * First try will not touch PCI bridge res.
>>   * Second and later try will clear small leaf bridge res.
>> @@ -2229,6 +2252,8 @@ void pci_assign_unassigned_root_bus_resources(struct pci_bus *bus)
>>  	goto again;
>>  
>>  dump:
>> +	restore_memory_decoding(bus);
>> +
>>  	/* Dump the resource on buses */
>>  	pci_bus_dump_resources(bus);
>>  }
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index e83ac93a4dcb..cb5df4c6a999 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -245,6 +245,8 @@ enum pci_dev_flags {
>>  	PCI_DEV_FLAGS_NO_RELAXED_ORDERING = (__force pci_dev_flags_t) (1 << 11),
>>  	/* Device does honor MSI masking despite saying otherwise */
>>  	PCI_DEV_FLAGS_HAS_MSI_MASKING = (__force pci_dev_flags_t) (1 << 12),
>> +	/* Firmware enabled memory decoding, to be restored if BAR is updated */
>> +	PCI_DEV_FLAGS_MEMORY_ENABLE = (__force pci_dev_flags_t) (1 << 13),
>>  };
>>  
>>  enum pci_irq_reroute_variant {
>> -- 
>> 2.45.2
>>