| 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: <de523682-a8b3-5434-bc1f-a06645f10183@quicinc.com>
Date: Mon, 17 Apr 2023 17:25:57 -0700
From: Elliot Berman <quic_eberman@...cinc.com>
To: Alex Elder <elder@...aro.org>,
Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>,
Prakruthi Deepak Heragu <quic_pheragu@...cinc.com>
CC: Murali Nalajala <quic_mnalajal@...cinc.com>,
Trilok Soni <quic_tsoni@...cinc.com>,
Srivatsa Vaddagiri <quic_svaddagi@...cinc.com>,
Carl van Schaik <quic_cvanscha@...cinc.com>,
Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
Bjorn Andersson <andersson@...nel.org>,
"Konrad Dybcio" <konrad.dybcio@...aro.org>,
Arnd Bergmann <arnd@...db.de>,
"Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Jonathan Corbet <corbet@....net>,
Bagas Sanjaya <bagasdotme@...il.com>,
Andy Gross <agross@...nel.org>,
Jassi Brar <jassisinghbrar@...il.com>,
<linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-doc@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v11 18/26] virt: gunyah: Translate gh_rm_hyp_resource into
gunyah_resource
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@...cinc.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
Powered by blists - more mailing lists