[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN PATCH v3] hvmloader: Enable MMIO and I/O decode, after all resource allocation
On Tue, 2020-04-14 at 16:12 +0200, Jan Beulich wrote: > CAUTION: This email originated from outside of the organization. Do > not click links or open attachments unless you can confirm the sender > and know the content is safe. > > > > On 14.04.2020 13:12, Harsha Shamsundara Havanur wrote: > > It was observed that PCI MMIO and/or IO BARs were programmed with > > memory and I/O decodes (bits 0 and 1 of PCI COMMAND register) > > enabled, > > during PCI setup phase. This resulted in incorrect memory mapping > > as > > soon as the lower half of the 64 bit bar is programmed. > > This displaced any RAM mappings under 4G. After the > > upper half is programmed PCI memory mapping is restored to its > > intended high mem location, but the RAM displaced is not restored. > > The OS then continues to boot and function until it tries to access > > the displaced RAM at which point it suffers a page fault and > > crashes. > > > > This patch address the issue by deferring enablement of memory and > > I/O decode in command register until all the resources, like > > interrupts > > I/O and/or MMIO BARs for all the PCI device functions are > > programmed, > > in the descending order of memory requested. > > > > Signed-off-by: Harsha Shamsundara Havanur <havanur@xxxxxxxxxx> > > Reviewed-by: Paul Durrant <pdurrant@xxxxxxxxxx> > > Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> > > You've fixed bugs, implying you need to drop tags covering the code > which was fixed. As said on v2, imo you should have dropped the tags > then already. > > > --- a/tools/firmware/hvmloader/pci.c > > +++ b/tools/firmware/hvmloader/pci.c > > @@ -84,6 +84,7 @@ void pci_setup(void) > > uint32_t vga_devfn = 256; > > uint16_t class, vendor_id, device_id; > > unsigned int bar, pin, link, isa_irq; > > + uint8_t pci_devfn_decode_type[256] = {}; > > I'm still waiting for a reply on my v1 comment on the stack > consumption of this, suggesting two 256-bit bitmaps instead. > I chose uint8 array over bitmaps as this reduces complexity of code to get and set individual bits. Sorry for missing this out in the v1 conversation. > > @@ -120,6 +121,9 @@ void pci_setup(void) > > */ > > bool allow_memory_relocate = 1; > > > > + BUILD_BUG_ON((typeof(*pci_devfn_decode_type))PCI_COMMAND_MEMOR > > Y != PCI_COMMAND_MEMORY); > > + BUILD_BUG_ON((typeof(*pci_devfn_decode_type))PCI_COMMAND_IO != > > PCI_COMMAND_IO); > > These lines are too long. > > > @@ -288,10 +292,21 @@ void pci_setup(void) > > printf("pci dev %02x:%x INT%c->IRQ%u\n", > > devfn>>3, devfn&7, 'A'+pin-1, isa_irq); > > } > > - > > /* Enable bus mastering. */ > > Please don't drop a blank line like this. > > > cmd = pci_readw(devfn, PCI_COMMAND); > > cmd |= PCI_COMMAND_MASTER; > > + > > + /* > > + * Disable memory and I/O decode, > > + * It is recommended that BAR programming be done whilst > > + * decode bits are cleared to avoid incorrect mappings > > being created, > > + * when 64-bit memory BAR is programmed first by writing > > the lower half > > + * and then the upper half, which first maps to an address > > under 4G > > + * replacing any RAM mapped in that address, which is not > > restored > > + * back after the upper half is written and PCI memory is > > correctly > > + * mapped to its intended high mem address. > > + */ > > + cmd &= ~(PCI_COMMAND_MEMORY | PCI_COMMAND_IO); > > pci_writew(devfn, PCI_COMMAND, cmd); > > } > > I'd like to note that the disabling of IO and MEMORY you do here > comes too > late: It should happen before any of the BARs gets written with ~0. > In > particular for 64-bit BARs these writes can again cause undue effects > on > the intermediately resulting effective addresses. > I agree, this needs to be done before writing BARS with ~0 > > @@ -526,10 +537,16 @@ void pci_setup(void) > > * has IO enabled, even if there is no I/O BAR on that > > * particular device. > > */ > > - cmd = pci_readw(vga_devfn, PCI_COMMAND); > > - cmd |= PCI_COMMAND_IO; > > - pci_writew(vga_devfn, PCI_COMMAND, cmd); > > + pci_devfn_decode_type[vga_devfn] |= PCI_COMMAND_IO; > > } > > + > > + /* Enable memory and I/O decode. */ > > + for ( devfn = 0; devfn < 256; devfn++ ) > > + if ( pci_devfn_decode_type[devfn] ) { > > Style: The brace belongs on its own line. > > To save one set of writes to the command registers I would, btw, > be okay with you moving the MASTER enabling here. > Good point. I will make these changes in v4. > Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |