| Message ID | 20240829010151.2813377-10-samuel.holland@sifive.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | riscv: Userspace pointer masking and tagged address ABI | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/vmtest-fixes-PR | fail | merge-conflict |
On Thu, Aug 29, 2024 at 6:32 AM Samuel Holland <samuel.holland@sifive.com> wrote: > > The interface for controlling pointer masking in VS-mode is henvcfg.PMM, > which is part of the Ssnpm extension, even though pointer masking in > HS-mode is provided by the Smnpm extension. As a result, emulating Smnpm > in the guest requires (only) Ssnpm on the host. > > Since the guest configures Smnpm through the SBI Firmware Features > interface, the extension can be disabled by failing the SBI call. Ssnpm > cannot be disabled without intercepting writes to the senvcfg CSR. > > Signed-off-by: Samuel Holland <samuel.holland@sifive.com> > --- > > (no changes since v2) > > Changes in v2: > - New patch for v2 > > arch/riscv/include/uapi/asm/kvm.h | 2 ++ > arch/riscv/kvm/vcpu_onereg.c | 3 +++ > 2 files changed, 5 insertions(+) > > diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h > index e97db3296456..4f24201376b1 100644 > --- a/arch/riscv/include/uapi/asm/kvm.h > +++ b/arch/riscv/include/uapi/asm/kvm.h > @@ -175,6 +175,8 @@ enum KVM_RISCV_ISA_EXT_ID { > KVM_RISCV_ISA_EXT_ZCF, > KVM_RISCV_ISA_EXT_ZCMOP, > KVM_RISCV_ISA_EXT_ZAWRS, > + KVM_RISCV_ISA_EXT_SMNPM, > + KVM_RISCV_ISA_EXT_SSNPM, > KVM_RISCV_ISA_EXT_MAX, > }; > > diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c > index b319c4c13c54..6f833ec2344a 100644 > --- a/arch/riscv/kvm/vcpu_onereg.c > +++ b/arch/riscv/kvm/vcpu_onereg.c > @@ -34,9 +34,11 @@ static const unsigned long kvm_isa_ext_arr[] = { > [KVM_RISCV_ISA_EXT_M] = RISCV_ISA_EXT_m, > [KVM_RISCV_ISA_EXT_V] = RISCV_ISA_EXT_v, > /* Multi letter extensions (alphabetically sorted) */ > + [KVM_RISCV_ISA_EXT_SMNPM] = RISCV_ISA_EXT_SSNPM, Why not use KVM_ISA_EXT_ARR() macro here ? > KVM_ISA_EXT_ARR(SMSTATEEN), > KVM_ISA_EXT_ARR(SSAIA), > KVM_ISA_EXT_ARR(SSCOFPMF), > + KVM_ISA_EXT_ARR(SSNPM), > KVM_ISA_EXT_ARR(SSTC), > KVM_ISA_EXT_ARR(SVINVAL), > KVM_ISA_EXT_ARR(SVNAPOT), > @@ -129,6 +131,7 @@ static bool kvm_riscv_vcpu_isa_disable_allowed(unsigned long ext) > case KVM_RISCV_ISA_EXT_M: > /* There is not architectural config bit to disable sscofpmf completely */ > case KVM_RISCV_ISA_EXT_SSCOFPMF: > + case KVM_RISCV_ISA_EXT_SSNPM: Why not add KVM_RISCV_ISA_EXT_SMNPM here ? Disabling Smnpm from KVM user space is very different from disabling Smnpm from Guest using SBI FWFT extension. The KVM user space should always add Smnpm in the Guest ISA string whenever the Host ISA string has it. The Guest must explicitly use SBI FWFT to enable Smnpm only after it sees Smnpm in ISA string. > case KVM_RISCV_ISA_EXT_SSTC: > case KVM_RISCV_ISA_EXT_SVINVAL: > case KVM_RISCV_ISA_EXT_SVNAPOT: > -- > 2.45.1 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv Regards, Anup
Hi Anup, On 2024-09-04 7:17 AM, Anup Patel wrote: > On Thu, Aug 29, 2024 at 6:32 AM Samuel Holland > <samuel.holland@sifive.com> wrote: >> >> The interface for controlling pointer masking in VS-mode is henvcfg.PMM, >> which is part of the Ssnpm extension, even though pointer masking in >> HS-mode is provided by the Smnpm extension. As a result, emulating Smnpm >> in the guest requires (only) Ssnpm on the host. >> >> Since the guest configures Smnpm through the SBI Firmware Features >> interface, the extension can be disabled by failing the SBI call. Ssnpm >> cannot be disabled without intercepting writes to the senvcfg CSR. >> >> Signed-off-by: Samuel Holland <samuel.holland@sifive.com> >> --- >> >> (no changes since v2) >> >> Changes in v2: >> - New patch for v2 >> >> arch/riscv/include/uapi/asm/kvm.h | 2 ++ >> arch/riscv/kvm/vcpu_onereg.c | 3 +++ >> 2 files changed, 5 insertions(+) >> >> diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h >> index e97db3296456..4f24201376b1 100644 >> --- a/arch/riscv/include/uapi/asm/kvm.h >> +++ b/arch/riscv/include/uapi/asm/kvm.h >> @@ -175,6 +175,8 @@ enum KVM_RISCV_ISA_EXT_ID { >> KVM_RISCV_ISA_EXT_ZCF, >> KVM_RISCV_ISA_EXT_ZCMOP, >> KVM_RISCV_ISA_EXT_ZAWRS, >> + KVM_RISCV_ISA_EXT_SMNPM, >> + KVM_RISCV_ISA_EXT_SSNPM, >> KVM_RISCV_ISA_EXT_MAX, >> }; >> >> diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c >> index b319c4c13c54..6f833ec2344a 100644 >> --- a/arch/riscv/kvm/vcpu_onereg.c >> +++ b/arch/riscv/kvm/vcpu_onereg.c >> @@ -34,9 +34,11 @@ static const unsigned long kvm_isa_ext_arr[] = { >> [KVM_RISCV_ISA_EXT_M] = RISCV_ISA_EXT_m, >> [KVM_RISCV_ISA_EXT_V] = RISCV_ISA_EXT_v, >> /* Multi letter extensions (alphabetically sorted) */ >> + [KVM_RISCV_ISA_EXT_SMNPM] = RISCV_ISA_EXT_SSNPM, > > Why not use KVM_ISA_EXT_ARR() macro here ? Because the extension name in the host does not match the extension name in the guest. Pointer masking for HS mode is provided by Smnpm. Pointer masking for VS mode is provided by Ssnpm at the hardware level, but this needs to appear to the guest as if Smnpm was implemented, since the guest thinks it is running on bare metal. >> KVM_ISA_EXT_ARR(SMSTATEEN), >> KVM_ISA_EXT_ARR(SSAIA), >> KVM_ISA_EXT_ARR(SSCOFPMF), >> + KVM_ISA_EXT_ARR(SSNPM), >> KVM_ISA_EXT_ARR(SSTC), >> KVM_ISA_EXT_ARR(SVINVAL), >> KVM_ISA_EXT_ARR(SVNAPOT), >> @@ -129,6 +131,7 @@ static bool kvm_riscv_vcpu_isa_disable_allowed(unsigned long ext) >> case KVM_RISCV_ISA_EXT_M: >> /* There is not architectural config bit to disable sscofpmf completely */ >> case KVM_RISCV_ISA_EXT_SSCOFPMF: >> + case KVM_RISCV_ISA_EXT_SSNPM: > > Why not add KVM_RISCV_ISA_EXT_SMNPM here ? > > Disabling Smnpm from KVM user space is very different from > disabling Smnpm from Guest using SBI FWFT extension. Until a successful SBI FWFT call to KVM to enable pointer masking for VS mode, the existence of Smnpm has no visible effect on the guest. So failing the SBI call is sufficient to pretend that the hardware does not support Smnpm. > The KVM user space should always add Smnpm in the > Guest ISA string whenever the Host ISA string has it. I disagree. Allowing userspace to disable extensions is useful for testing and to support migration to hosts which do not support those extensions. So I would only add extensions to this list if there is no possible way to disable them. > The Guest must explicitly use SBI FWFT to enable > Smnpm only after it sees Smnpm in ISA string. Yes, exactly, and the purpose of not including Smnpm in the switch case here is so that KVM user space can control whether or not it appears in the ISA string. Regards, Samuel >> case KVM_RISCV_ISA_EXT_SSTC: >> case KVM_RISCV_ISA_EXT_SVINVAL: >> case KVM_RISCV_ISA_EXT_SVNAPOT: >> -- >> 2.45.1 >> >> >> _______________________________________________ >> linux-riscv mailing list >> linux-riscv@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-riscv > > Regards, > Anup
On Wed, Sep 4, 2024 at 8:01 PM Samuel Holland <samuel.holland@sifive.com> wrote: > > Hi Anup, > > On 2024-09-04 7:17 AM, Anup Patel wrote: > > On Thu, Aug 29, 2024 at 6:32 AM Samuel Holland > > <samuel.holland@sifive.com> wrote: > >> > >> The interface for controlling pointer masking in VS-mode is henvcfg.PMM, > >> which is part of the Ssnpm extension, even though pointer masking in > >> HS-mode is provided by the Smnpm extension. As a result, emulating Smnpm > >> in the guest requires (only) Ssnpm on the host. > >> > >> Since the guest configures Smnpm through the SBI Firmware Features > >> interface, the extension can be disabled by failing the SBI call. Ssnpm > >> cannot be disabled without intercepting writes to the senvcfg CSR. > >> > >> Signed-off-by: Samuel Holland <samuel.holland@sifive.com> > >> --- > >> > >> (no changes since v2) > >> > >> Changes in v2: > >> - New patch for v2 > >> > >> arch/riscv/include/uapi/asm/kvm.h | 2 ++ > >> arch/riscv/kvm/vcpu_onereg.c | 3 +++ > >> 2 files changed, 5 insertions(+) > >> > >> diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h > >> index e97db3296456..4f24201376b1 100644 > >> --- a/arch/riscv/include/uapi/asm/kvm.h > >> +++ b/arch/riscv/include/uapi/asm/kvm.h > >> @@ -175,6 +175,8 @@ enum KVM_RISCV_ISA_EXT_ID { > >> KVM_RISCV_ISA_EXT_ZCF, > >> KVM_RISCV_ISA_EXT_ZCMOP, > >> KVM_RISCV_ISA_EXT_ZAWRS, > >> + KVM_RISCV_ISA_EXT_SMNPM, > >> + KVM_RISCV_ISA_EXT_SSNPM, > >> KVM_RISCV_ISA_EXT_MAX, > >> }; > >> > >> diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c > >> index b319c4c13c54..6f833ec2344a 100644 > >> --- a/arch/riscv/kvm/vcpu_onereg.c > >> +++ b/arch/riscv/kvm/vcpu_onereg.c > >> @@ -34,9 +34,11 @@ static const unsigned long kvm_isa_ext_arr[] = { > >> [KVM_RISCV_ISA_EXT_M] = RISCV_ISA_EXT_m, > >> [KVM_RISCV_ISA_EXT_V] = RISCV_ISA_EXT_v, > >> /* Multi letter extensions (alphabetically sorted) */ > >> + [KVM_RISCV_ISA_EXT_SMNPM] = RISCV_ISA_EXT_SSNPM, > > > > Why not use KVM_ISA_EXT_ARR() macro here ? > > Because the extension name in the host does not match the extension name in the > guest. Pointer masking for HS mode is provided by Smnpm. Pointer masking for VS > mode is provided by Ssnpm at the hardware level, but this needs to appear to the > guest as if Smnpm was implemented, since the guest thinks it is running on bare > metal. Okay, makes sense. > > >> KVM_ISA_EXT_ARR(SMSTATEEN), > >> KVM_ISA_EXT_ARR(SSAIA), > >> KVM_ISA_EXT_ARR(SSCOFPMF), > >> + KVM_ISA_EXT_ARR(SSNPM), > >> KVM_ISA_EXT_ARR(SSTC), > >> KVM_ISA_EXT_ARR(SVINVAL), > >> KVM_ISA_EXT_ARR(SVNAPOT), > >> @@ -129,6 +131,7 @@ static bool kvm_riscv_vcpu_isa_disable_allowed(unsigned long ext) > >> case KVM_RISCV_ISA_EXT_M: > >> /* There is not architectural config bit to disable sscofpmf completely */ > >> case KVM_RISCV_ISA_EXT_SSCOFPMF: > >> + case KVM_RISCV_ISA_EXT_SSNPM: > > > > Why not add KVM_RISCV_ISA_EXT_SMNPM here ? > > > > Disabling Smnpm from KVM user space is very different from > > disabling Smnpm from Guest using SBI FWFT extension. > > Until a successful SBI FWFT call to KVM to enable pointer masking for VS mode, > the existence of Smnpm has no visible effect on the guest. So failing the SBI > call is sufficient to pretend that the hardware does not support Smnpm. > > > The KVM user space should always add Smnpm in the > > Guest ISA string whenever the Host ISA string has it. > > I disagree. Allowing userspace to disable extensions is useful for testing and > to support migration to hosts which do not support those extensions. So I would > only add extensions to this list if there is no possible way to disable them. I am not saying to disallow KVM user space disabling Smnpm. The presence of Smnpm in ISA only means that it is present in HW but it needs to be explicitly configured/enabled using SBI FWFT. KVM user space can certainly disable extensions by not adding it to ISA string based on the KVMTOOL/QEMU-KVM command line option. Additionally, when SBI FWFT is added to KVM RISC-V. It will have its own way to explicitly disable firmware features from KVM user space. > > > The Guest must explicitly use SBI FWFT to enable > > Smnpm only after it sees Smnpm in ISA string. > > Yes, exactly, and the purpose of not including Smnpm in the switch case here is > so that KVM user space can control whether or not it appears in the ISA string. > > Regards, > Samuel > > >> case KVM_RISCV_ISA_EXT_SSTC: > >> case KVM_RISCV_ISA_EXT_SVINVAL: > >> case KVM_RISCV_ISA_EXT_SVNAPOT: > >> -- > >> 2.45.1 > >> > >> > >> _______________________________________________ > >> linux-riscv mailing list > >> linux-riscv@lists.infradead.org > >> http://lists.infradead.org/mailman/listinfo/linux-riscv > > > > Regards, > > Anup > Regards, Anup
Hi Anup, On 2024-09-04 9:45 AM, Anup Patel wrote: > On Wed, Sep 4, 2024 at 8:01 PM Samuel Holland <samuel.holland@sifive.com> wrote: >> On 2024-09-04 7:17 AM, Anup Patel wrote: >>> On Thu, Aug 29, 2024 at 6:32 AM Samuel Holland >>> <samuel.holland@sifive.com> wrote: >>>> >>>> The interface for controlling pointer masking in VS-mode is henvcfg.PMM, >>>> which is part of the Ssnpm extension, even though pointer masking in >>>> HS-mode is provided by the Smnpm extension. As a result, emulating Smnpm >>>> in the guest requires (only) Ssnpm on the host. >>>> >>>> Since the guest configures Smnpm through the SBI Firmware Features >>>> interface, the extension can be disabled by failing the SBI call. Ssnpm >>>> cannot be disabled without intercepting writes to the senvcfg CSR. >>>> >>>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com> >>>> --- >>>> >>>> (no changes since v2) >>>> >>>> Changes in v2: >>>> - New patch for v2 >>>> >>>> arch/riscv/include/uapi/asm/kvm.h | 2 ++ >>>> arch/riscv/kvm/vcpu_onereg.c | 3 +++ >>>> 2 files changed, 5 insertions(+) >>>> >>>> diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h >>>> index e97db3296456..4f24201376b1 100644 >>>> --- a/arch/riscv/include/uapi/asm/kvm.h >>>> +++ b/arch/riscv/include/uapi/asm/kvm.h >>>> @@ -175,6 +175,8 @@ enum KVM_RISCV_ISA_EXT_ID { >>>> KVM_RISCV_ISA_EXT_ZCF, >>>> KVM_RISCV_ISA_EXT_ZCMOP, >>>> KVM_RISCV_ISA_EXT_ZAWRS, >>>> + KVM_RISCV_ISA_EXT_SMNPM, >>>> + KVM_RISCV_ISA_EXT_SSNPM, >>>> KVM_RISCV_ISA_EXT_MAX, >>>> }; >>>> >>>> diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c >>>> index b319c4c13c54..6f833ec2344a 100644 >>>> --- a/arch/riscv/kvm/vcpu_onereg.c >>>> +++ b/arch/riscv/kvm/vcpu_onereg.c >>>> @@ -34,9 +34,11 @@ static const unsigned long kvm_isa_ext_arr[] = { >>>> [KVM_RISCV_ISA_EXT_M] = RISCV_ISA_EXT_m, >>>> [KVM_RISCV_ISA_EXT_V] = RISCV_ISA_EXT_v, >>>> /* Multi letter extensions (alphabetically sorted) */ >>>> + [KVM_RISCV_ISA_EXT_SMNPM] = RISCV_ISA_EXT_SSNPM, >>> >>> Why not use KVM_ISA_EXT_ARR() macro here ? >> >> Because the extension name in the host does not match the extension name in the >> guest. Pointer masking for HS mode is provided by Smnpm. Pointer masking for VS >> mode is provided by Ssnpm at the hardware level, but this needs to appear to the >> guest as if Smnpm was implemented, since the guest thinks it is running on bare >> metal. > > Okay, makes sense. > >> >>>> KVM_ISA_EXT_ARR(SMSTATEEN), >>>> KVM_ISA_EXT_ARR(SSAIA), >>>> KVM_ISA_EXT_ARR(SSCOFPMF), >>>> + KVM_ISA_EXT_ARR(SSNPM), >>>> KVM_ISA_EXT_ARR(SSTC), >>>> KVM_ISA_EXT_ARR(SVINVAL), >>>> KVM_ISA_EXT_ARR(SVNAPOT), >>>> @@ -129,6 +131,7 @@ static bool kvm_riscv_vcpu_isa_disable_allowed(unsigned long ext) >>>> case KVM_RISCV_ISA_EXT_M: >>>> /* There is not architectural config bit to disable sscofpmf completely */ >>>> case KVM_RISCV_ISA_EXT_SSCOFPMF: >>>> + case KVM_RISCV_ISA_EXT_SSNPM: >>> >>> Why not add KVM_RISCV_ISA_EXT_SMNPM here ? >>> >>> Disabling Smnpm from KVM user space is very different from >>> disabling Smnpm from Guest using SBI FWFT extension. >> >> Until a successful SBI FWFT call to KVM to enable pointer masking for VS mode, >> the existence of Smnpm has no visible effect on the guest. So failing the SBI >> call is sufficient to pretend that the hardware does not support Smnpm. >> >>> The KVM user space should always add Smnpm in the >>> Guest ISA string whenever the Host ISA string has it. >> >> I disagree. Allowing userspace to disable extensions is useful for testing and >> to support migration to hosts which do not support those extensions. So I would >> only add extensions to this list if there is no possible way to disable them. > > I am not saying to disallow KVM user space disabling Smnpm. Then I'm confused. This is the "return false;" switch case inside kvm_riscv_vcpu_isa_disable_allowed(). If I add KVM_RISCV_ISA_EXT_SMNPM here, then (unless I am misreading the code) I am disallowing KVM userspace from disabling Smnpm in the guest (i.e. preventing KVM userspace from removing Smnpm from the guest ISA string). If that is not desired, then why do you suggest I add KVM_RISCV_ISA_EXT_SMNPM here? > The presence of Smnpm in ISA only means that it is present in HW > but it needs to be explicitly configured/enabled using SBI FWFT. > > KVM user space can certainly disable extensions by not adding it to > ISA string based on the KVMTOOL/QEMU-KVM command line option. > Additionally, when SBI FWFT is added to KVM RISC-V. It will have its > own way to explicitly disable firmware features from KVM user space. I think we agree on this, but your explanation here appears to conflict with your suggested code change. Apologies if I'm missing something. Regards, Samuel >>> The Guest must explicitly use SBI FWFT to enable >>> Smnpm only after it sees Smnpm in ISA string. >> >> Yes, exactly, and the purpose of not including Smnpm in the switch case here is >> so that KVM user space can control whether or not it appears in the ISA string. >> >> Regards, >> Samuel >> >>>> case KVM_RISCV_ISA_EXT_SSTC: >>>> case KVM_RISCV_ISA_EXT_SVINVAL: >>>> case KVM_RISCV_ISA_EXT_SVNAPOT: >>>> -- >>>> 2.45.1 >>>> >>>> >>>> _______________________________________________ >>>> linux-riscv mailing list >>>> linux-riscv@lists.infradead.org >>>> http://lists.infradead.org/mailman/listinfo/linux-riscv >>> >>> Regards, >>> Anup >> > > Regards, > Anup
On Wed, Sep 4, 2024 at 8:27 PM Samuel Holland <samuel.holland@sifive.com> wrote: > > Hi Anup, > > On 2024-09-04 9:45 AM, Anup Patel wrote: > > On Wed, Sep 4, 2024 at 8:01 PM Samuel Holland <samuel.holland@sifive.com> wrote: > >> On 2024-09-04 7:17 AM, Anup Patel wrote: > >>> On Thu, Aug 29, 2024 at 6:32 AM Samuel Holland > >>> <samuel.holland@sifive.com> wrote: > >>>> > >>>> The interface for controlling pointer masking in VS-mode is henvcfg.PMM, > >>>> which is part of the Ssnpm extension, even though pointer masking in > >>>> HS-mode is provided by the Smnpm extension. As a result, emulating Smnpm > >>>> in the guest requires (only) Ssnpm on the host. > >>>> > >>>> Since the guest configures Smnpm through the SBI Firmware Features > >>>> interface, the extension can be disabled by failing the SBI call. Ssnpm > >>>> cannot be disabled without intercepting writes to the senvcfg CSR. > >>>> > >>>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com> > >>>> --- > >>>> > >>>> (no changes since v2) > >>>> > >>>> Changes in v2: > >>>> - New patch for v2 > >>>> > >>>> arch/riscv/include/uapi/asm/kvm.h | 2 ++ > >>>> arch/riscv/kvm/vcpu_onereg.c | 3 +++ > >>>> 2 files changed, 5 insertions(+) > >>>> > >>>> diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h > >>>> index e97db3296456..4f24201376b1 100644 > >>>> --- a/arch/riscv/include/uapi/asm/kvm.h > >>>> +++ b/arch/riscv/include/uapi/asm/kvm.h > >>>> @@ -175,6 +175,8 @@ enum KVM_RISCV_ISA_EXT_ID { > >>>> KVM_RISCV_ISA_EXT_ZCF, > >>>> KVM_RISCV_ISA_EXT_ZCMOP, > >>>> KVM_RISCV_ISA_EXT_ZAWRS, > >>>> + KVM_RISCV_ISA_EXT_SMNPM, > >>>> + KVM_RISCV_ISA_EXT_SSNPM, > >>>> KVM_RISCV_ISA_EXT_MAX, > >>>> }; > >>>> > >>>> diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c > >>>> index b319c4c13c54..6f833ec2344a 100644 > >>>> --- a/arch/riscv/kvm/vcpu_onereg.c > >>>> +++ b/arch/riscv/kvm/vcpu_onereg.c > >>>> @@ -34,9 +34,11 @@ static const unsigned long kvm_isa_ext_arr[] = { > >>>> [KVM_RISCV_ISA_EXT_M] = RISCV_ISA_EXT_m, > >>>> [KVM_RISCV_ISA_EXT_V] = RISCV_ISA_EXT_v, > >>>> /* Multi letter extensions (alphabetically sorted) */ > >>>> + [KVM_RISCV_ISA_EXT_SMNPM] = RISCV_ISA_EXT_SSNPM, > >>> > >>> Why not use KVM_ISA_EXT_ARR() macro here ? > >> > >> Because the extension name in the host does not match the extension name in the > >> guest. Pointer masking for HS mode is provided by Smnpm. Pointer masking for VS > >> mode is provided by Ssnpm at the hardware level, but this needs to appear to the > >> guest as if Smnpm was implemented, since the guest thinks it is running on bare > >> metal. > > > > Okay, makes sense. > > > >> > >>>> KVM_ISA_EXT_ARR(SMSTATEEN), > >>>> KVM_ISA_EXT_ARR(SSAIA), > >>>> KVM_ISA_EXT_ARR(SSCOFPMF), > >>>> + KVM_ISA_EXT_ARR(SSNPM), > >>>> KVM_ISA_EXT_ARR(SSTC), > >>>> KVM_ISA_EXT_ARR(SVINVAL), > >>>> KVM_ISA_EXT_ARR(SVNAPOT), > >>>> @@ -129,6 +131,7 @@ static bool kvm_riscv_vcpu_isa_disable_allowed(unsigned long ext) > >>>> case KVM_RISCV_ISA_EXT_M: > >>>> /* There is not architectural config bit to disable sscofpmf completely */ > >>>> case KVM_RISCV_ISA_EXT_SSCOFPMF: > >>>> + case KVM_RISCV_ISA_EXT_SSNPM: > >>> > >>> Why not add KVM_RISCV_ISA_EXT_SMNPM here ? > >>> > >>> Disabling Smnpm from KVM user space is very different from > >>> disabling Smnpm from Guest using SBI FWFT extension. > >> > >> Until a successful SBI FWFT call to KVM to enable pointer masking for VS mode, > >> the existence of Smnpm has no visible effect on the guest. So failing the SBI > >> call is sufficient to pretend that the hardware does not support Smnpm. > >> > >>> The KVM user space should always add Smnpm in the > >>> Guest ISA string whenever the Host ISA string has it. > >> > >> I disagree. Allowing userspace to disable extensions is useful for testing and > >> to support migration to hosts which do not support those extensions. So I would > >> only add extensions to this list if there is no possible way to disable them. > > > > I am not saying to disallow KVM user space disabling Smnpm. > > Then I'm confused. This is the "return false;" switch case inside > kvm_riscv_vcpu_isa_disable_allowed(). If I add KVM_RISCV_ISA_EXT_SMNPM here, > then (unless I am misreading the code) I am disallowing KVM userspace from > disabling Smnpm in the guest (i.e. preventing KVM userspace from removing Smnpm > from the guest ISA string). If that is not desired, then why do you suggest I > add KVM_RISCV_ISA_EXT_SMNPM here? Yes, adding KVM_RISCV_ISA_EXT_SMNPM here means KVM user space can't disable it using ONE_REG interface but KVM user space can certainly not add it in the Guest ISA string. > > > The presence of Smnpm in ISA only means that it is present in HW > > but it needs to be explicitly configured/enabled using SBI FWFT. > > > > KVM user space can certainly disable extensions by not adding it to > > ISA string based on the KVMTOOL/QEMU-KVM command line option. > > Additionally, when SBI FWFT is added to KVM RISC-V. It will have its > > own way to explicitly disable firmware features from KVM user space. > > I think we agree on this, but your explanation here appears to conflict with > your suggested code change. Apologies if I'm missing something. I think the confusion is about what does it mean when Smnpm is present in the ISA string. We have two approaches: 1) Presence of Smnpm in ISA string only means it is present in HW but says nothing about its enable/disable state. To configure/enable Smnpm, the supervisor must use SBI FWFT. 2) Presence of Smnpm in ISA string means it is present in HW and enabled at boot-time. To re-configure/disable Smnpm, the supervisor must use SBI FWFT. I am suggesting approach #1 but I am guessing you are leaning towards approach #2 ? For approach #2, additional hencfg.PMM configuration is required in this patch based on the state of KVM_RISCV_ISA_EXT_SMNPM. Regards, Anup > > Regards, > Samuel > > >>> The Guest must explicitly use SBI FWFT to enable > >>> Smnpm only after it sees Smnpm in ISA string. > >> > >> Yes, exactly, and the purpose of not including Smnpm in the switch case here is > >> so that KVM user space can control whether or not it appears in the ISA string. > >> > >> Regards, > >> Samuel > >> > >>>> case KVM_RISCV_ISA_EXT_SSTC: > >>>> case KVM_RISCV_ISA_EXT_SVINVAL: > >>>> case KVM_RISCV_ISA_EXT_SVNAPOT: > >>>> -- > >>>> 2.45.1 > >>>> > >>>> > >>>> _______________________________________________ > >>>> linux-riscv mailing list > >>>> linux-riscv@lists.infradead.org > >>>> http://lists.infradead.org/mailman/listinfo/linux-riscv > >>> > >>> Regards, > >>> Anup > >> > > > > Regards, > > Anup >
On 2024-09-04 10:20 AM, Anup Patel wrote: > On Wed, Sep 4, 2024 at 8:27 PM Samuel Holland <samuel.holland@sifive.com> wrote: >> >> Hi Anup, >> >> On 2024-09-04 9:45 AM, Anup Patel wrote: >>> On Wed, Sep 4, 2024 at 8:01 PM Samuel Holland <samuel.holland@sifive.com> wrote: >>>> On 2024-09-04 7:17 AM, Anup Patel wrote: >>>>> On Thu, Aug 29, 2024 at 6:32 AM Samuel Holland >>>>> <samuel.holland@sifive.com> wrote: >>>>>> >>>>>> The interface for controlling pointer masking in VS-mode is henvcfg.PMM, >>>>>> which is part of the Ssnpm extension, even though pointer masking in >>>>>> HS-mode is provided by the Smnpm extension. As a result, emulating Smnpm >>>>>> in the guest requires (only) Ssnpm on the host. >>>>>> >>>>>> Since the guest configures Smnpm through the SBI Firmware Features >>>>>> interface, the extension can be disabled by failing the SBI call. Ssnpm >>>>>> cannot be disabled without intercepting writes to the senvcfg CSR. >>>>>> >>>>>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com> >>>>>> --- >>>>>> >>>>>> (no changes since v2) >>>>>> >>>>>> Changes in v2: >>>>>> - New patch for v2 >>>>>> >>>>>> arch/riscv/include/uapi/asm/kvm.h | 2 ++ >>>>>> arch/riscv/kvm/vcpu_onereg.c | 3 +++ >>>>>> 2 files changed, 5 insertions(+) >>>>>> >>>>>> diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h >>>>>> index e97db3296456..4f24201376b1 100644 >>>>>> --- a/arch/riscv/include/uapi/asm/kvm.h >>>>>> +++ b/arch/riscv/include/uapi/asm/kvm.h >>>>>> @@ -175,6 +175,8 @@ enum KVM_RISCV_ISA_EXT_ID { >>>>>> KVM_RISCV_ISA_EXT_ZCF, >>>>>> KVM_RISCV_ISA_EXT_ZCMOP, >>>>>> KVM_RISCV_ISA_EXT_ZAWRS, >>>>>> + KVM_RISCV_ISA_EXT_SMNPM, >>>>>> + KVM_RISCV_ISA_EXT_SSNPM, >>>>>> KVM_RISCV_ISA_EXT_MAX, >>>>>> }; >>>>>> >>>>>> diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c >>>>>> index b319c4c13c54..6f833ec2344a 100644 >>>>>> --- a/arch/riscv/kvm/vcpu_onereg.c >>>>>> +++ b/arch/riscv/kvm/vcpu_onereg.c >>>>>> @@ -34,9 +34,11 @@ static const unsigned long kvm_isa_ext_arr[] = { >>>>>> [KVM_RISCV_ISA_EXT_M] = RISCV_ISA_EXT_m, >>>>>> [KVM_RISCV_ISA_EXT_V] = RISCV_ISA_EXT_v, >>>>>> /* Multi letter extensions (alphabetically sorted) */ >>>>>> + [KVM_RISCV_ISA_EXT_SMNPM] = RISCV_ISA_EXT_SSNPM, >>>>> >>>>> Why not use KVM_ISA_EXT_ARR() macro here ? >>>> >>>> Because the extension name in the host does not match the extension name in the >>>> guest. Pointer masking for HS mode is provided by Smnpm. Pointer masking for VS >>>> mode is provided by Ssnpm at the hardware level, but this needs to appear to the >>>> guest as if Smnpm was implemented, since the guest thinks it is running on bare >>>> metal. >>> >>> Okay, makes sense. >>> >>>> >>>>>> KVM_ISA_EXT_ARR(SMSTATEEN), >>>>>> KVM_ISA_EXT_ARR(SSAIA), >>>>>> KVM_ISA_EXT_ARR(SSCOFPMF), >>>>>> + KVM_ISA_EXT_ARR(SSNPM), >>>>>> KVM_ISA_EXT_ARR(SSTC), >>>>>> KVM_ISA_EXT_ARR(SVINVAL), >>>>>> KVM_ISA_EXT_ARR(SVNAPOT), >>>>>> @@ -129,6 +131,7 @@ static bool kvm_riscv_vcpu_isa_disable_allowed(unsigned long ext) >>>>>> case KVM_RISCV_ISA_EXT_M: >>>>>> /* There is not architectural config bit to disable sscofpmf completely */ >>>>>> case KVM_RISCV_ISA_EXT_SSCOFPMF: >>>>>> + case KVM_RISCV_ISA_EXT_SSNPM: >>>>> >>>>> Why not add KVM_RISCV_ISA_EXT_SMNPM here ? >>>>> >>>>> Disabling Smnpm from KVM user space is very different from >>>>> disabling Smnpm from Guest using SBI FWFT extension. >>>> >>>> Until a successful SBI FWFT call to KVM to enable pointer masking for VS mode, >>>> the existence of Smnpm has no visible effect on the guest. So failing the SBI >>>> call is sufficient to pretend that the hardware does not support Smnpm. >>>> >>>>> The KVM user space should always add Smnpm in the >>>>> Guest ISA string whenever the Host ISA string has it. >>>> >>>> I disagree. Allowing userspace to disable extensions is useful for testing and >>>> to support migration to hosts which do not support those extensions. So I would >>>> only add extensions to this list if there is no possible way to disable them. >>> >>> I am not saying to disallow KVM user space disabling Smnpm. >> >> Then I'm confused. This is the "return false;" switch case inside >> kvm_riscv_vcpu_isa_disable_allowed(). If I add KVM_RISCV_ISA_EXT_SMNPM here, >> then (unless I am misreading the code) I am disallowing KVM userspace from >> disabling Smnpm in the guest (i.e. preventing KVM userspace from removing Smnpm >> from the guest ISA string). If that is not desired, then why do you suggest I >> add KVM_RISCV_ISA_EXT_SMNPM here? > > Yes, adding KVM_RISCV_ISA_EXT_SMNPM here means KVM > user space can't disable it using ONE_REG interface but KVM user > space can certainly not add it in the Guest ISA string. Is there a problem with allowing KVM userspace to disable the ISA extension with the ONE_REG interface? If KVM userspace removes Smnpm from the ISA string without the host kernel's knowledge, that doesn't actually prevent the guest from successfully calling sbi_fwft_set(POINTER_MASKING_PMLEN, ...), so it doesn't guarantee that the VM can be migrated to a host without pointer masking support. So the ONE_REG interface still has value. (And that's my answer to your original question "Why not add KVM_RISCV_ISA_EXT_SMNPM here ?") >>> The presence of Smnpm in ISA only means that it is present in HW >>> but it needs to be explicitly configured/enabled using SBI FWFT. >>> >>> KVM user space can certainly disable extensions by not adding it to >>> ISA string based on the KVMTOOL/QEMU-KVM command line option. >>> Additionally, when SBI FWFT is added to KVM RISC-V. It will have its >>> own way to explicitly disable firmware features from KVM user space. >> >> I think we agree on this, but your explanation here appears to conflict with >> your suggested code change. Apologies if I'm missing something. > > I think the confusion is about what does it mean when Smnpm is present > in the ISA string. We have two approaches: > > 1) Presence of Smnpm in ISA string only means it is present in HW but > says nothing about its enable/disable state. To configure/enable > Smnpm, the supervisor must use SBI FWFT. > > 2) Presence of Smnpm in ISA string means it is present in HW and > enabled at boot-time. To re-configure/disable Smnpm, the supervisor > must use SBI FWFT. > > I am suggesting approach #1 but I am guessing you are leaning towards > approach #2 ? > > For approach #2, additional hencfg.PMM configuration is required in > this patch based on the state of KVM_RISCV_ISA_EXT_SMNPM. No, I am definitely suggesting only approach #1. My proposal for adding pointer masking to the SBI FWFT extension[1] specifies the feature as disabled by default, and this would apply both inside and ouside a VM. But I am also suggesting that the ONE_REG interface is a useful way to completely hide the extension from the guest, like we do for other extensions such as Svpbmt. The only difference between something like Svpbmt and Smnpm is that instead of clearing a bit in henvcfg to hide the extension from the guest, we reject calls to sbi_fwft_set(POINTER_MASKING_PMLEN, ...) when the ISA extension is hidden from the guest. Regards, Samuel [1]: https://github.com/riscv-non-isa/riscv-sbi-doc/pull/161
On Wed, Sep 4, 2024 at 9:25 PM Samuel Holland <samuel.holland@sifive.com> wrote: > > On 2024-09-04 10:20 AM, Anup Patel wrote: > > On Wed, Sep 4, 2024 at 8:27 PM Samuel Holland <samuel.holland@sifive.com> wrote: > >> > >> Hi Anup, > >> > >> On 2024-09-04 9:45 AM, Anup Patel wrote: > >>> On Wed, Sep 4, 2024 at 8:01 PM Samuel Holland <samuel.holland@sifive.com> wrote: > >>>> On 2024-09-04 7:17 AM, Anup Patel wrote: > >>>>> On Thu, Aug 29, 2024 at 6:32 AM Samuel Holland > >>>>> <samuel.holland@sifive.com> wrote: > >>>>>> > >>>>>> The interface for controlling pointer masking in VS-mode is henvcfg.PMM, > >>>>>> which is part of the Ssnpm extension, even though pointer masking in > >>>>>> HS-mode is provided by the Smnpm extension. As a result, emulating Smnpm > >>>>>> in the guest requires (only) Ssnpm on the host. > >>>>>> > >>>>>> Since the guest configures Smnpm through the SBI Firmware Features > >>>>>> interface, the extension can be disabled by failing the SBI call. Ssnpm > >>>>>> cannot be disabled without intercepting writes to the senvcfg CSR. > >>>>>> > >>>>>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com> > >>>>>> --- > >>>>>> > >>>>>> (no changes since v2) > >>>>>> > >>>>>> Changes in v2: > >>>>>> - New patch for v2 > >>>>>> > >>>>>> arch/riscv/include/uapi/asm/kvm.h | 2 ++ > >>>>>> arch/riscv/kvm/vcpu_onereg.c | 3 +++ > >>>>>> 2 files changed, 5 insertions(+) > >>>>>> > >>>>>> diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h > >>>>>> index e97db3296456..4f24201376b1 100644 > >>>>>> --- a/arch/riscv/include/uapi/asm/kvm.h > >>>>>> +++ b/arch/riscv/include/uapi/asm/kvm.h > >>>>>> @@ -175,6 +175,8 @@ enum KVM_RISCV_ISA_EXT_ID { > >>>>>> KVM_RISCV_ISA_EXT_ZCF, > >>>>>> KVM_RISCV_ISA_EXT_ZCMOP, > >>>>>> KVM_RISCV_ISA_EXT_ZAWRS, > >>>>>> + KVM_RISCV_ISA_EXT_SMNPM, > >>>>>> + KVM_RISCV_ISA_EXT_SSNPM, > >>>>>> KVM_RISCV_ISA_EXT_MAX, > >>>>>> }; > >>>>>> > >>>>>> diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c > >>>>>> index b319c4c13c54..6f833ec2344a 100644 > >>>>>> --- a/arch/riscv/kvm/vcpu_onereg.c > >>>>>> +++ b/arch/riscv/kvm/vcpu_onereg.c > >>>>>> @@ -34,9 +34,11 @@ static const unsigned long kvm_isa_ext_arr[] = { > >>>>>> [KVM_RISCV_ISA_EXT_M] = RISCV_ISA_EXT_m, > >>>>>> [KVM_RISCV_ISA_EXT_V] = RISCV_ISA_EXT_v, > >>>>>> /* Multi letter extensions (alphabetically sorted) */ > >>>>>> + [KVM_RISCV_ISA_EXT_SMNPM] = RISCV_ISA_EXT_SSNPM, > >>>>> > >>>>> Why not use KVM_ISA_EXT_ARR() macro here ? > >>>> > >>>> Because the extension name in the host does not match the extension name in the > >>>> guest. Pointer masking for HS mode is provided by Smnpm. Pointer masking for VS > >>>> mode is provided by Ssnpm at the hardware level, but this needs to appear to the > >>>> guest as if Smnpm was implemented, since the guest thinks it is running on bare > >>>> metal. > >>> > >>> Okay, makes sense. > >>> > >>>> > >>>>>> KVM_ISA_EXT_ARR(SMSTATEEN), > >>>>>> KVM_ISA_EXT_ARR(SSAIA), > >>>>>> KVM_ISA_EXT_ARR(SSCOFPMF), > >>>>>> + KVM_ISA_EXT_ARR(SSNPM), > >>>>>> KVM_ISA_EXT_ARR(SSTC), > >>>>>> KVM_ISA_EXT_ARR(SVINVAL), > >>>>>> KVM_ISA_EXT_ARR(SVNAPOT), > >>>>>> @@ -129,6 +131,7 @@ static bool kvm_riscv_vcpu_isa_disable_allowed(unsigned long ext) > >>>>>> case KVM_RISCV_ISA_EXT_M: > >>>>>> /* There is not architectural config bit to disable sscofpmf completely */ > >>>>>> case KVM_RISCV_ISA_EXT_SSCOFPMF: > >>>>>> + case KVM_RISCV_ISA_EXT_SSNPM: > >>>>> > >>>>> Why not add KVM_RISCV_ISA_EXT_SMNPM here ? > >>>>> > >>>>> Disabling Smnpm from KVM user space is very different from > >>>>> disabling Smnpm from Guest using SBI FWFT extension. > >>>> > >>>> Until a successful SBI FWFT call to KVM to enable pointer masking for VS mode, > >>>> the existence of Smnpm has no visible effect on the guest. So failing the SBI > >>>> call is sufficient to pretend that the hardware does not support Smnpm. > >>>> > >>>>> The KVM user space should always add Smnpm in the > >>>>> Guest ISA string whenever the Host ISA string has it. > >>>> > >>>> I disagree. Allowing userspace to disable extensions is useful for testing and > >>>> to support migration to hosts which do not support those extensions. So I would > >>>> only add extensions to this list if there is no possible way to disable them. > >>> > >>> I am not saying to disallow KVM user space disabling Smnpm. > >> > >> Then I'm confused. This is the "return false;" switch case inside > >> kvm_riscv_vcpu_isa_disable_allowed(). If I add KVM_RISCV_ISA_EXT_SMNPM here, > >> then (unless I am misreading the code) I am disallowing KVM userspace from > >> disabling Smnpm in the guest (i.e. preventing KVM userspace from removing Smnpm > >> from the guest ISA string). If that is not desired, then why do you suggest I > >> add KVM_RISCV_ISA_EXT_SMNPM here? > > > > Yes, adding KVM_RISCV_ISA_EXT_SMNPM here means KVM > > user space can't disable it using ONE_REG interface but KVM user > > space can certainly not add it in the Guest ISA string. > > Is there a problem with allowing KVM userspace to disable the ISA extension with > the ONE_REG interface? > > If KVM userspace removes Smnpm from the ISA string without the host kernel's > knowledge, that doesn't actually prevent the guest from successfully calling > sbi_fwft_set(POINTER_MASKING_PMLEN, ...), so it doesn't guarantee that the VM > can be migrated to a host without pointer masking support. So the ONE_REG > interface still has value. (And that's my answer to your original question "Why > not add KVM_RISCV_ISA_EXT_SMNPM here ?") Currently, disabling KVM_RISCV_ISA_EXT_SMNPM via ONE_REG will only clear the corresponding bit in VCPU isa bitmap. Basically, the KVM user space disabling KVM_RISCV_ISA_EXT_SMNPM for Guest changes nothing for the Guest/VM. On other hand, disabling KVM_RISCV_ISA_EXT_SVPBMT via ONE_REG will not only clear it from VCPU isa bitmap but also disable Svpmbt from henvcfg CSR for the Guest/VM. In other words, if disabling an ISA extension is allowed by the kvm_riscv_vcpu_isa_disable_allowed() then the Guest/VM must see a different behaviour when the ISA extension is disabled by KVM user space. > > >>> The presence of Smnpm in ISA only means that it is present in HW > >>> but it needs to be explicitly configured/enabled using SBI FWFT. > >>> > >>> KVM user space can certainly disable extensions by not adding it to > >>> ISA string based on the KVMTOOL/QEMU-KVM command line option. > >>> Additionally, when SBI FWFT is added to KVM RISC-V. It will have its > >>> own way to explicitly disable firmware features from KVM user space. > >> > >> I think we agree on this, but your explanation here appears to conflict with > >> your suggested code change. Apologies if I'm missing something. > > > > I think the confusion is about what does it mean when Smnpm is present > > in the ISA string. We have two approaches: > > > > 1) Presence of Smnpm in ISA string only means it is present in HW but > > says nothing about its enable/disable state. To configure/enable > > Smnpm, the supervisor must use SBI FWFT. > > > > 2) Presence of Smnpm in ISA string means it is present in HW and > > enabled at boot-time. To re-configure/disable Smnpm, the supervisor > > must use SBI FWFT. > > > > I am suggesting approach #1 but I am guessing you are leaning towards > > approach #2 ? > > > > For approach #2, additional hencfg.PMM configuration is required in > > this patch based on the state of KVM_RISCV_ISA_EXT_SMNPM. > > No, I am definitely suggesting only approach #1. My proposal for adding pointer > masking to the SBI FWFT extension[1] specifies the feature as disabled by > default, and this would apply both inside and ouside a VM. > > But I am also suggesting that the ONE_REG interface is a useful way to > completely hide the extension from the guest, like we do for other extensions > such as Svpbmt. The only difference between something like Svpbmt and Smnpm is > that instead of clearing a bit in henvcfg to hide the extension from the guest, > we reject calls to sbi_fwft_set(POINTER_MASKING_PMLEN, ...) when the ISA > extension is hidden from the guest. I think we are converging towards the same thing. How about this ? For this series, lets add KVM_RISCV_ISA_EXT_SMNPM to kvm_riscv_vcpu_isa_disable_allowed() so that for the time being KVM user space can't disable Smnpm. In the future, a separate series which adds SBI FWFT to KVM RISC-V will remove KVM_RISCV_ISA_EXT_SMNPM from the kvm_riscv_vcpu_isa_disable_allowed() because disabling Smnpm from KVM user space would mean that the POINTER_MASKING_PMLEN firmware feature is not available to the Guest/VM. This means in the future (after SBI FWFT is implemented in KVM RISC-V), Guest with Smnpm disabled can be migrated to a host without pointer masking. Regards, Anup
Hi Anup, On 2024-09-05 12:18 AM, Anup Patel wrote: > On Wed, Sep 4, 2024 at 9:25 PM Samuel Holland <samuel.holland@sifive.com> wrote: >> >> On 2024-09-04 10:20 AM, Anup Patel wrote: >>> On Wed, Sep 4, 2024 at 8:27 PM Samuel Holland <samuel.holland@sifive.com> wrote: >>>> >>>> Hi Anup, >>>> >>>> On 2024-09-04 9:45 AM, Anup Patel wrote: >>>>> On Wed, Sep 4, 2024 at 8:01 PM Samuel Holland <samuel.holland@sifive.com> wrote: >>>>>> On 2024-09-04 7:17 AM, Anup Patel wrote: >>>>>>> On Thu, Aug 29, 2024 at 6:32 AM Samuel Holland >>>>>>> <samuel.holland@sifive.com> wrote: >>>>>>>> >>>>>>>> The interface for controlling pointer masking in VS-mode is henvcfg.PMM, >>>>>>>> which is part of the Ssnpm extension, even though pointer masking in >>>>>>>> HS-mode is provided by the Smnpm extension. As a result, emulating Smnpm >>>>>>>> in the guest requires (only) Ssnpm on the host. >>>>>>>> >>>>>>>> Since the guest configures Smnpm through the SBI Firmware Features >>>>>>>> interface, the extension can be disabled by failing the SBI call. Ssnpm >>>>>>>> cannot be disabled without intercepting writes to the senvcfg CSR. >>>>>>>> >>>>>>>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com> >>>>>>>> --- >>>>>>>> >>>>>>>> (no changes since v2) >>>>>>>> >>>>>>>> Changes in v2: >>>>>>>> - New patch for v2 >>>>>>>> >>>>>>>> arch/riscv/include/uapi/asm/kvm.h | 2 ++ >>>>>>>> arch/riscv/kvm/vcpu_onereg.c | 3 +++ >>>>>>>> 2 files changed, 5 insertions(+) >>>>>>>> >>>>>>>> diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h >>>>>>>> index e97db3296456..4f24201376b1 100644 >>>>>>>> --- a/arch/riscv/include/uapi/asm/kvm.h >>>>>>>> +++ b/arch/riscv/include/uapi/asm/kvm.h >>>>>>>> @@ -175,6 +175,8 @@ enum KVM_RISCV_ISA_EXT_ID { >>>>>>>> KVM_RISCV_ISA_EXT_ZCF, >>>>>>>> KVM_RISCV_ISA_EXT_ZCMOP, >>>>>>>> KVM_RISCV_ISA_EXT_ZAWRS, >>>>>>>> + KVM_RISCV_ISA_EXT_SMNPM, >>>>>>>> + KVM_RISCV_ISA_EXT_SSNPM, >>>>>>>> KVM_RISCV_ISA_EXT_MAX, >>>>>>>> }; >>>>>>>> >>>>>>>> diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c >>>>>>>> index b319c4c13c54..6f833ec2344a 100644 >>>>>>>> --- a/arch/riscv/kvm/vcpu_onereg.c >>>>>>>> +++ b/arch/riscv/kvm/vcpu_onereg.c >>>>>>>> @@ -34,9 +34,11 @@ static const unsigned long kvm_isa_ext_arr[] = { >>>>>>>> [KVM_RISCV_ISA_EXT_M] = RISCV_ISA_EXT_m, >>>>>>>> [KVM_RISCV_ISA_EXT_V] = RISCV_ISA_EXT_v, >>>>>>>> /* Multi letter extensions (alphabetically sorted) */ >>>>>>>> + [KVM_RISCV_ISA_EXT_SMNPM] = RISCV_ISA_EXT_SSNPM, >>>>>>> >>>>>>> Why not use KVM_ISA_EXT_ARR() macro here ? >>>>>> >>>>>> Because the extension name in the host does not match the extension name in the >>>>>> guest. Pointer masking for HS mode is provided by Smnpm. Pointer masking for VS >>>>>> mode is provided by Ssnpm at the hardware level, but this needs to appear to the >>>>>> guest as if Smnpm was implemented, since the guest thinks it is running on bare >>>>>> metal. >>>>> >>>>> Okay, makes sense. >>>>> >>>>>> >>>>>>>> KVM_ISA_EXT_ARR(SMSTATEEN), >>>>>>>> KVM_ISA_EXT_ARR(SSAIA), >>>>>>>> KVM_ISA_EXT_ARR(SSCOFPMF), >>>>>>>> + KVM_ISA_EXT_ARR(SSNPM), >>>>>>>> KVM_ISA_EXT_ARR(SSTC), >>>>>>>> KVM_ISA_EXT_ARR(SVINVAL), >>>>>>>> KVM_ISA_EXT_ARR(SVNAPOT), >>>>>>>> @@ -129,6 +131,7 @@ static bool kvm_riscv_vcpu_isa_disable_allowed(unsigned long ext) >>>>>>>> case KVM_RISCV_ISA_EXT_M: >>>>>>>> /* There is not architectural config bit to disable sscofpmf completely */ >>>>>>>> case KVM_RISCV_ISA_EXT_SSCOFPMF: >>>>>>>> + case KVM_RISCV_ISA_EXT_SSNPM: >>>>>>> >>>>>>> Why not add KVM_RISCV_ISA_EXT_SMNPM here ? >>>>>>> >>>>>>> Disabling Smnpm from KVM user space is very different from >>>>>>> disabling Smnpm from Guest using SBI FWFT extension. >>>>>> >>>>>> Until a successful SBI FWFT call to KVM to enable pointer masking for VS mode, >>>>>> the existence of Smnpm has no visible effect on the guest. So failing the SBI >>>>>> call is sufficient to pretend that the hardware does not support Smnpm. >>>>>> >>>>>>> The KVM user space should always add Smnpm in the >>>>>>> Guest ISA string whenever the Host ISA string has it. >>>>>> >>>>>> I disagree. Allowing userspace to disable extensions is useful for testing and >>>>>> to support migration to hosts which do not support those extensions. So I would >>>>>> only add extensions to this list if there is no possible way to disable them. >>>>> >>>>> I am not saying to disallow KVM user space disabling Smnpm. >>>> >>>> Then I'm confused. This is the "return false;" switch case inside >>>> kvm_riscv_vcpu_isa_disable_allowed(). If I add KVM_RISCV_ISA_EXT_SMNPM here, >>>> then (unless I am misreading the code) I am disallowing KVM userspace from >>>> disabling Smnpm in the guest (i.e. preventing KVM userspace from removing Smnpm >>>> from the guest ISA string). If that is not desired, then why do you suggest I >>>> add KVM_RISCV_ISA_EXT_SMNPM here? >>> >>> Yes, adding KVM_RISCV_ISA_EXT_SMNPM here means KVM >>> user space can't disable it using ONE_REG interface but KVM user >>> space can certainly not add it in the Guest ISA string. >> >> Is there a problem with allowing KVM userspace to disable the ISA extension with >> the ONE_REG interface? >> >> If KVM userspace removes Smnpm from the ISA string without the host kernel's >> knowledge, that doesn't actually prevent the guest from successfully calling >> sbi_fwft_set(POINTER_MASKING_PMLEN, ...), so it doesn't guarantee that the VM >> can be migrated to a host without pointer masking support. So the ONE_REG >> interface still has value. (And that's my answer to your original question "Why >> not add KVM_RISCV_ISA_EXT_SMNPM here ?") > > Currently, disabling KVM_RISCV_ISA_EXT_SMNPM via ONE_REG > will only clear the corresponding bit in VCPU isa bitmap. Basically, the > KVM user space disabling KVM_RISCV_ISA_EXT_SMNPM for Guest > changes nothing for the Guest/VM. > > On other hand, disabling KVM_RISCV_ISA_EXT_SVPBMT via > ONE_REG will not only clear it from VCPU isa bitmap but also > disable Svpmbt from henvcfg CSR for the Guest/VM. > > In other words, if disabling an ISA extension is allowed by the > kvm_riscv_vcpu_isa_disable_allowed() then the Guest/VM must > see a different behaviour when the ISA extension is disabled by > KVM user space. > >> >>>>> The presence of Smnpm in ISA only means that it is present in HW >>>>> but it needs to be explicitly configured/enabled using SBI FWFT. >>>>> >>>>> KVM user space can certainly disable extensions by not adding it to >>>>> ISA string based on the KVMTOOL/QEMU-KVM command line option. >>>>> Additionally, when SBI FWFT is added to KVM RISC-V. It will have its >>>>> own way to explicitly disable firmware features from KVM user space. >>>> >>>> I think we agree on this, but your explanation here appears to conflict with >>>> your suggested code change. Apologies if I'm missing something. >>> >>> I think the confusion is about what does it mean when Smnpm is present >>> in the ISA string. We have two approaches: >>> >>> 1) Presence of Smnpm in ISA string only means it is present in HW but >>> says nothing about its enable/disable state. To configure/enable >>> Smnpm, the supervisor must use SBI FWFT. >>> >>> 2) Presence of Smnpm in ISA string means it is present in HW and >>> enabled at boot-time. To re-configure/disable Smnpm, the supervisor >>> must use SBI FWFT. >>> >>> I am suggesting approach #1 but I am guessing you are leaning towards >>> approach #2 ? >>> >>> For approach #2, additional hencfg.PMM configuration is required in >>> this patch based on the state of KVM_RISCV_ISA_EXT_SMNPM. >> >> No, I am definitely suggesting only approach #1. My proposal for adding pointer >> masking to the SBI FWFT extension[1] specifies the feature as disabled by >> default, and this would apply both inside and ouside a VM. >> >> But I am also suggesting that the ONE_REG interface is a useful way to >> completely hide the extension from the guest, like we do for other extensions >> such as Svpbmt. The only difference between something like Svpbmt and Smnpm is >> that instead of clearing a bit in henvcfg to hide the extension from the guest, >> we reject calls to sbi_fwft_set(POINTER_MASKING_PMLEN, ...) when the ISA >> extension is hidden from the guest. > > I think we are converging towards the same thing. > > How about this ? > > For this series, lets add KVM_RISCV_ISA_EXT_SMNPM to > kvm_riscv_vcpu_isa_disable_allowed() so that for the time > being KVM user space can't disable Smnpm. > > In the future, a separate series which adds SBI FWFT to > KVM RISC-V will remove KVM_RISCV_ISA_EXT_SMNPM > from the kvm_riscv_vcpu_isa_disable_allowed() because > disabling Smnpm from KVM user space would mean that > the POINTER_MASKING_PMLEN firmware feature is > not available to the Guest/VM. > > This means in the future (after SBI FWFT is implemented in > KVM RISC-V), Guest with Smnpm disabled can be migrated > to a host without pointer masking. OK, that is a reasonable compromise. I'll do that for v5. Regards, Samuel
diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h index e97db3296456..4f24201376b1 100644 --- a/arch/riscv/include/uapi/asm/kvm.h +++ b/arch/riscv/include/uapi/asm/kvm.h @@ -175,6 +175,8 @@ enum KVM_RISCV_ISA_EXT_ID { KVM_RISCV_ISA_EXT_ZCF, KVM_RISCV_ISA_EXT_ZCMOP, KVM_RISCV_ISA_EXT_ZAWRS, + KVM_RISCV_ISA_EXT_SMNPM, + KVM_RISCV_ISA_EXT_SSNPM, KVM_RISCV_ISA_EXT_MAX, }; diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c index b319c4c13c54..6f833ec2344a 100644 --- a/arch/riscv/kvm/vcpu_onereg.c +++ b/arch/riscv/kvm/vcpu_onereg.c @@ -34,9 +34,11 @@ static const unsigned long kvm_isa_ext_arr[] = { [KVM_RISCV_ISA_EXT_M] = RISCV_ISA_EXT_m, [KVM_RISCV_ISA_EXT_V] = RISCV_ISA_EXT_v, /* Multi letter extensions (alphabetically sorted) */ + [KVM_RISCV_ISA_EXT_SMNPM] = RISCV_ISA_EXT_SSNPM, KVM_ISA_EXT_ARR(SMSTATEEN), KVM_ISA_EXT_ARR(SSAIA), KVM_ISA_EXT_ARR(SSCOFPMF), + KVM_ISA_EXT_ARR(SSNPM), KVM_ISA_EXT_ARR(SSTC), KVM_ISA_EXT_ARR(SVINVAL), KVM_ISA_EXT_ARR(SVNAPOT), @@ -129,6 +131,7 @@ static bool kvm_riscv_vcpu_isa_disable_allowed(unsigned long ext) case KVM_RISCV_ISA_EXT_M: /* There is not architectural config bit to disable sscofpmf completely */ case KVM_RISCV_ISA_EXT_SSCOFPMF: + case KVM_RISCV_ISA_EXT_SSNPM: case KVM_RISCV_ISA_EXT_SSTC: case KVM_RISCV_ISA_EXT_SVINVAL: case KVM_RISCV_ISA_EXT_SVNAPOT:
The interface for controlling pointer masking in VS-mode is henvcfg.PMM, which is part of the Ssnpm extension, even though pointer masking in HS-mode is provided by the Smnpm extension. As a result, emulating Smnpm in the guest requires (only) Ssnpm on the host. Since the guest configures Smnpm through the SBI Firmware Features interface, the extension can be disabled by failing the SBI call. Ssnpm cannot be disabled without intercepting writes to the senvcfg CSR. Signed-off-by: Samuel Holland <samuel.holland@sifive.com> --- (no changes since v2) Changes in v2: - New patch for v2 arch/riscv/include/uapi/asm/kvm.h | 2 ++ arch/riscv/kvm/vcpu_onereg.c | 3 +++ 2 files changed, 5 insertions(+)