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 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.
+ * 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 =
+ _gfn(PFN_DOWN(is_hardware_domain(v->vpci.pdev->domain)
+ ? bar->addr : 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,9 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
if ( !bar->mem )
continue;
+ data.start_gfn = _gfn(PFN_DOWN(is_hardware_domain(d)
+ ? bar->addr : 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
On 30.09.2021 09:52, 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>
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.
> + * 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 ...
> + "%smap [%lx, %lx] -> %#"PRI_gfn" for d%d\n",
%pd please in new or altered code.
Jan
On 01.10.21 16:38, Jan Beulich wrote:
> On 30.09.2021 09:52, 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>
> 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
On Thu, Sep 30, 2021 at 10:52:19AM +0300, 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 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.
> + 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.
> 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.
> + ? bar->addr : bar->guest_addr));
I would place the '?' in the line above, but that's just my taste.
Thanks, Roger.
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@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 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
© 2016 - 2024 Red Hat, Inc.