date:20170906

[PATCH] arm64: KVM: VHE: reset PSTATE.UAO when switch to host

2017-09-06 Thread Dongjiu Geng

In VHE mode, host kernel runs in the EL2 and can enable
'User Access Override' when fs==KERNEL_DS so that it can
access kernel memory. However, PSTATE.UAO is set to 0 on
an exception taken from EL1 to EL2. Thus when VHE is used
and exception taken from a guest UAO will be disabled and
host will use the incorrect PSTATE.UAO. So check and reset
the PSTATE.UAO when switching to host.

Move the reset PSTATE.PAN on entry to EL2 together with
PSTATE.UAO reset.

Signed-off-by: Dongjiu Geng 
Signed-off-by: Haibin Zhang 
Tested-by: Dongjiu Geng 
---
 arch/arm64/kvm/hyp/entry.S  |  2 --
 arch/arm64/kvm/hyp/switch.c | 12 
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
index 12ee62d..7662ef5 100644
--- a/arch/arm64/kvm/hyp/entry.S
+++ b/arch/arm64/kvm/hyp/entry.S
@@ -96,8 +96,6 @@ ENTRY(__guest_exit)
 
add x1, x1, #VCPU_CONTEXT
 
-   ALTERNATIVE(nop, SET_PSTATE_PAN(1), ARM64_HAS_PAN, CONFIG_ARM64_PAN)
-
// Store the guest regs x2 and x3
stp x2, x3,   [x1, #CPU_XREG_OFFSET(2)]
 
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index a733461..715b3941 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -22,6 +22,7 @@
 #include 
 #include 
 #include 
+#include 
 
 static bool __hyp_text __fpsimd_enabled_nvhe(void)
 {
@@ -399,6 +400,17 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
 
__sysreg_restore_host_state(host_ctxt);
 
+   if (has_vhe()) {
+   /*
+* PSTATE was not saved over guest enter/exit, re-enable
+* any detecte features that might not have been set
+* correctly.
+*/
+   uao_thread_switch(current);
+   asm(ALTERNATIVE("nop", SET_PSTATE_PAN(1),
+   ARM64_HAS_PAN, CONFIG_ARM64_PAN));
+   }
+
if (fp_enabled) {
__fpsimd_save_state(_ctxt->gp_regs.fp_regs);
__fpsimd_restore_state(_ctxt->gp_regs.fp_regs);
-- 
1.8.3.1

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

RE: [PATCH v2 19/28] arm64/sve: ptrace and ELF coredump support

2017-09-06 Thread Okamoto, Takayuki

Hi Dave,

Thank you for your reply.

> Your fix looks correct and seems to work.  For stylistic reasons, I may
> write it like this instead, but the effect should be the same:
> 
>   header->max_vl = sve_max_vl;
>   if (WARN_ON(!sve_vl_valid(sve_max_vl))
>   header->max_vl = header->vl;

It is better than my fix.
Please, apply it at next version.

Best regards,
Takayuki Okamoto

> -Original Message-
> From: linux-arm-kernel
> [mailto:linux-arm-kernel-boun...@lists.infradead.org] On Behalf Of Dave
> Martin
> Sent: Thursday, September 7, 2017 3:17 AM
> To: Okamoto, Takayuki 
> Cc: linux-a...@vger.kernel.org; libc-al...@sourceware.org; Ard
> Biesheuvel ; Szabolcs Nagy
> ; g...@sourceware.org; Yao Qi ;
> Alan Hayward ; Will Deacon ;
> Oleg Nesterov ; Richard Sandiford
> ; Alexander Viro ;
> Catalin Marinas ; Alex Bennée
> ; kvmarm@lists.cs.columbia.edu;
> linux-arm-ker...@lists.infradead.org
> Subject: Re: [PATCH v2 19/28] arm64/sve: ptrace and ELF coredump support
> 
> On Wed, Sep 06, 2017 at 04:21:50PM +, Okamoto, Takayuki wrote:
> > Hi Dave,
> >
> > I am an engineer of the postK computer from Fujitsu.
> >
> > When I tried to read "max_vl" by ptrace with this patch on our local SVE
> > simulator, it was read as zero.
> > I think the cause of this incident is that "max_vl" is set as "header->vl"
> > only on warning case in sve_init_header_from_task().
> > "max_vl" should be set up also on normal case, like the following patch.
> >
> >
> > --- a/arch/arm64/kernel/ptrace.c
> > +++ b/arch/arm64/kernel/ptrace.c
> > @@ -755,6 +755,8 @@ static void sve_init_header_from_task(struct
> user_sve_header *header,
> >
> > if (WARN_ON(!sve_vl_valid(sve_max_vl)))
> > header->max_vl = header->vl;
> > +   else
> > +   header->max_vl = sve_max_vl;
> >
> > header->size = SVE_PT_SIZE(vq, header->flags);
> > header->max_size =
> SVE_PT_SIZE(sve_vq_from_vl(header->max_vl),
> 
> Hi, thanks for reporting this.
> 
> It looks like a refactoring mistake I made while removing BUG_ON()s,
> which I missed in my testing.
> 
> Your fix looks correct and seems to work.  For stylistic reasons, I may
> write it like this instead, but the effect should be the same:
> 
>   header->max_vl = sve_max_vl;
>   if (WARN_ON(!sve_vl_valid(sve_max_vl))
>   header->max_vl = header->vl;
> 
> Cheers
> ---Dave
> 
> >
> >
> > Best regards,
> > Takayuki Okamoto
> >
> > -Original Message-
> > From: gdb-ow...@sourceware.org [mailto:gdb-ow...@sourceware.org] On
> Behalf Of Dave Martin
> > Sent: Friday, September 1, 2017 2:01 AM
> > To: linux-arm-ker...@lists.infradead.org
> > Cc: Catalin Marinas ; Will Deacon
> ; Ard Biesheuvel ; Alex
> Bennée ; Szabolcs Nagy ;
> Richard Sandiford ;
> kvmarm@lists.cs.columbia.edu; libc-al...@sourceware.org;
> linux-a...@vger.kernel.org; g...@sourceware.org; Alan Hayward
> ; Yao Qi ; Oleg Nesterov
> ; Alexander Viro 
> > Subject: [PATCH v2 19/28] arm64/sve: ptrace and ELF coredump support
> >
> 
> [...]
> 
> > @@ -702,6 +737,210 @@ static int system_call_set(struct task_struct
> *target,
> > return ret;
> >  }
> >
> > +#ifdef CONFIG_ARM64_SVE
> > +
> > +static void sve_init_header_from_task(struct user_sve_header *header,
> > + struct task_struct *target)
> > +{
> > +   unsigned int vq;
> > +
> > +   memset(header, 0, sizeof(*header));
> > +
> > +   header->flags = test_tsk_thread_flag(target, TIF_SVE) ?
> > +   SVE_PT_REGS_SVE : SVE_PT_REGS_FPSIMD;
> > +   if (test_tsk_thread_flag(target, TIF_SVE_VL_INHERIT))
> > +   header->flags |= SVE_PT_VL_INHERIT;
> > +
> > +   header->vl = target->thread.sve_vl;
> > +   vq = sve_vq_from_vl(header->vl);
> > +
> > +   if (WARN_ON(!sve_vl_valid(sve_max_vl)))
> > +   header->max_vl = header->vl;
> > +
> > +   header->size = SVE_PT_SIZE(vq, header->flags);
> > +   header->max_size = SVE_PT_SIZE(sve_vq_from_vl(header->max_vl),
> > + SVE_PT_REGS_SVE);
> > +}
> 
> [...]
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

Re: [RFC PATCH 2/3] kvm: arm/arm64: vgic-vits: free its resource when vm reboot/reset

2017-09-06 Thread wanghaibin

On 2017/9/7 0:20, Auger Eric wrote:

> Hi,
> 
> On 06/09/2017 15:05, wanghaibin wrote:
>> This patch fix the migrate save tables failure.
>>
>> When the virtual machine is in booting and the devices haven't initialized,
>> the all virtual dte/ite may be invalid. If migrate at this moment, the save
>> tables interface traversal device list, and check the dte is valid or not.
>> if not, it will return the -EINVAL.
> 
> The issue on save is less clear to me. We are not checking the "dte" are
> valid as it is said above. We are scrolling the ITS lists - which may be
> empty - and dump them in guest memory.
> 
> On save() there are quite few checks that can cause a failure.
> vgic_its_check_id() can be among them. This typically requires the
> GITS_BASER to have been properly set. Failing on save looks OK to me in
> such situation.
> 
> Sorry but I don't get the purpose of this patch. Does it fix a save failure?


Yes, for save, vgic_its_check_id() func will check the L1 DTE valid or not 
through
the code like :

/* Each 1st level entry is represented by a 64-bit value. */
if (kvm_read_guest(its->dev->kvm,
   BASER_ADDRESS(baser) + index * sizeof(indirect_ptr),
   _ptr, sizeof(indirect_ptr)))
return false;

indirect_ptr = le64_to_cpu(indirect_ptr);

/* check the valid bit of the first level entry */
if (!(indirect_ptr & BIT_ULL(63)))
return false;

If invalid , the save will return -EINVAL caused by the vgic_its_check_id() 
with return the false value.

And form the cover letter, the problem happened when no one pci dev has been 
probed( guest driver haven't any
mapd, mapti), So the L1 DTEs are all invalid currently. Just like you said, at 
this moment migrate, we are scrolling
the ITS lists, next time check_id failed and save interface failed.

I think the final reason is the device list free problem, at the reset/reboot, 
ITS dev/clo/itt lists are not be free
and set NULL. So that, the save interface failed.
This patch try to free the resource when vm reboot/reset.
BTW: these lists will re-bulid when the reboot vm run the probe pci device step.

Thanks

> 
> Thanks
> 
> Eric
> 
> 
>>
>> This patch try to free the its list resource when vm reboot or reset to 
>> avoid this.
>>
>> Signed-off-by: wanghaibin 
>> ---
>>  virt/kvm/arm/arm.c   |  5 -
>>  virt/kvm/arm/vgic/vgic-its.c | 10 ++
>>  virt/kvm/arm/vgic/vgic.h |  1 +
>>  3 files changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>> index a39a1e1..db7632d 100644
>> --- a/virt/kvm/arm/arm.c
>> +++ b/virt/kvm/arm/arm.c
>> @@ -46,6 +46,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include "vgic.h"
>>  
>>  #ifdef REQUIRES_VIRT
>>  __asm__(".arch_extensionvirt");
>> @@ -901,8 +902,10 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct 
>> kvm_vcpu *vcpu,
>>   * Ensure a rebooted VM will fault in RAM pages and detect if the
>>   * guest MMU is turned off and flush the caches as needed.
>>   */
>> -if (vcpu->arch.has_run_once)
>> +if (vcpu->arch.has_run_once) {
>>  stage2_unmap_vm(vcpu->kvm);
>> +vgic_its_free_resource(vcpu->kvm);
>> +}
>>  
>>  vcpu_reset_hcr(vcpu);
>>  
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> index 25d614f..5c20352 100644
>> --- a/virt/kvm/arm/vgic/vgic-its.c
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -2467,6 +2467,16 @@ static int vgic_its_get_attr(struct kvm_device *dev,
>>  .has_attr = vgic_its_has_attr,
>>  };
>>  
>> +void vgic_its_free_resource(struct kvm *kvm)
>> +{
>> +struct kvm_device *dev, *tmp;
>> +
>> +list_for_each_entry_safe(dev, tmp, >devices, vm_node) {
>> +if(dev->ops == _arm_vgic_its_ops)
>> +vgic_its_free_list(kvm, dev->private);
>> +}
>> +}
>> +
>>  int kvm_vgic_register_its_device(void)
>>  {
>>  return kvm_register_device_ops(_arm_vgic_its_ops,
>> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
>> index c2be5b7..fbcbdfd 100644
>> --- a/virt/kvm/arm/vgic/vgic.h
>> +++ b/virt/kvm/arm/vgic/vgic.h
>> @@ -222,5 +222,6 @@ int vgic_v3_line_level_info_uaccess(struct kvm_vcpu 
>> *vcpu, bool is_write,
>>  
>>  bool lock_all_vcpus(struct kvm *kvm);
>>  void unlock_all_vcpus(struct kvm *kvm);
>> +void vgic_its_free_resource(struct kvm *kvm);
>>  
>>  #endif
>>
> 
> .
> 



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

re: [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits

2017-09-06 Thread gengdongjiu

The negative effect is that kernel can not access kernel space adress through 
copy_to/from_user even KERNEL_DS is set
发件人：vladimir.murzin
收件人：耿东久,marc.zyngier,christoffer.dall,pbonzini,rkrcmar,linux-arm-kernel,kvmarm,kvm,linux-kernel,suzuki.poulose,mark.rutland,Catalin
 Marinas
抄送：James.Morse,张海斌,黄韶宇
时间：2017-09-06 23:19:38
主题:Re: [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits

On 06/09/17 16:08, gengdongjiu wrote:
> It is similar with the PAN，when the guest traps to el2，it will reset the 
> pstate.PAN  to 0, and continue run。In fact the host 
> pstate.UAO  can be 1, but guest change it to 0 when trap 
> to EL2。so after swich to host，need to check whether set pstate.UAO 
>  again。

What negative effect do you see with change UAO to 0 (i.e. do not override)?

P. S.
Please, avoid HTML and top posting

Vladimir

> *发件人：*Vladimir Murzin
> *收件人：*耿东久,marc.zyngier,christoffer.d...@linaro.org,pbonz...@redhat.com,rkrc...@redhat.com,linux-arm-ker...@lists.infradead.org,kvmarm@lists.cs.columbia.edu,k...@vger.kernel.org,linux-kernel,suzuki.poul...@arm.com,mark.rutl...@arm.com,Catalin
>  Marinas
> *抄送：*James Morse,张海斌,黄韶宇
> *时间：*2017-09-06 22:41:09
> *主题:*Re: [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits
>
> On 06/09/17 15:10, gengdongjiu wrote:
>> Hi, Vladimir
>>
> Do you see effect of "PAN is unexpectedly enabled"?
 In fact I did not encounter this case, but I think it can exist.
 I think if host OS dynamically disable PAN, it wants the host kernel 
 access the user space address space not through copy_to/from_user
>>> API.
 Now if it is unexpectedly enabled, when host kernel still accesses the 
 user space address,  it will happen MMU fault exception.
>>>
>>> And this is expected! The only allowed channel for kernel <-> user is 
>>> uaccess API.
>>>
>>> I guess that you have test (and that great!) which violates that rule (for 
>>> testing purpose, why not?) and now you are trying to fit kernel into
>>> it...
>>
>>
>> If you think that makes sense for it, we do not consider the paste.PAN in 
>> the world-switch.
>> For the pstate.UAO issue, do you agree my fixing or you have other 
>> suggestion?  Also to other reviewer. Thanks.
>
> It would help if you give precise description on "pstate.UAO issue".
>
> Thanks
> Vladimir
>
>>
>> diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
>> index 9341376..c3dd761 100644
>> --- a/arch/arm64/kvm/hyp/sysreg-sr.c
>> +++ b/arch/arm64/kvm/hyp/sysreg-sr.c
>> @@ -21,6 +21,8 @@
>>  #include 
>>  #include 
>>
>> +#include 
>>
>>  /* Yes, this does nothing, on purpose */
>>  static void __hyp_text __sysreg_do_nothing(struct kvm_cpu_context *ctxt) { }
>>
>> @@ -121,8 +123,13 @@ static void __hyp_text __sysreg_restore_state(struct 
>> kvm_cpu_context *ctxt)
>> write_sysreg_el1(ctxt->gp_regs.spsr[KVM_SPSR_EL1],spsr);
>>  }
>>
>> +static void __hyp_text __sysreg_restore_state_vhe(struct kvm_cpu_context 
>> *ctxt)
>> +{
>> +uao_thread_switch(current);
>> +}
>> +
>>  static hyp_alternate_select(__sysreg_call_restore_host_state,
>> -   __sysreg_restore_state, __sysreg_do_nothing,
>> +   __sysreg_restore_state, 
>> __sysreg_restore_state_vhe,
>> ARM64_HAS_VIRT_HOST_EXTN);
>>
>>  void __hyp_text __sysreg_restore_host_state(struct kvm_cpu_context *ctxt)
>>
>>
>>>
>>> Cheers
>>> Vladimir
>>>


>
> Cheers
> Vladimir
>
>>
>>>
>>> Cheers
>>> Vladimir
>>>
>>> .
>>>
>>
>>
>
>
> .
>


>>
>

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

Re: [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits

2017-09-06 Thread kbuild test robot

Hi gengdongjiu,

[auto build test ERROR on arm64/for-next/core]
[also build test ERROR on v4.13 next-20170906]
[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/gengdongjiu/arm64-KVM-VHE-save-and-restore-some-PSTATE-bits/20170907-013418
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git 
for-next/core
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/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 >>):

   /tmp/ccT6UpbP.s: Assembler messages:
>> /tmp/ccT6UpbP.s:1415: Error: constant expression required
   /tmp/ccT6UpbP.s:1439: Error: constant expression required
>> /tmp/ccT6UpbP.s:1420: Error: attempt to move .org backwards
   /tmp/ccT6UpbP.s:1444: Error: attempt to move .org backwards

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


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

Re: [PATCH v2 19/28] arm64/sve: ptrace and ELF coredump support

2017-09-06 Thread Dave Martin

On Wed, Sep 06, 2017 at 04:21:50PM +, Okamoto, Takayuki wrote:
> Hi Dave,
> 
> I am an engineer of the postK computer from Fujitsu.
> 
> When I tried to read "max_vl" by ptrace with this patch on our local SVE 
> simulator, it was read as zero.
> I think the cause of this incident is that "max_vl" is set as "header->vl" 
> only on warning case in sve_init_header_from_task().
> "max_vl" should be set up also on normal case, like the following patch.
> 
> 
> --- a/arch/arm64/kernel/ptrace.c
> +++ b/arch/arm64/kernel/ptrace.c
> @@ -755,6 +755,8 @@ static void sve_init_header_from_task(struct 
> user_sve_header *header,
> 
> if (WARN_ON(!sve_vl_valid(sve_max_vl)))
> header->max_vl = header->vl;
> +   else
> +   header->max_vl = sve_max_vl;
> 
> header->size = SVE_PT_SIZE(vq, header->flags);
> header->max_size = SVE_PT_SIZE(sve_vq_from_vl(header->max_vl),

Hi, thanks for reporting this.

It looks like a refactoring mistake I made while removing BUG_ON()s,
which I missed in my testing.

Your fix looks correct and seems to work.  For stylistic reasons, I may
write it like this instead, but the effect should be the same:

header->max_vl = sve_max_vl;
if (WARN_ON(!sve_vl_valid(sve_max_vl))
header->max_vl = header->vl;

Cheers
---Dave

> 
> 
> Best regards,
> Takayuki Okamoto
> 
> -Original Message-
> From: gdb-ow...@sourceware.org [mailto:gdb-ow...@sourceware.org] On Behalf Of 
> Dave Martin
> Sent: Friday, September 1, 2017 2:01 AM
> To: linux-arm-ker...@lists.infradead.org
> Cc: Catalin Marinas ; Will Deacon 
> ; Ard Biesheuvel ; Alex 
> Bennée ; Szabolcs Nagy ; 
> Richard Sandiford ; kvmarm@lists.cs.columbia.edu; 
> libc-al...@sourceware.org; linux-a...@vger.kernel.org; g...@sourceware.org; 
> Alan Hayward ; Yao Qi ; Oleg Nesterov 
> ; Alexander Viro 
> Subject: [PATCH v2 19/28] arm64/sve: ptrace and ELF coredump support
> 

[...]

> @@ -702,6 +737,210 @@ static int system_call_set(struct task_struct *target,
>   return ret;
>  }
>  
> +#ifdef CONFIG_ARM64_SVE
> +
> +static void sve_init_header_from_task(struct user_sve_header *header,
> +   struct task_struct *target)
> +{
> + unsigned int vq;
> +
> + memset(header, 0, sizeof(*header));
> +
> + header->flags = test_tsk_thread_flag(target, TIF_SVE) ?
> + SVE_PT_REGS_SVE : SVE_PT_REGS_FPSIMD;
> + if (test_tsk_thread_flag(target, TIF_SVE_VL_INHERIT))
> + header->flags |= SVE_PT_VL_INHERIT;
> +
> + header->vl = target->thread.sve_vl;
> + vq = sve_vq_from_vl(header->vl);
> +
> + if (WARN_ON(!sve_vl_valid(sve_max_vl)))
> + header->max_vl = header->vl;
> +
> + header->size = SVE_PT_SIZE(vq, header->flags);
> + header->max_size = SVE_PT_SIZE(sve_vq_from_vl(header->max_vl),
> +   SVE_PT_REGS_SVE);
> +}

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

RE: [PATCH v2 19/28] arm64/sve: ptrace and ELF coredump support

2017-09-06 Thread Okamoto, Takayuki

Hi Dave,

I am an engineer of the postK computer from Fujitsu.

When I tried to read "max_vl" by ptrace with this patch on our local SVE 
simulator, it was read as zero.
I think the cause of this incident is that "max_vl" is set as "header->vl" 
only on warning case in sve_init_header_from_task().
"max_vl" should be set up also on normal case, like the following patch.


--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -755,6 +755,8 @@ static void sve_init_header_from_task(struct 
user_sve_header *header,

if (WARN_ON(!sve_vl_valid(sve_max_vl)))
header->max_vl = header->vl;
+   else
+   header->max_vl = sve_max_vl;

header->size = SVE_PT_SIZE(vq, header->flags);
header->max_size = SVE_PT_SIZE(sve_vq_from_vl(header->max_vl),


Best regards,
Takayuki Okamoto

-Original Message-
From: gdb-ow...@sourceware.org [mailto:gdb-ow...@sourceware.org] On Behalf Of 
Dave Martin
Sent: Friday, September 1, 2017 2:01 AM
To: linux-arm-ker...@lists.infradead.org
Cc: Catalin Marinas ; Will Deacon 
; Ard Biesheuvel ; Alex Bennée 
; Szabolcs Nagy ; Richard 
Sandiford ; kvmarm@lists.cs.columbia.edu; 
libc-al...@sourceware.org; linux-a...@vger.kernel.org; g...@sourceware.org; 
Alan Hayward ; Yao Qi ; Oleg Nesterov 
; Alexander Viro 
Subject: [PATCH v2 19/28] arm64/sve: ptrace and ELF coredump support

This patch defines and implements a new regset NT_ARM_SVE, which
describes a thread's SVE register state.  This allows a debugger to
manipulate the SVE state, as well as being included in ELF
coredumps for post-mortem debugging.

Because the regset size and layout are dependent on the thread's
current vector length, it is not possible to define a C struct to
describe the regset contents as is done for existing regsets.
Instead, and for the same reasons, NT_ARM_SVE is based on the
freeform variable-layout approach used for the SVE signal frame.

Additionally, to reduce debug overhead when debugging threads that
might or might not have live SVE register state, NT_ARM_SVE may be
presented in one of two different formats: the old struct
user_fpsimd_state format is embedded for describing the state of a
thread with no live SVE state, whereas a new variable-layout
structure is embedded for describing live SVE state.  This avoids a
debugger needing to poll NT_PRFPREG in addition to NT_ARM_SVE, and
allows existing userspace code to handle the non-SVE case without
too much modification.

For this to work, NT_ARM_SVE is defined with a fixed-format header
of type struct user_sve_header, which the recipient can use to
figure out the content, size and layout of the reset of the regset.
Accessor macros are defined to allow the vector-length-dependent
parts of the regset to be manipulated.

Signed-off-by: Alan Hayward 
Signed-off-by: Dave Martin 
Cc: Alex Bennée 

---

Changes since v1


Other changes related to Alex Bennée's comments:

* Migrate to SVE_VQ_BYTES instead of magic numbers.

Requested by Alex Bennée:

* Thin out BUG_ON()s:
Redundant BUG_ON()s and ones that just check invariants are removed.
Important sanity-checks are migrated to WARN_ON()s, with some
minimal best-effort patch-up code.

Other:

* [ABI fix] Bail out with -EIO if attempting to set the
SVE regs for an unsupported VL, instead of misparsing the regset data.

* Replace some in-kernel open-coded arithmetic with ALIGN()/
DIV_ROUND_UP().
---
 arch/arm64/include/asm/fpsimd.h  |  13 +-
 arch/arm64/include/uapi/asm/ptrace.h | 135 ++
 arch/arm64/kernel/fpsimd.c   |  40 +-
 arch/arm64/kernel/ptrace.c   | 270 +--
 include/uapi/linux/elf.h |   1 +
 5 files changed, 449 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
index 6c22624..2723cca 100644
--- a/arch/arm64/include/asm/fpsimd.h
+++ b/arch/arm64/include/asm/fpsimd.h
@@ -38,13 +38,16 @@ struct fpsimd_state {
__uint128_t vregs[32];
u32 fpsr;
u32 fpcr;
+   /*
+* For ptrace compatibility, pad to next 128-bit
+* boundary here if extending this struct.
+*/
};
};
/* the id of the last cpu to have restored this state */
unsigned int cpu;
 };
 
-
 #if defined(__KERNEL__) && defined(CONFIG_COMPAT)
 /* Masks for extracting the FPSR and FPCR from the FPSCR */
 #define VFP_FPSCR_STAT_MASK0xf89f
@@ -89,6 +92,10 @@ extern void sve_alloc(struct task_struct *task);
 extern void

Re: [RFC PATCH 2/3] kvm: arm/arm64: vgic-vits: free its resource when vm reboot/reset

2017-09-06 Thread Auger Eric

Hi,

On 06/09/2017 15:05, wanghaibin wrote:
> This patch fix the migrate save tables failure.
> 
> When the virtual machine is in booting and the devices haven't initialized,
> the all virtual dte/ite may be invalid. If migrate at this moment, the save
> tables interface traversal device list, and check the dte is valid or not.
> if not, it will return the -EINVAL.

The issue on save is less clear to me. We are not checking the "dte" are
valid as it is said above. We are scrolling the ITS lists - which may be
empty - and dump them in guest memory.

On save() there are quite few checks that can cause a failure.
vgic_its_check_id() can be among them. This typically requires the
GITS_BASER to have been properly set. Failing on save looks OK to me in
such situation.

Sorry but I don't get the purpose of this patch. Does it fix a save failure?

Thanks

Eric


> 
> This patch try to free the its list resource when vm reboot or reset to avoid 
> this.
> 
> Signed-off-by: wanghaibin 
> ---
>  virt/kvm/arm/arm.c   |  5 -
>  virt/kvm/arm/vgic/vgic-its.c | 10 ++
>  virt/kvm/arm/vgic/vgic.h |  1 +
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index a39a1e1..db7632d 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -46,6 +46,7 @@
>  #include 
>  #include 
>  #include 
> +#include "vgic.h"
>  
>  #ifdef REQUIRES_VIRT
>  __asm__(".arch_extension virt");
> @@ -901,8 +902,10 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu 
> *vcpu,
>* Ensure a rebooted VM will fault in RAM pages and detect if the
>* guest MMU is turned off and flush the caches as needed.
>*/
> - if (vcpu->arch.has_run_once)
> + if (vcpu->arch.has_run_once) {
>   stage2_unmap_vm(vcpu->kvm);
> + vgic_its_free_resource(vcpu->kvm);
> + }
>  
>   vcpu_reset_hcr(vcpu);
>  
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 25d614f..5c20352 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -2467,6 +2467,16 @@ static int vgic_its_get_attr(struct kvm_device *dev,
>   .has_attr = vgic_its_has_attr,
>  };
>  
> +void vgic_its_free_resource(struct kvm *kvm)
> +{
> + struct kvm_device *dev, *tmp;
> +
> + list_for_each_entry_safe(dev, tmp, >devices, vm_node) {
> + if(dev->ops == _arm_vgic_its_ops)
> + vgic_its_free_list(kvm, dev->private);
> + }
> +}
> +
>  int kvm_vgic_register_its_device(void)
>  {
>   return kvm_register_device_ops(_arm_vgic_its_ops,
> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> index c2be5b7..fbcbdfd 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -222,5 +222,6 @@ int vgic_v3_line_level_info_uaccess(struct kvm_vcpu 
> *vcpu, bool is_write,
>  
>  bool lock_all_vcpus(struct kvm *kvm);
>  void unlock_all_vcpus(struct kvm *kvm);
> +void vgic_its_free_resource(struct kvm *kvm);
>  
>  #endif
> 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits

2017-09-06 Thread Vladimir Murzin

On 06/09/17 16:08, gengdongjiu wrote:
> It is similar with the PAN，when the guest traps to el2，it will reset the 
> pstate.PAN  to 0, and continue run。In fact the host 
> pstate.UAO  can be 1, but guest change it to 0 when trap 
> to EL2。so after swich to host，need to check whether set pstate.UAO 
>  again。

What negative effect do you see with change UAO to 0 (i.e. do not override)?

P. S.
Please, avoid HTML and top posting

Vladimir

> *发件人：*Vladimir Murzin
> *收件人：*耿东久,marc.zyngier,christoffer.d...@linaro.org,pbonz...@redhat.com,rkrc...@redhat.com,linux-arm-ker...@lists.infradead.org,kvmarm@lists.cs.columbia.edu,k...@vger.kernel.org,linux-kernel,suzuki.poul...@arm.com,mark.rutl...@arm.com,Catalin
>  Marinas
> *抄送：*James Morse,张海斌,黄韶宇
> *时间：*2017-09-06 22:41:09
> *主题:*Re: [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits
> 
> On 06/09/17 15:10, gengdongjiu wrote:
>> Hi, Vladimir
>> 
> Do you see effect of "PAN is unexpectedly enabled"?
 In fact I did not encounter this case, but I think it can exist.
 I think if host OS dynamically disable PAN, it wants the host kernel 
 access the user space address space not through copy_to/from_user
>>> API.
 Now if it is unexpectedly enabled, when host kernel still accesses the 
 user space address,  it will happen MMU fault exception.
>>>
>>> And this is expected! The only allowed channel for kernel <-> user is 
>>> uaccess API.
>>>
>>> I guess that you have test (and that great!) which violates that rule (for 
>>> testing purpose, why not?) and now you are trying to fit kernel into
>>> it...
>> 
>> 
>> If you think that makes sense for it, we do not consider the paste.PAN in 
>> the world-switch.
>> For the pstate.UAO issue, do you agree my fixing or you have other 
>> suggestion?  Also to other reviewer. Thanks.
> 
> It would help if you give precise description on "pstate.UAO issue".
> 
> Thanks
> Vladimir
> 
>> 
>> diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
>> index 9341376..c3dd761 100644
>> --- a/arch/arm64/kvm/hyp/sysreg-sr.c
>> +++ b/arch/arm64/kvm/hyp/sysreg-sr.c
>> @@ -21,6 +21,8 @@
>>  #include 
>>  #include 
>> 
>> +#include 
>> 
>>  /* Yes, this does nothing, on purpose */
>>  static void __hyp_text __sysreg_do_nothing(struct kvm_cpu_context *ctxt) { }
>> 
>> @@ -121,8 +123,13 @@ static void __hyp_text __sysreg_restore_state(struct 
>> kvm_cpu_context *ctxt)
>> write_sysreg_el1(ctxt->gp_regs.spsr[KVM_SPSR_EL1],spsr);
>>  }
>> 
>> +static void __hyp_text __sysreg_restore_state_vhe(struct kvm_cpu_context 
>> *ctxt)
>> +{
>> +uao_thread_switch(current);
>> +}
>> +
>>  static hyp_alternate_select(__sysreg_call_restore_host_state,
>> -   __sysreg_restore_state, __sysreg_do_nothing,
>> +   __sysreg_restore_state, 
>> __sysreg_restore_state_vhe,
>> ARM64_HAS_VIRT_HOST_EXTN);
>> 
>>  void __hyp_text __sysreg_restore_host_state(struct kvm_cpu_context *ctxt)
>> 
>> 
>>>
>>> Cheers
>>> Vladimir
>>>


>
> Cheers
> Vladimir
>
>>
>>>
>>> Cheers
>>> Vladimir
>>>
>>> .
>>>
>>
>>
>
>
> .
>


>> 
> 

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

Re: [RFC PATCH 3/3] kvm: arm/arm64: vgic-its: fix return value for restore

2017-09-06 Thread Auger Eric

Hi Wanghaibin,

On 06/09/2017 15:05, wanghaibin wrote:
> This patch fix the migrate restore tables failure.
> 
> The same scene, at the destination, the restore tables interface traversal 
> guest
> memory, and check the dte/ite is valid or not.
> If all dtes/ites are invalid, we will do try next one, and the last it will 
> take
> the 1 return value, but currently, it be treated as error. That's not correct.
There's indeed a bug here! In case all entries are invalid we shouldn't
return an error.

One solution could be to relax the error checking in scan_its_table()
and do not return 1 when the whole length has been scanned. This would
fix your issue.

drawback of that change:
at the moment we check the consistency of the entry data (next offset
field). At the moment if the next_offset points to an entry outside of
the table scope we are able to return an error (for top level tables).

Otherwise, if we want to keep that check, I think we would need to add a
bool *valid parameter to entry_fn_t. in scan_its_table() we would return
1 only if last fn() call returns valid and len <= 0.

Thanks

Eric

> 
> This patch try to fix this problem.
> 
> Signed-off-by: wanghaibin 
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 5c20352..2c69aeb 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -2025,7 +2025,7 @@ static int vgic_its_restore_dte(struct vgic_its *its, 
> u32 id,
>   return PTR_ERR(dev);
>  
>   ret = vgic_its_restore_itt(its, dev);
> - if (ret) {
> + if (ret < 0) {
>   vgic_its_free_device(its->dev->kvm, dev);
>   return ret;
>   }
> @@ -2147,9 +2147,6 @@ static int vgic_its_restore_device_tables(struct 
> vgic_its *its)
>vgic_its_restore_dte, NULL);
>   }
>  
> - if (ret > 0)
> - ret = -EINVAL;
> -
>   return ret;
>  }
>  
> 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

答复:re: [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits

2017-09-06 Thread gengdongjiu


Fix a typo.

It is similar with the PAN，when the guest traps to el2，it will reset the 
pstate.UAO to 0, and continue run。In fact the host 
pstate.UAO can be 1, but guest change it to 0 when trap to 
EL2, so after swich to host，need to check whether set 
pstate.UAO again。

发件人：Vladimir Murzin
收件人：耿东久,marc.zyngier,christoffer.d...@linaro.org,pbonz...@redhat.com,rkrc...@redhat.com,linux-arm-ker...@lists.infradead.org,kvmarm@lists.cs.columbia.edu,k...@vger.kernel.org,linux-kernel,suzuki.poul...@arm.com,mark.rutl...@arm.com,Catalin
 Marinas
抄送：James Morse,张海斌,黄韶宇
时间：2017-09-06 22:41:09
主题:Re: [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits

On 06/09/17 15:10, gengdongjiu wrote:
> Hi, Vladimir
>
 Do you see effect of "PAN is unexpectedly enabled"?
>>> In fact I did not encounter this case, but I think it can exist.
>>> I think if host OS dynamically disable PAN, it wants the host kernel access 
>>> the user space address space not through copy_to/from_user
>> API.
>>> Now if it is unexpectedly enabled, when host kernel still accesses the user 
>>> space address,  it will happen MMU fault exception.
>>
>> And this is expected! The only allowed channel for kernel <-> user is 
>> uaccess API.
>>
>> I guess that you have test (and that great!) which violates that rule (for 
>> testing purpose, why not?) and now you are trying to fit kernel into
>> it...
>
>
> If you think that makes sense for it, we do not consider the paste.PAN in the 
> world-switch.
> For the pstate.UAO issue, do you agree my fixing or you have other 
> suggestion?  Also to other reviewer. Thanks.

It would help if you give precise description on "pstate.UAO issue".

Thanks
Vladimir

>
> diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
> index 9341376..c3dd761 100644
> --- a/arch/arm64/kvm/hyp/sysreg-sr.c
> +++ b/arch/arm64/kvm/hyp/sysreg-sr.c
> @@ -21,6 +21,8 @@
>  #include 
>  #include 
>
> +#include 
>
>  /* Yes, this does nothing, on purpose */
>  static void __hyp_text __sysreg_do_nothing(struct kvm_cpu_context *ctxt) { }
>
> @@ -121,8 +123,13 @@ static void __hyp_text __sysreg_restore_state(struct 
> kvm_cpu_context *ctxt)
> write_sysreg_el1(ctxt->gp_regs.spsr[KVM_SPSR_EL1],spsr);
>  }
>
> +static void __hyp_text __sysreg_restore_state_vhe(struct kvm_cpu_context 
> *ctxt)
> +{
> +uao_thread_switch(current);
> +}
> +
>  static hyp_alternate_select(__sysreg_call_restore_host_state,
> -   __sysreg_restore_state, __sysreg_do_nothing,
> +   __sysreg_restore_state, 
> __sysreg_restore_state_vhe,
> ARM64_HAS_VIRT_HOST_EXTN);
>
>  void __hyp_text __sysreg_restore_host_state(struct kvm_cpu_context *ctxt)
>
>
>>
>> Cheers
>> Vladimir
>>
>>>
>>>

 Cheers
 Vladimir

>
>>
>> Cheers
>> Vladimir
>>
>> .
>>
>
>


 .

>>>
>>>
>

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

re: [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits

2017-09-06 Thread gengdongjiu

It is similar with the PAN，when the guest traps to el2，it will reset the 
pstate.PAN to 0, and continue run。In fact the host 
pstate.UAO can be 1, but guest change it to 0 when trap to 
EL2。so after swich to host，need to check whether set 
pstate.UAO again。
发件人：Vladimir Murzin
收件人：耿东久,marc.zyngier,christoffer.d...@linaro.org,pbonz...@redhat.com,rkrc...@redhat.com,linux-arm-ker...@lists.infradead.org,kvmarm@lists.cs.columbia.edu,k...@vger.kernel.org,linux-kernel,suzuki.poul...@arm.com,mark.rutl...@arm.com,Catalin
 Marinas
抄送：James Morse,张海斌,黄韶宇
时间：2017-09-06 22:41:09
主题:Re: [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits

On 06/09/17 15:10, gengdongjiu wrote:
> Hi, Vladimir
>
 Do you see effect of "PAN is unexpectedly enabled"?
>>> In fact I did not encounter this case, but I think it can exist.
>>> I think if host OS dynamically disable PAN, it wants the host kernel access 
>>> the user space address space not through copy_to/from_user
>> API.
>>> Now if it is unexpectedly enabled, when host kernel still accesses the user 
>>> space address,  it will happen MMU fault exception.
>>
>> And this is expected! The only allowed channel for kernel <-> user is 
>> uaccess API.
>>
>> I guess that you have test (and that great!) which violates that rule (for 
>> testing purpose, why not?) and now you are trying to fit kernel into
>> it...
>
>
> If you think that makes sense for it, we do not consider the paste.PAN in the 
> world-switch.
> For the pstate.UAO issue, do you agree my fixing or you have other 
> suggestion?  Also to other reviewer. Thanks.

It would help if you give precise description on "pstate.UAO issue".

Thanks
Vladimir

>
> diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
> index 9341376..c3dd761 100644
> --- a/arch/arm64/kvm/hyp/sysreg-sr.c
> +++ b/arch/arm64/kvm/hyp/sysreg-sr.c
> @@ -21,6 +21,8 @@
>  #include 
>  #include 
>
> +#include 
>
>  /* Yes, this does nothing, on purpose */
>  static void __hyp_text __sysreg_do_nothing(struct kvm_cpu_context *ctxt) { }
>
> @@ -121,8 +123,13 @@ static void __hyp_text __sysreg_restore_state(struct 
> kvm_cpu_context *ctxt)
> write_sysreg_el1(ctxt->gp_regs.spsr[KVM_SPSR_EL1],spsr);
>  }
>
> +static void __hyp_text __sysreg_restore_state_vhe(struct kvm_cpu_context 
> *ctxt)
> +{
> +uao_thread_switch(current);
> +}
> +
>  static hyp_alternate_select(__sysreg_call_restore_host_state,
> -   __sysreg_restore_state, __sysreg_do_nothing,
> +   __sysreg_restore_state, 
> __sysreg_restore_state_vhe,
> ARM64_HAS_VIRT_HOST_EXTN);
>
>  void __hyp_text __sysreg_restore_host_state(struct kvm_cpu_context *ctxt)
>
>
>>
>> Cheers
>> Vladimir
>>
>>>
>>>

 Cheers
 Vladimir

>
>>
>> Cheers
>> Vladimir
>>
>> .
>>
>
>


 .

>>>
>>>
>

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

Re: [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits

2017-09-06 Thread Vladimir Murzin

On 06/09/17 15:10, gengdongjiu wrote:
> Hi, Vladimir
> 
 Do you see effect of "PAN is unexpectedly enabled"?
>>> In fact I did not encounter this case, but I think it can exist.
>>> I think if host OS dynamically disable PAN, it wants the host kernel access 
>>> the user space address space not through copy_to/from_user
>> API.
>>> Now if it is unexpectedly enabled, when host kernel still accesses the user 
>>> space address,  it will happen MMU fault exception.
>>
>> And this is expected! The only allowed channel for kernel <-> user is 
>> uaccess API.
>>
>> I guess that you have test (and that great!) which violates that rule (for 
>> testing purpose, why not?) and now you are trying to fit kernel into
>> it...
> 
> 
> If you think that makes sense for it, we do not consider the paste.PAN in the 
> world-switch.
> For the pstate.UAO issue, do you agree my fixing or you have other 
> suggestion?  Also to other reviewer. Thanks.

It would help if you give precise description on "pstate.UAO issue".

Thanks
Vladimir

> 
> diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
> index 9341376..c3dd761 100644
> --- a/arch/arm64/kvm/hyp/sysreg-sr.c
> +++ b/arch/arm64/kvm/hyp/sysreg-sr.c
> @@ -21,6 +21,8 @@
>  #include 
>  #include 
> 
> +#include 
> 
>  /* Yes, this does nothing, on purpose */
>  static void __hyp_text __sysreg_do_nothing(struct kvm_cpu_context *ctxt) { }
> 
> @@ -121,8 +123,13 @@ static void __hyp_text __sysreg_restore_state(struct 
> kvm_cpu_context *ctxt)
> write_sysreg_el1(ctxt->gp_regs.spsr[KVM_SPSR_EL1],spsr);
>  }
> 
> +static void __hyp_text __sysreg_restore_state_vhe(struct kvm_cpu_context 
> *ctxt)
> +{
> +uao_thread_switch(current);
> +}
> +
>  static hyp_alternate_select(__sysreg_call_restore_host_state,
> -   __sysreg_restore_state, __sysreg_do_nothing,
> +   __sysreg_restore_state, 
> __sysreg_restore_state_vhe,
> ARM64_HAS_VIRT_HOST_EXTN);
> 
>  void __hyp_text __sysreg_restore_host_state(struct kvm_cpu_context *ctxt)
> 
> 
>>
>> Cheers
>> Vladimir
>>
>>>
>>>

 Cheers
 Vladimir

>
>>
>> Cheers
>> Vladimir
>>
>> .
>>
>
>


 .

>>>
>>>
> 

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

Re: [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits

2017-09-06 Thread gengdongjiu

Hi, Vladimir

> >> Do you see effect of "PAN is unexpectedly enabled"?
> > In fact I did not encounter this case, but I think it can exist.
> > I think if host OS dynamically disable PAN, it wants the host kernel access 
> > the user space address space not through copy_to/from_user
> API.
> > Now if it is unexpectedly enabled, when host kernel still accesses the user 
> > space address,  it will happen MMU fault exception.
> 
> And this is expected! The only allowed channel for kernel <-> user is uaccess 
> API.
> 
> I guess that you have test (and that great!) which violates that rule (for 
> testing purpose, why not?) and now you are trying to fit kernel into
> it...


If you think that makes sense for it, we do not consider the paste.PAN in the 
world-switch.
For the pstate.UAO issue, do you agree my fixing or you have other suggestion?  
Also to other reviewer. Thanks.

diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
index 9341376..c3dd761 100644
--- a/arch/arm64/kvm/hyp/sysreg-sr.c
+++ b/arch/arm64/kvm/hyp/sysreg-sr.c
@@ -21,6 +21,8 @@
 #include 
 #include 

+#include 

 /* Yes, this does nothing, on purpose */
 static void __hyp_text __sysreg_do_nothing(struct kvm_cpu_context *ctxt) { }

@@ -121,8 +123,13 @@ static void __hyp_text __sysreg_restore_state(struct 
kvm_cpu_context *ctxt)
write_sysreg_el1(ctxt->gp_regs.spsr[KVM_SPSR_EL1],spsr);
 }

+static void __hyp_text __sysreg_restore_state_vhe(struct kvm_cpu_context *ctxt)
+{
+uao_thread_switch(current);
+}
+
 static hyp_alternate_select(__sysreg_call_restore_host_state,
-   __sysreg_restore_state, __sysreg_do_nothing,
+   __sysreg_restore_state, __sysreg_restore_state_vhe,
ARM64_HAS_VIRT_HOST_EXTN);

 void __hyp_text __sysreg_restore_host_state(struct kvm_cpu_context *ctxt)


> 
> Cheers
> Vladimir
> 
> >
> >
> >>
> >> Cheers
> >> Vladimir
> >>
> >>>
> 
>  Cheers
>  Vladimir
> 
>  .
> 
> >>>
> >>>
> >>
> >>
> >> .
> >>
> >
> >

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

Re: [PATCH] KVM: arm64: vgic-its: Fix wrong return value check in vgic_its_restore_device_tables

2017-09-06 Thread wanghaibin

On 2017/9/6 15:22, Auger Eric wrote:

> Hi Vijaya,
> 
> On 06/09/2017 07:26, Vijaya Kumar K wrote:
>> scan_its_table() return 1 on success.
> 
> As mentioned in the kernel-doc comment of scan_its_table, this latter
> returns 1 if the last element is not found. Than can happen while
> scanning an L2 table but shouldn't happen if we scan an L1 table.
> 
>  * Return: < 0 on error, 0 if last element was identified, 1 otherwise
>  * (the last element may not be found on second level tables)
> 

I think there maybe something wrong.
A weeks ago, I met the migrate failed both the save interface and restore 
interface.
The restore failed reason is that vm is in booting, and haven't to probe pci 
devs at that moment.
and all dtes is invalid at that moment.

In this case, the interface will return -EINVAL. I think we should take this as 
success.

Not only for dtes, but the ites.

Thanks.


> 
>  In the function vgic_its_restore_device_tables()
>> the return value of scan_its_table() is checked against success value
>> and returns -EINVAL. Hence migration fails for VM with ITS.
>>
>> With this patch the failure return value is checked while returning
>> -EINVAL.
>>
>> Signed-off-by: Vijaya Kumar K 
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> index aa6b68d..63f8ac3 100644
>> --- a/virt/kvm/arm/vgic/vgic-its.c
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -2142,7 +2142,7 @@ static int vgic_its_restore_device_tables(struct 
>> vgic_its *its)
>>   vgic_its_restore_dte, NULL);
>>  }
>>  
>> -if (ret > 0)
>> +if (ret <= 0)
>>  ret = -EINVAL;
> your modification would return -EINVAL for whatever error encountered
> during the scan table or if last element is found. I don't think this is
> what we want.
> 
> Thanks
> 
> Eric
>>  
>>  return ret;
>>
> ___
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> 
> 



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

[RFC PATCH 2/3] kvm: arm/arm64: vgic-vits: free its resource when vm reboot/reset

2017-09-06 Thread wanghaibin

This patch fix the migrate save tables failure.

When the virtual machine is in booting and the devices haven't initialized,
the all virtual dte/ite may be invalid. If migrate at this moment, the save
tables interface traversal device list, and check the dte is valid or not.
if not, it will return the -EINVAL.

This patch try to free the its list resource when vm reboot or reset to avoid 
this.

Signed-off-by: wanghaibin 
---
 virt/kvm/arm/arm.c   |  5 -
 virt/kvm/arm/vgic/vgic-its.c | 10 ++
 virt/kvm/arm/vgic/vgic.h |  1 +
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index a39a1e1..db7632d 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -46,6 +46,7 @@
 #include 
 #include 
 #include 
+#include "vgic.h"
 
 #ifdef REQUIRES_VIRT
 __asm__(".arch_extension   virt");
@@ -901,8 +902,10 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu 
*vcpu,
 * Ensure a rebooted VM will fault in RAM pages and detect if the
 * guest MMU is turned off and flush the caches as needed.
 */
-   if (vcpu->arch.has_run_once)
+   if (vcpu->arch.has_run_once) {
stage2_unmap_vm(vcpu->kvm);
+   vgic_its_free_resource(vcpu->kvm);
+   }
 
vcpu_reset_hcr(vcpu);
 
diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 25d614f..5c20352 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -2467,6 +2467,16 @@ static int vgic_its_get_attr(struct kvm_device *dev,
.has_attr = vgic_its_has_attr,
 };
 
+void vgic_its_free_resource(struct kvm *kvm)
+{
+   struct kvm_device *dev, *tmp;
+
+   list_for_each_entry_safe(dev, tmp, >devices, vm_node) {
+   if(dev->ops == _arm_vgic_its_ops)
+   vgic_its_free_list(kvm, dev->private);
+   }
+}
+
 int kvm_vgic_register_its_device(void)
 {
return kvm_register_device_ops(_arm_vgic_its_ops,
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index c2be5b7..fbcbdfd 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -222,5 +222,6 @@ int vgic_v3_line_level_info_uaccess(struct kvm_vcpu *vcpu, 
bool is_write,
 
 bool lock_all_vcpus(struct kvm *kvm);
 void unlock_all_vcpus(struct kvm *kvm);
+void vgic_its_free_resource(struct kvm *kvm);
 
 #endif
-- 
1.8.3.1


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

[RFC PATCH 1/3] kvm: arm/arm64: vgic-vits: separate vgic_its_free_list() function

2017-09-06 Thread wanghaibin

We slightly refactor vgic_its_destroy, separate vgic_its_free_list()
function for later patch invoke.

The patch also take a functional change. If the its->device_list.next
is NULL, we still should free the its.
Honestly, I can't understand How does the its->device_list.next is NULL
happened at this moment.

Signed-off-by: wanghaibin 
---
 virt/kvm/arm/vgic/vgic-its.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index aa6b68d..25d614f 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1624,10 +1624,8 @@ static void vgic_its_free_device(struct kvm *kvm, struct 
its_device *dev)
kfree(dev);
 }
 
-static void vgic_its_destroy(struct kvm_device *kvm_dev)
+static void vgic_its_free_list(struct kvm *kvm, struct vgic_its *its)
 {
-   struct kvm *kvm = kvm_dev->kvm;
-   struct vgic_its *its = kvm_dev->private;
struct list_head *cur, *temp;
 
/*
@@ -1653,7 +1651,14 @@ static void vgic_its_destroy(struct kvm_device *kvm_dev)
kfree(coll);
}
mutex_unlock(>its_lock);
+}
+
+static void vgic_its_destroy(struct kvm_device *kvm_dev)
+{
+   struct kvm *kvm = kvm_dev->kvm;
+   struct vgic_its *its = kvm_dev->private;
 
+   vgic_its_free_list(kvm, its);
kfree(its);
 }
 
-- 
1.8.3.1


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

[RFC PATCH 3/3] kvm: arm/arm64: vgic-its: fix return value for restore

2017-09-06 Thread wanghaibin

This patch fix the migrate restore tables failure.

The same scene, at the destination, the restore tables interface traversal guest
memory, and check the dte/ite is valid or not.
If all dtes/ites are invalid, we will do try next one, and the last it will take
the 1 return value, but currently, it be treated as error. That's not correct.

This patch try to fix this problem.

Signed-off-by: wanghaibin 
---
 virt/kvm/arm/vgic/vgic-its.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 5c20352..2c69aeb 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -2025,7 +2025,7 @@ static int vgic_its_restore_dte(struct vgic_its *its, u32 
id,
return PTR_ERR(dev);
 
ret = vgic_its_restore_itt(its, dev);
-   if (ret) {
+   if (ret < 0) {
vgic_its_free_device(its->dev->kvm, dev);
return ret;
}
@@ -2147,9 +2147,6 @@ static int vgic_its_restore_device_tables(struct vgic_its 
*its)
 vgic_its_restore_dte, NULL);
}
 
-   if (ret > 0)
-   ret = -EINVAL;
-
return ret;
 }
 
-- 
1.8.3.1


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

[RFC PATCH 0/3] fix migrate failed when vm is in booting

2017-09-06 Thread wanghaibin

We have a test scenario： vmlife and migrate fixed test.

Here is a problem； VM migration failed caused the qemu core which gdb trace:

#0  0xb023fe84 in raise () from /usr/lib64/libc.so.6
#1  0xb0241b80 in abort () from /usr/lib64/libc.so.6
#2  0x0046b408 in kvm_device_access (fd=, group=4, 
attr=1, val=, write=)
at /usr/src/debug/qemu-2.6.0/kvm-all.c:2064
#3  0x0059ade8 in vm_state_notify (running=running@entry=0, 
state=state@entry=RUN_STATE_FINISH_MIGRATE) at vl.c:1728
#4  0x0045736c in do_vm_stop 
(state=state@entry=RUN_STATE_FINISH_MIGRATE) at 
/usr/src/debug/qemu-2.6.0/cpus.c:744
#5  0x004573bc in vm_stop (state=state@entry=RUN_STATE_FINISH_MIGRATE) 
at /usr/src/debug/qemu-2.6.0/cpus.c:1434
#6  0x00457428 in vm_stop_force_state 
(state=state@entry=RUN_STATE_FINISH_MIGRATE) at 
/usr/src/debug/qemu-2.6.0/cpus.c:1442
#7  0x006d68d0 in migration_completion (s=s@entry=0xb92d60 
, current_active_state=4, 
current_active_state@entry=65535, old_vm_running=0xb03c2000, 
old_vm_running@entry=0xfffe934fc53f, 
start_time=0xfffe934fcce0, start_time@entry=0xfffe934fc540) at 
migration/migration.c:1798
#8  0x006d7480 in migration_thread (opaque=0xb92d60 
) at migration/migration.c:1995
#9  0xb0398dc4 in start_thread () from /usr/lib64/libpthread.so.0
#10 0xb02ef020 in thread_start () from /usr/lib64/libc.so.6

qemu failed log :
2017-08-28T12:34:29.654396Z qemu-kvm: KVM_SET_DEVICE_ATTR failed: Invalid 
argument

I try to debug it, and I found the migrate at the moment that the vm is still 
booting.

So just change the guest like :
diff --git a/drivers/pci/host/pci-host-common.c 
b/drivers/pci/host/pci-host-common.c
index e9a53ba..d73fc03 100644
--- a/drivers/pci/host/pci-host-common.c
+++ b/drivers/pci/host/pci-host-common.c
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 
 static int gen_pci_parse_request_of_pci_ranges(struct device *dev,
   struct list_head *resources, struct resource **bus_range)
@@ -119,6 +120,7 @@ int pci_host_common_probe(struct platform_device *pdev,
struct pci_bus *bus, *child;
struct pci_config_window *cfg;
struct list_head resources;
+   int i;
 
type = of_get_property(np, "device_type", NULL);
if (!type || strcmp(type, "pci")) {
@@ -164,6 +166,8 @@ int pci_host_common_probe(struct platform_device *pdev,
pcie_bus_configure_settings(child);
}
 
+   for (i=0;i<2; i++)
+   udelay(1000);
pci_bus_add_devices(bus);
return 0;
 }

And migrate at this delay time, It must be failed.

This patchset try to fix this problem.

BTW: This patchset just a demo, haven't do more test, and the implement maybe a 
little evil.

Thanks



wanghaibin (3):
  kvm: arm/arm64: vgic-vits: separate vgic_its_free_list() function
  kvm: arm/arm64: vgic-vits: free its resource when vm reboot/reset
  kvm: arm/arm64: vgic-its: fix return value for restore

 virt/kvm/arm/arm.c   |  5 -
 virt/kvm/arm/vgic/vgic-its.c | 26 +++---
 virt/kvm/arm/vgic/vgic.h |  1 +
 3 files changed, 24 insertions(+), 8 deletions(-)

-- 
1.8.3.1


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

Re: [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits

2017-09-06 Thread Vladimir Murzin

On 06/09/17 13:44, gengdongjiu wrote:
> 
> 
> On 2017/9/6 20:30, Vladimir Murzin wrote:
>> On 06/09/17 13:14, gengdongjiu wrote:
>>>
>>>
>>> On 2017/9/6 20:00, Vladimir Murzin wrote:
 On 06/09/17 11:35, gengdongjiu wrote:
> Vladimir,
>
> On 2017/9/6 17:41, Vladimir Murzin wrote:
>> Can you please elaborate on cases where PAN is not enabled?
>
> I mean the informal private usage, For example, he disabled the PAN 
> dynamically to let kernel space to access the user space.
> After he dynamic disabled the PAN, then switched to guest OS. after 
> return to host. he found the PAN stage is modified.
> Of cause this is not a formal usage, in our host kernel, it is always 
> enabled, no dynamic change, but I means it may exist such cases.
>
>

 So, in short, there is no real issue with PAN, right? What about UAO?
>>> For the PAN, if host OS dynamically enable/disable PAN should have issue.
>>> Do you think that is not a issue as above description?
>>>
>>> "host OS dynamically disable the PAN, but after go back from the guest OS, 
>>> The PAN is unexpectedly enabled"
>>
>> Do you see effect of "PAN is unexpectedly enabled"?
> In fact I did not encounter this case, but I think it can exist.
> I think if host OS dynamically disable PAN, it wants the host kernel access 
> the user space address space not through copy_to/from_user API.
> Now if it is unexpectedly enabled, when host kernel still accesses the user 
> space address,  it will happen MMU fault exception.

And this is expected! The only allowed channel for kernel <-> user is uaccess
API.

I guess that you have test (and that great!) which violates that rule (for
testing purpose, why not?) and now you are trying to fit kernel into it...

Cheers
Vladimir

> 
> 
>>
>> Cheers
>> Vladimir
>>
>>>

 Cheers
 Vladimir

 .

>>>
>>>
>>
>>
>> .
>>
> 
> 

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

Re: [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits

2017-09-06 Thread gengdongjiu



On 2017/9/6 20:30, Vladimir Murzin wrote:
> On 06/09/17 13:14, gengdongjiu wrote:
>>
>>
>> On 2017/9/6 20:00, Vladimir Murzin wrote:
>>> On 06/09/17 11:35, gengdongjiu wrote:
 Vladimir,

 On 2017/9/6 17:41, Vladimir Murzin wrote:
> Can you please elaborate on cases where PAN is not enabled?

 I mean the informal private usage, For example, he disabled the PAN 
 dynamically to let kernel space to access the user space.
 After he dynamic disabled the PAN, then switched to guest OS. after return 
 to host. he found the PAN stage is modified.
 Of cause this is not a formal usage, in our host kernel, it is always 
 enabled, no dynamic change, but I means it may exist such cases.


>>>
>>> So, in short, there is no real issue with PAN, right? What about UAO?
>> For the PAN, if host OS dynamically enable/disable PAN should have issue.
>> Do you think that is not a issue as above description?
>>
>> "host OS dynamically disable the PAN, but after go back from the guest OS, 
>> The PAN is unexpectedly enabled"
> 
> Do you see effect of "PAN is unexpectedly enabled"?
In fact I did not encounter this case, but I think it can exist.
I think if host OS dynamically disable PAN, it wants the host kernel access the 
user space address space not through copy_to/from_user API.
Now if it is unexpectedly enabled, when host kernel still accesses the user 
space address,  it will happen MMU fault exception.


> 
> Cheers
> Vladimir
> 
>>
>>>
>>> Cheers
>>> Vladimir
>>>
>>> .
>>>
>>
>>
> 
> 
> .
> 

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

Re: [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits

2017-09-06 Thread gengdongjiu



On 2017/9/6 20:00, Vladimir Murzin wrote:
> On 06/09/17 11:35, gengdongjiu wrote:
>> Vladimir,
>>
>> On 2017/9/6 17:41, Vladimir Murzin wrote:
>>> Can you please elaborate on cases where PAN is not enabled?
>>
>> I mean the informal private usage, For example, he disabled the PAN 
>> dynamically to let kernel space to access the user space.
>> After he dynamic disabled the PAN, then switched to guest OS. after return 
>> to host. he found the PAN stage is modified.
>> Of cause this is not a formal usage, in our host kernel, it is always 
>> enabled, no dynamic change, but I means it may exist such cases.
>>
>>
> 
> So, in short, there is no real issue with PAN, right? What about UAO?
For the pstate.UAO, current code has issue from my test. Because after 
switching from guest os, it does not set pstate.UAO again.
PAN is set again in your previous patch when switch to host, but UAO is not.
If you have concern about the save/restore PSTATE bits, may be we can use below 
modification to fix UAO issue.

diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
index 9341376..c3dd761 100644
--- a/arch/arm64/kvm/hyp/sysreg-sr.c
+++ b/arch/arm64/kvm/hyp/sysreg-sr.c
@@ -21,6 +21,8 @@
 #include 
 #include 

+#include 

 /* Yes, this does nothing, on purpose */
 static void __hyp_text __sysreg_do_nothing(struct kvm_cpu_context *ctxt) { }

@@ -121,8 +123,13 @@ static void __hyp_text __sysreg_restore_state(struct 
kvm_cpu_context *ctxt)
write_sysreg_el1(ctxt->gp_regs.spsr[KVM_SPSR_EL1],spsr);
 }

+static void __hyp_text __sysreg_restore_state_vhe(struct kvm_cpu_context *ctxt)
+{
+uao_thread_switch(current);
+}
+
 static hyp_alternate_select(__sysreg_call_restore_host_state,
-   __sysreg_restore_state, __sysreg_do_nothing,
+   __sysreg_restore_state, __sysreg_restore_state_vhe,
ARM64_HAS_VIRT_HOST_EXTN);

 void __hyp_text __sysreg_restore_host_state(struct kvm_cpu_context *ctxt)


> 
> Cheers
> Vladimir
> 
> .
> 

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

Re: [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits

2017-09-06 Thread Vladimir Murzin

On 06/09/17 13:14, gengdongjiu wrote:
> 
> 
> On 2017/9/6 20:00, Vladimir Murzin wrote:
>> On 06/09/17 11:35, gengdongjiu wrote:
>>> Vladimir,
>>>
>>> On 2017/9/6 17:41, Vladimir Murzin wrote:
 Can you please elaborate on cases where PAN is not enabled?
>>>
>>> I mean the informal private usage, For example, he disabled the PAN 
>>> dynamically to let kernel space to access the user space.
>>> After he dynamic disabled the PAN, then switched to guest OS. after return 
>>> to host. he found the PAN stage is modified.
>>> Of cause this is not a formal usage, in our host kernel, it is always 
>>> enabled, no dynamic change, but I means it may exist such cases.
>>>
>>>
>>
>> So, in short, there is no real issue with PAN, right? What about UAO?
> For the PAN, if host OS dynamically enable/disable PAN should have issue.
> Do you think that is not a issue as above description?
> 
> "host OS dynamically disable the PAN, but after go back from the guest OS, 
> The PAN is unexpectedly enabled"

Do you see effect of "PAN is unexpectedly enabled"?

Cheers
Vladimir

> 
>>
>> Cheers
>> Vladimir
>>
>> .
>>
> 
> 

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

[PATCH v3 3/5] KVM: arm/arm64: vgic: Support level-triggered mapped interrupts

2017-09-06 Thread Christoffer Dall

Level-triggered mapped IRQs are special because we only observe rising
edges as input to the VGIC, and we don't set the EOI flag and therefore
are not told when the level goes down, so that we can re-queue a new
interrupt when the level goes up.

One way to solve this problem is to side-step the logic of the VGIC and
special case the validation in the injection path, but it has the
unfortunate drawback of having to peak into the physical GIC state
whenever we want to know if the interrupt is pending on the virtual
distributor.

Instead, we can maintain the current semantics of a level triggered
interrupt by sort of treating it as an edge-triggered interrupt,
following from the fact that we only observe an asserting edge.  This
requires us to be a bit careful when populating the LRs and when folding
the state back in though:

 * We lower the line level when populating the LR, so that when
   subsequently observing an asserting edge, the VGIC will do the right
   thing.

 * If the guest never acked the interrupt while running (for example if
   it had masked interrupts at the CPU level while running), we have
   to preserve the pending state of the LR and move it back to the
   line_level field of the struct irq when folding LR state.

   If the guest never acked the interrupt while running, but changed the
   device state and lowered the line (again with interrupts masked) then
   we need to observe this change in the line_level.

   Both of the above situations are solved by sampling the physical line
   and set the line level when folding the LR back.

 * Finally, if the guest never acked the interrupt while running and
   sampling the line reveals that the device state has changed and the
   line has been lowered, we must clear the physical active state, since
   we will otherwise never be told when the interrupt becomes asserted
   again.

This has the added benefit of making the timer optimization patches
(https://lists.cs.columbia.edu/pipermail/kvmarm/2017-July/026343.html) a
bit simpler, because the timer code doesn't have to clear the active
state on the sync anymore.  It also potentially improves the performance
of the timer implementation because the GIC knows the state or the LR
and only needs to clear the
active state when the pending bit in the LR is still set, where the
timer has to always clear it when returning from running the guest with
an injected timer interrupt.

Signed-off-by: Christoffer Dall 
Reviewed-by: Marc Zyngier 
---
 virt/kvm/arm/vgic/vgic-v2.c | 29 +
 virt/kvm/arm/vgic/vgic-v3.c | 29 +
 virt/kvm/arm/vgic/vgic.c| 23 +++
 virt/kvm/arm/vgic/vgic.h|  7 +++
 4 files changed, 88 insertions(+)

diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
index e4187e5..618ed3f 100644
--- a/virt/kvm/arm/vgic/vgic-v2.c
+++ b/virt/kvm/arm/vgic/vgic-v2.c
@@ -104,6 +104,26 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)
irq->pending_latch = false;
}
 
+   /*
+* Level-triggered mapped IRQs are special because we only
+* observe rising edges as input to the VGIC.
+*
+* If the guest never acked the interrupt we have to sample
+* the physical line and set the line level, because the
+* device state could have changed or we simply need to
+* process the still pending interrupt later.
+*
+* If this causes us to lower the level, we have to also clear
+* the physical active state, since we will otherwise never be
+* told when the interrupt becomes asserted again.
+*/
+   if (vgic_irq_is_mapped_level(irq) && (val & 
GICH_LR_PENDING_BIT)) {
+   irq->line_level = vgic_get_phys_line_level(irq);
+
+   if (!irq->line_level)
+   vgic_irq_set_phys_active(irq, false);
+   }
+
spin_unlock(>irq_lock);
vgic_put_irq(vcpu->kvm, irq);
}
@@ -161,6 +181,15 @@ void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct 
vgic_irq *irq, int lr)
val |= GICH_LR_EOI;
}
 
+   /*
+* Level-triggered mapped IRQs are special because we only observe
+* rising edges as input to the VGIC.  We therefore lower the line
+* level here, so that we can take new virtual IRQs.  See
+* vgic_v2_fold_lr_state for more info.
+*/
+   if (vgic_irq_is_mapped_level(irq) && (val & GICH_LR_PENDING_BIT))
+   irq->line_level = false;
+
/* The GICv2 LR only holds five bits of priority. */
val |= (irq->priority >> 3) << GICH_LR_PRIORITY_SHIFT;
 
diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index

[PATCH v3 5/5] KVM: arm/arm64: Provide a vgic interrupt line level sample function

2017-09-06 Thread Christoffer Dall

The GIC sometimes need to sample the physical line of a mapped
interrupt.  As we know this to be notoriously slow, provide a callback
function for devices (such as the timer) which can do this much faster
than talking to the distributor, for example by comparing a few
in-memory values.  Fall back to the good old method of poking the
physical GIC if no callback is provided.

Signed-off-by: Christoffer Dall 
Reviewed-by: Marc Zyngier 
---
 include/kvm/arm_vgic.h| 13 -
 virt/kvm/arm/arch_timer.c | 16 +++-
 virt/kvm/arm/vgic/vgic.c  | 12 +---
 3 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 53f631b..7f76c9e 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -125,6 +125,17 @@ struct vgic_irq {
u8 priority;
enum vgic_irq_config config;/* Level or edge */
 
+   /*
+* Callback function pointer to in-kernel devices that can tell us the
+* state of the input level of mapped level-triggered IRQ faster than
+* peaking into the physical GIC.
+*
+* Always called in non-preemptible section and the functions can use
+* kvm_arm_get_running_vcpu() to get the vcpu pointer for private
+* IRQs.
+*/
+   bool (*get_input_level)(int vintid);
+
void *owner;/* Opaque pointer to reserve an 
interrupt
   for in-kernel devices. */
 };
@@ -309,7 +320,7 @@ void kvm_vgic_init_cpu_hardware(void);
 int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
bool level, void *owner);
 int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
- u32 vintid);
+ u32 vintid, bool (*get_input_level)(int vindid));
 int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int vintid);
 bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int vintid);
 
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index c4fa675..5cb96ca 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -645,6 +645,19 @@ static bool timer_irqs_are_valid(struct kvm_vcpu *vcpu)
return true;
 }
 
+static bool timer_get_input_level(int vintid)
+{
+   struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
+   struct arch_timer_context *timer;
+
+   if (vintid == vcpu_vtimer(vcpu)->irq.irq)
+   timer = vcpu_vtimer(vcpu);
+   else
+   BUG(); /* We only map the vtimer so far */
+
+   return kvm_timer_should_fire(timer);
+}
+
 int kvm_timer_enable(struct kvm_vcpu *vcpu)
 {
struct arch_timer_cpu *timer = >arch.timer_cpu;
@@ -666,7 +679,8 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
return -EINVAL;
}
 
-   ret = kvm_vgic_map_phys_irq(vcpu, host_vtimer_irq, vtimer->irq.irq);
+   ret = kvm_vgic_map_phys_irq(vcpu, host_vtimer_irq, vtimer->irq.irq,
+   timer_get_input_level);
if (ret)
return ret;
 
diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
index 7aec730..0ca2b3d 100644
--- a/virt/kvm/arm/vgic/vgic.c
+++ b/virt/kvm/arm/vgic/vgic.c
@@ -154,6 +154,9 @@ bool vgic_get_phys_line_level(struct vgic_irq *irq)
 
BUG_ON(!irq->hw);
 
+   if (irq->get_input_level)
+   return irq->get_input_level(irq->intid);
+
WARN_ON(irq_get_irqchip_state(irq->host_irq,
  IRQCHIP_STATE_PENDING,
  _level));
@@ -437,7 +440,8 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, 
unsigned int intid,
 
 /* @irq->irq_lock must be held */
 static int kvm_vgic_map_irq(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
-   unsigned int host_irq)
+   unsigned int host_irq,
+   bool (*get_input_level)(int vindid))
 {
struct irq_desc *desc;
struct irq_data *data;
@@ -457,6 +461,7 @@ static int kvm_vgic_map_irq(struct kvm_vcpu *vcpu, struct 
vgic_irq *irq,
irq->hw = true;
irq->host_irq = host_irq;
irq->hwintid = data->hwirq;
+   irq->get_input_level = get_input_level;
return 0;
 }
 
@@ -465,10 +470,11 @@ static inline void kvm_vgic_unmap_irq(struct vgic_irq 
*irq)
 {
irq->hw = false;
irq->hwintid = 0;
+   irq->get_input_level = NULL;
 }
 
 int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
- u32 vintid)
+ u32 vintid, bool (*get_input_level)(int vindid))
 {
struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, vintid);
int ret;
@@ -476,7 +482,7 @@ int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned 
int host_irq,
BUG_ON(!irq);
 
spin_lock(>irq_lock);
-   ret

[PATCH v3 0/5] Handle forwarded level-triggered interrupts

2017-09-06 Thread Christoffer Dall

This series illustrates an alternative approach to Eric Auger's direct EOI
setup patches [1] in terms of the KVM VGIC support.

The idea is to maintain existing semantics for the VGIC for mapped
level-triggered IRQs and think support for the timer into it.

Patch 1 is necessary to align the timer and VFIO ways of signaling the
VGIC.  Patch 2 is stolen from Eric's series and is necessary for these
patches to compile as well.  Patch 3 includes the core support for
mapped level-triggered interrupts.  Patch 4 handles guest MMIO access to
the virtual distributor.  Patch 5 moves some code around for patch 6.
Patch 6 implements an optimization for the timer.  The last two patches
could be deferred until the timer optimization series.

Based on v4.13

Changes since v2:
 - Removed patch 5 from v2 and integrating the changes in what's now
   patch 5 to make it easier to reuse code when adding VFIO integration.
 - Changed the virtual distributor MMIO handling to use the
   pending_latch and more closely match the semantics of SPENDR and
   CPENDR for both level and edge mapped interrupts.

Changes since v1:
 - Added necessary changes to the timer (Patch 1)
 - Added handling of guest MMIO accesses to the virtual distributor
   (Patch 4)
 - Addressed Marc's comments from the initial RFC (mostly renames)

Thanks,
-Christoffer

---

Christoffer Dall (4):
  KVM: arm/arm64: Don't cache the timer IRQ level
  KVM: arm/arm64: vgic: Support level-triggered mapped interrupts
  KVM: arm/arm64: Support VGIC dist pend/active changes for mapped IRQs
  KVM: arm/arm64: Provide a vgic interrupt line level sample function

Eric Auger (1):
  KVM: arm/arm64: vgic: restructure kvm_vgic_(un)map_phys_irq

 include/kvm/arm_vgic.h| 19 +++--
 virt/kvm/arm/arch_timer.c | 52 +++
 virt/kvm/arm/vgic/vgic-mmio.c | 33 +++
 virt/kvm/arm/vgic/vgic-v2.c   | 29 +
 virt/kvm/arm/vgic/vgic-v3.c   | 29 +
 virt/kvm/arm/vgic/vgic.c  | 96 ---
 virt/kvm/arm/vgic/vgic.h  |  8 
 7 files changed, 219 insertions(+), 47 deletions(-)

-- 
2.9.0

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

[PATCH v3 1/5] KVM: arm/arm64: Don't cache the timer IRQ level

2017-09-06 Thread Christoffer Dall

The timer was modeled after a strict idea of modelling an interrupt line
level in software, meaning that only transitions in the level needed to
be reported to the VGIC.  This works well for the timer, because the
arch timer code is in complete control of the device and can track the
transitions of the line.

However, as we are about to support using the HW bit in the VGIC not
just for the timer, but also for VFIO which cannot track transitions of
the interrupt line, we have to decide on an interface for level
triggered mapped interrupts to the GIC, which both the timer and VFIO
can use.

VFIO only sees an asserting transition of the physical interrupt line,
and tells the VGIC when that happens.  That means that part of the
interrupt flow is offloaded to the hardware.

To use the same interface for VFIO devices and the timer, we therefore
have to change the timer (we cannot change VFIO because it doesn't know
the details of the device it is assigning to a VM).

Luckily, changing the timer is simple, we just need to stop 'caching'
the line level, but instead let the VGIC know the state of the timer on
every entry to the guest, and the VGIC can ignore notifications using
its validate mechanism.

Signed-off-by: Christoffer Dall 
---
 virt/kvm/arm/arch_timer.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 8e89d63..2a5f877 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -219,9 +219,10 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, 
bool new_level,
int ret;
 
timer_ctx->active_cleared_last = false;
+   if (timer_ctx->irq.level != new_level)
+   trace_kvm_timer_update_irq(vcpu->vcpu_id, timer_ctx->irq.irq,
+  new_level);
timer_ctx->irq.level = new_level;
-   trace_kvm_timer_update_irq(vcpu->vcpu_id, timer_ctx->irq.irq,
-  timer_ctx->irq.level);
 
if (likely(irqchip_in_kernel(vcpu->kvm))) {
ret = kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id,
@@ -241,6 +242,7 @@ static void kvm_timer_update_state(struct kvm_vcpu *vcpu)
struct arch_timer_cpu *timer = >arch.timer_cpu;
struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
+   bool level;
 
/*
 * If userspace modified the timer registers via SET_ONE_REG before
@@ -251,11 +253,11 @@ static void kvm_timer_update_state(struct kvm_vcpu *vcpu)
if (unlikely(!timer->enabled))
return;
 
-   if (kvm_timer_should_fire(vtimer) != vtimer->irq.level)
-   kvm_timer_update_irq(vcpu, !vtimer->irq.level, vtimer);
+   level = kvm_timer_should_fire(vtimer);
+   kvm_timer_update_irq(vcpu, level, vtimer);
 
-   if (kvm_timer_should_fire(ptimer) != ptimer->irq.level)
-   kvm_timer_update_irq(vcpu, !ptimer->irq.level, ptimer);
+   level = kvm_timer_should_fire(ptimer);
+   kvm_timer_update_irq(vcpu, level, ptimer);
 }
 
 /* Schedule the background timer for the emulated timer. */
-- 
2.9.0

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

[PATCH v3 2/5] KVM: arm/arm64: vgic: restructure kvm_vgic_(un)map_phys_irq

2017-09-06 Thread Christoffer Dall

From: Eric Auger 

We want to reuse the core of the map/unmap functions for IRQ forwarding.
Let's move the computation of the hwirq in kvm_vgic_map_phys_irq and
pass the linux IRQ as parameter.

The host_irq is added to struct vgic_irq because it is needed in later
patches which manipulate the physical GIC state to support forwarded
IRQs.

We introduce kvm_vgic_map/unmap_irq which take a struct vgic_irq handle
as a parameter.

Signed-off-by: Eric Auger 
Signed-off-by: Christoffer Dall 
Acked-by: Marc Zyngier 
---
 include/kvm/arm_vgic.h|  8 ---
 virt/kvm/arm/arch_timer.c | 24 +--
 virt/kvm/arm/vgic/vgic.c  | 60 +++
 3 files changed, 51 insertions(+), 41 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 34dba51..53f631b 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -116,6 +116,7 @@ struct vgic_irq {
bool hw;/* Tied to HW IRQ */
struct kref refcount;   /* Used for LPIs */
u32 hwintid;/* HW INTID number */
+   unsigned int host_irq;  /* linux irq corresponding to hwintid */
union {
u8 targets; /* GICv2 target VCPUs mask */
u32 mpidr;  /* GICv3 target VCPU */
@@ -307,9 +308,10 @@ void kvm_vgic_init_cpu_hardware(void);
 
 int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
bool level, void *owner);
-int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq);
-int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq);
-bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq);
+int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
+ u32 vintid);
+int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int vintid);
+bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int vintid);
 
 int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
 
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 2a5f877..c4fa675 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -649,9 +649,6 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
 {
struct arch_timer_cpu *timer = >arch.timer_cpu;
struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
-   struct irq_desc *desc;
-   struct irq_data *data;
-   int phys_irq;
int ret;
 
if (timer->enabled)
@@ -669,26 +666,7 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
return -EINVAL;
}
 
-   /*
-* Find the physical IRQ number corresponding to the host_vtimer_irq
-*/
-   desc = irq_to_desc(host_vtimer_irq);
-   if (!desc) {
-   kvm_err("%s: no interrupt descriptor\n", __func__);
-   return -EINVAL;
-   }
-
-   data = irq_desc_get_irq_data(desc);
-   while (data->parent_data)
-   data = data->parent_data;
-
-   phys_irq = data->hwirq;
-
-   /*
-* Tell the VGIC that the virtual interrupt is tied to a
-* physical interrupt. We do that once per VCPU.
-*/
-   ret = kvm_vgic_map_phys_irq(vcpu, vtimer->irq.irq, phys_irq);
+   ret = kvm_vgic_map_phys_irq(vcpu, host_vtimer_irq, vtimer->irq.irq);
if (ret)
return ret;
 
diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
index fed717e..9d557efd 100644
--- a/virt/kvm/arm/vgic/vgic.c
+++ b/virt/kvm/arm/vgic/vgic.c
@@ -17,6 +17,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 
 #include "vgic.h"
 
@@ -403,38 +405,66 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, 
unsigned int intid,
return 0;
 }
 
-int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq)
+/* @irq->irq_lock must be held */
+static int kvm_vgic_map_irq(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
+   unsigned int host_irq)
 {
-   struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, virt_irq);
+   struct irq_desc *desc;
+   struct irq_data *data;
 
-   BUG_ON(!irq);
-
-   spin_lock(>irq_lock);
+   /*
+* Find the physical IRQ number corresponding to @host_irq
+*/
+   desc = irq_to_desc(host_irq);
+   if (!desc) {
+   kvm_err("%s: no interrupt descriptor\n", __func__);
+   return -EINVAL;
+   }
+   data = irq_desc_get_irq_data(desc);
+   while (data->parent_data)
+   data = data->parent_data;
 
irq->hw = true;
-   irq->hwintid = phys_irq;
+   irq->host_irq = host_irq;
+   irq->hwintid = data->hwirq;
+   return 0;
+}
+
+/* @irq->irq_lock must be held */
+static inline void kvm_vgic_unmap_irq(struct vgic_irq *irq)
+{
+   irq->hw =

Re: [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits

2017-09-06 Thread gengdongjiu



On 2017/9/6 20:00, Vladimir Murzin wrote:
> On 06/09/17 11:35, gengdongjiu wrote:
>> Vladimir,
>>
>> On 2017/9/6 17:41, Vladimir Murzin wrote:
>>> Can you please elaborate on cases where PAN is not enabled?
>>
>> I mean the informal private usage, For example, he disabled the PAN 
>> dynamically to let kernel space to access the user space.
>> After he dynamic disabled the PAN, then switched to guest OS. after return 
>> to host. he found the PAN stage is modified.
>> Of cause this is not a formal usage, in our host kernel, it is always 
>> enabled, no dynamic change, but I means it may exist such cases.
>>
>>
> 
> So, in short, there is no real issue with PAN, right? What about UAO?
For the PAN, if host OS dynamically enable/disable PAN should have issue.
Do you think that is not a issue as above description?

"host OS dynamically disable the PAN, but after go back from the guest OS, The 
PAN is unexpectedly enabled"

> 
> Cheers
> Vladimir
> 
> .
> 

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

Re: [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits

2017-09-06 Thread Vladimir Murzin

On 06/09/17 11:35, gengdongjiu wrote:
> Vladimir,
> 
> On 2017/9/6 17:41, Vladimir Murzin wrote:
>> Can you please elaborate on cases where PAN is not enabled?
> 
> I mean the informal private usage, For example, he disabled the PAN 
> dynamically to let kernel space to access the user space.
> After he dynamic disabled the PAN, then switched to guest OS. after return to 
> host. he found the PAN stage is modified.
> Of cause this is not a formal usage, in our host kernel, it is always 
> enabled, no dynamic change, but I means it may exist such cases.
> 
> 

So, in short, there is no real issue with PAN, right? What about UAO?

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

Re: [PATCH v6 0/7] Add RAS virtualization support for SEA/SEI notification type in KVM

2017-09-06 Thread gengdongjiu

Hi Peter,

On 2017/9/6 19:19, Peter Maydell wrote:
> On 28 August 2017 at 11:38, Dongjiu Geng  wrote:
>> In the firmware-first RAS solution, corrupt data is detected in a
>> memory location when guest OS application software executing at EL0
>> or guest OS kernel El1 software are reading from the memory. The
>> memory node records errors in an error record accessible using
>> system registers.
> 
> Hi Dongjiu -- it looks like this patch set is extending
> the API KVM provides to userspace, but it doesn't update
> the documentation in Documentation/virtual/kvm/api.txt.
> I appreciate the API is still somewhat under discussion,
> but if you can include the docs updates it's helpful to
> me for reviewing whether the API makes sense from the
> userspace consumer end of it.

sure, it should. thanks a lot for the reminder. I will update the related docs
in my next patch set version.

> 
> thanks
> -- PMM
> 
> .
> 

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

Re: [PATCH v6 0/7] Add RAS virtualization support for SEA/SEI notification type in KVM

2017-09-06 Thread Peter Maydell

On 28 August 2017 at 11:38, Dongjiu Geng  wrote:
> In the firmware-first RAS solution, corrupt data is detected in a
> memory location when guest OS application software executing at EL0
> or guest OS kernel El1 software are reading from the memory. The
> memory node records errors in an error record accessible using
> system registers.

Hi Dongjiu -- it looks like this patch set is extending
the API KVM provides to userspace, but it doesn't update
the documentation in Documentation/virtual/kvm/api.txt.
I appreciate the API is still somewhat under discussion,
but if you can include the docs updates it's helpful to
me for reviewing whether the API makes sense from the
userspace consumer end of it.

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

Re: [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits

2017-09-06 Thread gengdongjiu

Vladimir,

On 2017/9/6 17:41, Vladimir Murzin wrote:
> Can you please elaborate on cases where PAN is not enabled?

I mean the informal private usage, For example, he disabled the PAN dynamically 
to let kernel space to access the user space.
After he dynamic disabled the PAN, then switched to guest OS. after return to 
host. he found the PAN stage is modified.
Of cause this is not a formal usage, in our host kernel, it is always enabled, 
no dynamic change, but I means it may exist such cases.

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

Re: [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits

2017-09-06 Thread gengdongjiu

For UAO, if not save/restore PSTATE.UAO, we can use below fixing.

diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
index 9341376..c3dd761 100644
--- a/arch/arm64/kvm/hyp/sysreg-sr.c
+++ b/arch/arm64/kvm/hyp/sysreg-sr.c
@@ -21,6 +21,8 @@
 #include 
 #include 

+#include 
+
 /* Yes, this does nothing, on purpose */
 static void __hyp_text __sysreg_do_nothing(struct kvm_cpu_context *ctxt) { }

@@ -121,8 +123,13 @@ static void __hyp_text __sysreg_restore_state(struct 
kvm_cpu_context *ctxt)
write_sysreg_el1(ctxt->gp_regs.spsr[KVM_SPSR_EL1],spsr);
 }

+static void __hyp_text __sysreg_restore_state_vhe(struct kvm_cpu_context *ctxt)
+{
+uao_thread_switch(current);
+}
+
 static hyp_alternate_select(__sysreg_call_restore_host_state,
-   __sysreg_restore_state, __sysreg_do_nothing,
+   __sysreg_restore_state, __sysreg_restore_state_vhe,
ARM64_HAS_VIRT_HOST_EXTN);

 void __hyp_text __sysreg_restore_host_state(struct kvm_cpu_context *ctxt)



On 2017/9/6 17:32, gengdongjiu wrote:
> Hi Marc,
> 
> On 2017/9/6 16:17, Marc Zyngier wrote:
>> On 05/09/17 19:58, gengdongjiu wrote:
>>> when exit from guest, some host PSTATE bits may be lost, such as
>>> PSTATE.PAN or PSTATE.UAO. It is because host and hypervisor all run
>>> in the EL2, host PSTATE value cannot be saved and restored via
>>> SPSR_EL2. So if guest has changed the PSTATE, host continues with
>>> a wrong value guest has set.
>>>
>>> Signed-off-by: Dongjiu Geng 
>>> Signed-off-by: Haibin Zhang 
>>> ---
>>>  arch/arm64/include/asm/kvm_host.h |  8 +++
>>>  arch/arm64/include/asm/kvm_hyp.h  |  2 ++
>>>  arch/arm64/include/asm/sysreg.h   | 23 +++
>>>  arch/arm64/kvm/hyp/entry.S|  2 --
>>>  arch/arm64/kvm/hyp/switch.c   | 24 ++--
>>>  arch/arm64/kvm/hyp/sysreg-sr.c| 48 
>>> ---
>>>  6 files changed, 100 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/arch/arm64/include/asm/kvm_host.h 
>>> b/arch/arm64/include/asm/kvm_host.h
>>> index e923b58..cba7d3e 100644
>>> --- a/arch/arm64/include/asm/kvm_host.h
>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>> @@ -193,6 +193,12 @@ struct kvm_cpu_context {
>>> };
>>>  };
>>>  
>>> +struct kvm_cpu_host_pstate {
>>> +   u64 daif;
>>> +   u64 uao;
>>> +   u64 pan;
>>> +};
>>
>> I love it. This is the most expensive way of saving/restoring a single
>> 32bit value.
>>
>> More seriously, please see the discussion between James and Christoffer
>> there[1]. I expect James to address the PAN/UAO states together with the
>> debug state in the next iteration of his patch.
> 
> I roughly see the discussion between James and Christoffer, Seems Christoffer 
> does not suggest save and
> restore it, and suggest to do below, and UAO/PAN may not use the same ways.
> 
>   __kvm_vcpu_run(struct kvm_vcpu *vcpu)
>   {
>   if (has_vhe())
>   asm("msr daifset, #0xf");
> 
>   ...
>  exit_code = __guest_enter(vcpu, host_ctxt);
>   ...
> 
>   if (has_vhe())
>   asm("msr daifclr, #0xd");
>   }
> 
> 
> If not save/restore them, the KVM will set them according to the CPU 
> capability. For example below fixing, it will check CPU capability, If CPU 
> supports PAN,
> the kvm will always enable the PAN for the host. But in fact, the host may be 
> not enable the PAN.
> Of course for the UAO, we can use the similar fixing if Marc or Christoffer 
> is agreed. but seems not make sense.
> 
> commit cb96408da4e11698674abd04aeac941c1bed2038
> Author: Vladimir Murzin 
> Date:   Thu Sep 1 15:29:03 2016 +0100
> 
> arm64: KVM: VHE: reset PSTATE.PAN on entry to EL2
> 
> SCTLR_EL2.SPAN bit controls what happens with the PSTATE.PAN bit on an
> exception. However, this bit has no effect on the PSTATE.PAN when
> HCR_EL2.E2H or HCR_EL2.TGE is unset. Thus when VHE is used and
> exception taken from a guest PSTATE.PAN bit left unchanged and we
> continue with a value guest has set.
> 
> To address that always reset PSTATE.PAN on entry from EL1.
> 
> Fixes: 1f364c8c48a0 ("arm64: VHE: Add support for running Linux in EL2 
> mode")
> 
> Signed-off-by: Vladimir Murzin 
> Reviewed-by: James Morse 
> Acked-by: Marc Zyngier 
> Cc:  # v4.6+
> Signed-off-by: Christoffer Dall 
> 
> diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
> index 3967c231..b5926ee 100644
> --- a/arch/arm64/kvm/hyp/entry.S
> +++ b/arch/arm64/kvm/hyp/entry.S
> @@ -96,6 +96,8 @@ ENTRY(__guest_exit)
> 
> add x1, x1, #VCPU_CONTEXT
> 
> +   ALTERNATIVE(nop, SET_PSTATE_PAN(1), ARM64_HAS_PAN, CONFIG_ARM64_PAN)
> +
> // Store the guest regs x2 and x3
>

Re: [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits

2017-09-06 Thread Vladimir Murzin

On 06/09/17 10:32, gengdongjiu wrote:
> Hi Marc,
> 
> On 2017/9/6 16:17, Marc Zyngier wrote:
>> On 05/09/17 19:58, gengdongjiu wrote:
>>> when exit from guest, some host PSTATE bits may be lost, such as
>>> PSTATE.PAN or PSTATE.UAO. It is because host and hypervisor all run
>>> in the EL2, host PSTATE value cannot be saved and restored via
>>> SPSR_EL2. So if guest has changed the PSTATE, host continues with
>>> a wrong value guest has set.
>>>
>>> Signed-off-by: Dongjiu Geng 
>>> Signed-off-by: Haibin Zhang 
>>> ---
>>>  arch/arm64/include/asm/kvm_host.h |  8 +++
>>>  arch/arm64/include/asm/kvm_hyp.h  |  2 ++
>>>  arch/arm64/include/asm/sysreg.h   | 23 +++
>>>  arch/arm64/kvm/hyp/entry.S|  2 --
>>>  arch/arm64/kvm/hyp/switch.c   | 24 ++--
>>>  arch/arm64/kvm/hyp/sysreg-sr.c| 48 
>>> ---
>>>  6 files changed, 100 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/arch/arm64/include/asm/kvm_host.h 
>>> b/arch/arm64/include/asm/kvm_host.h
>>> index e923b58..cba7d3e 100644
>>> --- a/arch/arm64/include/asm/kvm_host.h
>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>> @@ -193,6 +193,12 @@ struct kvm_cpu_context {
>>> };
>>>  };
>>>  
>>> +struct kvm_cpu_host_pstate {
>>> +   u64 daif;
>>> +   u64 uao;
>>> +   u64 pan;
>>> +};
>>
>> I love it. This is the most expensive way of saving/restoring a single
>> 32bit value.
>>
>> More seriously, please see the discussion between James and Christoffer
>> there[1]. I expect James to address the PAN/UAO states together with the
>> debug state in the next iteration of his patch.
> 
> I roughly see the discussion between James and Christoffer, Seems Christoffer 
> does not suggest save and
> restore it, and suggest to do below, and UAO/PAN may not use the same ways.
> 
>   __kvm_vcpu_run(struct kvm_vcpu *vcpu)
>   {
>   if (has_vhe())
>   asm("msr daifset, #0xf");
> 
>   ...
>  exit_code = __guest_enter(vcpu, host_ctxt);
>   ...
> 
>   if (has_vhe())
>   asm("msr daifclr, #0xd");
>   }
> 
> 
> If not save/restore them, the KVM will set them according to the CPU 
> capability. For example below fixing, it will check CPU capability, If CPU 
> supports PAN,
> the kvm will always enable the PAN for the host. But in fact, the host may be 
> not enable the PAN.

Can you please elaborate on cases where PAN is not enabled?

Thanks
Vladimir

> Of course for the UAO, we can use the similar fixing if Marc or Christoffer 
> is agreed. but seems not make sense.
> 
> commit cb96408da4e11698674abd04aeac941c1bed2038
> Author: Vladimir Murzin 
> Date:   Thu Sep 1 15:29:03 2016 +0100
> 
> arm64: KVM: VHE: reset PSTATE.PAN on entry to EL2
> 
> SCTLR_EL2.SPAN bit controls what happens with the PSTATE.PAN bit on an
> exception. However, this bit has no effect on the PSTATE.PAN when
> HCR_EL2.E2H or HCR_EL2.TGE is unset. Thus when VHE is used and
> exception taken from a guest PSTATE.PAN bit left unchanged and we
> continue with a value guest has set.
> 
> To address that always reset PSTATE.PAN on entry from EL1.
> 
> Fixes: 1f364c8c48a0 ("arm64: VHE: Add support for running Linux in EL2 
> mode")
> 
> Signed-off-by: Vladimir Murzin 
> Reviewed-by: James Morse 
> Acked-by: Marc Zyngier 
> Cc:  # v4.6+
> Signed-off-by: Christoffer Dall 
> 
> diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
> index 3967c231..b5926ee 100644
> --- a/arch/arm64/kvm/hyp/entry.S
> +++ b/arch/arm64/kvm/hyp/entry.S
> @@ -96,6 +96,8 @@ ENTRY(__guest_exit)
> 
> add x1, x1, #VCPU_CONTEXT
> 
> +   ALTERNATIVE(nop, SET_PSTATE_PAN(1), ARM64_HAS_PAN, CONFIG_ARM64_PAN)
> +
> // Store the guest regs x2 and x3
> stp x2, x3,   [x1, #CPU_XREG_OFFSET(2)]
> 
> 
>>
>> Thanks,
>>
>>  M.
>>
>> [1] https://www.spinics.net/lists/arm-kernel/msg599798.html
>>
> 
> 

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

Re: [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits

2017-09-06 Thread gengdongjiu

Hi Marc,

On 2017/9/6 16:17, Marc Zyngier wrote:
> On 05/09/17 19:58, gengdongjiu wrote:
>> when exit from guest, some host PSTATE bits may be lost, such as
>> PSTATE.PAN or PSTATE.UAO. It is because host and hypervisor all run
>> in the EL2, host PSTATE value cannot be saved and restored via
>> SPSR_EL2. So if guest has changed the PSTATE, host continues with
>> a wrong value guest has set.
>>
>> Signed-off-by: Dongjiu Geng 
>> Signed-off-by: Haibin Zhang 
>> ---
>>  arch/arm64/include/asm/kvm_host.h |  8 +++
>>  arch/arm64/include/asm/kvm_hyp.h  |  2 ++
>>  arch/arm64/include/asm/sysreg.h   | 23 +++
>>  arch/arm64/kvm/hyp/entry.S|  2 --
>>  arch/arm64/kvm/hyp/switch.c   | 24 ++--
>>  arch/arm64/kvm/hyp/sysreg-sr.c| 48 
>> ---
>>  6 files changed, 100 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/kvm_host.h 
>> b/arch/arm64/include/asm/kvm_host.h
>> index e923b58..cba7d3e 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -193,6 +193,12 @@ struct kvm_cpu_context {
>>  };
>>  };
>>  
>> +struct kvm_cpu_host_pstate {
>> +u64 daif;
>> +u64 uao;
>> +u64 pan;
>> +};
> 
> I love it. This is the most expensive way of saving/restoring a single
> 32bit value.
> 
> More seriously, please see the discussion between James and Christoffer
> there[1]. I expect James to address the PAN/UAO states together with the
> debug state in the next iteration of his patch.

I roughly see the discussion between James and Christoffer, Seems Christoffer 
does not suggest save and
restore it, and suggest to do below, and UAO/PAN may not use the same ways.

  __kvm_vcpu_run(struct kvm_vcpu *vcpu)
  {
  if (has_vhe())
  asm("msr daifset, #0xf");

...
 exit_code = __guest_enter(vcpu, host_ctxt);
...

if (has_vhe())
  asm("msr daifclr, #0xd");
  }


If not save/restore them, the KVM will set them according to the CPU 
capability. For example below fixing, it will check CPU capability, If CPU 
supports PAN,
the kvm will always enable the PAN for the host. But in fact, the host may be 
not enable the PAN.
Of course for the UAO, we can use the similar fixing if Marc or Christoffer is 
agreed. but seems not make sense.

commit cb96408da4e11698674abd04aeac941c1bed2038
Author: Vladimir Murzin 
Date:   Thu Sep 1 15:29:03 2016 +0100

arm64: KVM: VHE: reset PSTATE.PAN on entry to EL2

SCTLR_EL2.SPAN bit controls what happens with the PSTATE.PAN bit on an
exception. However, this bit has no effect on the PSTATE.PAN when
HCR_EL2.E2H or HCR_EL2.TGE is unset. Thus when VHE is used and
exception taken from a guest PSTATE.PAN bit left unchanged and we
continue with a value guest has set.

To address that always reset PSTATE.PAN on entry from EL1.

Fixes: 1f364c8c48a0 ("arm64: VHE: Add support for running Linux in EL2 
mode")

Signed-off-by: Vladimir Murzin 
Reviewed-by: James Morse 
Acked-by: Marc Zyngier 
Cc:  # v4.6+
Signed-off-by: Christoffer Dall 

diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
index 3967c231..b5926ee 100644
--- a/arch/arm64/kvm/hyp/entry.S
+++ b/arch/arm64/kvm/hyp/entry.S
@@ -96,6 +96,8 @@ ENTRY(__guest_exit)

add x1, x1, #VCPU_CONTEXT

+   ALTERNATIVE(nop, SET_PSTATE_PAN(1), ARM64_HAS_PAN, CONFIG_ARM64_PAN)
+
// Store the guest regs x2 and x3
stp x2, x3,   [x1, #CPU_XREG_OFFSET(2)]


> 
> Thanks,
> 
>   M.
> 
> [1] https://www.spinics.net/lists/arm-kernel/msg599798.html
> 

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

Re: [PATCH v3 51/59] KVM: arm/arm64: GICv4: Add doorbell interrupt handling

2017-09-06 Thread Shannon Zhao



On 2017/8/1 1:26, Marc Zyngier wrote:
> When a vPE is not running, a VLPI being made pending results in a
> doorbell interrupt being delivered. Let's handle this interrupt
> and update the pending_last flag that indicates that VLPIs are
> pending. The corresponding vcpu is also kicked into action.
> 
> Signed-off-by: Marc Zyngier 
> ---
>  virt/kvm/arm/vgic/vgic-v4.c | 34 ++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-v4.c b/virt/kvm/arm/vgic/vgic-v4.c
> index 534d3051a078..6af3cde6d7d4 100644
> --- a/virt/kvm/arm/vgic/vgic-v4.c
> +++ b/virt/kvm/arm/vgic/vgic-v4.c
> @@ -21,6 +21,19 @@
>  
>  #include "vgic.h"
>  
> +static irqreturn_t vgic_v4_doorbell_handler(int irq, void *info)
> +{
> + struct kvm_vcpu *vcpu = info;
> +
> + if (!kvm_vgic_vcpu_pending_irq(vcpu)) {
> + vcpu->arch.vgic_cpu.vgic_v3.its_vpe.pending_last = true;
> + kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);
> + kvm_vcpu_kick(vcpu);
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
>  int vgic_v4_init(struct kvm *kvm)
>  {
>   struct vgic_dist *dist = >arch.vgic;
> @@ -57,16 +70,37 @@ int vgic_v4_init(struct kvm *kvm)
>   return ret;
>   }
>  
> + kvm_for_each_vcpu(i, vcpu, kvm) {
> + int irq = dist->its_vm.vpes[i]->irq;
> +
> + ret = request_irq(irq, vgic_v4_doorbell_handler,
> +   0, "vcpu", vcpu);
> + if (ret) {
> + kvm_err("failed to allocate vcpu IRQ%d\n", irq);
> + dist->its_vm.nr_vpes = i;
This overwirtes the nr_vpes while it uses kvm->online_vcpus in
its_alloc_vcpu_irqs to alloc irqs and if this fails it uses the
overwirten nr_vpes other than kvm->online_vcpus in its_free_vcpu_irqs to
free the irqs. So there will be memory leak on error path.

> + break;
> + }
> + }
> +
> + if (ret)
> + vgic_v4_teardown(kvm);
> +
>   return ret;
>  }
>  
>  void vgic_v4_teardown(struct kvm *kvm)
>  {
>   struct its_vm *its_vm = >arch.vgic.its_vm;
> + int i;
>  
>   if (!its_vm->vpes)
>   return;
>  
> + for (i = 0; i < its_vm->nr_vpes; i++) {
> + struct kvm_vcpu *vcpu = kvm_get_vcpu(kvm, i);
> + free_irq(its_vm->vpes[i]->irq, vcpu);
> + }
> +
>   its_free_vcpu_irqs(its_vm);
>   kfree(its_vm->vpes);
>   its_vm->nr_vpes = 0;
> 

Thanks,
-- 
Shannon

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

Re: [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits

2017-09-06 Thread Marc Zyngier

On 05/09/17 19:58, gengdongjiu wrote:
> when exit from guest, some host PSTATE bits may be lost, such as
> PSTATE.PAN or PSTATE.UAO. It is because host and hypervisor all run
> in the EL2, host PSTATE value cannot be saved and restored via
> SPSR_EL2. So if guest has changed the PSTATE, host continues with
> a wrong value guest has set.
> 
> Signed-off-by: Dongjiu Geng 
> Signed-off-by: Haibin Zhang 
> ---
>  arch/arm64/include/asm/kvm_host.h |  8 +++
>  arch/arm64/include/asm/kvm_hyp.h  |  2 ++
>  arch/arm64/include/asm/sysreg.h   | 23 +++
>  arch/arm64/kvm/hyp/entry.S|  2 --
>  arch/arm64/kvm/hyp/switch.c   | 24 ++--
>  arch/arm64/kvm/hyp/sysreg-sr.c| 48 
> ---
>  6 files changed, 100 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h 
> b/arch/arm64/include/asm/kvm_host.h
> index e923b58..cba7d3e 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -193,6 +193,12 @@ struct kvm_cpu_context {
>   };
>  };
>  
> +struct kvm_cpu_host_pstate {
> + u64 daif;
> + u64 uao;
> + u64 pan;
> +};

I love it. This is the most expensive way of saving/restoring a single
32bit value.

More seriously, please see the discussion between James and Christoffer
there[1]. I expect James to address the PAN/UAO states together with the
debug state in the next iteration of his patch.

Thanks,

M.

[1] https://www.spinics.net/lists/arm-kernel/msg599798.html
-- 
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] KVM: arm64: vgic-its: Fix wrong return value check in vgic_its_restore_device_tables

2017-09-06 Thread Auger Eric

Hi Vijaya,

On 06/09/2017 07:26, Vijaya Kumar K wrote:
> scan_its_table() return 1 on success.

As mentioned in the kernel-doc comment of scan_its_table, this latter
returns 1 if the last element is not found. Than can happen while
scanning an L2 table but shouldn't happen if we scan an L1 table.

 * Return: < 0 on error, 0 if last element was identified, 1 otherwise
 * (the last element may not be found on second level tables)


 In the function vgic_its_restore_device_tables()
> the return value of scan_its_table() is checked against success value
> and returns -EINVAL. Hence migration fails for VM with ITS.
> 
> With this patch the failure return value is checked while returning
> -EINVAL.
> 
> Signed-off-by: Vijaya Kumar K 
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index aa6b68d..63f8ac3 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -2142,7 +2142,7 @@ static int vgic_its_restore_device_tables(struct 
> vgic_its *its)
>vgic_its_restore_dte, NULL);
>   }
>  
> - if (ret > 0)
> + if (ret <= 0)
>   ret = -EINVAL;
your modification would return -EINVAL for whatever error encountered
during the scan table or if last element is found. I don't think this is
what we want.

Thanks

Eric
>  
>   return ret;
> 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH] KVM: arm64: vgic-its: Fix wrong return value check in vgic_its_restore_device_tables

2017-09-06 Thread Vijaya Kumar K

scan_its_table() return 1 on success. In the function 
vgic_its_restore_device_tables()
the return value of scan_its_table() is checked against success value
and returns -EINVAL. Hence migration fails for VM with ITS.

With this patch the failure return value is checked while returning
-EINVAL.

Signed-off-by: Vijaya Kumar K 

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index aa6b68d..63f8ac3 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -2142,7 +2142,7 @@ static int vgic_its_restore_device_tables(struct vgic_its 
*its)
 vgic_its_restore_dte, NULL);
}
 
-   if (ret > 0)
+   if (ret <= 0)
ret = -EINVAL;
 
return ret;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a 
Linux Foundation Collaborative Project

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

40 matches

Mail list logo