[SeaBIOS] [PATCH V4 2/2] hw/pci: check if pci2pci bridges implement optional limit registers
Michael S. Tsirkin
mst at redhat.com
Thu Apr 10 20:15:48 CEST 2014
On Thu, Apr 10, 2014 at 09:07:00PM +0300, Michael S. Tsirkin wrote:
> On Thu, Apr 10, 2014 at 08:44:18PM +0300, Marcel Apfelbaum wrote:
> > <I/O Base Register, I/O Limit Register> pair and
> > <Prefetchable Memory Base Register, Prefetchable Memory Limit Register> pair
> > are both optional.
> > Do not reserve ranges if the above registers are not implemented.
> >
> > Signed-off-by: Marcel Apfelbaum <marcel.a at redhat.com>
> > ---
> > src/fw/pciinit.c | 9 ++-------
> > src/hw/pci.c | 34 ++++++++++++++++++++++++++++++++++
> > src/hw/pci.h | 9 +++++++++
> > 3 files changed, 45 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
> > index 9b5d7ad..bbaecd6 100644
> > --- a/src/fw/pciinit.c
> > +++ b/src/fw/pciinit.c
> > @@ -26,13 +26,6 @@
> > #define PCI_BRIDGE_MEM_MIN (1<<21) // 2M == hugepage size
> > #define PCI_BRIDGE_IO_MIN 0x1000 // mandated by pci bridge spec
> >
> > -enum pci_region_type {
> > - PCI_REGION_TYPE_IO,
> > - PCI_REGION_TYPE_MEM,
> > - PCI_REGION_TYPE_PREFMEM,
> > - PCI_REGION_TYPE_COUNT,
> > -};
> > -
> > static const char *region_type_name[] = {
> > [ PCI_REGION_TYPE_IO ] = "io",
> > [ PCI_REGION_TYPE_MEM ] = "mem",
> > @@ -681,6 +674,8 @@ static int pci_bios_check_devices(struct pci_bus *busses)
> > for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) {
> > u64 align = (type == PCI_REGION_TYPE_IO) ?
> > PCI_BRIDGE_IO_MIN : PCI_BRIDGE_MEM_MIN;
> > + if (!pci_bridge_has_region(s->bus_dev, type))
> > + continue;
> > if (pci_region_align(&s->r[type]) > align)
> > align = pci_region_align(&s->r[type]);
> > u64 sum = pci_region_sum(&s->r[type]);
> > diff --git a/src/hw/pci.c b/src/hw/pci.c
> > index 77cdba2..d5979af 100644
> > --- a/src/hw/pci.c
> > +++ b/src/hw/pci.c
> > @@ -244,6 +244,40 @@ u8 pci_find_capability(struct pci_device *pci, u8 cap_id)
> > return 0;
> > }
> >
> > +static int pci_config_is_reserved(struct pci_device *pci, u32 addr)
>
> Why u32? It's u8 isn't it?
>
> > +{
> > + u8 val;
> > +
> > + val = pci_config_readb(pci->bdf, addr);
> > + pci_config_writeb(pci->bdf, addr, 0xFF);
> > +
> > + if (!(pci_config_readb(pci->bdf, addr)))
>
> () not needed after !
>
> > + return 1;
> > +
> > + pci_config_writeb(pci->bdf, addr, val);
> > + return 0;
> > +}
>
> Above read simply saves the register value, write now restores it.
> It really should have a comment explaning what each stage does.
>
> But I don't see why we still waste cycles on this save/restore
> dance. This is simply the result of making the API too generic.
> Open-code it below, and add a comment:
>
>
> /* Test whether bridge support forwarding of transactions
> * of a specific type.
> * Note: disables bridge's window registers as a side effect.
> */
> int pci_bridge_has_region(struct pci_device *pci,
> enum pci_region_type region_type)
> {
> u8 base;
>
> switch (region_type) {
> case PCI_REGION_TYPE_IO:
> base = PCI_IO_BASE;
> break;
> case PCI_REGION_TYPE_PREFMEM:
> base = PCI_PREF_MEMORY_BASE;
> break;
> default:
> /* Regular memory support is mandatory */
> return 1;
> }
>
> pci_config_writeb(pci->bdf, addr, 0xFF);
>
> return pci_config_readb(pci->bdf, addr) != 0;
> }
>
> > +
> > void
> > pci_reboot(void)
> > {
> > diff --git a/src/hw/pci.h b/src/hw/pci.h
> > index e828225..0aaa84c 100644
> > --- a/src/hw/pci.h
> > +++ b/src/hw/pci.h
> > @@ -12,6 +12,13 @@
> > #define PCI_NUM_REGIONS 7
> > #define PCI_BRIDGE_NUM_REGIONS 2
> >
> > +enum pci_region_type {
> > + PCI_REGION_TYPE_IO,
> > + PCI_REGION_TYPE_MEM,
> > + PCI_REGION_TYPE_PREFMEM,
> > + PCI_REGION_TYPE_COUNT,
> > +};
> > +
> > static inline u8 pci_bdf_to_bus(u16 bdf) {
> > return bdf >> 8;
> > }
> > @@ -117,6 +124,8 @@ int pci_init_device(const struct pci_device_id *ids
> > struct pci_device *pci_find_init_device(const struct pci_device_id *ids
> > , void *arg);
> > u8 pci_find_capability(struct pci_device *pci, u8 cap_id);
> > +int pci_bridge_has_region(struct pci_device *pci,
> > + enum pci_region_type region_type);
> > void pci_reboot(void);
> >
> > #endif
> > --
> > 1.8.3.1
> >
And of course these are all nitpicks, would be better to fix
but the patch is otherwise correct.
More information about the SeaBIOS
mailing list