On Thu, 01 Dec 2022 16:55:42 +0100
Nico Boehr <nrb@xxxxxxxxxxxxx> wrote:
> 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.
... according to a pattern
> >
> > 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.
looks good
>
> (But really that's also easy to see from the code below, so I am not sure if
> this really adds value.)
if you want to add documentation, do it properly, otherwise there is no
point in having documentation at all :)
>
> > > + * 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.
fair enough
