From: Oleksandr Andrushchenko <oleksandr_andrushchenko@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_andrushchenko@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));
+ 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));
+ data.start_mfn = _mfn(PFN_DOWN(bar->addr));
while ( (rc = rangeset_consume_ranges(bar->mem, map_range,
&data)) == -ERESTART )
process_pending_softirqs();
--
2.25.1
Hi Oleksandr,
On 23.09.2021 14:54, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@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_andrushchenko@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));
> + 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.
> + data.start_mfn = _mfn(PFN_DOWN(bar->addr));
> while ( (rc = rangeset_consume_ranges(bar->mem, map_range,
> &data)) == -ERESTART )
> process_pending_softirqs();
>
Cheers,
Michal
On 29.09.2021 10:13, Michal Orzel wrote: > On 23.09.2021 14:54, Oleksandr Andrushchenko wrote: >> @@ -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)); FWIW I agree, yet personally I think the conditional operator here even wants to move inside the _gfn(PFN_DOWN()). Jan
On 29.09.21 11:16, Jan Beulich wrote: > On 29.09.2021 10:13, Michal Orzel wrote: >> On 23.09.2021 14:54, Oleksandr Andrushchenko wrote: >>> @@ -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)); > FWIW I agree, yet personally I think the conditional operator here > even wants to move inside the _gfn(PFN_DOWN()). I can do it as well: data.start_gfn = _gfn(PFN_DOWN(is_hardware_domain(v->vpci.pdev->domain) ? bar->addr : bar->guest_addr)) But, help me please breaking it into multiline with 80 chars respected > > Jan > Thank you, Oleksandr
On 29.09.2021 10:24, Oleksandr Andrushchenko wrote:
>
> On 29.09.21 11:16, Jan Beulich wrote:
>> On 29.09.2021 10:13, Michal Orzel wrote:
>>> On 23.09.2021 14:54, Oleksandr Andrushchenko wrote:
>>>> @@ -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));
>> FWIW I agree, yet personally I think the conditional operator here
>> even wants to move inside the _gfn(PFN_DOWN()).
>
> I can do it as well:
>
> data.start_gfn = _gfn(PFN_DOWN(is_hardware_domain(v->vpci.pdev->domain) ? bar->addr : bar->guest_addr))
> But, help me please breaking it into multiline with 80 chars respected
Besides the option of latching v->vpci.pdev->domain or
is_hardware_domain(v->vpci.pdev->domain) into a helper variable,
data.start_gfn =
_gfn(PFN_DOWN(is_hardware_domain(v->vpci.pdev->domain)
? bar->addr : bar->guest_addr));
or
data.start_gfn =
_gfn(PFN_DOWN(is_hardware_domain(v->vpci.pdev->domain)
? bar->addr
: bar->guest_addr));
Jan
On 29.09.21 11:36, Jan Beulich wrote: > On 29.09.2021 10:24, Oleksandr Andrushchenko wrote: >> On 29.09.21 11:16, Jan Beulich wrote: >>> On 29.09.2021 10:13, Michal Orzel wrote: >>>> On 23.09.2021 14:54, Oleksandr Andrushchenko wrote: >>>>> @@ -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)); >>> FWIW I agree, yet personally I think the conditional operator here >>> even wants to move inside the _gfn(PFN_DOWN()). >> I can do it as well: >> >> data.start_gfn = _gfn(PFN_DOWN(is_hardware_domain(v->vpci.pdev->domain) ? bar->addr : bar->guest_addr)) >> But, help me please breaking it into multiline with 80 chars respected > Besides the option of latching v->vpci.pdev->domain or > is_hardware_domain(v->vpci.pdev->domain) into a helper variable, > > data.start_gfn = > _gfn(PFN_DOWN(is_hardware_domain(v->vpci.pdev->domain) > ? bar->addr : bar->guest_addr)); I'll go with this one, thank you > > or > > data.start_gfn = > _gfn(PFN_DOWN(is_hardware_domain(v->vpci.pdev->domain) > ? bar->addr > : bar->guest_addr)); > > Jan >
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@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_andrushchenko@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
© 2016 - 2024 Red Hat, Inc.