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

[Cornelia Huck]

On Tue, 19 Sep 2017 14:32:33 +0200
Halil Pasic <address@hidden> wrote:

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

The issue is not that the series is problematic, but that I need to put
a stop on changes at some point in time and just flush my queue -
otherwise this is not workable.

Therefore, I dequeued the series for now. I'll probably do another pull
request in the next weeks anyway - so there's unlikely to be a long
delay, and we can hash out everything without pressure.

It would be best to just think this through; then you can send a v3
with the feedback addressed and your 3270 patch on top.