| Message ID | 1254737223-16129-13-git-send-email-yamahata@valinux.co.jp |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Oct 05, 2009 at 07:06:52PM +0900, Isaku Yamahata wrote: > implemented pci 64bit bar support. > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > --- > hw/pci.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++-------- > hw/pci.h | 2 ++ > 2 files changed, 48 insertions(+), 8 deletions(-) > > diff --git a/hw/pci.c b/hw/pci.c > index 21565f5..09a6816 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -395,6 +395,13 @@ int pci_unregister_device(PCIDevice *pci_dev) > return 0; > } > > +static inline int pci_bar_is_mem64(const PCIIORegion *r) > +{ > + return !(r->type & PCI_ADDRESS_SPACE_IO) && > + (r->type & PCI_ADDRESS_SPACE_MEM_TYPE_MASK) == > + PCI_ADDRESS_SPACE_MEM_TYPE_64; > +} > + why are we checking PCI_ADDRESS_SPACE_IO? Let's not. > void pci_register_bar(PCIDevice *pci_dev, int region_num, > pcibus_t size, int type, > PCIMapIORegionFunc *map_func) > @@ -427,8 +434,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_mem64(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 +474,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_mem64(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; > @@ -473,11 +489,24 @@ static void pci_update_mappings(PCIDevice *d) > mappings, we handle specific values as invalid > mappings. */ > if (last_addr <= new_addr || new_addr == 0 || > - last_addr == PCI_BAR_UNMAPPED || > + last_addr == PCI_BAR_UNMAPPED) { > + new_addr = PCI_BAR_UNMAPPED; > + } > > - /* keep old behaviour > - * without this, PC ide doesn't work well. */ > - last_addr >= UINT32_MAX) { > + /* > + * work around: without this, PC ide and other devices > + * don't work well. > + * OS writes all 1 bits to 32 BAR to find its size Isn't memory disabled then? PCI spec says it should be ... > + * resulting in setting UINT32_MAX - alignemnt, typo > + * and then OS sets the BAR to where they really want > + * the BAR to sit. > + * On the other hand, there are some important areas > + * blow 4G on i386/x86_64. So setting BAR over those area typo > + * below 4G causes troubles. > + * We work around the issues by prohibitting BAR typo. > + * from sitting right blow 4G. typo > + */ > + if (!pci_bar_is_mem64(r) && last_addr >= UINT32_MAX) { > new_addr = PCI_BAR_UNMAPPED; > } > } else { > @@ -736,7 +765,16 @@ 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 = pci_bar_is_mem64(r)? "64 bit": "32 bit"; > + const char *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 cbf80c0..b65ce03 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_TYPE_MASK 0x06 > +#define PCI_ADDRESS_SPACE_MEM_TYPE_64 0x04 /* 64 bit address */ something wrong with text alignment here. pls align with tabs as the rest of surrounding code does. And I thought we were switching to names from pci_regs.h? > #define PCI_ADDRESS_SPACE_MEM_PREFETCH 0x08 > > typedef struct PCIIORegion { > -- > 1.6.0.2
On Mon, Oct 05, 2009 at 07:06:52PM +0900, Isaku Yamahata wrote: > implemented pci 64bit bar support. > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > --- > hw/pci.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++-------- > hw/pci.h | 2 ++ > 2 files changed, 48 insertions(+), 8 deletions(-) > > diff --git a/hw/pci.c b/hw/pci.c > index 21565f5..09a6816 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -395,6 +395,13 @@ int pci_unregister_device(PCIDevice *pci_dev) > return 0; > } > > +static inline int pci_bar_is_mem64(const PCIIORegion *r) > +{ > + return !(r->type & PCI_ADDRESS_SPACE_IO) && > + (r->type & PCI_ADDRESS_SPACE_MEM_TYPE_MASK) == > + PCI_ADDRESS_SPACE_MEM_TYPE_64; > +} > + > void pci_register_bar(PCIDevice *pci_dev, int region_num, > pcibus_t size, int type, > PCIMapIORegionFunc *map_func) > @@ -427,8 +434,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_mem64(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 +474,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_mem64(r)) { > + new_addr = pci_get_quad(d->config + config_ofs); From previous patch, config_ofs is region_num * 4 This is incorrect for 64 bit regions. > + } 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; > @@ -473,11 +489,24 @@ static void pci_update_mappings(PCIDevice *d) > mappings, we handle specific values as invalid > mappings. */ > if (last_addr <= new_addr || new_addr == 0 || > - last_addr == PCI_BAR_UNMAPPED || > + last_addr == PCI_BAR_UNMAPPED) { > + new_addr = PCI_BAR_UNMAPPED; > + } > > - /* keep old behaviour > - * without this, PC ide doesn't work well. */ > - last_addr >= UINT32_MAX) { > + /* > + * work around: without this, PC ide and other devices > + * don't work well. > + * OS writes all 1 bits to 32 BAR to find its size > + * resulting in setting UINT32_MAX - alignemnt, > + * and then OS sets the BAR to where they really want > + * the BAR to sit. > + * On the other hand, there are some important areas > + * blow 4G on i386/x86_64. So setting BAR over those area > + * below 4G causes troubles. > + * We work around the issues by prohibitting BAR > + * from sitting right blow 4G. > + */ > + if (!pci_bar_is_mem64(r) && last_addr >= UINT32_MAX) { > new_addr = PCI_BAR_UNMAPPED; > } > } else { > @@ -736,7 +765,16 @@ 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 = pci_bar_is_mem64(r)? "64 bit": "32 bit"; > + const char *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 cbf80c0..b65ce03 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_TYPE_MASK 0x06 > +#define PCI_ADDRESS_SPACE_MEM_TYPE_64 0x04 /* 64 bit address */ > #define PCI_ADDRESS_SPACE_MEM_PREFETCH 0x08 > > typedef struct PCIIORegion { > -- > 1.6.0.2
On Mon, Oct 05, 2009 at 02:06:30PM +0200, Michael S. Tsirkin wrote: > On Mon, Oct 05, 2009 at 07:06:52PM +0900, Isaku Yamahata wrote: > > implemented pci 64bit bar support. > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > > --- > > hw/pci.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++-------- > > hw/pci.h | 2 ++ > > 2 files changed, 48 insertions(+), 8 deletions(-) > > > > diff --git a/hw/pci.c b/hw/pci.c > > index 21565f5..09a6816 100644 > > --- a/hw/pci.c > > +++ b/hw/pci.c > > @@ -395,6 +395,13 @@ int pci_unregister_device(PCIDevice *pci_dev) > > return 0; > > } > > > > +static inline int pci_bar_is_mem64(const PCIIORegion *r) > > +{ > > + return !(r->type & PCI_ADDRESS_SPACE_IO) && > > + (r->type & PCI_ADDRESS_SPACE_MEM_TYPE_MASK) == > > + PCI_ADDRESS_SPACE_MEM_TYPE_64; > > +} > > + > > why are we checking PCI_ADDRESS_SPACE_IO? Let's not. Because bar handling logic for io bar and 32bit memory bar is same. For example, pci_register_bar() does. > > void pci_register_bar(PCIDevice *pci_dev, int region_num, > > pcibus_t size, int type, > > PCIMapIORegionFunc *map_func) > > @@ -427,8 +434,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_mem64(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 +474,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_mem64(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; > > @@ -473,11 +489,24 @@ static void pci_update_mappings(PCIDevice *d) > > mappings, we handle specific values as invalid > > mappings. */ > > if (last_addr <= new_addr || new_addr == 0 || > > - last_addr == PCI_BAR_UNMAPPED || > > + last_addr == PCI_BAR_UNMAPPED) { > > + new_addr = PCI_BAR_UNMAPPED; > > + } > > > > - /* keep old behaviour > > - * without this, PC ide doesn't work well. */ > > - last_addr >= UINT32_MAX) { > > + /* > > + * work around: without this, PC ide and other devices > > + * don't work well. > > + * OS writes all 1 bits to 32 BAR to find its size > > Isn't memory disabled then? PCI spec says it should be ... > > > + * resulting in setting UINT32_MAX - alignemnt, > > typo > > > + * and then OS sets the BAR to where they really want > > + * the BAR to sit. > > + * On the other hand, there are some important areas > > + * blow 4G on i386/x86_64. So setting BAR over those area > > typo > > > + * below 4G causes troubles. > > + * We work around the issues by prohibitting BAR > > typo. > > > + * from sitting right blow 4G. > > typo > > > + */ > > + if (!pci_bar_is_mem64(r) && last_addr >= UINT32_MAX) { > > new_addr = PCI_BAR_UNMAPPED; > > } > > } else { > > @@ -736,7 +765,16 @@ 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 = pci_bar_is_mem64(r)? "64 bit": "32 bit"; > > + const char *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 cbf80c0..b65ce03 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_TYPE_MASK 0x06 > > +#define PCI_ADDRESS_SPACE_MEM_TYPE_64 0x04 /* 64 bit address */ > > something wrong with text alignment here. pls align with tabs as the > rest of surrounding code does. > > And I thought we were switching to names from pci_regs.h? > > > #define PCI_ADDRESS_SPACE_MEM_PREFETCH 0x08 > > > > typedef struct PCIIORegion { >
On Mon, Oct 05, 2009 at 02:47:11PM +0200, Michael S. Tsirkin wrote: > > @@ -462,7 +474,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_mem64(r)) { > > + new_addr = pci_get_quad(d->config + config_ofs); > > From previous patch, config_ofs is region_num * 4 > This is incorrect for 64 bit regions. I don't see any problems. In the following example, 0, 2 ... will be used for region_number and 1 is left unused. BAR0 64bit memory BAR1 used by BAR0 64bit BAR2 ...
On Tue, Oct 06, 2009 at 06:38:30PM +0900, Isaku Yamahata wrote: > On Mon, Oct 05, 2009 at 02:06:30PM +0200, Michael S. Tsirkin wrote: > > On Mon, Oct 05, 2009 at 07:06:52PM +0900, Isaku Yamahata wrote: > > > implemented pci 64bit bar support. > > > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > > > --- > > > hw/pci.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++-------- > > > hw/pci.h | 2 ++ > > > 2 files changed, 48 insertions(+), 8 deletions(-) > > > > > > diff --git a/hw/pci.c b/hw/pci.c > > > index 21565f5..09a6816 100644 > > > --- a/hw/pci.c > > > +++ b/hw/pci.c > > > @@ -395,6 +395,13 @@ int pci_unregister_device(PCIDevice *pci_dev) > > > return 0; > > > } > > > > > > +static inline int pci_bar_is_mem64(const PCIIORegion *r) > > > +{ > > > + return !(r->type & PCI_ADDRESS_SPACE_IO) && > > > + (r->type & PCI_ADDRESS_SPACE_MEM_TYPE_MASK) == > > > + PCI_ADDRESS_SPACE_MEM_TYPE_64; > > > +} > > > + > > > > why are we checking PCI_ADDRESS_SPACE_IO? Let's not. > > Because bar handling logic for io bar and 32bit memory bar is same. > For example, pci_register_bar() does. Yes, but 64 bit handling does not care about this: if you see a 64 bit bar, you can handle it as 64 bit. No need to check the I/O bit. > > > > void pci_register_bar(PCIDevice *pci_dev, int region_num, > > > pcibus_t size, int type, > > > PCIMapIORegionFunc *map_func) > > > @@ -427,8 +434,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_mem64(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 +474,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_mem64(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; > > > @@ -473,11 +489,24 @@ static void pci_update_mappings(PCIDevice *d) > > > mappings, we handle specific values as invalid > > > mappings. */ > > > if (last_addr <= new_addr || new_addr == 0 || > > > - last_addr == PCI_BAR_UNMAPPED || > > > + last_addr == PCI_BAR_UNMAPPED) { > > > + new_addr = PCI_BAR_UNMAPPED; > > > + } > > > > > > - /* keep old behaviour > > > - * without this, PC ide doesn't work well. */ > > > - last_addr >= UINT32_MAX) { > > > + /* > > > + * work around: without this, PC ide and other devices > > > + * don't work well. > > > + * OS writes all 1 bits to 32 BAR to find its size > > > > Isn't memory disabled then? PCI spec says it should be ... > > > > > + * resulting in setting UINT32_MAX - alignemnt, > > > > typo > > > > > + * and then OS sets the BAR to where they really want > > > + * the BAR to sit. > > > + * On the other hand, there are some important areas > > > + * blow 4G on i386/x86_64. So setting BAR over those area > > > > typo > > > > > + * below 4G causes troubles. > > > + * We work around the issues by prohibitting BAR > > > > typo. > > > > > + * from sitting right blow 4G. > > > > typo > > > > > + */ > > > + if (!pci_bar_is_mem64(r) && last_addr >= UINT32_MAX) { > > > new_addr = PCI_BAR_UNMAPPED; > > > } > > > } else { > > > @@ -736,7 +765,16 @@ 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 = pci_bar_is_mem64(r)? "64 bit": "32 bit"; > > > + const char *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 cbf80c0..b65ce03 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_TYPE_MASK 0x06 > > > +#define PCI_ADDRESS_SPACE_MEM_TYPE_64 0x04 /* 64 bit address */ > > > > something wrong with text alignment here. pls align with tabs as the > > rest of surrounding code does. > > > > And I thought we were switching to names from pci_regs.h? > > > > > #define PCI_ADDRESS_SPACE_MEM_PREFETCH 0x08 > > > > > > typedef struct PCIIORegion { > > > > -- > yamahata
On Tue, Oct 06, 2009 at 06:42:02PM +0900, Isaku Yamahata wrote: > On Mon, Oct 05, 2009 at 02:47:11PM +0200, Michael S. Tsirkin wrote: > > > @@ -462,7 +474,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_mem64(r)) { > > > + new_addr = pci_get_quad(d->config + config_ofs); > > > > From previous patch, config_ofs is region_num * 4 > > This is incorrect for 64 bit regions. > > I don't see any problems. In the following example, > 0, 2 ... will be used for region_number and 1 is left unused. > > BAR0 64bit memory > BAR1 used by BAR0 64bit > BAR2 ... Oh, I see. It's up to the user of pci_register_bar to allocate sane region numbers. Fine. Maybe add a comment before pci_register_bar about legal values for region_num: not everyone knows about PCI internals. > -- > yamahata
diff --git a/hw/pci.c b/hw/pci.c index 21565f5..09a6816 100644 --- a/hw/pci.c +++ b/hw/pci.c @@ -395,6 +395,13 @@ int pci_unregister_device(PCIDevice *pci_dev) return 0; } +static inline int pci_bar_is_mem64(const PCIIORegion *r) +{ + return !(r->type & PCI_ADDRESS_SPACE_IO) && + (r->type & PCI_ADDRESS_SPACE_MEM_TYPE_MASK) == + PCI_ADDRESS_SPACE_MEM_TYPE_64; +} + void pci_register_bar(PCIDevice *pci_dev, int region_num, pcibus_t size, int type, PCIMapIORegionFunc *map_func) @@ -427,8 +434,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_mem64(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 +474,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_mem64(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; @@ -473,11 +489,24 @@ static void pci_update_mappings(PCIDevice *d) mappings, we handle specific values as invalid mappings. */ if (last_addr <= new_addr || new_addr == 0 || - last_addr == PCI_BAR_UNMAPPED || + last_addr == PCI_BAR_UNMAPPED) { + new_addr = PCI_BAR_UNMAPPED; + } - /* keep old behaviour - * without this, PC ide doesn't work well. */ - last_addr >= UINT32_MAX) { + /* + * work around: without this, PC ide and other devices + * don't work well. + * OS writes all 1 bits to 32 BAR to find its size + * resulting in setting UINT32_MAX - alignemnt, + * and then OS sets the BAR to where they really want + * the BAR to sit. + * On the other hand, there are some important areas + * blow 4G on i386/x86_64. So setting BAR over those area + * below 4G causes troubles. + * We work around the issues by prohibitting BAR + * from sitting right blow 4G. + */ + if (!pci_bar_is_mem64(r) && last_addr >= UINT32_MAX) { new_addr = PCI_BAR_UNMAPPED; } } else { @@ -736,7 +765,16 @@ 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 = pci_bar_is_mem64(r)? "64 bit": "32 bit"; + const char *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 cbf80c0..b65ce03 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_TYPE_MASK 0x06 +#define PCI_ADDRESS_SPACE_MEM_TYPE_64 0x04 /* 64 bit address */ #define PCI_ADDRESS_SPACE_MEM_PREFETCH 0x08 typedef struct PCIIORegion {
implemented pci 64bit bar support. Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> --- hw/pci.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++-------- hw/pci.h | 2 ++ 2 files changed, 48 insertions(+), 8 deletions(-)