Re: [PATCH V7 01/11] xen/pci: arm: add stub for is_memory_hole

[Oleksandr <olekstysh@xxxxxxxxx>]

Hello Rahul


On 19.07.22 20:42, Oleksandr Tyshchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>
> Add a stub for is_memory_hole which is required for PCI passthrough
> on Arm.
>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> ---
> OT: It looks like the discussion got stuck. As I understand this
> patch is not immediately needed in the context of current series
> as PCI passthrough is not enabled on Arm at the moment. So the patch
> could be added later on, but it is needed to allow PCI passthrough
> to be built on Arm for those who want to test it.
>
> Copy here some context provided by Julien:
>
> Here a summary of the discussion (+ some my follow-up thoughts):
>
> is_memory_hole() was recently introduced on x86 (see commit 75cc460a1b8c
> "xen/pci: detect when BARs are not suitably positioned") to check
> whether the BAR are positioned outside of a valid memory range. This was
> introduced to work-around quirky firmware.
>
> In theory, this could also happen on Arm. In practice, this may not
> happen but it sounds better to sanity check that the BAR contains
> "valid" I/O range.
>
> On x86, this is implemented by checking the region is not described is
> in the e820. IIUC, on Arm, the BARs have to be positioned in pre-defined
> ranges. So I think it would be possible to implement is_memory_hole() by
> going through the list of hostbridges and check the ranges.
>
> But first, I'd like to confirm my understanding with Rahul, and others.

May I please ask about your opinion on that?


> If we were going to go this route, I would also rename the function to
> be better match what it is doing (i.e. it checks the BAR is correctly
> placed). As a potentially optimization/hardening for Arm, we could pass
> the hostbridge so we don't have to walk all of them.
> ---
>   xen/arch/arm/mm.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 009b8cd9ef..bb34b97eb5 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1708,6 +1708,12 @@ unsigned long get_upper_mfn_bound(void)
>       return max_page - 1;
>   }
>   
> +bool is_memory_hole(mfn_t start, mfn_t end)
> +{
> +    /* TODO: this needs to be properly implemented. */
> +    return true;
> +}
> +
>   /*
>    * Local variables:
>    * mode: C
--
Regards,

Oleksandr Tyshchenko