On Thu, 2 Dec 2021 12:13:08 +0100
David Hildenbrand <david@xxxxxxxxxx> wrote:
> >> +static void wait_for_sclp_int(void)
> >> +{
> >> + /* Enable SCLP interrupts on this CPU only. */
> >> + ctl_set_bit(0, CTL0_SERVICE_SIGNAL);
> >> +
> >> + set_flag(1);
> >
> > why not just WRITE_ONCE/READ_ONCE?
>
> Because I shamelessly copied that from s390x/smp.c ;)
>
> >> + set_flag(0);
> >> +
> >> + /* Start CPU #1 and let it wait for the interrupt. */
> >> + psw.mask = extract_psw_mask();
> >> + psw.addr = (unsigned long)wait_for_sclp_int;
> >> + ret = smp_cpu_setup(1, psw);
> >> + if (ret) {
> >> + report_skip("cpu #1 not found");
> >
> > ...which means that this will hang, and so will all the other report*
> > functions. maybe you should manually unset the flag before calling the
> > various report* functions.
>
> Good point, thanks!
>
> >
> >> + goto out;
> >> + }
> >> +
> >> + /* Wait until the CPU #1 at least enabled SCLP interrupts. */
> >> + wait_for_flag();
> >> +
> >> + /*
> >> + * We'd have to jump trough some hoops to sense e.g., via SIGP
> >> + * CONDITIONAL EMERGENCY SIGNAL if CPU #1 is already in the
> >> + * wait state.
> >> + *
> >> + * Although not completely reliable, use SIGP SENSE RUNNING STATUS
> >> + * until not reported as running -- after all, our SCLP processing
> >> + * will take some time as well and make races very rare.
> >> + */
> >> + while(smp_sense_running_status(1));
if you wait here for CPU1 to be in wait state, then why did you need to
wait until it has set the flag earlier? can't you just wait here and not
use the whole wait_for_flag logic? smp_cpu_setup only returns after the
new CPU has started running.
I guess this was also inspired by smp.c :)
> >> +
> >> + h = alloc_page();
> >
> > do you really need to dynamically allocate one page?
> > is there a reason for not using a simple static buffer? (which you can
> > have aligned and statically initialized)
>
> I don't really have a strong opinion. I do prefer dynamic alloctions,
> though, if there isn't a good reason not to use them. No need to mess
> with page alignments manually.
fair enough, I also do not have a strong opinion
> >
> >> + memset(h, 0, sizeof(*h));
> >
> > otherwise, if you really want to allocate the memory, get rid of the
> > memset; the allocator always returns zeroed memory (unless you
> > explicitly ask not to by using flags)
>
> Right. "special" FLAG_DONTZERO in that semantics in that allocator.
>
> >
> >> + h->length = 4096;
> >> + ret = servc(SCLP_CMDW_READ_CPU_INFO, __pa(h));
> >> + if (ret) {
> >> + report_fail("SCLP_CMDW_READ_CPU_INFO failed");
> >> + goto out_destroy;
> >> + }
> >> +
> >> + /*
> >> + * Wait until the interrupt gets delivered on CPU #1, marking the
> >
> > why do you expect the interrupt to be delivered on CPU1? could it not
> > be delivered on CPU0?
>
> We don't enable SCLP interrupts + external interrupts on CPU #0 because
> we'll only call sclp_setup_int() on CPU #1.
oh right, I had missed that
>
> >
> >> + * SCLP requests as done.
> >> + */
> >> + sclp_wait_busy();
> >
> > this is logically not wrong (and should stay, because it makes clear
> > what you are trying to do), but strictly speaking it's not needed since
> > the report below will hang as long as the SCLP busy flag is set.
>
> Right. But it's really clearer to just have this in the code.
yep, which is why I said that this should stay :)
>
>
