| Message ID | 20201109125031.26409-2-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM PCI passthrough configuration and vPCI | expand |
On Mon, Nov 09, 2020 at 02:50:22PM +0200, Oleksandr Andrushchenko wrote: > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > According to https://wiki.xenproject.org/wiki/Linux_PVH: > > Items not supported by PVH > - PCI pass through (as of Xen 4.10) > > Allow running PCI remove code on ARM and do not assert for PVH domains. > > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > --- > tools/libxl/Makefile | 4 ++++ > tools/libxl/libxl_pci.c | 4 +++- > 2 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile > index 241da7fff6f4..f3806aafcb4e 100644 > --- a/tools/libxl/Makefile > +++ b/tools/libxl/Makefile > @@ -130,6 +130,10 @@ endif > > LIBXL_LIBS += -lyajl > > +ifeq ($(CONFIG_ARM),y) > +CFALGS += -DCONFIG_ARM > +endif > + > LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \ > libxl_dom.o libxl_exec.o libxl_xshelp.o libxl_device.o \ > libxl_internal.o libxl_utils.o libxl_uuid.o \ > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c > index bc5843b13701..b93cf976642b 100644 > --- a/tools/libxl/libxl_pci.c > +++ b/tools/libxl/libxl_pci.c > @@ -1915,8 +1915,10 @@ static void do_pci_remove(libxl__egc *egc, uint32_t domid, > goto out_fail; > } > } else { > + /* PCI passthrough can also run on ARM PVH */ > +#ifndef CONFIG_ARM > assert(type == LIBXL_DOMAIN_TYPE_PV); > - > +#endif I would just remove the assert now if this is to be used by Arm and you don't need to fork the file for Arm. Roger.
On 11/11/20 2:31 PM, Roger Pau Monné wrote: > On Mon, Nov 09, 2020 at 02:50:22PM +0200, Oleksandr Andrushchenko wrote: >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> >> According to https://wiki.xenproject.org/wiki/Linux_PVH: >> >> Items not supported by PVH >> - PCI pass through (as of Xen 4.10) >> >> Allow running PCI remove code on ARM and do not assert for PVH domains. >> >> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> --- >> tools/libxl/Makefile | 4 ++++ >> tools/libxl/libxl_pci.c | 4 +++- >> 2 files changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile >> index 241da7fff6f4..f3806aafcb4e 100644 >> --- a/tools/libxl/Makefile >> +++ b/tools/libxl/Makefile >> @@ -130,6 +130,10 @@ endif >> >> LIBXL_LIBS += -lyajl >> >> +ifeq ($(CONFIG_ARM),y) >> +CFALGS += -DCONFIG_ARM >> +endif >> + >> LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \ >> libxl_dom.o libxl_exec.o libxl_xshelp.o libxl_device.o \ >> libxl_internal.o libxl_utils.o libxl_uuid.o \ >> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c >> index bc5843b13701..b93cf976642b 100644 >> --- a/tools/libxl/libxl_pci.c >> +++ b/tools/libxl/libxl_pci.c >> @@ -1915,8 +1915,10 @@ static void do_pci_remove(libxl__egc *egc, uint32_t domid, >> goto out_fail; >> } >> } else { >> + /* PCI passthrough can also run on ARM PVH */ >> +#ifndef CONFIG_ARM >> assert(type == LIBXL_DOMAIN_TYPE_PV); >> - >> +#endif > I would just remove the assert now if this is to be used by Arm and > you don't need to fork the file for Arm. Sounds good, I will drop then But what would be the right explanation then? I mean why there was an ASSERT and now it is safe (for x86) to remove that? > > Roger. Thank you, Oleksandr
On Wed, Nov 11, 2020 at 01:10:01PM +0000, Oleksandr Andrushchenko wrote: > > On 11/11/20 2:31 PM, Roger Pau Monné wrote: > > On Mon, Nov 09, 2020 at 02:50:22PM +0200, Oleksandr Andrushchenko wrote: > >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > >> > >> According to https://wiki.xenproject.org/wiki/Linux_PVH: > >> > >> Items not supported by PVH > >> - PCI pass through (as of Xen 4.10) > >> > >> Allow running PCI remove code on ARM and do not assert for PVH domains. > >> > >> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > >> --- > >> tools/libxl/Makefile | 4 ++++ > >> tools/libxl/libxl_pci.c | 4 +++- > >> 2 files changed, 7 insertions(+), 1 deletion(-) > >> > >> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile > >> index 241da7fff6f4..f3806aafcb4e 100644 > >> --- a/tools/libxl/Makefile > >> +++ b/tools/libxl/Makefile > >> @@ -130,6 +130,10 @@ endif > >> > >> LIBXL_LIBS += -lyajl > >> > >> +ifeq ($(CONFIG_ARM),y) > >> +CFALGS += -DCONFIG_ARM > >> +endif > >> + > >> LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \ > >> libxl_dom.o libxl_exec.o libxl_xshelp.o libxl_device.o \ > >> libxl_internal.o libxl_utils.o libxl_uuid.o \ > >> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c > >> index bc5843b13701..b93cf976642b 100644 > >> --- a/tools/libxl/libxl_pci.c > >> +++ b/tools/libxl/libxl_pci.c > >> @@ -1915,8 +1915,10 @@ static void do_pci_remove(libxl__egc *egc, uint32_t domid, > >> goto out_fail; > >> } > >> } else { > >> + /* PCI passthrough can also run on ARM PVH */ > >> +#ifndef CONFIG_ARM > >> assert(type == LIBXL_DOMAIN_TYPE_PV); > >> - > >> +#endif > > I would just remove the assert now if this is to be used by Arm and > > you don't need to fork the file for Arm. > > Sounds good, I will drop then > > But what would be the right explanation then? I mean why there was an ASSERT > > and now it is safe (for x86) to remove that? An assert is just a safe belt, the expectation is that it's never hit by actual code. Given that this path will now also be used by PVH (even if only on Arm) I don't see the point in keeping the assert, and making it conditional to != Arm seems worse than just dropping it. Thanks, Roger.
On 11/11/20 3:55 PM, Roger Pau Monné wrote: > On Wed, Nov 11, 2020 at 01:10:01PM +0000, Oleksandr Andrushchenko wrote: >> On 11/11/20 2:31 PM, Roger Pau Monné wrote: >>> On Mon, Nov 09, 2020 at 02:50:22PM +0200, Oleksandr Andrushchenko wrote: >>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >>>> >>>> According to https://urldefense.com/v3/__https://wiki.xenproject.org/wiki/Linux_PVH__;!!GF_29dbcQIUBPA!nEHd6eivmqtdJxtrhO-3x2Mz9F50JsKUoV7WTEJd_D1N01DrBOJXzGW1QAqwshZ9AMxywbUhOA$ [wiki[.]xenproject[.]org]: >>>> >>>> Items not supported by PVH >>>> - PCI pass through (as of Xen 4.10) >>>> >>>> Allow running PCI remove code on ARM and do not assert for PVH domains. >>>> >>>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >>>> --- >>>> tools/libxl/Makefile | 4 ++++ >>>> tools/libxl/libxl_pci.c | 4 +++- >>>> 2 files changed, 7 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile >>>> index 241da7fff6f4..f3806aafcb4e 100644 >>>> --- a/tools/libxl/Makefile >>>> +++ b/tools/libxl/Makefile >>>> @@ -130,6 +130,10 @@ endif >>>> >>>> LIBXL_LIBS += -lyajl >>>> >>>> +ifeq ($(CONFIG_ARM),y) >>>> +CFALGS += -DCONFIG_ARM >>>> +endif >>>> + >>>> LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \ >>>> libxl_dom.o libxl_exec.o libxl_xshelp.o libxl_device.o \ >>>> libxl_internal.o libxl_utils.o libxl_uuid.o \ >>>> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c >>>> index bc5843b13701..b93cf976642b 100644 >>>> --- a/tools/libxl/libxl_pci.c >>>> +++ b/tools/libxl/libxl_pci.c >>>> @@ -1915,8 +1915,10 @@ static void do_pci_remove(libxl__egc *egc, uint32_t domid, >>>> goto out_fail; >>>> } >>>> } else { >>>> + /* PCI passthrough can also run on ARM PVH */ >>>> +#ifndef CONFIG_ARM >>>> assert(type == LIBXL_DOMAIN_TYPE_PV); >>>> - >>>> +#endif >>> I would just remove the assert now if this is to be used by Arm and >>> you don't need to fork the file for Arm. >> Sounds good, I will drop then >> >> But what would be the right explanation then? I mean why there was an ASSERT >> >> and now it is safe (for x86) to remove that? > An assert is just a safe belt, the expectation is that it's never hit > by actual code. Given that this path will now also be used by PVH > (even if only on Arm) I don't see the point in keeping the assert, and > making it conditional to != Arm seems worse than just dropping it. Ok, so I can write in the patch description something like: "this path is now used by PVH, so the assert is no longer valid" Does it sound ok? > Thanks, Roger. Thank you, Oleksandr
On Wed, Nov 11, 2020 at 02:12:56PM +0000, Oleksandr Andrushchenko wrote: > > On 11/11/20 3:55 PM, Roger Pau Monné wrote: > > On Wed, Nov 11, 2020 at 01:10:01PM +0000, Oleksandr Andrushchenko wrote: > >> On 11/11/20 2:31 PM, Roger Pau Monné wrote: > >>> On Mon, Nov 09, 2020 at 02:50:22PM +0200, Oleksandr Andrushchenko wrote: > >>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > >>>> > >>>> According to https://urldefense.com/v3/__https://wiki.xenproject.org/wiki/Linux_PVH__;!!GF_29dbcQIUBPA!nEHd6eivmqtdJxtrhO-3x2Mz9F50JsKUoV7WTEJd_D1N01DrBOJXzGW1QAqwshZ9AMxywbUhOA$ [wiki[.]xenproject[.]org]: > >>>> > >>>> Items not supported by PVH > >>>> - PCI pass through (as of Xen 4.10) > >>>> > >>>> Allow running PCI remove code on ARM and do not assert for PVH domains. > >>>> > >>>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > >>>> --- > >>>> tools/libxl/Makefile | 4 ++++ > >>>> tools/libxl/libxl_pci.c | 4 +++- > >>>> 2 files changed, 7 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile > >>>> index 241da7fff6f4..f3806aafcb4e 100644 > >>>> --- a/tools/libxl/Makefile > >>>> +++ b/tools/libxl/Makefile > >>>> @@ -130,6 +130,10 @@ endif > >>>> > >>>> LIBXL_LIBS += -lyajl > >>>> > >>>> +ifeq ($(CONFIG_ARM),y) > >>>> +CFALGS += -DCONFIG_ARM > >>>> +endif > >>>> + > >>>> LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \ > >>>> libxl_dom.o libxl_exec.o libxl_xshelp.o libxl_device.o \ > >>>> libxl_internal.o libxl_utils.o libxl_uuid.o \ > >>>> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c > >>>> index bc5843b13701..b93cf976642b 100644 > >>>> --- a/tools/libxl/libxl_pci.c > >>>> +++ b/tools/libxl/libxl_pci.c > >>>> @@ -1915,8 +1915,10 @@ static void do_pci_remove(libxl__egc *egc, uint32_t domid, > >>>> goto out_fail; > >>>> } > >>>> } else { > >>>> + /* PCI passthrough can also run on ARM PVH */ > >>>> +#ifndef CONFIG_ARM > >>>> assert(type == LIBXL_DOMAIN_TYPE_PV); > >>>> - > >>>> +#endif > >>> I would just remove the assert now if this is to be used by Arm and > >>> you don't need to fork the file for Arm. > >> Sounds good, I will drop then > >> > >> But what would be the right explanation then? I mean why there was an ASSERT > >> > >> and now it is safe (for x86) to remove that? > > An assert is just a safe belt, the expectation is that it's never hit > > by actual code. Given that this path will now also be used by PVH > > (even if only on Arm) I don't see the point in keeping the assert, and > > making it conditional to != Arm seems worse than just dropping it. > > Ok, so I can write in the patch description something like: > > "this path is now used by PVH, so the assert is no longer valid" > > Does it sound ok? LGTM. Roger.
diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile index 241da7fff6f4..f3806aafcb4e 100644 --- a/tools/libxl/Makefile +++ b/tools/libxl/Makefile @@ -130,6 +130,10 @@ endif LIBXL_LIBS += -lyajl +ifeq ($(CONFIG_ARM),y) +CFALGS += -DCONFIG_ARM +endif + LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \ libxl_dom.o libxl_exec.o libxl_xshelp.o libxl_device.o \ libxl_internal.o libxl_utils.o libxl_uuid.o \ diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c index bc5843b13701..b93cf976642b 100644 --- a/tools/libxl/libxl_pci.c +++ b/tools/libxl/libxl_pci.c @@ -1915,8 +1915,10 @@ static void do_pci_remove(libxl__egc *egc, uint32_t domid, goto out_fail; } } else { + /* PCI passthrough can also run on ARM PVH */ +#ifndef CONFIG_ARM assert(type == LIBXL_DOMAIN_TYPE_PV); - +#endif char *sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/resource", pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func); FILE *f = fopen(sysfs_path, "r");