Re: [PATCH v6 01/13] xen/pci: arm: add stub for is_memory_hole
On 04.02.22 13:00, Julien Grall wrote: > > > On 04/02/2022 10:35, Oleksandr Andrushchenko wrote: >> >> >> On 04.02.22 11:57, Julien Grall wrote: >>> Hi, >>> >>> On 04/02/2022 09:47, Oleksandr Andrushchenko wrote: >> 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 >>> I expect the function to be fully implemented before this is will be merged. >>> >>> So if it is fully implemented, then a fair chunk of what I wrote would not >>> be necessary to carry in the commit message. >> Well, we started from that we want *something* with TODO and now >> you request it to be fully implemented before it is merged. > > I don't think I ever suggested this patch would be merged as-is. Sorry if > this may have crossed like this. Np > > Instead, my intent by asking you to send a TODO patch is to start a > discussion how this function could be implemented for Arm. > > You sent a TODO but you didn't provide any summary on what is the issue, what > we want to achieve... Hence my request to add a bit more details so the other > reviewers can provide their opinion more easily. Ok, so we can discuss it here, but I won't have this patch in v7 > > Cheers, > Thank you, Oleksandr
Re: [PATCH v6 01/13] xen/pci: arm: add stub for is_memory_hole
On 04/02/2022 10:35, Oleksandr Andrushchenko wrote: On 04.02.22 11:57, Julien Grall wrote: Hi, On 04/02/2022 09:47, Oleksandr Andrushchenko wrote: 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 I expect the function to be fully implemented before this is will be merged. So if it is fully implemented, then a fair chunk of what I wrote would not be necessary to carry in the commit message. Well, we started from that we want *something* with TODO and now you request it to be fully implemented before it is merged. I don't think I ever suggested this patch would be merged as-is. Sorry if this may have crossed like this. Instead, my intent by asking you to send a TODO patch is to start a discussion how this function could be implemented for Arm. You sent a TODO but you didn't provide any summary on what is the issue, what we want to achieve... Hence my request to add a bit more details so the other reviewers can provide their opinion more easily. Cheers, -- Julien Grall
Re: [PATCH v6 01/13] xen/pci: arm: add stub for is_memory_hole
On 04.02.22 11:57, Julien Grall wrote: > Hi, > > On 04/02/2022 09:47, Oleksandr Andrushchenko wrote: 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 > I expect the function to be fully implemented before this is will be merged. > > So if it is fully implemented, then a fair chunk of what I wrote would not be > necessary to carry in the commit message. Well, we started from that we want *something* with TODO and now you request it to be fully implemented before it is merged. What do I miss here? > > Cheers, > Thank you, Oleksandr
Re: [PATCH v6 01/13] xen/pci: arm: add stub for is_memory_hole
Hi, On 04/02/2022 09:47, Oleksandr Andrushchenko wrote: 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 I expect the function to be fully implemented before this is will be merged. So if it is fully implemented, then a fair chunk of what I wrote would not be necessary to carry in the commit message. Cheers, -- Julien Grall
Re: [PATCH v6 01/13] xen/pci: arm: add stub for is_memory_hole
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
Add a stub for is_memory_hole which is required for PCI passthrough
on Arm.
Signed-off-by: Oleksandr Andrushchenko
---
Cc: Julien Grall
Cc: Stefano Stabellini
---
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
Re: [PATCH v6 01/13] xen/pci: arm: add stub for is_memory_hole
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
Add a stub for is_memory_hole which is required for PCI passthrough
on Arm.
Signed-off-by: Oleksandr Andrushchenko
---
Cc: Julien Grall
Cc: Stefano Stabellini
---
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?
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.
Cheers,
--
Julien Grall
Re: [PATCH v6 01/13] xen/pci: arm: add stub for is_memory_hole
Hi, Julien!
On 04.02.22 10:51, Julien Grall wrote:
> Hi,
>
> On 04/02/2022 06:34, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko
>>
>> Add a stub for is_memory_hole which is required for PCI passthrough
>> on Arm.
>>
>> Signed-off-by: Oleksandr Andrushchenko
>>
>> ---
>> Cc: Julien Grall
>> Cc: Stefano Stabellini
>> ---
>> 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
Could you please help me with the exact message you would like to see?
>
>> + return true;
>> +}
>> +
>> /*
>> * Local variables:
>> * mode: C
>
Thank you,
Oleksandr
Re: [PATCH v6 01/13] xen/pci: arm: add stub for is_memory_hole
Hi,
On 04/02/2022 06:34, Oleksandr Andrushchenko wrote:
From: Oleksandr Andrushchenko
Add a stub for is_memory_hole which is required for PCI passthrough
on Arm.
Signed-off-by: Oleksandr Andrushchenko
---
Cc: Julien Grall
Cc: Stefano Stabellini
---
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.
+return true;
+}
+
/*
* Local variables:
* mode: C
--
Julien Grall
[PATCH v6 01/13] xen/pci: arm: add stub for is_memory_hole
From: Oleksandr Andrushchenko
Add a stub for is_memory_hole which is required for PCI passthrough
on Arm.
Signed-off-by: Oleksandr Andrushchenko
---
Cc: Julien Grall
Cc: Stefano Stabellini
---
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. */
+return true;
+}
+
/*
* Local variables:
* mode: C
--
2.25.1
