| Message ID | 20211125110251.2877218-9-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PCI devices passthrough on Arm, part 3 | expand |
On Thu, Nov 25, 2021 at 01:02:45PM +0200, 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 PCI bus driver in the hardware domain. > This way hardware domain sees physical BAR values and guest sees > emulated ones. > > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > --- > Since v4: > - moved start_{gfn|mfn} calculation into map_range > - pass vpci_bar in the map_data instead of start_{gfn|mfn} > - s/guest_addr/guest_reg > Since v3: > - updated comment (Roger) > - removed gfn_add(map->start_gfn, rc); which is wrong > - use v->domain instead of v->vpci.pdev->domain > - removed odd e.g. in comment > - s/d%d/%pd in altered code > - use gdprintk for map/unmap logs > Since v2: > - improve readability for data.start_gfn and restructure ?: construct > Since v1: > - s/MSI/MSI-X in comments > > --- > --- > xen/drivers/vpci/header.c | 30 ++++++++++++++++++++++++++---- > 1 file changed, 26 insertions(+), 4 deletions(-) > > diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c > index cc49aa68886f..b0499d32c5d8 100644 > --- a/xen/drivers/vpci/header.c > +++ b/xen/drivers/vpci/header.c > @@ -30,6 +30,7 @@ > > struct map_data { > struct domain *d; > + const struct vpci_bar *bar; > bool map; > }; > > @@ -41,8 +42,25 @@ static int map_range(unsigned long s, unsigned long e, void *data, > > for ( ; ; ) > { > + /* Start address of the BAR as seen by the guest. */ > + gfn_t start_gfn = _gfn(PFN_DOWN(is_hardware_domain(map->d) > + ? map->bar->addr > + : map->bar->guest_reg)); > + /* Physical start address of the BAR. */ > + mfn_t start_mfn = _mfn(PFN_DOWN(map->bar->addr)); > unsigned long size = e - s + 1; > > + /* > + * 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. > + */ > + start_gfn = gfn_add(start_gfn, s - mfn_x(start_mfn)); When doing guests mappings the rangeset should represent the guest physical memory space, not the host one. So that collisions in the guest p2m can be avoided. Also a guest should be allowed to map the same mfn into multiple gfn. For example multiple BARs could share the same physical page on the host and the guest might like to map them at different pages in it's physmap. > + > + gdprintk(XENLOG_G_DEBUG, > + "%smap [%lx, %lx] -> %#"PRI_gfn" for %pd\n", > + map->map ? "" : "un", s, e, gfn_x(start_gfn), > + map->d); That's too chatty IMO, I could be fine with printing something along this lines from modify_bars, but not here because that function can be preempted and called multiple times. > /* > * ARM TODOs: > * - On ARM whether the memory is prefetchable or not should be passed > @@ -52,8 +70,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; > @@ -62,8 +82,8 @@ static int map_range(unsigned long s, unsigned long e, void *data, > if ( rc < 0 ) > { > printk(XENLOG_G_WARNING > - "Failed to identity %smap [%lx, %lx] for d%d: %d\n", > - map->map ? "" : "un", s, e, map->d->domain_id, rc); > + "Failed to identity %smap [%lx, %lx] for %pd: %d\n", > + map->map ? "" : "un", s, e, map->d, rc); You need to adjust the message here, as this is no longer an identity map for domUs. Thanks, Roger.
Hi, Roger! On 13.01.22 12:22, Roger Pau Monné wrote: > On Thu, Nov 25, 2021 at 01:02:45PM +0200, 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 PCI bus driver in the hardware domain. >> This way hardware domain sees physical BAR values and guest sees >> emulated ones. >> >> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> --- >> Since v4: >> - moved start_{gfn|mfn} calculation into map_range >> - pass vpci_bar in the map_data instead of start_{gfn|mfn} >> - s/guest_addr/guest_reg >> Since v3: >> - updated comment (Roger) >> - removed gfn_add(map->start_gfn, rc); which is wrong >> - use v->domain instead of v->vpci.pdev->domain >> - removed odd e.g. in comment >> - s/d%d/%pd in altered code >> - use gdprintk for map/unmap logs >> Since v2: >> - improve readability for data.start_gfn and restructure ?: construct >> Since v1: >> - s/MSI/MSI-X in comments >> >> --- >> --- >> xen/drivers/vpci/header.c | 30 ++++++++++++++++++++++++++---- >> 1 file changed, 26 insertions(+), 4 deletions(-) >> >> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c >> index cc49aa68886f..b0499d32c5d8 100644 >> --- a/xen/drivers/vpci/header.c >> +++ b/xen/drivers/vpci/header.c >> @@ -30,6 +30,7 @@ >> >> struct map_data { >> struct domain *d; >> + const struct vpci_bar *bar; >> bool map; >> }; >> >> @@ -41,8 +42,25 @@ static int map_range(unsigned long s, unsigned long e, void *data, >> >> for ( ; ; ) >> { >> + /* Start address of the BAR as seen by the guest. */ >> + gfn_t start_gfn = _gfn(PFN_DOWN(is_hardware_domain(map->d) >> + ? map->bar->addr >> + : map->bar->guest_reg)); >> + /* Physical start address of the BAR. */ >> + mfn_t start_mfn = _mfn(PFN_DOWN(map->bar->addr)); >> unsigned long size = e - s + 1; >> >> + /* >> + * 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. >> + */ >> + start_gfn = gfn_add(start_gfn, s - mfn_x(start_mfn)); > When doing guests mappings the rangeset should represent the guest > physical memory space, not the host one. So, it does > So that collisions in the > guest p2m can be avoided. Also a guest should be allowed to map the > same mfn into multiple gfn. For example multiple BARs could share the > same physical page on the host and the guest might like to map them at > different pages in it's physmap. There is no such restriction imposed > >> + >> + gdprintk(XENLOG_G_DEBUG, >> + "%smap [%lx, %lx] -> %#"PRI_gfn" for %pd\n", >> + map->map ? "" : "un", s, e, gfn_x(start_gfn), >> + map->d); > That's too chatty IMO, I could be fine with printing something along > this lines from modify_bars, but not here because that function can be > preempted and called multiple times. Ok, will move to modify_bars as these prints are really helpful for debug > >> /* >> * ARM TODOs: >> * - On ARM whether the memory is prefetchable or not should be passed >> @@ -52,8 +70,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; >> @@ -62,8 +82,8 @@ static int map_range(unsigned long s, unsigned long e, void *data, >> if ( rc < 0 ) >> { >> printk(XENLOG_G_WARNING >> - "Failed to identity %smap [%lx, %lx] for d%d: %d\n", >> - map->map ? "" : "un", s, e, map->d->domain_id, rc); >> + "Failed to identity %smap [%lx, %lx] for %pd: %d\n", >> + map->map ? "" : "un", s, e, map->d, rc); > You need to adjust the message here, as this is no longer an identity > map for domUs. Sure > > Thanks, Roger. Thank you, Oleksandr
>>> + gdprintk(XENLOG_G_DEBUG, >>> + "%smap [%lx, %lx] -> %#"PRI_gfn" for %pd\n", >>> + map->map ? "" : "un", s, e, gfn_x(start_gfn), >>> + map->d); >> That's too chatty IMO, I could be fine with printing something along >> this lines from modify_bars, but not here because that function can be >> preempted and called multiple times. > Ok, will move to modify_bars as these prints are really helpful for debug I tried to implement the same, but now in init_bars: diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c index 667c04cee3ae..92407e617609 100644 --- a/xen/drivers/vpci/header.c +++ b/xen/drivers/vpci/header.c @@ -57,10 +57,6 @@ static int map_range(unsigned long s, unsigned long e, void *data, */ start_gfn = gfn_add(start_gfn, s - mfn_x(start_mfn)); - gdprintk(XENLOG_G_DEBUG, - "%smap [%lx, %lx] -> %#"PRI_gfn" for %pd\n", - map->map ? "" : "un", s, e, gfn_x(start_gfn), - map->d); /* * ARM TODOs: * - On ARM whether the memory is prefetchable or not should be passed @@ -258,6 +254,28 @@ static void defer_map(struct domain *d, struct pci_dev *pdev, raise_softirq(SCHEDULE_SOFTIRQ); } +static int print_range(unsigned long s, unsigned long e, void *data) +{ + const struct map_data *map = data; + + for ( ; ; ) + { + gfn_t start_gfn = _gfn(PFN_DOWN(is_hardware_domain(map->d) + ? map->bar->addr + : map->bar->guest_reg)); + mfn_t start_mfn = _mfn(PFN_DOWN(map->bar->addr)); + + start_gfn = gfn_add(start_gfn, s - mfn_x(start_mfn)); + + gdprintk(XENLOG_G_DEBUG, + "%smap [%lx, %lx] -> %#"PRI_gfn" for %pd\n", + map->map ? "" : "un", s, e, gfn_x(start_gfn), + map->d); + } + + return 0; +} + static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only) { struct vpci_header *header = &pdev->vpci->header; @@ -423,7 +441,25 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only) if ( !map_pending ) pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd); else + { + struct map_data data = { + .d = pdev->domain, + .map = cmd & PCI_COMMAND_MEMORY, + }; + + for ( i = 0; i < ARRAY_SIZE(header->bars); i++ ) + { + const struct vpci_bar *bar = &header->bars[i]; + + if ( rangeset_is_empty(bar->mem) ) + continue; + + data.bar = bar; + rc = rangeset_report_ranges(bar->mem, 0, ~0ul, print_range, &data); + } + defer_map(dev->domain, dev, cmd, rom_only); + } return 0; To me, to implement a single DEBUG print, it is a bit an overkill. I do understand your concerns that "that function can be preempted and called multiple times", but taking look at the code above I think we can accept that for DEBUG builds. Could you please let me know if I: 1. Still need to implement (the patch above) 2. Drop DEBUG prints (those are really useful while debugging) 3. Leave the print where it was in map_range Thank you in advance, Oleksandr
On Wed, Feb 02, 2022 at 09:46:21AM +0000, Oleksandr Andrushchenko wrote: > > >>> + gdprintk(XENLOG_G_DEBUG, > >>> + "%smap [%lx, %lx] -> %#"PRI_gfn" for %pd\n", > >>> + map->map ? "" : "un", s, e, gfn_x(start_gfn), > >>> + map->d); > >> That's too chatty IMO, I could be fine with printing something along > >> this lines from modify_bars, but not here because that function can be > >> preempted and called multiple times. > > Ok, will move to modify_bars as these prints are really helpful for debug > I tried to implement the same, but now in init_bars: > > diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c > index 667c04cee3ae..92407e617609 100644 > --- a/xen/drivers/vpci/header.c > +++ b/xen/drivers/vpci/header.c > @@ -57,10 +57,6 @@ static int map_range(unsigned long s, unsigned long e, void *data, > */ > start_gfn = gfn_add(start_gfn, s - mfn_x(start_mfn)); > > - gdprintk(XENLOG_G_DEBUG, > - "%smap [%lx, %lx] -> %#"PRI_gfn" for %pd\n", > - map->map ? "" : "un", s, e, gfn_x(start_gfn), > - map->d); > /* > * ARM TODOs: > * - On ARM whether the memory is prefetchable or not should be passed > @@ -258,6 +254,28 @@ static void defer_map(struct domain *d, struct pci_dev *pdev, > raise_softirq(SCHEDULE_SOFTIRQ); > } > > +static int print_range(unsigned long s, unsigned long e, void *data) > +{ > + const struct map_data *map = data; > + > + for ( ; ; ) > + { > + gfn_t start_gfn = _gfn(PFN_DOWN(is_hardware_domain(map->d) > + ? map->bar->addr > + : map->bar->guest_reg)); > + mfn_t start_mfn = _mfn(PFN_DOWN(map->bar->addr)); > + > + start_gfn = gfn_add(start_gfn, s - mfn_x(start_mfn)); > + > + gdprintk(XENLOG_G_DEBUG, > + "%smap [%lx, %lx] -> %#"PRI_gfn" for %pd\n", > + map->map ? "" : "un", s, e, gfn_x(start_gfn), > + map->d); > + } This is an infinite loop AFAICT. Why do you need the for for? > + > + return 0; > +} > + > static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only) > { > struct vpci_header *header = &pdev->vpci->header; > @@ -423,7 +441,25 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only) > if ( !map_pending ) > pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd); > else > + { > + struct map_data data = { > + .d = pdev->domain, > + .map = cmd & PCI_COMMAND_MEMORY, > + }; > + > + for ( i = 0; i < ARRAY_SIZE(header->bars); i++ ) > + { > + const struct vpci_bar *bar = &header->bars[i]; > + > + if ( rangeset_is_empty(bar->mem) ) > + continue; > + > + data.bar = bar; > + rc = rangeset_report_ranges(bar->mem, 0, ~0ul, print_range, &data); Since this is per-BAR we should also print that information and the SBDF of the device, ie: %pd SBDF: (ROM)BAR%u %map [%lx, %lx] -> ... > + } > + > defer_map(dev->domain, dev, cmd, rom_only); > + } > > return 0; > > > To me, to implement a single DEBUG print, it is a bit an overkill. > I do understand your concerns that "that function can be > preempted and called multiple times", but taking look at the code > above I think we can accept that for DEBUG builds. It might be better if you print the per BAR positions at the top of modify_bars, where each BAR is added to the rangeset? Or do you care about reporting the holes also? Thanks, Roger.
On 02.02.22 12:34, Roger Pau Monné wrote: > On Wed, Feb 02, 2022 at 09:46:21AM +0000, Oleksandr Andrushchenko wrote: >>>>> + gdprintk(XENLOG_G_DEBUG, >>>>> + "%smap [%lx, %lx] -> %#"PRI_gfn" for %pd\n", >>>>> + map->map ? "" : "un", s, e, gfn_x(start_gfn), >>>>> + map->d); >>>> That's too chatty IMO, I could be fine with printing something along >>>> this lines from modify_bars, but not here because that function can be >>>> preempted and called multiple times. >>> Ok, will move to modify_bars as these prints are really helpful for debug >> I tried to implement the same, but now in init_bars: >> >> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c >> index 667c04cee3ae..92407e617609 100644 >> --- a/xen/drivers/vpci/header.c >> +++ b/xen/drivers/vpci/header.c >> @@ -57,10 +57,6 @@ static int map_range(unsigned long s, unsigned long e, void *data, >> */ >> start_gfn = gfn_add(start_gfn, s - mfn_x(start_mfn)); >> >> - gdprintk(XENLOG_G_DEBUG, >> - "%smap [%lx, %lx] -> %#"PRI_gfn" for %pd\n", >> - map->map ? "" : "un", s, e, gfn_x(start_gfn), >> - map->d); >> /* >> * ARM TODOs: >> * - On ARM whether the memory is prefetchable or not should be passed >> @@ -258,6 +254,28 @@ static void defer_map(struct domain *d, struct pci_dev *pdev, >> raise_softirq(SCHEDULE_SOFTIRQ); >> } >> >> +static int print_range(unsigned long s, unsigned long e, void *data) >> +{ >> + const struct map_data *map = data; >> + >> + for ( ; ; ) >> + { >> + gfn_t start_gfn = _gfn(PFN_DOWN(is_hardware_domain(map->d) >> + ? map->bar->addr >> + : map->bar->guest_reg)); >> + mfn_t start_mfn = _mfn(PFN_DOWN(map->bar->addr)); >> + >> + start_gfn = gfn_add(start_gfn, s - mfn_x(start_mfn)); >> + >> + gdprintk(XENLOG_G_DEBUG, >> + "%smap [%lx, %lx] -> %#"PRI_gfn" for %pd\n", >> + map->map ? "" : "un", s, e, gfn_x(start_gfn), >> + map->d); >> + } > This is an infinite loop AFAICT. Why do you need the for for? > >> + >> + return 0; >> +} >> + >> static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only) >> { >> struct vpci_header *header = &pdev->vpci->header; >> @@ -423,7 +441,25 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only) >> if ( !map_pending ) >> pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd); >> else >> + { >> + struct map_data data = { >> + .d = pdev->domain, >> + .map = cmd & PCI_COMMAND_MEMORY, >> + }; >> + >> + for ( i = 0; i < ARRAY_SIZE(header->bars); i++ ) >> + { >> + const struct vpci_bar *bar = &header->bars[i]; >> + >> + if ( rangeset_is_empty(bar->mem) ) >> + continue; >> + >> + data.bar = bar; >> + rc = rangeset_report_ranges(bar->mem, 0, ~0ul, print_range, &data); > Since this is per-BAR we should also print that information and the > SBDF of the device, ie: > > %pd SBDF: (ROM)BAR%u %map [%lx, %lx] -> ... > >> + } >> + >> defer_map(dev->domain, dev, cmd, rom_only); >> + } >> >> return 0; >> >> >> To me, to implement a single DEBUG print, it is a bit an overkill. >> I do understand your concerns that "that function can be >> preempted and called multiple times", but taking look at the code >> above I think we can accept that for DEBUG builds. > It might be better if you print the per BAR positions at the top of > modify_bars, where each BAR is added to the rangeset? Or do you care > about reporting the holes also? First of all I didn't run this code, so it is just to show the complexity If the approach itself is ok. If it is then I'll get it working: please do not review it literally yet. The original print was used to show only those {un}mappings that we actually do, no holes etc., so we need to print at the bottom of the init_bars, e.g. when the rangesets are all ready. Again, IMO, adding such a big piece of DEBUG code instead of printing a single DEBUG message could be a bit expansive. I still hear your concerns on *when* it is printed, but still think we can allow that. > > Thanks, Roger. Thank you, Oleksandr
On 02.02.2022 11:44, Oleksandr Andrushchenko wrote: > Again, IMO, adding such a big piece of DEBUG code instead of > printing a single DEBUG message could be a bit expansive. > I still hear your concerns on *when* it is printed, but still think we can > allow that. You do realize though that the mere act of logging a message may cause the need for preemption, and hence logging messages in such cases is detrimental to forward progress? Jan
On 02.02.22 13:11, Jan Beulich wrote: > On 02.02.2022 11:44, Oleksandr Andrushchenko wrote: >> Again, IMO, adding such a big piece of DEBUG code instead of >> printing a single DEBUG message could be a bit expansive. >> I still hear your concerns on *when* it is printed, but still think we can >> allow that. > You do realize though that the mere act of logging a message may cause > the need for preemption, and hence logging messages in such cases is > detrimental to forward progress? Then I will probably remove the print at all. It is easy to add if needed > > Jan > Thank you, Oleksandr
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c index cc49aa68886f..b0499d32c5d8 100644 --- a/xen/drivers/vpci/header.c +++ b/xen/drivers/vpci/header.c @@ -30,6 +30,7 @@ struct map_data { struct domain *d; + const struct vpci_bar *bar; bool map; }; @@ -41,8 +42,25 @@ static int map_range(unsigned long s, unsigned long e, void *data, for ( ; ; ) { + /* Start address of the BAR as seen by the guest. */ + gfn_t start_gfn = _gfn(PFN_DOWN(is_hardware_domain(map->d) + ? map->bar->addr + : map->bar->guest_reg)); + /* Physical start address of the BAR. */ + mfn_t start_mfn = _mfn(PFN_DOWN(map->bar->addr)); unsigned long size = e - s + 1; + /* + * 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. + */ + start_gfn = gfn_add(start_gfn, s - mfn_x(start_mfn)); + + gdprintk(XENLOG_G_DEBUG, + "%smap [%lx, %lx] -> %#"PRI_gfn" for %pd\n", + map->map ? "" : "un", s, e, gfn_x(start_gfn), + map->d); /* * ARM TODOs: * - On ARM whether the memory is prefetchable or not should be passed @@ -52,8 +70,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; @@ -62,8 +82,8 @@ static int map_range(unsigned long s, unsigned long e, void *data, if ( rc < 0 ) { printk(XENLOG_G_WARNING - "Failed to identity %smap [%lx, %lx] for d%d: %d\n", - map->map ? "" : "un", s, e, map->d->domain_id, rc); + "Failed to identity %smap [%lx, %lx] for %pd: %d\n", + map->map ? "" : "un", s, e, map->d, rc); break; } ASSERT(rc < size); @@ -160,6 +180,7 @@ bool vpci_process_pending(struct vcpu *v) if ( rangeset_is_empty(bar->mem) ) continue; + data.bar = bar; rc = rangeset_consume_ranges(bar->mem, map_range, &data); if ( rc == -ERESTART ) @@ -249,6 +270,7 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev, if ( rangeset_is_empty(bar->mem) ) continue; + data.bar = bar; while ( (rc = rangeset_consume_ranges(bar->mem, map_range, &data)) == -ERESTART ) process_pending_softirqs();