Re: [kvm-unit-tests PATCH v1 2/7] x86 UEFI: Refactor set up process

[Zixuan Wang <zxwang42@gmail.com>]

On Fri, Nov 5, 2021 at 11:54 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Sat, Oct 30, 2021, Zixuan Wang wrote:
> > +efi_status_t setup_efi(efi_bootinfo_t *efi_bootinfo)
> >  {
> > +     efi_status_t status;
> > +
> > +     status = setup_memory_allocator(efi_bootinfo);
> > +     if (status != EFI_SUCCESS) {
> > +             printf("Failed to set up memory allocator: ");
> > +             switch (status) {
> > +             case EFI_OUT_OF_RESOURCES:
> > +                     printf("No free memory region\n");
> > +                     break;
> > +             default:
> > +                     printf("Unknown error\n");
> > +                     break;
> > +             }
> > +             return status;
> > +     }
> > +
> > +     status = setup_rsdp(efi_bootinfo);
> > +     if (status != EFI_SUCCESS) {
> > +             printf("Cannot find RSDP in EFI system table\n");
> > +             return status;
> > +     }
> > +
> > +     status = setup_amd_sev();
> > +     if (status != EFI_SUCCESS) {
> > +             switch (status) {
> > +             case EFI_UNSUPPORTED:
> > +                     /* Continue if AMD SEV is not supported */
> > +                     break;
> > +             default:
> > +                     printf("Set up AMD SEV failed\n");
> > +                     return status;
> > +             }
> > +     }
>
> Looks like this is pre-existing behavior, but the switch is quite gratuituous,
> and arguably does the wrong thing for EFI_UNSUPPORTED here as attempting to setup
> SEV-ES without SEV is guaranteed to fail.  And it'd be really nice if the printf()
> actually provided the error (below might be wrong, I don't know the type of
> efi_status-t).
>
>         status = setup_amd_sev();
>
>         /* Continue on if AMD SEV isn't supported, but skip SEV-ES setup. */
>         if (status == EFI_UNSUPPORTED)
>                 goto continue_setup;
>
>         if (status != EFI_SUCCESS) {
>                 printf("AMD SEV setup failed, error = %d\n", status);
>                 return status;
>         }
>
>         /* Same as above, lack of SEV-ES is not a fatal error. */
>         status = setup_amd_sev_es();
>         if (status != EFI_SUCCESS && status != EFI_UNSUPPORTED) {
>                 printf("AMD SEV-ES setup failed, error = %d\n", status);
>                 return status;
>         }
>
> continue_setup:
>

I agree. The current setup_amd_sev_es() checks if SEV is available and
returns EFI_UNSUPPORTED if not, so it does no harm if called after
setup_amd_sev() fails. But I think we should not rely on these
underlying implementation details. I will include this part in the
next version.

Best regards,
Zixuan