Re: [Qemu-devel] [PATCH v3 4/5] 390x/css: introduce maximum data address checking

[Halil Pasic]

On 09/20/2017 10:06 AM, Cornelia Huck wrote:
> On Tue, 19 Sep 2017 20:27:44 +0200
> Halil Pasic <address@hidden> wrote:
> 
>> The architecture mandates the addresses to be  accessed on the first
>> indirection level (that is, the data addresses without IDA, and the
>> (M)IDAW addresses with (M)IDA) to be checked against an CCW format
>> dependent limit maximum address.  If a violation is detected, the storage
>> access is not to be performed and a channel program check needs to be
>> generated. As of today, we fail to do this check.
>>
>> Let us stick even closer to the architecture specification.
>>
>> Signed-off-by: Halil Pasic <address@hidden>
>> ---
>>  hw/s390x/css.c         | 10 ++++++++++
>>  include/hw/s390x/css.h |  1 +
>>  2 files changed, 11 insertions(+)
>>
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index 6b0cd8861b..2d37a9ddde 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -795,6 +795,11 @@ static inline int cds_check_len(CcwDataStream *cds, int 
>> len)
>>      return cds->flags & CDS_F_STREAM_BROKEN ? -EINVAL : len;
>>  }
>>  
>> +static inline bool cds_ccw_addrs_ok(hwaddr addr, int len, bool ccw_fmt1)
> 
> cds_cda_limit_ok?
> 

I use cda to point to the 2 level in case of IDA. This is about
level 1 (addressed by the ccw directly). That's why I used ccw_addrs
but if you think cds_cda_limit_ok is better I can live with that.

We could also think about renaming cds->cda. Btw what does cda stand
for (channel data address is my guess)?

Regards,
Halil

>> +{
>> +    return (addr + len) < (ccw_fmt1 ? (1UL << 31) : (1UL << 24));
>> +}
>> +
>>  static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len,
>>                                    CcwDataStreamOp op)
>>  {
> 
> Looks good.
>