| 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 >
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.
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(+)