On 6/17/20 7:11 PM, Halil Pasic wrote:
> On Tue, 16 Jun 2020 21:50:51 +0200
> Eric Farman <farman@xxxxxxxxxxxxx> wrote:
>
>> The interrupt path checks the FSM state when processing a final interrupt
>> (an interrupt that is neither subchannel active, nor device active),
>> to determine whether to call cp_free() and release the associated memory.
>> But, this does not fully close the window where a START comes in after a
>> HALT/CLEAR. If the START runs while the CLEAR interrupt is being processed,
>> the channel program struct will be allocated while the interrupt would be
>> considering whether or not to free it. If the FSM state is CP_PROCESSING,
>> then everything is fine. But if the START is able to issue its SSCH and get
>> a cc0, then the in-flight interrupt would have been for an unrelated
>> operation (perhaps none, if the subchannel was previously idle).
>>
>> The channel_program struct has an "initialized" flag that is set early
>> in the fsm_io_request() flow, to simplify the various cp_*() accessors.
>> Let's extend this idea to include a "started" flag that announces that the
>> channel program has successfully been issued to hardware. With this, the
>> interrupt path can determine whether the final interrupt should also
>> release the cp resources instead of relying on a transient FSM state.
>
> AFAICT cp->started is potentially accessed by multiple threads, form
> which at least one writes. Am I right?
Yup. And with the exception of the cp_free() call out of the interrupt
path, every one is accessed under the io_mutex. I'm still measuring
possible behavior at that point.
>
> Actually AFAICT you want to use cp->sarted for synchronization between
> multiple treads (I/O requester(s), IRQ handler(s)). How does the
> synchronization work for bool started itself, i.e. don't we have a data
> race on 'started'?
>
> A side note: I know, I asked a similar question about 'initialized' back
> then.
>
> Regards,
> Halil
>
>>
>> Signed-off-by: Eric Farman <farman@xxxxxxxxxxxxx>
>> ---
>> drivers/s390/cio/vfio_ccw_cp.c | 2 ++
>> drivers/s390/cio/vfio_ccw_cp.h | 1 +
>> drivers/s390/cio/vfio_ccw_drv.c | 2 +-
>> drivers/s390/cio/vfio_ccw_fsm.c | 11 +++++++++++
>> 4 files changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
>> index b9febc581b1f..7748eeef434e 100644
>> --- a/drivers/s390/cio/vfio_ccw_cp.c
>> +++ b/drivers/s390/cio/vfio_ccw_cp.c
>> @@ -657,6 +657,7 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)
>>
>> if (!ret) {
>> cp->initialized = true;
>> + cp->started = false;
>>
>> /* It is safe to force: if it was not set but idals used
>> * ccwchain_calc_length would have returned an error.
>> @@ -685,6 +686,7 @@ void cp_free(struct channel_program *cp)
>> return;
>>
>> cp->initialized = false;
>> + cp->started = false;
>> list_for_each_entry_safe(chain, temp, &cp->ccwchain_list, next) {
>> for (i = 0; i < chain->ch_len; i++) {
>> pfn_array_unpin_free(chain->ch_pa + i, cp->mdev);
>> diff --git a/drivers/s390/cio/vfio_ccw_cp.h b/drivers/s390/cio/vfio_ccw_cp.h
>> index ba31240ce965..7ea14910aaaa 100644
>> --- a/drivers/s390/cio/vfio_ccw_cp.h
>> +++ b/drivers/s390/cio/vfio_ccw_cp.h
>> @@ -39,6 +39,7 @@ struct channel_program {
>> union orb orb;
>> struct device *mdev;
>> bool initialized;
>> + bool started;
>> struct ccw1 *guest_cp;
>> };
>>
>> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
>> index 8c625b530035..7e2a790dc9a1 100644
>> --- a/drivers/s390/cio/vfio_ccw_drv.c
>> +++ b/drivers/s390/cio/vfio_ccw_drv.c
>> @@ -94,7 +94,7 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
>> (SCSW_ACTL_DEVACT | SCSW_ACTL_SCHACT));
>> if (scsw_is_solicited(&irb->scsw)) {
>> cp_update_scsw(&private->cp, &irb->scsw);
>> - if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING)
>> + if (is_final && private->cp.started)
>> cp_free(&private->cp);
>> }
>> mutex_lock(&private->io_mutex);
>> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
>> index 23e61aa638e4..d806f88eba72 100644
>> --- a/drivers/s390/cio/vfio_ccw_fsm.c
>> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
>> @@ -50,6 +50,7 @@ static int fsm_io_helper(struct vfio_ccw_private *private)
>> sch->schib.scsw.cmd.actl |= SCSW_ACTL_START_PEND;
>> ret = 0;
>> private->state = VFIO_CCW_STATE_CP_PENDING;
>> + private->cp.started = true;
>> break;
>> case 1: /* Status pending */
>> case 2: /* Busy */
>> @@ -246,6 +247,16 @@ static void fsm_io_request(struct vfio_ccw_private *private,
>> char *errstr = "request";
>> struct subchannel_id schid = get_schid(private);
>>
>> + if (private->cp.started) {
>> + io_region->ret_code = -EBUSY;
>> + VFIO_CCW_MSG_EVENT(2,
>> + "%pUl (%x.%x.%04x): busy\n",
>> + mdev_uuid(mdev), schid.cssid,
>> + schid.ssid, schid.sch_no);
>> + errstr = "busy";
>> + goto err_out;
>> + }
>> +
>> private->state = VFIO_CCW_STATE_CP_PROCESSING;
>> memcpy(scsw, io_region->scsw_area, sizeof(*scsw));
>>
>
