[SeaBIOS] [Qemu-devel] [PATCH V3 2/2] hw/pci: check if pci2pci bridges implement optional limit registers
Marcel Apfelbaum
marcel.a at redhat.com
Thu Apr 10 18:29:29 CEST 2014
On Thu, 2014-04-10 at 18:46 +0300, Michael S. Tsirkin wrote:
> On Thu, Apr 10, 2014 at 04:29:41PM +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>
>
>
> Reviewed-by: Michael S. Tsirkin <mst at redhat.com>
>
>
>
>
> > ---
> > src/fw/pciinit.c | 9 ++-------
> > src/hw/pci.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> > src/hw/pci.h | 9 +++++++++
> > 3 files changed, 59 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 055353d..27e7b1c 100644
> > --- a/src/hw/pci.c
> > +++ b/src/hw/pci.c
> > @@ -243,6 +243,54 @@ u8 pci_find_capability(struct pci_device *pci, u8 cap_id)
> > return 0;
> > }
> >
> > +static int pci_config_writableb(struct pci_device *pci, u32 addr, u8 test_val)
>
> Don't like this name.
> pci_config_writable_byte or better see below.
>
> > +{
> > + u8 val;
> > +
> > + val = pci_config_readb(pci->bdf, addr);
> > + pci_config_writeb(pci->bdf, addr, test_val);
> > +
> > + if (!(pci_config_readb(pci->bdf, addr)))
> > + return 0;
>
> This only works for readonly fields which return 0
> on read. Maybe name should reflect this:
> how about pci_config_is_reserved() ?
No problem with that
>
> There's no need to pass in test value either,
> it makes the interface fragile:
> passing e.g. 0x0 as value won't work.
Yes, the value would be the caller responsibility.
But I think that a value like 0xF0 (explained below)
will cover all we need.
>
> > +
> > + pci_config_writeb(pci->bdf, addr, val);
>
> Practically that's overkill.
I have to restore prev settings. I do not see this as
an overkill: a query should preserve the prev value.
And is also not expensive, and it is made only by bridges,
not a real issue from my point of view.
>
> Just set base > limit and there won't be
> need to save and restore: range is disabled.
That would be possible, yes. But again, I don't want
to change values in a query.
>
>
>
> > + return 1;
> > +}
> > +
> > +static int pci_config_writablew(struct pci_device *pci, u32 addr, u16 test_val)
> > +{
> > + u16 val;
> > +
> > + val = pci_config_readw(pci->bdf, addr);
> > + pci_config_writew(pci->bdf, addr, test_val);
> > +
> > + if (!(pci_config_readw(pci->bdf, addr)))
> > + return 0;
> > +
> > + pci_config_writew(pci->bdf, addr, val);
> > + return 1;
> > +}
>
>
> This one isn't needed at all.
I can use the byte function, yes, thanks, I'll drop it.
>
> > +
> > +int pci_bridge_has_region(struct pci_device *pci,
> > + enum pci_region_type region_type)
> > +{
> > + if (pci->class != PCI_CLASS_BRIDGE_PCI)
> > + return 0;
>
> Do we really need this test here?
Better safe than sorry. If is not a bridge this will mess up
pci device's configuration and the bug will be hard to find.
>
> > +
> > + switch (region_type) {
> > + case PCI_REGION_TYPE_IO:
> > + return pci_config_writableb(pci, PCI_IO_BASE, 0xF0) &&
> > + pci_config_writableb(pci, PCI_IO_LIMIT, 0xF0);
>
> I think we should only test base.
> Testing limit like this might cause issues as bridge now
> claims some resources temporarily.
I can test only one, yes. The spec said that both should be 0,
but going for one of them should be enough.
> Also why 0xF0 and not 0xFF? Seems like a random value.
0xF0 is not random, the last 4 bits are *always* read only, this
is why I don't test them. Any value starting from 0x10 will do.
0xFF it will still work, as the last 4 bits will be silently
dropped, but why not follow the spec.
>
>
> > + case PCI_REGION_TYPE_PREFMEM:
> > + return pci_config_writablew(pci, PCI_PREF_MEMORY_BASE, 0xFFF0) &&
> > + pci_config_writablew(pci, PCI_PREF_MEMORY_LIMIT, 0xFFF0);
> > + case PCI_REGION_TYPE_MEM: /* fall through */
> > + default:
> > + return 1;
> > + }
> > +
> > + return 1;
> > +}
> >
> > void
> > pci_reboot(void)
>
[...]
> > + }
> > +
> > + return 1;
>
> unreacheable code.
Right. Some compilation flags and automatic check tools might
have a problem with that, this why I left it there. I can remove
it, of course.
>
> > +}
>
>
>
>
> Here's a simpler implementation.
>
> 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;
> }
>
> /* Check that base is writeable. Safe as this increases base so it
> * won't cause bridge to claim new resources.
Yes, but it can claim less. Why mess with the orig value for
a query? I think a query should not modify a value, even if does
not affect the system. It is hard to track this kind of changes,
and restoring the value is not a big price to pay IMHO.
Thanks for the review,
Marcel
> */
> pci_config_writeb(pci->bdf, base, 0xFF);
> return !!pci_config_readb(pci->bdf, base);
> }
>
>
>
>
> > 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
More information about the SeaBIOS
mailing list