| Message ID | 20210617233918.10095-1-dpsmith@apertussolutions.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | xsm: refactoring xsm hooks | expand |
On 18/06/2021 00:39, Daniel P. Smith wrote: > Based on feedback from 2021 Xen Developers Summit the xsm-roles RFC > patch set is being split into two separate patch sets. This is the first > patch set and is focused purely on the clean up and refactoring of the > XSM hooks. > > This patch set refactors the xsm_ops wrapper hooks to use the alternative_call > infrastructure. Then proceeds to move and realign the headers to remove the > psuedo is/is not enable implementation. The remainder of the changes are clean up > and removing no longer necessary abstractions. > > <snip> > 51 files changed, 1309 insertions(+), 1413 deletions(-) The diffstat is great, but sadly CI says no. https://gitlab.com/xen-project/patchew/xen/-/pipelines/323044913 The problem is that ARM doesn't have alternative_vcall(). Given how much of an improvement this ought to be for hypercalls, I don't want to lose the vcalls. One option is to implement vcall() support on ARM, but that will leave new architectures (RISC-V on the way) with a heavy lift to get XSM to compile. Instead, what we want to do is make vcall() a common interface, falling back to a plain function pointer call for architectures which don't implement the optimisation. So something like: 1) Introduce CONFIG_HAS_VCALL, which is selected by X86 only right now 2) Introduce xen/vcall.h which uses CONFIG_HAS_VCALL to either include asm/vcall.h or provide the fallback implementation 3) Switch x86's current use over to this new interface The iommu_vcall() is a red herring, not adequately documented, and needs to stay in some form. Specifically, it needs to not become an alternative on ARM, even if ARM gains vcalls. I'd be tempted to rework it in 4) to use the common vcall() by default, and leave ARM as the special case overriding the default behaviour, along with an explanation of why it isn't a vcall(). Obviously, name subject bikeshedding. alternative_vcall() is a bit of a mouthful, and I don't think that alt_vcall() loses any salient information. Thoughts? ~Andrew
On 18.06.2021 12:14, Andrew Cooper wrote: > On 18/06/2021 00:39, Daniel P. Smith wrote: >> Based on feedback from 2021 Xen Developers Summit the xsm-roles RFC >> patch set is being split into two separate patch sets. This is the first >> patch set and is focused purely on the clean up and refactoring of the >> XSM hooks. >> >> This patch set refactors the xsm_ops wrapper hooks to use the alternative_call >> infrastructure. Then proceeds to move and realign the headers to remove the >> psuedo is/is not enable implementation. The remainder of the changes are clean up >> and removing no longer necessary abstractions. >> >> <snip> >> 51 files changed, 1309 insertions(+), 1413 deletions(-) > > The diffstat is great, but sadly CI says no. > https://gitlab.com/xen-project/patchew/xen/-/pipelines/323044913 > > The problem is that ARM doesn't have alternative_vcall(). Given how > much of an improvement this ought to be for hypercalls, I don't want to > lose the vcalls. > > One option is to implement vcall() support on ARM, but that will leave > new architectures (RISC-V on the way) with a heavy lift to get XSM to > compile. > > Instead, what we want to do is make vcall() a common interface, falling > back to a plain function pointer call for architectures which don't > implement the optimisation. So something like: > > 1) Introduce CONFIG_HAS_VCALL, which is selected by X86 only right now > 2) Introduce xen/vcall.h which uses CONFIG_HAS_VCALL to either include > asm/vcall.h or provide the fallback implementation A word on the suggested names: The 'v' in alternative_vcall() stands for "returning void", as opposed to alternative_call(). It's unclear to me what you see it stand for in the names you propose. Jan
On 6/18/21 6:14 AM, Andrew Cooper wrote: > On 18/06/2021 00:39, Daniel P. Smith wrote: >> Based on feedback from 2021 Xen Developers Summit the xsm-roles RFC >> patch set is being split into two separate patch sets. This is the first >> patch set and is focused purely on the clean up and refactoring of the >> XSM hooks. >> >> This patch set refactors the xsm_ops wrapper hooks to use the alternative_call >> infrastructure. Then proceeds to move and realign the headers to remove the >> psuedo is/is not enable implementation. The remainder of the changes are clean up >> and removing no longer necessary abstractions. >> >> <snip> >> 51 files changed, 1309 insertions(+), 1413 deletions(-) > > The diffstat is great, but sadly CI says no. > https://gitlab.com/xen-project/patchew/xen/-/pipelines/323044913 > > The problem is that ARM doesn't have alternative_vcall(). Given how > much of an improvement this ought to be for hypercalls, I don't want to > lose the vcalls. > > One option is to implement vcall() support on ARM, but that will leave > new architectures (RISC-V on the way) with a heavy lift to get XSM to > compile. > > Instead, what we want to do is make vcall() a common interface, falling > back to a plain function pointer call for architectures which don't > implement the optimisation. So something like: > > 1) Introduce CONFIG_HAS_VCALL, which is selected by X86 only right now > 2) Introduce xen/vcall.h which uses CONFIG_HAS_VCALL to either include > asm/vcall.h or provide the fallback implementation > 3) Switch x86's current use over to this new interface > > The iommu_vcall() is a red herring, not adequately documented, and needs > to stay in some form. Specifically, it needs to not become an > alternative on ARM, even if ARM gains vcalls. I'd be tempted to rework > it in 4) to use the common vcall() by default, and leave ARM as the > special case overriding the default behaviour, along with an explanation > of why it isn't a vcall(). > > Obviously, name subject bikeshedding. alternative_vcall() is a bit of a > mouthful, and I don't think that alt_vcall() loses any salient information. > > Thoughts? I can look at spinning an attempt at what you are suggesting and submitting. v/r, dps
On 18/06/2021 12:48, Jan Beulich wrote: > On 18.06.2021 12:14, Andrew Cooper wrote: >> On 18/06/2021 00:39, Daniel P. Smith wrote: >>> Based on feedback from 2021 Xen Developers Summit the xsm-roles RFC >>> patch set is being split into two separate patch sets. This is the first >>> patch set and is focused purely on the clean up and refactoring of the >>> XSM hooks. >>> >>> This patch set refactors the xsm_ops wrapper hooks to use the alternative_call >>> infrastructure. Then proceeds to move and realign the headers to remove the >>> psuedo is/is not enable implementation. The remainder of the changes are clean up >>> and removing no longer necessary abstractions. >>> >>> <snip> >>> 51 files changed, 1309 insertions(+), 1413 deletions(-) >> The diffstat is great, but sadly CI says no. >> https://gitlab.com/xen-project/patchew/xen/-/pipelines/323044913 >> >> The problem is that ARM doesn't have alternative_vcall(). Given how >> much of an improvement this ought to be for hypercalls, I don't want to >> lose the vcalls. >> >> One option is to implement vcall() support on ARM, but that will leave >> new architectures (RISC-V on the way) with a heavy lift to get XSM to >> compile. >> >> Instead, what we want to do is make vcall() a common interface, falling >> back to a plain function pointer call for architectures which don't >> implement the optimisation. So something like: >> >> 1) Introduce CONFIG_HAS_VCALL, which is selected by X86 only right now >> 2) Introduce xen/vcall.h which uses CONFIG_HAS_VCALL to either include >> asm/vcall.h or provide the fallback implementation > A word on the suggested names: The 'v' in alternative_vcall() stands for > "returning void", as opposed to alternative_call(). It's unclear to me > what you see it stand for in the names you propose. Urgh - yet another reason to prefer the Linux static_call() infrastructure. Would a general alt_call name be acceptable? ~Andrew
On 18.06.2021 23:21, Andrew Cooper wrote: > On 18/06/2021 12:48, Jan Beulich wrote: >> On 18.06.2021 12:14, Andrew Cooper wrote: >>> On 18/06/2021 00:39, Daniel P. Smith wrote: >>>> Based on feedback from 2021 Xen Developers Summit the xsm-roles RFC >>>> patch set is being split into two separate patch sets. This is the first >>>> patch set and is focused purely on the clean up and refactoring of the >>>> XSM hooks. >>>> >>>> This patch set refactors the xsm_ops wrapper hooks to use the alternative_call >>>> infrastructure. Then proceeds to move and realign the headers to remove the >>>> psuedo is/is not enable implementation. The remainder of the changes are clean up >>>> and removing no longer necessary abstractions. >>>> >>>> <snip> >>>> 51 files changed, 1309 insertions(+), 1413 deletions(-) >>> The diffstat is great, but sadly CI says no. >>> https://gitlab.com/xen-project/patchew/xen/-/pipelines/323044913 >>> >>> The problem is that ARM doesn't have alternative_vcall(). Given how >>> much of an improvement this ought to be for hypercalls, I don't want to >>> lose the vcalls. >>> >>> One option is to implement vcall() support on ARM, but that will leave >>> new architectures (RISC-V on the way) with a heavy lift to get XSM to >>> compile. >>> >>> Instead, what we want to do is make vcall() a common interface, falling >>> back to a plain function pointer call for architectures which don't >>> implement the optimisation. So something like: >>> >>> 1) Introduce CONFIG_HAS_VCALL, which is selected by X86 only right now >>> 2) Introduce xen/vcall.h which uses CONFIG_HAS_VCALL to either include >>> asm/vcall.h or provide the fallback implementation >> A word on the suggested names: The 'v' in alternative_vcall() stands for >> "returning void", as opposed to alternative_call(). It's unclear to me >> what you see it stand for in the names you propose. > > Urgh - yet another reason to prefer the Linux static_call() infrastructure. Which iirc wasn't there yet when I wrote ours. > Would a general alt_call name be acceptable? Well, it seemed a little terse to me at the time, but I'm not opposed. There's hardly anything else the "alt" there could stand for, I guess. Jan