Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA

[Halil Pasic]

On 09/19/2017 02:23 PM, Cornelia Huck wrote:
> On Tue, 19 Sep 2017 14:04:03 +0200
> Halil Pasic <address@hidden> wrote:
> 
>> On 09/19/2017 12:57 PM, Cornelia Huck wrote:
>>>>>>> +static inline int ida_read_next_idaw(CcwDataStream *cds)
>>>>>>> +{
>>>>>>> +    union {uint64_t fmt2; uint32_t fmt1; } idaw;      
>>>>>>                                            ^
>>>>>> Nit.
>>>>>>    
>>>> Maybe checkpatch wanted it this way. My memories are blurry.  
>>>
>>> I'd just leave it like that, tbh.
>>>   
>>>>>>> +    bool is_fmt2 = cds->flags & CDS_F_C64;
>>>>>>> +    int ret;
>>>>>>> +    hwaddr idaw_addr;
>>>>>>> +
>>>>>>> +    if (is_fmt2) {
>>>>>>> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
>>>>>>> +        if (idaw_addr & 0x07) {
>>>>>>> +            return -EINVAL; /* channel program check */
>>>>>>> +        }
>>>>>>> +        ret = address_space_rw(&address_space_memory, idaw_addr,
>>>>>>> +                               MEMTXATTRS_UNSPECIFIED, (void *) 
>>>>>>> &idaw.fmt2,
>>>>>>> +                               sizeof(idaw.fmt2), false);
>>>>>>> +        cds->cda = be64_to_cpu(idaw.fmt2);  
>>
>>
>>>>>>> +    } else {
>>>>>>> +        idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
>>>>>>> +        if (idaw_addr & 0x03) {      
>>>>>> ?:
>>>>>> (idaw_addr & 0x80000003)    
>>>>> Yes.
>>>>>     
>>>> I will double check this. Does not seem unreasonable but
>>>> double-checking is better.  
>>> Please let me know. I think the architecture says that the bit must be
>>> zero, and that we may (...) generate a channel program check.
>>>   
>>
>> Not exactly. The more significant bits part of the check
>> depend on the ccw format. This needs to be done for both
>> idaw formats. I would need to introduce a new flag, or
>> access the SubchDev to do this properly.
>>
>> Architecturally we also need to check the data addresses
>> from which we read so we have nothing bigger than 
>> (1 << 31) - 1 if we are working with format-1 idaws.
>>
>> I also think we did not take proper care of proper
>> maximum data address checks prior CwwDataStream which also
>> depend on the ccw format (in absence of IDAW or MIDAW).
>>
>> The ccw format dependent maximum address checks are (1 << 24) - 1
>> and (1 << 31) - 1 respectively for format-0 and format-1 (on
>> the first indirection level that is for non-IDA for the data,
>> and for (M)IDA for the (M)IDAWs).
>>
>> Reference:
>> PoP pages 16-25 and 16-26 "Invalid IDAW or MIDAW Addre" and
>> "Invalid Data Address".
> 
> Sigh, when are things ever easy :(
> 
> I'll need some time to review this. But more urgently, I need a break.
> 
>>
>> How shall we proceed?
> 
> Given the discussion about this patch in particular, I'll probably
> dequeue it and send it with the next batch of patches. We can also
> include the other improvements, then. Not sure whether I should just
> dequeue this one or the whole series (I really want to get the rest of
> the patches out...)
> 
> More after the break :)
> 

I think I can fix this pretty fast, and fixing this later is
an option too in my opinion as the patch is consistent with our
prior art (we ignored this maximum address checking requirement,
and we keep ignoring it). 

I would prefer not dequeue-ing the whole series because a
3270 fix of mine (which just made the internal review) depends
on this. IMHO, it's not like the series is fundamentally broken,
not ain't an improvement at all. It is not perfect, but it's
IMHO certainly an improvement over what we have.

Regards,
Halil