| Message ID | 20170306200622.GA27507@dhcp-27-118.brq.redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Alex, This should be its own patch, the series you've added it to was already committed. On Mon, Mar 06, 2017 at 09:06:23PM +0100, Alexander Gordeev wrote: > Cc: Thomas Huth <thuth@redhat.com> > Cc: Andrew Jones <drjones@redhat.com> > Cc: Peter Xu <peterx@redhat.com> > Signed-off-by: Alexander Gordeev <agordeev@redhat.com> > --- > lib/pci.c | 54 ++++++++++++++++++++++++++++++++++++++++-------------- > lib/pci.h | 21 +++++++++++++-------- > 2 files changed, 53 insertions(+), 22 deletions(-) > > diff --git a/lib/pci.c b/lib/pci.c > index daf398100b7e..98fc3849d297 100644 > --- a/lib/pci.c > +++ b/lib/pci.c > @@ -110,13 +110,14 @@ uint32_t pci_bar_mask(uint32_t bar) > PCI_BASE_ADDRESS_IO_MASK : PCI_BASE_ADDRESS_MEM_MASK; > } > > -uint32_t pci_bar_get(struct pci_dev *dev, int bar_num) > +uint32_t pci_bar_get(struct pci_dev *dev, unsigned int bar_num) > { > - return pci_config_readl(dev->bdf, PCI_BASE_ADDRESS_0 + > - bar_num * 4); > + CHECK_BAR_NUM(bar_num); > + > + return pci_config_readl(dev->bdf, PCI_BASE_ADDRESS_0 + bar_num * 4); > } > > -static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, int bar_num) > +static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, unsigned int bar_num) > { > uint32_t bar = pci_bar_get(dev, bar_num); > uint32_t mask = pci_bar_mask(bar); > @@ -132,15 +133,26 @@ static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, int bar_num) > return phys_addr; > } > > -phys_addr_t pci_bar_get_addr(struct pci_dev *dev, int bar_num) > +phys_addr_t pci_bar_get_addr(struct pci_dev *dev, unsigned int bar_num) > { > + CHECK_BAR_NUM(bar_num); > + > return dev->resource[bar_num]; > } > > -void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr) > +void pci_bar_set_addr(struct pci_dev *dev, > + unsigned int bar_num, phys_addr_t addr) > { > int off = PCI_BASE_ADDRESS_0 + bar_num * 4; > > + CHECK_BAR_NUM(bar_num); > + assert(addr != INVALID_PHYS_ADDR); Shouldn't this be something like assert((addr & PHYS_MASK) == addr) But that means we need to ensure all architectures define PHYS_MASK... > + assert(dev->resource[bar_num] != INVALID_PHYS_ADDR); > + if (pci_bar_is64(dev, bar_num)) { > + assert(bar_num + 1 < PCI_BAR_NUM); Use CHECK_BAR_NUM(bar_num + 1) > + assert(dev->resource[bar_num] == dev->resource[bar_num + 1]); Can this ever happen without the unit test explicitly messing things up? > + } > + > pci_config_writel(dev->bdf, off, (uint32_t)addr); > dev->resource[bar_num] = addr; > > @@ -161,7 +173,7 @@ void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr) > * The following pci_bar_size_helper() and pci_bar_size() functions > * implement the algorithm. > */ > -static uint32_t pci_bar_size_helper(struct pci_dev *dev, int bar_num) > +static uint32_t pci_bar_size_helper(struct pci_dev *dev, unsigned int bar_num) > { > int off = PCI_BASE_ADDRESS_0 + bar_num * 4; > uint16_t bdf = dev->bdf; > @@ -175,10 +187,12 @@ static uint32_t pci_bar_size_helper(struct pci_dev *dev, int bar_num) > return val; > } > > -phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num) > +phys_addr_t pci_bar_size(struct pci_dev *dev, unsigned int bar_num) > { > uint32_t bar, size; > > + CHECK_BAR_NUM(bar_num); > + > size = pci_bar_size_helper(dev, bar_num); > if (!size) > return 0; > @@ -196,21 +210,31 @@ phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num) > } > } > > -bool pci_bar_is_memory(struct pci_dev *dev, int bar_num) > +bool pci_bar_is_memory(struct pci_dev *dev, unsigned int bar_num) > { > - uint32_t bar = pci_bar_get(dev, bar_num); > + uint32_t bar; > + > + CHECK_BAR_NUM(bar_num); > + > + bar = pci_bar_get(dev, bar_num); This hunk is unnecessary, pci_bar_get already calls CHECK_BAR_NUM > > return !(bar & PCI_BASE_ADDRESS_SPACE_IO); > } > > -bool pci_bar_is_valid(struct pci_dev *dev, int bar_num) > +bool pci_bar_is_valid(struct pci_dev *dev, unsigned int bar_num) > { > + CHECK_BAR_NUM(bar_num); > + > return dev->resource[bar_num] != INVALID_PHYS_ADDR; > } > > -bool pci_bar_is64(struct pci_dev *dev, int bar_num) > +bool pci_bar_is64(struct pci_dev *dev, unsigned int bar_num) > { > - uint32_t bar = pci_bar_get(dev, bar_num); > + uint32_t bar; > + > + CHECK_BAR_NUM(bar_num); > + > + bar = pci_bar_get(dev, bar_num); Same as above (unnecessary hunk) > > if (bar & PCI_BASE_ADDRESS_SPACE_IO) > return false; > @@ -219,11 +243,13 @@ bool pci_bar_is64(struct pci_dev *dev, int bar_num) > PCI_BASE_ADDRESS_MEM_TYPE_64; > } > > -void pci_bar_print(struct pci_dev *dev, int bar_num) > +void pci_bar_print(struct pci_dev *dev, unsigned int bar_num) > { > phys_addr_t size, start, end; > uint32_t bar; > > + CHECK_BAR_NUM(bar_num); > + Unnecessary, pci_bar_is_valid already checks > if (!pci_bar_is_valid(dev, bar_num)) > return; > > diff --git a/lib/pci.h b/lib/pci.h > index 03cc0a72d48d..c770def840da 100644 > --- a/lib/pci.h > +++ b/lib/pci.h > @@ -18,6 +18,9 @@ enum { > #define PCI_BAR_NUM 6 > #define PCI_DEVFN_MAX 256 > > +#define CHECK_BAR_NUM(bar_num) \ > + do { assert(bar_num < PCI_BAR_NUM); } while (0) Just add 'bar_num >= 0 &&' to this macro and we don't need to change bar_num to unsigned everywhere > + > #define PCI_BDF_GET_DEVFN(x) ((x) & 0xff) > #define PCI_BDF_GET_BUS(x) (((x) >> 8) & 0xff) > > @@ -54,15 +57,17 @@ extern pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id); > * It is expected the caller is aware of the device BAR layout and never > * tries to address the middle of a 64-bit register. > */ > -extern phys_addr_t pci_bar_get_addr(struct pci_dev *dev, int bar_num); > -extern void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr); > -extern phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num); > -extern uint32_t pci_bar_get(struct pci_dev *dev, int bar_num); > + > +extern phys_addr_t pci_bar_get_addr(struct pci_dev *dev, unsigned int bar_num); > +extern void pci_bar_set_addr(struct pci_dev *dev, unsigned int bar_num, > + phys_addr_t addr); > +extern phys_addr_t pci_bar_size(struct pci_dev *dev, unsigned int bar_num); > +extern uint32_t pci_bar_get(struct pci_dev *dev, unsigned int bar_num); > extern uint32_t pci_bar_mask(uint32_t bar); > -extern bool pci_bar_is64(struct pci_dev *dev, int bar_num); > -extern bool pci_bar_is_memory(struct pci_dev *dev, int bar_num); > -extern bool pci_bar_is_valid(struct pci_dev *dev, int bar_num); > -extern void pci_bar_print(struct pci_dev *dev, int bar_num); > +extern bool pci_bar_is64(struct pci_dev *dev, unsigned int bar_num); > +extern bool pci_bar_is_memory(struct pci_dev *dev, unsigned int bar_num); > +extern bool pci_bar_is_valid(struct pci_dev *dev, unsigned int bar_num); > +extern void pci_bar_print(struct pci_dev *dev, unsigned int bar_num); > extern void pci_dev_print_id(struct pci_dev *dev); > extern void pci_dev_print(struct pci_dev *dev); > extern uint8_t pci_intx_line(struct pci_dev *dev); > -- > 1.8.3.1 > Thanks, drew
On Tue, Mar 07, 2017 at 03:22:53PM +0100, Andrew Jones wrote: > Hi Alex, > > This should be its own patch, the series you've added it to was > already committed. > > On Mon, Mar 06, 2017 at 09:06:23PM +0100, Alexander Gordeev wrote: > > Cc: Thomas Huth <thuth@redhat.com> > > Cc: Andrew Jones <drjones@redhat.com> > > Cc: Peter Xu <peterx@redhat.com> > > Signed-off-by: Alexander Gordeev <agordeev@redhat.com> > > --- > > lib/pci.c | 54 ++++++++++++++++++++++++++++++++++++++++-------------- > > lib/pci.h | 21 +++++++++++++-------- > > 2 files changed, 53 insertions(+), 22 deletions(-) > > > > diff --git a/lib/pci.c b/lib/pci.c > > index daf398100b7e..98fc3849d297 100644 > > --- a/lib/pci.c > > +++ b/lib/pci.c > > @@ -110,13 +110,14 @@ uint32_t pci_bar_mask(uint32_t bar) > > PCI_BASE_ADDRESS_IO_MASK : PCI_BASE_ADDRESS_MEM_MASK; > > } > > > > -uint32_t pci_bar_get(struct pci_dev *dev, int bar_num) > > +uint32_t pci_bar_get(struct pci_dev *dev, unsigned int bar_num) > > { > > - return pci_config_readl(dev->bdf, PCI_BASE_ADDRESS_0 + > > - bar_num * 4); > > + CHECK_BAR_NUM(bar_num); > > + > > + return pci_config_readl(dev->bdf, PCI_BASE_ADDRESS_0 + bar_num * 4); > > } > > > > -static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, int bar_num) > > +static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, unsigned int bar_num) > > { > > uint32_t bar = pci_bar_get(dev, bar_num); > > uint32_t mask = pci_bar_mask(bar); > > @@ -132,15 +133,26 @@ static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, int bar_num) > > return phys_addr; > > } > > > > -phys_addr_t pci_bar_get_addr(struct pci_dev *dev, int bar_num) > > +phys_addr_t pci_bar_get_addr(struct pci_dev *dev, unsigned int bar_num) > > { > > + CHECK_BAR_NUM(bar_num); > > + > > return dev->resource[bar_num]; > > } > > > > -void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr) > > +void pci_bar_set_addr(struct pci_dev *dev, > > + unsigned int bar_num, phys_addr_t addr) > > { > > int off = PCI_BASE_ADDRESS_0 + bar_num * 4; > > > > + CHECK_BAR_NUM(bar_num); > > + assert(addr != INVALID_PHYS_ADDR); > > Shouldn't this be something like > > assert((addr & PHYS_MASK) == addr) Nope. At this stage PCI bus is scanned and pci_dev::resource[] is initialized with proper values. A value of INVALID_PHYS_ADDR in dev->resource[bar_num] indicates BAR #bar_num is not implemented. Thus, we do not want to screw up this assumption with a test case trying to write INVALID_PHYS_ADDR to a the BAR. > But that means we need to ensure all architectures define PHYS_MASK... > > > + assert(dev->resource[bar_num] != INVALID_PHYS_ADDR); > > + if (pci_bar_is64(dev, bar_num)) { > > + assert(bar_num + 1 < PCI_BAR_NUM); > > Use CHECK_BAR_NUM(bar_num + 1) > > > + assert(dev->resource[bar_num] == dev->resource[bar_num + 1]); > > Can this ever happen without the unit test explicitly messing things up? > > > + } > > + > > pci_config_writel(dev->bdf, off, (uint32_t)addr); > > dev->resource[bar_num] = addr; > > > > @@ -161,7 +173,7 @@ void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr) > > * The following pci_bar_size_helper() and pci_bar_size() functions > > * implement the algorithm. > > */ > > -static uint32_t pci_bar_size_helper(struct pci_dev *dev, int bar_num) > > +static uint32_t pci_bar_size_helper(struct pci_dev *dev, unsigned int bar_num) > > { > > int off = PCI_BASE_ADDRESS_0 + bar_num * 4; > > uint16_t bdf = dev->bdf; > > @@ -175,10 +187,12 @@ static uint32_t pci_bar_size_helper(struct pci_dev *dev, int bar_num) > > return val; > > } > > > > -phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num) > > +phys_addr_t pci_bar_size(struct pci_dev *dev, unsigned int bar_num) > > { > > uint32_t bar, size; > > > > + CHECK_BAR_NUM(bar_num); > > + > > size = pci_bar_size_helper(dev, bar_num); > > if (!size) > > return 0; > > @@ -196,21 +210,31 @@ phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num) > > } > > } > > > > -bool pci_bar_is_memory(struct pci_dev *dev, int bar_num) > > +bool pci_bar_is_memory(struct pci_dev *dev, unsigned int bar_num) > > { > > - uint32_t bar = pci_bar_get(dev, bar_num); > > + uint32_t bar; > > + > > + CHECK_BAR_NUM(bar_num); > > + > > + bar = pci_bar_get(dev, bar_num); > > This hunk is unnecessary, pci_bar_get already calls CHECK_BAR_NUM Right, it is superfluous. I put it here intentionally to avoid the check to go away in case pci_bar_get() is changed somehow. Pure paranoia. I can remove it if you want here and elsewhere. > > return !(bar & PCI_BASE_ADDRESS_SPACE_IO); > > } > > > > -bool pci_bar_is_valid(struct pci_dev *dev, int bar_num) > > +bool pci_bar_is_valid(struct pci_dev *dev, unsigned int bar_num) > > { > > + CHECK_BAR_NUM(bar_num); > > + > > return dev->resource[bar_num] != INVALID_PHYS_ADDR; > > } > > > > -bool pci_bar_is64(struct pci_dev *dev, int bar_num) > > +bool pci_bar_is64(struct pci_dev *dev, unsigned int bar_num) > > { > > - uint32_t bar = pci_bar_get(dev, bar_num); > > + uint32_t bar; > > + > > + CHECK_BAR_NUM(bar_num); > > + > > + bar = pci_bar_get(dev, bar_num); > > Same as above (unnecessary hunk) > > > > > if (bar & PCI_BASE_ADDRESS_SPACE_IO) > > return false; > > @@ -219,11 +243,13 @@ bool pci_bar_is64(struct pci_dev *dev, int bar_num) > > PCI_BASE_ADDRESS_MEM_TYPE_64; > > } > > > > -void pci_bar_print(struct pci_dev *dev, int bar_num) > > +void pci_bar_print(struct pci_dev *dev, unsigned int bar_num) > > { > > phys_addr_t size, start, end; > > uint32_t bar; > > > > + CHECK_BAR_NUM(bar_num); > > + > > Unnecessary, pci_bar_is_valid already checks > > > if (!pci_bar_is_valid(dev, bar_num)) > > return; > > > > diff --git a/lib/pci.h b/lib/pci.h > > index 03cc0a72d48d..c770def840da 100644 > > --- a/lib/pci.h > > +++ b/lib/pci.h > > @@ -18,6 +18,9 @@ enum { > > #define PCI_BAR_NUM 6 > > #define PCI_DEVFN_MAX 256 > > > > +#define CHECK_BAR_NUM(bar_num) \ > > + do { assert(bar_num < PCI_BAR_NUM); } while (0) > > Just add 'bar_num >= 0 &&' to this macro and we don't need to > change bar_num to unsigned everywhere > > > + > > #define PCI_BDF_GET_DEVFN(x) ((x) & 0xff) > > #define PCI_BDF_GET_BUS(x) (((x) >> 8) & 0xff) > > > > @@ -54,15 +57,17 @@ extern pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id); > > * It is expected the caller is aware of the device BAR layout and never > > * tries to address the middle of a 64-bit register. > > */ > > -extern phys_addr_t pci_bar_get_addr(struct pci_dev *dev, int bar_num); > > -extern void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr); > > -extern phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num); > > -extern uint32_t pci_bar_get(struct pci_dev *dev, int bar_num); > > + > > +extern phys_addr_t pci_bar_get_addr(struct pci_dev *dev, unsigned int bar_num); > > +extern void pci_bar_set_addr(struct pci_dev *dev, unsigned int bar_num, > > + phys_addr_t addr); > > +extern phys_addr_t pci_bar_size(struct pci_dev *dev, unsigned int bar_num); > > +extern uint32_t pci_bar_get(struct pci_dev *dev, unsigned int bar_num); > > extern uint32_t pci_bar_mask(uint32_t bar); > > -extern bool pci_bar_is64(struct pci_dev *dev, int bar_num); > > -extern bool pci_bar_is_memory(struct pci_dev *dev, int bar_num); > > -extern bool pci_bar_is_valid(struct pci_dev *dev, int bar_num); > > -extern void pci_bar_print(struct pci_dev *dev, int bar_num); > > +extern bool pci_bar_is64(struct pci_dev *dev, unsigned int bar_num); > > +extern bool pci_bar_is_memory(struct pci_dev *dev, unsigned int bar_num); > > +extern bool pci_bar_is_valid(struct pci_dev *dev, unsigned int bar_num); > > +extern void pci_bar_print(struct pci_dev *dev, unsigned int bar_num); > > extern void pci_dev_print_id(struct pci_dev *dev); > > extern void pci_dev_print(struct pci_dev *dev); > > extern uint8_t pci_intx_line(struct pci_dev *dev); > > -- > > 1.8.3.1 > > > > Thanks, > drew
On Tue, Mar 07, 2017 at 08:39:06PM +0100, Alexander Gordeev wrote: > On Tue, Mar 07, 2017 at 03:22:53PM +0100, Andrew Jones wrote: > > Hi Alex, > > > > This should be its own patch, the series you've added it to was > > already committed. > > > > On Mon, Mar 06, 2017 at 09:06:23PM +0100, Alexander Gordeev wrote: > > > Cc: Thomas Huth <thuth@redhat.com> > > > Cc: Andrew Jones <drjones@redhat.com> > > > Cc: Peter Xu <peterx@redhat.com> > > > Signed-off-by: Alexander Gordeev <agordeev@redhat.com> > > > --- > > > lib/pci.c | 54 ++++++++++++++++++++++++++++++++++++++++-------------- > > > lib/pci.h | 21 +++++++++++++-------- > > > 2 files changed, 53 insertions(+), 22 deletions(-) > > > > > > diff --git a/lib/pci.c b/lib/pci.c > > > index daf398100b7e..98fc3849d297 100644 > > > --- a/lib/pci.c > > > +++ b/lib/pci.c > > > @@ -110,13 +110,14 @@ uint32_t pci_bar_mask(uint32_t bar) > > > PCI_BASE_ADDRESS_IO_MASK : PCI_BASE_ADDRESS_MEM_MASK; > > > } > > > > > > -uint32_t pci_bar_get(struct pci_dev *dev, int bar_num) > > > +uint32_t pci_bar_get(struct pci_dev *dev, unsigned int bar_num) > > > { > > > - return pci_config_readl(dev->bdf, PCI_BASE_ADDRESS_0 + > > > - bar_num * 4); > > > + CHECK_BAR_NUM(bar_num); > > > + > > > + return pci_config_readl(dev->bdf, PCI_BASE_ADDRESS_0 + bar_num * 4); > > > } > > > > > > -static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, int bar_num) > > > +static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, unsigned int bar_num) > > > { > > > uint32_t bar = pci_bar_get(dev, bar_num); > > > uint32_t mask = pci_bar_mask(bar); > > > @@ -132,15 +133,26 @@ static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, int bar_num) > > > return phys_addr; > > > } > > > > > > -phys_addr_t pci_bar_get_addr(struct pci_dev *dev, int bar_num) > > > +phys_addr_t pci_bar_get_addr(struct pci_dev *dev, unsigned int bar_num) > > > { > > > + CHECK_BAR_NUM(bar_num); > > > + > > > return dev->resource[bar_num]; > > > } > > > > > > -void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr) > > > +void pci_bar_set_addr(struct pci_dev *dev, > > > + unsigned int bar_num, phys_addr_t addr) > > > { > > > int off = PCI_BASE_ADDRESS_0 + bar_num * 4; > > > > > > + CHECK_BAR_NUM(bar_num); > > > + assert(addr != INVALID_PHYS_ADDR); > > > > Shouldn't this be something like > > > > assert((addr & PHYS_MASK) == addr) > > Nope. At this stage PCI bus is scanned and pci_dev::resource[] > is initialized with proper values. A value of INVALID_PHYS_ADDR in > dev->resource[bar_num] indicates BAR #bar_num is not implemented. > Thus, we do not want to screw up this assumption with a test case > trying to write INVALID_PHYS_ADDR to a the BAR. Of course, by why would any other invalid phys addr be OK? Hmm, maybe it should be allowed for test case purposes. > > > But that means we need to ensure all architectures define PHYS_MASK... > > > > > + assert(dev->resource[bar_num] != INVALID_PHYS_ADDR); > > > + if (pci_bar_is64(dev, bar_num)) { > > > + assert(bar_num + 1 < PCI_BAR_NUM); > > > > Use CHECK_BAR_NUM(bar_num + 1) > > > > > + assert(dev->resource[bar_num] == dev->resource[bar_num + 1]); > > > > Can this ever happen without the unit test explicitly messing things up? No answer for this. I don't see the point of an assert that can never fail. > > > > > + } > > > + > > > pci_config_writel(dev->bdf, off, (uint32_t)addr); > > > dev->resource[bar_num] = addr; > > > > > > @@ -161,7 +173,7 @@ void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr) > > > * The following pci_bar_size_helper() and pci_bar_size() functions > > > * implement the algorithm. > > > */ > > > -static uint32_t pci_bar_size_helper(struct pci_dev *dev, int bar_num) > > > +static uint32_t pci_bar_size_helper(struct pci_dev *dev, unsigned int bar_num) > > > { > > > int off = PCI_BASE_ADDRESS_0 + bar_num * 4; > > > uint16_t bdf = dev->bdf; > > > @@ -175,10 +187,12 @@ static uint32_t pci_bar_size_helper(struct pci_dev *dev, int bar_num) > > > return val; > > > } > > > > > > -phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num) > > > +phys_addr_t pci_bar_size(struct pci_dev *dev, unsigned int bar_num) > > > { > > > uint32_t bar, size; > > > > > > + CHECK_BAR_NUM(bar_num); > > > + > > > size = pci_bar_size_helper(dev, bar_num); > > > if (!size) > > > return 0; > > > @@ -196,21 +210,31 @@ phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num) > > > } > > > } > > > > > > -bool pci_bar_is_memory(struct pci_dev *dev, int bar_num) > > > +bool pci_bar_is_memory(struct pci_dev *dev, unsigned int bar_num) > > > { > > > - uint32_t bar = pci_bar_get(dev, bar_num); > > > + uint32_t bar; > > > + > > > + CHECK_BAR_NUM(bar_num); > > > + > > > + bar = pci_bar_get(dev, bar_num); > > > > This hunk is unnecessary, pci_bar_get already calls CHECK_BAR_NUM > > Right, it is superfluous. I put it here intentionally to avoid > the check to go away in case pci_bar_get() is changed somehow. > Pure paranoia. I can remove it if you want here and elsewhere. I'd prefer it be removed. No need for the clutter. > > > > return !(bar & PCI_BASE_ADDRESS_SPACE_IO); > > > } > > > > > > -bool pci_bar_is_valid(struct pci_dev *dev, int bar_num) > > > +bool pci_bar_is_valid(struct pci_dev *dev, unsigned int bar_num) > > > { > > > + CHECK_BAR_NUM(bar_num); > > > + > > > return dev->resource[bar_num] != INVALID_PHYS_ADDR; > > > } > > > > > > -bool pci_bar_is64(struct pci_dev *dev, int bar_num) > > > +bool pci_bar_is64(struct pci_dev *dev, unsigned int bar_num) > > > { > > > - uint32_t bar = pci_bar_get(dev, bar_num); > > > + uint32_t bar; > > > + > > > + CHECK_BAR_NUM(bar_num); > > > + > > > + bar = pci_bar_get(dev, bar_num); > > > > Same as above (unnecessary hunk) > > > > > > > > if (bar & PCI_BASE_ADDRESS_SPACE_IO) > > > return false; > > > @@ -219,11 +243,13 @@ bool pci_bar_is64(struct pci_dev *dev, int bar_num) > > > PCI_BASE_ADDRESS_MEM_TYPE_64; > > > } > > > > > > -void pci_bar_print(struct pci_dev *dev, int bar_num) > > > +void pci_bar_print(struct pci_dev *dev, unsigned int bar_num) > > > { > > > phys_addr_t size, start, end; > > > uint32_t bar; > > > > > > + CHECK_BAR_NUM(bar_num); > > > + > > > > Unnecessary, pci_bar_is_valid already checks > > > > > if (!pci_bar_is_valid(dev, bar_num)) > > > return; > > > > > > diff --git a/lib/pci.h b/lib/pci.h > > > index 03cc0a72d48d..c770def840da 100644 > > > --- a/lib/pci.h > > > +++ b/lib/pci.h > > > @@ -18,6 +18,9 @@ enum { > > > #define PCI_BAR_NUM 6 > > > #define PCI_DEVFN_MAX 256 > > > > > > +#define CHECK_BAR_NUM(bar_num) \ > > > + do { assert(bar_num < PCI_BAR_NUM); } while (0) > > > > Just add 'bar_num >= 0 &&' to this macro and we don't need to > > change bar_num to unsigned everywhere > > > > > + > > > #define PCI_BDF_GET_DEVFN(x) ((x) & 0xff) > > > #define PCI_BDF_GET_BUS(x) (((x) >> 8) & 0xff) > > > > > > @@ -54,15 +57,17 @@ extern pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id); > > > * It is expected the caller is aware of the device BAR layout and never > > > * tries to address the middle of a 64-bit register. > > > */ > > > -extern phys_addr_t pci_bar_get_addr(struct pci_dev *dev, int bar_num); > > > -extern void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr); > > > -extern phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num); > > > -extern uint32_t pci_bar_get(struct pci_dev *dev, int bar_num); > > > + > > > +extern phys_addr_t pci_bar_get_addr(struct pci_dev *dev, unsigned int bar_num); > > > +extern void pci_bar_set_addr(struct pci_dev *dev, unsigned int bar_num, > > > + phys_addr_t addr); > > > +extern phys_addr_t pci_bar_size(struct pci_dev *dev, unsigned int bar_num); > > > +extern uint32_t pci_bar_get(struct pci_dev *dev, unsigned int bar_num); > > > extern uint32_t pci_bar_mask(uint32_t bar); > > > -extern bool pci_bar_is64(struct pci_dev *dev, int bar_num); > > > -extern bool pci_bar_is_memory(struct pci_dev *dev, int bar_num); > > > -extern bool pci_bar_is_valid(struct pci_dev *dev, int bar_num); > > > -extern void pci_bar_print(struct pci_dev *dev, int bar_num); > > > +extern bool pci_bar_is64(struct pci_dev *dev, unsigned int bar_num); > > > +extern bool pci_bar_is_memory(struct pci_dev *dev, unsigned int bar_num); > > > +extern bool pci_bar_is_valid(struct pci_dev *dev, unsigned int bar_num); > > > +extern void pci_bar_print(struct pci_dev *dev, unsigned int bar_num); > > > extern void pci_dev_print_id(struct pci_dev *dev); > > > extern void pci_dev_print(struct pci_dev *dev); > > > extern uint8_t pci_intx_line(struct pci_dev *dev); > > > -- > > > 1.8.3.1 > > > > > > > Thanks, > > drew
On Wed, Mar 08, 2017 at 11:10:26AM +0100, Andrew Jones wrote: > > > > -void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr) > > > > +void pci_bar_set_addr(struct pci_dev *dev, > > > > + unsigned int bar_num, phys_addr_t addr) > > > > { > > > > int off = PCI_BASE_ADDRESS_0 + bar_num * 4; > > > > > > > > + CHECK_BAR_NUM(bar_num); > > > > + assert(addr != INVALID_PHYS_ADDR); > > > > > > Shouldn't this be something like > > > > > > assert((addr & PHYS_MASK) == addr) > > > > Nope. At this stage PCI bus is scanned and pci_dev::resource[] > > is initialized with proper values. A value of INVALID_PHYS_ADDR in > > dev->resource[bar_num] indicates BAR #bar_num is not implemented. > > Thus, we do not want to screw up this assumption with a test case > > trying to write INVALID_PHYS_ADDR to a the BAR. > > Of course, by why would any other invalid phys addr be OK? Hmm, maybe > it should be allowed for test case purposes. Yeah, I think it should be allowed. > > > But that means we need to ensure all architectures define PHYS_MASK... > > > > > > > + assert(dev->resource[bar_num] != INVALID_PHYS_ADDR); > > > > + if (pci_bar_is64(dev, bar_num)) { > > > > + assert(bar_num + 1 < PCI_BAR_NUM); > > > > > > Use CHECK_BAR_NUM(bar_num + 1) > > > > > > > + assert(dev->resource[bar_num] == dev->resource[bar_num + 1]); > > > > > > Can this ever happen without the unit test explicitly messing things up? No, unless there is really something wrong with the code. > No answer for this. I don't see the point of an assert that can never > fail. So this is code integrity assert, not test case input check. Do you want me to remove it? > > > Thanks, > > > drew
On Wed, Mar 08, 2017 at 12:40:59PM +0100, Alexander Gordeev wrote: > On Wed, Mar 08, 2017 at 11:10:26AM +0100, Andrew Jones wrote: > > > > > -void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr) > > > > > +void pci_bar_set_addr(struct pci_dev *dev, > > > > > + unsigned int bar_num, phys_addr_t addr) > > > > > { > > > > > int off = PCI_BASE_ADDRESS_0 + bar_num * 4; > > > > > > > > > > + CHECK_BAR_NUM(bar_num); > > > > > + assert(addr != INVALID_PHYS_ADDR); > > > > > > > > Shouldn't this be something like > > > > > > > > assert((addr & PHYS_MASK) == addr) > > > > > > Nope. At this stage PCI bus is scanned and pci_dev::resource[] > > > is initialized with proper values. A value of INVALID_PHYS_ADDR in > > > dev->resource[bar_num] indicates BAR #bar_num is not implemented. > > > Thus, we do not want to screw up this assumption with a test case > > > trying to write INVALID_PHYS_ADDR to a the BAR. > > > > Of course, by why would any other invalid phys addr be OK? Hmm, maybe > > it should be allowed for test case purposes. > > Yeah, I think it should be allowed. > > > > > But that means we need to ensure all architectures define PHYS_MASK... > > > > > > > > > + assert(dev->resource[bar_num] != INVALID_PHYS_ADDR); > > > > > + if (pci_bar_is64(dev, bar_num)) { > > > > > + assert(bar_num + 1 < PCI_BAR_NUM); > > > > > > > > Use CHECK_BAR_NUM(bar_num + 1) > > > > > > > > > + assert(dev->resource[bar_num] == dev->resource[bar_num + 1]); > > > > > > > > Can this ever happen without the unit test explicitly messing things up? > > No, unless there is really something wrong with the code. > > > No answer for this. I don't see the point of an assert that can never > > fail. > > So this is code integrity assert, not test case input check. > Do you want me to remove it? Yes please. We don't generally defend the lib code from itself. We use asserts for anything the user could influence and for some test case inputs, in order to help the test writer avoid going down obviously buggy paths. Although, like above, we also try to give the test writer freedom to do crazy things, as that's the whole point of a testsuite :-) Thanks, drew
diff --git a/lib/pci.c b/lib/pci.c index daf398100b7e..98fc3849d297 100644 --- a/lib/pci.c +++ b/lib/pci.c @@ -110,13 +110,14 @@ uint32_t pci_bar_mask(uint32_t bar) PCI_BASE_ADDRESS_IO_MASK : PCI_BASE_ADDRESS_MEM_MASK; } -uint32_t pci_bar_get(struct pci_dev *dev, int bar_num) +uint32_t pci_bar_get(struct pci_dev *dev, unsigned int bar_num) { - return pci_config_readl(dev->bdf, PCI_BASE_ADDRESS_0 + - bar_num * 4); + CHECK_BAR_NUM(bar_num); + + return pci_config_readl(dev->bdf, PCI_BASE_ADDRESS_0 + bar_num * 4); } -static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, int bar_num) +static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, unsigned int bar_num) { uint32_t bar = pci_bar_get(dev, bar_num); uint32_t mask = pci_bar_mask(bar); @@ -132,15 +133,26 @@ static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, int bar_num) return phys_addr; } -phys_addr_t pci_bar_get_addr(struct pci_dev *dev, int bar_num) +phys_addr_t pci_bar_get_addr(struct pci_dev *dev, unsigned int bar_num) { + CHECK_BAR_NUM(bar_num); + return dev->resource[bar_num]; } -void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr) +void pci_bar_set_addr(struct pci_dev *dev, + unsigned int bar_num, phys_addr_t addr) { int off = PCI_BASE_ADDRESS_0 + bar_num * 4; + CHECK_BAR_NUM(bar_num); + assert(addr != INVALID_PHYS_ADDR); + assert(dev->resource[bar_num] != INVALID_PHYS_ADDR); + if (pci_bar_is64(dev, bar_num)) { + assert(bar_num + 1 < PCI_BAR_NUM); + assert(dev->resource[bar_num] == dev->resource[bar_num + 1]); + } + pci_config_writel(dev->bdf, off, (uint32_t)addr); dev->resource[bar_num] = addr; @@ -161,7 +173,7 @@ void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr) * The following pci_bar_size_helper() and pci_bar_size() functions * implement the algorithm. */ -static uint32_t pci_bar_size_helper(struct pci_dev *dev, int bar_num) +static uint32_t pci_bar_size_helper(struct pci_dev *dev, unsigned int bar_num) { int off = PCI_BASE_ADDRESS_0 + bar_num * 4; uint16_t bdf = dev->bdf; @@ -175,10 +187,12 @@ static uint32_t pci_bar_size_helper(struct pci_dev *dev, int bar_num) return val; } -phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num) +phys_addr_t pci_bar_size(struct pci_dev *dev, unsigned int bar_num) { uint32_t bar, size; + CHECK_BAR_NUM(bar_num); + size = pci_bar_size_helper(dev, bar_num); if (!size) return 0; @@ -196,21 +210,31 @@ phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num) } } -bool pci_bar_is_memory(struct pci_dev *dev, int bar_num) +bool pci_bar_is_memory(struct pci_dev *dev, unsigned int bar_num) { - uint32_t bar = pci_bar_get(dev, bar_num); + uint32_t bar; + + CHECK_BAR_NUM(bar_num); + + bar = pci_bar_get(dev, bar_num); return !(bar & PCI_BASE_ADDRESS_SPACE_IO); } -bool pci_bar_is_valid(struct pci_dev *dev, int bar_num) +bool pci_bar_is_valid(struct pci_dev *dev, unsigned int bar_num) { + CHECK_BAR_NUM(bar_num); + return dev->resource[bar_num] != INVALID_PHYS_ADDR; } -bool pci_bar_is64(struct pci_dev *dev, int bar_num) +bool pci_bar_is64(struct pci_dev *dev, unsigned int bar_num) { - uint32_t bar = pci_bar_get(dev, bar_num); + uint32_t bar; + + CHECK_BAR_NUM(bar_num); + + bar = pci_bar_get(dev, bar_num); if (bar & PCI_BASE_ADDRESS_SPACE_IO) return false; @@ -219,11 +243,13 @@ bool pci_bar_is64(struct pci_dev *dev, int bar_num) PCI_BASE_ADDRESS_MEM_TYPE_64; } -void pci_bar_print(struct pci_dev *dev, int bar_num) +void pci_bar_print(struct pci_dev *dev, unsigned int bar_num) { phys_addr_t size, start, end; uint32_t bar; + CHECK_BAR_NUM(bar_num); + if (!pci_bar_is_valid(dev, bar_num)) return; diff --git a/lib/pci.h b/lib/pci.h index 03cc0a72d48d..c770def840da 100644 --- a/lib/pci.h +++ b/lib/pci.h @@ -18,6 +18,9 @@ enum { #define PCI_BAR_NUM 6 #define PCI_DEVFN_MAX 256 +#define CHECK_BAR_NUM(bar_num) \ + do { assert(bar_num < PCI_BAR_NUM); } while (0) + #define PCI_BDF_GET_DEVFN(x) ((x) & 0xff) #define PCI_BDF_GET_BUS(x) (((x) >> 8) & 0xff) @@ -54,15 +57,17 @@ extern pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id); * It is expected the caller is aware of the device BAR layout and never * tries to address the middle of a 64-bit register. */ -extern phys_addr_t pci_bar_get_addr(struct pci_dev *dev, int bar_num); -extern void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr); -extern phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num); -extern uint32_t pci_bar_get(struct pci_dev *dev, int bar_num); + +extern phys_addr_t pci_bar_get_addr(struct pci_dev *dev, unsigned int bar_num); +extern void pci_bar_set_addr(struct pci_dev *dev, unsigned int bar_num, + phys_addr_t addr); +extern phys_addr_t pci_bar_size(struct pci_dev *dev, unsigned int bar_num); +extern uint32_t pci_bar_get(struct pci_dev *dev, unsigned int bar_num); extern uint32_t pci_bar_mask(uint32_t bar); -extern bool pci_bar_is64(struct pci_dev *dev, int bar_num); -extern bool pci_bar_is_memory(struct pci_dev *dev, int bar_num); -extern bool pci_bar_is_valid(struct pci_dev *dev, int bar_num); -extern void pci_bar_print(struct pci_dev *dev, int bar_num); +extern bool pci_bar_is64(struct pci_dev *dev, unsigned int bar_num); +extern bool pci_bar_is_memory(struct pci_dev *dev, unsigned int bar_num); +extern bool pci_bar_is_valid(struct pci_dev *dev, unsigned int bar_num); +extern void pci_bar_print(struct pci_dev *dev, unsigned int bar_num); extern void pci_dev_print_id(struct pci_dev *dev); extern void pci_dev_print(struct pci_dev *dev); extern uint8_t pci_intx_line(struct pci_dev *dev);
Cc: Thomas Huth <thuth@redhat.com> Cc: Andrew Jones <drjones@redhat.com> Cc: Peter Xu <peterx@redhat.com> Signed-off-by: Alexander Gordeev <agordeev@redhat.com> --- lib/pci.c | 54 ++++++++++++++++++++++++++++++++++++++++-------------- lib/pci.h | 21 +++++++++++++-------- 2 files changed, 53 insertions(+), 22 deletions(-)