| Message ID | 20211124075942.2645445-5-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI devices passthrough on Arm, part 2 | expand |
Hi Oleksandr, On 24/11/2021 07:59, Oleksandr Andrushchenko wrote: > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > 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. > > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > 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. " With that (or a similar comment): Acked-by: Julien Grall <jgrall@amazon.com> Cheers,
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@epam.com> >> >> 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@epam.com> > > >> 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@amazon.com> Thank you, Oleksandr > > Cheers, >
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 I tried to re-order, but this patch already depends on the previous in the series. It needs to be fixed in a different way then which will change the patches above, so I leave this where it is. Thank you, Oleksandr
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. */ + return 1; } /*