On 7/11/22 14:03, Igor Mammedov wrote:
> On Fri, 1 Jul 2022 17:10:14 +0100
> Joao Martins <joao.m.mart...@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.
>>
>> Relocating RAM regions to after the 1Tb hole has consequences for
>> guest ABI because we are changing the memory mapping, so make
>> sure that only new machine enforce but not older ones.
>>
>> Signed-off-by: Joao Martins <joao.m.mart...@oracle.com>
>> ---
>> hw/i386/pc.c | 6 ++++--
>> hw/i386/pc_piix.c | 2 ++
>> hw/i386/pc_q35.c | 2 ++
>> include/hw/i386/pc.h | 1 +
>> 4 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 07025b510540..f99e16a5db4b 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1013,9 +1013,10 @@ void pc_memory_init(PCMachineState *pcms,
>> /*
>> * The HyperTransport range close to the 1T boundary is unique to AMD
>> * hosts with IOMMUs enabled. Restrict the ram-above-4g relocation
>> - * to above 1T to AMD vCPUs only.
>> + * to above 1T to AMD vCPUs only. @enforce_valid_iova is only false in
>> + * older machine types (<= 7.0) for compatibility purposes.
>> */
>> - if (IS_AMD_CPU(&cpu->env)) {
>> + if (IS_AMD_CPU(&cpu->env) && pcmc->enforce_valid_iova) {
>> pc_set_amd_above_4g_mem_start(pcms, pci_hole64_size);
>>
>> /*
>> @@ -1950,6 +1951,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;
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index f3c726e42400..504ddd0deece 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -444,9 +444,11 @@ DEFINE_I440FX_MACHINE(v7_1, "pc-i440fx-7.1", NULL,
>>
>> static void pc_i440fx_7_0_machine_options(MachineClass *m)
>> {
>> + PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>> pc_i440fx_7_1_machine_options(m);
>> m->alias = NULL;
>> m->is_default = false;
>> + pcmc->enforce_valid_iova = false;
>> compat_props_add(m->compat_props, hw_compat_7_0, hw_compat_7_0_len);
>> compat_props_add(m->compat_props, pc_compat_7_0, pc_compat_7_0_len);
>> }
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index 5a4a737fe203..4b747c59c19a 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -381,8 +381,10 @@ DEFINE_Q35_MACHINE(v7_1, "pc-q35-7.1", NULL,
>>
>> static void pc_q35_7_0_machine_options(MachineClass *m)
>> {
>> + PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>> pc_q35_7_1_machine_options(m);
>> m->alias = NULL;
>> + pcmc->enforce_valid_iova = false;
>> compat_props_add(m->compat_props, hw_compat_7_0, hw_compat_7_0_len);
>> compat_props_add(m->compat_props, pc_compat_7_0, pc_compat_7_0_len);
>> }
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 568c226d3034..3a873ff69499 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -118,6 +118,7 @@ struct PCMachineClass {
>> bool has_reserved_memory;
>> bool enforce_aligned_dimm;
>> bool broken_reserved_end;
>> + bool enforce_valid_iova;
>
> maybe
> s/enforce_valid_iova/enforce_amd_1tb_hole/
> to be less ambiguous
>
That's much better, let me change the name into that.
> otherwise looks good to me so
> Acked-by: Igor Mammedov <imamm...@redhat.com>
>
Thanks!
