On 10/18/2018 05:15 PM, Pierre Morel wrote:
> On 18/10/2018 14:38, Halil Pasic wrote:
>>
>>
>> On 10/18/2018 09:51 AM, Pierre Morel wrote:
>>> VFIO_CCW_STATE_BOXED and VFIO_CCW_STATE_BUSY have
>>> identical actions for the same events.
>>>
>>> Let's merge both into a single state to simplify the code.
>>> We choose to keep VFIO_CCW_STATE_BUSY.
>>>
>>
>> Your commit message does not convince me. From what I
>> see VFIO_CCW_STATE_BOXED was a bad idea in the first
>> place:
>> * is very very different than DEV_STATE_BOXED,
>> * its role is obscure and undocumented.
>>
>> With your patch however, we get a weird VFIO_CCW_STATE_BUSY -->
>> VFIO_CCW_STATE_BUSY state transition, which I don't consider
>> much better:
>> First we go into VFIO_CCW_STATE_BUSY in fsm_io_request() and then
>> again in fsm_io_helper() which is called by fsm_io_request().
>>
>> So good path used to go
>>
>> VFIO_CCW_STATE_IDLE --> VFIO_CCW_STATE_BOXED --> VFIO_CCW_STATE_BUSY --> ?
>> and with your patch it goes
>> VFIO_CCW_STATE_IDLE --> VFIO_CCW_STATE_BUSY --> VFIO_CCW_STATE_BUSY --> ?
>
> The patch does not deal with state changes.
> Only with merging the two states table entries.
>
Sorry this is not true. Please examine your first hunk. And if you want
to remove a state from a FSM (e.g. by merging it into another state), you
do have to deal with state changes.
Are you willing to do something like
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 797a827..f0705bf 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -28,7 +28,6 @@ static int fsm_io_helper(struct vfio_ccw_private *private)
sch = private->sch;
spin_lock_irqsave(sch->lock, flags);
- private->state = VFIO_CCW_STATE_BUSY;
orb = cp_get_orb(&private->cp, (u32)(addr_t)sch, sch->lpm);
or not? If not, what does speak against it?
>>
>> From what I see, the role of VFIO_CCW_STATE_BOXED is to differentiate
>> between 'doing I/O on the host subchannel' and 'looking at the request,
>> doing translation, and whatever we need to before interacting with the
>> host subchannel'. Normally state is exposed via sysfs, but for vfio-ccw
>> it ain't. Does anybody know why?
>
> I think you are confusing CSS and CCW devices.
> CSS devices do not expose a state. And yes, vfio-ccw name is misleading because vfio-ccw is a css driver. ;)
> (Historical reasons)
Yes, is actually is a subchannel driver, in a sense that the conceptually
driven entity is a subchannel. CSS stands for channel subysytem, and in Linux
for the bus on which all subchannels live.
The names are quite confusing, but I'm not confused by the names.
I'm rather asking myself, what information should be available to the
management software and to the host admin about a passed trough subchannel
and the device sitting behind it.
IMHO what users want to accomplish is passing-through a device, and not
passing through a subchannel.
>
> The vfio-ccw driver do not expose this state to userland or even inside the kernel. It is purely internal.
>
> Exposing the mediated device internal state is not foreseen in this patch.
>
That was obvious. I asked the question because it was convenient to.
> Regards,
> Pierre
>
>
