Re: [Xen-devel] [RFC PATCH 11/24] ARM: vGICv3: handle virtual LPI pending and property tables

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi,

On 29/10/16 01:39, Stefano Stabellini wrote:
> On Wed, 28 Sep 2016, Andre Przywara 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>
>> ---
>> 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
....

>> +        reg = v->domain->arch.vgic.rdist_propbase;
>> +        vgic_reg64_update(&reg, 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;
>> +        nr_pages = DIV_ROUND_UP(nr_pages, PAGE_SIZE);
> 
> Do we need to set an upper limit on nr_pages? We don't really want to
> allow (2^0x1f)/4096 pages, right?

Why not? This is the virtual property table, and the *guest* provides
the memory. We just comply here and map it. I don't see any issue.

[ .... ]

>> 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);
> 
> Where is the GIC_IRQ_GUEST_ENABLED unset?

In the patch where the INV command is emulated. This is how
enabling/disabling LPI works: Software (the guest here) sets the bit in
the property table and issues an ITS command to notify the ITS
(emulation) about it.

>>      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;
> 
> Do we need to keep both rdist_propbase and proptable? It is easy to go
> from proptable to rdist_propbase and I guess it is not an operation that
> is done often? If so, we could save some memory and remove it.

The code has changed meanwhile, so this does not apply direclty anymore,
but just to make sure:
We need rdist_propbase separately, because a guest can happily set and
change it as often as it wants before enabling LPIs. We shouldn't (and
we don't) allocate memory now (and so set proptable) until the LPIs get
enabled.

>>  #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;
> 
> Same here.

And the same rationale applies here.

Fixed / addresses the rest.

Cheers,
Andre.

>>          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;
> 
> Please #define 0xfc. Do we need to check for lpi overflows? As in lpi
> numbers larger than proptable size?
> 
> 
>> +}
>> +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;
>>  
>>  /*

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel