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.
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.
