| Message ID | 1255069742-15724-17-git-send-email-yamahata@valinux.co.jp |
|---|---|
| State | Under Review |
| Headers | show |
On Fri, Oct 09, 2009 at 03:28:49PM +0900, Isaku Yamahata wrote: > implemented pci 64bit bar support. > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > --- > hw/pci.c | 34 +++++++++++++++++++++++++++++----- > hw/pci.h | 2 ++ > 2 files changed, 31 insertions(+), 5 deletions(-) > > diff --git a/hw/pci.c b/hw/pci.c > index e7f8fb4..ee3ab05 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -454,6 +454,13 @@ static int pci_unregister_device(DeviceState *dev) > return 0; > } > > +static inline int pci_bar_is_mem64(const PCIIORegion *r) > +{ > + return !(r->type & PCI_BASE_ADDRESS_SPACE_IO) && > + (r->type & PCI_BASE_ADDRESS_MEM_TYPE_MASK) == > + PCI_BASE_ADDRESS_MEM_TYPE_64; > +} > + This should be just return r->type & PCI_BASE_ADDRESS_MEM_TYPE_64 and then this can be open-coded. PCI_BASE_ADDRESS_MEM_TYPE_MASK will also not be needed. I have a feeling I said this already, and the spec is pretty clear: there is exactly 1 bit that means 64 bit memory. No reason to check anything else. > void pci_register_bar(PCIDevice *pci_dev, int region_num, > pcibus_t size, int type, > PCIMapIORegionFunc *map_func) > @@ -484,8 +491,13 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num, > wmask |= PCI_ROM_ADDRESS_ENABLE; > } > 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) > @@ -513,7 +525,11 @@ static void pci_update_mappings(PCIDevice *d) > } > } else { > if (cmd & PCI_COMMAND_MEMORY) { > - new_addr = pci_get_long(d->config + pci_bar(d, i)); > + if (pci_bar_is_mem64(r)) { > + new_addr = pci_get_quad(d->config + pci_bar(d, i)); > + } else { > + new_addr = pci_get_long(d->config + pci_bar(d, i)); > + } > /* the ROM slot has a specific enable bit */ > if (i == PCI_ROM_SLOT && !(new_addr & PCI_ROM_ADDRESS_ENABLE)) > goto no_mem_map; > @@ -531,7 +547,7 @@ static void pci_update_mappings(PCIDevice *d) > * Without this, PC ide doesn't work well. > * TODO: remove this work around. > */ > - last_addr >= UINT32_MAX) { > + (!pci_bar_is_mem64(r) && last_addr >= UINT32_MAX)) { > new_addr = PCI_BAR_UNMAPPED; > } > } else { > @@ -773,8 +789,16 @@ static void pci_info_device(PCIDevice *d) > " [0x%04"FMT_PCIBUS"].\n", > r->addr, r->addr + r->size - 1); > } else { > - monitor_printf(mon, "32 bit memory at 0x%08"FMT_PCIBUS > + const char *type = pci_bar_is_mem64(r)? "64 bit": "32 bit"; > + const char *prefetch = ""; > + > + if (r->type & PCI_BASE_ADDRESS_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 8fbd45e..269981a 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -114,6 +114,8 @@ typedef struct PCIIORegion { > #define PCI_BASE_ADDRESS_0 0x10 /* 32 bits */ > #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_64 0x04 /* 64 bit address */ > #define PCI_BASE_ADDRESS_MEM_PREFETCH 0x08 /* prefetchable? */ > #define PCI_PRIMARY_BUS 0x18 /* Primary bus number */ > #define PCI_SECONDARY_BUS 0x19 /* Secondary bus number */ > -- > 1.6.0.2
On Sat, Oct 10, 2009 at 09:39:29PM +0200, Michael S. Tsirkin wrote: > On Fri, Oct 09, 2009 at 03:28:49PM +0900, Isaku Yamahata wrote: > > implemented pci 64bit bar support. > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > > --- > > hw/pci.c | 34 +++++++++++++++++++++++++++++----- > > hw/pci.h | 2 ++ > > 2 files changed, 31 insertions(+), 5 deletions(-) > > > > diff --git a/hw/pci.c b/hw/pci.c > > index e7f8fb4..ee3ab05 100644 > > --- a/hw/pci.c > > +++ b/hw/pci.c > > @@ -454,6 +454,13 @@ static int pci_unregister_device(DeviceState *dev) > > return 0; > > } > > > > +static inline int pci_bar_is_mem64(const PCIIORegion *r) > > +{ > > + return !(r->type & PCI_BASE_ADDRESS_SPACE_IO) && > > + (r->type & PCI_BASE_ADDRESS_MEM_TYPE_MASK) == > > + PCI_BASE_ADDRESS_MEM_TYPE_64; > > +} > > + > > This should be just > return r->type & PCI_BASE_ADDRESS_MEM_TYPE_64 > and then this can be open-coded. > PCI_BASE_ADDRESS_MEM_TYPE_MASK will also not be needed. r->type & PCI_BASE_ADDRESS_MEM_TYPE_64 can be true for IO space. For example, bar of IO space base with size 4 is 0x5. Thus PCI_BASE_ADDRESS_SPACE_IO check is necessary. > I have a feeling I said this already, and the spec is pretty > clear: there is exactly 1 bit that means 64 bit memory. > No reason to check anything else. It isn't to me because the spec says that (Table 6-4 page 226) bit[2:3] 00 32bit 01 reserved(obsolete below 1M) 10 64bit 11 reserved Although 11 isn't likely to be used in the future, it is safe to use mask. > > void pci_register_bar(PCIDevice *pci_dev, int region_num, > > pcibus_t size, int type, > > PCIMapIORegionFunc *map_func) > > @@ -484,8 +491,13 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num, > > wmask |= PCI_ROM_ADDRESS_ENABLE; > > } > > 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) > > @@ -513,7 +525,11 @@ static void pci_update_mappings(PCIDevice *d) > > } > > } else { > > if (cmd & PCI_COMMAND_MEMORY) { > > - new_addr = pci_get_long(d->config + pci_bar(d, i)); > > + if (pci_bar_is_mem64(r)) { > > + new_addr = pci_get_quad(d->config + pci_bar(d, i)); > > + } else { > > + new_addr = pci_get_long(d->config + pci_bar(d, i)); > > + } > > /* the ROM slot has a specific enable bit */ > > if (i == PCI_ROM_SLOT && !(new_addr & PCI_ROM_ADDRESS_ENABLE)) > > goto no_mem_map; > > @@ -531,7 +547,7 @@ static void pci_update_mappings(PCIDevice *d) > > * Without this, PC ide doesn't work well. > > * TODO: remove this work around. > > */ > > - last_addr >= UINT32_MAX) { > > + (!pci_bar_is_mem64(r) && last_addr >= UINT32_MAX)) { > > new_addr = PCI_BAR_UNMAPPED; > > } > > } else { > > @@ -773,8 +789,16 @@ static void pci_info_device(PCIDevice *d) > > " [0x%04"FMT_PCIBUS"].\n", > > r->addr, r->addr + r->size - 1); > > } else { > > - monitor_printf(mon, "32 bit memory at 0x%08"FMT_PCIBUS > > + const char *type = pci_bar_is_mem64(r)? "64 bit": "32 bit"; > > + const char *prefetch = ""; > > + > > + if (r->type & PCI_BASE_ADDRESS_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 8fbd45e..269981a 100644 > > --- a/hw/pci.h > > +++ b/hw/pci.h > > @@ -114,6 +114,8 @@ typedef struct PCIIORegion { > > #define PCI_BASE_ADDRESS_0 0x10 /* 32 bits */ > > #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_64 0x04 /* 64 bit address */ > > #define PCI_BASE_ADDRESS_MEM_PREFETCH 0x08 /* prefetchable? */ > > #define PCI_PRIMARY_BUS 0x18 /* Primary bus number */ > > #define PCI_SECONDARY_BUS 0x19 /* Secondary bus number */ >
On Tue, Oct 13, 2009 at 10:52:54PM +0900, Isaku Yamahata wrote: > On Sat, Oct 10, 2009 at 09:39:29PM +0200, Michael S. Tsirkin wrote: > > On Fri, Oct 09, 2009 at 03:28:49PM +0900, Isaku Yamahata wrote: > > > implemented pci 64bit bar support. > > > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > > > --- > > > hw/pci.c | 34 +++++++++++++++++++++++++++++----- > > > hw/pci.h | 2 ++ > > > 2 files changed, 31 insertions(+), 5 deletions(-) > > > > > > diff --git a/hw/pci.c b/hw/pci.c > > > index e7f8fb4..ee3ab05 100644 > > > --- a/hw/pci.c > > > +++ b/hw/pci.c > > > @@ -454,6 +454,13 @@ static int pci_unregister_device(DeviceState *dev) > > > return 0; > > > } > > > > > > +static inline int pci_bar_is_mem64(const PCIIORegion *r) > > > +{ > > > + return !(r->type & PCI_BASE_ADDRESS_SPACE_IO) && > > > + (r->type & PCI_BASE_ADDRESS_MEM_TYPE_MASK) == > > > + PCI_BASE_ADDRESS_MEM_TYPE_64; > > > +} > > > + > > > > This should be just > > return r->type & PCI_BASE_ADDRESS_MEM_TYPE_64 Or r->type == PCI_BASE_ADDRESS_MEM_TYPE_64 if you like that more. > > and then this can be open-coded. which is the important bit :) > > PCI_BASE_ADDRESS_MEM_TYPE_MASK will also not be needed. > > r->type & PCI_BASE_ADDRESS_MEM_TYPE_64 can be true for IO space. > For example, bar of IO space base with size 4 is 0x5. I don't think we ever leak bits from BAR into r->type in this way. Do we? > Thus PCI_BASE_ADDRESS_SPACE_IO check is necessary. > > > > I have a feeling I said this already, and the spec is pretty > > clear: there is exactly 1 bit that means 64 bit memory. > > No reason to check anything else. > > It isn't to me because the spec says that (Table 6-4 page 226) > bit[2:3] > 00 32bit > 01 reserved(obsolete below 1M) > 10 64bit > 11 reserved > > Although 11 isn't likely to be used in the future, it is safe to use mask. Well, if you care about this, I would say the right place to check for unknown region types is when registering them, not at a random time when we use them later. > > > void pci_register_bar(PCIDevice *pci_dev, int region_num, > > > pcibus_t size, int type, > > > PCIMapIORegionFunc *map_func) > > > @@ -484,8 +491,13 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num, > > > wmask |= PCI_ROM_ADDRESS_ENABLE; > > > } > > > 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) > > > @@ -513,7 +525,11 @@ static void pci_update_mappings(PCIDevice *d) > > > } > > > } else { > > > if (cmd & PCI_COMMAND_MEMORY) { > > > - new_addr = pci_get_long(d->config + pci_bar(d, i)); > > > + if (pci_bar_is_mem64(r)) { > > > + new_addr = pci_get_quad(d->config + pci_bar(d, i)); > > > + } else { > > > + new_addr = pci_get_long(d->config + pci_bar(d, i)); > > > + } > > > /* the ROM slot has a specific enable bit */ > > > if (i == PCI_ROM_SLOT && !(new_addr & PCI_ROM_ADDRESS_ENABLE)) > > > goto no_mem_map; > > > @@ -531,7 +547,7 @@ static void pci_update_mappings(PCIDevice *d) > > > * Without this, PC ide doesn't work well. > > > * TODO: remove this work around. > > > */ > > > - last_addr >= UINT32_MAX) { > > > + (!pci_bar_is_mem64(r) && last_addr >= UINT32_MAX)) { > > > new_addr = PCI_BAR_UNMAPPED; > > > } > > > } else { > > > @@ -773,8 +789,16 @@ static void pci_info_device(PCIDevice *d) > > > " [0x%04"FMT_PCIBUS"].\n", > > > r->addr, r->addr + r->size - 1); > > > } else { > > > - monitor_printf(mon, "32 bit memory at 0x%08"FMT_PCIBUS > > > + const char *type = pci_bar_is_mem64(r)? "64 bit": "32 bit"; > > > + const char *prefetch = ""; > > > + > > > + if (r->type & PCI_BASE_ADDRESS_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 8fbd45e..269981a 100644 > > > --- a/hw/pci.h > > > +++ b/hw/pci.h > > > @@ -114,6 +114,8 @@ typedef struct PCIIORegion { > > > #define PCI_BASE_ADDRESS_0 0x10 /* 32 bits */ > > > #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_64 0x04 /* 64 bit address */ > > > #define PCI_BASE_ADDRESS_MEM_PREFETCH 0x08 /* prefetchable? */ > > > #define PCI_PRIMARY_BUS 0x18 /* Primary bus number */ > > > #define PCI_SECONDARY_BUS 0x19 /* Secondary bus number */ > > > > -- > yamahata
diff --git a/hw/pci.c b/hw/pci.c index e7f8fb4..ee3ab05 100644 --- a/hw/pci.c +++ b/hw/pci.c @@ -454,6 +454,13 @@ static int pci_unregister_device(DeviceState *dev) return 0; } +static inline int pci_bar_is_mem64(const PCIIORegion *r) +{ + return !(r->type & PCI_BASE_ADDRESS_SPACE_IO) && + (r->type & PCI_BASE_ADDRESS_MEM_TYPE_MASK) == + PCI_BASE_ADDRESS_MEM_TYPE_64; +} + void pci_register_bar(PCIDevice *pci_dev, int region_num, pcibus_t size, int type, PCIMapIORegionFunc *map_func) @@ -484,8 +491,13 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num, wmask |= PCI_ROM_ADDRESS_ENABLE; } 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) @@ -513,7 +525,11 @@ static void pci_update_mappings(PCIDevice *d) } } else { if (cmd & PCI_COMMAND_MEMORY) { - new_addr = pci_get_long(d->config + pci_bar(d, i)); + if (pci_bar_is_mem64(r)) { + new_addr = pci_get_quad(d->config + pci_bar(d, i)); + } else { + new_addr = pci_get_long(d->config + pci_bar(d, i)); + } /* the ROM slot has a specific enable bit */ if (i == PCI_ROM_SLOT && !(new_addr & PCI_ROM_ADDRESS_ENABLE)) goto no_mem_map; @@ -531,7 +547,7 @@ static void pci_update_mappings(PCIDevice *d) * Without this, PC ide doesn't work well. * TODO: remove this work around. */ - last_addr >= UINT32_MAX) { + (!pci_bar_is_mem64(r) && last_addr >= UINT32_MAX)) { new_addr = PCI_BAR_UNMAPPED; } } else { @@ -773,8 +789,16 @@ static void pci_info_device(PCIDevice *d) " [0x%04"FMT_PCIBUS"].\n", r->addr, r->addr + r->size - 1); } else { - monitor_printf(mon, "32 bit memory at 0x%08"FMT_PCIBUS + const char *type = pci_bar_is_mem64(r)? "64 bit": "32 bit"; + const char *prefetch = ""; + + if (r->type & PCI_BASE_ADDRESS_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 8fbd45e..269981a 100644 --- a/hw/pci.h +++ b/hw/pci.h @@ -114,6 +114,8 @@ typedef struct PCIIORegion { #define PCI_BASE_ADDRESS_0 0x10 /* 32 bits */ #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_64 0x04 /* 64 bit address */ #define PCI_BASE_ADDRESS_MEM_PREFETCH 0x08 /* prefetchable? */ #define PCI_PRIMARY_BUS 0x18 /* Primary bus number */ #define PCI_SECONDARY_BUS 0x19 /* Secondary bus number */
implemented pci 64bit bar support. Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> --- hw/pci.c | 34 +++++++++++++++++++++++++++++----- hw/pci.h | 2 ++ 2 files changed, 31 insertions(+), 5 deletions(-)