From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.6 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 93DBDC4361B for ; Fri, 18 Dec 2020 15:40:54 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 0367C23B54 for ; Fri, 18 Dec 2020 15:40:53 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0367C23B54 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:60480 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1kqHsC-0001bg-PO for qemu-devel@archiver.kernel.org; Fri, 18 Dec 2020 10:40:52 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:58702) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kqHjy-0000Vi-JM for qemu-devel@nongnu.org; Fri, 18 Dec 2020 10:32:22 -0500 Received: from us-smtp-delivery-124.mimecast.com ([63.128.21.124]:51671) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.90_1) (envelope-from ) id 1kqHjw-0003aP-Pi for qemu-devel@nongnu.org; Fri, 18 Dec 2020 10:32:22 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1608305540; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=xOziwv1LEQ+iXlKE0HHfLJTU6f/ND8eTWwJ7aow9xcM=; b=U+08aPIuz8DIWHirNtblty3FaeN8ui46L4FwtBt4JJ/T+ZavYUOjnHUbB1YVxz4ypNL7Ec TioQCUr4a15pT014Pl1U/3QVfCatWORilAzyNwzEN//SR5ICYC2d01SYuAfm5LnuXQLaKY U1JzVv3gZsU5Tn7wqlrzWu/tdrBI+jk= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-26-ulDsiCkQOBqC3x43ah6kIA-1; Fri, 18 Dec 2020 10:32:16 -0500 X-MC-Unique: ulDsiCkQOBqC3x43ah6kIA-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id CF230180A09C; Fri, 18 Dec 2020 15:32:14 +0000 (UTC) Received: from gondolin (ovpn-113-130.ams2.redhat.com [10.36.113.130]) by smtp.corp.redhat.com (Postfix) with ESMTP id 75D3228D05; Fri, 18 Dec 2020 15:32:09 +0000 (UTC) Date: Fri, 18 Dec 2020 16:32:06 +0100 From: Cornelia Huck To: Pierre Morel Subject: Re: [PATCH v2 2/2] s390x/pci: Fix memory_region_access_valid call Message-ID: <20201218163206.7b8efa2a.cohuck@redhat.com> In-Reply-To: <2c5a2ccb-dbe1-f355-3980-462be1d93942@linux.ibm.com> References: <1608243397-29428-1-git-send-email-mjrosato@linux.ibm.com> <1608243397-29428-3-git-send-email-mjrosato@linux.ibm.com> <72f4e03f-7208-6af0-4cd2-9715d9f9ec77@linux.ibm.com> <20201218120440.36b56e80.cohuck@redhat.com> <2c5a2ccb-dbe1-f355-3980-462be1d93942@linux.ibm.com> Organization: Red Hat GmbH MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=cohuck@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Received-SPF: pass client-ip=63.128.21.124; envelope-from=cohuck@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H4=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=unavailable autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: thuth@redhat.com, Matthew Rosato , david@redhat.com, richard.henderson@linaro.org, qemu-devel@nongnu.org, pasic@linux.ibm.com, borntraeger@de.ibm.com, qemu-s390x@nongnu.org Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" On Fri, 18 Dec 2020 15:32:08 +0100 Pierre Morel wrote: > On 12/18/20 12:04 PM, Cornelia Huck wrote: > > On Fri, 18 Dec 2020 10:37:38 +0100 > > Pierre Morel wrote: > > > >> On 12/17/20 11:16 PM, Matthew Rosato wrote: > >>> In pcistb_service_handler, a call is made to validate that the memory > >>> region can be accessed. However, the call is made using the entire length > >>> of the pcistb operation, which can be larger than the allowed memory > >>> access size (8). Since we already know that the provided buffer is a > >>> multiple of 8, fix the call to memory_region_access_valid to iterate > >>> over the memory region in the same way as the subsequent call to > >>> memory_region_dispatch_write. > >>> > >>> Fixes: 863f6f52b7 ("s390: implement pci instructions") > >>> Signed-off-by: Matthew Rosato > >>> --- > >>> hw/s390x/s390-pci-inst.c | 10 ++++++---- > >>> 1 file changed, 6 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c > >>> index e230293..76b08a3 100644 > >>> --- a/hw/s390x/s390-pci-inst.c > >>> +++ b/hw/s390x/s390-pci-inst.c > >>> @@ -821,10 +821,12 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr, > >>> mr = s390_get_subregion(mr, offset, len); > >>> offset -= mr->addr; > >>> > >>> - if (!memory_region_access_valid(mr, offset, len, true, > >>> - MEMTXATTRS_UNSPECIFIED)) { > >>> - s390_program_interrupt(env, PGM_OPERAND, ra); > >>> - return 0; > >>> + for (i = 0; i < len; i += 8) { > >>> + if (!memory_region_access_valid(mr, offset + i, 8, true, > >>> + MEMTXATTRS_UNSPECIFIED)) { > >>> + s390_program_interrupt(env, PGM_OPERAND, ra); > >>> + return 0; > >>> + } > >>> } > >>> > >>> if (s390_cpu_virt_mem_read(cpu, gaddr, ar, buffer, len)) { > >>> > >> > >> wouldn't it be made automatically by defining the io_region > >> max_access_size when reading the bars in clp_service_call? > >> > > > > But that's already what is happening, isn't it? The access check is > > done for a size that is potentially too large, while the actual access > > will happen in chunks of 8? I think that this patch is correct. > > > > Sorry I was too rapid and half wrong in my writing I was also not > specific enough. > > In MemoryRegionOps we have a field valid with a callback accepts(). > > I was wondering if doing the check in the accept() callback which is > called by the memory_region_access_valid() function and then using > max_access_size would not be cleaner. > > Note that it does not change a lot but only where the check is done. But where would we add those ops? My understanding is that pcistb acts on whatever region the device provided, and that differs from device to device?