From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44475) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WYJVP-00078M-6s for qemu-devel@nongnu.org; Thu, 10 Apr 2014 14:15:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WYJVJ-0003SK-1f for qemu-devel@nongnu.org; Thu, 10 Apr 2014 14:15:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40456) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WYJVI-0003SF-Q2 for qemu-devel@nongnu.org; Thu, 10 Apr 2014 14:15:12 -0400 Date: Thu, 10 Apr 2014 21:15:48 +0300 From: "Michael S. Tsirkin" Message-ID: <20140410181548.GA20852@redhat.com> References: <1397151858-20263-1-git-send-email-marcel.a@redhat.com> <1397151858-20263-3-git-send-email-marcel.a@redhat.com> <20140410180700.GC28144@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20140410180700.GC28144@redhat.com> Subject: Re: [Qemu-devel] [PATCH V4 2/2] hw/pci: check if pci2pci bridges implement optional limit registers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marcel Apfelbaum Cc: kevin@koconnor.net, seabios@seabios.org, kraxel@redhat.com, qemu-devel@nongnu.org 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: > > pair and > > pair > > are both optional. > > Do not reserve ranges if the above registers are not implemented. > > > > Signed-off-by: Marcel Apfelbaum > > --- > > 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.