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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 19.11.21 14:33, Jan Beulich wrote:
> On 05.11.2021 07:56, 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.
> I'm sorry to be picky, but I don't think host bridges set up BARs. What
> I think you mean is "as set up by the PCI bus driver in the hardware
> domain". Of course this then still leaves out the case of firmware
> doing so, and hence the dom0less case.
Sounds, good I will use your wording, thanks
>
>> --- 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;
> As of the previous patch you process this on a per-BAR basis. Why don't
> you simply put const struct vpci_bar * here? This would e.g. avoid the
> need to keep in sync the identical expressions in vpci_process_pending()
> and apply_map().
Aha, you mean to move

+            data.start_gfn =
+                 _gfn(PFN_DOWN(is_hardware_domain(v->domain)
+                               ? bar->addr : bar->guest_addr));
+            data.start_mfn = _mfn(PFN_DOWN(bar->addr));

into map_range. Makes sense, it seems I can do that

>> @@ -37,12 +41,24 @@ 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;
> Imo this wants to move into the more narrow scope, ...
>
>>       int rc;
>>   
>>       for ( ; ; )
>>       {
>>           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(map->start_gfn, s - mfn_x(map->start_mfn));
> ... allowing (in principle) for this to become its initializer.
Yes, good idea
>
> Jan
>
Thank you,
Oleksandr