arm64/nexus: use actual PA range for I/O memory limit

[ehem_freebsd_m5p.com]

Summary

Addresses wider than the processor bus certainly won't be too useful. Differential D26144/git commit dd6fd1d4307247432272a10521ea19a70bb157bb was valuable for pointing to the appropriate places.

Appears D26133/dd6fd1d4307247432272a10521ea19a70bb157bb was incorrectly using the total amount of memory, rather than the maximum address. Correct this small error.

Leave behind get_physaddr_bits() and get_physaddr_count() as inline static functions. These values might potentially be needed for other code.

[ehem_freebsd_m5p.com]

This is being proposed due to problems being run into when trying to get Xen operational. This is very much preliminary.

[ehem_freebsd_m5p.com]

This part is simply the observation, why the heck will bus_alloc_resource(..., SYS_RES_MEMORY, 0, ~0, $large_size,) happily return address ranges (and sizes) which cannot possibly be valid (architecture doesn't allow for processors which have 64 address lines). This simply limits the potential ranges to potentially valid physical addresses.

More widely, I'm wondering if BUS_SPACE_MAXADDR and BUS_SPACE_MAXSIZE need to be replaced with a call to a theoretical arch_get_parange().

ARM really also needs similar action.

The next issue is then what is exactly is being managed? I suspect the work on Xen will be beneficial as it causes certain cross-architecture beliefs to be challenged.

[ehem_freebsd_m5p.com]

Yes, in nexus_attach() it would need to be nexus_get_parange() - 1. This happens when you're not really sure what should be done.

[mhorne]

Yes, this is desirable. I suppose it would also avoid the overflow noted by D30280.

Can you move the duplicated logic to a new function so it can be used in both places? sys/arm64/arm64/machdep.c would be an acceptable home for it, with a prototype in cpu.h.

[ehem_freebsd_m5p.com]

Getting things closer to a ready version. Now a common function, the
two (?) off-by-ones are fixed.

[ehem_freebsd_m5p.com]

Comparing with line 52 of sys/x86/acpica/srat.c, it appears acpi_pxm_init() should be getting the maximum valid address as the second argument. This means D26144 was off-by-one. Since the need for the aarch64 nexus is the same value, have the function providing that be the global one. Since I foresee someone wanting the number of address bits or the total count of addresses, leave get_physaddr_bits() and get_physaddr_count() behind with the potential to be made global.

[ehem_freebsd_m5p.com]

(one remaining issue is whether I've added the functions in the right place, I'm unsure if perhaps they should be more towards top/bottom of cpu.h and machdep.c)

[ehem_freebsd_m5p.com]

Any chance of a review of D30297? I'm unsure I've placed things in the appropriate places in files and I'm unsure what people think of style. Due to the limitations of the resource manager code the nexus is effectively broken right now and my goal needs this working.

[imp]

The nexus parts look good. Don't know if the determination of max address is right....

[mhorne]

sys/arm64/arm64/machdep.c

919–927

IMO the switch was much more readable. Unless you have a specific need for get_physaddr_bits() in mind I would rather you keep the original logic.

sys/arm64/include/cpu.h

211

nit: we typically don't add extern to function declarations.

[ehem_freebsd_m5p.com]

Fixing goof on original version, hopefully not abusing constants to make things nicer.

[ehem_freebsd_m5p.com]

sys/arm64/arm64/machdep.c

919–927

Yes, this was bothering me. I've been unsure of the best option.

Due to ID_AA64MMFR0_PARange_SHIFT being zero, the current one is possible. This is gross since the shifts should be taken out, but it does work.

I think this really points at a problem with armreg.h. Most of the values are available only with the left shift included which only works for switch() { } constructs and is poor for arrays. Really armreg.h should include the constants both with and without the left shift included.

Worse, all the ID_AA64MMFR0_PARange_* values include the shift, but a few do not include the shift (the EXCP_* constants for ESR_ELx_EXCEPTION()).

sys/arm64/include/cpu.h

211

That can be removed on commit. I'm in the crowd which thinks functions defaulting to external, but variables defaulting to static is a language problem. The better option would have been defaulting to static for everything.

[mhorne]

sys/arm64/arm64/machdep.c

919–927

Do you need get_physaddr_bits() for something? If not, please don't provide it. It is complicating an otherwise simple move of code from one place to another.

I would go further to say that we need only one of get_physaddr_max/get_physaddr_count.

[ehem_freebsd_m5p.com]

sys/arm64/arm64/machdep.c

919–927

No, I don't presently have plans for get_physaddr_bits(). My concern is the bit count is near certain to be useful to someone. I don't know who. I don't know when. Someone will want the bit count and this will cause a larger diff down the line.

If you really want, I can reduce it to get_physaddr_count() and only that, but please mark my words the additional functions will be desired enough that later they will be (re)added.

[ehem_freebsd_m5p.com]

I would compare the triplet of get_physaddr_bits()/get_physaddr_count()/get_physaddr_max() to the triplets of PAGE_SHIFT/PAGE_SIZE/PAGE_MASK in sys/arm64/include/param.h. Since my present requirement is to have one of these three, I'm willing to simplify, but I see them coming back in the future. Simply need to type something...

[mhorne]

In D30297#700016, @ehem_freebsd_m5p.com wrote:

I would compare the triplet of get_physaddr_bits()/get_physaddr_count()/get_physaddr_max() to the triplets of PAGE_SHIFT/PAGE_SIZE/PAGE_MASK in sys/arm64/include/param.h. Since my present requirement is to have one of these three, I'm willing to simplify, but I see them coming back in the future. Simply need to type something...

It seems like you are asking me to insist again on my requested changes, but I'm not interested in doing that. I gave my feedback with its reasoning, how you decide to handle it is up to you. If the code as written retains the previous behaviour of parse_pxm_tables() in all cases (which it appears to do, other than the inconsequential -1 offset to acpi_pxm_init), then I will not block the change from proceeding.

[ehem_freebsd_m5p.com]

Actually, I do believe the -1 offset to acpi_pxm_init() is significant. I opted for this version due to the other information I found. acpi_pxm_init() is declared in sys/dev/acpica/acpivar.h as int acpi_pxm_init(int ncpus, vm_paddr_t maxphys); and also used by sys/x86/acpica/srat.c where cpu_getmaxphyaddr() is used as the parameter. Those both appear to be using the maximum valid address (-1), so I believe the above is correct and what was previously there was incorrect (though I don't know how much effect it had).