Re: [PATCH for-4.17] x86/pci: allow BARs to be positioned on e820 reserved regions

[Jan Beulich <jbeulich@suse.com>]

On 05.10.2022 16:12, Roger Pau Monné wrote:
> Hm, I have the following diff attached below which is more limited,
> and not so bad I think.  Initially I wanted to introduce an
> efi_all_mapped() helper, but that would require exposing EFI_MEMORY_TYPE
> which is quite intrusive.
> 
> Let me know if you think the proposal below is better and I will
> formally send a patch with it.

Hmm, personally I like this slightly better for, as you say, its more
limited effect. Objectively, however, I'm still unconvinced of making
this an EFI special case. How would non-EFI firmware go about
communicating that it is going to access a device at runtime (which,
as said before, I consider a no-go in the first place)? Likely by
putting its BAR range(s) in an E820_RESERVED region.

Plus the MMIO range covered on the system in question is pretty large.
That way we're still allowing pretty wide an abuse by the firmware.
Furthermore the MCFG range would also be covered by an
EfiMemoryMappedIO descriptor (in fact that's the only use of the type
I had been aware of so far). IOW the change specifically permits an
overlap of a BAR with an MCFG range.

Who's the manufacturer of the system? Or put in different words - how
likely is it that we could first gain understanding on their
intentions with this region? You did say the system hangs hard without
some kind of workaround, but that doesn't clarify to me in how far a
use of the device by the firmware was actually involved there.

Have you considered other routes towards dealing with the issue? One
approach coming to mind would build on top of what you've been doing
so far (either variant): Besides avoiding the turning off of memory
decode, also invoke pci_ro_device(), thus protecting it from having
its BARs relocated, and also preventing any driver in Dom0 to gain
control of it, thus avoiding two parties competing for the device.

Relocating the BAR outside of the reserved region would be nice, but
will likely not resolve the hang.

In any event I'm still hoping to have a 3rd view on the situation as a
whole, irrespective of specific ideas towards possible workarounds ...

Independent of the above a couple of purely cosmetic comments:

> @@ -98,3 +99,28 @@ int pci_conf_write_intercept(unsigned int seg, unsigned int bdf,
>  
>      return rc;
>  }
> +
> +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
> +{
> +    /*
> +     * Check if BAR is not overlapping with any memory region defined
> +     * in the memory map.
> +     */
> +    if ( is_memory_hole(start, end) )
> +        return true;
> +
> +    /*
> +     * Also allow BARs placed on EfiMemoryMappedIO regions in order to deal
> +     * with EFI firmware using those regions to place the BARs of devices that
> +     * can be used during runtime.  But print a warning when doing so.
> +     */
> +    if ( !efi_all_runtime_mmio(mfn_to_maddr(start),
> +                               mfn_to_maddr(mfn_add(end, 1))) )
> +        return false;
> +
> +    printk(XENLOG_WARNING
> +           "%pp: BAR [%#" PRI_mfn ", %#" PRI_mfn "] overlaps reserved region\n",
> +           &pdev->sbdf, mfn_x(start), mfn_x(end));

Perhaps re-word the message now that the check is a different one?

> --- a/xen/common/efi/runtime.c
> +++ b/xen/common/efi/runtime.c
> @@ -78,6 +78,30 @@ bool efi_enabled(unsigned int feature)
>      return test_bit(feature, &efi_flags);
>  }
>  
> +/*
> + * This function checks if the entire range [start,end) is contained inside of
> + * a single EfiMemoryMappedIO descriptor with the runtime attribute set.
> + */
> +bool efi_all_runtime_mmio(uint64_t start, uint64_t end)
> +{
> +    unsigned int i;
> +
> +    for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
> +    {
> +        EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;

const?

> +        uint64_t len = desc->NumberOfPages << EFI_PAGE_SHIFT;
> +
> +        if ( desc->Type != EfiMemoryMappedIO ||
> +             !(desc->Attribute & EFI_MEMORY_RUNTIME) )
> +            continue;
> +
> +        if ( start >= desc->PhysicalStart && end <= desc->PhysicalStart + len )
> +            return true;
> +    }
> +
> +    return false;
> +}
> +
>  #ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */

Perhaps put the function inside this #ifdef?

Jan