Re: [PATCH v3 06/11] vpci/header: Handle p2m range sets per BAR
On 26.10.2021 11:40, Roger Pau Monné wrote: > On Mon, Oct 25, 2021 at 11:51:57AM +, Oleksandr Andrushchenko wrote: >> Hi, Roger! >> Could you please take a look at the below? >> Jan was questioning the per BAR range set approach, so it >> is crucial for the maintainer (you) to answer here. > > I'm open to suggestions to using something different than a rangeset > per BAR, but lacking any concrete proposal I think using rangesets is > fine. The main reason for my objection is that for the average BAR the rangeset will hold exactly one range. That's not an efficient way to express a single range. > One possible way might be to extend rangesets so that private data > could be stored for each rangeset range, but that would then make > merging operations impossible, likewise splitting ranges would be > troublesome. Indeed, so I don't view this as an option. Jan
Re: [PATCH v3 06/11] vpci/header: Handle p2m range sets per BAR
Hi, Roger!
On 26.10.21 12:08, Roger Pau Monné wrote:
> On Thu, Sep 30, 2021 at 10:52:18AM +0300, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko
>>
>> Instead of handling a single range set, that contains all the memory
>> regions of all the BARs and ROM, have them per BAR.
>>
>> This is in preparation of making non-identity mappings in p2m for the
>> MMIOs/ROM.
>>
>> Signed-off-by: Oleksandr Andrushchenko
>> ---
>> xen/drivers/vpci/header.c | 172 ++
>> xen/include/xen/vpci.h| 3 +-
>> 2 files changed, 122 insertions(+), 53 deletions(-)
>>
>> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
>> index ec4d215f36ff..9c603d26d302 100644
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -131,49 +131,75 @@ static void modify_decoding(const struct pci_dev
>> *pdev, uint16_t cmd,
>>
>> bool vpci_process_pending(struct vcpu *v)
>> {
>> -if ( v->vpci.mem )
>> +if ( v->vpci.num_mem_ranges )
>> {
>> struct map_data data = {
>> .d = v->domain,
>> .map = v->vpci.cmd & PCI_COMMAND_MEMORY,
>> };
>> -int rc = rangeset_consume_ranges(v->vpci.mem, map_range, );
>> +struct pci_dev *pdev = v->vpci.pdev;
>> +struct vpci_header *header = >vpci->header;
>> +unsigned int i;
>>
>> -if ( rc == -ERESTART )
>> -return true;
>> +for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
>> +{
>> +struct vpci_bar *bar = >bars[i];
>> +int rc;
>>
>> -spin_lock(>vpci.pdev->vpci->lock);
>> -/* Disable memory decoding unconditionally on failure. */
>> -modify_decoding(v->vpci.pdev,
>> -rc ? v->vpci.cmd & ~PCI_COMMAND_MEMORY :
>> v->vpci.cmd,
>> -!rc && v->vpci.rom_only);
>> -spin_unlock(>vpci.pdev->vpci->lock);
>> +if ( !bar->mem )
>> +continue;
>>
>> -rangeset_destroy(v->vpci.mem);
>> -v->vpci.mem = NULL;
>> -if ( rc )
>> -/*
>> - * FIXME: in case of failure remove the device from the domain.
>> - * Note that there might still be leftover mappings. While this
>> is
>> - * safe for Dom0, for DomUs the domain will likely need to be
>> - * killed in order to avoid leaking stale p2m mappings on
>> - * failure.
>> - */
>> -vpci_remove_device(v->vpci.pdev);
>> +rc = rangeset_consume_ranges(bar->mem, map_range, );
>> +
>> +if ( rc == -ERESTART )
>> +return true;
>> +
>> +spin_lock(>vpci->lock);
>> +/* Disable memory decoding unconditionally on failure. */
>> +modify_decoding(pdev,
>> +rc ? v->vpci.cmd & ~PCI_COMMAND_MEMORY :
>> v->vpci.cmd,
>> +!rc && v->vpci.rom_only);
>> +spin_unlock(>vpci->lock);
>> +
>> +rangeset_destroy(bar->mem);
> Now that the rangesets are per-BAR we might have to consider
> allocating them at initialization time and not destroying them when
> empty. We could replace the NULL checks with rangeset_is_empty
> instead. Not that you have to do this on this patch, but I think it's
> worth mentioning.
Yes, this is a good idea. I will re-work the patch to create/destroy
the rangesets once in add/remove
>
>> +bar->mem = NULL;
>> +v->vpci.num_mem_ranges--;
>> +if ( rc )
>> +/*
>> + * FIXME: in case of failure remove the device from the
>> domain.
>> + * Note that there might still be leftover mappings. While
>> this is
>> + * safe for Dom0, for DomUs the domain will likely need to
>> be
>> + * killed in order to avoid leaking stale p2m mappings on
>> + * failure.
>> + */
>> +vpci_remove_device(pdev);
>> +}
>> }
>>
>> return false;
>> }
>>
>> static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
>> -struct rangeset *mem, uint16_t cmd)
>> +uint16_t cmd)
>> {
>> struct map_data data = { .d = d, .map = true };
>> -int rc;
>> +struct vpci_header *header = >vpci->header;
>> +int rc = 0;
>> +unsigned int i;
>> +
>> +for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
>> +{
>> +struct vpci_bar *bar = >bars[i];
>>
>> -while ( (rc = rangeset_consume_ranges(mem, map_range, )) ==
>> -ERESTART )
>> -process_pending_softirqs();
>> -rangeset_destroy(mem);
>> +if ( !bar->mem )
>> +continue;
>> +
>> +while ( (rc = rangeset_consume_ranges(bar->mem, map_range,
>> + )) == -ERESTART )
>> +
Re: [PATCH v3 06/11] vpci/header: Handle p2m range sets per BAR
On Mon, Oct 25, 2021 at 11:51:57AM +, Oleksandr Andrushchenko wrote: > Hi, Roger! > Could you please take a look at the below? > Jan was questioning the per BAR range set approach, so it > is crucial for the maintainer (you) to answer here. I'm open to suggestions to using something different than a rangeset per BAR, but lacking any concrete proposal I think using rangesets is fine. One possible way might be to extend rangesets so that private data could be stored for each rangeset range, but that would then make merging operations impossible, likewise splitting ranges would be troublesome. We could then store the physical BAR address in that private data and use the rangeset addresses as guest physical address space. It's unclear however that this approach would be any better than just using a rangeset per BAR. Thanks, Roger.
Re: [PATCH v3 06/11] vpci/header: Handle p2m range sets per BAR
On Thu, Sep 30, 2021 at 10:52:18AM +0300, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko
>
> Instead of handling a single range set, that contains all the memory
> regions of all the BARs and ROM, have them per BAR.
>
> This is in preparation of making non-identity mappings in p2m for the
> MMIOs/ROM.
>
> Signed-off-by: Oleksandr Andrushchenko
> ---
> xen/drivers/vpci/header.c | 172 ++
> xen/include/xen/vpci.h| 3 +-
> 2 files changed, 122 insertions(+), 53 deletions(-)
>
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index ec4d215f36ff..9c603d26d302 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -131,49 +131,75 @@ static void modify_decoding(const struct pci_dev *pdev,
> uint16_t cmd,
>
> bool vpci_process_pending(struct vcpu *v)
> {
> -if ( v->vpci.mem )
> +if ( v->vpci.num_mem_ranges )
> {
> struct map_data data = {
> .d = v->domain,
> .map = v->vpci.cmd & PCI_COMMAND_MEMORY,
> };
> -int rc = rangeset_consume_ranges(v->vpci.mem, map_range, );
> +struct pci_dev *pdev = v->vpci.pdev;
> +struct vpci_header *header = >vpci->header;
> +unsigned int i;
>
> -if ( rc == -ERESTART )
> -return true;
> +for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
> +{
> +struct vpci_bar *bar = >bars[i];
> +int rc;
>
> -spin_lock(>vpci.pdev->vpci->lock);
> -/* Disable memory decoding unconditionally on failure. */
> -modify_decoding(v->vpci.pdev,
> -rc ? v->vpci.cmd & ~PCI_COMMAND_MEMORY : v->vpci.cmd,
> -!rc && v->vpci.rom_only);
> -spin_unlock(>vpci.pdev->vpci->lock);
> +if ( !bar->mem )
> +continue;
>
> -rangeset_destroy(v->vpci.mem);
> -v->vpci.mem = NULL;
> -if ( rc )
> -/*
> - * FIXME: in case of failure remove the device from the domain.
> - * Note that there might still be leftover mappings. While this
> is
> - * safe for Dom0, for DomUs the domain will likely need to be
> - * killed in order to avoid leaking stale p2m mappings on
> - * failure.
> - */
> -vpci_remove_device(v->vpci.pdev);
> +rc = rangeset_consume_ranges(bar->mem, map_range, );
> +
> +if ( rc == -ERESTART )
> +return true;
> +
> +spin_lock(>vpci->lock);
> +/* Disable memory decoding unconditionally on failure. */
> +modify_decoding(pdev,
> +rc ? v->vpci.cmd & ~PCI_COMMAND_MEMORY :
> v->vpci.cmd,
> +!rc && v->vpci.rom_only);
> +spin_unlock(>vpci->lock);
> +
> +rangeset_destroy(bar->mem);
Now that the rangesets are per-BAR we might have to consider
allocating them at initialization time and not destroying them when
empty. We could replace the NULL checks with rangeset_is_empty
instead. Not that you have to do this on this patch, but I think it's
worth mentioning.
> +bar->mem = NULL;
> +v->vpci.num_mem_ranges--;
> +if ( rc )
> +/*
> + * FIXME: in case of failure remove the device from the
> domain.
> + * Note that there might still be leftover mappings. While
> this is
> + * safe for Dom0, for DomUs the domain will likely need to be
> + * killed in order to avoid leaking stale p2m mappings on
> + * failure.
> + */
> +vpci_remove_device(pdev);
> +}
> }
>
> return false;
> }
>
> static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
> -struct rangeset *mem, uint16_t cmd)
> +uint16_t cmd)
> {
> struct map_data data = { .d = d, .map = true };
> -int rc;
> +struct vpci_header *header = >vpci->header;
> +int rc = 0;
> +unsigned int i;
> +
> +for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
> +{
> +struct vpci_bar *bar = >bars[i];
>
> -while ( (rc = rangeset_consume_ranges(mem, map_range, )) ==
> -ERESTART )
> -process_pending_softirqs();
> -rangeset_destroy(mem);
> +if ( !bar->mem )
> +continue;
> +
> +while ( (rc = rangeset_consume_ranges(bar->mem, map_range,
> + )) == -ERESTART )
> +process_pending_softirqs();
> +rangeset_destroy(bar->mem);
> +bar->mem = NULL;
> +}
> if ( !rc )
> modify_decoding(pdev, cmd, false);
>
> @@ -181,7 +207,7 @@ static int __init apply_map(struct domain *d, const
> struct pci_dev *pdev,
> }
>
> static void defer_map(struct domain *d,
Re: [PATCH v3 06/11] vpci/header: Handle p2m range sets per BAR
Hi, Roger!
Could you please take a look at the below?
Jan was questioning the per BAR range set approach, so it
is crucial for the maintainer (you) to answer here.
Thank you in advance,
Oleksandr
On 30.09.21 10:52, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko
>
> Instead of handling a single range set, that contains all the memory
> regions of all the BARs and ROM, have them per BAR.
>
> This is in preparation of making non-identity mappings in p2m for the
> MMIOs/ROM.
>
> Signed-off-by: Oleksandr Andrushchenko
> ---
> xen/drivers/vpci/header.c | 172 ++
> xen/include/xen/vpci.h| 3 +-
> 2 files changed, 122 insertions(+), 53 deletions(-)
>
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index ec4d215f36ff..9c603d26d302 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -131,49 +131,75 @@ static void modify_decoding(const struct pci_dev *pdev,
> uint16_t cmd,
>
> bool vpci_process_pending(struct vcpu *v)
> {
> -if ( v->vpci.mem )
> +if ( v->vpci.num_mem_ranges )
> {
> struct map_data data = {
> .d = v->domain,
> .map = v->vpci.cmd & PCI_COMMAND_MEMORY,
> };
> -int rc = rangeset_consume_ranges(v->vpci.mem, map_range, );
> +struct pci_dev *pdev = v->vpci.pdev;
> +struct vpci_header *header = >vpci->header;
> +unsigned int i;
>
> -if ( rc == -ERESTART )
> -return true;
> +for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
> +{
> +struct vpci_bar *bar = >bars[i];
> +int rc;
>
> -spin_lock(>vpci.pdev->vpci->lock);
> -/* Disable memory decoding unconditionally on failure. */
> -modify_decoding(v->vpci.pdev,
> -rc ? v->vpci.cmd & ~PCI_COMMAND_MEMORY : v->vpci.cmd,
> -!rc && v->vpci.rom_only);
> -spin_unlock(>vpci.pdev->vpci->lock);
> +if ( !bar->mem )
> +continue;
>
> -rangeset_destroy(v->vpci.mem);
> -v->vpci.mem = NULL;
> -if ( rc )
> -/*
> - * FIXME: in case of failure remove the device from the domain.
> - * Note that there might still be leftover mappings. While this
> is
> - * safe for Dom0, for DomUs the domain will likely need to be
> - * killed in order to avoid leaking stale p2m mappings on
> - * failure.
> - */
> -vpci_remove_device(v->vpci.pdev);
> +rc = rangeset_consume_ranges(bar->mem, map_range, );
> +
> +if ( rc == -ERESTART )
> +return true;
> +
> +spin_lock(>vpci->lock);
> +/* Disable memory decoding unconditionally on failure. */
> +modify_decoding(pdev,
> +rc ? v->vpci.cmd & ~PCI_COMMAND_MEMORY :
> v->vpci.cmd,
> +!rc && v->vpci.rom_only);
> +spin_unlock(>vpci->lock);
> +
> +rangeset_destroy(bar->mem);
> +bar->mem = NULL;
> +v->vpci.num_mem_ranges--;
> +if ( rc )
> +/*
> + * FIXME: in case of failure remove the device from the
> domain.
> + * Note that there might still be leftover mappings. While
> this is
> + * safe for Dom0, for DomUs the domain will likely need to be
> + * killed in order to avoid leaking stale p2m mappings on
> + * failure.
> + */
> +vpci_remove_device(pdev);
> +}
> }
>
> return false;
> }
>
> static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
> -struct rangeset *mem, uint16_t cmd)
> +uint16_t cmd)
> {
> struct map_data data = { .d = d, .map = true };
> -int rc;
> +struct vpci_header *header = >vpci->header;
> +int rc = 0;
> +unsigned int i;
> +
> +for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
> +{
> +struct vpci_bar *bar = >bars[i];
>
> -while ( (rc = rangeset_consume_ranges(mem, map_range, )) ==
> -ERESTART )
> -process_pending_softirqs();
> -rangeset_destroy(mem);
> +if ( !bar->mem )
> +continue;
> +
> +while ( (rc = rangeset_consume_ranges(bar->mem, map_range,
> + )) == -ERESTART )
> +process_pending_softirqs();
> +rangeset_destroy(bar->mem);
> +bar->mem = NULL;
> +}
> if ( !rc )
> modify_decoding(pdev, cmd, false);
>
> @@ -181,7 +207,7 @@ static int __init apply_map(struct domain *d, const
> struct pci_dev *pdev,
> }
>
> static void defer_map(struct domain *d, struct pci_dev *pdev,
> - struct rangeset *mem, uint16_t cmd,
