Re: [PATCH v7 1/8] AMD/IOMMU: check / convert IVMD ranges for being / to be reserved
On 26.08.2021 14:31, Jan Beulich wrote:
> 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
>>> Reviewed-by: Paul Durrant
>>> ---
>>> 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.
Since I haven't heard back on any of the replies I gave to your comments,
I'm going to assume they were sufficient to address your concerns. I'm
therefore planning to put in the part of the series which has R-b perhaps
tomorrow (with the tiny adjustment(s) that I've made following your
comments, which iirc were just spelling issues). If you disagree, please
reply there.
As to the particular concern of yours towards visibility of error-like
log messages: There's e.g. a significant amount of pre-existing
'AMD_IOMMU_DEBUG("IVHD Error: ...", ...)'. Instead of introducing
inconsistencies here, I think I'll add a patch on top introducing
AMD_IOMMU_ERROR(), AMD_IOMMU_WARN(), and AMD_IOMMU_VERBOSE(), converting
present AMD_IOMMU_DEBUG() as I see fit (and then up for discussion).
Jan
Re: [PATCH v7 1/8] AMD/IOMMU: check / convert IVMD ranges for being / to be reserved
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
>> Reviewed-by: Paul Durrant
>> ---
>> 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
Re: [PATCH v7 1/8] AMD/IOMMU: check / convert IVMD ranges for being / to be reserved
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
> Reviewed-by: Paul Durrant
> ---
> 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.
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".
~Andrew
> +return -EIO;
> +}
> +}
> +
> if ( ivmd_block->header.flags & ACPI_IVMD_EXCLUSION_RANGE )
> exclusion = true;
> else if ( ivmd_block->header.flags & ACPI_IVMD_UNITY )
>
[PATCH v7 1/8] AMD/IOMMU: check / convert IVMD ranges for being / to be reserved
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
Reviewed-by: Paul Durrant
---
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);
+return -EIO;
+}
+}
+
if ( ivmd_block->header.flags & ACPI_IVMD_EXCLUSION_RANGE )
exclusion = true;
else if ( ivmd_block->header.flags & ACPI_IVMD_UNITY )
