| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<BL1PR12MB58493D17E23751A06FC931DDE7E72@BL1PR12MB5849.namprd12.prod.outlook.com>
Date: Fri, 10 May 2024 09:06:12 +0000
From: "Chen, Jiqian" <Jiqian.Chen@....com>
To: Jürgen Groß <jgross@...e.com>
CC: Stefano Stabellini <sstabellini@...nel.org>, Bjorn Helgaas
<bhelgaas@...gle.com>, "Rafael J . Wysocki" <rafael@...nel.org>,
Roger Pau Monné <roger.pau@...rix.com>,
"xen-devel@...ts.xenproject.org" <xen-devel@...ts.xenproject.org>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>, "Huang, Ray"
<Ray.Huang@....com>, "Chen, Jiqian" <Jiqian.Chen@....com>
Subject: Re: [RFC KERNEL PATCH v6 3/3] xen/privcmd: Add new syscall to get gsi
from irq
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@....com>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@....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 ?
>
>> + return 0;
>> +}
>> +
>> #ifdef CONFIG_XEN_PV_DOM0
>> int __init pci_xen_initial_domain(void)
>> {
>> diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
>> index 27553673e46b..80d4f7faac64 100644
>> --- a/drivers/xen/events/events_base.c
>> +++ b/drivers/xen/events/events_base.c
>> @@ -953,6 +953,43 @@ int xen_irq_from_gsi(unsigned gsi)
>> }
>> EXPORT_SYMBOL_GPL(xen_irq_from_gsi);
>> +int xen_gsi_from_irq(unsigned irq)
>> +{
>> + struct irq_info *info;
>> +
>> + list_for_each_entry(info, &xen_irq_list_head, list) {
>> + if (info->type != IRQT_PIRQ)
>> + continue;
>> +
>> + if (info->irq == irq)
>> + return info->u.pirq.gsi;
>> + }
>> +
>> + return -1;
>> +}
>> +EXPORT_SYMBOL_GPL(xen_gsi_from_irq);
>> +
>> +int xen_pvh_add_gsi_irq_map(unsigned gsi, unsigned irq)
>> +{
>> + int tmp_irq;
>> + struct irq_info *info;
>> +
>> + tmp_irq = xen_irq_from_gsi(gsi);
>> + if (tmp_irq != -1)
>> + return -EEXIST;
>> +
>> + info = kzalloc(sizeof(*info), GFP_KERNEL);
>> + if (info == NULL)
>> + panic("Unable to allocate metadata for GSI%d\n", gsi);
>
> Please don't kill the system here, just return -ENOMEM.
Will change in next version.
>
>> +
>> + info->type = IRQT_PIRQ;
I am considering whether I need to use a new type(like IRQT_GSI) here to distinguish with IRQT_PIRQ, because function restore_pirqs will process all IRQT_PIRQ.
>> + info->irq = irq;
>> + info->u.pirq.gsi = gsi;
>> + list_add_tail(&info->list, &xen_irq_list_head);
>
> I think you need some kind of locking to protect changing of the list against
> concurrent accesses.
OK, will add a lock in next version.
>
>> +
>> + return 0;
>> +}
>> +
>> static void __unbind_from_irq(struct irq_info *info, unsigned int irq)
>> {
>> evtchn_port_t evtchn;
>> @@ -2295,6 +2332,8 @@ void __init xen_init_IRQ(void)
>> xen_init_setup_upcall_vector();
>> xen_alloc_callback_vector();
>> + if (xen_pvh_domain())
>> + pci_xen_pvh_init();
>> if (xen_hvm_domain()) {
>> native_init_IRQ();
>> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
>> index 67dfa4778864..11feed529e1d 100644
>> --- a/drivers/xen/privcmd.c
>> +++ b/drivers/xen/privcmd.c
>> @@ -842,6 +842,21 @@ static long privcmd_ioctl_mmap_resource(struct file *file,
>> return rc;
>> }
>> +static long privcmd_ioctl_gsi_from_irq(struct file *file, void __user *udata)
>> +{
>> + struct privcmd_gsi_from_irq kdata;
>> +
>> + if (copy_from_user(&kdata, udata, sizeof(kdata)))
>> + return -EFAULT;
>> +
>> + kdata.gsi = xen_gsi_from_irq(kdata.irq);
>> +
>> + if (copy_to_user(udata, &kdata, sizeof(kdata)))
>> + return -EFAULT;
>> +
>> + return 0;
>
> Shouldn't you return an error if xen_gsi_from_irq() returned -1?
Oh, will change in next version.
>
>
> Juergen
--
Best regards,
Jiqian Chen.
Powered by blists - more mailing lists