On Tue, 30 Jun 2020, Philippe Mathieu-Daudé wrote:
On 6/29/20 11:31 PM, BALATON Zoltan wrote:
Besides the above here's another use case of the fix ups that I wanted
to keep:

https://patchew.org/QEMU/cover.1592315226.git.bala...@eik.bme.hu/b5f4598529a77f15f554c593e9be2d0ff9e5fab3.1592315226.git.bala...@eik.bme.hu/


This board normally uses OpenBIOS which gets RAM size from fw_cfg and
so works with whatever amount of RAM (also Linux booted with -kernel
probably does not care) so any -memory value is valid. However some
may want to also use original firmware ROM for compatibility which
detects RAM reading SPD eeproms (the i2c emulation needed for that is
not working yet but once that's fixed this will be the case). I want
to add smbus_eeproms for this but do not want to just abort for cases
where -memory given by user cannot be covered with SPD data. Instead a
warning and covering as much RAM as possible should be enough (the ROM
will detect less RAM than given with -m
but that's OK and better than just bailing out without a message
tripping an assert). But I don't want to replicate in board code the
calculation and checks the spd_data_generate() function does anyway
(that would just puzzle reviewers for every use of this functions).

Previously this was possible with my original spd_data_generate()
implementation. What's your suggestion to bring that functionality
back without breaking Error API? Maybe adding new parameters to tell
the spd_data_generate() which fixups are allowed?
[...]
What I'd like is reverting f26740c61a57f and fix that some other way so
I don't have to duplicate size check in board code as can be seen in the
patchew link above but could just call spd_data_generate() to do its
job. This was discussed at the time that patch was in review you can
read it here:

http://patchwork.ozlabs.org/project/qemu-devel/patch/20200420132826.8879-3-arm...@redhat.com/


My points were not really considered then, now that I have another use
case maybe it could be revisited and fixed. What I want is to be able to
call spd_data_generate() from board code with whatever siz�© (the
board does not need to know about SPD limits and so cannot pre-check the
size) and the function should return the largest possible size SPD and
some indication if the size was not used completely. If Error cannot be
used for this, return the message or error some other way but let the
board code decide if it wants to abort or it can use the smaller SPD. Do
not assert in the helper function. Maybe the DIMM type fix up can be
dropped and only keep the size fix up so then we don't need to use error
twice, the board could call the function again if a different type is
also acceptable, since only sam460ex would need this I can do that there
for type fixup and call spd_data_generate() again with DDR2 if first one
with DDR could not fit all ram. But at least the asserts should be
dropped for this and the size check brought back. Then adding SPD to
mac_oldworld could also be done by calling spd_data_generate() instead
of duplicating the checks this function does anyway. This board has
three slots so if user says -m 1400 it would call spd_data_generate()
with 1400 first, get back 512 SPD that it adds to first slot then calls
spd_data_generate() again with 888, gets 512 again that it adds to 2nd
slot and calls spd_data_generate() for last slot with 376 which would
give 256 and 120 remaining that it may warn the user about but still
continue because the SPD data is only used by a ROM from real hardware
(that may be used for compatibility with some software) but the default
OpenBIOS disregards SPD data and would still use 1400 so it's not an
error to abort on. Simply if using a firmare ROM then only 1280 MB of
the 1400 will be available due to its limitations but that's not a
reason to force users to change their command line. Printing a warning
is enough to hint they may use different value but aborting without an
error message on an assert which is the current situation is not really
a user friendly way.

I just noticed we have a MachineClass::fixup_ram_size() handler. There
is only one implementation (on s390x) which does a bit the opposite:
If the user asks for -m 1400, it will pad to a valid physical size,
so in your example to 1472 MiB. Then the guest can use only 1400 if it
is happy with it. 72 MiB are wasted, but this is still better than the
576 MiB wasted if we were using 2 GiB instead ;)

See commit 5c30ef937f5:

static ram_addr_t s390_fixup_ram_size(ram_addr_t sz)
{
   /* same logic as in sclp.c */
   int increment_size = 20;
   ram_addr_t newsz;

   while ((sz >> increment_size) > MAX_STORAGE_INCREMENTS) {
       increment_size++;
   }
   newsz = sz >> increment_size << increment_size;

   if (sz != newsz) {
       qemu_printf("Ram size %" PRIu64 "MB was fixed up to %" PRIu64
                   "MB to match machine restrictions. "
                   "Consider updating "
                   "the guest definition.\n", (uint64_t) (sz / MiB),
                   (uint64_t) (newsz / MiB));
   }
   return newsz;
}

May have a look but first sight it might work for sam460ex but not for the mac_oldworld problem as described above without duplicating size checks in board code that I'd like to avoid and have it only in spd_data_generate for clarity and avoiding mistakes in duplicated copies. What the above could be good for is to avoid unused RAM area but that's not related to spd_data_generate and if it returns error some way or asserts.

I think the problem with fix up was that memdev will allocate mem region and it does not have a callback for the board to apply fixup or any other way to specify constraints so by the time board code runs the memory region is already allocated with the user specified size so all the board code can do is use that or abort (or not use and waste some amount). Maybe s390x wasn't converted to this or using it differently?

Regards,
BALATON Zoltan

Reply via email to