| Message ID | 1449523949-21898-8-git-send-email-keith.busch@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Mon, Dec 7, 2015 at 11:32 PM, Keith Busch <keith.busch@intel.com> wrote: > PCI-e segments will continue to use the lower 16 bits as required by > ACPI. Special domains may use the full 32-bits. > > Signed-off-by: Keith Busch <keith.busch@intel.com> > --- > lib/filter.c | 2 +- > lib/pci.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/lib/filter.c b/lib/filter.c > index d4254a0..075dc2f 100644 > --- a/lib/filter.c > +++ b/lib/filter.c > @@ -45,7 +45,7 @@ pci_filter_parse_slot_v33(struct pci_filter *f, char *str) > if (str[0] && strcmp(str, "*")) > { > long int x = strtol(str, &e, 16); > - if ((e && *e) || (x < 0 || x > 0xffff)) > + if ((e && *e) || (x < 0)) > return "Invalid domain number"; > f->domain = x; > } > diff --git a/lib/pci.h b/lib/pci.h > index 10ba831..7e42765 100644 > --- a/lib/pci.h > +++ b/lib/pci.h > @@ -119,7 +119,7 @@ struct pci_param *pci_walk_params(struct pci_access *acc, struct pci_param *prev > > struct pci_dev { > struct pci_dev *next; /* Next device in the chain */ > - u16 domain; /* PCI domain (host bridge) */ > + int32_t domain; /* PCI domain (host bridge) */ Why not u32 ? > u8 bus, dev, func; /* Bus inside domain, device and function */ > > /* These fields are set by pci_fill_info() */ > -- > 1.7.10.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
Hi Keith, On Mon, Dec 07, 2015 at 02:32:29PM -0700, Keith Busch wrote: > PCI-e segments will continue to use the lower 16 bits as required by > ACPI. Special domains may use the full 32-bits. > > Signed-off-by: Keith Busch <keith.busch@intel.com> > --- > lib/filter.c | 2 +- > lib/pci.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/lib/filter.c b/lib/filter.c > index d4254a0..075dc2f 100644 > --- a/lib/filter.c > +++ b/lib/filter.c > @@ -45,7 +45,7 @@ pci_filter_parse_slot_v33(struct pci_filter *f, char *str) > if (str[0] && strcmp(str, "*")) > { > long int x = strtol(str, &e, 16); > - if ((e && *e) || (x < 0 || x > 0xffff)) > + if ((e && *e) || (x < 0)) Just out of curiosity (I don't maintain pciutils; Martin would apply this one), is there some part of the PCI or PCI firmware spec that is relevant to this change? Maybe this is connected to parsing things exported by the kernel and not directly tied to PCI at the spec level. Whatever it is, a pointer to the producer of the information you're consuming here would help us understand and review the patch. > return "Invalid domain number"; > f->domain = x; > } > diff --git a/lib/pci.h b/lib/pci.h > index 10ba831..7e42765 100644 > --- a/lib/pci.h > +++ b/lib/pci.h > @@ -119,7 +119,7 @@ struct pci_param *pci_walk_params(struct pci_access *acc, struct pci_param *prev > > struct pci_dev { > struct pci_dev *next; /* Next device in the chain */ > - u16 domain; /* PCI domain (host bridge) */ > + int32_t domain; /* PCI domain (host bridge) */ > u8 bus, dev, func; /* Bus inside domain, device and function */ > > /* These fields are set by pci_fill_info() */ > -- > 1.7.10.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 17, 2015 at 11:15:45AM -0600, Bjorn Helgaas wrote: > > @@ -45,7 +45,7 @@ pci_filter_parse_slot_v33(struct pci_filter *f, char *str) > > if (str[0] && strcmp(str, "*")) > > { > > long int x = strtol(str, &e, 16); > > - if ((e && *e) || (x < 0 || x > 0xffff)) > > + if ((e && *e) || (x < 0)) > > Just out of curiosity (I don't maintain pciutils; Martin would apply > this one), is there some part of the PCI or PCI firmware spec that is > relevant to this change? Maybe this is connected to parsing things > exported by the kernel and not directly tied to PCI at the spec level. > > Whatever it is, a pointer to the producer of the information you're > consuming here would help us understand and review the patch. Hi Bjorn, This is not tied to anything defined in PCI spec. Domain numbers being a software construct (ACPI6, §6.5.6), we don't need to constrain the representation. ACPI defines 16-bit segments, and domains provided by this new host bridge do not define _SEG, so this series proposes domain numbers outside the ACPI reachable range to avoid potential clashes. The pciutils patch just synchronizes the essential tooling software with the kernel software's new representation. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 17, 2015 at 05:34:46PM +0000, Keith Busch wrote: > On Thu, Dec 17, 2015 at 11:15:45AM -0600, Bjorn Helgaas wrote: > > > @@ -45,7 +45,7 @@ pci_filter_parse_slot_v33(struct pci_filter *f, char *str) > > > if (str[0] && strcmp(str, "*")) > > > { > > > long int x = strtol(str, &e, 16); > > > - if ((e && *e) || (x < 0 || x > 0xffff)) > > > + if ((e && *e) || (x < 0)) > > > > Just out of curiosity (I don't maintain pciutils; Martin would apply > > this one), is there some part of the PCI or PCI firmware spec that is > > relevant to this change? Maybe this is connected to parsing things > > exported by the kernel and not directly tied to PCI at the spec level. > > > > Whatever it is, a pointer to the producer of the information you're > > consuming here would help us understand and review the patch. > > Hi Bjorn, > > This is not tied to anything defined in PCI spec. Domain numbers being > a software construct (ACPI6, §6.5.6), we don't need to constrain the > representation. ACPI defines 16-bit segments, and domains provided by > this new host bridge do not define _SEG, so this series proposes domain > numbers outside the ACPI reachable range to avoid potential clashes. > > The pciutils patch just synchronizes the essential tooling software with > the kernel software's new representation. That's what I figured. It'd be useful to know exactly what is on the other end of this, e.g., a Linux /proc or /sys file or whatever it is. Your changelog assumes a lot of implicit knowledge about Linux, VMD, and the previous patches in this series. But pciutils is not Linux-specific, and it's maintained completely separately from Linux. This patch needs to supply enough explicit context that it makes sense all by itself, apart from the kernel series. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello! > PCI-e segments will continue to use the lower 16 bits as required by > ACPI. Special domains may use the full 32-bits. > > Signed-off-by: Keith Busch <keith.busch@intel.com> > --- > lib/filter.c | 2 +- > lib/pci.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/lib/filter.c b/lib/filter.c > index d4254a0..075dc2f 100644 > --- a/lib/filter.c > +++ b/lib/filter.c > @@ -45,7 +45,7 @@ pci_filter_parse_slot_v33(struct pci_filter *f, char *str) > if (str[0] && strcmp(str, "*")) > { > long int x = strtol(str, &e, 16); > - if ((e && *e) || (x < 0 || x > 0xffff)) > + if ((e && *e) || (x < 0)) > return "Invalid domain number"; > f->domain = x; > } > diff --git a/lib/pci.h b/lib/pci.h > index 10ba831..7e42765 100644 > --- a/lib/pci.h > +++ b/lib/pci.h > @@ -119,7 +119,7 @@ struct pci_param *pci_walk_params(struct pci_access *acc, struct pci_param *prev > > struct pci_dev { > struct pci_dev *next; /* Next device in the chain */ > - u16 domain; /* PCI domain (host bridge) */ > + int32_t domain; /* PCI domain (host bridge) */ > u8 bus, dev, func; /* Bus inside domain, device and function */ This is definitely not enough. Try grepping the source for "domain" :-) At least the following places need updating, too: o struct pci_filter and operations on it o Format strings for printing domains at various places o ABI compability ... changing a field in the middle of struct pci_dev (or pci_filter) is going to break ABI, so you either need to change the structures in a backward-compatible way, or to use ABI versioning. Also, we should decide on what type the domain should have -- currently, some places use "int", others use u16, and your patch introduces int32_t. I would prefer u32 myself, but especially in the filters we should be careful about how to encode "any domain". Have a nice new year Martin -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, thanks for the feedback. I've a few follow up questions. On Sun, Jan 03, 2016 at 03:11:24PM +0100, Martin Mares wrote: > This is definitely not enough. Try grepping the source for "domain" :-) > > At least the following places need updating, too: > > o struct pci_filter and operations on it Not sure I follow. struct pci_filter's domain was already a 32-bit int. > o Format strings for printing domains at various places Are you wanting a %04x for 16 bit domains and %08x for 32 bit ones? The %04x specifier still works with 32-bit values. We just need a bit so this new h/w can't collide with ACPI _SEG defined domains. I don't know of any real need for the full 32-bits; we'd do fine using only 17 bits, so thought the leading 0's wasn't useful. > o ABI compability ... changing a field in the middle of struct pci_dev > (or pci_filter) is going to break ABI, so you either need to change the > structures in a backward-compatible way, or to use ABI versioning. It looks like there's a 16-bit gap after device_class. Would it be acceptable place the domain's upper 16 bits in there to keep ABI compatibility? > Also, we should decide on what type the domain should have -- currently, some > places use "int", others use u16, and your patch introduces int32_t. I would > prefer u32 myself, but especially in the filters we should be careful about > how to encode "any domain". I left it as a signed int to allow a negative number for "any", and that's also what the linux kernel uses. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi! > On Sun, Jan 03, 2016 at 03:11:24PM +0100, Martin Mares wrote: > > This is definitely not enough. Try grepping the source for "domain" :-) > > > > At least the following places need updating, too: > > > > o struct pci_filter and operations on it > > Not sure I follow. struct pci_filter's domain was already a 32-bit int. Yes, but it has to encode a "don't care" value, too. However, if 17 bits are sufficient, then let us define that the domain number is currently 31-bit and keep -1 as "don't care". In any case, it would be nice to make the sysfs back-end check that the number provided by the kernel fits in 31 bits. > > o Format strings for printing domains at various places > > Are you wanting a %04x for 16 bit domains and %08x for 32 bit ones? The > %04x specifier still works with 32-bit values. After some more thought, keeping %04x will be better. > > o ABI compability ... changing a field in the middle of struct pci_dev > > (or pci_filter) is going to break ABI, so you either need to change the > > structures in a backward-compatible way, or to use ABI versioning. > > It looks like there's a 16-bit gap after device_class. Would it be > acceptable place the domain's upper 16 bits in there to keep ABI > compatibility? This would mean that all new programs supporting 32-bit domains would have to combine both fields to get the full domain number. I think we can just rename the 16-bit field to domain_16 (where only the lower 16 bits will be stored) and add a new 32-bit domain field at the end of the public part of the structure. Since the structures are always allocated by libpci, that will work. Old programs will fail silently on machines with large domain numbers, but that is hopefully acceptable. Also, we should add a new version of some basic function call, for example pci_init, so that new programs will require a new version of the ABI. Martin -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/lib/filter.c b/lib/filter.c index d4254a0..075dc2f 100644 --- a/lib/filter.c +++ b/lib/filter.c @@ -45,7 +45,7 @@ pci_filter_parse_slot_v33(struct pci_filter *f, char *str) if (str[0] && strcmp(str, "*")) { long int x = strtol(str, &e, 16); - if ((e && *e) || (x < 0 || x > 0xffff)) + if ((e && *e) || (x < 0)) return "Invalid domain number"; f->domain = x; } diff --git a/lib/pci.h b/lib/pci.h index 10ba831..7e42765 100644 --- a/lib/pci.h +++ b/lib/pci.h @@ -119,7 +119,7 @@ struct pci_param *pci_walk_params(struct pci_access *acc, struct pci_param *prev struct pci_dev { struct pci_dev *next; /* Next device in the chain */ - u16 domain; /* PCI domain (host bridge) */ + int32_t domain; /* PCI domain (host bridge) */ u8 bus, dev, func; /* Bus inside domain, device and function */ /* These fields are set by pci_fill_info() */
PCI-e segments will continue to use the lower 16 bits as required by ACPI. Special domains may use the full 32-bits. Signed-off-by: Keith Busch <keith.busch@intel.com> --- lib/filter.c | 2 +- lib/pci.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)