Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 25 Aug 2022 08:47:25 -0700
From:   Ira Weiny <ira.weiny@intel.com>
To:     Jonathan Cameron <Jonathan.Cameron@huawei.com>
CC:     Dan Williams <dan.j.williams@intel.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Alison Schofield <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        Ben Widawsky <bwidawsk@kernel.org>,
        <linux-cxl@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-pci@vger.kernel.org>
Subject: Re: [PATCH V2 1/2] PCI: Allow drivers to request exclusive config
 regions
Message-ID: <YweZjRYVcT5uCg2i@iweiny-desk3>
References: <20220824232450.723179-1-ira.weiny@intel.com>
 <20220824232450.723179-2-ira.weiny@intel.com>
 <20220825160658.000051a6@huawei.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20220825160658.000051a6@huawei.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?RD7sRe8jD5AvRXyjyX0WbC3O/te8OV2MW2ppzn8oXvpXIL0aL9yHyUtQ6Akp?=
 =?us-ascii?Q?ckCp+uzAq7S3dKdqIiXdz5DsQrnpbkW81Y0Dj1gn+d5N75B4WwqO0Bu3e3Zi?=
 =?us-ascii?Q?BkBfjBGJR8A+o/QGER9oEUGAsaiD+EQpdkHHpfaMPQEde13Mu9F+ee0NXUjX?=
 =?us-ascii?Q?TzfeHrEeKBjer36jGspwRfgdqG9jLvyduvty5CSBw/3///PF8DevzaQX6IVk?=
 =?us-ascii?Q?PJMtS/l8oMoTTsV8TbWArlMP9uCOBsnwgkfHMJV9pgvu2yG5PeyZF1CvNNRC?=
 =?us-ascii?Q?Yv6tIbdSvL7aXj4Idtv9up6QPMDNA6CpCkE9Vjzudy9II2XlBgvNyp3CVLqa?=
 =?us-ascii?Q?h4/rS7s65sfDX0jtgINzHYusrHLqPDG6FG2dXDPR/NvjT1CUvSTVdLkEODpE?=
 =?us-ascii?Q?BhMJot+/CXlGkl7urKhbKEuHMM8hz0zOKDW+0GT/g57O7F4WY+hEJl6kVFip?=
 =?us-ascii?Q?taMCDY1LvbVfcnExZ7UXvIUN1727MaVe6WsfAKBN7uoWLL/QN45Mq5hc0pOE?=
 =?us-ascii?Q?kiy4R0UJ/cAcbYOOnek3JFxkrgFXmFf+Yy2aPwBkGwUHIAsLILqKSyu0Gdr6?=
 =?us-ascii?Q?XJjFqbMHFstB+atywvMbe7Jy8ijLGtKo/OnN1auRsMciSf4C/4tZhQPiJxB+?=
 =?us-ascii?Q?pyt+4cxqNcwZJE9HicrPwdzwZ+IqvwLgZgFkgcafFyMmOoZ/MpggB3/CfKEe?=
 =?us-ascii?Q?UQVufGbC5C6JwWShdw7eUa2WCs4lQB7igHstVHPwAIMl/yRyvUgrYDVORodq?=
 =?us-ascii?Q?sqNQ0tY3fV8+PEx97o1Ju/9u5msafWCHD+wjEadMPAlMo6ANQjEjI5xDwV7w?=
 =?us-ascii?Q?LmPrc3ipxEE6yC6EL5FfCYXMighd+W0N4248N5iRSuzQJhKG8PI1+qJBG2mX?=
 =?us-ascii?Q?Q4kW2FUNWFwc35SHHbKylEOdpxw8TQsOPfKYgZtlXBjsvxOKQ8pDXibYNmG1?=
 =?us-ascii?Q?ikYUXQAThCFe2eoYo1XMAb/Q9zMTbSUDHXi8VQ2337pP2YGIC9TS4TmL52Ou?=
 =?us-ascii?Q?um1diDnAkg7QOr32CtbHsU5gzJPTjUEWRvtcTbLJP2YcuzdfuEehXFWWvq34?=
 =?us-ascii?Q?t30cGmhUGKuAHJpEh7Odas4q5ugnxlLm5+h8RJjwt4w/W+k1wQ5WDcPvXEd5?=
 =?us-ascii?Q?N3UHunsCf6nwVRqEYkRmgWqI+qkP/+QWpEj7hQiZRCsJEIX0TN1zhtvCzYYM?=
 =?us-ascii?Q?LZ1cWTkrzgQdZZSpdbGoUy26MoXXG5zFkKgDrG/jUiPRKZvNDOy5HyTIbVWo?=
 =?us-ascii?Q?2dZLvUEVz1PPoGN5ZyjuHbIi2tG8zBJwYdDO9h4eZ3R0Os1CAniecFLOjGSH?=
 =?us-ascii?Q?cw3WEg1Na1iXdXGLKlPmNlsJse71yxTjJK29BnyY64ZnTVqEInbjsn/r9n+W?=
 =?us-ascii?Q?oub1znmHSFTIJhzo9RWyGPa+Z/t5TUojp0mG/nb7mUWSH1GUQMbXZNqVQB7j?=
 =?us-ascii?Q?erxIMWYIrG4qzCCWYzBFo2pXyoINoxgYpnRd46594SiMC0ESSpA7DuYXwmam?=
 =?us-ascii?Q?wd6zQOEFK9C6Ly5VVxxMaJEvJRGFPLZywdulroq1nGLGEiB/euZ3bjS8RWN/?=
 =?us-ascii?Q?05/QRuz7/yVaQzBbPM4g8qE4tyiRgl8mPv1IxJsD?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 74c748fd-629a-4090-09eb-08da86b11d93
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Aug 2022 15:47:29.1105
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: w53bfHEMUTcRg6TGXFP/YRl12VRO0fm04o0cb1JnWMt+ZxnE076+SYpBIP5dhaXEsFlse3CUO1dwlnXhlYUMiA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB4090
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Aug 25, 2022 at 04:06:58PM +0100, Jonathan Cameron wrote:
> On Wed, 24 Aug 2022 16:24:49 -0700
> ira.weiny@intel.com wrote:
> 
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > PCI config space access from user space has traditionally been
> > unrestricted with writes being an understood risk for device operation.
> > 
> > Unfortunately, device breakage or odd behavior from config writes lacks
> > indicators that can leave driver writers confused when evaluating
> > failures.  This is especially true with the new PCIe Data Object
> > Exchange (DOE) mailbox protocol where backdoor shenanigans from user
> > space through things such as vendor defined protocols may affect device
> > operation without complete breakage.
> > 
> > A prior proposal restricted read and writes completely.[1]  Greg and
> > Bjorn pointed out that proposal is flawed for a couple of reasons.
> > First, lspci should always be allowed and should not interfere with any
> > device operation.  Second, setpci is a valuable tool that is sometimes
> > necessary and it should not be completely restricted.[2]  Finally
> > methods exist for full lock of device access if required.
> > 
> > Even though access should not be restricted it would be nice for driver
> > writers to be able to flag critical parts of the config space such that
> > interference from user space can be detected.
> > 
> > Introduce pci_request_config_region_exclusive() to mark exclusive config
> > regions.  Such regions trigger a warning and kernel taint if accessed
> > via user space.
> > 
> > Create pci_warn_once() to restrict the user from spamming the log.
> > 
> > [1] https://lore.kernel.org/all/161663543465.1867664.5674061943008380442.stgit@dwillia2-desk3.amr.corp.intel.com/
> > [2] https://lore.kernel.org/all/YF8NGeGv9vYcMfTV@kroah.com/
> > 
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Suggested-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> One comment inline.
> 
> I'm not totally convinced of the necessity of this, but done this way
> it has very little impact so I'm fine with it.
> 
> Other than the comment about not realigning things...
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thanks!

[snip]

> >  /* drivers/pci/bus.c */
> >  void pci_add_resource(struct list_head *resources, struct resource *res);
> >  void pci_add_resource_offset(struct list_head *resources, struct resource *res,
> > @@ -2486,14 +2502,15 @@ void pci_uevent_ers(struct pci_dev *pdev, enum  pci_ers_result err_type);
> >  #define pci_printk(level, pdev, fmt, arg...) \
> >  	dev_printk(level, &(pdev)->dev, fmt, ##arg)
> >  
> > -#define pci_emerg(pdev, fmt, arg...)	dev_emerg(&(pdev)->dev, fmt, ##arg)
> > -#define pci_alert(pdev, fmt, arg...)	dev_alert(&(pdev)->dev, fmt, ##arg)
> > -#define pci_crit(pdev, fmt, arg...)	dev_crit(&(pdev)->dev, fmt, ##arg)
> > -#define pci_err(pdev, fmt, arg...)	dev_err(&(pdev)->dev, fmt, ##arg)
> > -#define pci_warn(pdev, fmt, arg...)	dev_warn(&(pdev)->dev, fmt, ##arg)
> > -#define pci_notice(pdev, fmt, arg...)	dev_notice(&(pdev)->dev, fmt, ##arg)
> > -#define pci_info(pdev, fmt, arg...)	dev_info(&(pdev)->dev, fmt, ##arg)
> > -#define pci_dbg(pdev, fmt, arg...)	dev_dbg(&(pdev)->dev, fmt, ##arg)
> > +#define pci_emerg(pdev, fmt, arg...)	 dev_emerg(&(pdev)->dev, fmt, ##arg)
> > +#define pci_alert(pdev, fmt, arg...)	 dev_alert(&(pdev)->dev, fmt, ##arg)
> > +#define pci_crit(pdev, fmt, arg...)	 dev_crit(&(pdev)->dev, fmt, ##arg)
> > +#define pci_err(pdev, fmt, arg...)	 dev_err(&(pdev)->dev, fmt, ##arg)
> > +#define pci_warn(pdev, fmt, arg...)	 dev_warn(&(pdev)->dev, fmt, ##arg)
> > +#define pci_warn_once(pdev, fmt, arg...) dev_warn_once(&(pdev)->dev, fmt, ##arg)
> > +#define pci_notice(pdev, fmt, arg...)	 dev_notice(&(pdev)->dev, fmt, ##arg)
> > +#define pci_info(pdev, fmt, arg...)	 dev_info(&(pdev)->dev, fmt, ##arg)
> > +#define pci_dbg(pdev, fmt, arg...)	 dev_dbg(&(pdev)->dev, fmt, ##arg)
> 
> This realignment is a lot of noise.  Do we really care about one diffentlyu
> aligned entry? + if you are going to do it two tabs rather than a space
> following the tab (I think that's what you have here?)

I struggled a bit on this.  Not aligning makes the final code look odd while
the patch looks good.  Aligning with 2 tabs pushes everything past the 80 col
standard.

This seemed like a good compromise.

Thanks for the review,
Ira