| Message ID | 20210930075223.860329-8-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI devices passthrough on Arm, part 3 | expand |
On 30.09.2021 09:52, Oleksandr Andrushchenko wrote: > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > Take into account guest's BAR view and program its p2m accordingly: > gfn is guest's view of the BAR and mfn is the physical BAR value as set > up by the host bridge in the hardware domain. > This way hardware doamin sees physical BAR values and guest sees > emulated ones. > > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> Just a couple of nits, as I remain unconvinced of the rangeset related choice in the earlier patch. > @@ -37,12 +41,28 @@ static int map_range(unsigned long s, unsigned long e, void *data, > unsigned long *c) > { > const struct map_data *map = data; > + gfn_t start_gfn; > int rc; > > for ( ; ; ) > { > unsigned long size = e - s + 1; > > + /* > + * Any BAR may have holes in its memory we want to map, e.g. > + * we don't want to map MSI-X regions which may be a part of that BAR, > + * e.g. when a single BAR is used for both MMIO and MSI-X. This second "e.g." seems, to me at least, quite redundant with the first one. > + * In this case MSI-X regions are subtracted from the mapping, but > + * map->start_gfn still points to the very beginning of the BAR. > + * So if there is a hole present then we need to adjust start_gfn > + * to reflect the fact of that substraction. > + */ > + start_gfn = gfn_add(map->start_gfn, s - mfn_x(map->start_mfn)); > + > + printk(XENLOG_G_DEBUG Do you really mean this to be active even in release builds? Might get quite noisy ... > + "%smap [%lx, %lx] -> %#"PRI_gfn" for d%d\n", %pd please in new or altered code. Jan
On 01.10.21 16:38, Jan Beulich wrote: > On 30.09.2021 09:52, Oleksandr Andrushchenko wrote: >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> >> Take into account guest's BAR view and program its p2m accordingly: >> gfn is guest's view of the BAR and mfn is the physical BAR value as set >> up by the host bridge in the hardware domain. >> This way hardware doamin sees physical BAR values and guest sees >> emulated ones. >> >> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > Just a couple of nits, as I remain unconvinced of the rangeset related > choice in the earlier patch. > >> @@ -37,12 +41,28 @@ static int map_range(unsigned long s, unsigned long e, void *data, >> unsigned long *c) >> { >> const struct map_data *map = data; >> + gfn_t start_gfn; >> int rc; >> >> for ( ; ; ) >> { >> unsigned long size = e - s + 1; >> >> + /* >> + * Any BAR may have holes in its memory we want to map, e.g. >> + * we don't want to map MSI-X regions which may be a part of that BAR, >> + * e.g. when a single BAR is used for both MMIO and MSI-X. > This second "e.g." seems, to me at least, quite redundant with the first > one. Ok > >> + * In this case MSI-X regions are subtracted from the mapping, but >> + * map->start_gfn still points to the very beginning of the BAR. >> + * So if there is a hole present then we need to adjust start_gfn >> + * to reflect the fact of that substraction. >> + */ >> + start_gfn = gfn_add(map->start_gfn, s - mfn_x(map->start_mfn)); >> + >> + printk(XENLOG_G_DEBUG > Do you really mean this to be active even in release builds? Might get > quite noisy ... I can change this one to "gdprintk(XENLOG_G_DEBUG" and leave the below one as "printk(XENLOG_G_WARNING" Or you also mean the warning to be gdprintk? > >> + "%smap [%lx, %lx] -> %#"PRI_gfn" for d%d\n", > %pd please in new or altered code. Will change > > Jan > Thank you, Oleksandr
On Thu, Sep 30, 2021 at 10:52:19AM +0300, Oleksandr Andrushchenko wrote: > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > Take into account guest's BAR view and program its p2m accordingly: > gfn is guest's view of the BAR and mfn is the physical BAR value as set > up by the host bridge in the hardware domain. > This way hardware doamin sees physical BAR values and guest sees > emulated ones. > > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > --- > Since v2: > - improve readability for data.start_gfn and restructure ?: construct > Since v1: > - s/MSI/MSI-X in comments > --- > xen/drivers/vpci/header.c | 34 ++++++++++++++++++++++++++++++++-- > 1 file changed, 32 insertions(+), 2 deletions(-) > > diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c > index 9c603d26d302..f23c956cde6c 100644 > --- a/xen/drivers/vpci/header.c > +++ b/xen/drivers/vpci/header.c > @@ -30,6 +30,10 @@ > > struct map_data { > struct domain *d; > + /* Start address of the BAR as seen by the guest. */ > + gfn_t start_gfn; > + /* Physical start address of the BAR. */ > + mfn_t start_mfn; > bool map; > }; > > @@ -37,12 +41,28 @@ static int map_range(unsigned long s, unsigned long e, void *data, > unsigned long *c) > { > const struct map_data *map = data; > + gfn_t start_gfn; > int rc; > > for ( ; ; ) > { > unsigned long size = e - s + 1; > > + /* > + * Any BAR may have holes in its memory we want to map, e.g. > + * we don't want to map MSI-X regions which may be a part of that BAR, > + * e.g. when a single BAR is used for both MMIO and MSI-X. IMO there are too many 'e.g.' here. > + * In this case MSI-X regions are subtracted from the mapping, but > + * map->start_gfn still points to the very beginning of the BAR. > + * So if there is a hole present then we need to adjust start_gfn > + * to reflect the fact of that substraction. > + */ I would simply the comment a bit: /* * Ranges to be mapped don't always start at the BAR start address, as * there can be holes or partially consumed ranges. Account for the * offset of the current address from the BAR start. */ Apart from MSI-X related holes on x86 at least we support preemption here, which means a range could be partially mapped before yielding. > + start_gfn = gfn_add(map->start_gfn, s - mfn_x(map->start_mfn)); > + > + printk(XENLOG_G_DEBUG > + "%smap [%lx, %lx] -> %#"PRI_gfn" for d%d\n", > + map->map ? "" : "un", s, e, gfn_x(start_gfn), > + map->d->domain_id); > /* > * ARM TODOs: > * - On ARM whether the memory is prefetchable or not should be passed > @@ -52,8 +72,10 @@ static int map_range(unsigned long s, unsigned long e, void *data, > * - {un}map_mmio_regions doesn't support preemption. > */ > > - rc = map->map ? map_mmio_regions(map->d, _gfn(s), size, _mfn(s)) > - : unmap_mmio_regions(map->d, _gfn(s), size, _mfn(s)); > + rc = map->map ? map_mmio_regions(map->d, start_gfn, > + size, _mfn(s)) > + : unmap_mmio_regions(map->d, start_gfn, > + size, _mfn(s)); > if ( rc == 0 ) > { > *c += size; > @@ -69,6 +91,7 @@ static int map_range(unsigned long s, unsigned long e, void *data, > ASSERT(rc < size); > *c += rc; > s += rc; > + gfn_add(map->start_gfn, rc); I think increasing map->start_gfn is wrong here, as it would get out of sync with map->start_mfn then, and the calculations done to obtain start_gfn would then be wrong. > if ( general_preempt_check() ) > return -ERESTART; > } > @@ -149,6 +172,10 @@ bool vpci_process_pending(struct vcpu *v) > if ( !bar->mem ) > continue; > > + data.start_gfn = > + _gfn(PFN_DOWN(is_hardware_domain(v->vpci.pdev->domain) You can just use v->domain here. > + ? bar->addr : bar->guest_addr)); I would place the '?' in the line above, but that's just my taste. Thanks, Roger.
Hi, Roger! On 26.10.21 13:35, Roger Pau Monné wrote: > On Thu, Sep 30, 2021 at 10:52:19AM +0300, Oleksandr Andrushchenko wrote: >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> >> Take into account guest's BAR view and program its p2m accordingly: >> gfn is guest's view of the BAR and mfn is the physical BAR value as set >> up by the host bridge in the hardware domain. >> This way hardware doamin sees physical BAR values and guest sees >> emulated ones. >> >> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> >> --- >> Since v2: >> - improve readability for data.start_gfn and restructure ?: construct >> Since v1: >> - s/MSI/MSI-X in comments >> --- >> xen/drivers/vpci/header.c | 34 ++++++++++++++++++++++++++++++++-- >> 1 file changed, 32 insertions(+), 2 deletions(-) >> >> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c >> index 9c603d26d302..f23c956cde6c 100644 >> --- a/xen/drivers/vpci/header.c >> +++ b/xen/drivers/vpci/header.c >> @@ -30,6 +30,10 @@ >> >> struct map_data { >> struct domain *d; >> + /* Start address of the BAR as seen by the guest. */ >> + gfn_t start_gfn; >> + /* Physical start address of the BAR. */ >> + mfn_t start_mfn; >> bool map; >> }; >> >> @@ -37,12 +41,28 @@ static int map_range(unsigned long s, unsigned long e, void *data, >> unsigned long *c) >> { >> const struct map_data *map = data; >> + gfn_t start_gfn; >> int rc; >> >> for ( ; ; ) >> { >> unsigned long size = e - s + 1; >> >> + /* >> + * Any BAR may have holes in its memory we want to map, e.g. >> + * we don't want to map MSI-X regions which may be a part of that BAR, >> + * e.g. when a single BAR is used for both MMIO and MSI-X. > IMO there are too many 'e.g.' here. > >> + * In this case MSI-X regions are subtracted from the mapping, but >> + * map->start_gfn still points to the very beginning of the BAR. >> + * So if there is a hole present then we need to adjust start_gfn >> + * to reflect the fact of that substraction. >> + */ > I would simply the comment a bit: > > /* > * Ranges to be mapped don't always start at the BAR start address, as > * there can be holes or partially consumed ranges. Account for the > * offset of the current address from the BAR start. > */ > > Apart from MSI-X related holes on x86 at least we support preemption > here, which means a range could be partially mapped before yielding. Thank you, will use your comment which is shorter and still clear >> + start_gfn = gfn_add(map->start_gfn, s - mfn_x(map->start_mfn)); >> + >> + printk(XENLOG_G_DEBUG >> + "%smap [%lx, %lx] -> %#"PRI_gfn" for d%d\n", >> + map->map ? "" : "un", s, e, gfn_x(start_gfn), >> + map->d->domain_id); >> /* >> * ARM TODOs: >> * - On ARM whether the memory is prefetchable or not should be passed >> @@ -52,8 +72,10 @@ static int map_range(unsigned long s, unsigned long e, void *data, >> * - {un}map_mmio_regions doesn't support preemption. >> */ >> >> - rc = map->map ? map_mmio_regions(map->d, _gfn(s), size, _mfn(s)) >> - : unmap_mmio_regions(map->d, _gfn(s), size, _mfn(s)); >> + rc = map->map ? map_mmio_regions(map->d, start_gfn, >> + size, _mfn(s)) >> + : unmap_mmio_regions(map->d, start_gfn, >> + size, _mfn(s)); >> if ( rc == 0 ) >> { >> *c += size; >> @@ -69,6 +91,7 @@ static int map_range(unsigned long s, unsigned long e, void *data, >> ASSERT(rc < size); >> *c += rc; >> s += rc; >> + gfn_add(map->start_gfn, rc); > I think increasing map->start_gfn is wrong here, as it would get out > of sync with map->start_mfn then, and the calculations done to obtain > start_gfn would then be wrong. Indeed, will remove it > >> if ( general_preempt_check() ) >> return -ERESTART; >> } >> @@ -149,6 +172,10 @@ bool vpci_process_pending(struct vcpu *v) >> if ( !bar->mem ) >> continue; >> >> + data.start_gfn = >> + _gfn(PFN_DOWN(is_hardware_domain(v->vpci.pdev->domain) > You can just use v->domain here. Ok > >> + ? bar->addr : bar->guest_addr)); > I would place the '?' in the line above, but that's just my taste. Hmmm, this chunk was discussed before and this is the result of that discussion ;) So, I'll better keep it as is > > Thanks, Roger. Thank you, Oleksandr
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c index 9c603d26d302..f23c956cde6c 100644 --- a/xen/drivers/vpci/header.c +++ b/xen/drivers/vpci/header.c @@ -30,6 +30,10 @@ struct map_data { struct domain *d; + /* Start address of the BAR as seen by the guest. */ + gfn_t start_gfn; + /* Physical start address of the BAR. */ + mfn_t start_mfn; bool map; }; @@ -37,12 +41,28 @@ static int map_range(unsigned long s, unsigned long e, void *data, unsigned long *c) { const struct map_data *map = data; + gfn_t start_gfn; int rc; for ( ; ; ) { unsigned long size = e - s + 1; + /* + * Any BAR may have holes in its memory we want to map, e.g. + * we don't want to map MSI-X regions which may be a part of that BAR, + * e.g. when a single BAR is used for both MMIO and MSI-X. + * In this case MSI-X regions are subtracted from the mapping, but + * map->start_gfn still points to the very beginning of the BAR. + * So if there is a hole present then we need to adjust start_gfn + * to reflect the fact of that substraction. + */ + start_gfn = gfn_add(map->start_gfn, s - mfn_x(map->start_mfn)); + + printk(XENLOG_G_DEBUG + "%smap [%lx, %lx] -> %#"PRI_gfn" for d%d\n", + map->map ? "" : "un", s, e, gfn_x(start_gfn), + map->d->domain_id); /* * ARM TODOs: * - On ARM whether the memory is prefetchable or not should be passed @@ -52,8 +72,10 @@ static int map_range(unsigned long s, unsigned long e, void *data, * - {un}map_mmio_regions doesn't support preemption. */ - rc = map->map ? map_mmio_regions(map->d, _gfn(s), size, _mfn(s)) - : unmap_mmio_regions(map->d, _gfn(s), size, _mfn(s)); + rc = map->map ? map_mmio_regions(map->d, start_gfn, + size, _mfn(s)) + : unmap_mmio_regions(map->d, start_gfn, + size, _mfn(s)); if ( rc == 0 ) { *c += size; @@ -69,6 +91,7 @@ static int map_range(unsigned long s, unsigned long e, void *data, ASSERT(rc < size); *c += rc; s += rc; + gfn_add(map->start_gfn, rc); if ( general_preempt_check() ) return -ERESTART; } @@ -149,6 +172,10 @@ bool vpci_process_pending(struct vcpu *v) if ( !bar->mem ) continue; + data.start_gfn = + _gfn(PFN_DOWN(is_hardware_domain(v->vpci.pdev->domain) + ? bar->addr : bar->guest_addr)); + data.start_mfn = _mfn(PFN_DOWN(bar->addr)); rc = rangeset_consume_ranges(bar->mem, map_range, &data); if ( rc == -ERESTART ) @@ -194,6 +221,9 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev, if ( !bar->mem ) continue; + data.start_gfn = _gfn(PFN_DOWN(is_hardware_domain(d) + ? bar->addr : bar->guest_addr)); + data.start_mfn = _mfn(PFN_DOWN(bar->addr)); while ( (rc = rangeset_consume_ranges(bar->mem, map_range, &data)) == -ERESTART ) process_pending_softirqs();