On 18.10.2017 11:52, Cornelia Huck wrote:
> On Wed, 18 Oct 2017 11:30:47 +0200
> Thomas Huth <th...@redhat.com> wrote:
>
>> On 17.10.2017 16:04, Halil Pasic wrote:
>>> Simplify the error handling of the SSCH and RSCH handler avoiding
>>> arbitrary and cryptic error codes being used to tell how the instruction
>>> is supposed to end. Let the code detecting the condition tell how it's
>>> to be handled in a less ambiguous way. It's best to handle SSCH and RSCH
>>> in one go as the emulation of the two shares a lot of code.
>>>
>>> For passthrough this change isn't pure refactoring, but changes the way
>>> kernel reported EFAULT is handled. After clarifying the kernel interface
>>> we decided that EFAULT shall be mapped to unit exception. Same goes for
>>> unexpected error codes and absence of required ORB flags.
>>>
>>> Signed-off-by: Halil Pasic <pa...@linux.vnet.ibm.com>
>>> ---
>>> hw/s390x/css.c | 84
>>> +++++++++++++--------------------------------
>>> hw/s390x/s390-ccw.c | 11 +++---
>>> hw/vfio/ccw.c | 28 +++++++++++----
>>> include/hw/s390x/css.h | 23 +++++++++----
>>> include/hw/s390x/s390-ccw.h | 2 +-
>>> target/s390x/ioinst.c | 53 ++++------------------------
>>> 6 files changed, 75 insertions(+), 126 deletions(-)
>>>
>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>>> index aa233d5f8a..ff5a05c34b 100644
>>> --- a/hw/s390x/css.c
>>> +++ b/hw/s390x/css.c
>>> @@ -1181,12 +1181,11 @@ static void sch_handle_start_func_virtual(SubchDev
>>> *sch)
>>>
>>> }
>>>
>>> -static int sch_handle_start_func_passthrough(SubchDev *sch)
>>> +static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch)
>>> {
>>>
>>> PMCW *p = &sch->curr_status.pmcw;
>>> SCSW *s = &sch->curr_status.scsw;
>>> - int ret;
>>>
>>> ORB *orb = &sch->orb;
>>> if (!(s->ctrl & SCSW_ACTL_SUSP)) {
>>> @@ -1200,31 +1199,12 @@ static int
>>> sch_handle_start_func_passthrough(SubchDev *sch)
>>> */
>>> if (!(orb->ctrl0 & ORB_CTRL0_MASK_PFCH) ||
>>> !(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
>>> - return -EINVAL;
>>> + warn_report("vfio-ccw requires PFCH and C64 flags set...");
>>
>> Not sure, but should this maybe rather be a
>> "qemu_log_mask(LOG_GUEST_ERROR, ...)" instead?
>
> Is that visible by default, though? I'd rather want the admin to be
> able to find a hint in a log somewhere why the guest I/O is rejected.
Well, the guest could also trigger this condition on purpose (e.g.
kvm-unit-tests), so I wonder whether we want to see the warning in that
case, too...
IMHO this is exactly what qemu_log_mask(LOG_GUEST_ERROR, ...) has been
implemented for: Log errors from the guest in case you suspect that the
guest is doing something wrong. But that's just my 0.02 €, feel free to
ignore me.
>>> @@ -1844,27 +1816,23 @@ void css_do_schm(uint8_t mbk, int update, int dct,
>>> uint64_t mbo)
>>> }
>>> }
>>>
>>> -int css_do_rsch(SubchDev *sch)
>>> +IOInstEnding css_do_rsch(SubchDev *sch)
>>> {
>>> SCSW *s = &sch->curr_status.scsw;
>>> PMCW *p = &sch->curr_status.pmcw;
>>> - int ret;
>>>
>>> if (~(p->flags) & (PMCW_FLAGS_MASK_DNV | PMCW_FLAGS_MASK_ENA)) {
>>> - ret = -ENODEV;
>>> - goto out;
>>> + return IOINST_CC_NOT_OPERATIONAL;
>>> }
>>>
>>> if (s->ctrl & SCSW_STCTL_STATUS_PEND) {
>>> - ret = -EINPROGRESS;
>>> - goto out;
>>> + return IOINST_CC_STATUS_PRESENT;
>>> }
>>>
>>> if (((s->ctrl & SCSW_CTRL_MASK_FCTL) != SCSW_FCTL_START_FUNC) ||
>>> (s->ctrl & SCSW_ACTL_RESUME_PEND) ||
>>> (!(s->ctrl & SCSW_ACTL_SUSP))) {
>>> - ret = -EINVAL;
>>> - goto out;
>>> + return IOINST_CC_BUSY;
>>
>> Why is EINVAL now mapped to IOINST_CC_BUSY? Shouldn't that be
>> IOINST_CC_STATUS_PRESENT instead?
>
> No, that is correct (see the PoP for when cc 2 is supposed to be set by
> rsch).
So if this is on purpose, this change in behavior should also be
mentioned in the patch description, I think.
Thomas
