Re: [kvm-unit-tests PATCH v1 1/3] s390x: add library for skey-related functions

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

On Thu, 01 Dec 2022 16:55:42 +0100
Nico Boehr <nrb@linux.ibm.com> wrote:

> Quoting Claudio Imbrenda (2022-12-01 14:16:50)
> > On Thu,  1 Dec 2022 09:46:40 +0100
> > Nico Boehr <nrb@linux.ibm.com> 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