diff mbox series

[RFC,KERNEL,v3,3/3] PCI/sysfs: Add gsi sysfs for pci_dev

Message ID 20231210161519.1550860-4-Jiqian.Chen@amd.com
State Superseded
Headers show
Series Support device passthrough when dom0 is PVH on Xen | expand

Commit Message

Jiqian Chen Dec. 10, 2023, 4:15 p.m. UTC
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.

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(+)

Comments

Roger Pau Monné Dec. 11, 2023, 5:57 p.m. UTC | #1
On Mon, Dec 11, 2023 at 12:15:19AM +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.
> 
> 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;

It would be better if the gsi if fetched without requiring calling
acpi_pci_irq_enable(), as the gsi doesn't require the interrupt to be
enabled.  The gsi is known at boot time and won't change for the
lifetime of the device.

>  
>  	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);

const

Thanks, Roger.
Jiqian Chen Dec. 12, 2023, 6:34 a.m. UTC | #2
On 2023/12/12 01:57, Roger Pau Monné wrote:
> On Mon, Dec 11, 2023 at 12:15:19AM +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.
>>
>> 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;
> 
> It would be better if the gsi if fetched without requiring calling
> acpi_pci_irq_enable(), as the gsi doesn't require the interrupt to be
> enabled.  The gsi is known at boot time and won't change for the
> lifetime of the device.
Do you have any suggest places to do this?

> 
>>  
>>  	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);
> 
> const
Do you mean "const struct pci_dev *pdev = to_pci_dev(dev);" ?

> 
> Thanks, Roger.
Roger Pau Monné Dec. 12, 2023, 9:18 a.m. UTC | #3
On Tue, Dec 12, 2023 at 06:34:27AM +0000, Chen, Jiqian wrote:
> 
> On 2023/12/12 01:57, Roger Pau Monné wrote:
> > On Mon, Dec 11, 2023 at 12:15:19AM +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.
> >>
> >> 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;
> > 
> > It would be better if the gsi if fetched without requiring calling
> > acpi_pci_irq_enable(), as the gsi doesn't require the interrupt to be
> > enabled.  The gsi is known at boot time and won't change for the
> > lifetime of the device.
> Do you have any suggest places to do this?

I'm not an expert on this, but drivers/pci/pci-sysfs.c would seem like
a better place, together with the rest of the resources.

Maybe my understanding is incorrect, but given the suggested placement
in acpi_pci_irq_enable() I think the device would need to bind the
interrupt in order for the gsi node to appear on sysfs?

Would the current approach work if the device is assigned to pciback
on the kernel command line, and thus never owned by any driver in
dom0?

> > 
> >>  
> >>  	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);
> > 
> > const
> Do you mean "const struct pci_dev *pdev = to_pci_dev(dev);" ?

Yup.

Thanks, Roger.
Jiqian Chen Dec. 13, 2023, 3:31 a.m. UTC | #4
On 2023/12/12 17:18, Roger Pau Monné wrote:
> On Tue, Dec 12, 2023 at 06:34:27AM +0000, Chen, Jiqian wrote:
>>
>> On 2023/12/12 01:57, Roger Pau Monné wrote:
>>> On Mon, Dec 11, 2023 at 12:15:19AM +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.
>>>>
>>>> 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;
>>>
>>> It would be better if the gsi if fetched without requiring calling
>>> acpi_pci_irq_enable(), as the gsi doesn't require the interrupt to be
>>> enabled.  The gsi is known at boot time and won't change for the
>>> lifetime of the device.
>> Do you have any suggest places to do this?
> 
> I'm not an expert on this, but drivers/pci/pci-sysfs.c would seem like
> a better place, together with the rest of the resources.
I'm not familiar with this too. But it seems pci-sysfs.c only creates sysfs node and supports the read/write method without initializing the values.
If want to initialize the value of gsi here. An approach to initialize it is to call acpi_pci_irq_lookup to get gsi number when the first time it is read?

> 
> Maybe my understanding is incorrect, but given the suggested placement
> in acpi_pci_irq_enable() I think the device would need to bind the
> interrupt in order for the gsi node to appear on sysfs?
No, gsi sysfs has existed there, in acpi_pci_irq_enable is to initialize the value of gsi.

> 
> Would the current approach work if the device is assigned to pciback
> on the kernel command line, and thus never owned by any driver in
> dom0?
If assigned to pciback, I think pciback will enable the device, and then acpi_pci_irq_enable will be called, and then the gsi will be initialized. So, current can work.

> 
>>>
>>>>  
>>>>  	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);
>>>
>>> const
>> Do you mean "const struct pci_dev *pdev = to_pci_dev(dev);" ?
> 
> Yup.
> 
> Thanks, Roger.
Roger Pau Monné Dec. 13, 2023, 12:12 p.m. UTC | #5
On Wed, Dec 13, 2023 at 03:31:21AM +0000, Chen, Jiqian wrote:
> On 2023/12/12 17:18, Roger Pau Monné wrote:
> > On Tue, Dec 12, 2023 at 06:34:27AM +0000, Chen, Jiqian wrote:
> >>
> >> On 2023/12/12 01:57, Roger Pau Monné wrote:
> >>> On Mon, Dec 11, 2023 at 12:15:19AM +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.
> >>>>
> >>>> 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;
> >>>
> >>> It would be better if the gsi if fetched without requiring calling
> >>> acpi_pci_irq_enable(), as the gsi doesn't require the interrupt to be
> >>> enabled.  The gsi is known at boot time and won't change for the
> >>> lifetime of the device.
> >> Do you have any suggest places to do this?
> > 
> > I'm not an expert on this, but drivers/pci/pci-sysfs.c would seem like
> > a better place, together with the rest of the resources.
> I'm not familiar with this too. But it seems pci-sysfs.c only creates sysfs node and supports the read/write method without initializing the values.
> If want to initialize the value of gsi here. An approach to initialize it is to call acpi_pci_irq_lookup to get gsi number when the first time it is read?

Hm, maybe, I don't really have much experience with sysfs, so don't
know how nodes are usually initialized.

> > 
> > Maybe my understanding is incorrect, but given the suggested placement
> > in acpi_pci_irq_enable() I think the device would need to bind the
> > interrupt in order for the gsi node to appear on sysfs?
> No, gsi sysfs has existed there, in acpi_pci_irq_enable is to initialize the value of gsi.
> 
> > 
> > Would the current approach work if the device is assigned to pciback
> > on the kernel command line, and thus never owned by any driver in
> > dom0?
> If assigned to pciback, I think pciback will enable the device, and then acpi_pci_irq_enable will be called, and then the gsi will be initialized. So, current can work.

This needs checking to be sure, I'm certainly not that familiar.  You
would need to at least test that it works properly when the device is
hidden using xen-pciback.hide=(SBDF) in the Linux kernel command line.

Thanks, Roger.
Jiqian Chen Dec. 14, 2023, 7:08 a.m. UTC | #6
On 2023/12/13 20:12, Roger Pau Monné wrote:
> On Wed, Dec 13, 2023 at 03:31:21AM +0000, Chen, Jiqian wrote:
>> On 2023/12/12 17:18, Roger Pau Monné wrote:
>>> On Tue, Dec 12, 2023 at 06:34:27AM +0000, Chen, Jiqian wrote:
>>>>
>>>> On 2023/12/12 01:57, Roger Pau Monné wrote:
>>>>> On Mon, Dec 11, 2023 at 12:15:19AM +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.
>>>>>>
>>>>>> 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;
>>>>>
>>>>> It would be better if the gsi if fetched without requiring calling
>>>>> acpi_pci_irq_enable(), as the gsi doesn't require the interrupt to be
>>>>> enabled.  The gsi is known at boot time and won't change for the
>>>>> lifetime of the device.
>>>> Do you have any suggest places to do this?
>>>
>>> I'm not an expert on this, but drivers/pci/pci-sysfs.c would seem like
>>> a better place, together with the rest of the resources.
>> I'm not familiar with this too. But it seems pci-sysfs.c only creates sysfs node and supports the read/write method without initializing the values.
>> If want to initialize the value of gsi here. An approach to initialize it is to call acpi_pci_irq_lookup to get gsi number when the first time it is read?
> 
> Hm, maybe, I don't really have much experience with sysfs, so don't
> know how nodes are usually initialized.
Maybe the maintainers of sysfs can give some suggest places to initialize the value of gsi.

> 
>>>
>>> Maybe my understanding is incorrect, but given the suggested placement
>>> in acpi_pci_irq_enable() I think the device would need to bind the
>>> interrupt in order for the gsi node to appear on sysfs?
>> No, gsi sysfs has existed there, in acpi_pci_irq_enable is to initialize the value of gsi.
>>
>>>
>>> Would the current approach work if the device is assigned to pciback
>>> on the kernel command line, and thus never owned by any driver in
>>> dom0?
>> If assigned to pciback, I think pciback will enable the device, and then acpi_pci_irq_enable will be called, and then the gsi will be initialized. So, current can work.
> 
> This needs checking to be sure, I'm certainly not that familiar.  You
> would need to at least test that it works properly when the device is
> hidden using xen-pciback.hide=(SBDF) in the Linux kernel command line.
Sure, I have validated it on my side. Both the "Static assignment for built-in xen-pciback(xen-pciback.hide=(SBDF))" and the "Dynamic assignment with xl(sudo modprobe xen-pciback & sudo xl pci-assignable-add SBDF)" can work fine with current implementation.

> 
> Thanks, Roger.
Roger Pau Monné Dec. 14, 2023, 8:46 a.m. UTC | #7
On Thu, Dec 14, 2023 at 07:08:32AM +0000, Chen, Jiqian wrote:
> On 2023/12/13 20:12, Roger Pau Monné wrote:
> > On Wed, Dec 13, 2023 at 03:31:21AM +0000, Chen, Jiqian wrote:
> >> On 2023/12/12 17:18, Roger Pau Monné wrote:
> >>> On Tue, Dec 12, 2023 at 06:34:27AM +0000, Chen, Jiqian wrote:
> >>>>
> >>>> On 2023/12/12 01:57, Roger Pau Monné wrote:
> >>>>> On Mon, Dec 11, 2023 at 12:15:19AM +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.
> >>>>>>
> >>>>>> 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;
> >>>>>
> >>>>> It would be better if the gsi if fetched without requiring calling
> >>>>> acpi_pci_irq_enable(), as the gsi doesn't require the interrupt to be
> >>>>> enabled.  The gsi is known at boot time and won't change for the
> >>>>> lifetime of the device.
> >>>> Do you have any suggest places to do this?
> >>>
> >>> I'm not an expert on this, but drivers/pci/pci-sysfs.c would seem like
> >>> a better place, together with the rest of the resources.
> >> I'm not familiar with this too. But it seems pci-sysfs.c only creates sysfs node and supports the read/write method without initializing the values.
> >> If want to initialize the value of gsi here. An approach to initialize it is to call acpi_pci_irq_lookup to get gsi number when the first time it is read?
> > 
> > Hm, maybe, I don't really have much experience with sysfs, so don't
> > know how nodes are usually initialized.
> Maybe the maintainers of sysfs can give some suggest places to initialize the value of gsi.
> 
> > 
> >>>
> >>> Maybe my understanding is incorrect, but given the suggested placement
> >>> in acpi_pci_irq_enable() I think the device would need to bind the
> >>> interrupt in order for the gsi node to appear on sysfs?
> >> No, gsi sysfs has existed there, in acpi_pci_irq_enable is to initialize the value of gsi.
> >>
> >>>
> >>> Would the current approach work if the device is assigned to pciback
> >>> on the kernel command line, and thus never owned by any driver in
> >>> dom0?
> >> If assigned to pciback, I think pciback will enable the device, and then acpi_pci_irq_enable will be called, and then the gsi will be initialized. So, current can work.
> > 
> > This needs checking to be sure, I'm certainly not that familiar.  You
> > would need to at least test that it works properly when the device is
> > hidden using xen-pciback.hide=(SBDF) in the Linux kernel command line.
> Sure, I have validated it on my side. Both the "Static assignment for built-in xen-pciback(xen-pciback.hide=(SBDF))" and the "Dynamic assignment with xl(sudo modprobe xen-pciback & sudo xl pci-assignable-add SBDF)" can work fine with current implementation.

Oh, OK, if that's the case I don't have much objection in doing the
initialization in acpi_pci_irq_enable(), that's an internal Linux
detail.  I mostly care about the GSI being exposed in sysfs in a
non-Xen specific way.

Thanks, Roger.
Jiqian Chen Dec. 14, 2023, 9:03 a.m. UTC | #8
On 2023/12/14 16:46, Roger Pau Monné wrote:
> On Thu, Dec 14, 2023 at 07:08:32AM +0000, Chen, Jiqian wrote:
>> On 2023/12/13 20:12, Roger Pau Monné wrote:
>>> On Wed, Dec 13, 2023 at 03:31:21AM +0000, Chen, Jiqian wrote:
>>>> On 2023/12/12 17:18, Roger Pau Monné wrote:
>>>>> On Tue, Dec 12, 2023 at 06:34:27AM +0000, Chen, Jiqian wrote:
>>>>>>
>>>>>> On 2023/12/12 01:57, Roger Pau Monné wrote:
>>>>>>> On Mon, Dec 11, 2023 at 12:15:19AM +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.
>>>>>>>>
>>>>>>>> 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;
>>>>>>>
>>>>>>> It would be better if the gsi if fetched without requiring calling
>>>>>>> acpi_pci_irq_enable(), as the gsi doesn't require the interrupt to be
>>>>>>> enabled.  The gsi is known at boot time and won't change for the
>>>>>>> lifetime of the device.
>>>>>> Do you have any suggest places to do this?
>>>>>
>>>>> I'm not an expert on this, but drivers/pci/pci-sysfs.c would seem like
>>>>> a better place, together with the rest of the resources.
>>>> I'm not familiar with this too. But it seems pci-sysfs.c only creates sysfs node and supports the read/write method without initializing the values.
>>>> If want to initialize the value of gsi here. An approach to initialize it is to call acpi_pci_irq_lookup to get gsi number when the first time it is read?
>>>
>>> Hm, maybe, I don't really have much experience with sysfs, so don't
>>> know how nodes are usually initialized.
>> Maybe the maintainers of sysfs can give some suggest places to initialize the value of gsi.
>>
>>>
>>>>>
>>>>> Maybe my understanding is incorrect, but given the suggested placement
>>>>> in acpi_pci_irq_enable() I think the device would need to bind the
>>>>> interrupt in order for the gsi node to appear on sysfs?
>>>> No, gsi sysfs has existed there, in acpi_pci_irq_enable is to initialize the value of gsi.
>>>>
>>>>>
>>>>> Would the current approach work if the device is assigned to pciback
>>>>> on the kernel command line, and thus never owned by any driver in
>>>>> dom0?
>>>> If assigned to pciback, I think pciback will enable the device, and then acpi_pci_irq_enable will be called, and then the gsi will be initialized. So, current can work.
>>>
>>> This needs checking to be sure, I'm certainly not that familiar.  You
>>> would need to at least test that it works properly when the device is
>>> hidden using xen-pciback.hide=(SBDF) in the Linux kernel command line.
>> Sure, I have validated it on my side. Both the "Static assignment for built-in xen-pciback(xen-pciback.hide=(SBDF))" and the "Dynamic assignment with xl(sudo modprobe xen-pciback & sudo xl pci-assignable-add SBDF)" can work fine with current implementation.
> 
> Oh, OK, if that's the case I don't have much objection in doing the
> initialization in acpi_pci_irq_enable(), that's an internal Linux
> detail.  I mostly care about the GSI being exposed in sysfs in a
> non-Xen specific way.
Yes, current implementation is a Linux internal way, not a Xen specific. In baremetal Linux, I also can see gsi sysfs. Thank you.

> 
> Thanks, Roger.
diff mbox series

Patch

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 60ca768bc867..7ef9060b239c 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)