Re: [PATCH v5 08/14] vpci/header: program p2m with guest BAR view

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

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@xxxxxxxx>
>>
>> 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@xxxxxxxx>
>> ---
>> 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