Quoting Claudio Imbrenda (2024-09-25 17:34:52)
> > +static uint8_t prefix_buf[LC_SIZE] __attribute__((aligned(LC_SIZE)));
>
> wait, you are using LC_SIZE in this file... even though it's only
> defined in the next patch.
>
> I think you should swap patch 1 and 2
will do
[...]
> > +/*
> > + * Verify that the refbk pointer is a real address and not a virtual
> > + * address. This is tested by enabling DAT and establishing a mapping
> > + * for the refbk that is outside of the bounds of our (guest-)physical
>
> s/physical/real/ (or absolute)
Huh? Why? Talking about the size here, so absolute, real and physical are
all equivalent here.
[...]
> > + /*
> > + * Put a valid refbk at refbk_in_reverse_prefix.
> > + */
> > + memcpy(refbk_in_reverse_prefix, &pfault_init_refbk, sizeof(pfault_init_refbk));
> > +
> > + ry = diag258(refbk_in_reverse_prefix);
> > + report(!ry, "real address refbk accessed");
> > +
> > + /*
> > + * Activating should have worked. Cancel the activation and expect
> > + * return 0. If activation would not have worked, this should return with
> > + * 4 (pfault handshaking not active).
> > + */
> > + ry = diag258(&pfault_cancel_refbk);
> > + report(!ry, "handshaking canceled");
> > +
> > + set_prefix(old_prefix);
> > +
> > + report_prefix_pop();
> > +}
>
> it seems like you are only testing the first page of lowcore; can you
> expand the test to also test the second page?
Would you mind leaving this for a future extension?
> > +
> > +/*
> > + * Verify that a refbk exceeding physical memory is not accepted, even
> > + * when crossing a frame boundary.
> > + */
> > +static void test_refbk_crossing(void)
> > +{
> > + const size_t bytes_in_last_page = 8;
>
> are there any alignment requirements for the buffer?
> if so, that should also be tested (either that a fault is triggered or
> that the lowest bytes are ignored, depending on how it is defined to
> work)
There are alignment requirements. Would it be OK to do this in a future
extension?
> > +/*
> > + * Verify that a refbk with an invalid refdiagc is not accepted.
> > + */
> > +static void test_refbk_invalid_diagcode(void)
> > +{
> > + struct pfault_refbk refbk = pfault_init_refbk;
> > +
> > + report_prefix_push("invalid refdiagc");
> > + refbk.refdiagc = 0xc0fe;
>
> other testcases in this file depend on invalid codes failing; maybe
> move this test up?
Yes, thanks, done.
> > +int main(void)
> > +{
> > + report_prefix_push("diag258");
> > +
> > + expect_pgm_int();
> > + diag258(&pfault_init_refbk);
> > + if (clear_pgm_int() == PGM_INT_CODE_SPECIFICATION) {
> > + report_skip("diag258 not supported");
> > + } else {
> > + test_priv();
> > + test_refbk_real();
>
> should probably go here....
test_refbk_real() relies on the invalid diagcode doing nothing, so it
should go *before* that one.
