From owner-freebsd-arch@freebsd.org Sun Aug 30 05:25:06 2015 Return-Path: Delivered-To: freebsd-arch@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id F36729C4C85 for ; Sun, 30 Aug 2015 05:25:05 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail109.syd.optusnet.com.au (mail109.syd.optusnet.com.au [211.29.132.80]) by mx1.freebsd.org (Postfix) with ESMTP id A3756286 for ; Sun, 30 Aug 2015 05:25:05 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c211-30-166-197.carlnfd1.nsw.optusnet.com.au (c211-30-166-197.carlnfd1.nsw.optusnet.com.au [211.30.166.197]) by mail109.syd.optusnet.com.au (Postfix) with ESMTPS id C9B21D66097; Sun, 30 Aug 2015 15:02:11 +1000 (AEST) Date: Sun, 30 Aug 2015 15:02:10 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Justin Hibbits cc: Adrian Chadd , Marcel Moolenaar , "freebsd-arch@freebsd.org" Subject: Re: Devices with 36-bit paddr on 32-bit system In-Reply-To: Message-ID: <20150830130612.L890@besplex.bde.org> References: <1568331.OrSoeYfXsf@ralph.baldwin.cx> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=L4TgOLn8 c=1 sm=1 tr=0 a=KA6XNC2GZCFrdESI5ZmdjQ==:117 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=kj9zAlcOel0A:10 a=JSiOX3MnzAab593RgikA:9 a=RBkRyt4X6hdqDwO5:21 a=_7EL_taM2OgDYD4P:21 a=CjuIK1q_8ugA:10 X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 30 Aug 2015 05:25:06 -0000 On Sat, 29 Aug 2015, Justin Hibbits wrote: > Hey, I already had that thought, too, you just encouraged me to take > it :) Anyway, here's an initial patch. I've *only* tested it on one > PowerPC board (the one needing this change), none of my other devices, > and no other architectures. Thoughts? It has lots of style bugs and type errors. % Index: sys/dev/ata/ata-pci.c % =================================================================== % --- sys/dev/ata/ata-pci.c (revision 287189) % +++ sys/dev/ata/ata-pci.c (working copy) % @@ -217,7 +217,7 @@ % % struct resource * % ata_pci_alloc_resource(device_t dev, device_t child, int type, int *rid, % - u_long start, u_long end, u_long count, u_int flags) % + bus_addr_t start, bus_addr_t end, u_long count, u_int flags) The first style bug is in the first changed line (line made longer than 80 columns by blind substitution). % Index: sys/dev/ata/ata-pci.h % =================================================================== % --- sys/dev/ata/ata-pci.h (revision 287189) % +++ sys/dev/ata/ata-pci.h (working copy) % @@ -535,7 +535,7 @@ % int ata_pci_print_child(device_t dev, device_t child); % int ata_pci_child_location_str(device_t dev, device_t child, char *buf, % size_t buflen); % -struct resource * ata_pci_alloc_resource(device_t dev, device_t child, int type, int *rid, u_long start, u_long end, u_long count, u_int flags); % +struct resource * ata_pci_alloc_resource(device_t dev, device_t child, int type, int *rid, bus_addr_t start, bus_addr_t end, u_long count, u_int flags); % int ata_pci_release_resource(device_t dev, device_t child, int type, int rid, struct resource *r); % int ata_pci_setup_intr(device_t dev, device_t child, struct resource *irq, int flags, driver_filter_t *filter, driver_intr_t *function, void *argument, void **cookiep); % int ata_pci_teardown_intr(device_t dev, device_t child, struct resource *irq, void *cookie); These were already misformatted. No comment on further style bugs from blind substitution. % Index: sys/dev/ata/chipsets/ata-promise.c % =================================================================== % --- sys/dev/ata/chipsets/ata-promise.c (revision 287189) % +++ sys/dev/ata/chipsets/ata-promise.c (working copy) % @@ -191,18 +191,19 @@ % !BUS_READ_IVAR(device_get_parent(GRANDPARENT(dev)), % GRANDPARENT(dev), PCI_IVAR_DEVID, &devid) && % ((devid == ATA_DEC_21150) || (devid == ATA_DEC_21150_1))) { % - static long start = 0, end = 0; % + static bus_addr_t start = 0; % + static u_long count = 0; Renaming the variable is an unrelated fix. The closure of this fix is quite large: - bus_get_resource() is undocumented except for a bogus reference to bus_get_resource(9) in bus_set_resource(9) , so the meaning of its last arg takes more work than necessary to check. The source file bus_get_resource.9 doesn't exist, and the installed file bus_get_resource.9.gz cannot be a link to bus_set_resource.9.gz since the top-level bus man pages are partially correctly organized with only 1 function per file. - the meaning of bus_get_resource()'s args are hard to see from its prototype since its prototype has no paramater names. uses a nonense mixture of no parameter names, parameter names without underscores, and parameter names with underscores. % % if (pci_get_slot(dev) == 1) { % - bus_get_resource(dev, SYS_RES_IRQ, 0, &start, &end); % + bus_get_resource(dev, SYS_RES_IRQ, 0, &start, &count); % strcat(buffer, " (channel 0+1)"); % } % - else if (pci_get_slot(dev) == 2 && start && end) { % - bus_set_resource(dev, SYS_RES_IRQ, 0, start, end); % + else if (pci_get_slot(dev) == 2 && start && count) { % + bus_set_resource(dev, SYS_RES_IRQ, 0, start, count); % strcat(buffer, " (channel 2+3)"); % } % else { % - start = end = 0; % + start = count = 0; This assumes that the types are similar enough for the conversion to work and not give a warning. If the types are integral, then the conversion always works and compilers shouldn't warn if it is a downcast (since 0 is representable in both types). But it isn't clear that the types are integral. pc98 uses handles, but bus_addr_t is still an integer and the handles are used to map it to the actual address. Perhaps large addresses should be handled similarly. % } % } % sprintf(buffer, "%s %s controller", buffer, ata_mode2str(idx->max_dma)); % Index: sys/dev/fdt/simplebus.c % =================================================================== % --- sys/dev/fdt/simplebus.c (revision 287189) % +++ sys/dev/fdt/simplebus.c (working copy) % ... % @@ -365,7 +365,8 @@ % if (j == sc->nranges && sc->nranges != 0) { % if (bootverbose) % device_printf(bus, "Could not map resource " % - "%#lx-%#lx\n", start, end); % + "%#llx-%#llx\n", (uint64_t)start, % + (uint64_t)end); % % return (NULL); % } This uses the long long abomination, and isn't even correct since type type isn't even (unsigned) long long. The type starts as u_long, which was not unsigned long long on any arch. It is cast to uint64_t. This breaks it if it is larger than uint64_t. This makes it compatible with unsigned long long on 32-bit arches, but has no effect on 32-bit arches -- there the type remains as u_long and the above fails to compile. No comment on further uses of the abomination. % Index: sys/dev/gpio/gpiobus.c % =================================================================== % --- sys/dev/gpio/gpiobus.c (revision 287189) % +++ sys/dev/gpio/gpiobus.c (working copy) % @@ -178,7 +178,7 @@ % sc->sc_intr_rman.rm_type = RMAN_ARRAY; % sc->sc_intr_rman.rm_descr = "GPIO Interrupts"; % if (rman_init(&sc->sc_intr_rman) != 0 || % - rman_manage_region(&sc->sc_intr_rman, 0, ~0) != 0) % + rman_manage_region(&sc->sc_intr_rman, 0, ~(bus_addr_t)0) != 0) I think ~0 gave the correct value except in the 1's complement case. Stranglely, ~0ul is more fragile than ~0. Elsewhere you spell this better as RM_MAX_END. % panic("%s: failed to set up rman.", __func__); % % if (GPIO_PIN_MAX(sc->sc_dev, &sc->sc_npins) != 0) % ... % @@ -516,7 +516,7 @@ % % if (type != SYS_RES_IRQ) % return (NULL); % - isdefault = (start == 0UL && end == ~0UL && count == 1); % + isdefault = (start == 0UL && end == ~(bus_addr_t)0UL && count == 1); Not using RM_MIN_START and RM_MAX_END gives even worse spelling than above. Both of the UL's are unnecessary, and the second one is bogus since the cast gives the correct type and it is not always UL. % rle = NULL; % if (isdefault) { % rl = BUS_GET_RESOURCE_LIST(bus, child); % Index: sys/dev/ofw/ofwbus.c % =================================================================== % --- sys/dev/ofw/ofwbus.c (revision 287189) % +++ sys/dev/ofw/ofwbus.c (working copy) % @@ -156,7 +156,7 @@ % sc->sc_mem_rman.rm_descr = "Device Memory"; % if (rman_init(&sc->sc_intr_rman) != 0 || % rman_init(&sc->sc_mem_rman) != 0 || % - rman_manage_region(&sc->sc_intr_rman, 0, ~0) != 0 || % + rman_manage_region(&sc->sc_intr_rman, 0, BUS_SPACE_MAXADDR) != 0 || Looks like a logic error. This is an rman function, so it should use an rman limit (now spelled RMAN_MAX_END), not a bus space limit. % rman_manage_region(&sc->sc_mem_rman, 0, BUS_SPACE_MAXADDR) != 0) The old code had a mixture of ~0 and BUS_SPACE_MAXADDR. This almost made sense. The first thing being managed is an interrupt id. Interrupts ids are normally small nonnegative integers. But they are still represented as u_longs or bus addresses. ~0 works for signed integers of any size and also for unsigned integers of any size except in the 1's complement case, and seems to have been used to reflect the smaller range required. % panic("%s: failed to set up rmans.", __func__); % % ... % @@ -201,7 +201,7 @@ % } % start = rle->start; % count = ulmax(count, rle->count); % - end = ulmax(rle->end, start + count - 1); % + end = qmax(rle->end, start + count - 1); This doesn't use the long long abomination, but assumes that quads are large enough. This is very fragile. No one knows what quads are, but they are currently int64_t. uquads are uint64_t, but uqmax() doesn't exist. So the above only works for 63-bit addresses. It seems to be very broken already for 64-bit bus_addr_t, since that gives the 64-bit address RMAN_MAX_END. No comment on further uses of quads. % } % % switch (type) { % ... % Index: sys/dev/pci/pci_pci.c % =================================================================== % --- sys/dev/pci/pci_pci.c (revision 287189) % +++ sys/dev/pci/pci_pci.c (working copy) % @@ -387,7 +388,7 @@ % char buf[64]; % int error, rid; % % - if (max_address != (u_long)max_address) % + if (max_address != (bus_addr_t)max_address) % max_address = ~0ul; pci uses pci_addr_t for addresses including max_address. pci_addr_t happens to be uint64_t, so it works better with a larger rman type/ Assume that the rman type is not larger. Then the above checks that the rman type is large enough to hold the current max_address, and if not it reduces to the _old_ rman limit. It should reduce to the current rman limit (RMAN_END_MAX). If the rman type is larger, then this code has no effect. Assigning RMAN_END_MAX to max_address would overflow, but is not reached since the rman type is large enough to hold any max_address. % w->rman.rm_start = 0; % w->rman.rm_end = max_address; % ... % Index: sys/kern/subr_rman.c % =================================================================== % --- sys/kern/subr_rman.c (revision 287189) % +++ sys/kern/subr_rman.c (working copy) % @@ -90,8 +90,8 @@ % TAILQ_ENTRY(resource_i) r_link; % LIST_ENTRY(resource_i) r_sharelink; % LIST_HEAD(, resource_i) *r_sharehead; % - u_long r_start; /* index of the first entry in this resource */ % - u_long r_end; /* index of the last entry (inclusive) */ % + bus_addr_t r_start; /* index of the first entry in this resource */ % + bus_addr_t r_end; /* index of the last entry (inclusive) */ Blind substitution also breaks indentation of comments. % u_int r_flags; % void *r_virtual; /* virtual address of this resource */ % struct device *r_dev; /* device which has allocated this resource */ % @@ -135,7 +135,7 @@ % } % % if (rm->rm_start == 0 && rm->rm_end == 0) RMAN_MIN_START is still spelled as 0 in many places. Since it is not very magic, this spelling is better. Let prototypes comvert plain 0 to the actual type. % ... % @@ -174,7 +174,7 @@ % % /* Skip entries before us. */ % TAILQ_FOREACH(s, &rm->rm_list, r_link) { % - if (s->r_end == ULONG_MAX) % + if (s->r_end == BUS_SPACE_MAXADDR) Why not RMAN_MAX_END? I think some cases of RMAN_MAX_END are actually magic meaning "not really the end, but a magic (default?) value". A different RMAN_FOO could be used for this. % Index: sys/powerpc/powerpc/nexus.c % =================================================================== % --- sys/powerpc/powerpc/nexus.c (revision 287189) % +++ sys/powerpc/powerpc/nexus.c (working copy) % @@ -189,13 +189,13 @@ % { % % if (type == SYS_RES_MEMORY) { % - vm_offset_t start; % + vm_paddr_t start; I think more places uses this instead of bus_addr_t or uint64_t. % void *p; % % - start = (vm_offset_t) rman_get_start(r); % + start = rman_get_start(r); The conversion is still logically non-null. % ... % Index: sys/sys/bus.h % =================================================================== % --- sys/sys/bus.h (revision 287189) % +++ sys/sys/bus.h (working copy) % @@ -29,6 +29,7 @@ % #ifndef _SYS_BUS_H_ % #define _SYS_BUS_H_ % % +#include % #include % #include % #include % @@ -292,8 +293,8 @@ % int rid; /**< @brief resource identifier */ % int flags; /**< @brief resource flags */ % struct resource *res; /**< @brief the real resource when allocated */ % - u_long start; /**< @brief start of resource range */ % - u_long end; /**< @brief end of resource range */ % + bus_addr_t start; /**< @brief start of resource range */ % + bus_addr_t end; /**< @brief end of resource range */ I think rman functions should use an rman type and not hard-code bus_addr_t. Related bus functions should then use this type. Style bugs from blind substitution can be reduced by using a less verbose name. % u_long count; /**< @brief count within range */ % }; % STAILQ_HEAD(resource_list, resource_list_entry); % ... % @@ -474,7 +475,8 @@ % static __inline struct resource * % bus_alloc_resource_any(device_t dev, int type, int *rid, u_int flags) % { % - return (bus_alloc_resource(dev, type, rid, 0ul, ~0ul, 1, flags)); % + return (bus_alloc_resource(dev, type, rid, (bus_addr_t)0ul, % + ~(bus_addr_t)0ul, 1, flags)); "ul" is now bogus, as above for UL except with a worse spelling. (Apparently, RLIM_FOO is not available here, so you have to expand it inline.) The first cast to bus_addr_t also has no affect. % } % % /* % Index: sys/sys/rman.h % =================================================================== % --- sys/sys/rman.h (revision 287189) % +++ sys/sys/rman.h (working copy) % @@ -54,6 +54,9 @@ % #define RF_ALIGNMENT_LOG2(x) ((x) << RF_ALIGNMENT_SHIFT) % #define RF_ALIGNMENT(x) (((x) & RF_ALIGNMENT_MASK) >> RF_ALIGNMENT_SHIFT) Example of normal style (except for the long line). % % +#define RM_MIN_START ((bus_addr_t)0) % +#define RM_MAX_END (~(bus_addr_t)0) Style bugs (space instead of table after define). There are no bogus ULs here. The first cast to bus_addr_t is probably unecessary here too. % @@ -108,8 +111,8 @@ % struct resource_head rm_list; % struct mtx *rm_mtx; /* mutex used to protect rm_list */ % TAILQ_ENTRY(rman) rm_link; /* link in list of all rmans */ % - u_long rm_start; /* index of globally first entry */ % - u_long rm_end; /* index of globally last entry */ % + bus_addr_t rm_start; /* index of globally first entry */ % + bus_addr_t rm_end; /* index of globally last entry */ % enum rman_type rm_type; /* what type of resource this is */ % const char *rm_descr; /* text descripion of this resource */ % }; This has mounds of new and old style bugs: new: unformatting by blind substitution old: - still uses old style of a tab instead of a space after struct, enum, and even in the middle of "const char" - avoids misindented comment for rm_list by not having one - has minimally misindented comment for rm_link after first using excessive indentation for the member name (then a space instead of a tab before the comment) - has minimally misindented comment for rm_type (after first using excessive indentation after enum, use only a space for the member name and the comment). Bruce