Received: by 2002:a05:6a10:8c0a:0:0:0:0 with SMTP id go10csp4179054pxb; Mon, 1 Feb 2021 14:53:36 -0800 (PST) X-Google-Smtp-Source: ABdhPJwJNNwi1smC+B8tgxqP+v7LpZF16e9vSPE/21NwjteaXAVcbP0pUeuqXjdcNeflgyfKvIfo X-Received: by 2002:a05:6402:4ce:: with SMTP id n14mr21270471edw.309.1612220015993; Mon, 01 Feb 2021 14:53:35 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1612220015; cv=none; d=google.com; s=arc-20160816; b=zMT7Bs0EVhK2r6HnC+b3tPsnnTZ3CNNM45Kh1RyJLe9OiMdr4Vka/dVNIXVnwXl5Ea zg5xccmZzwPOoQ9C8aVpHEQqvcEaGPIXeYt3uVfVbSbxHoRa7Q9tBVOGUDehltBfgNfX 57tMMJdYSMlV3VNXu0v2eN07rFpIlZynzdpKX2+8022mrTMwYyPiOCrB0TfMIqy7OW6a P25ijamB9+BvPFcuc/8PEO/myDzSmX5CfHdefom98rOgx3kfxHnC6EE54WDiwwMXHy/7 BMGWVDRdkxITxkX9TSu7mfPnx8zh0FAysHzlm4YYsxfaW20KcNfpII7Ehknce4Wvp06T 6aag== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :references:message-id:subject:cc:to:from:date:ironport-sdr :ironport-sdr; bh=1FQF2WV5z7OMwaSdOdMxljrwkZQixms7J0jZ+mFtHIk=; b=WDNXxxyTHQHUE94sBgSjRGN5CbLK9+ijdcjtd7pyhNJy6k+jxYGirSz+/+/tXzLFqK b41ja6XwQkGEDEyJKHDtqx3QOS5JHiT1WJM0PpfQGnurO/WtqSD+0j1BfD5czW3H99uG MS6oK/zKsvHkkqKmYDNBxSDh60Xt9p53ghFmhvilbJOw4c9cldfnQhTHHVYAYMi9wLmw Z1GpjT5BdYzHcV8q8AxehSmY7KToVmmFZK8l+onompELrROzH/H8zntda60Po2DXuKFz weluVW5328W5JJXkrORN12qInVVj++qyFySYuQWSIiN/AbZhw1YDaeOW7+/jQs3AAgGp eqDw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=intel.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id hd10si5446776ejc.208.2021.02.01.14.53.11; Mon, 01 Feb 2021 14:53:35 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230092AbhBAWvh (ORCPT + 99 others); Mon, 1 Feb 2021 17:51:37 -0500 Received: from mga06.intel.com ([134.134.136.31]:7376 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229537AbhBAWvf (ORCPT ); Mon, 1 Feb 2021 17:51:35 -0500 IronPort-SDR: cJb3lk1A3o2TRSkYcsftZYQzplplRi6NGDhMqnig6YQjEGQH+YWaMc78g/J1DfbzDQ/6/d4hoP dWc/hYh92xjg== X-IronPort-AV: E=McAfee;i="6000,8403,9882"; a="242282256" X-IronPort-AV: E=Sophos;i="5.79,393,1602572400"; d="scan'208";a="242282256" Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 01 Feb 2021 14:50:54 -0800 IronPort-SDR: oHhKkHh06bdvRvwNm4jSmHYo4PebBn4mdxQpmKigtdPMjAR5Owzp9NEOOAlwauFaaJNwXsXR65 iOJnaFfVq0DA== X-IronPort-AV: E=Sophos;i="5.79,393,1602572400"; d="scan'208";a="371722457" Received: from jambrizm-mobl1.amr.corp.intel.com (HELO intel.com) ([10.252.133.15]) by orsmga002-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 01 Feb 2021 14:50:53 -0800 Date: Mon, 1 Feb 2021 14:50:52 -0800 From: Ben Widawsky To: David Rientjes Cc: linux-cxl@vger.kernel.org, linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org, linux-nvdimm@lists.01.org, linux-pci@vger.kernel.org, Bjorn Helgaas , Chris Browy , Christoph Hellwig , Dan Williams , Ira Weiny , Jon Masters , Jonathan Cameron , Rafael Wysocki , Randy Dunlap , Vishal Verma , daniel.lll@alibaba-inc.com, "John Groves (jgroves)" , "Kelley, Sean V" Subject: Re: [PATCH 03/14] cxl/mem: Find device capabilities Message-ID: <20210201225052.vrrvuxrsgmddjzbb@intel.com> References: <20210130002438.1872527-1-ben.widawsky@intel.com> <20210130002438.1872527-4-ben.widawsky@intel.com> <234711bf-c03f-9aca-e0b5-ca677add3ea@google.com> <20210201165352.wi7tzpnd4ymxlms4@intel.com> <32f33dd-97a-8b1c-d488-e5198a3d7748@google.com> <20210201215857.ud5cpg7hbxj2j5bx@intel.com> <20210201222859.lzw3gvxuqebukvr6@intel.com> <20210201223314.qh24uxd7ajdppgfl@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 21-02-01 14:45:00, David Rientjes wrote: > On Mon, 1 Feb 2021, Ben Widawsky wrote: > > > > > > > > > > +static int cxl_mem_setup_mailbox(struct cxl_mem *cxlm) > > > > > > > > > +{ > > > > > > > > > + const int cap = cxl_read_mbox_reg32(cxlm, CXLDEV_MB_CAPS_OFFSET); > > > > > > > > > + > > > > > > > > > + cxlm->mbox.payload_size = > > > > > > > > > + 1 << CXL_GET_FIELD(cap, CXLDEV_MB_CAP_PAYLOAD_SIZE); > > > > > > > > > + > > > > > > > > > + /* 8.2.8.4.3 */ > > > > > > > > > + if (cxlm->mbox.payload_size < 256) { > > > > > > > > > + dev_err(&cxlm->pdev->dev, "Mailbox is too small (%zub)", > > > > > > > > > + cxlm->mbox.payload_size); > > > > > > > > > + return -ENXIO; > > > > > > > > > + } > > > > > > > > > > > > > > > > Any reason not to check cxlm->mbox.payload_size > (1 << 20) as well and > > > > > > > > return ENXIO if true? > > > > > > > > > > > > > > If some crazy vendor wanted to ship a mailbox larger than 1M, why should the > > > > > > > driver not allow it? > > > > > > > > > > > > > > > > > > > Because the spec disallows it :) > > > > > > > > > > I don't see it being the driver's responsibility to enforce spec correctness > > > > > though. In certain cases, I need to use the spec, like I have to pick /some/ > > > > > mailbox timeout. For other cases... > > > > > > > > > > I'm not too familiar with what other similar drivers may or may not do in > > > > > situations like this. The current 256 limit is mostly a reflection of that being > > > > > too small to even support advertised mandatory commands. So things can't work in > > > > > that scenario, but things can work if they have a larger register size (so long > > > > > as the BAR advertises enough space). > > > > > > > > > > > > > I don't think things can work above 1MB, either, though. Section > > > > 8.2.8.4.5 specifies 20 bits to define the payload length, if this is > > > > larger than cxlm->mbox.payload_size it would venture into the reserved > > > > bits of the command register. > > > > > > > > So is the idea to allow cxl_mem_setup_mailbox() to succeed with a payload > > > > size > 1MB and then only check 20 bits for the command register? > > > > > > So it's probably a spec bug, but actually the payload size is 21 bits... I'll > > > check if that was a mistake. > > > > Well I guess they wanted to be able to specify 1M exactly... Spec should > > probably say you can't go over 1M > > > > I think that's what 8.2.8.4.3 says, no? And then 8.2.8.4.5 says you > can use up to Payload Size. That's why my recommendation was to enforce > this in cxl_mem_setup_mailbox() up front. Yeah. I asked our spec people to update 8.2.8.4.5 to make it clearer. I'd argue the intent is how you describe it, but the implementation isn't. My argument was silly anyway because if you specify greater than 1M as your payload, you will get EINVAL at the ioctl. The value of how it works today is the driver will at least bind and allow you to interact with it. How strongly do you feel about this?