Date:   Wed, 6 Jun 2018 16:15:31 +0200
From:   Pierre Morel <pmorel@...ux.ibm.com>
To:     Cornelia Huck <cohuck@...hat.com>
Cc:     Dong Jia Shi <bjsdjshi@...ux.ibm.com>,
        Halil Pasic <pasic@...ux.ibm.com>, linux-s390@...r.kernel.org,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        qemu-s390x@...gnu.org, qemu-devel@...gnu.org
Subject: Re: [PATCH RFC 2/2] vfio-ccw: support for halt/clear subchannel

On 06/06/2018 14:21, Cornelia Huck wrote:
> On Tue, 5 Jun 2018 17:23:02 +0200
> Pierre Morel <pmorel@...ux.ibm.com> wrote:
>
>> On 05/06/2018 15:14, Cornelia Huck wrote:
>>> On Tue, 22 May 2018 17:10:44 +0200
>>> Pierre Morel <pmorel@...ux.ibm.com> wrote:
>>>   
>>>> On 22/05/2018 14:52, Cornelia Huck wrote:
>>>>> On Wed, 16 May 2018 15:32:48 +0200
>>>>> Pierre Morel <pmorel@...ux.ibm.com> wrote:
>>>>>      
>>>>>> On 15/05/2018 18:10, Cornelia Huck wrote:
>>>>>>> On Fri, 11 May 2018 11:33:35 +0200
>>>>>>> Pierre Morel <pmorel@...ux.ibm.com> wrote:
>>>>>>>         
>>>>>>>> On 09/05/2018 17:48, Cornelia Huck wrote:
...snip...
> Not sure if I parse this correctly... but the architecture says that
> the subchannel has the {start,halt,clear} function set as a result of
> {start,halt,clear} subchannel, doesn't it?
The fc field of the SCSW indicates the functions pending or in progress
resulting of the execution of the instructions START, CLEAR or HALT which
have been accepted by the subchannel.
Up to 2 functions being pending or in progress.
The fc field is only updated when the condition code is set to 0 during 
the execution
of the instruction.

1) The guest do a SSCH instruction
2) we intercept and QEMU issue the write with SSCH bit set
3) In the driver we are called in write and issue the SSCH instruction
4) the subchannel (the real one) set the start bit in fc field

later
1) The guest do a CSCH instruction
2) we intercept and QEMU issue the write with CSCH bit set
3) In the driver we are called in write and issue the CSCH instruction
4) the subchannel (the real one) set the clear bit in fc and clear the 
start bit


The subchannel accept to handle functions (start, halt, clear) 
asynchronously
otherwise it could not accept a CLEAR instruction to stop a previous START
instruction.
But the instructions are issued one after the other to the sub-channel.

I think we can only agree on this and that we had at some point a 
misunderstanding.


>
>>>   
>>>> I do not understand the "processing all in one function".
>>>>
>>>> Since yo already have 3 function to process these three instructions.
>>>>
>>>> Do you mean the if .. else if .. else if ?
>>> Yes. There is a lot of common handling for each of these.
>> There are also differences and it breaks the FSM
> Depends on what we will do with the fsm ;)

Yes :)

>
>>>   
>>>> Then I come back to what you said earlier on the precedence of the clear
>>>> instruction:
>>>>
>>>> 1) do we have a use case to have more than one bit set in the fctl field?
>>>>
>>>> - if no, there is no need for precedence
>>> It mirrors what the hardware does: you just set an additional bit if
>>> processing has not yet finished.
>> I do not agree, this is true for the SCSW but not for instructions.
>> We receive instructions in VFIO and give back status.
>> The name used to provide the command is misleading.
> Confused. What we get over the interface is an scsw (the current scsw
> for the subchannel in QEMU). A halt does set an additional bit in the
> fctl if the start is not yet finished.
>
> But that had me re-reading the PoP, and clear is indeed different (it,
> ah, clears the other bits). So, clear handling is different enough from
> the others, and I'm not sure anymore whether it makes sense to handle
> start and halt together. I'll rework this.

OK, it matches with my comprehension.

>
>>>   
>>>> - if yes, why should clear have precedence ?
>>> Because it does on the hardware?
>> What you say is right if we would have a register inside the subchannel
>> where we write the commands.
>> But this is not what we handle we handle separate instructions coming
>> from an instruction stream.
>>
>> We do never receive two instructions at the same time, but each after
>> the other.
>> If the sub-channel is busy on IO a clear or a cancel must be able to
>> stop the IO.
>> I agree upon this.
>> But we do not have any other command in the same call.
>>
>> If we would construct the interface differently, for example using an
>> mmap() system
>> call and let the user ORing the command bitfield before using an ioctl
>> to inform
>> us from the change, or if we poll on the command bitfield we should
>> implement
>> it like you say.
>> But this is not what we do, and this is not what the architecture does.
>> does it?
> The thing is that the guest does not interact with this interface at
> all, it is just the backend implementation. The instructions set the
> bits in the scsw fctl field, and we get the scsw from QEMU. By the
> architecture, both start and halt may be set in the fctl at the same
> time. [That this currently does not happen because QEMU is not really
> handling things asynchronously is an implementation detail.]

I do not understand the point of sending a halt and a start to the same
sub-channel at the same time?

If QEMU, once asynchronous, can do this, it could just
halt the start before asking this to the backend. Don't it?

Another point is that the start must have been accepted by the
sub-channel for the start bit in the fc field of the SCSW to be set.

>
>>>   
>>>>      How do QEMU set more than one bit in fctl?
>>>>      why should we alter the order of the instructions given by the guest?
>>>>      How can we know this order if there are multiple instructions at once?
>>> In the future, we should return after we fired off the start etc.
>>> request even if we did not receive an interrupt yet, so that the guest
>>> might do a halt or clear before the start has finished.
>> This is already what is done here:
>> We fire off the start (go to BUSY state) and return
>>
>> If the guest want to start another command it polls on
>> the vfio_write() untill the channel isn't BUSY anymore.
> It's not the guest that polls, but QEMU (resp. another user space
> program). And it should be able to fire a halt etc. even if the start
> function is still active, as the architecture allows that. (I would
> expect the real hardware to give us a busy if applicable anyway.)

I do not find where QEMU is waiting when the sub-channel is busy.
I found a loop on EAGAIN (which is never returned but by the system AFAIU)
and I missed the place where a retry is implemented when I followed
the back calls.

>
...snip...
> On re-reading the PoP, the three of them do have some different
> requirements:
>
> - cancel is currently handled completely in QEMU, and it also has quite
>    different semantics as it is not asynchronous etc.
> - halt can be made pending in addition to a start function
> - clear will need to clear anything that's currently going on
>
> I'll need to rethink some of this; probably does not make sense for you
> to try to integrate my patches.
>

Make sense (that it doesn't (make sense)).

Regards,

Pierre



-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Powered by blists - more mailing lists