Re: [PATCH v7 1/8] AMD/IOMMU: check / convert IVMD ranges for being / to be reserved

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.08.2021 14:10, Andrew Cooper wrote:
> On 26/08/2021 08:23, Jan Beulich wrote:
>> While the specification doesn't say so, just like for VT-d's RMRRs no
>> good can come from these ranges being e.g. conventional RAM or entirely
>> unmarked and hence usable for placing e.g. PCI device BARs. Check
>> whether they are, and put in some limited effort to convert to reserved.
>> (More advanced logic can be added if actual problems are found with this
>> simplistic variant.)
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> Reviewed-by: Paul Durrant <paul@xxxxxxx>
>> ---
>> v7: Re-base.
>> v5: New.
>>
>> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
>> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
>> @@ -384,6 +384,38 @@ static int __init parse_ivmd_block(const
>>      AMD_IOMMU_DEBUG("IVMD Block: type %#x phys %#lx len %#lx\n",
>>                      ivmd_block->header.type, start_addr, mem_length);
>>  
>> +    if ( !e820_all_mapped(base, limit + PAGE_SIZE, E820_RESERVED) )
>> +    {
>> +        paddr_t addr;
>> +
>> +        AMD_IOMMU_DEBUG("IVMD: [%lx,%lx) is not (entirely) in reserved 
>> memory\n",
>> +                        base, limit + PAGE_SIZE);
>> +
>> +        for ( addr = base; addr <= limit; addr += PAGE_SIZE )
>> +        {
>> +            unsigned int type = page_get_ram_type(maddr_to_mfn(addr));
>> +
>> +            if ( type == RAM_TYPE_UNKNOWN )
>> +            {
>> +                if ( e820_add_range(&e820, addr, addr + PAGE_SIZE,
>> +                                    E820_RESERVED) )
>> +                    continue;
>> +                AMD_IOMMU_DEBUG("IVMD Error: Page at %lx couldn't be 
>> reserved\n",
>> +                                addr);
>> +                return -EIO;
>> +            }
>> +
>> +            /* Types which won't be handed out are considered good enough. 
>> */
>> +            if ( !(type & (RAM_TYPE_RESERVED | RAM_TYPE_ACPI |
>> +                           RAM_TYPE_UNUSABLE)) )
>> +                continue;
>> +
>> +            AMD_IOMMU_DEBUG("IVMD Error: Page at %lx can't be converted\n",
>> +                            addr);
> 
> I think these print messages need to more than just debug.  The first
> one is a warning, whereas the final two are hard errors liable to impact
> the correct running of the system.

Well, people would observe IOMMUs not getting put in use. I was following
existing style in this regard on the assumption that in such an event
people would (be told to) enable "iommu=debug". Hence ...

> Especially as you're putting them in to try and spot problem cases, they
> should be visible by default for when we inevitably get bug reports to
> xen-devel saying "something changed with passthrough in Xen 4.16".

... I can convert to ordinary printk(), provided you're convinced the
described model isn't reasonable and introducing a logging inconsistency
is worth it.

Jan