On Mon, 25 Jun 2018 15:16:15 +0200 David Hildenbrand <da...@redhat.com> wrote:
> On 25.06.2018 15:07, Cornelia Huck wrote: > > On Mon, 25 Jun 2018 09:42:11 -0300 > > Philippe Mathieu-Daudé <f4...@amsat.org> wrote: > > > >> It eases code review, unit is explicit. > >> > >> Patch generated using: > >> > >> $ git grep -E '(1024|2048|4096|8192|(<<|>>).?(10|20|30))' hw/ include/hw/ > >> > >> and modified manually. > >> > >> Signed-off-by: Philippe Mathieu-Daudé <f4...@amsat.org> > >> Reviewed-by: Thomas Huth <th...@redhat.com> > >> Acked-by: Cornelia Huck <coh...@redhat.com> > > > > Hm, I had not looked at the v2+ patches, as this already carried my > > ack... > > > >> --- > >> hw/s390x/s390-skeys.c | 3 ++- > >> hw/s390x/s390-stattrib.c | 3 ++- > > > > ...but these two were added later on. > > > >> hw/s390x/sclp.c | 3 ++- > >> 3 files changed, 6 insertions(+), 3 deletions(-) > >> > >> diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c > >> index 76241c240e..15f7ab0e53 100644 > >> --- a/hw/s390x/s390-skeys.c > >> +++ b/hw/s390x/s390-skeys.c > >> @@ -10,6 +10,7 @@ > >> */ > >> > >> #include "qemu/osdep.h" > >> +#include "qemu/units.h" > >> #include "hw/boards.h" > >> #include "hw/s390x/storage-keys.h" > >> #include "qapi/error.h" > >> @@ -19,7 +20,7 @@ > >> #include "sysemu/kvm.h" > >> #include "migration/register.h" > >> > >> -#define S390_SKEYS_BUFFER_SIZE 131072 /* Room for 128k storage keys */ > >> +#define S390_SKEYS_BUFFER_SIZE (128 * KiB) /* Room for 128k storage keys > >> */ > > > > This one looks confusing to me. We're not allocating 128 chunks of 1 > > KiB size, but space enough for 128k byte values. What do others think? > > Hm, as this define is called "_SIZE" it should be the right thing to do. > > I would agree if it would be "_SKEY.*_COUNT" Yes, but I found it a bit non-intuitive, as it is not immediately clear that we want to support 128k byte values. It's probably clearer than the previous magic value, though. No real objections to this change from my side, though.
