Devices with 36-bit paddr on 32-bit system

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

Devices with 36-bit paddr on 32-bit system

Justin Hibbits-2
With my work porting FreeBSD to PowerPC e500mc and e5500, I have
devices in my device tree mapped well above the 4GB mark
(0xffexxxxxx), and have no idea how to properly address them for
resources in rman.  Do we already have a solution to support this?
Part of the problem is the powerpc nexus does a straight convert to
vm_offset_t of rman_get_start() (itself returning a u_long), and
vm_offset_t is not necessarily equal to vm_paddr_t (on Book-E powerpc
vm_offset_t is 32-bits, vm_paddr_t is 64-bits).

Could rman be thought that resources aren't necessarily u_long?  The
only thought I have is to assume in the nexus code that the bottom 4
bits of the 32-bit address are actually the top bits of the 36-bit
address, and generate those in ofw_bus_reg_to_rl(), since the bottom
12 bits can be ignored anyways (pmap_mapdev() only maps full pages at
a minimum).  This seems kinda kludgy to me, though.

Any thoughts?

- Justin
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Devices with 36-bit paddr on 32-bit system

Marcel Moolenaar

> On Aug 24, 2015, at 11:44 PM, Justin Hibbits <[hidden email]> wrote:
>
> With my work porting FreeBSD to PowerPC e500mc and e5500, I have
> devices in my device tree mapped well above the 4GB mark
> (0xffexxxxxx), and have no idea how to properly address them for
> resources in rman.  Do we already have a solution to support this?
> Part of the problem is the powerpc nexus does a straight convert to
> vm_offset_t of rman_get_start() (itself returning a u_long), and
> vm_offset_t is not necessarily equal to vm_paddr_t (on Book-E powerpc
> vm_offset_t is 32-bits, vm_paddr_t is 64-bits).
I think the best solution is to represent a resource address
space with a type other than u_long. It makes sense to have
it use bus_addr_t or vm_paddr_t for example. Such a change
comes at a high price for sure, but you’ll fix it once and
for all. I don’t think you should kluge your way out of this...

--
Marcel Moolenaar
[hidden email]


signature.asc (859 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Devices with 36-bit paddr on 32-bit system

John-Mark Gurney-2
In reply to this post by Justin Hibbits-2
Justin Hibbits wrote this message on Mon, Aug 24, 2015 at 23:44 -0700:

> With my work porting FreeBSD to PowerPC e500mc and e5500, I have
> devices in my device tree mapped well above the 4GB mark
> (0xffexxxxxx), and have no idea how to properly address them for
> resources in rman.  Do we already have a solution to support this?
> Part of the problem is the powerpc nexus does a straight convert to
> vm_offset_t of rman_get_start() (itself returning a u_long), and
> vm_offset_t is not necessarily equal to vm_paddr_t (on Book-E powerpc
> vm_offset_t is 32-bits, vm_paddr_t is 64-bits).
>
> Could rman be thought that resources aren't necessarily u_long?  The
> only thought I have is to assume in the nexus code that the bottom 4
> bits of the 32-bit address are actually the top bits of the 36-bit
> address, and generate those in ofw_bus_reg_to_rl(), since the bottom
> 12 bits can be ignored anyways (pmap_mapdev() only maps full pages at
> a minimum).  This seems kinda kludgy to me, though.
>
> Any thoughts?

I'd look at how i386's PAE does it, as this is exactly the same type
of issue that PAE has...

--
  John-Mark Gurney Voice: +1 415 225 5579

     "All that I will do, has been done, All that I have, has not."
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Devices with 36-bit paddr on 32-bit system

Peter Grehan
> I'd look at how i386's PAE does it, as this is exactly the same type
> of issue that PAE has...

  i386 PAE doesn't have any h/w resources at addresses > 4GB, which is
the problem Justin is seeing on his ppc platform.

later,

Peter.

_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Devices with 36-bit paddr on 32-bit system

John Baldwin
In reply to this post by Marcel Moolenaar
On Tuesday, August 25, 2015 08:55:45 AM Marcel Moolenaar wrote:

>
> > On Aug 24, 2015, at 11:44 PM, Justin Hibbits <[hidden email]> wrote:
> >
> > With my work porting FreeBSD to PowerPC e500mc and e5500, I have
> > devices in my device tree mapped well above the 4GB mark
> > (0xffexxxxxx), and have no idea how to properly address them for
> > resources in rman.  Do we already have a solution to support this?
> > Part of the problem is the powerpc nexus does a straight convert to
> > vm_offset_t of rman_get_start() (itself returning a u_long), and
> > vm_offset_t is not necessarily equal to vm_paddr_t (on Book-E powerpc
> > vm_offset_t is 32-bits, vm_paddr_t is 64-bits).
>
> I think the best solution is to represent a resource address
> space with a type other than u_long. It makes sense to have
> it use bus_addr_t or vm_paddr_t for example. Such a change
> comes at a high price for sure, but you’ll fix it once and
> for all. I don’t think you should kluge your way out of this...

Expanding beyond u_long is the right solution.  PAE doesn't generally suffer
from this on i386 (though the ram0 device "punts" and ignores RAM ranges above
4G as a workaround).

However, u_long is baked into the bus resource API quite a bit.  Specifically,
the values 0ul and ~0ul are used as magic numbers in lots of places to request
"anywhere" locations.  Some of this has been mitigated by
bus_alloc_resource_any(), but that doesn't cover all cases.  You will probably
want to add some explicit constants and do a sweep replacing the magic numbers
with those first (and MFC the constants at least to make it easier to port
drivers across branches).  Then you can change the type.

As far as the best type: rman's are in theory generic and not just for bus
addresses.  I'd be tempted to let each platform define an rman_addr_t along
with RMAN_ADDR_MAX constants, but in practice it is probably fine to use
bus_addr_t and BUS_SPACE_MAXADDR.  If you do that it also means you can skip
the step of having to MFC new constants.

Note that various bus APIs will have to change to use bus_addr_t instead of
u_long as well.

--
John Baldwin
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Devices with 36-bit paddr on 32-bit system

Adrian Chadd-4
+1 on this.

Also - justin/i figured it out (well, I made the suggestion, he did
the suggestion) which is just to do a big/single mapping of the
relevant hardware window into vmem early in boot, and hack that bus
nexus to treat things as being in that vmem region. He's gotten pretty
far along the device bringup path now. That way the rmem allocation is
just from that vmem region.



-adrian



On 28 August 2015 at 10:35, John Baldwin <[hidden email]> wrote:

> On Tuesday, August 25, 2015 08:55:45 AM Marcel Moolenaar wrote:
>>
>> > On Aug 24, 2015, at 11:44 PM, Justin Hibbits <[hidden email]> wrote:
>> >
>> > With my work porting FreeBSD to PowerPC e500mc and e5500, I have
>> > devices in my device tree mapped well above the 4GB mark
>> > (0xffexxxxxx), and have no idea how to properly address them for
>> > resources in rman.  Do we already have a solution to support this?
>> > Part of the problem is the powerpc nexus does a straight convert to
>> > vm_offset_t of rman_get_start() (itself returning a u_long), and
>> > vm_offset_t is not necessarily equal to vm_paddr_t (on Book-E powerpc
>> > vm_offset_t is 32-bits, vm_paddr_t is 64-bits).
>>
>> I think the best solution is to represent a resource address
>> space with a type other than u_long. It makes sense to have
>> it use bus_addr_t or vm_paddr_t for example. Such a change
>> comes at a high price for sure, but you’ll fix it once and
>> for all. I don’t think you should kluge your way out of this...
>
> Expanding beyond u_long is the right solution.  PAE doesn't generally suffer
> from this on i386 (though the ram0 device "punts" and ignores RAM ranges above
> 4G as a workaround).
>
> However, u_long is baked into the bus resource API quite a bit.  Specifically,
> the values 0ul and ~0ul are used as magic numbers in lots of places to request
> "anywhere" locations.  Some of this has been mitigated by
> bus_alloc_resource_any(), but that doesn't cover all cases.  You will probably
> want to add some explicit constants and do a sweep replacing the magic numbers
> with those first (and MFC the constants at least to make it easier to port
> drivers across branches).  Then you can change the type.
>
> As far as the best type: rman's are in theory generic and not just for bus
> addresses.  I'd be tempted to let each platform define an rman_addr_t along
> with RMAN_ADDR_MAX constants, but in practice it is probably fine to use
> bus_addr_t and BUS_SPACE_MAXADDR.  If you do that it also means you can skip
> the step of having to MFC new constants.
>
> Note that various bus APIs will have to change to use bus_addr_t instead of
> u_long as well.
>
> --
> John Baldwin
> _______________________________________________
> [hidden email] mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-arch
> To unsubscribe, send any mail to "[hidden email]"
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Devices with 36-bit paddr on 32-bit system

Justin Hibbits-2
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?

tl;dr I went with using bus_addr_t for the addresses, and kept u_long
for the sizes (I can change it to use bus_size_t instead, but it's
already vm_offset_t on PowerPC, which is long anyway).

Since I took the diff as a whole, and erased unrelated bits, there may
still be unrelated bits in the diff, which can be ignored (all part of
my larger work).

- Justin

On Fri, Aug 28, 2015 at 1:59 PM, Adrian Chadd <[hidden email]> wrote:

> +1 on this.
>
> Also - justin/i figured it out (well, I made the suggestion, he did
> the suggestion) which is just to do a big/single mapping of the
> relevant hardware window into vmem early in boot, and hack that bus
> nexus to treat things as being in that vmem region. He's gotten pretty
> far along the device bringup path now. That way the rmem allocation is
> just from that vmem region.
>
>
>
> -adrian
>
>
>
> On 28 August 2015 at 10:35, John Baldwin <[hidden email]> wrote:
>> On Tuesday, August 25, 2015 08:55:45 AM Marcel Moolenaar wrote:
>>>
>>> > On Aug 24, 2015, at 11:44 PM, Justin Hibbits <[hidden email]> wrote:
>>> >
>>> > With my work porting FreeBSD to PowerPC e500mc and e5500, I have
>>> > devices in my device tree mapped well above the 4GB mark
>>> > (0xffexxxxxx), and have no idea how to properly address them for
>>> > resources in rman.  Do we already have a solution to support this?
>>> > Part of the problem is the powerpc nexus does a straight convert to
>>> > vm_offset_t of rman_get_start() (itself returning a u_long), and
>>> > vm_offset_t is not necessarily equal to vm_paddr_t (on Book-E powerpc
>>> > vm_offset_t is 32-bits, vm_paddr_t is 64-bits).
>>>
>>> I think the best solution is to represent a resource address
>>> space with a type other than u_long. It makes sense to have
>>> it use bus_addr_t or vm_paddr_t for example. Such a change
>>> comes at a high price for sure, but you’ll fix it once and
>>> for all. I don’t think you should kluge your way out of this...
>>
>> Expanding beyond u_long is the right solution.  PAE doesn't generally suffer
>> from this on i386 (though the ram0 device "punts" and ignores RAM ranges above
>> 4G as a workaround).
>>
>> However, u_long is baked into the bus resource API quite a bit.  Specifically,
>> the values 0ul and ~0ul are used as magic numbers in lots of places to request
>> "anywhere" locations.  Some of this has been mitigated by
>> bus_alloc_resource_any(), but that doesn't cover all cases.  You will probably
>> want to add some explicit constants and do a sweep replacing the magic numbers
>> with those first (and MFC the constants at least to make it easier to port
>> drivers across branches).  Then you can change the type.
>>
>> As far as the best type: rman's are in theory generic and not just for bus
>> addresses.  I'd be tempted to let each platform define an rman_addr_t along
>> with RMAN_ADDR_MAX constants, but in practice it is probably fine to use
>> bus_addr_t and BUS_SPACE_MAXADDR.  If you do that it also means you can skip
>> the step of having to MFC new constants.
>>
>> Note that various bus APIs will have to change to use bus_addr_t instead of
>> u_long as well.
>>
>> --
>> John Baldwin
>> _______________________________________________
>> [hidden email] mailing list
>> https://lists.freebsd.org/mailman/listinfo/freebsd-arch
>> To unsubscribe, send any mail to "[hidden email]"

_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"

rman.diff (68K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Devices with 36-bit paddr on 32-bit system

Marcel Moolenaar

> On Aug 29, 2015, at 11:35 AM, Justin Hibbits <[hidden email]> wrote:
>
> tl;dr I went with using bus_addr_t for the addresses, and kept u_long
> for the sizes (I can change it to use bus_size_t instead, but it's
> already vm_offset_t on PowerPC, which is long anyway).

Unfortunately, you want to change count from u_long to something wider
as well. If you have more than 4GB of memory and want a resource for it,
you have start=0, end>4GB and count>4GB for the entire range.

--
Marcel Moolenaar
[hidden email]


signature.asc (859 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Devices with 36-bit paddr on 32-bit system

Bruce Evans-4
In reply to this post by Justin Hibbits-2
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.  <sys/bus.h> 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 <machine/_bus.h>
%  #include <machine/_limits.h>
%  #include <sys/_bus_dma.h>
%  #include <sys/ioccom.h>
% @@ -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
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Devices with 36-bit paddr on 32-bit system

Bruce Evans-4
On Sun, 30 Aug 2015, Bruce Evans wrote:

> % ...
> % 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 <machine/_bus.h>
> %  #include <machine/_limits.h>
> %  #include <sys/_bus_dma.h>
> %  #include <sys/ioccom.h>
> % @@ -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 */

Mail programs (mostly mine) corrupted the formatting more competely.

> 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 */
> %  };

Or just use uintmax_t for everything in rman.  rman was written before
C99 broke C by making u_long no longer the largest integer type.  It
used u_long because it was the largest integer type (though it actually
wasn't, since FreeBSD used nonstandard extensions in Gnu C) and it is
easiest to use a single non-typedefed type that is large enough for all
cases.  uintmax_t is C99's replacement of u_long.

I don't like the bloat from using uintmax_t for everything, but rman
should only used for initialization so uintmax_t for rman should only
give space bloat, only on 32-bit arches.

An rman typedef for this type allows re-optimizing the 32-bit arches,
but brings back the problem of typedefed types being hard to use.

Bruce
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Devices with 36-bit paddr on 32-bit system

Justin Hibbits-2
Hi Bruce,

On Sat, Aug 29, 2015 at 10:49 PM, Bruce Evans <[hidden email]> wrote:

> On Sun, 30 Aug 2015, Bruce Evans wrote:
>
>> % ...
>> % 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 <machine/_bus.h>
>> %  #include <machine/_limits.h>
>> %  #include <sys/_bus_dma.h>
>> %  #include <sys/ioccom.h>
>> % @@ -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
>> */
>
>
> Mail programs (mostly mine) corrupted the formatting more competely.
>
>> 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 */
>> %  };
>
>
> Or just use uintmax_t for everything in rman.  rman was written before
> C99 broke C by making u_long no longer the largest integer type.  It
> used u_long because it was the largest integer type (though it actually
> wasn't, since FreeBSD used nonstandard extensions in Gnu C) and it is
> easiest to use a single non-typedefed type that is large enough for all
> cases.  uintmax_t is C99's replacement of u_long.
>
> I don't like the bloat from using uintmax_t for everything, but rman
> should only used for initialization so uintmax_t for rman should only
> give space bloat, only on 32-bit arches.
>
> An rman typedef for this type allows re-optimizing the 32-bit arches,
> but brings back the problem of typedefed types being hard to use.
>
> Bruce

Thanks for the lengthy review.  I am well aware of (most of) the style
issues and intend to address them all before releasing a final patch.
My goal with this rough early release was to solicit comments on the
approach taken, and if there should be a better way to address the
overall problem.  I did consider your suggestion of uintmax_t across
the board early on, but felt there must be a better type name to
identify purpose.  A 'rman_addr_t' is better than bus_addr_t, so I can
easily make that mechanical change (simple sed on the diff would do
it).  Also, good eye on the qmax()/qmin() needing to have unsigned
counterparts, like I said, I haven't yet tested on a full 64-bit
platform :)

Regarding casts and macros, since this was an evolutionary process, I
haven't yet cleaned things up (why clean it up, if people tell me the
basic approach sucks anyway?), so that'll come in round 2, but thanks
for identifying them, they may have slipped through later patches.
I'm not tied one way or the other to RM_MIN_START vs 0.  Keeping it 0
would definitely be easier on the fingers, less rewrites, and I do
have more to change for the RM_MAX_END (choose a better name?)

As for the 'long long abomination', do you have anything better?  I
can't use PRIxPTR, because if that was possible I wouldn't need to
make this change in the first place.  I guess I could use %jx, or
PRIxMAX, and cast to uintmax_t.

So, that all being said, any suggestion on what the naming should be?
I'll pose: rman_addr_t and rman_size_t, as counterparts to bus_*_t.
Yes, it's more verbose, but it also shows usage intent.  However, I
would also yield to uintmax_t if others think that's the best option
as well.

- Justin
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"