Messages in this thread | | | Date | Mon, 17 Apr 2023 17:25:57 -0700 | Subject | Re: [PATCH v11 18/26] virt: gunyah: Translate gh_rm_hyp_resource into gunyah_resource | From | Elliot Berman <> |
| |
On 3/31/2023 7:26 AM, Alex Elder wrote: > On 3/3/23 7:06 PM, Elliot Berman wrote: >> When booting a Gunyah virtual machine, the host VM may gain capabilities >> to interact with resources for the guest virtual machine. Examples of >> such resources are vCPUs or message queues. To use those resources, we >> need to translate the RM response into a gunyah_resource structure which >> are useful to Linux drivers. Presently, Linux drivers need only to know >> the type of resource, the capability ID, and an interrupt. >> >> On ARM64 systems, the interrupt reported by Gunyah is the GIC interrupt >> ID number and always a SPI. >> >> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> > > Several comments here, nothing major. -Alex > >> --- >> arch/arm64/include/asm/gunyah.h | 23 +++++ >> drivers/virt/gunyah/rsc_mgr.c | 163 +++++++++++++++++++++++++++++++- >> include/linux/gunyah.h | 4 + >> include/linux/gunyah_rsc_mgr.h | 3 + >> 4 files changed, 192 insertions(+), 1 deletion(-) >> create mode 100644 arch/arm64/include/asm/gunyah.h >> >> diff --git a/arch/arm64/include/asm/gunyah.h >> b/arch/arm64/include/asm/gunyah.h >> new file mode 100644 >> index 000000000000..64cfb964efee >> --- /dev/null >> +++ b/arch/arm64/include/asm/gunyah.h >> @@ -0,0 +1,23 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/* >> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights >> reserved. >> + */ >> +#ifndef __ASM_GUNYAH_H_ >> +#define __ASM_GUNYAH_H_ > > Maybe just one _ at the beginning and none at the end? > Follow the same convention across all your header files. > (Maybe you're looking at other files in the same directory > as this one, but that's not consistent.) > >> + >> +#include <linux/irq.h> >> +#include <dt-bindings/interrupt-controller/arm-gic.h> >> + >> +static inline int arch_gh_fill_irq_fwspec_params(u32 virq, struct >> irq_fwspec *fwspec) >> +{ >> + if (virq < 32 || virq > 1019) >> + return -EINVAL; > > What is special about VIRQs greater than 1019 (minus 32)? > > It's probably documented somewhere but it's worth adding a > comment here to explain the check. > > You would know better than I, but could/should the caller > be responsible for this check? (Not a big deal.) >
I think definitely not the caller should be responsible for this check.
On arm systems, the IRQ # that is returned is the hwirq # for the GIC.
Presently, Gunyah only gives us SPI interrupts [32,1019] so I've written a translation to a SPI fwspec.
>> + >> + fwspec->param_count = 3; >> + fwspec->param[0] = GIC_SPI; >> + fwspec->param[1] = virq - 32; > > And why is 32 subtracted? >
GIC driver expects the SPI #, not the hwirq #.
>> + fwspec->param[2] = IRQ_TYPE_EDGE_RISING; >> + return 0; >> +} >> + >> +#endif >> diff --git a/drivers/virt/gunyah/rsc_mgr.c >> b/drivers/virt/gunyah/rsc_mgr.c >> index d7ce692d0067..383be5ac0f44 100644 >> --- a/drivers/virt/gunyah/rsc_mgr.c >> +++ b/drivers/virt/gunyah/rsc_mgr.c >> @@ -17,6 +17,8 @@ >> #include <linux/platform_device.h> >> #include <linux/miscdevice.h> >> +#include <asm/gunyah.h> >> + >> #include "rsc_mgr.h" >> #include "vm_mgr.h" >> @@ -132,6 +134,7 @@ struct gh_rm_connection { >> * @send_lock: synchronization to allow only one request to be sent >> at a time >> * @nh: notifier chain for clients interested in RM notification >> messages >> * @miscdev: /dev/gunyah >> + * @irq_domain: Domain to translate Gunyah hwirqs to Linux irqs >> */ >> struct gh_rm { >> struct device *dev; >> @@ -150,6 +153,7 @@ struct gh_rm { >> struct blocking_notifier_head nh; >> struct miscdevice miscdev; >> + struct irq_domain *irq_domain; >> }; >> /** >> @@ -190,6 +194,134 @@ static inline int gh_rm_remap_error(enum >> gh_rm_error rm_error) >> } >> } >> +struct gh_irq_chip_data { >> + u32 gh_virq; >> +}; >> + >> +static struct irq_chip gh_rm_irq_chip = { >> + .name = "Gunyah", >> + .irq_enable = irq_chip_enable_parent, >> + .irq_disable = irq_chip_disable_parent, >> + .irq_ack = irq_chip_ack_parent, >> + .irq_mask = irq_chip_mask_parent, >> + .irq_mask_ack = irq_chip_mask_ack_parent, >> + .irq_unmask = irq_chip_unmask_parent, >> + .irq_eoi = irq_chip_eoi_parent, >> + .irq_set_affinity = irq_chip_set_affinity_parent, >> + .irq_set_type = irq_chip_set_type_parent, >> + .irq_set_wake = irq_chip_set_wake_parent, >> + .irq_set_vcpu_affinity = irq_chip_set_vcpu_affinity_parent, >> + .irq_retrigger = irq_chip_retrigger_hierarchy, >> + .irq_get_irqchip_state = irq_chip_get_parent_state, >> + .irq_set_irqchip_state = irq_chip_set_parent_state, >> + .flags = IRQCHIP_SET_TYPE_MASKED | >> + IRQCHIP_SKIP_SET_WAKE | >> + IRQCHIP_MASK_ON_SUSPEND, >> +}; >> + >> +static int gh_rm_irq_domain_alloc(struct irq_domain *d, unsigned int >> virq, unsigned int nr_irqs, >> + void *arg) >> +{ >> + struct gh_irq_chip_data *chip_data, *spec = arg; >> + struct irq_fwspec parent_fwspec; >> + struct gh_rm *rm = d->host_data; >> + u32 gh_virq = spec->gh_virq; >> + int ret; >> + >> + if (nr_irqs != 1 || gh_virq == U32_MAX) > > Does U32_MAX have special meaning? Why are you checking for it? > Whatever it is, you should explain why this is invalid here. >
This was holdover from deprecated Gunyah code. Since there are new features/version checks it's not possible for Linux to encounter these values. I've dropped it in v12.
Thanks, Elliot
| |