Re: [RFC PATCH 27/49] ARM: new VGIC: Add MMIO handling framework

[Julien Grall <julien.grall@arm.com>]

Hi Andre,7

On 09/02/18 14:39, Andre Przywara wrote:
> Add an MMIO handling framework to the VGIC emulation:
> Each register is described by its offset, size (or number of bits per
> IRQ, if applicable) and the read/write handler functions. We provide
> initialization macros to describe each GIC register later easily.
> 
> Separate dispatch functions for read and write accesses are connected
> to Xen's MMIO handling framework and binary-search for the responsible
> register handler based on the offset address within the region.
> 
> The register handler prototype are courtesy of Christoffer Dall.
> 
> This is based on Linux commit 4493b1c4866a, written by Marc Zyngier.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
>   xen/arch/arm/vgic/vgic-mmio.c | 192 ++++++++++++++++++++++++++++++++++++++++++
>   xen/arch/arm/vgic/vgic-mmio.h | 145 +++++++++++++++++++++++++++++++
>   xen/arch/arm/vgic/vgic.h      |   4 +
>   3 files changed, 341 insertions(+)
>   create mode 100644 xen/arch/arm/vgic/vgic-mmio.c
>   create mode 100644 xen/arch/arm/vgic/vgic-mmio.h
> 
> diff --git a/xen/arch/arm/vgic/vgic-mmio.c b/xen/arch/arm/vgic/vgic-mmio.c
> new file mode 100644
> index 0000000000..3c70945466
> --- /dev/null
> +++ b/xen/arch/arm/vgic/vgic-mmio.c
> @@ -0,0 +1,192 @@
> +/*
> + * VGIC MMIO handling functions
> + * Imported from Linux ("new" KVM VGIC) and heavily adapted to Xen.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <xen/bitops.h>
> +#include <xen/lib.h>
> +#include <xen/sched.h>
> +#include <asm/arm_vgic.h>
> +#include <asm/byteorder.h>
> +
> +#include "vgic.h"
> +#include "vgic-mmio.h"
> +
> +unsigned long vgic_mmio_read_raz(struct vcpu *vcpu,
> +                 paddr_t addr, unsigned int len)

Indentation.

> +{
> +    return 0;
> +}
> +
> +unsigned long vgic_mmio_read_rao(struct vcpu *vcpu,
> +                 paddr_t addr, unsigned int len)

Indentation.

> +{
> +    return -1UL;
> +}
> +
> +void vgic_mmio_write_wi(struct vcpu *vcpu, paddr_t addr,
> +            unsigned int len, unsigned long val)

Indentation.

> +{
> +    /* Ignore */
> +}
> +
> +static int match_region(const void *key, const void *elt)
> +{
> +    const unsigned int offset = (unsigned long)key;
> +    const struct vgic_register_region *region = elt;
> +
> +    if ( offset < region->reg_offset )
> +        return -1;
> +
> +    if ( offset >= region->reg_offset + region->len )
> +        return 1;
> +
> +    return 0;
> +}
> +
> +const struct vgic_register_region *
> +vgic_find_mmio_region(const struct vgic_register_region *regions,

Any reason to export this?

> +              int nr_regions, unsigned int offset)

Indentation.

> +{
> +    return bsearch((void *)(uintptr_t)offset, regions, nr_regions,
> +               sizeof(regions[0]), match_region);
> +}
> +
> +static bool check_region(const struct domain *d,
> +             const struct vgic_register_region *region,
> +             paddr_t addr, int len)

Indentation.

> +{
> +    int flags, nr_irqs = d->arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS;
> + > +    switch (len)

switch ( ... )

> +    {
> +    case sizeof(u8):

s/u8/uint8_t/ here an below.

> +        flags = VGIC_ACCESS_8bit;
> +        break;
> +    case sizeof(u32):
> +        flags = VGIC_ACCESS_32bit;
> +        break;
> +    case sizeof(u64):
> +        flags = VGIC_ACCESS_64bit;
> +        break;
> +    default:
> +        return false;
> +    }
> +
> +    if ( (region->access_flags & flags) && IS_ALIGNED(addr, len) )
> +    {
> +        if ( !region->bits_per_irq )
> +            return true;
> +
> +        /* Do we access a non-allocated IRQ? */
> +        return VGIC_ADDR_TO_INTID(addr, region->bits_per_irq) < nr_irqs;
> +    }
> +
> +    return false;
> +}
> +
> +const struct vgic_register_region *
> +vgic_get_mmio_region(struct vcpu *vcpu, struct vgic_io_device *iodev,


Any reason to export this?

> +             paddr_t addr, int len)

Indentation and unsigned int please.

> +{
> +    const struct vgic_register_region *region;
> +
> +    region = vgic_find_mmio_region(iodev->regions, iodev->nr_regions,
> +                       addr - iodev->base_addr);
> +    if ( !region || !check_region(vcpu->domain, region, addr, len) )
> +        return NULL;
> +
> +    return region;
> +}
> +
> +static int dispatch_mmio_read(struct vcpu *vcpu, mmio_info_t *info,
> +                  register_t *r, void *priv)

Indentation.

> +{
> +    struct vgic_io_device *iodev = priv;
> +    const struct vgic_register_region *region;
> +    unsigned long data = 0;
> +    paddr_t addr = info->gpa;
> +    int len = 1U << info->dabt.size;
> +
> +    region = vgic_get_mmio_region(vcpu, iodev, addr, len);
> +    if ( !region )
> +    {
> +        memset(r, 0, len);
> +        return 0;
> +    }
> +
> +    switch (iodev->iodev_type)
> +    {
> +    case IODEV_CPUIF:
> +        data = region->read(vcpu, addr, len);
> +        break;
> +    case IODEV_DIST:
> +        data = region->read(vcpu, addr, len);
> +        break;
> +    case IODEV_REDIST:
> +        data = region->read(iodev->redist_vcpu, addr, len);
> +        break;
> +    case IODEV_ITS:
> +        data = region->its_read(vcpu->domain, iodev->its, addr, len);
> +        break;
> +    }
> +
> +    memcpy(r, &data, len);
> +
> +    return 1;
> +}
> +
> +static int dispatch_mmio_write(struct vcpu *vcpu, mmio_info_t *info,
> +                   register_t r, void *priv)
> +{
> +    struct vgic_io_device *iodev = priv;
> +    const struct vgic_register_region *region;
> +    unsigned long data = r;
> +    paddr_t addr = info->gpa;
> +    int len = 1U << info->dabt.size;
> +
> +    region = vgic_get_mmio_region(vcpu, iodev, addr, len);
> +    if ( !region )
> +        return 0;
> +
> +    switch (iodev->iodev_type)
> +    {
> +    case IODEV_CPUIF:
> +        region->write(vcpu, addr, len, data);
> +        break;
> +    case IODEV_DIST:
> +        region->write(vcpu, addr, len, data);
> +        break;
> +    case IODEV_REDIST:
> +        region->write(iodev->redist_vcpu, addr, len, data);
> +        break;
> +    case IODEV_ITS:
> +        region->its_write(vcpu->domain, iodev->its, addr, len, data);
> +        break;
> +    }
> +
> +    return 1;
> +}
> +
> +struct mmio_handler_ops xen_io_gic_ops = {

I would rename to vgic_io_ops.

> +    .read = dispatch_mmio_read,
> +    .write = dispatch_mmio_write,
> +};
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/vgic/vgic-mmio.h b/xen/arch/arm/vgic/vgic-mmio.h
> new file mode 100644
> index 0000000000..375b70561d
> --- /dev/null
> +++ b/xen/arch/arm/vgic/vgic-mmio.h
> @@ -0,0 +1,145 @@
> +/*
> + * Copyright (C) 2015, 2016 ARM Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +#ifndef __KVM_ARM_VGIC_MMIO_H__
> +#define __KVM_ARM_VGIC_MMIO_H__

Please use update the guard.

> +
> +struct vgic_register_region {
> +    unsigned int reg_offset;
> +    unsigned int len;
> +    unsigned int bits_per_irq;
> +    unsigned int access_flags;
> +    union
> +    {
> +        unsigned long (*read)(struct vcpu *vcpu, paddr_t addr,
> +                              unsigned int len);
> +        unsigned long (*its_read)(struct domain *d, struct vgic_its *its,
> +                                  paddr_t addr, unsigned int len);
> +    };
> +    union
> +    {
> +        void (*write)(struct vcpu *vcpu, paddr_t addr,
> +                      unsigned int len, unsigned long val);
> +        void (*its_write)(struct domain *d, struct vgic_its *its,
> +                          paddr_t addr, unsigned int len,
> +                          unsigned long val);
> +    };
> +    unsigned long (*uaccess_read)(struct vcpu *vcpu, paddr_t addr,
> +                                  unsigned int len);
> +    union
> +    {
> +        void (*uaccess_write)(struct vcpu *vcpu, paddr_t addr,
> +                              unsigned int len, unsigned long val);
> +        int (*uaccess_its_write)(struct domain *d, struct vgic_its *its,
> +                                 paddr_t addr, unsigned int len,
> +                                 unsigned long val);
> +    };

I don't think uaccess helpers makes sense for Xen.

> +};
> +
> +extern struct mmio_handler_ops xen_io_gic_ops;
> +
> +#define VGIC_ACCESS_8bit    1
> +#define VGIC_ACCESS_32bit   2
> +#define VGIC_ACCESS_64bit   4
> +
> +/*
> + * Generate a mask that covers the number of bytes required to address
> + * up to 1024 interrupts, each represented by <bits> bits. This assumes
> + * that <bits> is a power of two.
> + */
> +#define VGIC_ADDR_IRQ_MASK(bits) (((bits) * 1024 / 8) - 1)
> +
> +/*
> + * (addr & mask) gives us the _byte_ offset for the INT ID.
> + * We multiply this by 8 the get the _bit_ offset, then divide this by
> + * the number of bits to learn the actual INT ID.
> + * But instead of a division (which requires a "long long div" implementation),
> + * we shift by the binary logarithm of <bits>.
> + * This assumes that <bits> is a power of two.
> + */
> +#define VGIC_ADDR_TO_INTID(addr, bits)  (((addr) & VGIC_ADDR_IRQ_MASK(bits)) * \
> +                                         8 >> LOG_2(bits))

We are going to switch to ilog2 (see Sameer's patch "xen/bitops: Rename 
LOG_2 to ilog2").

> +
> +/*
> + * Some VGIC registers store per-IRQ information, with a different number
> + * of bits per IRQ. For those registers this macro is used.
> + * The _WITH_LENGTH version instantiates registers with a fixed length
> + * and is mutually exclusive with the _PER_IRQ version.
> + */
> +#define REGISTER_DESC_WITH_BITS_PER_IRQ(off, rd, wr, ur, uw, bpi, acc)  \
> +    {                           \
> +        .reg_offset = off,      \
> +        .bits_per_irq = bpi,    \
> +        .len = bpi * 1024 / 8,  \
> +        .access_flags = acc,    \
> +        .read = rd,             \
> +        .write = wr,            \
> +        .uaccess_read = ur,     \
> +        .uaccess_write = uw,    \
> +    }
> +
> +#define REGISTER_DESC_WITH_LENGTH(off, rd, wr, length, acc)     \
> +    {                           \
> +        .reg_offset = off,      \
> +        .bits_per_irq = 0,      \
> +        .len = length,          \
> +        .access_flags = acc,    \
> +        .read = rd,             \
> +        .write = wr,            \
> +    }
> +
> +#define REGISTER_DESC_WITH_LENGTH_UACCESS(off, rd, wr, urd, uwr, length, acc) \
> +    {                           \
> +        .reg_offset = off,      \
> +        .bits_per_irq = 0,      \
> +        .len = length,          \
> +        .access_flags = acc,    \
> +        .read = rd,             \
> +        .write = wr,            \
> +        .uaccess_read = urd,    \
> +        .uaccess_write = uwr,   \
> +    }
> +
> +int kvm_vgic_register_mmio_region(struct domain *d, struct vcpu *vcpu,
> +                                  struct vgic_register_region *reg_desc,
> +                                  struct vgic_io_device *region,
> +                                  int nr_irqs, bool offset_private);

You want to do some clean-up in the prototype below. Only the one used 
in the patch should be added. The other should either move in there 
corresponding patch or dropped if not used.

> +
> +unsigned long vgic_data_mmio_bus_to_host(const void *val, unsigned int len);
> +
> +void vgic_data_host_to_mmio_bus(void *buf, unsigned int len,
> +                                unsigned long data);
> +
> +unsigned long extract_bytes(u64 data, unsigned int offset,
> +                            unsigned int num);
> +
> +u64 update_64bit_reg(u64 reg, unsigned int offset, unsigned int len,
> +                     unsigned long val);
> +
> +unsigned long vgic_mmio_read_raz(struct vcpu *vcpu,
> +                                 paddr_t addr, unsigned int len);
> +
> +unsigned long vgic_mmio_read_rao(struct vcpu *vcpu,
> +                                 paddr_t addr, unsigned int len);
> +
> +void vgic_mmio_write_wi(struct vcpu *vcpu, paddr_t addr,
> +                        unsigned int len, unsigned long val);
> +
> +/* Find the proper register handler entry given a certain address offset */
> +const struct vgic_register_region *
> +vgic_find_mmio_region(const struct vgic_register_region *regions,
> +                      int nr_regions, unsigned int offset);
> +
> +#endif
> diff --git a/xen/arch/arm/vgic/vgic.h b/xen/arch/arm/vgic/vgic.h
> index 771ca6f046..426b34d0ce 100644
> --- a/xen/arch/arm/vgic/vgic.h
> +++ b/xen/arch/arm/vgic/vgic.h
> @@ -27,6 +27,10 @@ static inline bool irq_is_pending(struct vgic_irq *irq)
>           return irq->pending_latch || irq->line_level;
>   }
>   
> +const struct vgic_register_region *
> +vgic_get_mmio_region(struct vcpu *vcpu, struct vgic_io_device *iodev,
> +             paddr_t addr, int len);
> +

Why this one is added in vgic.h and not kept in vgic-mmio.h?

>   struct vgic_irq *vgic_get_irq(struct domain *d, struct vcpu *vcpu,
>                                 u32 intid);
>   void vgic_put_irq(struct domain *d, struct vgic_irq *irq);
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel