Re: [PATCH v2 07/11] vpci/header: program p2m with guest BAR view

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

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