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

[Dong Jia Shi]

* Halil Pasic <address@hidden> [2017-09-19 14:04:03 +0200]:

I have no problem with the rest parts of the discussion in this thread.

> 
> 
> 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.
> > 
My fault... This is the address of an IDAW, not the content (data
address) in an IDAW. So what Halil pointed out is the right direction to
go I think.

I will review in the thread of the new version (v3).

> 
> 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.
Right. This is what I actually wanted to say.

> 
> 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;
> >>>>> +}

-- 
Dong Jia Shi