date:20160803

Re: [PATCH] KVM: arm64: ITS: move ITS registration into first VCPU run

2016-08-03 Thread Auger Eric



On 03/08/2016 19:56, Auger Eric wrote:
> Hi
> 
> On 03/08/2016 19:48, Auger Eric wrote:
>> Hi Andre, Christoffer,
>>
>> On 03/08/2016 19:18, Andre Przywara wrote:
>>> Hi,
>>>
>>> On 03/08/16 18:11, Christoffer Dall wrote:
 On Wed, Aug 03, 2016 at 03:57:45PM +0100, Andre Przywara wrote:
> Currently we register ITS devices upon userland issuing the CTRL_INIT
> ioctl to mark initialization of the ITS as done.
> This deviates from the initialization sequence of the existing GIC
> devices and does not play well with the way QEMU handles things.
> To be more in line with what we are used to, register the ITS(es) just
> before the first VCPU is about to run, so in the map_resources() call.
> This involves iterating through the list of KVM devices and handle each
> ITS that we find.
>
> Signed-off-by: Andre Przywara 
> ---
> Hi,
>
> this is based upon next-20160728 plus Christoffer's kvm_device locking
> fix from today. Please let me know what tree I should base upon and I
> will rebase.
> Eric, Christoffer: does that do what you expect? Can QEMU live with that?
>
> Cheers,
> Andre.
>
>  virt/kvm/arm/vgic/vgic-its.c | 56 
> 
>  virt/kvm/arm/vgic/vgic-v3.c  |  6 +
>  virt/kvm/arm/vgic/vgic.h |  6 +
>  3 files changed, 58 insertions(+), 10 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 07411cf..e677a60 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -1288,13 +1288,13 @@ void vgic_enable_lpis(struct kvm_vcpu *vcpu)
>   its_sync_lpi_pending_table(vcpu);
>  }
>  
> -static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its)
> +static int vgic_register_its_iodev(struct kvm *kvm, struct vgic_its *its)
>  {
>   struct vgic_io_device *iodev = >iodev;
>   int ret;
>  
> - if (its->initialized)
> - return 0;
> + if (!its->initialized)
> + return -EBUSY;
>  
>   if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base))
>   return -ENXIO;
> @@ -1311,9 +1311,6 @@ static int vgic_its_init_its(struct kvm *kvm, 
> struct vgic_its *its)
> KVM_VGIC_V3_ITS_SIZE, >dev);
>   mutex_unlock(>slots_lock);
>  
> - if (!ret)
> - its->initialized = true;
> -
>   return ret;
>  }
>  
> @@ -1435,9 +1432,6 @@ static int vgic_its_set_attr(struct kvm_device *dev,
>   if (type != KVM_VGIC_ITS_ADDR_TYPE)
>   return -ENODEV;
>  
> - if (its->initialized)
> - return -EBUSY;
> -
>   if (copy_from_user(, uaddr, sizeof(addr)))
>   return -EFAULT;
>  
> @@ -1453,7 +1447,9 @@ static int vgic_its_set_attr(struct kvm_device *dev,
>   case KVM_DEV_ARM_VGIC_GRP_CTRL:
>   switch (attr->attr) {
>   case KVM_DEV_ARM_VGIC_CTRL_INIT:
> - return vgic_its_init_its(dev->kvm, its);
> + its->initialized = true;
> +
> + return 0;
>   }
>   break;
>   }
> @@ -1498,3 +1494,43 @@ int kvm_vgic_register_its_device(void)
>   return kvm_register_device_ops(_arm_vgic_its_ops,
>  KVM_DEV_TYPE_ARM_VGIC_ITS);
>  }
> +
> +/*
> + * Registers all ITSes with the kvm_io_bus framework.
> + * To follow the existing VGIC initialization sequence, this has to be
> + * done as late as possible, just before the first VCPU runs.
> + */
> +int vgic_register_its_iodevs(struct kvm *kvm)
> +{
> + struct kvm_device *dev;
> + int ret = 0;
> +
> + mutex_lock(>devices_lock);
> +
> + list_for_each_entry(dev, >devices, vm_node) {
> + if (dev->ops != _arm_vgic_its_ops)
> + continue;
> +
> + ret = vgic_register_its_iodev(kvm, dev->private);
> + if (ret)
> + break;
> + }
> +
> + if (ret) {
> + /* Iterate backwards to roll back previous registrations. */
> + for (dev = list_prev_entry(dev, vm_node);
> +  >vm_node != >devices;
> +  dev = list_prev_entry(dev, vm_node)) {
> + struct vgic_its *its = dev->private;
> +
> + if (dev->ops != _arm_vgic_its_ops)
> + continue;
> +
> + kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS,
> +   >iodev.dev);
> + }
> + }

 is the unregister really necessary?
>>>
>>> I was wondering the same, but we do it for the GICv3 redistributors as
>>> well (though that was introduced

Re: [PATCH] KVM: arm64: ITS: move ITS registration into first VCPU run

2016-08-03 Thread kbuild test robot

Hi Andre,

[auto build test ERROR on kvmarm/next]
[also build test ERROR on next-20160803]
[cannot apply to v4.7]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Andre-Przywara/KVM-arm64-ITS-move-ITS-registration-into-first-VCPU-run/20160803-235253
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git next
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm64 

All errors (new ones prefixed by >>):

   arch/arm64/kvm/../../../virt/kvm/arm/vgic/vgic-its.c: In function 
'vgic_register_its_iodevs':
>> arch/arm64/kvm/../../../virt/kvm/arm/vgic/vgic-its.c:1508:17: error: 'struct 
>> kvm' has no member named 'devices_lock'
 mutex_lock(>devices_lock);
^
   arch/arm64/kvm/../../../virt/kvm/arm/vgic/vgic-its.c:1534:19: error: 'struct 
kvm' has no member named 'devices_lock'
 mutex_unlock(>devices_lock);
  ^

vim +1508 arch/arm64/kvm/../../../virt/kvm/arm/vgic/vgic-its.c

  1502   */
  1503  int vgic_register_its_iodevs(struct kvm *kvm)
  1504  {
  1505  struct kvm_device *dev;
  1506  int ret = 0;
  1507  
> 1508  mutex_lock(>devices_lock);
  1509  
  1510  list_for_each_entry(dev, >devices, vm_node) {
  1511  if (dev->ops != _arm_vgic_its_ops)

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] KVM: arm64: ITS: move ITS registration into first VCPU run

2016-08-03 Thread Auger Eric

Hi

On 03/08/2016 19:48, Auger Eric wrote:
> Hi Andre, Christoffer,
> 
> On 03/08/2016 19:18, Andre Przywara wrote:
>> Hi,
>>
>> On 03/08/16 18:11, Christoffer Dall wrote:
>>> On Wed, Aug 03, 2016 at 03:57:45PM +0100, Andre Przywara wrote:
 Currently we register ITS devices upon userland issuing the CTRL_INIT
 ioctl to mark initialization of the ITS as done.
 This deviates from the initialization sequence of the existing GIC
 devices and does not play well with the way QEMU handles things.
 To be more in line with what we are used to, register the ITS(es) just
 before the first VCPU is about to run, so in the map_resources() call.
 This involves iterating through the list of KVM devices and handle each
 ITS that we find.

 Signed-off-by: Andre Przywara 
 ---
 Hi,

 this is based upon next-20160728 plus Christoffer's kvm_device locking
 fix from today. Please let me know what tree I should base upon and I
 will rebase.
 Eric, Christoffer: does that do what you expect? Can QEMU live with that?

 Cheers,
 Andre.

  virt/kvm/arm/vgic/vgic-its.c | 56 
 
  virt/kvm/arm/vgic/vgic-v3.c  |  6 +
  virt/kvm/arm/vgic/vgic.h |  6 +
  3 files changed, 58 insertions(+), 10 deletions(-)

 diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
 index 07411cf..e677a60 100644
 --- a/virt/kvm/arm/vgic/vgic-its.c
 +++ b/virt/kvm/arm/vgic/vgic-its.c
 @@ -1288,13 +1288,13 @@ void vgic_enable_lpis(struct kvm_vcpu *vcpu)
its_sync_lpi_pending_table(vcpu);
  }
  
 -static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its)
 +static int vgic_register_its_iodev(struct kvm *kvm, struct vgic_its *its)
  {
struct vgic_io_device *iodev = >iodev;
int ret;
  
 -  if (its->initialized)
 -  return 0;
 +  if (!its->initialized)
 +  return -EBUSY;
  
if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base))
return -ENXIO;
 @@ -1311,9 +1311,6 @@ static int vgic_its_init_its(struct kvm *kvm, struct 
 vgic_its *its)
  KVM_VGIC_V3_ITS_SIZE, >dev);
mutex_unlock(>slots_lock);
  
 -  if (!ret)
 -  its->initialized = true;
 -
return ret;
  }
  
 @@ -1435,9 +1432,6 @@ static int vgic_its_set_attr(struct kvm_device *dev,
if (type != KVM_VGIC_ITS_ADDR_TYPE)
return -ENODEV;
  
 -  if (its->initialized)
 -  return -EBUSY;
 -
if (copy_from_user(, uaddr, sizeof(addr)))
return -EFAULT;
  
 @@ -1453,7 +1447,9 @@ static int vgic_its_set_attr(struct kvm_device *dev,
case KVM_DEV_ARM_VGIC_GRP_CTRL:
switch (attr->attr) {
case KVM_DEV_ARM_VGIC_CTRL_INIT:
 -  return vgic_its_init_its(dev->kvm, its);
 +  its->initialized = true;
 +
 +  return 0;
}
break;
}
 @@ -1498,3 +1494,43 @@ int kvm_vgic_register_its_device(void)
return kvm_register_device_ops(_arm_vgic_its_ops,
   KVM_DEV_TYPE_ARM_VGIC_ITS);
  }
 +
 +/*
 + * Registers all ITSes with the kvm_io_bus framework.
 + * To follow the existing VGIC initialization sequence, this has to be
 + * done as late as possible, just before the first VCPU runs.
 + */
 +int vgic_register_its_iodevs(struct kvm *kvm)
 +{
 +  struct kvm_device *dev;
 +  int ret = 0;
 +
 +  mutex_lock(>devices_lock);
 +
 +  list_for_each_entry(dev, >devices, vm_node) {
 +  if (dev->ops != _arm_vgic_its_ops)
 +  continue;
 +
 +  ret = vgic_register_its_iodev(kvm, dev->private);
 +  if (ret)
 +  break;
 +  }
 +
 +  if (ret) {
 +  /* Iterate backwards to roll back previous registrations. */
 +  for (dev = list_prev_entry(dev, vm_node);
 +   >vm_node != >devices;
 +   dev = list_prev_entry(dev, vm_node)) {
 +  struct vgic_its *its = dev->private;
 +
 +  if (dev->ops != _arm_vgic_its_ops)
 +  continue;
 +
 +  kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS,
 +>iodev.dev);
 +  }
 +  }
>>>
>>> is the unregister really necessary?
>>
>> I was wondering the same, but we do it for the GICv3 redistributors as
>> well (though that was introduced by the same stupid author).
>> That being said I would be too happy to remove both these dodgy routines
>> if we agree that a

Re: [PATCH] KVM: arm64: ITS: move ITS registration into first VCPU run

2016-08-03 Thread Christoffer Dall

On Wed, Aug 03, 2016 at 07:48:15PM +0200, Auger Eric wrote:
> Hi Andre, Christoffer,
> 
> On 03/08/2016 19:18, Andre Przywara wrote:
> > Hi,
> > 
> > On 03/08/16 18:11, Christoffer Dall wrote:
> >> On Wed, Aug 03, 2016 at 03:57:45PM +0100, Andre Przywara wrote:
> >>> Currently we register ITS devices upon userland issuing the CTRL_INIT
> >>> ioctl to mark initialization of the ITS as done.
> >>> This deviates from the initialization sequence of the existing GIC
> >>> devices and does not play well with the way QEMU handles things.
> >>> To be more in line with what we are used to, register the ITS(es) just
> >>> before the first VCPU is about to run, so in the map_resources() call.
> >>> This involves iterating through the list of KVM devices and handle each
> >>> ITS that we find.
> >>>
> >>> Signed-off-by: Andre Przywara 
> >>> ---
> >>> Hi,
> >>>
> >>> this is based upon next-20160728 plus Christoffer's kvm_device locking
> >>> fix from today. Please let me know what tree I should base upon and I
> >>> will rebase.
> >>> Eric, Christoffer: does that do what you expect? Can QEMU live with that?
> >>>
> >>> Cheers,
> >>> Andre.
> >>>
> >>>  virt/kvm/arm/vgic/vgic-its.c | 56 
> >>> 
> >>>  virt/kvm/arm/vgic/vgic-v3.c  |  6 +
> >>>  virt/kvm/arm/vgic/vgic.h |  6 +
> >>>  3 files changed, 58 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> >>> index 07411cf..e677a60 100644
> >>> --- a/virt/kvm/arm/vgic/vgic-its.c
> >>> +++ b/virt/kvm/arm/vgic/vgic-its.c
> >>> @@ -1288,13 +1288,13 @@ void vgic_enable_lpis(struct kvm_vcpu *vcpu)
> >>>   its_sync_lpi_pending_table(vcpu);
> >>>  }
> >>>  
> >>> -static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its)
> >>> +static int vgic_register_its_iodev(struct kvm *kvm, struct vgic_its *its)
> >>>  {
> >>>   struct vgic_io_device *iodev = >iodev;
> >>>   int ret;
> >>>  
> >>> - if (its->initialized)
> >>> - return 0;
> >>> + if (!its->initialized)
> >>> + return -EBUSY;
> >>>  
> >>>   if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base))
> >>>   return -ENXIO;
> >>> @@ -1311,9 +1311,6 @@ static int vgic_its_init_its(struct kvm *kvm, 
> >>> struct vgic_its *its)
> >>> KVM_VGIC_V3_ITS_SIZE, >dev);
> >>>   mutex_unlock(>slots_lock);
> >>>  
> >>> - if (!ret)
> >>> - its->initialized = true;
> >>> -
> >>>   return ret;
> >>>  }
> >>>  
> >>> @@ -1435,9 +1432,6 @@ static int vgic_its_set_attr(struct kvm_device *dev,
> >>>   if (type != KVM_VGIC_ITS_ADDR_TYPE)
> >>>   return -ENODEV;
> >>>  
> >>> - if (its->initialized)
> >>> - return -EBUSY;
> >>> -
> >>>   if (copy_from_user(, uaddr, sizeof(addr)))
> >>>   return -EFAULT;
> >>>  
> >>> @@ -1453,7 +1447,9 @@ static int vgic_its_set_attr(struct kvm_device *dev,
> >>>   case KVM_DEV_ARM_VGIC_GRP_CTRL:
> >>>   switch (attr->attr) {
> >>>   case KVM_DEV_ARM_VGIC_CTRL_INIT:
> >>> - return vgic_its_init_its(dev->kvm, its);
> >>> + its->initialized = true;
> >>> +
> >>> + return 0;
> >>>   }
> >>>   break;
> >>>   }
> >>> @@ -1498,3 +1494,43 @@ int kvm_vgic_register_its_device(void)
> >>>   return kvm_register_device_ops(_arm_vgic_its_ops,
> >>>  KVM_DEV_TYPE_ARM_VGIC_ITS);
> >>>  }
> >>> +
> >>> +/*
> >>> + * Registers all ITSes with the kvm_io_bus framework.
> >>> + * To follow the existing VGIC initialization sequence, this has to be
> >>> + * done as late as possible, just before the first VCPU runs.
> >>> + */
> >>> +int vgic_register_its_iodevs(struct kvm *kvm)
> >>> +{
> >>> + struct kvm_device *dev;
> >>> + int ret = 0;
> >>> +
> >>> + mutex_lock(>devices_lock);
> >>> +
> >>> + list_for_each_entry(dev, >devices, vm_node) {
> >>> + if (dev->ops != _arm_vgic_its_ops)
> >>> + continue;
> >>> +
> >>> + ret = vgic_register_its_iodev(kvm, dev->private);
> >>> + if (ret)
> >>> + break;
> >>> + }
> >>> +
> >>> + if (ret) {
> >>> + /* Iterate backwards to roll back previous registrations. */
> >>> + for (dev = list_prev_entry(dev, vm_node);
> >>> +  >vm_node != >devices;
> >>> +  dev = list_prev_entry(dev, vm_node)) {
> >>> + struct vgic_its *its = dev->private;
> >>> +
> >>> + if (dev->ops != _arm_vgic_its_ops)
> >>> + continue;
> >>> +
> >>> + kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS,
> >>> +   >iodev.dev);
> >>> + }
> >>> + }
> >>
> >> is the unregister really necessary?
> > 
> > I was wondering the same, but we do it for the GICv3 redistributors as
> > well (though that was introduced by the same stupid author).
>

Re: [PATCH] KVM: arm64: ITS: move ITS registration into first VCPU run

2016-08-03 Thread Auger Eric

Hi Andre, Christoffer,

On 03/08/2016 19:18, Andre Przywara wrote:
> Hi,
> 
> On 03/08/16 18:11, Christoffer Dall wrote:
>> On Wed, Aug 03, 2016 at 03:57:45PM +0100, Andre Przywara wrote:
>>> Currently we register ITS devices upon userland issuing the CTRL_INIT
>>> ioctl to mark initialization of the ITS as done.
>>> This deviates from the initialization sequence of the existing GIC
>>> devices and does not play well with the way QEMU handles things.
>>> To be more in line with what we are used to, register the ITS(es) just
>>> before the first VCPU is about to run, so in the map_resources() call.
>>> This involves iterating through the list of KVM devices and handle each
>>> ITS that we find.
>>>
>>> Signed-off-by: Andre Przywara 
>>> ---
>>> Hi,
>>>
>>> this is based upon next-20160728 plus Christoffer's kvm_device locking
>>> fix from today. Please let me know what tree I should base upon and I
>>> will rebase.
>>> Eric, Christoffer: does that do what you expect? Can QEMU live with that?
>>>
>>> Cheers,
>>> Andre.
>>>
>>>  virt/kvm/arm/vgic/vgic-its.c | 56 
>>> 
>>>  virt/kvm/arm/vgic/vgic-v3.c  |  6 +
>>>  virt/kvm/arm/vgic/vgic.h |  6 +
>>>  3 files changed, 58 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>>> index 07411cf..e677a60 100644
>>> --- a/virt/kvm/arm/vgic/vgic-its.c
>>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>>> @@ -1288,13 +1288,13 @@ void vgic_enable_lpis(struct kvm_vcpu *vcpu)
>>> its_sync_lpi_pending_table(vcpu);
>>>  }
>>>  
>>> -static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its)
>>> +static int vgic_register_its_iodev(struct kvm *kvm, struct vgic_its *its)
>>>  {
>>> struct vgic_io_device *iodev = >iodev;
>>> int ret;
>>>  
>>> -   if (its->initialized)
>>> -   return 0;
>>> +   if (!its->initialized)
>>> +   return -EBUSY;
>>>  
>>> if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base))
>>> return -ENXIO;
>>> @@ -1311,9 +1311,6 @@ static int vgic_its_init_its(struct kvm *kvm, struct 
>>> vgic_its *its)
>>>   KVM_VGIC_V3_ITS_SIZE, >dev);
>>> mutex_unlock(>slots_lock);
>>>  
>>> -   if (!ret)
>>> -   its->initialized = true;
>>> -
>>> return ret;
>>>  }
>>>  
>>> @@ -1435,9 +1432,6 @@ static int vgic_its_set_attr(struct kvm_device *dev,
>>> if (type != KVM_VGIC_ITS_ADDR_TYPE)
>>> return -ENODEV;
>>>  
>>> -   if (its->initialized)
>>> -   return -EBUSY;
>>> -
>>> if (copy_from_user(, uaddr, sizeof(addr)))
>>> return -EFAULT;
>>>  
>>> @@ -1453,7 +1447,9 @@ static int vgic_its_set_attr(struct kvm_device *dev,
>>> case KVM_DEV_ARM_VGIC_GRP_CTRL:
>>> switch (attr->attr) {
>>> case KVM_DEV_ARM_VGIC_CTRL_INIT:
>>> -   return vgic_its_init_its(dev->kvm, its);
>>> +   its->initialized = true;
>>> +
>>> +   return 0;
>>> }
>>> break;
>>> }
>>> @@ -1498,3 +1494,43 @@ int kvm_vgic_register_its_device(void)
>>> return kvm_register_device_ops(_arm_vgic_its_ops,
>>>KVM_DEV_TYPE_ARM_VGIC_ITS);
>>>  }
>>> +
>>> +/*
>>> + * Registers all ITSes with the kvm_io_bus framework.
>>> + * To follow the existing VGIC initialization sequence, this has to be
>>> + * done as late as possible, just before the first VCPU runs.
>>> + */
>>> +int vgic_register_its_iodevs(struct kvm *kvm)
>>> +{
>>> +   struct kvm_device *dev;
>>> +   int ret = 0;
>>> +
>>> +   mutex_lock(>devices_lock);
>>> +
>>> +   list_for_each_entry(dev, >devices, vm_node) {
>>> +   if (dev->ops != _arm_vgic_its_ops)
>>> +   continue;
>>> +
>>> +   ret = vgic_register_its_iodev(kvm, dev->private);
>>> +   if (ret)
>>> +   break;
>>> +   }
>>> +
>>> +   if (ret) {
>>> +   /* Iterate backwards to roll back previous registrations. */
>>> +   for (dev = list_prev_entry(dev, vm_node);
>>> +>vm_node != >devices;
>>> +dev = list_prev_entry(dev, vm_node)) {
>>> +   struct vgic_its *its = dev->private;
>>> +
>>> +   if (dev->ops != _arm_vgic_its_ops)
>>> +   continue;
>>> +
>>> +   kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS,
>>> + >iodev.dev);
>>> +   }
>>> +   }
>>
>> is the unregister really necessary?
> 
> I was wondering the same, but we do it for the GICv3 redistributors as
> well (though that was introduced by the same stupid author).
> That being said I would be too happy to remove both these dodgy routines
> if we agree that a failure will ultimately lead to a VM teardown and is
> thus not needed.

Well to me this will lead to a

Re: [PATCH] KVM: arm64: ITS: move ITS registration into first VCPU run

2016-08-03 Thread Christoffer Dall

On Wed, Aug 03, 2016 at 06:18:48PM +0100, Andre Przywara wrote:
> Hi,
> 
> On 03/08/16 18:11, Christoffer Dall wrote:
> > On Wed, Aug 03, 2016 at 03:57:45PM +0100, Andre Przywara wrote:
> >> Currently we register ITS devices upon userland issuing the CTRL_INIT
> >> ioctl to mark initialization of the ITS as done.
> >> This deviates from the initialization sequence of the existing GIC
> >> devices and does not play well with the way QEMU handles things.
> >> To be more in line with what we are used to, register the ITS(es) just
> >> before the first VCPU is about to run, so in the map_resources() call.
> >> This involves iterating through the list of KVM devices and handle each
> >> ITS that we find.
> >>
> >> Signed-off-by: Andre Przywara 
> >> ---
> >> Hi,
> >>
> >> this is based upon next-20160728 plus Christoffer's kvm_device locking
> >> fix from today. Please let me know what tree I should base upon and I
> >> will rebase.
> >> Eric, Christoffer: does that do what you expect? Can QEMU live with that?
> >>
> >> Cheers,
> >> Andre.
> >>
> >>  virt/kvm/arm/vgic/vgic-its.c | 56 
> >> 
> >>  virt/kvm/arm/vgic/vgic-v3.c  |  6 +
> >>  virt/kvm/arm/vgic/vgic.h |  6 +
> >>  3 files changed, 58 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> >> index 07411cf..e677a60 100644
> >> --- a/virt/kvm/arm/vgic/vgic-its.c
> >> +++ b/virt/kvm/arm/vgic/vgic-its.c
> >> @@ -1288,13 +1288,13 @@ void vgic_enable_lpis(struct kvm_vcpu *vcpu)
> >>its_sync_lpi_pending_table(vcpu);
> >>  }
> >>  
> >> -static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its)
> >> +static int vgic_register_its_iodev(struct kvm *kvm, struct vgic_its *its)
> >>  {
> >>struct vgic_io_device *iodev = >iodev;
> >>int ret;
> >>  
> >> -  if (its->initialized)
> >> -  return 0;
> >> +  if (!its->initialized)
> >> +  return -EBUSY;
> >>  
> >>if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base))
> >>return -ENXIO;
> >> @@ -1311,9 +1311,6 @@ static int vgic_its_init_its(struct kvm *kvm, struct 
> >> vgic_its *its)
> >>  KVM_VGIC_V3_ITS_SIZE, >dev);
> >>mutex_unlock(>slots_lock);
> >>  
> >> -  if (!ret)
> >> -  its->initialized = true;
> >> -
> >>return ret;
> >>  }
> >>  
> >> @@ -1435,9 +1432,6 @@ static int vgic_its_set_attr(struct kvm_device *dev,
> >>if (type != KVM_VGIC_ITS_ADDR_TYPE)
> >>return -ENODEV;
> >>  
> >> -  if (its->initialized)
> >> -  return -EBUSY;
> >> -
> >>if (copy_from_user(, uaddr, sizeof(addr)))
> >>return -EFAULT;
> >>  
> >> @@ -1453,7 +1447,9 @@ static int vgic_its_set_attr(struct kvm_device *dev,
> >>case KVM_DEV_ARM_VGIC_GRP_CTRL:
> >>switch (attr->attr) {
> >>case KVM_DEV_ARM_VGIC_CTRL_INIT:
> >> -  return vgic_its_init_its(dev->kvm, its);
> >> +  its->initialized = true;
> >> +
> >> +  return 0;
> >>}
> >>break;
> >>}
> >> @@ -1498,3 +1494,43 @@ int kvm_vgic_register_its_device(void)
> >>return kvm_register_device_ops(_arm_vgic_its_ops,
> >>   KVM_DEV_TYPE_ARM_VGIC_ITS);
> >>  }
> >> +
> >> +/*
> >> + * Registers all ITSes with the kvm_io_bus framework.
> >> + * To follow the existing VGIC initialization sequence, this has to be
> >> + * done as late as possible, just before the first VCPU runs.
> >> + */
> >> +int vgic_register_its_iodevs(struct kvm *kvm)
> >> +{
> >> +  struct kvm_device *dev;
> >> +  int ret = 0;
> >> +
> >> +  mutex_lock(>devices_lock);
> >> +
> >> +  list_for_each_entry(dev, >devices, vm_node) {
> >> +  if (dev->ops != _arm_vgic_its_ops)
> >> +  continue;
> >> +
> >> +  ret = vgic_register_its_iodev(kvm, dev->private);
> >> +  if (ret)
> >> +  break;
> >> +  }
> >> +
> >> +  if (ret) {
> >> +  /* Iterate backwards to roll back previous registrations. */
> >> +  for (dev = list_prev_entry(dev, vm_node);
> >> +   >vm_node != >devices;
> >> +   dev = list_prev_entry(dev, vm_node)) {
> >> +  struct vgic_its *its = dev->private;
> >> +
> >> +  if (dev->ops != _arm_vgic_its_ops)
> >> +  continue;
> >> +
> >> +  kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS,
> >> +>iodev.dev);
> >> +  }
> >> +  }
> > 
> > is the unregister really necessary?
> 
> I was wondering the same, but we do it for the GICv3 redistributors as
> well (though that was introduced by the same stupid author).
> That being said I would be too happy to remove both these dodgy routines
> if we agree that a failure will ultimately lead to a VM teardown and is
>

Re: [PATCH v2] KVM: arm64: ITS: return 1 on successful MSI injection

2016-08-03 Thread Christoffer Dall

On Tue, Aug 02, 2016 at 01:56:05PM +0100, Andre Przywara wrote:
> According to the KVM API documentation a successful MSI injection
> should return a value > 0 on success.
> Return possible errors in vgic_its_trigger_msi() and report a
> successful injection back to userland, while also reporting the
> case where the MSI could not be delivered due to the guest not
> having the LPI mapped, for instance.
> 
> Signed-off-by: Andre Przywara 
> ---
> Hi,
> 
> my brain is too toasted at the moment to find the proper error values
> that we should return in vgic_its_trigger_msi() for the various cases
> that could go wrong, so please feel free to suggest better values.
> In the end we return either 0 or 1 (and ignore any error value at the
> ITS level), so it shouldn't really make much difference.
> 
> Cheers,
> Andre.
> 
> Changelog v1 .. v2:
> - rework vgic_its_trigger_msi() to return proper error values
> - return 0 to userland if the LPI could not be injected
> 
>  include/linux/irqchip/arm-gic-v3.h |  1 +
>  virt/kvm/arm/vgic/vgic-its.c   | 48 
> +++---
>  2 files changed, 30 insertions(+), 19 deletions(-)
> 
> diff --git a/include/linux/irqchip/arm-gic-v3.h 
> b/include/linux/irqchip/arm-gic-v3.h
> index 56b0b7e..99ac022 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -337,6 +337,7 @@
>   */
>  #define E_ITS_MOVI_UNMAPPED_INTERRUPT0x010107
>  #define E_ITS_MOVI_UNMAPPED_COLLECTION   0x010109
> +#define E_ITS_INT_UNMAPPED_INTERRUPT 0x010307
>  #define E_ITS_CLEAR_UNMAPPED_INTERRUPT   0x010507
>  #define E_ITS_MAPD_DEVICE_OOR0x010801
>  #define E_ITS_MAPC_PROCNUM_OOR   0x010902
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 07411cf..456903c 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -441,39 +441,48 @@ static unsigned long vgic_mmio_read_its_idregs(struct 
> kvm *kvm,
>   * Find the target VCPU and the LPI number for a given devid/eventid pair
>   * and make this IRQ pending, possibly injecting it.
>   * Must be called with the its_lock mutex held.
> + * Returns 0 on success, a positive error value for any ITS mapping
> + * related errors and negative error values for generic errors.
>   */
> -static void vgic_its_trigger_msi(struct kvm *kvm, struct vgic_its *its,
> -  u32 devid, u32 eventid)
> +static int vgic_its_trigger_msi(struct kvm *kvm, struct vgic_its *its,
> + u32 devid, u32 eventid)
>  {
> + struct kvm_vcpu *vcpu;
>   struct its_itte *itte;
>  
>   if (!its->enabled)
> - return;
> + return -EBUSY;
>  
>   itte = find_itte(its, devid, eventid);
> - /* Triggering an unmapped IRQ gets silently dropped. */
> - if (itte && its_is_collection_mapped(itte->collection)) {
> - struct kvm_vcpu *vcpu;
> -
> - vcpu = kvm_get_vcpu(kvm, itte->collection->target_addr);
> - if (vcpu && vcpu->arch.vgic_cpu.lpis_enabled) {
> - spin_lock(>irq->irq_lock);
> - itte->irq->pending = true;
> - vgic_queue_irq_unlock(kvm, itte->irq);
> - }
> - }
> + if (!itte || !its_is_collection_mapped(itte->collection))
> + return E_ITS_INT_UNMAPPED_INTERRUPT;
> +
> + vcpu = kvm_get_vcpu(kvm, itte->collection->target_addr);
> + if (!vcpu)
> + return E_ITS_INT_UNMAPPED_INTERRUPT;
> +
> + if (!vcpu->arch.vgic_cpu.lpis_enabled)
> + return -EBUSY;
> +
> + spin_lock(>irq->irq_lock);
> + itte->irq->pending = true;
> + vgic_queue_irq_unlock(kvm, itte->irq);
> +
> + return 0;
>  }
>  
>  /*
>   * Queries the KVM IO bus framework to get the ITS pointer from the given
>   * doorbell address.
>   * We then call vgic_its_trigger_msi() with the decoded data.
> + * According to the KVM_SIGNAL_MSI API description returns 1 on success.
>   */
>  int vgic_its_inject_msi(struct kvm *kvm, struct kvm_msi *msi)
>  {
>   u64 address;
>   struct kvm_io_device *kvm_io_dev;
>   struct vgic_io_device *iodev;
> + int ret;
>  
>   if (!vgic_has_its(kvm))
>   return -ENODEV;
> @@ -490,10 +499,13 @@ int vgic_its_inject_msi(struct kvm *kvm, struct kvm_msi 
> *msi)
>   iodev = container_of(kvm_io_dev, struct vgic_io_device, dev);
>  
>   mutex_lock(>its->its_lock);
> - vgic_its_trigger_msi(kvm, iodev->its, msi->devid, msi->data);
> + ret = vgic_its_trigger_msi(kvm, iodev->its, msi->devid, msi->data);
>   mutex_unlock(>its->its_lock);
>  
> - return 0;
> + if (ret)
> + return 0;
> + else
> + return 1;

I think this should be:

  if (ret < 0)
return ret;
  else if (ret)
return 0;
  else
return 1;


Thanks,

Re: [PATCH] KVM: arm64: ITS: move ITS registration into first VCPU run

2016-08-03 Thread Andre Przywara

Hi,

On 03/08/16 18:11, Christoffer Dall wrote:
> On Wed, Aug 03, 2016 at 03:57:45PM +0100, Andre Przywara wrote:
>> Currently we register ITS devices upon userland issuing the CTRL_INIT
>> ioctl to mark initialization of the ITS as done.
>> This deviates from the initialization sequence of the existing GIC
>> devices and does not play well with the way QEMU handles things.
>> To be more in line with what we are used to, register the ITS(es) just
>> before the first VCPU is about to run, so in the map_resources() call.
>> This involves iterating through the list of KVM devices and handle each
>> ITS that we find.
>>
>> Signed-off-by: Andre Przywara 
>> ---
>> Hi,
>>
>> this is based upon next-20160728 plus Christoffer's kvm_device locking
>> fix from today. Please let me know what tree I should base upon and I
>> will rebase.
>> Eric, Christoffer: does that do what you expect? Can QEMU live with that?
>>
>> Cheers,
>> Andre.
>>
>>  virt/kvm/arm/vgic/vgic-its.c | 56 
>> 
>>  virt/kvm/arm/vgic/vgic-v3.c  |  6 +
>>  virt/kvm/arm/vgic/vgic.h |  6 +
>>  3 files changed, 58 insertions(+), 10 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> index 07411cf..e677a60 100644
>> --- a/virt/kvm/arm/vgic/vgic-its.c
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -1288,13 +1288,13 @@ void vgic_enable_lpis(struct kvm_vcpu *vcpu)
>>  its_sync_lpi_pending_table(vcpu);
>>  }
>>  
>> -static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its)
>> +static int vgic_register_its_iodev(struct kvm *kvm, struct vgic_its *its)
>>  {
>>  struct vgic_io_device *iodev = >iodev;
>>  int ret;
>>  
>> -if (its->initialized)
>> -return 0;
>> +if (!its->initialized)
>> +return -EBUSY;
>>  
>>  if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base))
>>  return -ENXIO;
>> @@ -1311,9 +1311,6 @@ static int vgic_its_init_its(struct kvm *kvm, struct 
>> vgic_its *its)
>>KVM_VGIC_V3_ITS_SIZE, >dev);
>>  mutex_unlock(>slots_lock);
>>  
>> -if (!ret)
>> -its->initialized = true;
>> -
>>  return ret;
>>  }
>>  
>> @@ -1435,9 +1432,6 @@ static int vgic_its_set_attr(struct kvm_device *dev,
>>  if (type != KVM_VGIC_ITS_ADDR_TYPE)
>>  return -ENODEV;
>>  
>> -if (its->initialized)
>> -return -EBUSY;
>> -
>>  if (copy_from_user(, uaddr, sizeof(addr)))
>>  return -EFAULT;
>>  
>> @@ -1453,7 +1447,9 @@ static int vgic_its_set_attr(struct kvm_device *dev,
>>  case KVM_DEV_ARM_VGIC_GRP_CTRL:
>>  switch (attr->attr) {
>>  case KVM_DEV_ARM_VGIC_CTRL_INIT:
>> -return vgic_its_init_its(dev->kvm, its);
>> +its->initialized = true;
>> +
>> +return 0;
>>  }
>>  break;
>>  }
>> @@ -1498,3 +1494,43 @@ int kvm_vgic_register_its_device(void)
>>  return kvm_register_device_ops(_arm_vgic_its_ops,
>> KVM_DEV_TYPE_ARM_VGIC_ITS);
>>  }
>> +
>> +/*
>> + * Registers all ITSes with the kvm_io_bus framework.
>> + * To follow the existing VGIC initialization sequence, this has to be
>> + * done as late as possible, just before the first VCPU runs.
>> + */
>> +int vgic_register_its_iodevs(struct kvm *kvm)
>> +{
>> +struct kvm_device *dev;
>> +int ret = 0;
>> +
>> +mutex_lock(>devices_lock);
>> +
>> +list_for_each_entry(dev, >devices, vm_node) {
>> +if (dev->ops != _arm_vgic_its_ops)
>> +continue;
>> +
>> +ret = vgic_register_its_iodev(kvm, dev->private);
>> +if (ret)
>> +break;
>> +}
>> +
>> +if (ret) {
>> +/* Iterate backwards to roll back previous registrations. */
>> +for (dev = list_prev_entry(dev, vm_node);
>> + >vm_node != >devices;
>> + dev = list_prev_entry(dev, vm_node)) {
>> +struct vgic_its *its = dev->private;
>> +
>> +if (dev->ops != _arm_vgic_its_ops)
>> +continue;
>> +
>> +kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS,
>> +  >iodev.dev);
>> +}
>> +}
> 
> is the unregister really necessary?

I was wondering the same, but we do it for the GICv3 redistributors as
well (though that was introduced by the same stupid author).
That being said I would be too happy to remove both these dodgy routines
if we agree that a failure will ultimately lead to a VM teardown and is
thus not needed.

Cheers,
Andre.

> 
>> +
>> +mutex_unlock(>devices_lock);
>> +return ret;
>> +}
>> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
>> index 0506543..f0d9b2b 100644
>> ---

Re: [PATCH] KVM: arm64: ITS: move ITS registration into first VCPU run

2016-08-03 Thread Christoffer Dall

On Wed, Aug 03, 2016 at 03:57:45PM +0100, Andre Przywara wrote:
> Currently we register ITS devices upon userland issuing the CTRL_INIT
> ioctl to mark initialization of the ITS as done.
> This deviates from the initialization sequence of the existing GIC
> devices and does not play well with the way QEMU handles things.
> To be more in line with what we are used to, register the ITS(es) just
> before the first VCPU is about to run, so in the map_resources() call.
> This involves iterating through the list of KVM devices and handle each
> ITS that we find.
> 
> Signed-off-by: Andre Przywara 
> ---
> Hi,
> 
> this is based upon next-20160728 plus Christoffer's kvm_device locking

Since my fix was horribly broken, I suggest you just base this on next
and let it be similarly broken to the vfio-device that we already have
in the kernel.

That way, we get the userspace ABI right and we fix KVM in general for -rc2.

Paolo/Radim, are you ok with this?

Thanks,
-Christoffer
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] KVM: arm64: ITS: move ITS registration into first VCPU run

2016-08-03 Thread Christoffer Dall

On Wed, Aug 03, 2016 at 03:57:45PM +0100, Andre Przywara wrote:
> Currently we register ITS devices upon userland issuing the CTRL_INIT
> ioctl to mark initialization of the ITS as done.
> This deviates from the initialization sequence of the existing GIC
> devices and does not play well with the way QEMU handles things.
> To be more in line with what we are used to, register the ITS(es) just
> before the first VCPU is about to run, so in the map_resources() call.
> This involves iterating through the list of KVM devices and handle each
> ITS that we find.
> 
> Signed-off-by: Andre Przywara 
> ---
> Hi,
> 
> this is based upon next-20160728 plus Christoffer's kvm_device locking
> fix from today. Please let me know what tree I should base upon and I
> will rebase.
> Eric, Christoffer: does that do what you expect? Can QEMU live with that?
> 
> Cheers,
> Andre.
> 
>  virt/kvm/arm/vgic/vgic-its.c | 56 
> 
>  virt/kvm/arm/vgic/vgic-v3.c  |  6 +
>  virt/kvm/arm/vgic/vgic.h |  6 +
>  3 files changed, 58 insertions(+), 10 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 07411cf..e677a60 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -1288,13 +1288,13 @@ void vgic_enable_lpis(struct kvm_vcpu *vcpu)
>   its_sync_lpi_pending_table(vcpu);
>  }
>  
> -static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its)
> +static int vgic_register_its_iodev(struct kvm *kvm, struct vgic_its *its)
>  {
>   struct vgic_io_device *iodev = >iodev;
>   int ret;
>  
> - if (its->initialized)
> - return 0;
> + if (!its->initialized)
> + return -EBUSY;
>  
>   if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base))
>   return -ENXIO;
> @@ -1311,9 +1311,6 @@ static int vgic_its_init_its(struct kvm *kvm, struct 
> vgic_its *its)
> KVM_VGIC_V3_ITS_SIZE, >dev);
>   mutex_unlock(>slots_lock);
>  
> - if (!ret)
> - its->initialized = true;
> -
>   return ret;
>  }
>  
> @@ -1435,9 +1432,6 @@ static int vgic_its_set_attr(struct kvm_device *dev,
>   if (type != KVM_VGIC_ITS_ADDR_TYPE)
>   return -ENODEV;
>  
> - if (its->initialized)
> - return -EBUSY;
> -
>   if (copy_from_user(, uaddr, sizeof(addr)))
>   return -EFAULT;
>  
> @@ -1453,7 +1447,9 @@ static int vgic_its_set_attr(struct kvm_device *dev,
>   case KVM_DEV_ARM_VGIC_GRP_CTRL:
>   switch (attr->attr) {
>   case KVM_DEV_ARM_VGIC_CTRL_INIT:
> - return vgic_its_init_its(dev->kvm, its);
> + its->initialized = true;
> +
> + return 0;
>   }
>   break;
>   }
> @@ -1498,3 +1494,43 @@ int kvm_vgic_register_its_device(void)
>   return kvm_register_device_ops(_arm_vgic_its_ops,
>  KVM_DEV_TYPE_ARM_VGIC_ITS);
>  }
> +
> +/*
> + * Registers all ITSes with the kvm_io_bus framework.
> + * To follow the existing VGIC initialization sequence, this has to be
> + * done as late as possible, just before the first VCPU runs.
> + */
> +int vgic_register_its_iodevs(struct kvm *kvm)
> +{
> + struct kvm_device *dev;
> + int ret = 0;
> +
> + mutex_lock(>devices_lock);
> +
> + list_for_each_entry(dev, >devices, vm_node) {
> + if (dev->ops != _arm_vgic_its_ops)
> + continue;
> +
> + ret = vgic_register_its_iodev(kvm, dev->private);
> + if (ret)
> + break;
> + }
> +
> + if (ret) {
> + /* Iterate backwards to roll back previous registrations. */
> + for (dev = list_prev_entry(dev, vm_node);
> +  >vm_node != >devices;
> +  dev = list_prev_entry(dev, vm_node)) {
> + struct vgic_its *its = dev->private;
> +
> + if (dev->ops != _arm_vgic_its_ops)
> + continue;
> +
> + kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS,
> +   >iodev.dev);
> + }
> + }

is the unregister really necessary?

> +
> + mutex_unlock(>devices_lock);
> + return ret;
> +}
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index 0506543..f0d9b2b 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -289,6 +289,12 @@ int vgic_v3_map_resources(struct kvm *kvm)
>   goto out;
>   }
>  
> + ret = vgic_register_its_iodevs(kvm);
> + if (ret) {
> + kvm_err("Unable to register VGIC ITS MMIO regions\n");
> + goto out;
> + }
> +
>   dist->ready = true;
>  
>  out:
> diff --git a/virt/kvm/arm/vgic/vgic.h

[PATCH 3/3] KVM: arm64: vgic-its: Make updates to propbaser/pendbaser atomic

2016-08-03 Thread Christoffer Dall

There are two problems with the current implementation of the MMIO
handlers for the propbaser and pendbaser:

First, the write to the value itself is not guaranteed to be an atomic
64-bit write so two concurrent writes to the structure field could be
intermixed.

Second, because we do a read-modify-update operation without any
synchronization, if we have two 32-bit accesses to separate parts of the
register, we can loose one of them.

We can take the KVM mutex to synchronize accesses to these registers.

Signed-off-by: Christoffer Dall 
---
 virt/kvm/arm/vgic/vgic-mmio-v3.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index ff668e0..e38b7a0 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -306,16 +306,19 @@ static void vgic_mmio_write_propbase(struct kvm_vcpu 
*vcpu,
 {
struct vgic_dist *dist = >kvm->arch.vgic;
struct vgic_cpu *vgic_cpu = >arch.vgic_cpu;
-   u64 propbaser = dist->propbaser;
+   u64 propbaser;
 
/* Storing a value with LPIs already enabled is undefined */
if (vgic_cpu->lpis_enabled)
return;
 
+   mutex_lock(>kvm->lock);
+   propbaser = dist->propbaser;
propbaser = update_64bit_reg(propbaser, addr & 4, len, val);
propbaser = vgic_sanitise_propbaser(propbaser);
 
dist->propbaser = propbaser;
+   mutex_unlock(>kvm->lock);
 }
 
 static unsigned long vgic_mmio_read_pendbase(struct kvm_vcpu *vcpu,
@@ -331,16 +334,19 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu 
*vcpu,
 unsigned long val)
 {
struct vgic_cpu *vgic_cpu = >arch.vgic_cpu;
-   u64 pendbaser = vgic_cpu->pendbaser;
+   u64 pendbaser;
 
/* Storing a value with LPIs already enabled is undefined */
if (vgic_cpu->lpis_enabled)
return;
 
+   mutex_lock(>kvm->lock);
+   pendbaser = vgic_cpu->pendbaser;
pendbaser = update_64bit_reg(pendbaser, addr & 4, len, val);
pendbaser = vgic_sanitise_pendbaser(pendbaser);
 
vgic_cpu->pendbaser = pendbaser;
+   mutex_unlock(>kvm->lock);
 }
 
 /*
-- 
2.9.0

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v2 1/3] KVM: arm64: vgic-its: Handle errors from vgic_add_lpi

2016-08-03 Thread Christoffer Dall

During low memory conditions, we could be dereferencing a NULL pointer
when vgic_add_lpi fails to allocate memory.

Consider for example this call sequence:

  vgic_its_cmd_handle_mapi
  itte->irq = vgic_add_lpi(kvm, lpi_nr);
  update_lpi_config(kvm, itte->irq, NULL);
  ret = kvm_read_guest(kvm, propbase + irq->intid
 
 kaboom?

Instead, return an error pointer from vgic_add_lpi and check the return
value from its single caller.

Signed-off-by: Christoffer Dall 
---
Changes since v1:
 - Don't errornously get an extra kref refernce for the struct vgic_irq
 - Don't rework the entire error handling of the function, but follow
   what Marc suggested he prefers based on his fixup patch.
 virt/kvm/arm/vgic/vgic-its.c | 18 ++
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 07411cf..424f7a5 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -51,7 +51,7 @@ static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 
intid)
 
irq = kzalloc(sizeof(struct vgic_irq), GFP_KERNEL);
if (!irq)
-   return NULL;
+   return ERR_PTR(-ENOMEM);
 
INIT_LIST_HEAD(>lpi_list);
INIT_LIST_HEAD(>ap_list);
@@ -693,10 +693,11 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, 
struct vgic_its *its,
u32 device_id = its_cmd_get_deviceid(its_cmd);
u32 event_id = its_cmd_get_id(its_cmd);
u32 coll_id = its_cmd_get_collection(its_cmd);
-   struct its_itte *itte;
+   struct its_itte *itte, *new_itte = NULL;
struct its_device *device;
struct its_collection *collection, *new_coll = NULL;
int lpi_nr;
+   struct vgic_irq *irq;
 
device = find_its_device(its, device_id);
if (!device)
@@ -720,7 +721,7 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct 
vgic_its *its,
 
itte = find_itte(its, device_id, event_id);
if (!itte) {
-   itte = kzalloc(sizeof(struct its_itte), GFP_KERNEL);
+   new_itte = itte = kzalloc(sizeof(struct its_itte), GFP_KERNEL);
if (!itte) {
if (new_coll)
vgic_its_free_collection(its, coll_id);
@@ -733,7 +734,16 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, 
struct vgic_its *its,
 
itte->collection = collection;
itte->lpi = lpi_nr;
-   itte->irq = vgic_add_lpi(kvm, lpi_nr);
+
+   irq = vgic_add_lpi(kvm, lpi_nr);
+   if (IS_ERR(irq)) {
+   if (new_coll)
+   vgic_its_free_collection(its, coll_id);
+   kfree(new_itte);
+   return PTR_ERR(irq);
+   }
+   itte->irq = irq;
+
update_affinity_itte(kvm, itte);
 
/*
-- 
2.9.0

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 0/3] KVM: arm64: vgic-its fixes

2016-08-03 Thread Christoffer Dall

Here are three small fixes resulting from looking at the pull request of
kvmarm/next into kvm/next.

They basically deal with corner cases in the reference count handling and
atomicity issues.

Christoffer Dall (3):
  KVM: arm64: vgic-its: Handle errors from vgic_add_lpi
  KVM: arm64: vgic-its: Plug race in vgic_put_irq
  KVM: arm64: vgic-its: Make updates to propbaser/pendbaser atomic

 virt/kvm/arm/vgic/vgic-its.c | 18 ++
 virt/kvm/arm/vgic/vgic-mmio-v3.c | 10 --
 virt/kvm/arm/vgic/vgic.c | 20 ++--
 3 files changed, 32 insertions(+), 16 deletions(-)

-- 
2.9.0

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 2/3] KVM: arm64: vgic-its: Plug race in vgic_put_irq

2016-08-03 Thread Christoffer Dall

Right now the following sequence of events can happen:

  1. Thread X calls vgic_put_irq
  2. Thread Y calls vgic_add_lpi
  3. Thread Y gets lpi_list_lock
  4. Thread X drops the ref count to 0 and blocks on lpi_list_lock
  5. Thread Y finds the irq via the lpi_list_lock, raises the ref
 count to 1, and release the lpi_list_lock.
  6. Thread X proceeds and frees the irq.

Avoid this by holding the spinlock around the kref_put.

Signed-off-by: Christoffer Dall 
---
 virt/kvm/arm/vgic/vgic.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
index e7aeac7..fb8c0ab 100644
--- a/virt/kvm/arm/vgic/vgic.c
+++ b/virt/kvm/arm/vgic/vgic.c
@@ -117,22 +117,22 @@ static void vgic_irq_release(struct kref *ref)
 
 void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq)
 {
-   struct vgic_dist *dist;
+   struct vgic_dist *dist = >arch.vgic;
 
if (irq->intid < VGIC_MIN_LPI)
return;
 
-   if (!kref_put(>refcount, vgic_irq_release))
-   return;
-
-   dist = >arch.vgic;
-
spin_lock(>lpi_list_lock);
-   list_del(>lpi_list);
-   dist->lpi_list_count--;
-   spin_unlock(>lpi_list_lock);
+   if (!kref_put(>refcount, vgic_irq_release)) {
+   spin_unlock(>lpi_list_lock);
+   return;
+   } else {
+   list_del(>lpi_list);
+   dist->lpi_list_count--;
+   spin_unlock(>lpi_list_lock);
 
-   kfree(irq);
+   kfree(irq);
+   }
 }
 
 /**
-- 
2.9.0

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH] KVM: arm64: ITS: move ITS registration into first VCPU run

2016-08-03 Thread Andre Przywara

Currently we register ITS devices upon userland issuing the CTRL_INIT
ioctl to mark initialization of the ITS as done.
This deviates from the initialization sequence of the existing GIC
devices and does not play well with the way QEMU handles things.
To be more in line with what we are used to, register the ITS(es) just
before the first VCPU is about to run, so in the map_resources() call.
This involves iterating through the list of KVM devices and handle each
ITS that we find.

Signed-off-by: Andre Przywara 
---
Hi,

this is based upon next-20160728 plus Christoffer's kvm_device locking
fix from today. Please let me know what tree I should base upon and I
will rebase.
Eric, Christoffer: does that do what you expect? Can QEMU live with that?

Cheers,
Andre.

 virt/kvm/arm/vgic/vgic-its.c | 56 
 virt/kvm/arm/vgic/vgic-v3.c  |  6 +
 virt/kvm/arm/vgic/vgic.h |  6 +
 3 files changed, 58 insertions(+), 10 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 07411cf..e677a60 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1288,13 +1288,13 @@ void vgic_enable_lpis(struct kvm_vcpu *vcpu)
its_sync_lpi_pending_table(vcpu);
 }
 
-static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its)
+static int vgic_register_its_iodev(struct kvm *kvm, struct vgic_its *its)
 {
struct vgic_io_device *iodev = >iodev;
int ret;
 
-   if (its->initialized)
-   return 0;
+   if (!its->initialized)
+   return -EBUSY;
 
if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base))
return -ENXIO;
@@ -1311,9 +1311,6 @@ static int vgic_its_init_its(struct kvm *kvm, struct 
vgic_its *its)
  KVM_VGIC_V3_ITS_SIZE, >dev);
mutex_unlock(>slots_lock);
 
-   if (!ret)
-   its->initialized = true;
-
return ret;
 }
 
@@ -1435,9 +1432,6 @@ static int vgic_its_set_attr(struct kvm_device *dev,
if (type != KVM_VGIC_ITS_ADDR_TYPE)
return -ENODEV;
 
-   if (its->initialized)
-   return -EBUSY;
-
if (copy_from_user(, uaddr, sizeof(addr)))
return -EFAULT;
 
@@ -1453,7 +1447,9 @@ static int vgic_its_set_attr(struct kvm_device *dev,
case KVM_DEV_ARM_VGIC_GRP_CTRL:
switch (attr->attr) {
case KVM_DEV_ARM_VGIC_CTRL_INIT:
-   return vgic_its_init_its(dev->kvm, its);
+   its->initialized = true;
+
+   return 0;
}
break;
}
@@ -1498,3 +1494,43 @@ int kvm_vgic_register_its_device(void)
return kvm_register_device_ops(_arm_vgic_its_ops,
   KVM_DEV_TYPE_ARM_VGIC_ITS);
 }
+
+/*
+ * Registers all ITSes with the kvm_io_bus framework.
+ * To follow the existing VGIC initialization sequence, this has to be
+ * done as late as possible, just before the first VCPU runs.
+ */
+int vgic_register_its_iodevs(struct kvm *kvm)
+{
+   struct kvm_device *dev;
+   int ret = 0;
+
+   mutex_lock(>devices_lock);
+
+   list_for_each_entry(dev, >devices, vm_node) {
+   if (dev->ops != _arm_vgic_its_ops)
+   continue;
+
+   ret = vgic_register_its_iodev(kvm, dev->private);
+   if (ret)
+   break;
+   }
+
+   if (ret) {
+   /* Iterate backwards to roll back previous registrations. */
+   for (dev = list_prev_entry(dev, vm_node);
+>vm_node != >devices;
+dev = list_prev_entry(dev, vm_node)) {
+   struct vgic_its *its = dev->private;
+
+   if (dev->ops != _arm_vgic_its_ops)
+   continue;
+
+   kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS,
+ >iodev.dev);
+   }
+   }
+
+   mutex_unlock(>devices_lock);
+   return ret;
+}
diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index 0506543..f0d9b2b 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -289,6 +289,12 @@ int vgic_v3_map_resources(struct kvm *kvm)
goto out;
}
 
+   ret = vgic_register_its_iodevs(kvm);
+   if (ret) {
+   kvm_err("Unable to register VGIC ITS MMIO regions\n");
+   goto out;
+   }
+
dist->ready = true;
 
 out:
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index 1d8e21d..6c4625c 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -84,6 +84,7 @@ void vgic_v3_enable(struct kvm_vcpu *vcpu);
 int vgic_v3_probe(const struct gic_kvm_info *info);
 int vgic_v3_map_resources(struct kvm

Re: [RFC PATCH v1 2/4] arm/arm64: vgic-new: Add distributor and redistributor access

2016-08-03 Thread Christoffer Dall

On Wed, Aug 03, 2016 at 02:03:39PM +0530, Vijay Kilari wrote:
> On Tue, Aug 2, 2016 at 8:13 PM, Christoffer Dall
>  wrote:
> > On Wed, Jul 20, 2016 at 06:32:26PM +0530, vijay.kil...@gmail.com wrote:
> >> From: Vijaya Kumar K 
> >>
> >> VGICv3 Distributor and Redistributor registers are accessed using
> >> KVM_DEV_ARM_VGIC_GRP_DIST_REGS and KVM_DEV_ARM_VGIC_GRP_DIST_REGS
> >> with KVM_SET_DEVICE_ATTR and KVM_GET_DEVICE_ATTR ioctls.
> >> These registers are accessed as 32-bit and cpu mpidr
> >> value passed along with register offset is used to identify the
> >> cpu for redistributor registers access.
> >>
> >> The draft version of VGIC v3 specification is define here
> >> https://lists.cs.columbia.edu/pipermail/kvmarm/2016-May/020355.html
> >>
> >> Signed-off-by: Vijaya Kumar K 
> >> ---
> >>  arch/arm64/include/uapi/asm/kvm.h   |3 +
> >>  virt/kvm/arm/vgic/vgic-kvm-device.c |   72 ++--
> >>  virt/kvm/arm/vgic/vgic-mmio-v3.c|  105 
> >> +++
> >>  virt/kvm/arm/vgic/vgic-mmio.c   |2 +-
> >>  virt/kvm/arm/vgic/vgic.h|8 +++
> >>  5 files changed, 183 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/arch/arm64/include/uapi/asm/kvm.h 
> >> b/arch/arm64/include/uapi/asm/kvm.h
> >> index f209ea1..a6b996e 100644
> >> --- a/arch/arm64/include/uapi/asm/kvm.h
> >> +++ b/arch/arm64/include/uapi/asm/kvm.h
> >> @@ -199,10 +199,13 @@ struct kvm_arch_memory_slot {
> >>  #define KVM_DEV_ARM_VGIC_GRP_CPU_REGS2
> >>  #define   KVM_DEV_ARM_VGIC_CPUID_SHIFT   32
> >>  #define   KVM_DEV_ARM_VGIC_CPUID_MASK(0xffULL << 
> >> KVM_DEV_ARM_VGIC_CPUID_SHIFT)
> >> +#define   KVM_DEV_ARM_VGIC_V3_CPUID_MASK \
> >> + (0xULL << 
> >> KVM_DEV_ARM_VGIC_CPUID_SHIFT)
> >>  #define   KVM_DEV_ARM_VGIC_OFFSET_SHIFT  0
> >>  #define   KVM_DEV_ARM_VGIC_OFFSET_MASK   (0xULL << 
> >> KVM_DEV_ARM_VGIC_OFFSET_SHIFT)
> >>  #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS 3
> >>  #define KVM_DEV_ARM_VGIC_GRP_CTRL4
> >> +#define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
> >>  #define   KVM_DEV_ARM_VGIC_CTRL_INIT 0
> >>
> >>  /* Device Control API on vcpu fd */
> >> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c 
> >> b/virt/kvm/arm/vgic/vgic-kvm-device.c
> >> index cace996..996a720 100644
> >> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
> >> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
> >> @@ -266,10 +266,17 @@ static int vgic_attr_regs_access(struct kvm_device 
> >> *dev,
> >>   int cpuid, ret, c;
> >>   struct kvm_vcpu *vcpu, *tmp_vcpu;
> >>   int vcpu_lock_idx = -1;
> >> + struct vgic_dist *vgic = >kvm->arch.vgic;
> >>
> >> - cpuid = (attr->attr & KVM_DEV_ARM_VGIC_CPUID_MASK) >>
> >> -  KVM_DEV_ARM_VGIC_CPUID_SHIFT;
> >> - vcpu = kvm_get_vcpu(dev->kvm, cpuid);
> >> + if (vgic->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2) {
> >> + cpuid = (attr->attr & KVM_DEV_ARM_VGIC_CPUID_MASK) >>
> >> +  KVM_DEV_ARM_VGIC_CPUID_SHIFT;
> >> + vcpu = kvm_get_vcpu(dev->kvm, cpuid);
> >> + } else {
> >> + cpuid = (attr->attr & KVM_DEV_ARM_VGIC_V3_CPUID_MASK) >>
> >> +  KVM_DEV_ARM_VGIC_CPUID_SHIFT;
> >> + vcpu = kvm_mpidr_to_vcpu(dev->kvm, cpuid);
> >> + }
> >>   addr = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
> >>
> >>   mutex_lock(>kvm->lock);
> >> @@ -301,7 +308,19 @@ static int vgic_attr_regs_access(struct kvm_device 
> >> *dev,
> >>   ret = vgic_v2_cpuif_uaccess(vcpu, is_write, addr, 
> >> >reg32);
> >>   break;
> >>   case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> >> - ret = vgic_v2_dist_uaccess(vcpu, is_write, addr, 
> >> >reg32);
> >> + if (vgic->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
> >> + ret = vgic_v2_dist_uaccess(vcpu, is_write, addr,
> >> +>reg32);
> >> + else
> >> + ret = vgic_v3_dist_uaccess(vcpu, is_write, addr,
> >> +>reg32);
> >> + break;
> >> + case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
> >> + if (vgic->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
> >> + ret = vgic_v3_redist_uaccess(vcpu, is_write, addr,
> >> +  >reg32);
> >> + else
> >> + ret = -EINVAL;
> >>   break;
> >>   default:
> >>   ret = -EINVAL;
> >> @@ -411,13 +430,51 @@ struct kvm_device_ops kvm_arm_vgic_v2_ops = {
> >>  static int vgic_v3_set_attr(struct kvm_device *dev,
> >>   struct kvm_device_attr *attr)
> >>  {
> >> - return vgic_set_common_attr(dev, attr);
> >> + int ret;
> >> +
> >> + ret = vgic_set_common_attr(dev, attr);
> >> + if (ret != -ENXIO)
> >>

Re: [RFC PATCH v1 2/4] arm/arm64: vgic-new: Add distributor and redistributor access

2016-08-03 Thread Vijay Kilari

On Tue, Aug 2, 2016 at 8:13 PM, Christoffer Dall
 wrote:
> On Wed, Jul 20, 2016 at 06:32:26PM +0530, vijay.kil...@gmail.com wrote:
>> From: Vijaya Kumar K 
>>
>> VGICv3 Distributor and Redistributor registers are accessed using
>> KVM_DEV_ARM_VGIC_GRP_DIST_REGS and KVM_DEV_ARM_VGIC_GRP_DIST_REGS
>> with KVM_SET_DEVICE_ATTR and KVM_GET_DEVICE_ATTR ioctls.
>> These registers are accessed as 32-bit and cpu mpidr
>> value passed along with register offset is used to identify the
>> cpu for redistributor registers access.
>>
>> The draft version of VGIC v3 specification is define here
>> https://lists.cs.columbia.edu/pipermail/kvmarm/2016-May/020355.html
>>
>> Signed-off-by: Vijaya Kumar K 
>> ---
>>  arch/arm64/include/uapi/asm/kvm.h   |3 +
>>  virt/kvm/arm/vgic/vgic-kvm-device.c |   72 ++--
>>  virt/kvm/arm/vgic/vgic-mmio-v3.c|  105 
>> +++
>>  virt/kvm/arm/vgic/vgic-mmio.c   |2 +-
>>  virt/kvm/arm/vgic/vgic.h|8 +++
>>  5 files changed, 183 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/arm64/include/uapi/asm/kvm.h 
>> b/arch/arm64/include/uapi/asm/kvm.h
>> index f209ea1..a6b996e 100644
>> --- a/arch/arm64/include/uapi/asm/kvm.h
>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>> @@ -199,10 +199,13 @@ struct kvm_arch_memory_slot {
>>  #define KVM_DEV_ARM_VGIC_GRP_CPU_REGS2
>>  #define   KVM_DEV_ARM_VGIC_CPUID_SHIFT   32
>>  #define   KVM_DEV_ARM_VGIC_CPUID_MASK(0xffULL << 
>> KVM_DEV_ARM_VGIC_CPUID_SHIFT)
>> +#define   KVM_DEV_ARM_VGIC_V3_CPUID_MASK \
>> + (0xULL << KVM_DEV_ARM_VGIC_CPUID_SHIFT)
>>  #define   KVM_DEV_ARM_VGIC_OFFSET_SHIFT  0
>>  #define   KVM_DEV_ARM_VGIC_OFFSET_MASK   (0xULL << 
>> KVM_DEV_ARM_VGIC_OFFSET_SHIFT)
>>  #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS 3
>>  #define KVM_DEV_ARM_VGIC_GRP_CTRL4
>> +#define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
>>  #define   KVM_DEV_ARM_VGIC_CTRL_INIT 0
>>
>>  /* Device Control API on vcpu fd */
>> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c 
>> b/virt/kvm/arm/vgic/vgic-kvm-device.c
>> index cace996..996a720 100644
>> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
>> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
>> @@ -266,10 +266,17 @@ static int vgic_attr_regs_access(struct kvm_device 
>> *dev,
>>   int cpuid, ret, c;
>>   struct kvm_vcpu *vcpu, *tmp_vcpu;
>>   int vcpu_lock_idx = -1;
>> + struct vgic_dist *vgic = >kvm->arch.vgic;
>>
>> - cpuid = (attr->attr & KVM_DEV_ARM_VGIC_CPUID_MASK) >>
>> -  KVM_DEV_ARM_VGIC_CPUID_SHIFT;
>> - vcpu = kvm_get_vcpu(dev->kvm, cpuid);
>> + if (vgic->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2) {
>> + cpuid = (attr->attr & KVM_DEV_ARM_VGIC_CPUID_MASK) >>
>> +  KVM_DEV_ARM_VGIC_CPUID_SHIFT;
>> + vcpu = kvm_get_vcpu(dev->kvm, cpuid);
>> + } else {
>> + cpuid = (attr->attr & KVM_DEV_ARM_VGIC_V3_CPUID_MASK) >>
>> +  KVM_DEV_ARM_VGIC_CPUID_SHIFT;
>> + vcpu = kvm_mpidr_to_vcpu(dev->kvm, cpuid);
>> + }
>>   addr = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
>>
>>   mutex_lock(>kvm->lock);
>> @@ -301,7 +308,19 @@ static int vgic_attr_regs_access(struct kvm_device *dev,
>>   ret = vgic_v2_cpuif_uaccess(vcpu, is_write, addr, >reg32);
>>   break;
>>   case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
>> - ret = vgic_v2_dist_uaccess(vcpu, is_write, addr, >reg32);
>> + if (vgic->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
>> + ret = vgic_v2_dist_uaccess(vcpu, is_write, addr,
>> +>reg32);
>> + else
>> + ret = vgic_v3_dist_uaccess(vcpu, is_write, addr,
>> +>reg32);
>> + break;
>> + case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
>> + if (vgic->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
>> + ret = vgic_v3_redist_uaccess(vcpu, is_write, addr,
>> +  >reg32);
>> + else
>> + ret = -EINVAL;
>>   break;
>>   default:
>>   ret = -EINVAL;
>> @@ -411,13 +430,51 @@ struct kvm_device_ops kvm_arm_vgic_v2_ops = {
>>  static int vgic_v3_set_attr(struct kvm_device *dev,
>>   struct kvm_device_attr *attr)
>>  {
>> - return vgic_set_common_attr(dev, attr);
>> + int ret;
>> +
>> + ret = vgic_set_common_attr(dev, attr);
>> + if (ret != -ENXIO)
>> + return ret;
>> +
>> + switch (attr->group) {
>> + case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
>> + case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS: {
>> + u32 __user *uaddr = (u32 __user *)(long)attr->addr;
>> + union ureg reg;
>> +
>> + if

Re: [PATCH] KVM: arm/arm64: initialize new VGIC even without host GIC

2016-08-03 Thread Stefan Agner

On 2016-08-02 07:34, Andre Przywara wrote:
> The new VGIC code is always in the VCPU entry/exit path, even when the
> actual GIC hardware initialization found the VGIC unusable (due to
> non-aligned base addresses or a missing maintenance interrupt, for
> instance).
> Since in this case the VGIC structures aren't initialized properly, the
> host kernel crashes on a NULL pointer dereference.
> Initialize each VCPU's ap_list even with the VGIC unavailable, so the
> VGIC code now just iterates an empty list in that case and no longer
> crashes the kernel.
> Also take care of the arch timer (which becomes unusable as well without
> a VGIC) by using a static key to prevent arch timer code to run in the
> hot path.
> Much of the code was inspired by Marc Zyngier.
> 
> Signed-off-by: Andre Przywara 
> Reported-by: Stefan Agner 
> ---
> Hi Stefan, Drew,
> 
> can you test this on your systems which showed the issue?

Looks that this catches the issue nicely too, the guest kernel boots
fine!

Tested-by: Stefan Agner 

--
Stefan


> 
> Cheers,
> Andre.
> 
>  arch/arm/kvm/arm.c| 14 ++
>  include/kvm/arm_arch_timer.h  |  2 ++
>  virt/kvm/arm/arch_timer.c | 11 +++
>  virt/kvm/arm/vgic/vgic-init.c |  9 -
>  4 files changed, 27 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index f1bde7c..cef35ce 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -294,7 +294,8 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
>   bitmap_zero(vcpu->arch.features, KVM_VCPU_MAX_FEATURES);
>  
>   /* Set up the timer */
> - kvm_timer_vcpu_init(vcpu);
> + if (vgic_present)
> + kvm_timer_vcpu_init(vcpu);
>  
>   kvm_arm_reset_debug_ptr(vcpu);
>  
> @@ -1208,6 +1209,7 @@ static int init_subsystems(void)
>   break;
>   case -ENODEV:
>   case -ENXIO:
> + kvm_err("No useable vgic detected\n");
>   vgic_present = false;
>   err = 0;
>   break;
> @@ -1218,9 +1220,13 @@ static int init_subsystems(void)
>   /*
>* Init HYP architected timer support
>*/
> - err = kvm_timer_hyp_init();
> - if (err)
> - goto out;
> + if (vgic_present) {
> + err = kvm_timer_hyp_init();
> + if (err)
> + goto out;
> + } else {
> + static_branch_enable(_arch_timer_disabled);
> + }
>  
>   kvm_perf_init();
>   kvm_coproc_table_init();
> diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> index dda39d8..64f361f 100644
> --- a/include/kvm/arm_arch_timer.h
> +++ b/include/kvm/arm_arch_timer.h
> @@ -23,6 +23,8 @@
>  #include 
>  #include 
>  
> +extern struct static_key_false kvm_arch_timer_disabled;
> +
>  struct arch_timer_kvm {
>   /* Virtual offset */
>   cycle_t cntvoff;
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 4fde8c7..71fb04c 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -34,6 +34,8 @@ static struct timecounter *timecounter;
>  static struct workqueue_struct *wqueue;
>  static unsigned int host_vtimer_irq;
>  
> +DEFINE_STATIC_KEY_FALSE(kvm_arch_timer_disabled);
> +
>  void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu)
>  {
>   vcpu->arch.timer_cpu.active_cleared_last = false;
> @@ -41,6 +43,9 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu)
>  
>  static cycle_t kvm_phys_timer_read(void)
>  {
> + if (static_branch_unlikely(_arch_timer_disabled))
> + return 0;
> +
>   return timecounter->cc->read(timecounter->cc);
>  }
>  
> @@ -104,6 +109,9 @@ static u64 kvm_timer_compute_delta(struct kvm_vcpu *vcpu)
>  {
>   cycle_t cval, now;
>  
> + if (static_branch_unlikely(_arch_timer_disabled))
> + return 0;
> +
>   cval = vcpu->arch.timer_cpu.cntv_cval;
>   now = kvm_phys_timer_read() - vcpu->kvm->arch.timer.cntvoff;
>  
> @@ -148,6 +156,9 @@ static bool kvm_timer_irq_can_fire(struct kvm_vcpu *vcpu)
>  {
>   struct arch_timer_cpu *timer = >arch.timer_cpu;
>  
> + if (static_branch_unlikely(_arch_timer_disabled))
> + return false;
> +
>   return !(timer->cntv_ctl & ARCH_TIMER_CTRL_IT_MASK) &&
>   (timer->cntv_ctl & ARCH_TIMER_CTRL_ENABLE);
>  }
> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
> index 2c7f0d5..b92133c 100644
> --- a/virt/kvm/arm/vgic/vgic-init.c
> +++ b/virt/kvm/arm/vgic/vgic-init.c
> @@ -56,6 +56,10 @@ void kvm_vgic_early_init(struct kvm *kvm)
>  
>  void kvm_vgic_vcpu_early_init(struct kvm_vcpu *vcpu)
>  {
> + struct vgic_cpu *vgic_cpu = >arch.vgic_cpu;
> +
> + INIT_LIST_HEAD(_cpu->ap_list_head);
> + spin_lock_init(_cpu->ap_list_lock);
>  }
>  
>  /* CREATION */
> @@ -392,11 +396,6 @@ int kvm_vgic_hyp_init(void)
>   if (!gic_kvm_info)
>

Re: [PATCH] KVM: arm64: ITS: move ITS registration into first VCPU run

Re: [PATCH] KVM: arm64: ITS: move ITS registration into first VCPU run

Re: [PATCH] KVM: arm64: ITS: move ITS registration into first VCPU run

Re: [PATCH] KVM: arm64: ITS: move ITS registration into first VCPU run

Re: [PATCH] KVM: arm64: ITS: move ITS registration into first VCPU run

Re: [PATCH] KVM: arm64: ITS: move ITS registration into first VCPU run

Re: [PATCH v2] KVM: arm64: ITS: return 1 on successful MSI injection

Re: [PATCH] KVM: arm64: ITS: move ITS registration into first VCPU run

Re: [PATCH] KVM: arm64: ITS: move ITS registration into first VCPU run

Re: [PATCH] KVM: arm64: ITS: move ITS registration into first VCPU run

[PATCH 3/3] KVM: arm64: vgic-its: Make updates to propbaser/pendbaser atomic

[PATCH v2 1/3] KVM: arm64: vgic-its: Handle errors from vgic_add_lpi

[PATCH 0/3] KVM: arm64: vgic-its fixes

[PATCH 2/3] KVM: arm64: vgic-its: Plug race in vgic_put_irq

[PATCH] KVM: arm64: ITS: move ITS registration into first VCPU run

Re: [RFC PATCH v1 2/4] arm/arm64: vgic-new: Add distributor and redistributor access

Re: [RFC PATCH v1 2/4] arm/arm64: vgic-new: Add distributor and redistributor access

Re: [PATCH] KVM: arm/arm64: initialize new VGIC even without host GIC

18 matches

Site Navigation

Mail list logo

Footer information