| Message ID | 1254514577-11896-13-git-send-email-yamahata@valinux.co.jp |
|---|---|
| State | Superseded |
| Headers | show |
On Sat, Oct 03, 2009 at 05:16:04AM +0900, Isaku Yamahata wrote: > implemented pci 64bit bar support. > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > --- > 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 > >
On Sun, Oct 04, 2009 at 12:26:46PM +0200, Michael S. Tsirkin wrote: > On Sat, Oct 03, 2009 at 05:16:04AM +0900, Isaku Yamahata wrote: > > implemented pci 64bit bar support. > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > > --- > > 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 I'll add a comment above the line in the patch. In a word, it is a work around of qemu implementation limitation. So far bar was only 32bit, just wrapping around check worked. But now it's 64bit, explicit 4G check becomes needed. OS writes all one bits to BAR register to detect its size meaning that memory 32 bit bar is once set right below 4GB, and then maps the area to where OS really want it to exist. Since mapping memory space at 4G causes troubles, pci_update_mapping() have to work around not to map right below 4G. > > } > > } 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. Will drop unused ones. > > #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. Will use PCI_ADDRESS_MEM_TYPE_64. Since it's not a bit, (>type & PCI_ADDRESS_SPACE_MEM_TYPE_MASK) == PCI_ADDRESS_SPACE_MEM_TYPE_64 makes sense. > Nit: we don't need () around the == comparison. > > > +} > > + > > #define PCI_ROM_SLOT 6 > > #define PCI_NUM_REGIONS 7 > > >
On Mon, Oct 05, 2009 at 06:45:06PM +0900, Isaku Yamahata wrote: > On Sun, Oct 04, 2009 at 12:26:46PM +0200, Michael S. Tsirkin wrote: > > On Sat, Oct 03, 2009 at 05:16:04AM +0900, Isaku Yamahata wrote: > > > implemented pci 64bit bar support. > > > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > > > --- > > > 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 > > I'll add a comment above the line in the patch. > > In a word, it is a work around of qemu implementation limitation. > So far bar was only 32bit, just wrapping around check worked. > But now it's 64bit, explicit 4G check becomes needed. > OS writes all one bits to BAR register to detect its size > meaning that memory 32 bit bar is once set right below 4GB, > and then maps the area to where OS really want it to exist. PCI spec actually says: Decode (I/O or memory) of a register is disabled via the command register before sizing a Base Address register. Is the OS in question out of spec then? > Since mapping memory space at 4G causes troubles, What kind of trouble? Does it happen to clash with the -1 value we use for invalid regions? Would changing invalid region to some other value help? > pci_update_mapping() have to work around not to map right below 4G. Thats out of spec, isn't it? Please add a TODO so we'll remember to fix it. > > > > } > > > } 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. > > Will drop unused ones. > > > > > #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. > > Will use PCI_ADDRESS_MEM_TYPE_64. > Since it's not a bit, > (>type & PCI_ADDRESS_SPACE_MEM_TYPE_MASK) == PCI_ADDRESS_SPACE_MEM_TYPE_64 > makes sense. It is a bit. PCI_ADDRESS_SPACE_MEM_TYPE_MASK also checks for below 1M which we don't define and so do not care about. > > Nit: we don't need () around the == comparison. > > > > > +} > > > + > > > #define PCI_ROM_SLOT 6 > > > #define PCI_NUM_REGIONS 7 > > > > > > > -- > yamahata
On Mon, Oct 05, 2009 at 12:08:42PM +0200, Michael S. Tsirkin wrote: > On Mon, Oct 05, 2009 at 06:45:06PM +0900, Isaku Yamahata wrote: > > On Sun, Oct 04, 2009 at 12:26:46PM +0200, Michael S. Tsirkin wrote: > > > On Sat, Oct 03, 2009 at 05:16:04AM +0900, Isaku Yamahata wrote: > > > > implemented pci 64bit bar support. > > > > > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > > > > --- > > > > 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 > > > > I'll add a comment above the line in the patch. > > > > In a word, it is a work around of qemu implementation limitation. > > So far bar was only 32bit, just wrapping around check worked. > > But now it's 64bit, explicit 4G check becomes needed. > > OS writes all one bits to BAR register to detect its size > > meaning that memory 32 bit bar is once set right below 4GB, > > and then maps the area to where OS really want it to exist. > > PCI spec actually says: > Decode (I/O or memory) of a register is disabled > via the command register before sizing a Base Address register. > Is the OS in question out of spec then? > > > Since mapping memory space at 4G causes troubles, > > What kind of trouble? Does it happen to clash with the -1 value we use for > invalid regions? Would changing invalid region to some other value help? PCI ide didn't work well on Linux. Linux failed to find any disks. Changing type from uint32_t to uint64_t causes the logic of address wrapping around of 32bit bar. So I had to keep 32bit bar wrap around logic explicitly with uint64_t to boot Linux. > > pci_update_mapping() have to work around not to map right below 4G. > > Thats out of spec, isn't it? Please add a TODO so we'll remember to fix it. Right. Will do.
On Mon, Oct 05, 2009 at 07:26:47PM +0900, Isaku Yamahata wrote: > On Mon, Oct 05, 2009 at 12:08:42PM +0200, Michael S. Tsirkin wrote: > > On Mon, Oct 05, 2009 at 06:45:06PM +0900, Isaku Yamahata wrote: > > > On Sun, Oct 04, 2009 at 12:26:46PM +0200, Michael S. Tsirkin wrote: > > > > On Sat, Oct 03, 2009 at 05:16:04AM +0900, Isaku Yamahata wrote: > > > > > implemented pci 64bit bar support. > > > > > > > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > > > > > --- > > > > > 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 > > > > > > I'll add a comment above the line in the patch. > > > > > > In a word, it is a work around of qemu implementation limitation. > > > So far bar was only 32bit, just wrapping around check worked. > > > But now it's 64bit, explicit 4G check becomes needed. > > > OS writes all one bits to BAR register to detect its size > > > meaning that memory 32 bit bar is once set right below 4GB, > > > and then maps the area to where OS really want it to exist. > > > > PCI spec actually says: > > Decode (I/O or memory) of a register is disabled > > via the command register before sizing a Base Address register. > > Is the OS in question out of spec then? > > > > > Since mapping memory space at 4G causes troubles, > > > > What kind of trouble? Does it happen to clash with the -1 value we use for > > invalid regions? Would changing invalid region to some other value help? > > PCI ide didn't work well on Linux. Linux failed to find any disks. > Changing type from uint32_t to uint64_t causes the logic of > address wrapping around of 32bit bar. > So I had to keep 32bit bar wrap around logic explicitly > with uint64_t to boot Linux. I'm not saying it's wrong. I'm just asking for clarification on how this helps. > > > > pci_update_mapping() have to work around not to map right below 4G. > > > > Thats out of spec, isn't it? Please add a TODO so we'll remember to fix it. > > Right. Will do. > > -- > yamahata
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; } } 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; + } + + 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 */ +#define PCI_ADDRESS_SPACE_MEM_TYPE_MASK 0x06 #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) +{ + return !(r->type & PCI_ADDRESS_SPACE_IO) && + ((r->type & PCI_ADDRESS_SPACE_MEM_TYPE_MASK) == + PCI_ADDRESS_SPACE_MEM_64); +} + #define PCI_ROM_SLOT 6 #define PCI_NUM_REGIONS 7
implemented pci 64bit bar support. Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> --- hw/pci.c | 41 ++++++++++++++++++++++++++++++++++++----- hw/pci.h | 9 +++++++++ 2 files changed, 45 insertions(+), 5 deletions(-)