On Wed, 23 Nov 2022 17:12:52 +0100
Nico Boehr <nrb@xxxxxxxxxxxxx> 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.
