From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-qk1-f180.google.com (mail-qk1-f180.google.com [209.85.222.180]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 3F26022099 for ; Fri, 22 Mar 2024 17:09:41 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.222.180 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1711127382; cv=none; b=NBdG29Nt8hUg1dcU0387/X5u02RhH7Y/U+eAB4jDbuVLz+zsZpXElAY4cUPMPtPhLnv0sKboFDWEgx21xR8mqGicPkxCRtwXTnKN3VTNgIx1z6yMU9tkt6OzgEBEvCKuf+V7Cjum8g9K/WW6UHkYFaj3fcT+3C3F6Sevmco5oyY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1711127382; c=relaxed/simple; bh=7ur9VHXBBmtR/BzJooZLLcq+YeBquA9kLKNbVLwTOXU=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=kEHOusoFEPgtbjzicvF4vodKYFKHdtS/KclFPBfTj1TPD76CsZpDnrlLZ1qSfP1KxDnmkfmLetheV6eZHXprhNvYXu8yycyzfVbDGWdpjLS+765HNMfjtbZq3jhB41QQVwbpzu1k6EXZmRz7pfVovUhHXRYCdN/UYtZkn1jC428= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ziepe.ca; spf=pass smtp.mailfrom=ziepe.ca; dkim=pass (2048-bit key) header.d=ziepe.ca header.i=@ziepe.ca header.b=Ost4oKU1; arc=none smtp.client-ip=209.85.222.180 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ziepe.ca Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=ziepe.ca Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=ziepe.ca header.i=@ziepe.ca header.b="Ost4oKU1" Received: by mail-qk1-f180.google.com with SMTP id af79cd13be357-78a3bccc41dso30901485a.0 for ; Fri, 22 Mar 2024 10:09:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ziepe.ca; s=google; t=1711127380; x=1711732180; darn=lists.linux.dev; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=ApAHAb/kVXuA2vroBRV/sxLLRFwBeKrRqNlXizxetGM=; b=Ost4oKU1etubPl3LgQ5daYlmDWR4FtgBKfBftyNAkeAyGdkMtZ3NPnP9Je8cF/6vkV UfMtmH3+osB3wx3spM1G4ou/ozcFCKRXlmJy6gmieMAWcBBlcsQen/OnhASzqwkrHRYF 3+DzkhGq6dPipCi5MGflaCtnlDx7oLx1c+LpiJfCGszYfhX0Uqqeq9NuCXZHG2dzIYGX fFDEniG4HkObdrtjVh70mgdaNw1cmX/GVwQPyngRgABWvAaDW+9QSr39Fa8lwUuQ0eCE miLqFaSs2hnmt3HHK6bq/XR8nR2HNS3Hx1dhXxq8ULNYK+UkITvX5WBJjxBIeI/QUh5b 6cmQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1711127380; x=1711732180; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=ApAHAb/kVXuA2vroBRV/sxLLRFwBeKrRqNlXizxetGM=; b=Bhxra+9rgsWcQqT3a9x/ZF4cKwXYzs+mcqqt0g5WRZ5a9N9CHYn6RRDjj0p59Xyj81 2nshOwS9vL8WNVHuBOwuJ7tJDHD4FkjuTNiGsqcgZUJ4517Tpks8E7dyyaM2jQVZdySm oYuHuiJ05CT8AzR75TQIJTpR7UuvrovMGlTU8PxAd2pOgZfMNK+wzhoxaPd3e8VdMQBt OvH+W4F9zvgQJIBmBjdh29OswVEpojQY4GABXF3m4Hk3BqdgQyiU8SUXKUHPbhjjbgd7 yIu/vhCNNqXfvpkQnl5AZUlDlQB8phditskyNqFJWmBHjiuHsMAxNyuWWKe+qhL7snqv eqkQ== X-Forwarded-Encrypted: i=1; AJvYcCUq4JJv9fJc4lORIG4FYYhSzgv6NvCPQc2bSBa3uwqUBi6NcVrocC2S6cP3dHVSbTXnb45xIk44b1ugMhebEQwoZ3fML6U= X-Gm-Message-State: AOJu0Yzov/yW3/uUnVmOH8dfcbWdlP9hC7oytoV9JSGH+AFyK6P9UG0R +iDBXLF8+wQ5puVprCKC0vRT7GtO4Cui/UHscIOS0/+AcgIZmf1igEjIf28fhF0= X-Google-Smtp-Source: AGHT+IEL8xR2S4VoPl+Aw327BSxdHoOXFRAq/bqB5JLQ7T86b5yNtZewLe/kyN/nOGUpgLU+Lu7bKg== X-Received: by 2002:ae9:e401:0:b0:78a:2d4b:3b06 with SMTP id q1-20020ae9e401000000b0078a2d4b3b06mr18512qkc.76.1711127380253; Fri, 22 Mar 2024 10:09:40 -0700 (PDT) Received: from ziepe.ca (hlfxns017vw-142-68-80-239.dhcp-dynamic.fibreop.ns.bellaliant.net. [142.68.80.239]) by smtp.gmail.com with ESMTPSA id a12-20020a05620a02ec00b00789fa326156sm921596qko.82.2024.03.22.10.09.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 22 Mar 2024 10:09:39 -0700 (PDT) Received: from jgg by wakko with local (Exim 4.95) (envelope-from ) id 1rniOh-00CSno-4j; Fri, 22 Mar 2024 14:09:39 -0300 Date: Fri, 22 Mar 2024 14:09:39 -0300 From: Jason Gunthorpe To: Baolu Lu Cc: Kevin Tian , Joerg Roedel , Will Deacon , Robin Murphy , Jean-Philippe Brucker , Nicolin Chen , Yi Liu , Jacob Pan , Joel Granados , iommu@lists.linux.dev, virtualization@lists.linux-foundation.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3 4/8] iommufd: Add iommufd fault object Message-ID: <20240322170939.GJ66976@ziepe.ca> References: <20240122073903.24406-1-baolu.lu@linux.intel.com> <20240122073903.24406-5-baolu.lu@linux.intel.com> <20240308180332.GX9225@ziepe.ca> Precedence: bulk X-Mailing-List: iommu@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: On Fri, Mar 15, 2024 at 09:46:06AM +0800, Baolu Lu wrote: > On 3/9/24 2:03 AM, Jason Gunthorpe wrote: > > On Mon, Jan 22, 2024 at 03:38:59PM +0800, Lu Baolu wrote: > > > --- /dev/null > > > +++ b/drivers/iommu/iommufd/fault.c > > > @@ -0,0 +1,255 @@ > > > +// SPDX-License-Identifier: GPL-2.0-only > > > +/* Copyright (C) 2024 Intel Corporation > > > + */ > > > +#define pr_fmt(fmt) "iommufd: " fmt > > > + > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > + > > > +#include "iommufd_private.h" > > > + > > > +static int device_add_fault(struct iopf_group *group) > > > +{ > > > + struct iommufd_device *idev = group->cookie->private; > > > + void *curr; > > > + > > > + curr = xa_cmpxchg(&idev->faults, group->last_fault.fault.prm.grpid, > > > + NULL, group, GFP_KERNEL); > > > + > > > + return curr ? xa_err(curr) : 0; > > > +} > > > + > > > +static void device_remove_fault(struct iopf_group *group) > > > +{ > > > + struct iommufd_device *idev = group->cookie->private; > > > + > > > + xa_store(&idev->faults, group->last_fault.fault.prm.grpid, > > > + NULL, GFP_KERNEL); > > > > xa_erase ? > > Yes. Sure. > > > Is grpid OK to use this way? Doesn't it come from the originating > > device? > > The group ID is generated by the hardware. Here, we use it as an index > in the fault array to ensure it can be quickly retrieved in the page > fault response path. I'm nervous about this, we are trusting HW outside the kernel to provide unique grp id's which are integral to how the kernel operates.. > > > +static ssize_t iommufd_fault_fops_read(struct file *filep, char __user *buf, > > > + size_t count, loff_t *ppos) > > > +{ > > > + size_t fault_size = sizeof(struct iommu_hwpt_pgfault); > > > + struct iommufd_fault *fault = filep->private_data; > > > + struct iommu_hwpt_pgfault data; > > > + struct iommufd_device *idev; > > > + struct iopf_group *group; > > > + struct iopf_fault *iopf; > > > + size_t done = 0; > > > + int rc; > > > + > > > + if (*ppos || count % fault_size) > > > + return -ESPIPE; > > > + > > > + mutex_lock(&fault->mutex); > > > + while (!list_empty(&fault->deliver) && count > done) { > > > + group = list_first_entry(&fault->deliver, > > > + struct iopf_group, node); > > > + > > > + if (list_count_nodes(&group->faults) * fault_size > count - done) > > > + break; > > > + > > > + idev = (struct iommufd_device *)group->cookie->private; > > > + list_for_each_entry(iopf, &group->faults, list) { > > > + iommufd_compose_fault_message(&iopf->fault, &data, idev); > > > + rc = copy_to_user(buf + done, &data, fault_size); > > > + if (rc) > > > + goto err_unlock; > > > + done += fault_size; > > > + } > > > + > > > + rc = device_add_fault(group); > > > > See I wonder if this should be some xa_alloc or something instead of > > trying to use the grpid? > > So this magic number will be passed to user space in the fault message. > And the user will then include this number in its response message. The > response message is valid only when the magic number matches. Do I get > you correctly? Yes, then it is simple xa_alloc() and xa_load() without any other searching and we don't have to rely on the grpid to be correctly formed by the PCI device. But I don't know about performance xa_alloc() is pretty fast but trusting the grpid would be faster.. IMHO from a uapi perspective we should have a definate "cookie" that gets echo'd back. If the kernel uses xa_alloc or grpid to build that cookie it doesn't matter to the uAPI. Jason