Message-ID: <965c4c84-be1a-ba30-400a-62481ae7400a@linux.intel.com>
Date: Tue, 12 Nov 2024 18:16:41 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Christian König <christian.koenig@....com>
cc: Michał Winiarski <michal.winiarski@...el.com>, 
    Bjorn Helgaas <bhelgaas@...gle.com>, linux-pci@...r.kernel.org, 
    LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] PCI: Fix BAR resizing when VF BARs are assigned

On Tue, 12 Nov 2024, Christian König wrote:

> Am 12.11.24 um 15:28 schrieb Ilpo Järvinen:
> 
> On Tue, 12 Nov 2024, Christian König wrote:
> 
> Am 12.11.24 um 14:42 schrieb Ilpo Järvinen:
> 
> __resource_resize_store() attempts to release all resources of the
> device before attempting the resize. The loop, however, only covers
> standard BARs (< PCI_STD_NUM_BARS). If a device has VF BARs that are
> assigned, pci_reassign_bridge_resources() finds the bridge window still
> has some assigned child resources and returns -NOENT which makes
> pci_resize_resource() to detect an error and abort the resize.
> 
> 
> Looks good in general, but a couple of comments.
> 
> Similarly, an assigned Expansion ROM resource prevents the resize.
> 
> 
> Expansion ROMs were intentionally left untouched by the initial patch since those are
> usually 32bit resources and the resize was implemented only for 64bit BARs.
> 
> Change the release loop to cover both the VF BARs and Expansion ROM
> which allows the resize operation to release the bridge windows and
> attempt to assigned them again with the different size.
> 
> 
> I'm not sure if Expansion ROMs should be touched here. Those are 32bit resources
> usually and notoriously broken in the ACPI tables.
> 
> What I've seen multiple times that after releasing them you can't move nor assign
> them again to their original position.
> 
> Background is that some ACPI table denotes the location of the ROM as reserved and we
> only accept the Expansion ROM at that location because of a quirk (which is of course
> not applied when you resize).
> 
> On the other hand do we have use cases for resizing 32bit BARs? So far we resized
> those only by accident.
> 
> Thanks for taking a look!
> 
> This is not about resizing 32bit BARs.
> 
> Is that somehow enforced so they cannot end up into the same bridge 
> window? Because we can only resize a bridge window if we release all its 
> child resources.
> 
> 
> Good question, I'm not 100% sure either.
> 
> IIRC the PCI subsystem has a fallback which tries to squeeze 64bit BARs into 32bit
> windows of bridges if nothing else works.

Yes, as far as I understand the code, that can happen.

> But the PCI subsystem never does that the other way around. Simply because a 32bit
> BAR doesn't necessary work in a 64bit window.
>
> So you should never need to free up a 32bit BAR to resize and/or move a 64bit BAR
> because that operation should move the 64bit BAR into the 64bit window anyway.

I fail to follow your logic here. Why exactly the 64bit resizable BAR 
couldn't end up into a bridge window that is shared with Expansion ROM 
(and perhaps other 32-bit resources) consider the fallbacks? And why 
couldn't the resize be attempted there?

> That we only free up resources with the same flags is taken care of this code here:
> 
>                 if (pci_resource_len(pdev, i) &&
>                     pci_resource_flags(pdev, i) == flags)
>                         pci_release_resource(pdev, i);
> 
> The flags should contain IORESOURCE_MEM_64 for 64bit BARs. So we most likely don't
> need a special handling for Expansion ROMs or 32bit BARs.

Most likely true, neither would get released and the resize ends up 
failing.

However, now that you brought it up, I question if that flags check there 
is correct at all. I think it would simply want to check if the resource 
to be resized shares the bridge window with the i'th resource? Do you 
agree? (And that check has nothing to do with 32/64bit.)

This is far from the only inconsistency I've come across while recently 
trying to address a number of issues in the resource fitting and 
assignment logic. I'm slowly trying to come into understanding of mess 
this ->flags and resource fitting & allocation is and hopefully 
eventually make it something that is actually tractable (yeah, wish me 
luck :-/).

So I don't want to accept "we most likely don't need a special handling 
for Expansion ROMs or 32bit BARs" just because it happens to work, I'd 
want that check to make sense too.

It would always be possible to explicitly do something if we detect 
Expansion ROM resource that is assigned (and perhaps not disabled?) but 
even then, preventing resize from proceeding just because something bad 
might happen to the Expansion ROM seems a bit big hammer to me (but 
could certainly e.g. print a warning if that gets attempted for ROM).

-- 
 i.

> Regards,
> Christian.
> 
> As __resource_resize_store() checks first that no driver is bound to
> the PCI device before resizing is allowed, SR-IOV cannot be enabled
> during resize so it is safe to release also the IOV resources.
> 
> Fixes: 8bb705e3e79d ("PCI: Add pci_resize_resource() for resizing BARs")
> 
> 
> The code in question was added by 91fa127794ac ("PCI: Expose PCIe Resizable BAR
> support via sysfs").
> 
> Oh right. I don't know why I ended up picking that other commit (sure, I 
> was also looking that other diff but maybe it was just a copy-paste 
> error).
> 
> 
> 
> 

Powered by blists - more mailing lists