|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH 11/24] ARM: vGICv3: handle virtual LPI pending and property tables
Hi,
On 24/10/16 16:32, Vijay Kilari wrote:
> On Wed, Sep 28, 2016 at 11:54 PM, Andre Przywara <andre.przywara@xxxxxxx>
> wrote:
>> Allow a guest to provide the address and size for the memory regions
>> it has reserved for the GICv3 pending and property tables.
>> We sanitise the various fields of the respective redistributor
>> registers and map those pages into Xen's address space to have easy
>> access.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>> ---
>> xen/arch/arm/vgic-v3.c | 189
>> ++++++++++++++++++++++++++++++++++++++----
>> xen/arch/arm/vgic.c | 4 +
>> xen/include/asm-arm/domain.h | 7 +-
>> xen/include/asm-arm/gic-its.h | 10 ++-
>> xen/include/asm-arm/vgic.h | 3 +
>> 5 files changed, 197 insertions(+), 16 deletions(-)
>>
>> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
>> index e9b6490..8fe8386 100644
>> --- a/xen/arch/arm/vgic-v3.c
>> +++ b/xen/arch/arm/vgic-v3.c
>> @@ -20,12 +20,14 @@
>>
>> #include <xen/bitops.h>
>> #include <xen/config.h>
>> +#include <xen/domain_page.h>
>> #include <xen/lib.h>
>> #include <xen/init.h>
>> #include <xen/softirq.h>
>> #include <xen/irq.h>
>> #include <xen/sched.h>
>> #include <xen/sizes.h>
>> +#include <xen/vmap.h>
>> #include <asm/current.h>
>> #include <asm/mmio.h>
>> #include <asm/gic_v3_defs.h>
>> @@ -228,12 +230,14 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu
>> *v, mmio_info_t *info,
>> goto read_reserved;
>>
>> case VREG64(GICR_PROPBASER):
>> - /* LPI's not implemented */
>> - goto read_as_zero_64;
>> + if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
>> + *r = vgic_reg64_extract(v->domain->arch.vgic.rdist_propbase, info);
>> + return 1;
>>
>> case VREG64(GICR_PENDBASER):
>> - /* LPI's not implemented */
>> - goto read_as_zero_64;
>> + if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
>> + *r = vgic_reg64_extract(v->arch.vgic.rdist_pendbase, info);
>> + return 1;
>>
>> case 0x0080:
>> goto read_reserved;
>> @@ -301,11 +305,6 @@ bad_width:
>> domain_crash_synchronous();
>> return 0;
>>
>> -read_as_zero_64:
>> - if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
>> - *r = 0;
>> - return 1;
>> -
>> read_as_zero_32:
>> if ( dabt.size != DABT_WORD ) goto bad_width;
>> *r = 0;
>> @@ -330,11 +329,149 @@ read_unknown:
>> return 1;
>> }
>>
>> +static uint64_t vgic_sanitise_field(uint64_t reg, uint64_t field_mask,
>> + int field_shift,
>> + uint64_t (*sanitise_fn)(uint64_t))
>> +{
>> + uint64_t field = (reg & field_mask) >> field_shift;
>> +
>> + field = sanitise_fn(field) << field_shift;
>> + return (reg & ~field_mask) | field;
>> +}
>> +
>> +/* We want to avoid outer shareable. */
>> +static uint64_t vgic_sanitise_shareability(uint64_t field)
>> +{
>> + switch (field) {
>> + case GIC_BASER_OuterShareable:
>> + return GIC_BASER_InnerShareable;
>> + default:
>> + return field;
>> + }
>> +}
>> +
>> +/* Avoid any inner non-cacheable mapping. */
>> +static uint64_t vgic_sanitise_inner_cacheability(uint64_t field)
>> +{
>> + switch (field) {
>> + case GIC_BASER_CACHE_nCnB:
>> + case GIC_BASER_CACHE_nC:
>> + return GIC_BASER_CACHE_RaWb;
>> + default:
>> + return field;
>> + }
>> +}
>> +
>> +/* Non-cacheable or same-as-inner are OK. */
>> +static uint64_t vgic_sanitise_outer_cacheability(uint64_t field)
>> +{
>> + switch (field) {
>> + case GIC_BASER_CACHE_SameAsInner:
>> + case GIC_BASER_CACHE_nC:
>> + return field;
>> + default:
>> + return GIC_BASER_CACHE_nC;
>> + }
>> +}
>> +
>> +static uint64_t sanitize_propbaser(uint64_t reg)
>> +{
>> + reg = vgic_sanitise_field(reg, GICR_PROPBASER_SHAREABILITY_MASK,
>> + GICR_PROPBASER_SHAREABILITY_SHIFT,
>> + vgic_sanitise_shareability);
>> + reg = vgic_sanitise_field(reg, GICR_PROPBASER_INNER_CACHEABILITY_MASK,
>> + GICR_PROPBASER_INNER_CACHEABILITY_SHIFT,
>> + vgic_sanitise_inner_cacheability);
>> + reg = vgic_sanitise_field(reg, GICR_PROPBASER_OUTER_CACHEABILITY_MASK,
>> + GICR_PROPBASER_OUTER_CACHEABILITY_SHIFT,
>> + vgic_sanitise_outer_cacheability);
>> +
>> + reg &= ~PROPBASER_RES0_MASK;
>> + reg &= ~GENMASK(51, 48);
>> + return reg;
>> +}
>> +
>> +static uint64_t sanitize_pendbaser(uint64_t reg)
>> +{
>> + reg = vgic_sanitise_field(reg, GICR_PENDBASER_SHAREABILITY_MASK,
>> + GICR_PENDBASER_SHAREABILITY_SHIFT,
>> + vgic_sanitise_shareability);
>> + reg = vgic_sanitise_field(reg, GICR_PENDBASER_INNER_CACHEABILITY_MASK,
>> + GICR_PENDBASER_INNER_CACHEABILITY_SHIFT,
>> + vgic_sanitise_inner_cacheability);
>> + reg = vgic_sanitise_field(reg, GICR_PENDBASER_OUTER_CACHEABILITY_MASK,
>> + GICR_PENDBASER_OUTER_CACHEABILITY_SHIFT,
>> + vgic_sanitise_outer_cacheability);
>> +
>> + reg &= ~PENDBASER_RES0_MASK;
>> + reg &= ~GENMASK(51, 48);
>> + return reg;
>> +}
>> +
>> +/*
>> + * Allow mapping some parts of guest memory into Xen's VA space to have easy
>> + * access to it. This is to allow ITS configuration data to be held in
>> + * guest memory and avoid using Xen memory for that.
>> + */
>> +void *map_guest_pages(struct domain *d, paddr_t guest_addr, int nr_pages)
> I think this file is not right place to put this generic function
Yeah, possibly.
>> +{
>> + mfn_t onepage;
>> + mfn_t *pages;
>> + int i;
>> + void *ptr;
>> +
>> + /* TODO: free previous mapping, change prototype? use get-put-put? */
>> +
>> + guest_addr &= PAGE_MASK;
>> +
>> + if ( nr_pages == 1 )
>> + {
>> + pages = &onepage;
>> + } else
>> + {
>> + pages = xmalloc_array(mfn_t, nr_pages);
>> + if ( !pages )
>> + return NULL;
>> + }
>> +
>> + for (i = 0; i < nr_pages; i++)
>> + {
>> + get_page_from_gfn(d, (guest_addr >> PAGE_SHIFT) + i, NULL,
>> P2M_ALLOC);
>
> check return value of this function
Yes.
>> + pages[i] = _mfn((guest_addr + i * PAGE_SIZE) >> PAGE_SHIFT);
>> + }
>> +
>> + ptr = vmap(pages, nr_pages);
>> +
>> + if ( nr_pages > 1 )
>> + xfree(pages);
>> +
>> + return ptr;
>> +}
>> +
>> +void unmap_guest_pages(void *va, int nr_pages)
> same here. Can be put in generic file p2m.c?
>> +{
>> + paddr_t pa;
>> + unsigned long i;
>> +
>> + if ( !va )
>> + return;
>> +
>> + va = (void *)((uintptr_t)va & PAGE_MASK);
>> + pa = virt_to_maddr(va);
> can use _pa()
Do you mean __pa()? Which is defined to be exactly virt_to_maddr()?
I prefer the more verbose version, which is more readable, IMHO.
>> +
>> + vunmap(va);
>> + for (i = 0; i < nr_pages; i++)
>> + put_page(mfn_to_page((pa >> PAGE_SHIFT) + i));
>> +
>> + return;
>> +}
>> +
>> static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info,
>> uint32_t gicr_reg,
>> register_t r)
>> {
>> struct hsr_dabt dabt = info->dabt;
>> + uint64_t reg;
>>
>> switch ( gicr_reg )
>> {
>> @@ -375,13 +512,37 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu
>> *v, mmio_info_t *info,
>> case 0x0050:
>> goto write_reserved;
>>
>> - case VREG64(GICR_PROPBASER):
>> - /* LPI is not implemented */
>> - goto write_ignore_64;
>> + case VREG64(GICR_PROPBASER): {
>> + int nr_pages;
>> +
>> + if ( info->dabt.size < DABT_WORD ) goto bad_width;
>> + if ( v->arch.vgic.flags & VGIC_V3_LPIS_ENABLED )
>> + return 1;
>> +
>> + reg = v->domain->arch.vgic.rdist_propbase;
>> + vgic_reg64_update(®, r, info);
>> + reg = sanitize_propbaser(reg);
>> + v->domain->arch.vgic.rdist_propbase = reg;
>>
>> + nr_pages = BIT((v->domain->arch.vgic.rdist_propbase & 0x1f) + 1) -
>> 8192;
> should be validated against HOST_LPIS?
I don't think so. The actual LPI numbers are totally independent between
host and Dom0.
So why and how should this be matched?
>> + nr_pages = DIV_ROUND_UP(nr_pages, PAGE_SIZE);
>> + unmap_guest_pages(v->domain->arch.vgic.proptable, nr_pages);
>> + v->domain->arch.vgic.proptable = map_guest_pages(v->domain,
>> + reg & GENMASK(47,
>> 12),
>> + nr_pages);
>> + return 1;
>> + }
>> case VREG64(GICR_PENDBASER):
>> - /* LPI is not implemented */
>> - goto write_ignore_64;
>> + if ( info->dabt.size < DABT_WORD ) goto bad_width;
>
> check on VGIC_V3_LPIS_ENABLED is required
Right, forgot this.
>> + reg = v->arch.vgic.rdist_pendbase;
>> + vgic_reg64_update(®, r, info);
>> + reg = sanitize_pendbaser(reg);
>> + v->arch.vgic.rdist_pendbase = reg;
>> +
>> + unmap_guest_pages(v->arch.vgic.pendtable, 16);
> why only 16 pages are unmapped?
Well, it matches the allocation below, but I agree that this should
match the advertised number of LPIs in GICD_TYPER.
Cheers,
Andre.
>> + v->arch.vgic.pendtable = map_guest_pages(v->domain,
>> + reg & GENMASK(47, 12), 16);
>> + return 1;
>>
>> case 0x0080:
>> goto write_reserved;
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index b961551..4d9304f 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -488,6 +488,10 @@ struct pending_irq *lpi_to_pending(struct vcpu *v,
>> unsigned int lpi,
>> empty->pirq.irq = lpi;
>> }
>>
>> + /* Update the enabled status */
>> + if ( gicv3_lpi_is_enabled(v->domain, lpi) )
>> + set_bit(GIC_IRQ_GUEST_ENABLED, &empty->pirq.status);
>> +
>> return &empty->pirq;
>> }
>>
>> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
>> index ae8a9de..0cd3500 100644
>> --- a/xen/include/asm-arm/domain.h
>> +++ b/xen/include/asm-arm/domain.h
>> @@ -109,6 +109,8 @@ struct arch_domain
>> } *rdist_regions;
>> int nr_regions; /* Number of rdist regions */
>> uint32_t rdist_stride; /* Re-Distributor stride */
>> + uint64_t rdist_propbase;
>> + uint8_t *proptable;
>> #endif
>> } vgic;
>>
>> @@ -247,7 +249,10 @@ struct arch_vcpu
>>
>> /* GICv3: redistributor base and flags for this vCPU */
>> paddr_t rdist_base;
>> -#define VGIC_V3_RDIST_LAST (1 << 0) /* last vCPU of the rdist */
>> +#define VGIC_V3_RDIST_LAST (1 << 0) /* last vCPU of the rdist */
>> +#define VGIC_V3_LPIS_ENABLED (1 << 1)
>> + uint64_t rdist_pendbase;
>> + unsigned long *pendtable;
>> uint8_t flags;
>> struct list_head pending_lpi_list;
>> } vgic;
>> diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h
>> index 1f881c0..3b2e5c0 100644
>> --- a/xen/include/asm-arm/gic-its.h
>> +++ b/xen/include/asm-arm/gic-its.h
>> @@ -139,7 +139,11 @@ int gicv3_lpi_drop_host_lpi(struct host_its *its,
>>
>> static inline int gicv3_lpi_get_priority(struct domain *d, uint32_t lpi)
>> {
>> - return GIC_PRI_IRQ;
>> + return d->arch.vgic.proptable[lpi - 8192] & 0xfc;
>> +}
>> +static inline bool gicv3_lpi_is_enabled(struct domain *d, uint32_t lpi)
>> +{
>> + return d->arch.vgic.proptable[lpi - 8192] & LPI_PROP_ENABLED;
>> }
>>
>> #else
>> @@ -185,6 +189,10 @@ static inline int gicv3_lpi_get_priority(struct domain
>> *d, uint32_t lpi)
>> {
>> return GIC_PRI_IRQ;
>> }
>> +static inline bool gicv3_lpi_is_enabled(struct domain *d, uint32_t lpi)
>> +{
>> + return false;
>> +}
>>
>> #endif /* CONFIG_HAS_ITS */
>>
>> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
>> index 4e29ba6..2b216cc 100644
>> --- a/xen/include/asm-arm/vgic.h
>> +++ b/xen/include/asm-arm/vgic.h
>> @@ -285,6 +285,9 @@ VGIC_REG_HELPERS(32, 0x3);
>>
>> #undef VGIC_REG_HELPERS
>>
>> +void *map_guest_pages(struct domain *d, paddr_t guest_addr, int nr_pages);
>> +void unmap_guest_pages(void *va, int nr_pages);
>> +
>> enum gic_sgi_mode;
>>
>> /*
>> --
>> 2.9.0
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@xxxxxxxxxxxxx
>> https://lists.xen.org/xen-devel
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |