| Message ID | 20210923125501.234252-8-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI devices passthrough on Arm, part 3 | expand |
Hi Oleksandr, On 23.09.2021 14:54, 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 v1: > - s/MSI/MSI-X in comments > --- > xen/drivers/vpci/header.c | 35 +++++++++++++++++++++++++++++++++-- > 1 file changed, 33 insertions(+), 2 deletions(-) > > diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c > index 9c603d26d302..bdd18599b205 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 = is_hardware_domain(v->vpci.pdev->domain) ? > + _gfn(PFN_DOWN(bar->addr)) : > + _gfn(PFN_DOWN(bar->guest_addr)); I believe this would look better with the following alignment: data.start_gfn = is_hardware_domain(v->vpci.pdev->domain) ? _gfn(PFN_DOWN(bar->addr)) : _gfn(PFN_DOWN(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,10 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev, > if ( !bar->mem ) > continue; > > + data.start_gfn = is_hardware_domain(d) ? > + _gfn(PFN_DOWN(bar->addr)) : > + _gfn(PFN_DOWN(bar->guest_addr)); This one also. > + data.start_mfn = _mfn(PFN_DOWN(bar->addr)); > while ( (rc = rangeset_consume_ranges(bar->mem, map_range, > &data)) == -ERESTART ) > process_pending_softirqs(); > Cheers, Michal
On 29.09.2021 10:13, Michal Orzel wrote: > On 23.09.2021 14:54, Oleksandr Andrushchenko wrote: >> @@ -149,6 +172,10 @@ bool vpci_process_pending(struct vcpu *v) >> if ( !bar->mem ) >> continue; >> >> + data.start_gfn = is_hardware_domain(v->vpci.pdev->domain) ? >> + _gfn(PFN_DOWN(bar->addr)) : >> + _gfn(PFN_DOWN(bar->guest_addr)); > I believe this would look better with the following alignment: > data.start_gfn = is_hardware_domain(v->vpci.pdev->domain) > ? _gfn(PFN_DOWN(bar->addr)) > : _gfn(PFN_DOWN(bar->guest_addr)); FWIW I agree, yet personally I think the conditional operator here even wants to move inside the _gfn(PFN_DOWN()). Jan
Hi, Michal! On 29.09.21 11:13, Michal Orzel wrote: > Hi Oleksandr, > > On 23.09.2021 14:54, 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 v1: >> - s/MSI/MSI-X in comments >> --- >> xen/drivers/vpci/header.c | 35 +++++++++++++++++++++++++++++++++-- >> 1 file changed, 33 insertions(+), 2 deletions(-) >> >> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c >> index 9c603d26d302..bdd18599b205 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 = is_hardware_domain(v->vpci.pdev->domain) ? >> + _gfn(PFN_DOWN(bar->addr)) : >> + _gfn(PFN_DOWN(bar->guest_addr)); > I believe this would look better with the following alignment: > data.start_gfn = is_hardware_domain(v->vpci.pdev->domain) > ? _gfn(PFN_DOWN(bar->addr)) > : _gfn(PFN_DOWN(bar->guest_addr)); I can change that if it improves readability >> + data.start_mfn = _mfn(PFN_DOWN(bar->addr)); >> rc = rangeset_consume_ranges(bar->mem, map_range, &data); >> >> if ( rc == -ERESTART ) >> @@ -194,6 +221,10 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev, >> if ( !bar->mem ) >> continue; >> >> + data.start_gfn = is_hardware_domain(d) ? >> + _gfn(PFN_DOWN(bar->addr)) : >> + _gfn(PFN_DOWN(bar->guest_addr)); > This one also. ditto >> + data.start_mfn = _mfn(PFN_DOWN(bar->addr)); >> while ( (rc = rangeset_consume_ranges(bar->mem, map_range, >> &data)) == -ERESTART ) >> process_pending_softirqs(); >> > Cheers, > Michal
On 29.09.21 11:16, Jan Beulich wrote: > On 29.09.2021 10:13, Michal Orzel wrote: >> On 23.09.2021 14:54, Oleksandr Andrushchenko wrote: >>> @@ -149,6 +172,10 @@ bool vpci_process_pending(struct vcpu *v) >>> if ( !bar->mem ) >>> continue; >>> >>> + data.start_gfn = is_hardware_domain(v->vpci.pdev->domain) ? >>> + _gfn(PFN_DOWN(bar->addr)) : >>> + _gfn(PFN_DOWN(bar->guest_addr)); >> I believe this would look better with the following alignment: >> data.start_gfn = is_hardware_domain(v->vpci.pdev->domain) >> ? _gfn(PFN_DOWN(bar->addr)) >> : _gfn(PFN_DOWN(bar->guest_addr)); > FWIW I agree, yet personally I think the conditional operator here > even wants to move inside the _gfn(PFN_DOWN()). I can do it as well: data.start_gfn = _gfn(PFN_DOWN(is_hardware_domain(v->vpci.pdev->domain) ? bar->addr : bar->guest_addr)) But, help me please breaking it into multiline with 80 chars respected > > Jan > Thank you, Oleksandr
On 29.09.2021 10:24, Oleksandr Andrushchenko wrote: > > On 29.09.21 11:16, Jan Beulich wrote: >> On 29.09.2021 10:13, Michal Orzel wrote: >>> On 23.09.2021 14:54, Oleksandr Andrushchenko wrote: >>>> @@ -149,6 +172,10 @@ bool vpci_process_pending(struct vcpu *v) >>>> if ( !bar->mem ) >>>> continue; >>>> >>>> + data.start_gfn = is_hardware_domain(v->vpci.pdev->domain) ? >>>> + _gfn(PFN_DOWN(bar->addr)) : >>>> + _gfn(PFN_DOWN(bar->guest_addr)); >>> I believe this would look better with the following alignment: >>> data.start_gfn = is_hardware_domain(v->vpci.pdev->domain) >>> ? _gfn(PFN_DOWN(bar->addr)) >>> : _gfn(PFN_DOWN(bar->guest_addr)); >> FWIW I agree, yet personally I think the conditional operator here >> even wants to move inside the _gfn(PFN_DOWN()). > > I can do it as well: > > data.start_gfn = _gfn(PFN_DOWN(is_hardware_domain(v->vpci.pdev->domain) ? bar->addr : bar->guest_addr)) > But, help me please breaking it into multiline with 80 chars respected Besides the option of latching v->vpci.pdev->domain or is_hardware_domain(v->vpci.pdev->domain) into a helper variable, data.start_gfn = _gfn(PFN_DOWN(is_hardware_domain(v->vpci.pdev->domain) ? bar->addr : bar->guest_addr)); or data.start_gfn = _gfn(PFN_DOWN(is_hardware_domain(v->vpci.pdev->domain) ? bar->addr : bar->guest_addr)); Jan
On 29.09.21 11:36, Jan Beulich wrote: > On 29.09.2021 10:24, Oleksandr Andrushchenko wrote: >> On 29.09.21 11:16, Jan Beulich wrote: >>> On 29.09.2021 10:13, Michal Orzel wrote: >>>> On 23.09.2021 14:54, Oleksandr Andrushchenko wrote: >>>>> @@ -149,6 +172,10 @@ bool vpci_process_pending(struct vcpu *v) >>>>> if ( !bar->mem ) >>>>> continue; >>>>> >>>>> + data.start_gfn = is_hardware_domain(v->vpci.pdev->domain) ? >>>>> + _gfn(PFN_DOWN(bar->addr)) : >>>>> + _gfn(PFN_DOWN(bar->guest_addr)); >>>> I believe this would look better with the following alignment: >>>> data.start_gfn = is_hardware_domain(v->vpci.pdev->domain) >>>> ? _gfn(PFN_DOWN(bar->addr)) >>>> : _gfn(PFN_DOWN(bar->guest_addr)); >>> FWIW I agree, yet personally I think the conditional operator here >>> even wants to move inside the _gfn(PFN_DOWN()). >> I can do it as well: >> >> data.start_gfn = _gfn(PFN_DOWN(is_hardware_domain(v->vpci.pdev->domain) ? bar->addr : bar->guest_addr)) >> But, help me please breaking it into multiline with 80 chars respected > Besides the option of latching v->vpci.pdev->domain or > is_hardware_domain(v->vpci.pdev->domain) into a helper variable, > > data.start_gfn = > _gfn(PFN_DOWN(is_hardware_domain(v->vpci.pdev->domain) > ? bar->addr : bar->guest_addr)); I'll go with this one, thank you > > or > > data.start_gfn = > _gfn(PFN_DOWN(is_hardware_domain(v->vpci.pdev->domain) > ? bar->addr > : bar->guest_addr)); > > Jan >
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c index 9c603d26d302..bdd18599b205 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 = is_hardware_domain(v->vpci.pdev->domain) ? + _gfn(PFN_DOWN(bar->addr)) : + _gfn(PFN_DOWN(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,10 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev, if ( !bar->mem ) continue; + data.start_gfn = is_hardware_domain(d) ? + _gfn(PFN_DOWN(bar->addr)) : + _gfn(PFN_DOWN(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();