[PATCH v2 12/15] KVM: arm64: sync LPI configuration and pending tables

[eric.auger@linaro.org (Eric Auger)]

On 08/25/2015 05:47 PM, Andre Przywara wrote:
> Hi Eric,
> 
> On 14/08/15 13:35, Eric Auger wrote:
>> On 08/14/2015 01:58 PM, Eric Auger wrote:
>>> On 07/10/2015 04:21 PM, Andre Przywara wrote:
>>>> The LPI configuration and pending tables of the GICv3 LPIs are held
>>>> in tables in (guest) memory. To achieve reasonable performance, we
>>>> cache this data in our own data structures, so we need to sync those
>>>> two views from time to time. This behaviour is well described in the
>>>> GICv3 spec and is also exercised by hardware, so the sync points are
>>>> well known.
>>>>
>>>> Provide functions that read the guest memory and store the
>>>> information from the configuration and pending tables in the kernel.
>>>>
>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>>> ---
>>> would help to have change log between v1 -> v2 (valid for the whole series)
>>>>  include/kvm/arm_vgic.h  |   2 +
>>>>  virt/kvm/arm/its-emul.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  virt/kvm/arm/its-emul.h |   3 ++
>>>>  3 files changed, 129 insertions(+)
>>>>
>>>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>>>> index 2a67a10..323c33a 100644
>>>> --- a/include/kvm/arm_vgic.h
>>>> +++ b/include/kvm/arm_vgic.h
>>>> @@ -167,6 +167,8 @@ struct vgic_its {
>>>>  	int			cwriter;
>>>>  	struct list_head	device_list;
>>>>  	struct list_head	collection_list;
>>>> +	/* memory used for buffering guest's memory */
>>>> +	void			*buffer_page;
>>>>  };
>>>>  
>>>>  struct vgic_dist {
>>>> diff --git a/virt/kvm/arm/its-emul.c b/virt/kvm/arm/its-emul.c
>>>> index b9c40d7..05245cb 100644
>>>> --- a/virt/kvm/arm/its-emul.c
>>>> +++ b/virt/kvm/arm/its-emul.c
>>>> @@ -50,6 +50,7 @@ struct its_itte {
>>>>  	struct its_collection *collection;
>>>>  	u32 lpi;
>>>>  	u32 event_id;
>>>> +	u8 priority;
>>>>  	bool enabled;
>>>>  	unsigned long *pending;
>>>>  };
>>>> @@ -70,8 +71,124 @@ static struct its_itte *find_itte_by_lpi(struct kvm *kvm, int lpi)
>>>>  	return NULL;
>>>>  }
>>>>  
>>>> +#define LPI_PROP_ENABLE_BIT(p)	((p) & LPI_PROP_ENABLED)
>>>> +#define LPI_PROP_PRIORITY(p)	((p) & 0xfc)
>>>> +
>>>> +/* stores the priority and enable bit for a given LPI */
>>>> +static void update_lpi_config(struct kvm *kvm, struct its_itte *itte, u8 prop)
>>>> +{
>>>> +	itte->priority = LPI_PROP_PRIORITY(prop);
>>>> +	itte->enabled  = LPI_PROP_ENABLE_BIT(prop);
>>>> +}
>>>> +
>>>> +#define GIC_LPI_OFFSET 8192
>>>> +
>>>> +/* We scan the table in chunks the size of the smallest page size */
>>> 4kB chunks?
>>>> +#define CHUNK_SIZE 4096U
>>>> +
>>>>  #define BASER_BASE_ADDRESS(x) ((x) & 0xfffffffff000ULL)
>>>>  
>>>> +static int nr_idbits_propbase(u64 propbaser)
>>>> +{
>>>> +	int nr_idbits = (1U << (propbaser & 0x1f)) + 1;
>>>> +
>>>> +	return max(nr_idbits, INTERRUPT_ID_BITS_ITS);
>>>> +}
>>>> +
>>>> +/*
>>>> + * Scan the whole LPI configuration table and put the LPI configuration
>>>> + * data in our own data structures. This relies on the LPI being
>>>> + * mapped before.
>>>> + */
>>>> +static bool its_update_lpis_configuration(struct kvm *kvm)
>>>> +{
>>>> +	struct vgic_dist *dist = &kvm->arch.vgic;
>>>> +	u8 *prop = dist->its.buffer_page;
>>>> +	u32 tsize;
>>>> +	gpa_t propbase;
>>>> +	int lpi = GIC_LPI_OFFSET;
>>>> +	struct its_itte *itte;
>>>> +	struct its_device *device;
>>>> +	int ret;
>>>> +
>>>> +	propbase = BASER_BASE_ADDRESS(dist->propbaser);
>>>> +	tsize = nr_idbits_propbase(dist->propbaser);
>>>> +
>>>> +	while (tsize > 0) {
>>>> +		int chunksize = min(tsize, CHUNK_SIZE);
>>>> +
>>>> +		ret = kvm_read_guest(kvm, propbase, prop, chunksize);
>>> I think you still have the spin_lock issue  since if my understanding is
>>> correct this is called from
>>> vgic_handle_mmio_access/vcall_range_handler/gic_enable_lpis
>>> where vgic_handle_mmio_access. Or does it take another path?
>>>
>>> Shouldn't we create a new kvm_io_device to avoid holding the dist lock?
>>
>> Sorry I forgot it was the case already. But currently we always register
>> the same io ops (registration entry point being
>> vgic_register_kvm_io_dev) and maybe we should have separate dispatcher
>> function for dist, redit and its?
> 
> What would be the idea behind it? To have separate locks for each? I
> don't think that will work, as some ITS functions are called from GICv3
> register handler functions which manipulate members of the distributor
> structure.

Yes it was the idea.

 So I am more in favour of dropping the dist lock in these
> cases before handing off execution to ITS specific functions.

OK. Let's see how the refactoring looks like then

Cheers

Eric
> 
> Cheers,
> Andre.
>