Re: [kvm-unit-tests PATCH v1 1/2] s390x: add a library for CMM-related functions

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

On Wed, 23 Nov 2022 17:12:52 +0100
Nico Boehr <nrb@linux.ibm.com> wrote:

> Quoting Claudio Imbrenda (2022-11-23 13:13:38)
> > > diff --git a/lib/s390x/cmm.c b/lib/s390x/cmm.c
> > > new file mode 100644
> > > index 000000000000..9609cea68950  
> [...]
> > > +static inline unsigned long get_page_addr(uint8_t *pagebuf, int page_idx)  
> > 
> > I don't like the name of this function, but maybe you can just get rid
> > of it (see below)  
> 
> Didn't like repeating the cast with the address calculation every time, but your solution with the cast first is good too. Done.
> 
> [...]
> > > +/*
> > > + * Verify CMM page states on pagebuf.
> > > + * Page states must have been set by cmm_set_page_states on pagebuf before.
> > > + * page_count must be a multiple of 4.
> > > + *
> > > + * If page states match the expected result,
> > > + * will return true and result will be untouched. When a mismatch occurs, will
> > > + * return false and result will be filled with details on the first mismatch.
> > > + */
> > > +bool cmm_verify_page_states(uint8_t *pagebuf, int page_count, struct cmm_verify_result *result)
> > > +{
> > > +     int i, state_mask, actual_state;  
> > 
> > I think "expected_mask" would be a better name, and maybe call the
> > other one "actual_mask"  
> 
> Yes, makes perfect sense. Done.
> 
> > > +
> > > +     assert(page_count % 4 == 0);
> > > +
> > > +     for (i = 0; i < page_count; i++) {
> > > +             actual_state = essa(ESSA_GET_STATE, get_page_addr(pagebuf, i));  
> > 
> > addr + i * PAGE_SIZE (if we get rid of get_page_addr)
> >   
> > > +             /* extract the usage state in bits 60 and 61 */
> > > +             actual_state = (actual_state >> 2) & 0x3;  
> > 
> > actual_mask = BIT((actual_mask >> 2) & 3);  
> 
> Yes makes sense, I will also adjust the comment a bit.
> 
> [...]
> > > +void cmm_report_verify_fail(struct cmm_verify_result const *result)
> > > +{
> > > +     report_fail("page state mismatch: first page = %d, expected_mask = 0x%x, actual_mask = 0x%x", result->page_mismatch, result->expected_mask, result->actual_mask);  
> > 
> > it would be a good idea to also print the actual address where the
> > mismatch was found (with %p and (pagebuf + result->page_mismatch))  
> 
> pagebuf is not available here, I want to avoid adding another argument, so I'll add a new field for the address in cmm_verify_result.
> 
> > > diff --git a/lib/s390x/cmm.h b/lib/s390x/cmm.h
> > > new file mode 100644
> > > index 000000000000..56e188c78704  
> [...]
> > > +struct cmm_verify_result {
> > > +     int page_mismatch;
> > > +     int expected_mask;
> > > +     int actual_mask;
> > > +};  
> > 
> > I'm not too fond of this, I wonder if it's possible to just return the
> > struct (maybe make the masks chars, since they will be small)  
> 
> No real reason to optimize for size, but also no reason not to, so I just changed it.
> 
> > but I am not sure if the code will actually look better in the end  
> 
> I am not a fan of returning structs, but none of my arguments against it apply
> here, so I changed it. Will add a field verify_failed to cmm_verify_result. 

I'm not sure I follow, but I guess I'll see it in v2

just make sure the code is not uglier, otherwise it's pointless :)

> 
> This has the nice side effect that I don't have to do 
>   if(cmm_verify_page_states()) 
>     report_pass()
>   else
>     cmm_report_verify_fail()
> in every caller, but can just move this whole logic to a function.