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_andrushche...@epam.com>
>>
>> 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_andrushche...@epam.com>
>>
>> ---
>> 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
