Hi Ricardo,
On 9/9/21 8:22 PM, Ricardo Koller wrote:
> On Thu, Sep 09, 2021 at 03:54:31PM +0200, Eric Auger wrote:
>> Hi Ricardo,
>>
>> On 9/8/21 11:03 PM, Ricardo Koller wrote:
>>> This test attempts (and fails) to set a redistributor region using the
>>> legacy KVM_VGIC_V3_ADDR_TYPE_REDIST that's partially above the
>>> VM-specified IPA size.
>>>
>>> Signed-off-by: Ricardo Koller <ricarkol@xxxxxxxxxx>
>>> ---
>>> .../testing/selftests/kvm/aarch64/vgic_init.c | 44 +++++++++++++++++++
>>> 1 file changed, 44 insertions(+)
>>>
>>> diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c
>>> index 623f31a14326..6dd7b5e91421 100644
>>> --- a/tools/testing/selftests/kvm/aarch64/vgic_init.c
>>> +++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
>>> @@ -285,6 +285,49 @@ static void test_vcpus_then_vgic(void)
>>> vm_gic_destroy(&v);
>>> }
>>>
>>> +static void test_redist_above_vm_pa_bits(enum vm_guest_mode mode)
>>> +{
>>> + struct vm_gic v;
>>> + int ret, i;
>>> + uint32_t vcpuids[] = { 1, 2, 3, 4, };
>>> + int pa_bits = vm_guest_mode_params[mode].pa_bits;
>>> + uint64_t addr, psize = 1ULL << pa_bits;
>>> +
>>> + /* Add vcpu 1 */
>>> + v.vm = vm_create_with_vcpus(mode, 1, DEFAULT_GUEST_PHY_PAGES,
>>> + 0, 0, guest_code, vcpuids);
>>> + v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
>>> +
>>> + /* Set space for half a redist, we have 1 vcpu, so this fails. */
>>> + addr = psize - 0x10000;
>>> + ret = _kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>>> + KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
>>> + TEST_ASSERT(ret && errno == EINVAL, "not enough space for one redist");
>>> +
>>> + /* Set space for 3 redists, we have 1 vcpu, so this succeeds. */
>>> + addr = psize - (3 * 2 * 0x10000);
>>> + kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>>> + KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
>> I think you need to test both the old API (KVM_VGIC_V3_ADDR_TYPE_REDIST)
>> and the new one (KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION).
>>
>> Can't you integrate those new checks in existing tests,
>> subtest_redist_regions() and subtest_dist_rdist() which already tests
>> base addr beyond IPA limit (but not range end unfortunately). look for
>> E2BIG.
>>
> Had some issues adapting subtest_dist_rdist() as the IPA range check for
> ADDR_TYPE_REDIST is done at 1st vcpu run. subtest_dist_rdist() is
> already used to set overlapping dist/redist regions, which is then
> checked to generate EINVAL on 1st vcpu run. If subtest_dist_rdist() is
> also used to set the redist region above phys_size, then there won't be
> a way of checking that the vcpu run fails because of both the overlap
> and IPA issue. It was simpler and cleaner to just add a new function
> for the ADDR_TYPE_REDIST IPA range test. Will adapt
OK I see, then effectively adding a new test was more straightforward.
> subtest_redist_regions() as the check for ADDR_TYPE_REDIST_REGION can be
> done when setting the regions.
OK
>
> Related Question:
>
> Both the KVM_VGIC_V3_ADDR_TYPE_REDIST and KVM_RUN currently return
> EINVAL with my proposed change (not E2BIG). I will change
> KVM_VGIC_V3_ADDR_TYPE_REDIST to fail with E2BIG, but will leave KVM_RUN
> failing with EINVAL. Would you say that's the correct behavior?
This looks OK to me, as long as the KVM uapi doc documents is aligned.
Thanks
Eric
>
> Thanks,
> Ricardo
>
>> Thanks
>>
>> Eric
>>> +
>>> + addr = 0x00000;
>>> + kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>>> + KVM_VGIC_V3_ADDR_TYPE_DIST, &addr, true);
>>> +
>>> + /* Add three vcpus (2, 3, 4). */
>>> + for (i = 1; i < 4; ++i)
>>> + vm_vcpu_add_default(v.vm, vcpuids[i], guest_code);
>>> +
>>> + kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
>>> + KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
>>> +
>>> + /* Attempt to run a vcpu without enough redist space. */
>>> + ret = run_vcpu(v.vm, vcpuids[3]);
>>> + TEST_ASSERT(ret && errno == EINVAL,
>>> + "redist base+size above IPA detected on 1st vcpu run");
>>> +
>>> + vm_gic_destroy(&v);
>>> +}
>>> +
>>> static void test_new_redist_regions(void)
>>> {
>>> void *dummy = NULL;
>>> @@ -542,6 +585,7 @@ int main(int ac, char **av)
>>> test_kvm_device();
>>> test_vcpus_then_vgic();
>>> test_vgic_then_vcpus();
>>> + test_redist_above_vm_pa_bits(VM_MODE_DEFAULT);
>>> test_new_redist_regions();
>>> test_typer_accesses();
>>> test_last_bit_redist_regions();
