qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 66/86] ppc/{ppc440_bamboo,sam460x}: drop RAM size fixup


From: BALATON Zoltan
Subject: Re: [PATCH v2 66/86] ppc/{ppc440_bamboo,sam460x}: drop RAM size fixup
Date: Fri, 17 Jan 2020 18:32:32 +0100 (CET)
User-agent: Alpine 2.21.99999 (BSF 352 2019-06-22)

On Fri, 17 Jan 2020, Igor Mammedov wrote:
On Fri, 17 Jan 2020 16:38:02 +0100
Philippe Mathieu-Daudé <address@hidden> wrote:

On 1/17/20 11:46 AM, Igor Mammedov wrote:
On Wed, 15 Jan 2020 22:33:46 +0100 (CET)
BALATON Zoltan <address@hidden> wrote:

[...]
v2 basically works the same as original code, except of that
instead of fixing up ram_size, it ask user to fix CLI to
the same value.

but I've not yet given up to keep current functionality
(also for other boards where this series would drop it). If you don't like
a generic callback then how about letting the board optionally specify in
the machine class next to default_ram_size some constraints such as
min_ram_size, max_ram_size and an array of valid_ram_sizes (like
ppc460ex_sdram_bank_sizes[]) that the generic code doing the allocation
could check and clamp sizes to the nearest valid one emitting a warning
and only bail out if constraints are not given. This would cover most
cases expect the e500 where it has an alignment without max so
representing it as a list of valid values is probably not practical (could
also have ram_alignment but then a generic callback may be simpler). Would
that way work for you?

I agree that we need to consolidate ram_size checks,
that would allow to clean up boards that practically do
the mostly the same. In the end it would allow to introspect
supported ram_sizes which would be good for upper layers.

I even started something in that direction but dropped
all of that patches as not directly related to this
series purpose.
Phillipe is also trying to something like this for raspi
board.

As for clamping, I still think that it's not good idea
to accept invalid value and fix it up.
Users should get what they asked for or get a clear error
(it's double true if user is management software as it
won't read any sort of warnings, well not all human users
would head warnings either as far as QEMU continues to run)

Anyways, most maintainers agreed to dropping fixups
and generalizing checks or generalizing fix up for convenience
of users frameworks are out of scope of this series.

Someone can work on these refactorings later on
(assuming someone is enough interested in it)

PS:
If you insist on fixing ram_size up, it would still be possible
to do so locally within board. One can take all RAM that user
provided and then create clamped memory region alias.
It's rather ugly but will do the job without affecting generic
code. (though I wouldn't volunteer to write up such a hack)

I think from the virtualization point of view, you don't want to
allocate unused resources. Is that why you say it is "ugly"?

 From the emulation point of view, it is easier to stay as close as
possible to the hardware, where sometimes resources are wasted to
simplify the design (or reduce production delays).
If it matches hardware, clamping doesn't look "ugly" to me.


gist of discussion in this case is that:

bamboo HW supports 4 banks of RAM that could be populated by
256/128/64/32/16/8Mb DIMMs

I care more about sam460ex but limitations are similar (with different list of supported DIMMs). This comes from how the SoC's memory controller stores memory size (that's similar to how SPD data encodes it) so we can't represent different size.

so -m XXX must be multiple of that, otherwise we are not
able distribute all -m specified size between supported banks.

Current patch then would error out with
 "Max 4 banks of 256, 128, 64, 32, 16, 8 MB DIMM/bank supported"
 "Possible valid RAM size: XXX"
where XXX is clamped value original code uses to after fixup

Argument was that it's not convenient for users do what they
would do on real hw (i.e. calculate desired size manually
based on available banks/DIMMs).

On real hardware one cannot have invalid RAM size because you can only plug DIMMs of above sizes so the problem does not happen. The argument was more that it worked before and now users may find their previously working options to error out. Current version of this patch is better as it gives valid option so the user who does not know about SoC limitations can just use the printed value and doesn't have to calculate valid value.

I gave an attempt to dumping all possible sizes and it results
in not practically useful unique ~80 entries list, so that idea
also goes down the drain.

I agree that it's not convenient, but I think that human user
can easily adapt to error message and either fix CLI to use
suggested value (i.e. what they are getting now) or pick desired
DIMMs number/size and do the math.

This version is acceptable and I can live with it. If every other board is happy with returning error then it's OK to do the same here as long as it's easy for users to fix their command line by using the printed value.

Regards,
BALATON Zoltan

reply via email to

[Prev in Thread] Current Thread [Next in Thread]