Re: [PATCH v6 01/13] xen/pci: arm: add stub for is_memory_hole

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 04.02.22 11:41, Julien Grall wrote:
> On 04/02/2022 09:01, Oleksandr Andrushchenko wrote:
>> On 04.02.22 10:51, Julien Grall wrote:
>>> Hi,
>>>
>>> On 04/02/2022 06:34, Oleksandr Andrushchenko 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>
>>>>
>>>> ---
>>>> Cc: Julien Grall <julien@xxxxxxx>
>>>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>>>> ---
>>>> New in v6
>>>> ---
>>>>    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 b1eae767c27c..c32e34a182a2 100644
>>>> --- a/xen/arch/arm/mm.c
>>>> +++ b/xen/arch/arm/mm.c
>>>> @@ -1640,6 +1640,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. */
>>>
>>> I was hoping to see a summary of the discussion from IRC somewhere in the 
>>> patch (maybe after ---). This would help to bring up to speed the others 
>>> that were not on IRC.
>> I am not quite sure what needs to be put here as the summary
>
> At least some details on why this is a TODO. Is it because you are unsure of 
> the implementation? Is it because you wanted to send early?...
>
> IOW, what are you expecting from the reviewers?
Well, I just need to allow PCI passthrough to be built on Arm at the moment.
Clearly, without this stub I can't do so. This is the only intention now.
Of course, while PCI passthrough on Arm is still not really enabled those
who want trying it will need reverting the offending patch otherwise.
I am fine both ways
>
>> Could you please help me with the exact message you would like to see?
>
> 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.
>
> 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.
It seems this needs to live in the commit message then? So, it is easy to find
as everything after "---" is going to be dropped on commit
>
> Cheers,
>
Thank you,
Oleksandr