Re: [PATCH] hw/arm/virt: KVM: The IPA lower bound is 32

[Marc Zyngier]

On Tue, 09 Mar 2021 20:06:06 +0000,
Andrew Jones <drjones@redhat.com> wrote:
> 
> On Tue, Mar 09, 2021 at 07:21:58PM +0000, Marc Zyngier wrote:
> > Hi Andrew,
> > 
> > On Tue, 09 Mar 2021 18:59:39 +0000,
> > Andrew Jones <drjones@redhat.com> wrote:
> > > 
> > > The virt machine already checks KVM_CAP_ARM_VM_IPA_SIZE to get the
> > > upper bound of the IPA size. If that bound is lower than the highest
> > > possible GPA for the machine, then QEMU will error out. However, the
> > > IPA is set to 40 when the highest GPA is less than or equal to 40,
> > > even when KVM may only support an IPA limit as low as 32. This means
> > > KVM may fail the VM creation unnecessarily. Additionally, 40 is
> > > selected with the value 0, which means use the default, and that gets
> > > around a check in some versions of KVM, causing a difficult to debug
> > > fail. Always use the IPA size that corresponds to the highest possible
> > > GPA, unless it's lower than 32, in which case use 32.
> > > 
> > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > ---
> > >  hw/arm/virt.c | 9 ++++-----
> > >  1 file changed, 4 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > index 371147f3ae9c..7bf563715b4e 100644
> > > --- a/hw/arm/virt.c
> > > +++ b/hw/arm/virt.c
> > > @@ -2547,14 +2547,13 @@ static int virt_kvm_type(MachineState *ms, const 
> > > char *type_str)
> > >                       "require an IPA range (%d bits) larger than "
> > >                       "the one supported by the host (%d bits)",
> > >                       requested_pa_size, max_vm_pa_size);
> > > -       exit(1);
> > > +        exit(1);
> > >      }
> > > +
> > >      /*
> > > -     * By default we return 0 which corresponds to an implicit legacy
> > > -     * 40b IPA setting. Otherwise we return the actual requested PA
> > > -     * logsize
> > > +     * KVM requires the IPA size to be at least 32 bits.
> > >       */
> > > -    return requested_pa_size > 40 ? requested_pa_size : 0;
> > > +    return requested_pa_size < 32 ? 32 : requested_pa_size;
> > 
> > Doesn't this break on older kernels (pre 233a7cb23531, which is
> > anything up 4.19), where the 'type' parameter passed at VM creation
> > time was expected to be 0 and nothing else?
> 
> Doh, of course!
> 
> > 
> > I had a quick go at qemu a couple of weeks back and came up with the
> > following hack, but never actually tested it (there is no way qemu can
> > fit in the initramfs I am feeding to this damn machine...).
> > 
> > Thanks,
> > 
> >     M.
> > 
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 371147f3ae..3301bb8dba 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -2534,8 +2534,11 @@ static HotplugHandler 
> > *virt_machine_get_hotplug_handler(MachineState *machine,
> >  static int virt_kvm_type(MachineState *ms, const char *type_str)
> >  {
> >      VirtMachineState *vms = VIRT_MACHINE(ms);
> > -    int max_vm_pa_size = kvm_arm_get_max_vm_ipa_size(ms);
> > +    int max_vm_pa_size;
> >      int requested_pa_size;
> > +    bool fixed_ipa;
> > +
> > +    max_vm_pa_size = kvm_arm_get_max_vm_ipa_size(ms, &fixed_ipa);
> >  
> >      /* we freeze the memory map to compute the highest gpa */
> >      virt_set_memmap(vms);
> > @@ -2550,11 +2553,12 @@ static int virt_kvm_type(MachineState *ms, const 
> > char *type_str)
> >         exit(1);
> >      }
> >      /*
> > -     * By default we return 0 which corresponds to an implicit legacy
> > -     * 40b IPA setting. Otherwise we return the actual requested PA
> > -     * logsize
> > +     * We return the requested PA log size, unless KVM only supports
> > +     * the implicit legacy 40b IPA setting, in which case we return 0.
> >       */
> > -    return requested_pa_size > 40 ? requested_pa_size : 0;
> > +    if (fixed_ipa)
> > +        return 0;
> > +    return requested_pa_size;
> 
> Looks good to me. I think we should still ensure requested_pa_size is at
> least 32 though.

Yes, absolutely.

> I can add that and test this tomorrow. Should I post it with your
> authorship?

No, please keep it with yours (add a Suggested-by: tag if you really
want to). If you can give it a good shake, that'd be great. I'll try
and repost the kernel fixes tomorrow.

Thanks,

        M.

-- 
Without deviation from the norm, progress is not possible.