diff mbox series

[2/2] KVM: arm64: Don't map 'kvm_vgic_global_state' at EL2 with pKVM

Message ID 20241022144016.27350-3-will@kernel.org (mailing list archive)
State New, archived
Headers show
Series KVM: arm64: Simplify handling of GICv3 hardware with broken SEIS | expand

Commit Message

Will Deacon Oct. 22, 2024, 2:40 p.m. UTC
Now that 'kvm_vgic_global_state' is no longer needed for ICC_CTLR_EL1
emulation on machines with a broken SEIS implementation, drop the
pKVM hypervisor mapping of the page.

Cc: Marc Zyngier <maz@kernel.org>
Cc: Oliver Upton <oliver.upton@linux.dev>
Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/kvm/hyp/nvhe/setup.c | 17 -----------------
 1 file changed, 17 deletions(-)

Comments

Marc Zyngier Oct. 22, 2024, 5:01 p.m. UTC | #1
On Tue, 22 Oct 2024 15:40:16 +0100,
Will Deacon <will@kernel.org> wrote:
> 
> Now that 'kvm_vgic_global_state' is no longer needed for ICC_CTLR_EL1
> emulation on machines with a broken SEIS implementation, drop the
> pKVM hypervisor mapping of the page.
> 
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Oliver Upton <oliver.upton@linux.dev>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/kvm/hyp/nvhe/setup.c | 17 -----------------
>  1 file changed, 17 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/nvhe/setup.c b/arch/arm64/kvm/hyp/nvhe/setup.c
> index 174007f3fadd..8fec099c2775 100644
> --- a/arch/arm64/kvm/hyp/nvhe/setup.c
> +++ b/arch/arm64/kvm/hyp/nvhe/setup.c
> @@ -95,7 +95,6 @@ static int recreate_hyp_mappings(phys_addr_t phys, unsigned long size,
>  {
>  	void *start, *end, *virt = hyp_phys_to_virt(phys);
>  	unsigned long pgt_size = hyp_s1_pgtable_pages() << PAGE_SHIFT;
> -	enum kvm_pgtable_prot prot;
>  	int ret, i;
>  
>  	/* Recreate the hyp page-table using the early page allocator */
> @@ -148,22 +147,6 @@ static int recreate_hyp_mappings(phys_addr_t phys, unsigned long size,
>  	}
>  
>  	pkvm_create_host_sve_mappings();
> -
> -	/*
> -	 * Map the host sections RO in the hypervisor, but transfer the
> -	 * ownership from the host to the hypervisor itself to make sure they
> -	 * can't be donated or shared with another entity.
> -	 *
> -	 * The ownership transition requires matching changes in the host
> -	 * stage-2. This will be done later (see finalize_host_mappings()) once
> -	 * the hyp_vmemmap is addressable.
> -	 */
> -	prot = pkvm_mkstate(PAGE_HYP_RO, PKVM_PAGE_SHARED_OWNED);
> -	ret = pkvm_create_mappings(&kvm_vgic_global_state,
> -				   &kvm_vgic_global_state + 1, prot);
> -	if (ret)
> -		return ret;
> -
>  	return 0;
>  }

Maybe add a note indicating that nVHE/hVHE still have that particular
mapping via the rodata section?

Reviewed-by: Marc Zyngier <maz@kernel.org>

	M.
Will Deacon Oct. 23, 2024, 4:39 p.m. UTC | #2
On Tue, Oct 22, 2024 at 06:01:17PM +0100, Marc Zyngier wrote:
> On Tue, 22 Oct 2024 15:40:16 +0100,
> Will Deacon <will@kernel.org> wrote:
> > 
> > Now that 'kvm_vgic_global_state' is no longer needed for ICC_CTLR_EL1
> > emulation on machines with a broken SEIS implementation, drop the
> > pKVM hypervisor mapping of the page.
> > 
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Oliver Upton <oliver.upton@linux.dev>
> > Signed-off-by: Will Deacon <will@kernel.org>
> > ---
> >  arch/arm64/kvm/hyp/nvhe/setup.c | 17 -----------------
> >  1 file changed, 17 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/hyp/nvhe/setup.c b/arch/arm64/kvm/hyp/nvhe/setup.c
> > index 174007f3fadd..8fec099c2775 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/setup.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/setup.c
> > @@ -95,7 +95,6 @@ static int recreate_hyp_mappings(phys_addr_t phys, unsigned long size,
> >  {
> >  	void *start, *end, *virt = hyp_phys_to_virt(phys);
> >  	unsigned long pgt_size = hyp_s1_pgtable_pages() << PAGE_SHIFT;
> > -	enum kvm_pgtable_prot prot;
> >  	int ret, i;
> >  
> >  	/* Recreate the hyp page-table using the early page allocator */
> > @@ -148,22 +147,6 @@ static int recreate_hyp_mappings(phys_addr_t phys, unsigned long size,
> >  	}
> >  
> >  	pkvm_create_host_sve_mappings();
> > -
> > -	/*
> > -	 * Map the host sections RO in the hypervisor, but transfer the
> > -	 * ownership from the host to the hypervisor itself to make sure they
> > -	 * can't be donated or shared with another entity.
> > -	 *
> > -	 * The ownership transition requires matching changes in the host
> > -	 * stage-2. This will be done later (see finalize_host_mappings()) once
> > -	 * the hyp_vmemmap is addressable.
> > -	 */
> > -	prot = pkvm_mkstate(PAGE_HYP_RO, PKVM_PAGE_SHARED_OWNED);
> > -	ret = pkvm_create_mappings(&kvm_vgic_global_state,
> > -				   &kvm_vgic_global_state + 1, prot);
> > -	if (ret)
> > -		return ret;
> > -
> >  	return 0;
> >  }
> 
> Maybe add a note indicating that nVHE/hVHE still have that particular
> mapping via the rodata section?

I can add something to the commit message, is that what you had in mind?

> Reviewed-by: Marc Zyngier <maz@kernel.org>

Thanks!

Will
Marc Zyngier Oct. 23, 2024, 4:53 p.m. UTC | #3
On Wed, 23 Oct 2024 17:39:08 +0100,
Will Deacon <will@kernel.org> wrote:
> 
> On Tue, Oct 22, 2024 at 06:01:17PM +0100, Marc Zyngier wrote:
> > On Tue, 22 Oct 2024 15:40:16 +0100,
> > Will Deacon <will@kernel.org> wrote:
> > > 
> > > Now that 'kvm_vgic_global_state' is no longer needed for ICC_CTLR_EL1
> > > emulation on machines with a broken SEIS implementation, drop the
> > > pKVM hypervisor mapping of the page.
> > > 
> > > Cc: Marc Zyngier <maz@kernel.org>
> > > Cc: Oliver Upton <oliver.upton@linux.dev>
> > > Signed-off-by: Will Deacon <will@kernel.org>
> > > ---
> > >  arch/arm64/kvm/hyp/nvhe/setup.c | 17 -----------------
> > >  1 file changed, 17 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kvm/hyp/nvhe/setup.c b/arch/arm64/kvm/hyp/nvhe/setup.c
> > > index 174007f3fadd..8fec099c2775 100644
> > > --- a/arch/arm64/kvm/hyp/nvhe/setup.c
> > > +++ b/arch/arm64/kvm/hyp/nvhe/setup.c
> > > @@ -95,7 +95,6 @@ static int recreate_hyp_mappings(phys_addr_t phys, unsigned long size,
> > >  {
> > >  	void *start, *end, *virt = hyp_phys_to_virt(phys);
> > >  	unsigned long pgt_size = hyp_s1_pgtable_pages() << PAGE_SHIFT;
> > > -	enum kvm_pgtable_prot prot;
> > >  	int ret, i;
> > >  
> > >  	/* Recreate the hyp page-table using the early page allocator */
> > > @@ -148,22 +147,6 @@ static int recreate_hyp_mappings(phys_addr_t phys, unsigned long size,
> > >  	}
> > >  
> > >  	pkvm_create_host_sve_mappings();
> > > -
> > > -	/*
> > > -	 * Map the host sections RO in the hypervisor, but transfer the
> > > -	 * ownership from the host to the hypervisor itself to make sure they
> > > -	 * can't be donated or shared with another entity.
> > > -	 *
> > > -	 * The ownership transition requires matching changes in the host
> > > -	 * stage-2. This will be done later (see finalize_host_mappings()) once
> > > -	 * the hyp_vmemmap is addressable.
> > > -	 */
> > > -	prot = pkvm_mkstate(PAGE_HYP_RO, PKVM_PAGE_SHARED_OWNED);
> > > -	ret = pkvm_create_mappings(&kvm_vgic_global_state,
> > > -				   &kvm_vgic_global_state + 1, prot);
> > > -	if (ret)
> > > -		return ret;
> > > -
> > >  	return 0;
> > >  }
> > 
> > Maybe add a note indicating that nVHE/hVHE still have that particular
> > mapping via the rodata section?
> 
> I can add something to the commit message, is that what you had in mind?

Yes. But maybe Oliver can add that when applying the series?

Thanks,

	M.
diff mbox series

Patch

diff --git a/arch/arm64/kvm/hyp/nvhe/setup.c b/arch/arm64/kvm/hyp/nvhe/setup.c
index 174007f3fadd..8fec099c2775 100644
--- a/arch/arm64/kvm/hyp/nvhe/setup.c
+++ b/arch/arm64/kvm/hyp/nvhe/setup.c
@@ -95,7 +95,6 @@  static int recreate_hyp_mappings(phys_addr_t phys, unsigned long size,
 {
 	void *start, *end, *virt = hyp_phys_to_virt(phys);
 	unsigned long pgt_size = hyp_s1_pgtable_pages() << PAGE_SHIFT;
-	enum kvm_pgtable_prot prot;
 	int ret, i;
 
 	/* Recreate the hyp page-table using the early page allocator */
@@ -148,22 +147,6 @@  static int recreate_hyp_mappings(phys_addr_t phys, unsigned long size,
 	}
 
 	pkvm_create_host_sve_mappings();
-
-	/*
-	 * Map the host sections RO in the hypervisor, but transfer the
-	 * ownership from the host to the hypervisor itself to make sure they
-	 * can't be donated or shared with another entity.
-	 *
-	 * The ownership transition requires matching changes in the host
-	 * stage-2. This will be done later (see finalize_host_mappings()) once
-	 * the hyp_vmemmap is addressable.
-	 */
-	prot = pkvm_mkstate(PAGE_HYP_RO, PKVM_PAGE_SHARED_OWNED);
-	ret = pkvm_create_mappings(&kvm_vgic_global_state,
-				   &kvm_vgic_global_state + 1, prot);
-	if (ret)
-		return ret;
-
 	return 0;
 }