Quoting Claudio Imbrenda (2022-12-01 14:16:50)
> On Thu, 1 Dec 2022 09:46:40 +0100
> Nico Boehr <nrb@xxxxxxxxxxxxx> wrote:
[...]
> > diff --git a/lib/s390x/skey.c b/lib/s390x/skey.c
> > new file mode 100644
> > index 000000000000..100f0949a244
[...]
> > +/*
> > + * Set storage keys on pagebuf.
>
> surely you should explain better what the function does (e.g. how are
> you setting the keys and why)
Well there is the comment below which explains why the * 2 is needed, so what
about this paragraph (after merging the commits as discussed before):
* Each page's storage key is generated by taking the page's index in pagebuf,
* XOR-ing that with the given seed and then multipling the result with two.
(But really that's also easy to see from the code below, so I am not sure if
this really adds value.)
> > + * pagebuf must point to page_count consecutive pages.
> > + */
> > +void skey_set_keys(uint8_t *pagebuf, unsigned long page_count)
>
> this name does not make clear what the function is doing. at first one
> would think that it sets the same key for all pages.
>
> maybe something like set_storage_keys_test_pattern or
> skey_set_test_pattern or something like that
Oh that's a nice suggestion, thanks.
>
> > +{
> > + unsigned char key_to_set;
> > + unsigned long i;
> > +
> > + for (i = 0; i < page_count; i++) {
> > + /*
> > + * Storage keys are 7 bit, lowest bit is always returned as zero
> > + * by iske.
> > + * This loop will set all 7 bits which means we set fetch
> > + * protection as well as reference and change indication for
> > + * some keys.
> > + */
> > + key_to_set = i * 2;
> > + set_storage_key(pagebuf + i * PAGE_SIZE, key_to_set, 1);
>
> why not just i * 2 instead of using key_to_set ?
Well you answered that yourself :)
In patch 2, the key_to_set expression becomes a bit more complex, so the extra
variable makes sense to me.
