| Message ID | 20180209143937.28866-28-andre.przywara@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | New VGIC(-v2) implementation | expand |
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,
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@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? 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, >
Hi Andre, On 13/02/18 18:17, Andre Przywara wrote: > On 13/02/18 16:52, Julien Grall wrote: >>> +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. I don't want to see code that is going to potentially rot. If we really need it, we can add it afterwards. Cheers,
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) +{ + return 0; +} + +unsigned long vgic_mmio_read_rao(struct vcpu *vcpu, + paddr_t addr, unsigned int len) +{ + return -1UL; +} + +void vgic_mmio_write_wi(struct vcpu *vcpu, paddr_t addr, + unsigned int len, unsigned long val) +{ + /* 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, + int nr_regions, unsigned int offset) +{ + 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) +{ + int flags, nr_irqs = d->arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS; + + switch (len) + { + case sizeof(u8): + 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, + paddr_t addr, int len) +{ + 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) +{ + 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 = { + .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__ + +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); + }; +}; + +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)) + +/* + * 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); + +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); + 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);
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