| Message ID | 20220204063459.680961-2-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PCI devices passthrough on Arm, part 3 | expand |
Hi, On 04/02/2022 06:34, Oleksandr Andrushchenko wrote: > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > Add a stub for is_memory_hole which is required for PCI passthrough > on Arm. > > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > --- > Cc: Julien Grall <julien@xen.org> > Cc: Stefano Stabellini <sstabellini@kernel.org> > --- > 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
Hi, Julien! On 04.02.22 10:51, Julien Grall wrote: > Hi, > > On 04/02/2022 06:34, Oleksandr Andrushchenko wrote: >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> >> Add a stub for is_memory_hole which is required for PCI passthrough >> on Arm. >> >> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> >> --- >> Cc: Julien Grall <julien@xen.org> >> Cc: Stefano Stabellini <sstabellini@kernel.org> >> --- >> 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
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@epam.com> >>> >>> Add a stub for is_memory_hole which is required for PCI passthrough >>> on Arm. >>> >>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >>> >>> --- >>> Cc: Julien Grall <julien@xen.org> >>> Cc: Stefano Stabellini <sstabellini@kernel.org> >>> --- >>> 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,
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@epam.com> >>>> >>>> Add a stub for is_memory_hole which is required for PCI passthrough >>>> on Arm. >>>> >>>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >>>> >>>> --- >>>> Cc: Julien Grall <julien@xen.org> >>>> Cc: Stefano Stabellini <sstabellini@kernel.org> >>>> --- >>>> 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
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,
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
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,
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
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