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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

Hi, Roger!

On 26.10.21 13:35, Roger Pau Monné wrote:
> On Thu, Sep 30, 2021 at 10:52:19AM +0300, 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 v2:
>> - improve readability for data.start_gfn and restructure ?: construct
>> Since v1:
>>   - s/MSI/MSI-X in comments
>> ---
>>   xen/drivers/vpci/header.c | 34 ++++++++++++++++++++++++++++++++--
>>   1 file changed, 32 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
>> index 9c603d26d302..f23c956cde6c 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.
> IMO there are too many 'e.g.' here.
>
>> +         * 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.
>> +         */
> I would simply the comment a bit:
>
> /*
>   * 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.
>   */
>
> Apart from MSI-X related holes on x86 at least we support preemption
> here, which means a range could be partially mapped before yielding.
Thank you, will use your comment which is shorter and still clear
>> +        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);
> I think increasing map->start_gfn is wrong here, as it would get out
> of sync with map->start_mfn then, and the calculations done to obtain
> start_gfn would then be wrong.
Indeed, will remove it
>
>>           if ( general_preempt_check() )
>>                   return -ERESTART;
>>       }
>> @@ -149,6 +172,10 @@ bool vpci_process_pending(struct vcpu *v)
>>               if ( !bar->mem )
>>                   continue;
>>   
>> +            data.start_gfn =
>> +                 _gfn(PFN_DOWN(is_hardware_domain(v->vpci.pdev->domain)
> You can just use v->domain here.
Ok
>
>> +                               ? bar->addr : bar->guest_addr));
> I would place the '?' in the line above, but that's just my taste.
Hmmm, this chunk was discussed before and this is the result of
that discussion ;) So, I'll better keep it as is
>
> Thanks, Roger.
Thank you,
Oleksandr