| Message ID | 20201212185010.26579-6-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM/ARM: Some vgic fixes and init sequence KVM selftests | expand |
Hi Eric, On 12/12/20 6:50 PM, Eric Auger wrote: > has_run_once is set to true at the beginning of > kvm_vcpu_first_run_init(). This generally is not an issue > except when exercising the code with KVM selftests. Indeed, > if kvm_vgic_map_resources() fails due to erroneous user settings, > has_run_once is set and this prevents from continuing > executing the test. This patch moves the assignment after the > kvm_vgic_map_resources(). > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > --- > arch/arm64/kvm/arm.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > index c0ffb019ca8b..331fae6bff31 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -540,8 +540,6 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu) > if (!kvm_arm_vcpu_is_finalized(vcpu)) > return -EPERM; > > - vcpu->arch.has_run_once = true; > - > if (likely(irqchip_in_kernel(kvm))) { > /* > * Map the VGIC hardware resources before running a vcpu the > @@ -560,6 +558,8 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu) > static_branch_inc(&userspace_irqchip_in_use); > } > > + vcpu->arch.has_run_once = true; I have a few concerns regarding this: 1. Moving has_run_once = true here seems very arbitrary to me - kvm_timer_enable() and kvm_arm_pmu_v3_enable(), below it, can both fail because of erroneous user values. If there's a reason why the assignment cannot be moved at the end of the function, I think it should be clearly stated in a comment for the people who might be tempted to write similar tests for the timer or pmu. 2. There are many ways that kvm_vgic_map_resources() can fail, other than incorrect user settings. I started digging into how kvm_vgic_map_resources()->vgic_v2_map_resources() can fail for a VGIC V2 and this is what I managed to find before I gave up: * vgic_init() can fail in: - kvm_vgic_dist_init() - vgic_v3_init() - kvm_vgic_setup_default_irq_routing() * vgic_register_dist_iodev() can fail in: - vgic_v3_init_dist_iodev() - kvm_io_bus_register_dev()(*) * kvm_phys_addr_ioremap() can fail in: - kvm_mmu_topup_memory_cache() - kvm_pgtable_stage2_map() So if any of the functions below fail, are we 100% sure it is safe to allow the user to execute kvm_vgic_map_resources() again? (*) It looks to me like kvm_io_bus_register_dev() doesn't take into account a caller that tries to register the same device address range and it will create another identical range. Is this intentional? Is it a bug that should be fixed? Or am I misunderstanding the function? Thanks, Alex > + > ret = kvm_timer_enable(vcpu); > if (ret) > return ret;
Hi Alexandru, On 1/12/21 3:55 PM, Alexandru Elisei wrote: > Hi Eric, > > On 12/12/20 6:50 PM, Eric Auger wrote: >> has_run_once is set to true at the beginning of >> kvm_vcpu_first_run_init(). This generally is not an issue >> except when exercising the code with KVM selftests. Indeed, >> if kvm_vgic_map_resources() fails due to erroneous user settings, >> has_run_once is set and this prevents from continuing >> executing the test. This patch moves the assignment after the >> kvm_vgic_map_resources(). >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> --- >> arch/arm64/kvm/arm.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c >> index c0ffb019ca8b..331fae6bff31 100644 >> --- a/arch/arm64/kvm/arm.c >> +++ b/arch/arm64/kvm/arm.c >> @@ -540,8 +540,6 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu) >> if (!kvm_arm_vcpu_is_finalized(vcpu)) >> return -EPERM; >> >> - vcpu->arch.has_run_once = true; >> - >> if (likely(irqchip_in_kernel(kvm))) { >> /* >> * Map the VGIC hardware resources before running a vcpu the >> @@ -560,6 +558,8 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu) >> static_branch_inc(&userspace_irqchip_in_use); >> } >> >> + vcpu->arch.has_run_once = true; > > I have a few concerns regarding this: > > 1. Moving has_run_once = true here seems very arbitrary to me - kvm_timer_enable() > and kvm_arm_pmu_v3_enable(), below it, can both fail because of erroneous user > values. If there's a reason why the assignment cannot be moved at the end of the > function, I think it should be clearly stated in a comment for the people who > might be tempted to write similar tests for the timer or pmu. Setting has_run_once = true at the entry of the function looks to me even more arbitrary. I agree with you that eventually has_run_once may be moved at the very end but maybe this can be done later once timer, pmu tests haven ben written > > 2. There are many ways that kvm_vgic_map_resources() can fail, other than > incorrect user settings. I started digging into how > kvm_vgic_map_resources()->vgic_v2_map_resources() can fail for a VGIC V2 and this > is what I managed to find before I gave up: > > * vgic_init() can fail in: > - kvm_vgic_dist_init() > - vgic_v3_init() > - kvm_vgic_setup_default_irq_routing() > * vgic_register_dist_iodev() can fail in: > - vgic_v3_init_dist_iodev() > - kvm_io_bus_register_dev()(*) > * kvm_phys_addr_ioremap() can fail in: > - kvm_mmu_topup_memory_cache() > - kvm_pgtable_stage2_map() I changed the commit msg so that "incorrect user settings" sounds as an example. > > So if any of the functions below fail, are we 100% sure it is safe to allow the > user to execute kvm_vgic_map_resources() again? I think additional tests will confirm this. However at the moment, moving the assignment, which does not look wrong to me, allows to greatly simplify the tests so I would tend to say that it is worth. > > (*) It looks to me like kvm_io_bus_register_dev() doesn't take into account a > caller that tries to register the same device address range and it will create > another identical range. Is this intentional? Is it a bug that should be fixed? Or > am I misunderstanding the function? doesn't kvm_io_bus_cmp() do the check? Thanks Eric > > Thanks, > Alex >> + >> ret = kvm_timer_enable(vcpu); >> if (ret) >> return ret; >
Hi Eric, On 1/14/21 10:02 AM, Auger Eric wrote: > Hi Alexandru, > > On 1/12/21 3:55 PM, Alexandru Elisei wrote: >> Hi Eric, >> >> On 12/12/20 6:50 PM, Eric Auger wrote: >>> has_run_once is set to true at the beginning of >>> kvm_vcpu_first_run_init(). This generally is not an issue >>> except when exercising the code with KVM selftests. Indeed, >>> if kvm_vgic_map_resources() fails due to erroneous user settings, >>> has_run_once is set and this prevents from continuing >>> executing the test. This patch moves the assignment after the >>> kvm_vgic_map_resources(). >>> >>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>> --- >>> arch/arm64/kvm/arm.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c >>> index c0ffb019ca8b..331fae6bff31 100644 >>> --- a/arch/arm64/kvm/arm.c >>> +++ b/arch/arm64/kvm/arm.c >>> @@ -540,8 +540,6 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu) >>> if (!kvm_arm_vcpu_is_finalized(vcpu)) >>> return -EPERM; >>> >>> - vcpu->arch.has_run_once = true; >>> - >>> if (likely(irqchip_in_kernel(kvm))) { >>> /* >>> * Map the VGIC hardware resources before running a vcpu the >>> @@ -560,6 +558,8 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu) >>> static_branch_inc(&userspace_irqchip_in_use); >>> } >>> >>> + vcpu->arch.has_run_once = true; >> I have a few concerns regarding this: >> >> 1. Moving has_run_once = true here seems very arbitrary to me - kvm_timer_enable() >> and kvm_arm_pmu_v3_enable(), below it, can both fail because of erroneous user >> values. If there's a reason why the assignment cannot be moved at the end of the >> function, I think it should be clearly stated in a comment for the people who >> might be tempted to write similar tests for the timer or pmu. > Setting has_run_once = true at the entry of the function looks to me > even more arbitrary. I agree with you that eventually has_run_once may Or it could be it's there to prevent the user from calling kvm_vgic_map_resources() a second time after it failed. This is what I'm concerned about and I think deserves more investigation. Thanks, Alex > be moved at the very end but maybe this can be done later once timer, > pmu tests haven ben written >> 2. There are many ways that kvm_vgic_map_resources() can fail, other than >> incorrect user settings. I started digging into how >> kvm_vgic_map_resources()->vgic_v2_map_resources() can fail for a VGIC V2 and this >> is what I managed to find before I gave up: >> >> * vgic_init() can fail in: >> - kvm_vgic_dist_init() >> - vgic_v3_init() >> - kvm_vgic_setup_default_irq_routing() >> * vgic_register_dist_iodev() can fail in: >> - vgic_v3_init_dist_iodev() >> - kvm_io_bus_register_dev()(*) >> * kvm_phys_addr_ioremap() can fail in: >> - kvm_mmu_topup_memory_cache() >> - kvm_pgtable_stage2_map() > I changed the commit msg so that "incorrect user settings" sounds as an > example. >> So if any of the functions below fail, are we 100% sure it is safe to allow the >> user to execute kvm_vgic_map_resources() again? > I think additional tests will confirm this. However at the moment, > moving the assignment, which does not look wrong to me, allows to > greatly simplify the tests so I would tend to say that it is worth. >> (*) It looks to me like kvm_io_bus_register_dev() doesn't take into account a >> caller that tries to register the same device address range and it will create >> another identical range. Is this intentional? Is it a bug that should be fixed? Or >> am I misunderstanding the function? > doesn't kvm_io_bus_cmp() do the check? > > Thanks > > Eric >> Thanks, >> Alex >>> + >>> ret = kvm_timer_enable(vcpu); >>> if (ret) >>> return ret;
Hi Alexandru, On 1/20/21 4:56 PM, Alexandru Elisei wrote: > Hi Eric, > > On 1/14/21 10:02 AM, Auger Eric wrote: >> Hi Alexandru, >> >> On 1/12/21 3:55 PM, Alexandru Elisei wrote: >>> Hi Eric, >>> >>> On 12/12/20 6:50 PM, Eric Auger wrote: >>>> has_run_once is set to true at the beginning of >>>> kvm_vcpu_first_run_init(). This generally is not an issue >>>> except when exercising the code with KVM selftests. Indeed, >>>> if kvm_vgic_map_resources() fails due to erroneous user settings, >>>> has_run_once is set and this prevents from continuing >>>> executing the test. This patch moves the assignment after the >>>> kvm_vgic_map_resources(). >>>> >>>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>>> --- >>>> arch/arm64/kvm/arm.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c >>>> index c0ffb019ca8b..331fae6bff31 100644 >>>> --- a/arch/arm64/kvm/arm.c >>>> +++ b/arch/arm64/kvm/arm.c >>>> @@ -540,8 +540,6 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu) >>>> if (!kvm_arm_vcpu_is_finalized(vcpu)) >>>> return -EPERM; >>>> >>>> - vcpu->arch.has_run_once = true; >>>> - >>>> if (likely(irqchip_in_kernel(kvm))) { >>>> /* >>>> * Map the VGIC hardware resources before running a vcpu the >>>> @@ -560,6 +558,8 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu) >>>> static_branch_inc(&userspace_irqchip_in_use); >>>> } >>>> >>>> + vcpu->arch.has_run_once = true; >>> I have a few concerns regarding this: >>> >>> 1. Moving has_run_once = true here seems very arbitrary to me - kvm_timer_enable() >>> and kvm_arm_pmu_v3_enable(), below it, can both fail because of erroneous user >>> values. If there's a reason why the assignment cannot be moved at the end of the >>> function, I think it should be clearly stated in a comment for the people who >>> might be tempted to write similar tests for the timer or pmu. >> Setting has_run_once = true at the entry of the function looks to me >> even more arbitrary. I agree with you that eventually has_run_once may > > Or it could be it's there to prevent the user from calling > kvm_vgic_map_resources() a second time after it failed. This is what I'm concerned > about and I think deserves more investigation. I have reworked my kvm selftests to live without that change. Thanks Eric > > Thanks, > Alex >> be moved at the very end but maybe this can be done later once timer, >> pmu tests haven ben written >>> 2. There are many ways that kvm_vgic_map_resources() can fail, other than >>> incorrect user settings. I started digging into how >>> kvm_vgic_map_resources()->vgic_v2_map_resources() can fail for a VGIC V2 and this >>> is what I managed to find before I gave up: >>> >>> * vgic_init() can fail in: >>> - kvm_vgic_dist_init() >>> - vgic_v3_init() >>> - kvm_vgic_setup_default_irq_routing() >>> * vgic_register_dist_iodev() can fail in: >>> - vgic_v3_init_dist_iodev() >>> - kvm_io_bus_register_dev()(*) >>> * kvm_phys_addr_ioremap() can fail in: >>> - kvm_mmu_topup_memory_cache() >>> - kvm_pgtable_stage2_map() >> I changed the commit msg so that "incorrect user settings" sounds as an >> example. >>> So if any of the functions below fail, are we 100% sure it is safe to allow the >>> user to execute kvm_vgic_map_resources() again? >> I think additional tests will confirm this. However at the moment, >> moving the assignment, which does not look wrong to me, allows to >> greatly simplify the tests so I would tend to say that it is worth. >>> (*) It looks to me like kvm_io_bus_register_dev() doesn't take into account a >>> caller that tries to register the same device address range and it will create >>> another identical range. Is this intentional? Is it a bug that should be fixed? Or >>> am I misunderstanding the function? >> doesn't kvm_io_bus_cmp() do the check? >> >> Thanks >> >> Eric >>> Thanks, >>> Alex >>>> + >>>> ret = kvm_timer_enable(vcpu); >>>> if (ret) >>>> return ret; >
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index c0ffb019ca8b..331fae6bff31 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -540,8 +540,6 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu) if (!kvm_arm_vcpu_is_finalized(vcpu)) return -EPERM; - vcpu->arch.has_run_once = true; - if (likely(irqchip_in_kernel(kvm))) { /* * Map the VGIC hardware resources before running a vcpu the @@ -560,6 +558,8 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu) static_branch_inc(&userspace_irqchip_in_use); } + vcpu->arch.has_run_once = true; + ret = kvm_timer_enable(vcpu); if (ret) return ret;
has_run_once is set to true at the beginning of kvm_vcpu_first_run_init(). This generally is not an issue except when exercising the code with KVM selftests. Indeed, if kvm_vgic_map_resources() fails due to erroneous user settings, has_run_once is set and this prevents from continuing executing the test. This patch moves the assignment after the kvm_vgic_map_resources(). Signed-off-by: Eric Auger <eric.auger@redhat.com> --- arch/arm64/kvm/arm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)