Re: [PATCH v11 18/26] virt: gunyah: Translate gh_rm_hyp_resource into gunyah_resource

[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@xxxxxxxxxxx>
>
> 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