Re: [RFC KERNEL PATCH v6 3/3] xen/privcmd: Add new syscall to get gsi from irq

["Chen, Jiqian" <Jiqian.Chen@amd.com>]

Hi,
On 2024/5/10 17:06, Chen, Jiqian wrote:
> Hi,
> 
> On 2024/5/10 14:46, Jürgen Groß wrote:
>> On 19.04.24 05:36, Jiqian Chen wrote:
>>> In PVH dom0, it uses the linux local interrupt mechanism,
>>> when it allocs irq for a gsi, it is dynamic, and follow
>>> the principle of applying first, distributing first. And
>>> the irq number is alloced from small to large, but the
>>> applying gsi number is not, may gsi 38 comes before gsi 28,
>>> it causes the irq number is not equal with the gsi number.
>>> And when passthrough a device, QEMU will use device's gsi
>>> number to do pirq mapping, but the gsi number is got from
>>> file /sys/bus/pci/devices/<sbdf>/irq, irq!= gsi, so it will
>>> fail when mapping.
>>> And in current linux codes, there is no method to translate
>>> irq to gsi for userspace.
>>>
>>> For above purpose, record the relationship of gsi and irq
>>> when PVH dom0 do acpi_register_gsi_ioapic for devices and
>>> adds a new syscall into privcmd to let userspace can get
>>> that translation when they have a need.
>>>
>>> Co-developed-by: Huang Rui <ray.huang@amd.com>
>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>>> ---
>>>   arch/x86/include/asm/apic.h      |  8 +++++++
>>>   arch/x86/include/asm/xen/pci.h   |  5 ++++
>>>   arch/x86/kernel/acpi/boot.c      |  2 +-
>>>   arch/x86/pci/xen.c               | 21 +++++++++++++++++
>>>   drivers/xen/events/events_base.c | 39 ++++++++++++++++++++++++++++++++
>>>   drivers/xen/privcmd.c            | 19 ++++++++++++++++
>>>   include/uapi/xen/privcmd.h       |  7 ++++++
>>>   include/xen/events.h             |  5 ++++
>>>   8 files changed, 105 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
>>> index 9d159b771dc8..dd4139250895 100644
>>> --- a/arch/x86/include/asm/apic.h
>>> +++ b/arch/x86/include/asm/apic.h
>>> @@ -169,6 +169,9 @@ extern bool apic_needs_pit(void);
>>>     extern void apic_send_IPI_allbutself(unsigned int vector);
>>>   +extern int acpi_register_gsi_ioapic(struct device *dev, u32 gsi,
>>> +                    int trigger, int polarity);
>>> +
>>>   #else /* !CONFIG_X86_LOCAL_APIC */
>>>   static inline void lapic_shutdown(void) { }
>>>   #define local_apic_timer_c2_ok        1
>>> @@ -183,6 +186,11 @@ static inline void apic_intr_mode_init(void) { }
>>>   static inline void lapic_assign_system_vectors(void) { }
>>>   static inline void lapic_assign_legacy_vector(unsigned int i, bool r) { }
>>>   static inline bool apic_needs_pit(void) { return true; }
>>> +static inline int acpi_register_gsi_ioapic(struct device *dev, u32 gsi,
>>> +                    int trigger, int polarity)
>>> +{
>>> +    return (int)gsi;
>>> +}
>>>   #endif /* !CONFIG_X86_LOCAL_APIC */
>>>     #ifdef CONFIG_X86_X2APIC
>>> diff --git a/arch/x86/include/asm/xen/pci.h b/arch/x86/include/asm/xen/pci.h
>>> index 9015b888edd6..aa8ded61fc2d 100644
>>> --- a/arch/x86/include/asm/xen/pci.h
>>> +++ b/arch/x86/include/asm/xen/pci.h
>>> @@ -5,6 +5,7 @@
>>>   #if defined(CONFIG_PCI_XEN)
>>>   extern int __init pci_xen_init(void);
>>>   extern int __init pci_xen_hvm_init(void);
>>> +extern int __init pci_xen_pvh_init(void);
>>>   #define pci_xen 1
>>>   #else
>>>   #define pci_xen 0
>>> @@ -13,6 +14,10 @@ static inline int pci_xen_hvm_init(void)
>>>   {
>>>       return -1;
>>>   }
>>> +static inline int pci_xen_pvh_init(void)
>>> +{
>>> +    return -1;
>>> +}
>>>   #endif
>>>   #ifdef CONFIG_XEN_PV_DOM0
>>>   int __init pci_xen_initial_domain(void);
>>> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
>>> index 85a3ce2a3666..72c73458c083 100644
>>> --- a/arch/x86/kernel/acpi/boot.c
>>> +++ b/arch/x86/kernel/acpi/boot.c
>>> @@ -749,7 +749,7 @@ static int acpi_register_gsi_pic(struct device *dev, u32 gsi,
>>>   }
>>>     #ifdef CONFIG_X86_LOCAL_APIC
>>> -static int acpi_register_gsi_ioapic(struct device *dev, u32 gsi,
>>> +int acpi_register_gsi_ioapic(struct device *dev, u32 gsi,
>>>                       int trigger, int polarity)
>>>   {
>>>       int irq = gsi;
>>> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
>>> index 652cd53e77f6..f056ab5c0a06 100644
>>> --- a/arch/x86/pci/xen.c
>>> +++ b/arch/x86/pci/xen.c
>>> @@ -114,6 +114,21 @@ static int acpi_register_gsi_xen_hvm(struct device *dev, u32 gsi,
>>>                    false /* no mapping of GSI to PIRQ */);
>>>   }
>>>   +static int acpi_register_gsi_xen_pvh(struct device *dev, u32 gsi,
>>> +                    int trigger, int polarity)
>>> +{
>>> +    int irq;
>>> +
>>> +    irq = acpi_register_gsi_ioapic(dev, gsi, trigger, polarity);
>>> +    if (irq < 0)
>>> +        return irq;
>>> +
>>> +    if (xen_pvh_add_gsi_irq_map(gsi, irq) == -EEXIST)
>>> +        printk(KERN_INFO "Already map the GSI :%u and IRQ: %d\n", gsi, irq);
>>> +
>>> +    return irq;
>>> +}
>>> +
>>>   #ifdef CONFIG_XEN_PV_DOM0
>>>   static int xen_register_gsi(u32 gsi, int triggering, int polarity)
>>>   {
>>> @@ -558,6 +573,12 @@ int __init pci_xen_hvm_init(void)
>>>       return 0;
>>>   }
>>>   +int __init pci_xen_pvh_init(void)
>>> +{
>>> +    __acpi_register_gsi = acpi_register_gsi_xen_pvh;
>>
>> No support for unregistering the gsi again?
> __acpi_unregister_gsi is set in function acpi_set_irq_model_ioapic.
> Maybe I need to use a new function to call acpi_unregister_gsi_ioapic and remove the mapping of irq and gsi from xen_irq_list_head ?
When I tried to support unregistering the gsi and removing the mapping during disable device,
I encountered that after running "xl pci-assignable-add 03:00.0", callstack pcistub_init_device->xen_pcibk_reset_device->pci_disable_device->pcibios_disable_device->acpi_pci_irq_disable->__acpi_unregister_gsi
removed the mapping, after that when user space called xen_gsi_from_irq to get gsi, it failed.

To cover above case, I want to change the implementation of xen_gsi_from_irq to pass sbdf to get the gsi instead of passing irq,
Because the sbdf and gsi of a device is unique and wiil not be changed even device is disabled or re-enabled.

Do you think this kind of change is acceptable?

> 
>>
>>> +    return 0;
>>> +}
>>> +
>>
>> Juergen
> 

-- 
Best regards,
Jiqian Chen.