On 05.03.2024 10:25, Roger Pau Monné wrote: > On Mon, Mar 04, 2024 at 02:25:45PM +0100, Jan Beulich wrote: >> On 04.03.2024 11:02, Roger Pau Monné wrote: >>>> --- a/tools/firmware/hvmloader/pci.c >>>> +++ b/tools/firmware/hvmloader/pci.c >>>> @@ -33,6 +33,13 @@ uint32_t pci_mem_start = HVM_BELOW_4G_MM >>>> const uint32_t pci_mem_end = RESERVED_MEMBASE; >>>> uint64_t pci_hi_mem_start = 0, pci_hi_mem_end = 0; >>>> >>>> +/* >>>> + * BARs larger than this value are put in 64-bit space unconditionally. >>>> That >>>> + * is, such BARs also don't play into the determination of how big the >>>> lowmem >>>> + * MMIO hole needs to be. >>>> + */ >>>> +#define HUGE_BAR_THRESH GB(1) >>> >>> I would be fine with defining this to an even lower number, like >>> 256Mb, as to avoid as much as possible memory relocation in order to >>> make the MMIO hole bigger. >> >> As suggested in a post-commit-message remark, the main question then is >> how to justify this. > > I think the justification is to avoid having to relocate memory in > order to attempt to make the hole below 4Gb larger.
Upon further thinking about it, I'm now pretty convinced that any lowering of the boundary would better be a separate change. Right here I'd like to stick to just the technically implied boundary. >>>> @@ -446,8 +455,9 @@ void pci_setup(void) >>>> * the code here assumes it to be.) >>>> * Should either of those two conditions change, this code will >>>> break. >>>> */ >>>> - using_64bar = bars[i].is_64bar && bar64_relocate >>>> - && (mmio_total > (mem_resource.max - mem_resource.base)); >>>> + using_64bar = bars[i].is_64bar && bar64_relocate && >>>> + (mmio_total > (mem_resource.max - mem_resource.base) || >>>> + bar_sz > HUGE_BAR_THRESH); >>>> bar_data = pci_readl(devfn, bar_reg); >>>> >>>> if ( (bar_data & PCI_BASE_ADDRESS_SPACE) == >>>> @@ -467,7 +477,8 @@ void pci_setup(void) >>>> resource = &mem_resource; >>>> bar_data &= ~PCI_BASE_ADDRESS_MEM_MASK; >>>> } >>>> - mmio_total -= bar_sz; >>>> + if ( bar_sz <= HUGE_BAR_THRESH ) >>>> + mmio_total -= bar_sz; >>> >>> I'm missing the part where hvmloader notifies QEMU of the possibly >>> expanded base and size memory PCI MMIO regions, so that those are >>> reflected in the PCI root complex registers? >> >> I don't understand this comment: I'm not changing the interaction >> with qemu at all. Whatever the new calculation it'll be communicated >> to qemu just as before. > > That wasn't a complain about the patch, just me failing to see where > this is done. I see. Is there any such needed though? There's nothing root-complex-ish in PIIX4 after all, for not knowing of PCIe yet. The only datasheet I have readily available is for a slightly older variant of the 82371AB, yet I can't spot any registers there which would need updating (to inform qemu). >>> Overall I think we could simplify the code by having a hardcoded 1Gb >>> PCI MMIO hole below 4Gb, fill it with all the 32bit BARs and >>> (re)locate all 64bit BARs above 4Gb (not that I'm requesting you to do >>> it here). >> >> I'm afraid that would not work very well with OSes which aren't 64-bit >> BAR / PA aware (first and foremost non-PAE 32-bit ones). Iirc that's >> the reason why it wasn't done like you suggest back at the time. > > There will still be a ~1Gb window < 4Gb, so quite a bit of space. Yet not enough to fit a single 1Gb BAR. > I'm unsure whether such OSes will have drivers to manage devices with > that huge BARs in the first place. Assuming at least basic functionality of gfx cards is backwards compatible, I see nothing wrong with an old driver successfully attaching to a modern card surfacing, say, a 256Mb BAR. I'm afraid we need to be conservative here and keep configurations working which in principle can work without using any 64-bit addresses. Jan
