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

[Julien Grall]

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,

--
Julien Grall

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

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

[Andre Przywara]

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 
>> ---
>>   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 00..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 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#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 || 

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

[Julien Grall]

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 
---
  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 00..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 
+#include 
+#include 
+#include 
+#include 
+
+#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;
+