| Message ID | 20250513163438.3942405-15-tabba@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: Mapping guest_memfd backed memory at the host for software protected VMs | expand |
On Tue, May 13, 2025 at 9:35 AM Fuad Tabba <tabba@google.com> wrote: > > Enable mapping guest_memfd in arm64. For now, it applies to all > VMs in arm64 that use guest_memfd. In the future, new VM types > can restrict this via kvm_arch_gmem_supports_shared_mem(). > > Signed-off-by: Fuad Tabba <tabba@google.com> > --- > arch/arm64/include/asm/kvm_host.h | 10 ++++++++++ > arch/arm64/kvm/Kconfig | 1 + > 2 files changed, 11 insertions(+) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 08ba91e6fb03..2514779f5131 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -1593,4 +1593,14 @@ static inline bool kvm_arch_has_irq_bypass(void) > return true; > } > > +static inline bool kvm_arch_supports_gmem(struct kvm *kvm) > +{ > + return IS_ENABLED(CONFIG_KVM_GMEM); > +} This is written as if it is okay for CONFIG_KVM_GMEM not to be enabled, but when disabling CONFIG_KVM_GMEM you will get an error for redefining kvm_arch_supports_gmem(). I think you either want to include: #define kvm_arch_supports_gmem kvm_arch_supports_gmem or just do something closer to what x86 does: #ifdef CONFIG_KVM_GMEM #define kvm_arch_supports_gmem(kvm) true #endif > + > +static inline bool kvm_arch_vm_supports_gmem_shared_mem(struct kvm *kvm) > +{ > + return IS_ENABLED(CONFIG_KVM_GMEM_SHARED_MEM); > +} And this applies here as well. #define kvm_arch_vm_supports_gmem_shared_mem kvm_arch_vm_supports_gmem_shared_mem or #ifdef CONFIG_KVM_GMEM #define kvm_arch_vm_supports_gmem_shared_mem(kvm) IS_ENABLED(CONFIG_KVM_GMEM_SHARED_MEM); #endif > + > #endif /* __ARM64_KVM_HOST_H__ */ > diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig > index 096e45acadb2..8c1e1964b46a 100644 > --- a/arch/arm64/kvm/Kconfig > +++ b/arch/arm64/kvm/Kconfig > @@ -38,6 +38,7 @@ menuconfig KVM > select HAVE_KVM_VCPU_RUN_PID_CHANGE > select SCHED_INFO > select GUEST_PERF_EVENTS if PERF_EVENTS > + select KVM_GMEM_SHARED_MEM This makes it impossible to see the error, but I think we should fix it anyway. :) > help > Support hosting virtualized guest machines. > > -- > 2.49.0.1045.g170613ef41-goog >
Hi David, On Wed, 21 May 2025 at 09:05, David Hildenbrand <david@redhat.com> wrote: > > On 13.05.25 18:34, Fuad Tabba wrote: > > Enable mapping guest_memfd in arm64. For now, it applies to all > > VMs in arm64 that use guest_memfd. In the future, new VM types > > can restrict this via kvm_arch_gmem_supports_shared_mem(). > > > > Signed-off-by: Fuad Tabba <tabba@google.com> > > --- > > arch/arm64/include/asm/kvm_host.h | 10 ++++++++++ > > arch/arm64/kvm/Kconfig | 1 + > > 2 files changed, 11 insertions(+) > > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > > index 08ba91e6fb03..2514779f5131 100644 > > --- a/arch/arm64/include/asm/kvm_host.h > > +++ b/arch/arm64/include/asm/kvm_host.h > > @@ -1593,4 +1593,14 @@ static inline bool kvm_arch_has_irq_bypass(void) > > return true; > > } > > > > +static inline bool kvm_arch_supports_gmem(struct kvm *kvm) > > +{ > > + return IS_ENABLED(CONFIG_KVM_GMEM); > > +} > > + > > +static inline bool kvm_arch_vm_supports_gmem_shared_mem(struct kvm *kvm) > > +{ > > + return IS_ENABLED(CONFIG_KVM_GMEM_SHARED_MEM); > > +} > > + > > #endif /* __ARM64_KVM_HOST_H__ */ > > diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig > > index 096e45acadb2..8c1e1964b46a 100644 > > --- a/arch/arm64/kvm/Kconfig > > +++ b/arch/arm64/kvm/Kconfig > > @@ -38,6 +38,7 @@ menuconfig KVM > > select HAVE_KVM_VCPU_RUN_PID_CHANGE > > select SCHED_INFO > > select GUEST_PERF_EVENTS if PERF_EVENTS > > + select KVM_GMEM_SHARED_MEM > > help > > Support hosting virtualized guest machines. > > > > Do we have to reject somewhere if we are given a guest_memfd that was > *not* created using the SHARED flag? Or will existing checks already > reject that? We don't reject, but I don't think we need to. A user can create a guest_memfd that's private in arm64, it would just be useless. Cheers, /fuad > -- > Cheers, > > David / dhildenb >
On 21.05.25 12:29, Fuad Tabba wrote: > On Wed, 21 May 2025 at 11:26, David Hildenbrand <david@redhat.com> wrote: >> >> On 21.05.25 12:12, Fuad Tabba wrote: >>> Hi David, >>> >>> On Wed, 21 May 2025 at 09:05, David Hildenbrand <david@redhat.com> wrote: >>>> >>>> On 13.05.25 18:34, Fuad Tabba wrote: >>>>> Enable mapping guest_memfd in arm64. For now, it applies to all >>>>> VMs in arm64 that use guest_memfd. In the future, new VM types >>>>> can restrict this via kvm_arch_gmem_supports_shared_mem(). >>>>> >>>>> Signed-off-by: Fuad Tabba <tabba@google.com> >>>>> --- >>>>> arch/arm64/include/asm/kvm_host.h | 10 ++++++++++ >>>>> arch/arm64/kvm/Kconfig | 1 + >>>>> 2 files changed, 11 insertions(+) >>>>> >>>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >>>>> index 08ba91e6fb03..2514779f5131 100644 >>>>> --- a/arch/arm64/include/asm/kvm_host.h >>>>> +++ b/arch/arm64/include/asm/kvm_host.h >>>>> @@ -1593,4 +1593,14 @@ static inline bool kvm_arch_has_irq_bypass(void) >>>>> return true; >>>>> } >>>>> >>>>> +static inline bool kvm_arch_supports_gmem(struct kvm *kvm) >>>>> +{ >>>>> + return IS_ENABLED(CONFIG_KVM_GMEM); >>>>> +} >>>>> + >>>>> +static inline bool kvm_arch_vm_supports_gmem_shared_mem(struct kvm *kvm) >>>>> +{ >>>>> + return IS_ENABLED(CONFIG_KVM_GMEM_SHARED_MEM); >>>>> +} >>>>> + >>>>> #endif /* __ARM64_KVM_HOST_H__ */ >>>>> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig >>>>> index 096e45acadb2..8c1e1964b46a 100644 >>>>> --- a/arch/arm64/kvm/Kconfig >>>>> +++ b/arch/arm64/kvm/Kconfig >>>>> @@ -38,6 +38,7 @@ menuconfig KVM >>>>> select HAVE_KVM_VCPU_RUN_PID_CHANGE >>>>> select SCHED_INFO >>>>> select GUEST_PERF_EVENTS if PERF_EVENTS >>>>> + select KVM_GMEM_SHARED_MEM >>>>> help >>>>> Support hosting virtualized guest machines. >>>>> >>>> >>>> Do we have to reject somewhere if we are given a guest_memfd that was >>>> *not* created using the SHARED flag? Or will existing checks already >>>> reject that? >>> >>> We don't reject, but I don't think we need to. A user can create a >>> guest_memfd that's private in arm64, it would just be useless. >> >> But the arm64 fault routine would not be able to handle that properly, no? > > Actually it would. The function user_mem_abort() doesn't care whether > it's private or shared. It would fault it into the guest correctly > regardless. I think what I meant is that: if it's !shared (private only), shared accesses (IOW all access without CoCo) should be taken from the user space mapping. But user_mem_abort() would blindly go to kvm_gmem_get_pfn() because "is_gmem = kvm_slot_has_gmem(memslot) = true". In other words, arm64 would have to *ignore* guest_memfd that does not support shared? That's why I was wondering whether we should just immediately refuse such guest_memfds.
Hi David, On Wed, 21 May 2025 at 13:44, David Hildenbrand <david@redhat.com> wrote: > > On 21.05.25 12:29, Fuad Tabba wrote: > > On Wed, 21 May 2025 at 11:26, David Hildenbrand <david@redhat.com> wrote: > >> > >> On 21.05.25 12:12, Fuad Tabba wrote: > >>> Hi David, > >>> > >>> On Wed, 21 May 2025 at 09:05, David Hildenbrand <david@redhat.com> wrote: > >>>> > >>>> On 13.05.25 18:34, Fuad Tabba wrote: > >>>>> Enable mapping guest_memfd in arm64. For now, it applies to all > >>>>> VMs in arm64 that use guest_memfd. In the future, new VM types > >>>>> can restrict this via kvm_arch_gmem_supports_shared_mem(). > >>>>> > >>>>> Signed-off-by: Fuad Tabba <tabba@google.com> > >>>>> --- > >>>>> arch/arm64/include/asm/kvm_host.h | 10 ++++++++++ > >>>>> arch/arm64/kvm/Kconfig | 1 + > >>>>> 2 files changed, 11 insertions(+) > >>>>> > >>>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > >>>>> index 08ba91e6fb03..2514779f5131 100644 > >>>>> --- a/arch/arm64/include/asm/kvm_host.h > >>>>> +++ b/arch/arm64/include/asm/kvm_host.h > >>>>> @@ -1593,4 +1593,14 @@ static inline bool kvm_arch_has_irq_bypass(void) > >>>>> return true; > >>>>> } > >>>>> > >>>>> +static inline bool kvm_arch_supports_gmem(struct kvm *kvm) > >>>>> +{ > >>>>> + return IS_ENABLED(CONFIG_KVM_GMEM); > >>>>> +} > >>>>> + > >>>>> +static inline bool kvm_arch_vm_supports_gmem_shared_mem(struct kvm *kvm) > >>>>> +{ > >>>>> + return IS_ENABLED(CONFIG_KVM_GMEM_SHARED_MEM); > >>>>> +} > >>>>> + > >>>>> #endif /* __ARM64_KVM_HOST_H__ */ > >>>>> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig > >>>>> index 096e45acadb2..8c1e1964b46a 100644 > >>>>> --- a/arch/arm64/kvm/Kconfig > >>>>> +++ b/arch/arm64/kvm/Kconfig > >>>>> @@ -38,6 +38,7 @@ menuconfig KVM > >>>>> select HAVE_KVM_VCPU_RUN_PID_CHANGE > >>>>> select SCHED_INFO > >>>>> select GUEST_PERF_EVENTS if PERF_EVENTS > >>>>> + select KVM_GMEM_SHARED_MEM > >>>>> help > >>>>> Support hosting virtualized guest machines. > >>>>> > >>>> > >>>> Do we have to reject somewhere if we are given a guest_memfd that was > >>>> *not* created using the SHARED flag? Or will existing checks already > >>>> reject that? > >>> > >>> We don't reject, but I don't think we need to. A user can create a > >>> guest_memfd that's private in arm64, it would just be useless. > >> > >> But the arm64 fault routine would not be able to handle that properly, no? > > > > Actually it would. The function user_mem_abort() doesn't care whether > > it's private or shared. It would fault it into the guest correctly > > regardless. > > > I think what I meant is that: if it's !shared (private only), shared > accesses (IOW all access without CoCo) should be taken from the user > space mapping. > > But user_mem_abort() would blindly go to kvm_gmem_get_pfn() because > "is_gmem = kvm_slot_has_gmem(memslot) = true". Yes, since it is a gmem-backed slot. > In other words, arm64 would have to *ignore* guest_memfd that does not > support shared? > > That's why I was wondering whether we should just immediately refuse > such guest_memfds. My thinking is that if a user deliberately creates a guest_memfd-backed slot without designating it as being sharable, then either they would find out when they try to map that memory to the host userspace (mapping it would fail), or it could be that they deliberately want to set up a VM with memslots that not mappable at all by the host. Perhaps to add some layer of security (although a very flimsy one, since it's not a confidential guest). I'm happy to a check to prevent this. The question is, how to do it exactly (I assume it would be in kvm_gmem_create())? Would it be arch-specific, i.e., prevent arm64 from creating non-shared guest_memfd backed memslots? Or do it by VM type? Even if we do it by VM-type it would need to be arch-specific, since we allow private guest_memfd slots for the default VM in x86, but we wouldn't for arm64. We could add another function, along the lines of kvm_arch_supports_gmem_only_shared_mem(), but considering that it actually works, and (arguably) would behave as intended, I'm not sure if it's worth the complexity. What do you think? Cheers, /fuad > > -- > Cheers, > > David / dhildenb >
On 21.05.25 15:15, Fuad Tabba wrote: > Hi David, > > On Wed, 21 May 2025 at 13:44, David Hildenbrand <david@redhat.com> wrote: >> >> On 21.05.25 12:29, Fuad Tabba wrote: >>> On Wed, 21 May 2025 at 11:26, David Hildenbrand <david@redhat.com> wrote: >>>> >>>> On 21.05.25 12:12, Fuad Tabba wrote: >>>>> Hi David, >>>>> >>>>> On Wed, 21 May 2025 at 09:05, David Hildenbrand <david@redhat.com> wrote: >>>>>> >>>>>> On 13.05.25 18:34, Fuad Tabba wrote: >>>>>>> Enable mapping guest_memfd in arm64. For now, it applies to all >>>>>>> VMs in arm64 that use guest_memfd. In the future, new VM types >>>>>>> can restrict this via kvm_arch_gmem_supports_shared_mem(). >>>>>>> >>>>>>> Signed-off-by: Fuad Tabba <tabba@google.com> >>>>>>> --- >>>>>>> arch/arm64/include/asm/kvm_host.h | 10 ++++++++++ >>>>>>> arch/arm64/kvm/Kconfig | 1 + >>>>>>> 2 files changed, 11 insertions(+) >>>>>>> >>>>>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >>>>>>> index 08ba91e6fb03..2514779f5131 100644 >>>>>>> --- a/arch/arm64/include/asm/kvm_host.h >>>>>>> +++ b/arch/arm64/include/asm/kvm_host.h >>>>>>> @@ -1593,4 +1593,14 @@ static inline bool kvm_arch_has_irq_bypass(void) >>>>>>> return true; >>>>>>> } >>>>>>> >>>>>>> +static inline bool kvm_arch_supports_gmem(struct kvm *kvm) >>>>>>> +{ >>>>>>> + return IS_ENABLED(CONFIG_KVM_GMEM); >>>>>>> +} >>>>>>> + >>>>>>> +static inline bool kvm_arch_vm_supports_gmem_shared_mem(struct kvm *kvm) >>>>>>> +{ >>>>>>> + return IS_ENABLED(CONFIG_KVM_GMEM_SHARED_MEM); >>>>>>> +} >>>>>>> + >>>>>>> #endif /* __ARM64_KVM_HOST_H__ */ >>>>>>> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig >>>>>>> index 096e45acadb2..8c1e1964b46a 100644 >>>>>>> --- a/arch/arm64/kvm/Kconfig >>>>>>> +++ b/arch/arm64/kvm/Kconfig >>>>>>> @@ -38,6 +38,7 @@ menuconfig KVM >>>>>>> select HAVE_KVM_VCPU_RUN_PID_CHANGE >>>>>>> select SCHED_INFO >>>>>>> select GUEST_PERF_EVENTS if PERF_EVENTS >>>>>>> + select KVM_GMEM_SHARED_MEM >>>>>>> help >>>>>>> Support hosting virtualized guest machines. >>>>>>> >>>>>> >>>>>> Do we have to reject somewhere if we are given a guest_memfd that was >>>>>> *not* created using the SHARED flag? Or will existing checks already >>>>>> reject that? >>>>> >>>>> We don't reject, but I don't think we need to. A user can create a >>>>> guest_memfd that's private in arm64, it would just be useless. >>>> >>>> But the arm64 fault routine would not be able to handle that properly, no? >>> >>> Actually it would. The function user_mem_abort() doesn't care whether >>> it's private or shared. It would fault it into the guest correctly >>> regardless. >> >> >> I think what I meant is that: if it's !shared (private only), shared >> accesses (IOW all access without CoCo) should be taken from the user >> space mapping. >> >> But user_mem_abort() would blindly go to kvm_gmem_get_pfn() because >> "is_gmem = kvm_slot_has_gmem(memslot) = true". > > Yes, since it is a gmem-backed slot. > >> In other words, arm64 would have to *ignore* guest_memfd that does not >> support shared? >> >> That's why I was wondering whether we should just immediately refuse >> such guest_memfds. > > My thinking is that if a user deliberately creates a > guest_memfd-backed slot without designating it as being sharable, then > either they would find out when they try to map that memory to the > host userspace (mapping it would fail), or it could be that they > deliberately want to set up a VM with memslots that not mappable at > all by the host. Hm. But that would meant that we interpret "private" memory as a concept that is not understood by the VM. Because the VM does not know what "private" memory is ... > Perhaps to add some layer of security (although a > very flimsy one, since it's not a confidential guest). Exactly my point. If you don't want to mmap it then ... don't mmap it :) > > I'm happy to a check to prevent this. The question is, how to do it > exactly (I assume it would be in kvm_gmem_create())? Would it be > arch-specific, i.e., prevent arm64 from creating non-shared > guest_memfd backed memslots? Or do it by VM type? Even if we do it by > VM-type it would need to be arch-specific, since we allow private > guest_memfd slots for the default VM in x86, but we wouldn't for > arm64. > > We could add another function, along the lines of > kvm_arch_supports_gmem_only_shared_mem(), but considering that it > actually works, and (arguably) would behave as intended, I'm not sure > if it's worth the complexity. > > What do you think? My thinking was to either block this at slot creation time or at guest_memfd creation time. And we should probably block that for other VM types as well that do not support private memory? I mean, creating guest_memfd for private memory when there is no concept of private memory for the VM is ... weird, no? :)
Hi David, On Wed, 21 May 2025 at 14:22, David Hildenbrand <david@redhat.com> wrote: > > On 21.05.25 15:15, Fuad Tabba wrote: > > Hi David, > > > > On Wed, 21 May 2025 at 13:44, David Hildenbrand <david@redhat.com> wrote: > >> > >> On 21.05.25 12:29, Fuad Tabba wrote: > >>> On Wed, 21 May 2025 at 11:26, David Hildenbrand <david@redhat.com> wrote: > >>>> > >>>> On 21.05.25 12:12, Fuad Tabba wrote: > >>>>> Hi David, > >>>>> > >>>>> On Wed, 21 May 2025 at 09:05, David Hildenbrand <david@redhat.com> wrote: > >>>>>> > >>>>>> On 13.05.25 18:34, Fuad Tabba wrote: > >>>>>>> Enable mapping guest_memfd in arm64. For now, it applies to all > >>>>>>> VMs in arm64 that use guest_memfd. In the future, new VM types > >>>>>>> can restrict this via kvm_arch_gmem_supports_shared_mem(). > >>>>>>> > >>>>>>> Signed-off-by: Fuad Tabba <tabba@google.com> > >>>>>>> --- > >>>>>>> arch/arm64/include/asm/kvm_host.h | 10 ++++++++++ > >>>>>>> arch/arm64/kvm/Kconfig | 1 + > >>>>>>> 2 files changed, 11 insertions(+) > >>>>>>> > >>>>>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > >>>>>>> index 08ba91e6fb03..2514779f5131 100644 > >>>>>>> --- a/arch/arm64/include/asm/kvm_host.h > >>>>>>> +++ b/arch/arm64/include/asm/kvm_host.h > >>>>>>> @@ -1593,4 +1593,14 @@ static inline bool kvm_arch_has_irq_bypass(void) > >>>>>>> return true; > >>>>>>> } > >>>>>>> > >>>>>>> +static inline bool kvm_arch_supports_gmem(struct kvm *kvm) > >>>>>>> +{ > >>>>>>> + return IS_ENABLED(CONFIG_KVM_GMEM); > >>>>>>> +} > >>>>>>> + > >>>>>>> +static inline bool kvm_arch_vm_supports_gmem_shared_mem(struct kvm *kvm) > >>>>>>> +{ > >>>>>>> + return IS_ENABLED(CONFIG_KVM_GMEM_SHARED_MEM); > >>>>>>> +} > >>>>>>> + > >>>>>>> #endif /* __ARM64_KVM_HOST_H__ */ > >>>>>>> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig > >>>>>>> index 096e45acadb2..8c1e1964b46a 100644 > >>>>>>> --- a/arch/arm64/kvm/Kconfig > >>>>>>> +++ b/arch/arm64/kvm/Kconfig > >>>>>>> @@ -38,6 +38,7 @@ menuconfig KVM > >>>>>>> select HAVE_KVM_VCPU_RUN_PID_CHANGE > >>>>>>> select SCHED_INFO > >>>>>>> select GUEST_PERF_EVENTS if PERF_EVENTS > >>>>>>> + select KVM_GMEM_SHARED_MEM > >>>>>>> help > >>>>>>> Support hosting virtualized guest machines. > >>>>>>> > >>>>>> > >>>>>> Do we have to reject somewhere if we are given a guest_memfd that was > >>>>>> *not* created using the SHARED flag? Or will existing checks already > >>>>>> reject that? > >>>>> > >>>>> We don't reject, but I don't think we need to. A user can create a > >>>>> guest_memfd that's private in arm64, it would just be useless. > >>>> > >>>> But the arm64 fault routine would not be able to handle that properly, no? > >>> > >>> Actually it would. The function user_mem_abort() doesn't care whether > >>> it's private or shared. It would fault it into the guest correctly > >>> regardless. > >> > >> > >> I think what I meant is that: if it's !shared (private only), shared > >> accesses (IOW all access without CoCo) should be taken from the user > >> space mapping. > >> > >> But user_mem_abort() would blindly go to kvm_gmem_get_pfn() because > >> "is_gmem = kvm_slot_has_gmem(memslot) = true". > > > > Yes, since it is a gmem-backed slot. > > > >> In other words, arm64 would have to *ignore* guest_memfd that does not > >> support shared? > >> > >> That's why I was wondering whether we should just immediately refuse > >> such guest_memfds. > > > > My thinking is that if a user deliberately creates a > > guest_memfd-backed slot without designating it as being sharable, then > > either they would find out when they try to map that memory to the > > host userspace (mapping it would fail), or it could be that they > > deliberately want to set up a VM with memslots that not mappable at > > all by the host. > > Hm. But that would meant that we interpret "private" memory as a concept > that is not understood by the VM. Because the VM does not know what > "private" memory is ... > > > Perhaps to add some layer of security (although a > > very flimsy one, since it's not a confidential guest). > > Exactly my point. If you don't want to mmap it then ... don't mmap it :) > > > > > I'm happy to a check to prevent this. The question is, how to do it > > exactly (I assume it would be in kvm_gmem_create())? Would it be > > arch-specific, i.e., prevent arm64 from creating non-shared > > guest_memfd backed memslots? Or do it by VM type? Even if we do it by > > VM-type it would need to be arch-specific, since we allow private > > guest_memfd slots for the default VM in x86, but we wouldn't for > > arm64. > > > > We could add another function, along the lines of > > kvm_arch_supports_gmem_only_shared_mem(), but considering that it > > actually works, and (arguably) would behave as intended, I'm not sure > > if it's worth the complexity. > > > > What do you think? > > My thinking was to either block this at slot creation time or at > guest_memfd creation time. And we should probably block that for other > VM types as well that do not support private memory? > > I mean, creating guest_memfd for private memory when there is no concept > of private memory for the VM is ... weird, no? :) Actually, I could add this as an arch-specific check in arch/arm64/kvm/mmu.c:kvm_arch_prepare_memory_region(). That way, core KVM/guest_memfd code doesn't need to handle this arm64-specific behavior. Does that sound good? Thanks, /fuad > -- > Cheers, > > David / dhildenb >
On 21.05.25 15:32, Fuad Tabba wrote: > Hi David, > > On Wed, 21 May 2025 at 14:22, David Hildenbrand <david@redhat.com> wrote: >> >> On 21.05.25 15:15, Fuad Tabba wrote: >>> Hi David, >>> >>> On Wed, 21 May 2025 at 13:44, David Hildenbrand <david@redhat.com> wrote: >>>> >>>> On 21.05.25 12:29, Fuad Tabba wrote: >>>>> On Wed, 21 May 2025 at 11:26, David Hildenbrand <david@redhat.com> wrote: >>>>>> >>>>>> On 21.05.25 12:12, Fuad Tabba wrote: >>>>>>> Hi David, >>>>>>> >>>>>>> On Wed, 21 May 2025 at 09:05, David Hildenbrand <david@redhat.com> wrote: >>>>>>>> >>>>>>>> On 13.05.25 18:34, Fuad Tabba wrote: >>>>>>>>> Enable mapping guest_memfd in arm64. For now, it applies to all >>>>>>>>> VMs in arm64 that use guest_memfd. In the future, new VM types >>>>>>>>> can restrict this via kvm_arch_gmem_supports_shared_mem(). >>>>>>>>> >>>>>>>>> Signed-off-by: Fuad Tabba <tabba@google.com> >>>>>>>>> --- >>>>>>>>> arch/arm64/include/asm/kvm_host.h | 10 ++++++++++ >>>>>>>>> arch/arm64/kvm/Kconfig | 1 + >>>>>>>>> 2 files changed, 11 insertions(+) >>>>>>>>> >>>>>>>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >>>>>>>>> index 08ba91e6fb03..2514779f5131 100644 >>>>>>>>> --- a/arch/arm64/include/asm/kvm_host.h >>>>>>>>> +++ b/arch/arm64/include/asm/kvm_host.h >>>>>>>>> @@ -1593,4 +1593,14 @@ static inline bool kvm_arch_has_irq_bypass(void) >>>>>>>>> return true; >>>>>>>>> } >>>>>>>>> >>>>>>>>> +static inline bool kvm_arch_supports_gmem(struct kvm *kvm) >>>>>>>>> +{ >>>>>>>>> + return IS_ENABLED(CONFIG_KVM_GMEM); >>>>>>>>> +} >>>>>>>>> + >>>>>>>>> +static inline bool kvm_arch_vm_supports_gmem_shared_mem(struct kvm *kvm) >>>>>>>>> +{ >>>>>>>>> + return IS_ENABLED(CONFIG_KVM_GMEM_SHARED_MEM); >>>>>>>>> +} >>>>>>>>> + >>>>>>>>> #endif /* __ARM64_KVM_HOST_H__ */ >>>>>>>>> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig >>>>>>>>> index 096e45acadb2..8c1e1964b46a 100644 >>>>>>>>> --- a/arch/arm64/kvm/Kconfig >>>>>>>>> +++ b/arch/arm64/kvm/Kconfig >>>>>>>>> @@ -38,6 +38,7 @@ menuconfig KVM >>>>>>>>> select HAVE_KVM_VCPU_RUN_PID_CHANGE >>>>>>>>> select SCHED_INFO >>>>>>>>> select GUEST_PERF_EVENTS if PERF_EVENTS >>>>>>>>> + select KVM_GMEM_SHARED_MEM >>>>>>>>> help >>>>>>>>> Support hosting virtualized guest machines. >>>>>>>>> >>>>>>>> >>>>>>>> Do we have to reject somewhere if we are given a guest_memfd that was >>>>>>>> *not* created using the SHARED flag? Or will existing checks already >>>>>>>> reject that? >>>>>>> >>>>>>> We don't reject, but I don't think we need to. A user can create a >>>>>>> guest_memfd that's private in arm64, it would just be useless. >>>>>> >>>>>> But the arm64 fault routine would not be able to handle that properly, no? >>>>> >>>>> Actually it would. The function user_mem_abort() doesn't care whether >>>>> it's private or shared. It would fault it into the guest correctly >>>>> regardless. >>>> >>>> >>>> I think what I meant is that: if it's !shared (private only), shared >>>> accesses (IOW all access without CoCo) should be taken from the user >>>> space mapping. >>>> >>>> But user_mem_abort() would blindly go to kvm_gmem_get_pfn() because >>>> "is_gmem = kvm_slot_has_gmem(memslot) = true". >>> >>> Yes, since it is a gmem-backed slot. >>> >>>> In other words, arm64 would have to *ignore* guest_memfd that does not >>>> support shared? >>>> >>>> That's why I was wondering whether we should just immediately refuse >>>> such guest_memfds. >>> >>> My thinking is that if a user deliberately creates a >>> guest_memfd-backed slot without designating it as being sharable, then >>> either they would find out when they try to map that memory to the >>> host userspace (mapping it would fail), or it could be that they >>> deliberately want to set up a VM with memslots that not mappable at >>> all by the host. >> >> Hm. But that would meant that we interpret "private" memory as a concept >> that is not understood by the VM. Because the VM does not know what >> "private" memory is ... >> >>> Perhaps to add some layer of security (although a >>> very flimsy one, since it's not a confidential guest). >> >> Exactly my point. If you don't want to mmap it then ... don't mmap it :) >> >>> >>> I'm happy to a check to prevent this. The question is, how to do it >>> exactly (I assume it would be in kvm_gmem_create())? Would it be >>> arch-specific, i.e., prevent arm64 from creating non-shared >>> guest_memfd backed memslots? Or do it by VM type? Even if we do it by >>> VM-type it would need to be arch-specific, since we allow private >>> guest_memfd slots for the default VM in x86, but we wouldn't for >>> arm64. >>> >>> We could add another function, along the lines of >>> kvm_arch_supports_gmem_only_shared_mem(), but considering that it >>> actually works, and (arguably) would behave as intended, I'm not sure >>> if it's worth the complexity. >>> >>> What do you think? >> >> My thinking was to either block this at slot creation time or at >> guest_memfd creation time. And we should probably block that for other >> VM types as well that do not support private memory? >> >> I mean, creating guest_memfd for private memory when there is no concept >> of private memory for the VM is ... weird, no? :) > > Actually, I could add this as an arch-specific check in > arch/arm64/kvm/mmu.c:kvm_arch_prepare_memory_region(). That way, core > KVM/guest_memfd code doesn't need to handle this arm64-specific behavior. > > Does that sound good? Yes, but only do so if you agree.
On Wed, 21 May 2025 at 14:45, David Hildenbrand <david@redhat.com> wrote: > > On 21.05.25 15:32, Fuad Tabba wrote: > > Hi David, > > > > On Wed, 21 May 2025 at 14:22, David Hildenbrand <david@redhat.com> wrote: > >> > >> On 21.05.25 15:15, Fuad Tabba wrote: > >>> Hi David, > >>> > >>> On Wed, 21 May 2025 at 13:44, David Hildenbrand <david@redhat.com> wrote: > >>>> > >>>> On 21.05.25 12:29, Fuad Tabba wrote: > >>>>> On Wed, 21 May 2025 at 11:26, David Hildenbrand <david@redhat.com> wrote: > >>>>>> > >>>>>> On 21.05.25 12:12, Fuad Tabba wrote: > >>>>>>> Hi David, > >>>>>>> > >>>>>>> On Wed, 21 May 2025 at 09:05, David Hildenbrand <david@redhat.com> wrote: > >>>>>>>> > >>>>>>>> On 13.05.25 18:34, Fuad Tabba wrote: > >>>>>>>>> Enable mapping guest_memfd in arm64. For now, it applies to all > >>>>>>>>> VMs in arm64 that use guest_memfd. In the future, new VM types > >>>>>>>>> can restrict this via kvm_arch_gmem_supports_shared_mem(). > >>>>>>>>> > >>>>>>>>> Signed-off-by: Fuad Tabba <tabba@google.com> > >>>>>>>>> --- > >>>>>>>>> arch/arm64/include/asm/kvm_host.h | 10 ++++++++++ > >>>>>>>>> arch/arm64/kvm/Kconfig | 1 + > >>>>>>>>> 2 files changed, 11 insertions(+) > >>>>>>>>> > >>>>>>>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > >>>>>>>>> index 08ba91e6fb03..2514779f5131 100644 > >>>>>>>>> --- a/arch/arm64/include/asm/kvm_host.h > >>>>>>>>> +++ b/arch/arm64/include/asm/kvm_host.h > >>>>>>>>> @@ -1593,4 +1593,14 @@ static inline bool kvm_arch_has_irq_bypass(void) > >>>>>>>>> return true; > >>>>>>>>> } > >>>>>>>>> > >>>>>>>>> +static inline bool kvm_arch_supports_gmem(struct kvm *kvm) > >>>>>>>>> +{ > >>>>>>>>> + return IS_ENABLED(CONFIG_KVM_GMEM); > >>>>>>>>> +} > >>>>>>>>> + > >>>>>>>>> +static inline bool kvm_arch_vm_supports_gmem_shared_mem(struct kvm *kvm) > >>>>>>>>> +{ > >>>>>>>>> + return IS_ENABLED(CONFIG_KVM_GMEM_SHARED_MEM); > >>>>>>>>> +} > >>>>>>>>> + > >>>>>>>>> #endif /* __ARM64_KVM_HOST_H__ */ > >>>>>>>>> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig > >>>>>>>>> index 096e45acadb2..8c1e1964b46a 100644 > >>>>>>>>> --- a/arch/arm64/kvm/Kconfig > >>>>>>>>> +++ b/arch/arm64/kvm/Kconfig > >>>>>>>>> @@ -38,6 +38,7 @@ menuconfig KVM > >>>>>>>>> select HAVE_KVM_VCPU_RUN_PID_CHANGE > >>>>>>>>> select SCHED_INFO > >>>>>>>>> select GUEST_PERF_EVENTS if PERF_EVENTS > >>>>>>>>> + select KVM_GMEM_SHARED_MEM > >>>>>>>>> help > >>>>>>>>> Support hosting virtualized guest machines. > >>>>>>>>> > >>>>>>>> > >>>>>>>> Do we have to reject somewhere if we are given a guest_memfd that was > >>>>>>>> *not* created using the SHARED flag? Or will existing checks already > >>>>>>>> reject that? > >>>>>>> > >>>>>>> We don't reject, but I don't think we need to. A user can create a > >>>>>>> guest_memfd that's private in arm64, it would just be useless. > >>>>>> > >>>>>> But the arm64 fault routine would not be able to handle that properly, no? > >>>>> > >>>>> Actually it would. The function user_mem_abort() doesn't care whether > >>>>> it's private or shared. It would fault it into the guest correctly > >>>>> regardless. > >>>> > >>>> > >>>> I think what I meant is that: if it's !shared (private only), shared > >>>> accesses (IOW all access without CoCo) should be taken from the user > >>>> space mapping. > >>>> > >>>> But user_mem_abort() would blindly go to kvm_gmem_get_pfn() because > >>>> "is_gmem = kvm_slot_has_gmem(memslot) = true". > >>> > >>> Yes, since it is a gmem-backed slot. > >>> > >>>> In other words, arm64 would have to *ignore* guest_memfd that does not > >>>> support shared? > >>>> > >>>> That's why I was wondering whether we should just immediately refuse > >>>> such guest_memfds. > >>> > >>> My thinking is that if a user deliberately creates a > >>> guest_memfd-backed slot without designating it as being sharable, then > >>> either they would find out when they try to map that memory to the > >>> host userspace (mapping it would fail), or it could be that they > >>> deliberately want to set up a VM with memslots that not mappable at > >>> all by the host. > >> > >> Hm. But that would meant that we interpret "private" memory as a concept > >> that is not understood by the VM. Because the VM does not know what > >> "private" memory is ... > >> > >>> Perhaps to add some layer of security (although a > >>> very flimsy one, since it's not a confidential guest). > >> > >> Exactly my point. If you don't want to mmap it then ... don't mmap it :) > >> > >>> > >>> I'm happy to a check to prevent this. The question is, how to do it > >>> exactly (I assume it would be in kvm_gmem_create())? Would it be > >>> arch-specific, i.e., prevent arm64 from creating non-shared > >>> guest_memfd backed memslots? Or do it by VM type? Even if we do it by > >>> VM-type it would need to be arch-specific, since we allow private > >>> guest_memfd slots for the default VM in x86, but we wouldn't for > >>> arm64. > >>> > >>> We could add another function, along the lines of > >>> kvm_arch_supports_gmem_only_shared_mem(), but considering that it > >>> actually works, and (arguably) would behave as intended, I'm not sure > >>> if it's worth the complexity. > >>> > >>> What do you think? > >> > >> My thinking was to either block this at slot creation time or at > >> guest_memfd creation time. And we should probably block that for other > >> VM types as well that do not support private memory? > >> > >> I mean, creating guest_memfd for private memory when there is no concept > >> of private memory for the VM is ... weird, no? :) > > > > Actually, I could add this as an arch-specific check in > > arch/arm64/kvm/mmu.c:kvm_arch_prepare_memory_region(). That way, core > > KVM/guest_memfd code doesn't need to handle this arm64-specific behavior. > > > > Does that sound good? > > Yes, but only do so if you agree. Ack :) /fuad > -- > Cheers, > > David / dhildenb >
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 08ba91e6fb03..2514779f5131 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -1593,4 +1593,14 @@ static inline bool kvm_arch_has_irq_bypass(void) return true; } +static inline bool kvm_arch_supports_gmem(struct kvm *kvm) +{ + return IS_ENABLED(CONFIG_KVM_GMEM); +} + +static inline bool kvm_arch_vm_supports_gmem_shared_mem(struct kvm *kvm) +{ + return IS_ENABLED(CONFIG_KVM_GMEM_SHARED_MEM); +} + #endif /* __ARM64_KVM_HOST_H__ */ diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig index 096e45acadb2..8c1e1964b46a 100644 --- a/arch/arm64/kvm/Kconfig +++ b/arch/arm64/kvm/Kconfig @@ -38,6 +38,7 @@ menuconfig KVM select HAVE_KVM_VCPU_RUN_PID_CHANGE select SCHED_INFO select GUEST_PERF_EVENTS if PERF_EVENTS + select KVM_GMEM_SHARED_MEM help Support hosting virtualized guest machines.
Enable mapping guest_memfd in arm64. For now, it applies to all VMs in arm64 that use guest_memfd. In the future, new VM types can restrict this via kvm_arch_gmem_supports_shared_mem(). Signed-off-by: Fuad Tabba <tabba@google.com> --- arch/arm64/include/asm/kvm_host.h | 10 ++++++++++ arch/arm64/kvm/Kconfig | 1 + 2 files changed, 11 insertions(+)