[V7,06/11] vpci/header: program p2m with guest BAR view

Commit Message

Oleksandr Tyshchenko July 19, 2022, 5:42 p.m. UTC

From: Oleksandr Andrushchenko <[e-mail]>

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 <[e-mail]>
---
Since v5:
- remove debug print in map_range callback
- remove "identity" from the debug print
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 | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

Comments

Jan Beulich July 27, 2022, 10:19 a.m. UTC | #1

On 19.07.2022 19:42, Oleksandr Tyshchenko wrote:
> From: Oleksandr Andrushchenko <[e-mail]>
> 
> 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 <[e-mail]>

If taking the previous patch as given, the patch here looks okay to me.
But since I'm still not really agreeing with what the previous patch
does, both that and this one will need to be judged on by Roger once
he's back. I'm sorry for the therefore resulting further delay.

Jan

Oleksandr Tyshchenko July 27, 2022, 5:06 p.m. UTC | #2

On 27.07.22 13:19, Jan Beulich wrote:

Hello Jan


> On 19.07.2022 19:42, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Andrushchenko <[e-mail]>
>>
>> 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 <[e-mail]>
> If taking the previous patch as given, the patch here looks okay to me.

This is a good news, thank you.


> But since I'm still not really agreeing with what the previous patch
> does,

Previous? Sorry, I didn't see any comments given for "[PATCH V7 05/11] 
vpci/header: handle p2m range sets per BAR".

Or do you perhaps mean "[PATCH V7 03/11] vpci/header: implement guest 
BAR register handlers" where you explicitly mentioned concerns?


> both that and this one will need to be judged on by Roger once
> he's back.


Let's wait for Roger's input here as well.


> I'm sorry for the therefore resulting further delay.

no problem


>
> Jan

Jan Beulich July 28, 2022, 7:04 a.m. UTC | #3

On 27.07.2022 19:06, Oleksandr wrote:
> On 27.07.22 13:19, Jan Beulich wrote:
>> On 19.07.2022 19:42, Oleksandr Tyshchenko wrote:
>>> From: Oleksandr Andrushchenko <[e-mail]>
>>>
>>> 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 <[e-mail]>
>> If taking the previous patch as given, the patch here looks okay to me.
> 
> This is a good news, thank you.
> 
> 
>> But since I'm still not really agreeing with what the previous patch
>> does,
> 
> Previous? Sorry, I didn't see any comments given for "[PATCH V7 05/11] 
> vpci/header: handle p2m range sets per BAR".
> 
> Or do you perhaps mean "[PATCH V7 03/11] vpci/header: implement guest 
> BAR register handlers" where you explicitly mentioned concerns?

No, I mean the previous patch, to which I had given comments in a much
earlier version. Roger looks to agree with the approach taken, so my
comments were (legitimately) put off. But with me not agreeing with
the approach, it's not very reasonable for me to (further) review that
change. Hence my deferral to Roger.

Jan

Patch

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index f14ff11882..4e6547a54d 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,21 @@  static int cf_check map_range(
 
     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));
+
         /*
          * ARM TODOs:
          * - On ARM whether the memory is prefetchable or not should be passed
@@ -52,8 +66,8 @@  static int cf_check map_range(
          * - {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 +76,8 @@  static int cf_check map_range(
         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 %smap [%lx, %lx] for %pd: %d\n",
+                   map->map ? "" : "un", s, e, map->d, rc);
             break;
         }
         ASSERT(rc < size);
@@ -155,6 +169,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 )
@@ -218,6 +233,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 )
         {