Re: [PATCH v7 4/7] xen/arm: account IO handler for emulated PCI host bridge

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

Hi, Julien!

On 08.12.21 18:53, Julien Grall wrote:
> Hi Oleksandr,
>
> On 24/11/2021 07:59, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>
>> At the moment, we always allocate an extra 16 slots for IO handlers
>> (see MAX_IO_HANDLER). So while adding an IO trap handler for the emulated
>> PCI host bridge we are not breaking anything, but we have a latent bug
>> as the maximum number of IOs may be exceeded.
>> Fix this by explicitly telling that we have an additional IO handler, so it 
>> is
>> accounted.
>>
>> Fixes: d59168dc05a5 ("xen/arm: Enable the existing x86 virtual PCI support 
>> for ARM")
>
> In general, it is better to have the fixes at the beginning of a series. So 
> they are relying on less rework and easier to backport (if needed).
>
> In this case, PCI passthrough is still a technical preview so it doesn't 
> matter too much.
I am planning to resend the whole series, so I can move this to the bottom,
but it is indeed doesn't matter at the moment
>
>>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> >
>> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
>> index ccd998d8dba2..8e801f275879 100644
>> --- a/xen/arch/arm/vpci.c
>> +++ b/xen/arch/arm/vpci.c
>> @@ -126,7 +126,8 @@ unsigned int domain_vpci_get_num_mmio_handlers(struct 
>> domain *d)
>>           return ret < 0 ? 0 : ret;
>>       }
>>   -    return 0;
>> +    /* For a single emulated host bridge's configuration space. */
>
> This comment is lacking some context. I would suggest to reword to something 
> like:
>
> "For the guests, each host bridge requires one region to cover the 
> configuration space. At the moment, we only expose a single host bridge.
> "
>
Ok, will change
> With that (or a similar comment):
>
> Acked-by: Julien Grall <jgrall@xxxxxxxxxx>
Thank you,
Oleksandr
>
> Cheers,
>