From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MuOKl-0002ZJ-Ma for qemu-devel@nongnu.org; Sun, 04 Oct 2009 06:28:55 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MuOKg-0002Ti-6Q for qemu-devel@nongnu.org; Sun, 04 Oct 2009 06:28:54 -0400 Received: from [199.232.76.173] (port=36535 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MuOKf-0002TG-Qi for qemu-devel@nongnu.org; Sun, 04 Oct 2009 06:28:49 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48982) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MuOKf-0004eF-0j for qemu-devel@nongnu.org; Sun, 04 Oct 2009 06:28:49 -0400 Date: Sun, 4 Oct 2009 12:26:46 +0200 From: "Michael S. Tsirkin" Message-ID: <20091004102644.GM16887@redhat.com> References: <1254514577-11896-1-git-send-email-yamahata@valinux.co.jp> <1254514577-11896-13-git-send-email-yamahata@valinux.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1254514577-11896-13-git-send-email-yamahata@valinux.co.jp> Subject: [Qemu-devel] Re: [PATCH 12/25] pci: 64bit bar support. List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Isaku Yamahata Cc: qemu-devel@nongnu.org On Sat, Oct 03, 2009 at 05:16:04AM +0900, Isaku Yamahata wrote: > implemented pci 64bit bar support. > > Signed-off-by: Isaku Yamahata > --- > hw/pci.c | 41 ++++++++++++++++++++++++++++++++++++----- > hw/pci.h | 9 +++++++++ > 2 files changed, 45 insertions(+), 5 deletions(-) > > diff --git a/hw/pci.c b/hw/pci.c > index 510fad2..75af2cd 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -427,8 +427,13 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num, > addr = 0x10 + region_num * 4; > } > pci_set_long(pci_dev->config + addr, type); > - pci_set_long(pci_dev->wmask + addr, wmask & 0xffffffff); > - pci_set_long(pci_dev->cmask + addr, 0xffffffff); > + if (pci_bar_is_64bit(r)) { > + pci_set_quad(pci_dev->wmask + addr, wmask); > + pci_set_quad(pci_dev->cmask + addr, ~0ULL); > + } else { > + pci_set_long(pci_dev->wmask + addr, wmask & 0xffffffff); > + pci_set_long(pci_dev->cmask + addr, 0xffffffff); > + } > } > > static void pci_update_mappings(PCIDevice *d) > @@ -462,7 +467,11 @@ static void pci_update_mappings(PCIDevice *d) > } > } else { > if (cmd & PCI_COMMAND_MEMORY) { > - new_addr = pci_get_long(d->config + config_ofs); > + if (pci_bar_is_64bit(r)) { > + new_addr = pci_get_quad(d->config + config_ofs); > + } else { > + new_addr = pci_get_long(d->config + config_ofs); > + } > /* the ROM slot has a specific enable bit */ > if (i == PCI_ROM_SLOT && !(new_addr & 1)) > goto no_mem_map; > @@ -477,7 +486,7 @@ static void pci_update_mappings(PCIDevice *d) > > /* keep old behaviour > * without this, PC ide doesn't work well. */ > - last_addr >= UINT32_MAX) { > + (!pci_bar_is_64bit(r) && last_addr >= UINT32_MAX)) { > new_addr = PCI_BAR_UNMAPPED; I don't really understand what is this doing > } > } else { > @@ -736,7 +745,29 @@ static void pci_info_device(PCIDevice *d) > monitor_printf(mon, "I/O at 0x%04"FMT_pcibus" [0x%04"FMT_pcibus"].\n", > r->addr, r->addr + r->size - 1); > } else { > - monitor_printf(mon, "32 bit memory at 0x%08"FMT_pcibus" [0x%08"FMT_pcibus"].\n", > + const char *type; > + const char* prefetch; > + > + switch (r->type & PCI_ADDRESS_SPACE_MEM_TYPE_MASK) { > + case PCI_ADDRESS_SPACE_MEM: > + type = "32 bit"; > + break; > + case PCI_ADDRESS_SPACE_MEM_64: > + type = "64 bit"; > + break; > + default: > + type = "unknown"; > + break; this default is just dead code? How about type = is_64bit() ? "64" : "32" > + } > + > + prefetch = ""; > + if (r->type & PCI_ADDRESS_SPACE_MEM_PREFETCH) { > + prefetch = " prefetchable"; > + } > + > + monitor_printf(mon, "%s%s memory at " > + "0x%08"FMT_pcibus" [0x%08"FMT_pcibus"].\n", > + type, prefetch, > r->addr, r->addr + r->size - 1); > } > } > diff --git a/hw/pci.h b/hw/pci.h > index c209f34..0455b30 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -84,6 +84,8 @@ typedef int PCIUnregisterFunc(PCIDevice *pci_dev); > > #define PCI_ADDRESS_SPACE_MEM 0x00 > #define PCI_ADDRESS_SPACE_IO 0x01 > +#define PCI_ADDRESS_SPACE_MEM_64 0x04 /* 64 bit address */ PCI_ADDRESS_MEM_TYPE_64 is a better name: it's a type of memory, not a separate address space. Actually, we should probably just reuse macros from pci_regs.h and put them with rest of macros imported from there. The list of relevant macros (prefixes with // these that we don't use and probably shouldn't put in header): #define PCI_BASE_ADDRESS_SPACE 0x01 /* 0 = memory, 1 = I/O */ #define PCI_BASE_ADDRESS_SPACE_IO 0x01 #define PCI_BASE_ADDRESS_SPACE_MEMORY 0x00 //#define PCI_BASE_ADDRESS_MEM_TYPE_MASK 0x06 //#define PCI_BASE_ADDRESS_MEM_TYPE_32 0x00 /* 32 bit address */ //#define PCI_BASE_ADDRESS_MEM_TYPE_1M 0x02 /* Below 1M [obsolete] */ #define PCI_BASE_ADDRESS_MEM_TYPE_64 0x04 /* 64 bit address */ #define PCI_BASE_ADDRESS_MEM_PREFETCH 0x08 /* prefetchable? */ > +#define PCI_ADDRESS_SPACE_MEM_TYPE_MASK 0x06 This one is used in pci_regs.h to include 64 bit an 1M. what do you use it for? It seems unnecessary. > #define PCI_ADDRESS_SPACE_MEM_PREFETCH 0x08 > > typedef struct PCIIORegion { > @@ -94,6 +96,13 @@ typedef struct PCIIORegion { > PCIMapIORegionFunc *map_func; > } PCIIORegion; > > +static inline int pci_bar_is_64bit(const PCIIORegion *r) Is it used outside pci.c? If no, do not put in pci.h > +{ > + return !(r->type & PCI_ADDRESS_SPACE_IO) && > + ((r->type & PCI_ADDRESS_SPACE_MEM_TYPE_MASK) == > + PCI_ADDRESS_SPACE_MEM_64); This is probably wrong: we should only care about the MEM_64 bit. Why don't we just return (r->type & PCI_BASE_ADDRESS_MEM_TYPE_64)? And this function is simple enough to be open-coded then. Nit: we don't need () around the == comparison. > +} > + > #define PCI_ROM_SLOT 6 > #define PCI_NUM_REGIONS 7 > > -- > 1.6.0.2 > >