Re: [RFC KERNEL PATCH v4 3/3] PCI/sysfs: Add gsi sysfs for pci_dev

From: "Chen, Jiqian" <Jiqian.Chen@amd.com>
To: Bjorn Helgaas <helgaas@kernel.org>,
	"Rafael J . Wysocki" <rafael@kernel.org>,
	Len Brown <lenb@kernel.org>
Cc: "Juergen Gross" <jgross@suse.com>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Oleksandr Tyshchenko" <oleksandr_tyshchenko@epam.com>,
	"Boris Ostrovsky" <boris.ostrovsky@oracle.com>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>,
	"Hildebrand, Stewart" <Stewart.Hildebrand@amd.com>,
	"Huang, Ray" <Ray.Huang@amd.com>,
	"Chen, Jiqian" <Jiqian.Chen@amd.com>,
	"Ragiadakou, Xenia" <Xenia.Ragiadakou@amd.com>
Subject: Re: [RFC KERNEL PATCH v4 3/3] PCI/sysfs: Add gsi sysfs for pci_dev
Date: Tue, 23 Jan 2024 10:13:52 +0000	[thread overview]
Message-ID: <BL1PR12MB5849D7A9EC2BEB55CAF3A889E7742@BL1PR12MB5849.namprd12.prod.outlook.com> (raw)
In-Reply-To: <20240122233731.GA291870@bhelgaas>

On 2024/1/23 07:37, Bjorn Helgaas wrote:
> On Fri, Jan 05, 2024 at 02:22:17PM +0800, Jiqian Chen wrote:
>> There is a need for some scenarios to use gsi sysfs.
>> For example, when xen passthrough a device to dumU, it will
>> use gsi to map pirq, but currently userspace can't get gsi
>> number.
>> So, add gsi sysfs for that and for other potential scenarios.
> 
> Isn't GSI really an ACPI-specific concept?
I also added the Maintains of ACPI to get some inputs.
Hi Rafael J. Wysocki and Len Brown, do you have any suggestions about this patch?

> 
> I don't know enough about Xen to know why it needs the GSI in
> userspace.  Is this passthrough brand new functionality that can't be
> done today because we don't expose the GSI yet?
In Xen architecture, there is a privileged domain named Dom0 that has ACPI support and is responsible for detecting and controlling the hardware, also it performs privileged operations such as the creation of normal (unprivileged) domains DomUs. When we give to a DomU direct access to a device, we need also to route the physical interrupts to the DomU. In order to do so Xen needs to setup and map the interrupts appropriately. For the case of GSI interrupts, since Xen does not have support to get the ACPI routing info in the hypervisor itself, it needs to get this info from Dom0. One way would be for this info to be exposed in sysfs and the xen toolstack that runs in Dom0's userspace to get this info reading sysfs and then pass it to Xen.

And I have tried another approach in the past version patches that keeping irq to gsi mappings and then xen tool was consulting the map via a syscall and was passing the info to Xen. But it was rejected by Xen maintainers because they thought the mappings and translations were all Linux internal actions, and has nothing to do with Xen, so they suggested me to expose the GSI in sysfs because it is cleaner and easier to retrieve it in userspace.
This is my past version:
Kernel: https://lore.kernel.org/lkml/20231124103123.3263471-1-Jiqian.Chen@amd.com/T/#m8d20edd326cf7735c2804f0371e8a63b6beec60c
Xen: https://lore.kernel.org/xen-devel/20231124104136.3263722-1-Jiqian.Chen@amd.com/T/#m9f9068d558822af0a5b28cd241cab4d779e36974

> 
> How does userspace use the GSI?  I see "to map pirq", but I think we
> should have more concrete details about exactly what is needed and how
> it is used before adding something new in sysfs.
As above reason.

> 
> Is there some more generic kernel interface we could use
> for this?
No, there is no method for now, I think.

> 
> s/dumU/DomU/ ?  (I dunno, but https://www.google.com/search?q=xen+dumu
> suggests it :))
> 
>> Co-developed-by: Huang Rui <ray.huang@amd.com>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>> ---
>>  drivers/acpi/pci_irq.c  |  1 +
>>  drivers/pci/pci-sysfs.c | 11 +++++++++++
>>  include/linux/pci.h     |  2 ++
>>  3 files changed, 14 insertions(+)
>>
>> diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
>> index 630fe0a34bc6..739a58755df2 100644
>> --- a/drivers/acpi/pci_irq.c
>> +++ b/drivers/acpi/pci_irq.c
>> @@ -449,6 +449,7 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
>>  		kfree(entry);
>>  		return 0;
>>  	}
>> +	dev->gsi = gsi;
>>  
>>  	rc = acpi_register_gsi(&dev->dev, gsi, triggering, polarity);
>>  	if (rc < 0) {
>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>> index 2321fdfefd7d..c51df88d079e 100644
>> --- a/drivers/pci/pci-sysfs.c
>> +++ b/drivers/pci/pci-sysfs.c
>> @@ -71,6 +71,16 @@ static ssize_t irq_show(struct device *dev,
>>  }
>>  static DEVICE_ATTR_RO(irq);
>>  
>> +static ssize_t gsi_show(struct device *dev,
>> +			struct device_attribute *attr,
>> +			char *buf)
>> +{
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +
>> +	return sysfs_emit(buf, "%u\n", pdev->gsi);
>> +}
>> +static DEVICE_ATTR_RO(gsi);
>> +
>>  static ssize_t broken_parity_status_show(struct device *dev,
>>  					 struct device_attribute *attr,
>>  					 char *buf)
>> @@ -596,6 +606,7 @@ static struct attribute *pci_dev_attrs[] = {
>>  	&dev_attr_revision.attr,
>>  	&dev_attr_class.attr,
>>  	&dev_attr_irq.attr,
>> +	&dev_attr_gsi.attr,
>>  	&dev_attr_local_cpus.attr,
>>  	&dev_attr_local_cpulist.attr,
>>  	&dev_attr_modalias.attr,
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index dea043bc1e38..0618d4a87a50 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -529,6 +529,8 @@ struct pci_dev {
>>  
>>  	/* These methods index pci_reset_fn_methods[] */
>>  	u8 reset_methods[PCI_NUM_RESET_METHODS]; /* In priority order */
>> +
>> +	unsigned int	gsi;
>>  };
>>  
>>  static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
>> -- 
>> 2.34.1
>>
>>

-- 
Best regards,
Jiqian Chen.