Re: [PATCH RFC 6/6] i386/pc: Add a machine property for AMD-only enforcing of valid IOVAs

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 22 Jun 2021 16:49:05 +0100
Joao Martins <joao.m.martins@oracle.com> wrote:

> The added enforcing is only relevant in the case of AMD where the range
> right before the 1TB is restricted and cannot be DMA mapped by the
> kernel consequently leading to IOMMU INVALID_DEVICE_REQUEST or possibly
> other kinds of IOMMU events in the AMD IOMMU.
> 
> Although, there's a case where it may make sense to disable the IOVA
> relocation/validation when migrating from a non-valid-IOVA-aware qemu to
> one that supports it.

we can keep old machines broken, that's what you are doing in
 *_machine_options() but for new machine it can be unconditionally
enforced.
So I miss the point of having user visible knob to disable that.
(it's not like one would be able to find a new machine type that
do not support new memory layout)
I'd drop user visible property and keep only hunks need for
*_machine_options().
 
> Relocating RAM regions to after the 1Tb hole has consequences for guest
> ABI because we are changing the memory mapping, and thus it may make
> sense to allow admin to disable the validation (e.g. upon migration) to
> either 1) Fail early when the VFIO DMA_MAP ioctl fails thus preventing
> the migration to happen 'gracefully' or 2) allow booting a guest
> unchanged from source host without risking changing the PCI mmio hole64
> or other things we consider in the valid IOVA range changing underneath
> the guest.
> 
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>  hw/i386/pc.c         | 29 +++++++++++++++++++++++++++--
>  hw/i386/pc_piix.c    |  2 ++
>  hw/i386/pc_q35.c     |  2 ++
>  include/hw/i386/pc.h |  2 ++
>  4 files changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 65885cc16037..eb08a6d1a2b9 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -902,10 +902,14 @@ struct GPARange usable_iova_ranges[] = {
>  
>   uint32_t nb_iova_ranges = DEFAULT_NR_USABLE_IOVAS;
>  
> -static void init_usable_iova_ranges(void)
> +static void init_usable_iova_ranges(PCMachineClass *pcmc)
>  {
>      uint32_t eax, vendor[3];
>  
> +    if (!pcmc->enforce_valid_iova) {
> +        return;
> +    }
> +
>      host_cpuid(0x0, 0, &eax, &vendor[0], &vendor[2], &vendor[1]);
>      if (IS_AMD_VENDOR(vendor)) {
>          usable_iova_ranges[0].end = AMD_MAX_PHYSADDR_BELOW_1TB;
> @@ -1000,7 +1004,7 @@ void pc_memory_init(PCMachineState *pcms,
>      assert(machine->ram_size == x86ms->below_4g_mem_size +
>                                  x86ms->above_4g_mem_size);
>  
> -    init_usable_iova_ranges();
> +    init_usable_iova_ranges(pcmc);
>  
>      linux_boot = (machine->kernel_filename != NULL);
>  
> @@ -1685,6 +1689,23 @@ static void pc_machine_set_hpet(Object *obj, bool value, Error **errp)
>      pcms->hpet_enabled = value;
>  }
>  
> +static bool pc_machine_get_enforce_valid_iova(Object *obj, Error **errp)
> +{
> +    PCMachineState *pcms = PC_MACHINE(obj);
> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> +
> +    return pcmc->enforce_valid_iova;
> +}
> +
> +static void pc_machine_set_enforce_valid_iova(Object *obj, bool value,
> +                                                  Error **errp)
> +{
> +    PCMachineState *pcms = PC_MACHINE(obj);
> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> +
> +    pcmc->enforce_valid_iova = value;
> +}
> +
>  static void pc_machine_get_max_ram_below_4g(Object *obj, Visitor *v,
>                                              const char *name, void *opaque,
>                                              Error **errp)
> @@ -1851,6 +1872,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      pcmc->has_reserved_memory = true;
>      pcmc->kvmclock_enabled = true;
>      pcmc->enforce_aligned_dimm = true;
> +    pcmc->enforce_valid_iova = true;
>      /* BIOS ACPI tables: 128K. Other BIOS datastructures: less than 4K reported
>       * to be used at the moment, 32K should be enough for a while.  */
>      pcmc->acpi_data_size = 0x20000 + 0x8000;
> @@ -1913,6 +1935,9 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>          NULL, NULL);
>      object_class_property_set_description(oc, PC_MACHINE_MAX_FW_SIZE,
>          "Maximum combined firmware size");
> +
> +    object_class_property_add_bool(oc, PC_MACHINE_ENFORCE_VALID_IOVA,
> +        pc_machine_get_enforce_valid_iova, pc_machine_set_enforce_valid_iova);
>  }
>  
>  static const TypeInfo pc_machine_info = {
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 30b8bd6ea92d..21a08e2f6a4c 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -427,11 +427,13 @@ DEFINE_I440FX_MACHINE(v6_1, "pc-i440fx-6.1", NULL,
>  
>  static void pc_i440fx_6_0_machine_options(MachineClass *m)
>  {
> +    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>      pc_i440fx_6_1_machine_options(m);
>      m->alias = NULL;
>      m->is_default = false;
>      compat_props_add(m->compat_props, hw_compat_6_0, hw_compat_6_0_len);
>      compat_props_add(m->compat_props, pc_compat_6_0, pc_compat_6_0_len);
> +    pcmc->enforce_valid_iova = false;
>  }
>  
>  DEFINE_I440FX_MACHINE(v6_0, "pc-i440fx-6.0", NULL,
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 46a0f196f413..80bb89a9bae1 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -357,10 +357,12 @@ DEFINE_Q35_MACHINE(v6_1, "pc-q35-6.1", NULL,
>  
>  static void pc_q35_6_0_machine_options(MachineClass *m)
>  {
> +    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>      pc_q35_6_1_machine_options(m);
>      m->alias = NULL;
>      compat_props_add(m->compat_props, hw_compat_6_0, hw_compat_6_0_len);
>      compat_props_add(m->compat_props, pc_compat_6_0, pc_compat_6_0_len);
> +    pcmc->enforce_valid_iova = false;
>  }
>  
>  DEFINE_Q35_MACHINE(v6_0, "pc-q35-6.0", NULL,
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index b924aef3a218..7337f6f2d014 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -63,6 +63,7 @@ typedef struct PCMachineState {
>  #define PC_MACHINE_SATA             "sata"
>  #define PC_MACHINE_PIT              "pit"
>  #define PC_MACHINE_MAX_FW_SIZE      "max-fw-size"
> +#define PC_MACHINE_ENFORCE_VALID_IOVA  "enforce-valid-iova"
>  /**
>   * PCMachineClass:
>   *
> @@ -113,6 +114,7 @@ struct PCMachineClass {
>      bool has_reserved_memory;
>      bool enforce_aligned_dimm;
>      bool broken_reserved_end;
> +    bool enforce_valid_iova;
>  
>      /* generate legacy CPU hotplug AML */
>      bool legacy_cpu_hotplug;