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

[Halil Pasic]

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".

How shall we proceed?

Halil

>>>>  
>>>>> +            return -EINVAL; /* channel program check */
>>>>> +
>>>>> +        }
>>>>> +        ret = address_space_rw(&address_space_memory, idaw_addr,
>>>>> +                               MEMTXATTRS_UNSPECIFIED, (void *) 
>>>>> &idaw.fmt1,
>>>>> +                               sizeof(idaw.fmt1), false);
>>>>> +        cds->cda = be64_to_cpu(idaw.fmt1);>>>>> +    }
>>>>> +    ++(cds->at_idaw);
>>>>> +    if (ret != MEMTX_OK) {
>>>>> +        /* assume inaccessible address */
>>>>> +        return -EINVAL; /* channel program check */
>>>>> +
>>>>> +    }
>>>>> +    return 0;
>>>>> +}