On 4/21/20 8:28 AM, Cornelia Huck wrote:
> On Fri, 17 Apr 2020 04:34:39 +0200
> Eric Farman <far...@linux.ibm.com> wrote:
>
>> We have a use case (vfio-ccw) where a CRW is already built and
>> ready to use. Rather than teasing out the components just to
>> reassemble it later, let's rework this code so we can queue a
>> fully-qualified CRW directly.
>>
>> Signed-off-by: Eric Farman <far...@linux.ibm.com>
>> ---
>> hw/s390x/css.c | 44 ++++++++++++++++++++++++++++--------------
>> include/hw/s390x/css.h | 1 +
>> 2 files changed, 30 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index a44faa3549..a72c09adbe 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -2170,30 +2170,23 @@ void css_subch_assign(uint8_t cssid, uint8_t ssid,
>> uint16_t schid,
>> }
>> }
>>
>> -void css_queue_crw(uint8_t rsc, uint8_t erc, int solicited,
>> - int chain, uint16_t rsid)
>> +void css_queue_crw_cont(CRW crw)
>
> Don't really like this name, as it makes me think of 'continuation'
> instead of 'container'.
>
> css_queue_crw_container?
> css_crw_add_to_queue?
>
> Naming is hard :(
Yeah, I don't like it either. Just took it from the variable "crw_cont"
in the original code, but it does seem more like a continuation. Since
the "container" is something built in that routine, maybe the
"add_to_queue" variant fits better.
>
>> {
>> CrwContainer *crw_cont;
>>
>> - trace_css_crw(rsc, erc, rsid, chain ? "(chained)" : "");
>> + trace_css_crw((crw.flags & CRW_FLAGS_MASK_RSC) >> 8,
>> + crw.flags & CRW_FLAGS_MASK_ERC,
>> + crw.rsid,
>> + (crw.flags & CRW_FLAGS_MASK_C) ? "(chained)" : "");
>> +
>> /* TODO: Maybe use a static crw pool? */
>> crw_cont = g_try_new0(CrwContainer, 1);
>> if (!crw_cont) {
>> channel_subsys.crws_lost = true;
>> return;
>
> Now that we actually pass something in, do we want to inform the caller
> whether the crw was queued or not?
Hrm... Well I guess we could use it to break out of our loop in patch
7. But for the existing callers of css_queue_crw(), it doesn't provide
much value to anyone but css_generate_css_crws(). I'll poke at this a bit.
>
>> }
>> - crw_cont->crw.flags = (rsc << 8) | erc;
>> - if (solicited) {
>> - crw_cont->crw.flags |= CRW_FLAGS_MASK_S;
>> - }
>> - if (chain) {
>> - crw_cont->crw.flags |= CRW_FLAGS_MASK_C;
>> - }
>> - crw_cont->crw.rsid = rsid;
>> - if (channel_subsys.crws_lost) {
>> - crw_cont->crw.flags |= CRW_FLAGS_MASK_R;
>> - channel_subsys.crws_lost = false;
>> - }
>> +
>> + crw_cont->crw = crw;
>>
>> QTAILQ_INSERT_TAIL(&channel_subsys.pending_crws, crw_cont, sibling);
>>
>
> Generally, looks sane to me.
>
