|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH 27/49] ARM: new VGIC: Add MMIO handling framework
Hi,
On 13/02/18 16:52, Julien Grall wrote:
> 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@xxxxxxxxxx>
>> ---
>> 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?
Good catch, this is needed in KVM to do the user space access, where we
re-use these functions to call into the MMIO handlers.
So I can make them static and then loose the prototype down below as well.
>
>> + 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.
True, I was unsure about whether to keep them. I have the gut feeling we
need it later when we want to suspend/resume the VGIC, so removing
everything and then simplifying the code afterwards might bite us in the
future.
So as long as it doesn't really hurt, I am tempted to keep that code in,
which also keeps it closer the the KVM implementation.
But if you insist: deleting this is easy ;-)
>> +};
>> +
>> +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").
\o/
>
>> +
>> +/*
>> + * 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.
Thanks for the heads up. In general I didn't spend much time on the
prototypes in header files, so there might indeed by some leftovers from
KVM.
Consider the rest fixed.
Cheers,
Andre.
>> +
>> +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,
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |