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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 01.10.21 16:38, Jan Beulich wrote:
> On 30.09.2021 09:52, 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>
> Just a couple of nits, as I remain unconvinced of the rangeset related
> choice in the earlier patch.
>
>> @@ -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.
> This second "e.g." seems, to me at least, quite redundant with the first
> one.
Ok
>
>> +         * 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
> Do you really mean this to be active even in release builds? Might get
> quite noisy ...
I can change this one to "gdprintk(XENLOG_G_DEBUG"
and leave the below one as "printk(XENLOG_G_WARNING"
Or you also mean the warning to be gdprintk?
>
>> +               "%smap [%lx, %lx] -> %#"PRI_gfn" for d%d\n",
> %pd please in new or altered code.
Will change
>
> Jan
>
Thank you,
Oleksandr