| Message ID | 1410990981-665-4-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
Is DEVICE type the correct default, for device regions that are prefetchable? The guest might want something less restrictive to optimize device access. On 09/17/2014 02:56 PM, Ard Biesheuvel wrote: > Add support for read-only MMIO passthrough mappings by adding a > 'writable' parameter to kvm_phys_addr_ioremap. For the moment, > mappings will be read-write even if 'writable' is false, but once > the definition of PAGE_S2_DEVICE gets changed, those mappings will > be created read-only. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > arch/arm/kvm/mmu.c | 5 ++++- > arch/arm64/include/asm/kvm_mmu.h | 2 +- > virt/kvm/arm/vgic.c | 3 ++- > 3 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c > index c093e95ff7ef..fe53c3a30383 100644 > --- a/arch/arm/kvm/mmu.c > +++ b/arch/arm/kvm/mmu.c > @@ -674,7 +674,7 @@ static int stage2_set_pte(struct kvm *kvm, struct kvm_mmu_memory_cache *cache, > * @size: The size of the mapping > */ > int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa, > - phys_addr_t pa, unsigned long size) > + phys_addr_t pa, unsigned long size, bool writable) > { > phys_addr_t addr, end; > int ret = 0; > @@ -687,6 +687,9 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa, > for (addr = guest_ipa; addr < end; addr += PAGE_SIZE) { > pte_t pte = pfn_pte(pfn, PAGE_S2_DEVICE); > > + if (writable) > + kvm_set_s2pte_writable(&pte); > + > ret = mmu_topup_memory_cache(&cache, 2, 2); > if (ret) > goto out; > diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h > index 737da742b293..7474e611bb2a 100644 > --- a/arch/arm64/include/asm/kvm_mmu.h > +++ b/arch/arm64/include/asm/kvm_mmu.h > @@ -78,7 +78,7 @@ void free_hyp_pgds(void); > int kvm_alloc_stage2_pgd(struct kvm *kvm); > void kvm_free_stage2_pgd(struct kvm *kvm); > int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa, > - phys_addr_t pa, unsigned long size); > + phys_addr_t pa, unsigned long size, bool writable); > > int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run); > > diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c > index 73eba793b17f..c2bdbf4e25c2 100644 > --- a/virt/kvm/arm/vgic.c > +++ b/virt/kvm/arm/vgic.c > @@ -1628,7 +1628,8 @@ int kvm_vgic_init(struct kvm *kvm) > } > > ret = kvm_phys_addr_ioremap(kvm, kvm->arch.vgic.vgic_cpu_base, > - vgic->vcpu_base, KVM_VGIC_V2_CPU_SIZE); > + vgic->vcpu_base, KVM_VGIC_V2_CPU_SIZE, > + true); > if (ret) { > kvm_err("Unable to remap VGIC CPU to VCPU\n"); > goto out; >
On Wed, Sep 24, 2014 at 05:03:18PM -0700, Mario Smarduch wrote: > Is DEVICE type the correct default, for device > regions that are prefetchable? The guest might > want something less restrictive to optimize device > access. > We don't have a use case for this yet, and we aren't supporting generic platform device passthrough yet. So far this patch is solving a real problem, but if we need something more flexible, then patches are welcome :) -Christoffer
On Wed, Sep 17, 2014 at 02:56:18PM -0700, Ard Biesheuvel wrote: > Add support for read-only MMIO passthrough mappings by adding a > 'writable' parameter to kvm_phys_addr_ioremap. For the moment, > mappings will be read-write even if 'writable' is false, but once > the definition of PAGE_S2_DEVICE gets changed, those mappings will > be created read-only. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > arch/arm/kvm/mmu.c | 5 ++++- > arch/arm64/include/asm/kvm_mmu.h | 2 +- > virt/kvm/arm/vgic.c | 3 ++- > 3 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c > index c093e95ff7ef..fe53c3a30383 100644 > --- a/arch/arm/kvm/mmu.c > +++ b/arch/arm/kvm/mmu.c > @@ -674,7 +674,7 @@ static int stage2_set_pte(struct kvm *kvm, struct kvm_mmu_memory_cache *cache, > * @size: The size of the mapping > */ > int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa, > - phys_addr_t pa, unsigned long size) > + phys_addr_t pa, unsigned long size, bool writable) > { > phys_addr_t addr, end; > int ret = 0; > @@ -687,6 +687,9 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa, > for (addr = guest_ipa; addr < end; addr += PAGE_SIZE) { > pte_t pte = pfn_pte(pfn, PAGE_S2_DEVICE); > > + if (writable) > + kvm_set_s2pte_writable(&pte); > + > ret = mmu_topup_memory_cache(&cache, 2, 2); > if (ret) > goto out; > diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h > index 737da742b293..7474e611bb2a 100644 > --- a/arch/arm64/include/asm/kvm_mmu.h > +++ b/arch/arm64/include/asm/kvm_mmu.h > @@ -78,7 +78,7 @@ void free_hyp_pgds(void); > int kvm_alloc_stage2_pgd(struct kvm *kvm); > void kvm_free_stage2_pgd(struct kvm *kvm); > int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa, > - phys_addr_t pa, unsigned long size); > + phys_addr_t pa, unsigned long size, bool writable); > > int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run); > > diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c > index 73eba793b17f..c2bdbf4e25c2 100644 > --- a/virt/kvm/arm/vgic.c > +++ b/virt/kvm/arm/vgic.c > @@ -1628,7 +1628,8 @@ int kvm_vgic_init(struct kvm *kvm) > } > > ret = kvm_phys_addr_ioremap(kvm, kvm->arch.vgic.vgic_cpu_base, > - vgic->vcpu_base, KVM_VGIC_V2_CPU_SIZE); > + vgic->vcpu_base, KVM_VGIC_V2_CPU_SIZE, > + true); > if (ret) { > kvm_err("Unable to remap VGIC CPU to VCPU\n"); > goto out; > -- > 1.8.3.2 > You forgot the 32-bit prototype, but I can fix that up when applying it. Thanks, -Christoffer
On 09/29/2014 05:23 AM, Christoffer Dall wrote: > On Wed, Sep 24, 2014 at 05:03:18PM -0700, Mario Smarduch wrote: >> Is DEVICE type the correct default, for device >> regions that are prefetchable? The guest might >> want something less restrictive to optimize device >> access. >> > We don't have a use case for this yet, and we aren't supporting generic > platform device passthrough yet. > > So far this patch is solving a real problem, but if we need something > more flexible, then patches are welcome :) > > -Christoffer > I understand the fix addresses a real issue, but once it's in, it may go unnoticed for quite a while. I would recommend some warning or comment that guest has no control over device memory attributes. In my opinion setting 2nd stage attributes to cacheble (with exception of GIC range) should work letting the guest driver manage attributes. - Mario
On Mon, Sep 29, 2014 at 11:02:16AM -0700, Mario Smarduch wrote: > On 09/29/2014 05:23 AM, Christoffer Dall wrote: > > On Wed, Sep 24, 2014 at 05:03:18PM -0700, Mario Smarduch wrote: > >> Is DEVICE type the correct default, for device > >> regions that are prefetchable? The guest might > >> want something less restrictive to optimize device > >> access. > >> > > We don't have a use case for this yet, and we aren't supporting generic > > platform device passthrough yet. > > > > So far this patch is solving a real problem, but if we need something > > more flexible, then patches are welcome :) > > > > -Christoffer > > > > I understand the fix addresses a real issue, but once it's in, > it may go unnoticed for quite a while. > > I would recommend some warning or comment that guest has no > control over device memory attributes. In my opinion > setting 2nd stage attributes to cacheble (with exception of > GIC range) should work letting the guest driver manage > attributes. > Your case is only relevant for device-passthrough which we don't even support yet, and we would have to somehow mandate that we only allow the guest cacheable device memory regions for devices where it's safe to do so. As long as we don't have anything measurable or any infrastructure supporting the case you are referring to, it's really not on my radar just yet. Plus, we are fixing current functionality and in no way precluding later patches from optimizing something in the future, so as I said, patches are welcome. Thanks, -Christoffer
On 17/09/14 22:56, Ard Biesheuvel wrote: > Add support for read-only MMIO passthrough mappings by adding a > 'writable' parameter to kvm_phys_addr_ioremap. For the moment, > mappings will be read-write even if 'writable' is false, but once > the definition of PAGE_S2_DEVICE gets changed, those mappings will > be created read-only. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > arch/arm/kvm/mmu.c | 5 ++++- > arch/arm64/include/asm/kvm_mmu.h | 2 +- > virt/kvm/arm/vgic.c | 3 ++- > 3 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c > index c093e95ff7ef..fe53c3a30383 100644 > --- a/arch/arm/kvm/mmu.c > +++ b/arch/arm/kvm/mmu.c > @@ -674,7 +674,7 @@ static int stage2_set_pte(struct kvm *kvm, struct kvm_mmu_memory_cache *cache, > * @size: The size of the mapping > */ > int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa, > - phys_addr_t pa, unsigned long size) > + phys_addr_t pa, unsigned long size, bool writable) > { > phys_addr_t addr, end; > int ret = 0; > @@ -687,6 +687,9 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa, > for (addr = guest_ipa; addr < end; addr += PAGE_SIZE) { > pte_t pte = pfn_pte(pfn, PAGE_S2_DEVICE); > > + if (writable) > + kvm_set_s2pte_writable(&pte); > + > ret = mmu_topup_memory_cache(&cache, 2, 2); > if (ret) > goto out; > diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h > index 737da742b293..7474e611bb2a 100644 > --- a/arch/arm64/include/asm/kvm_mmu.h > +++ b/arch/arm64/include/asm/kvm_mmu.h > @@ -78,7 +78,7 @@ void free_hyp_pgds(void); > int kvm_alloc_stage2_pgd(struct kvm *kvm); > void kvm_free_stage2_pgd(struct kvm *kvm); > int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa, > - phys_addr_t pa, unsigned long size); > + phys_addr_t pa, unsigned long size, bool writable); I'm afraid you missed the equivalent 32bit declaration. > > int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run); > > diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c > index 73eba793b17f..c2bdbf4e25c2 100644 > --- a/virt/kvm/arm/vgic.c > +++ b/virt/kvm/arm/vgic.c > @@ -1628,7 +1628,8 @@ int kvm_vgic_init(struct kvm *kvm) > } > > ret = kvm_phys_addr_ioremap(kvm, kvm->arch.vgic.vgic_cpu_base, > - vgic->vcpu_base, KVM_VGIC_V2_CPU_SIZE); > + vgic->vcpu_base, KVM_VGIC_V2_CPU_SIZE, > + true); > if (ret) { > kvm_err("Unable to remap VGIC CPU to VCPU\n"); > goto out; > Thanks, M.
On 9 October 2014 15:07, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 17/09/14 22:56, Ard Biesheuvel wrote: >> Add support for read-only MMIO passthrough mappings by adding a >> 'writable' parameter to kvm_phys_addr_ioremap. For the moment, >> mappings will be read-write even if 'writable' is false, but once >> the definition of PAGE_S2_DEVICE gets changed, those mappings will >> be created read-only. >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> arch/arm/kvm/mmu.c | 5 ++++- >> arch/arm64/include/asm/kvm_mmu.h | 2 +- >> virt/kvm/arm/vgic.c | 3 ++- >> 3 files changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c >> index c093e95ff7ef..fe53c3a30383 100644 >> --- a/arch/arm/kvm/mmu.c >> +++ b/arch/arm/kvm/mmu.c >> @@ -674,7 +674,7 @@ static int stage2_set_pte(struct kvm *kvm, struct kvm_mmu_memory_cache *cache, >> * @size: The size of the mapping >> */ >> int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa, >> - phys_addr_t pa, unsigned long size) >> + phys_addr_t pa, unsigned long size, bool writable) >> { >> phys_addr_t addr, end; >> int ret = 0; >> @@ -687,6 +687,9 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa, >> for (addr = guest_ipa; addr < end; addr += PAGE_SIZE) { >> pte_t pte = pfn_pte(pfn, PAGE_S2_DEVICE); >> >> + if (writable) >> + kvm_set_s2pte_writable(&pte); >> + >> ret = mmu_topup_memory_cache(&cache, 2, 2); >> if (ret) >> goto out; >> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h >> index 737da742b293..7474e611bb2a 100644 >> --- a/arch/arm64/include/asm/kvm_mmu.h >> +++ b/arch/arm64/include/asm/kvm_mmu.h >> @@ -78,7 +78,7 @@ void free_hyp_pgds(void); >> int kvm_alloc_stage2_pgd(struct kvm *kvm); >> void kvm_free_stage2_pgd(struct kvm *kvm); >> int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa, >> - phys_addr_t pa, unsigned long size); >> + phys_addr_t pa, unsigned long size, bool writable); > > I'm afraid you missed the equivalent 32bit declaration. > Christoffer had spotted that as well, and indicated he would not mind fixing it up when applying. >> >> int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run); >> >> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c >> index 73eba793b17f..c2bdbf4e25c2 100644 >> --- a/virt/kvm/arm/vgic.c >> +++ b/virt/kvm/arm/vgic.c >> @@ -1628,7 +1628,8 @@ int kvm_vgic_init(struct kvm *kvm) >> } >> >> ret = kvm_phys_addr_ioremap(kvm, kvm->arch.vgic.vgic_cpu_base, >> - vgic->vcpu_base, KVM_VGIC_V2_CPU_SIZE); >> + vgic->vcpu_base, KVM_VGIC_V2_CPU_SIZE, >> + true); >> if (ret) { >> kvm_err("Unable to remap VGIC CPU to VCPU\n"); >> goto out; >> > > Thanks, > > M. > -- > Jazz is not dead. It just smells funny... >
On 09/10/14 14:11, Ard Biesheuvel wrote: > On 9 October 2014 15:07, Marc Zyngier <marc.zyngier@arm.com> wrote: >> On 17/09/14 22:56, Ard Biesheuvel wrote: >>> Add support for read-only MMIO passthrough mappings by adding a >>> 'writable' parameter to kvm_phys_addr_ioremap. For the moment, >>> mappings will be read-write even if 'writable' is false, but once >>> the definition of PAGE_S2_DEVICE gets changed, those mappings will >>> be created read-only. >>> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> --- >>> arch/arm/kvm/mmu.c | 5 ++++- >>> arch/arm64/include/asm/kvm_mmu.h | 2 +- >>> virt/kvm/arm/vgic.c | 3 ++- >>> 3 files changed, 7 insertions(+), 3 deletions(-) >>> >>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c >>> index c093e95ff7ef..fe53c3a30383 100644 >>> --- a/arch/arm/kvm/mmu.c >>> +++ b/arch/arm/kvm/mmu.c >>> @@ -674,7 +674,7 @@ static int stage2_set_pte(struct kvm *kvm, struct kvm_mmu_memory_cache *cache, >>> * @size: The size of the mapping >>> */ >>> int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa, >>> - phys_addr_t pa, unsigned long size) >>> + phys_addr_t pa, unsigned long size, bool writable) >>> { >>> phys_addr_t addr, end; >>> int ret = 0; >>> @@ -687,6 +687,9 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa, >>> for (addr = guest_ipa; addr < end; addr += PAGE_SIZE) { >>> pte_t pte = pfn_pte(pfn, PAGE_S2_DEVICE); >>> >>> + if (writable) >>> + kvm_set_s2pte_writable(&pte); >>> + >>> ret = mmu_topup_memory_cache(&cache, 2, 2); >>> if (ret) >>> goto out; >>> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h >>> index 737da742b293..7474e611bb2a 100644 >>> --- a/arch/arm64/include/asm/kvm_mmu.h >>> +++ b/arch/arm64/include/asm/kvm_mmu.h >>> @@ -78,7 +78,7 @@ void free_hyp_pgds(void); >>> int kvm_alloc_stage2_pgd(struct kvm *kvm); >>> void kvm_free_stage2_pgd(struct kvm *kvm); >>> int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa, >>> - phys_addr_t pa, unsigned long size); >>> + phys_addr_t pa, unsigned long size, bool writable); >> >> I'm afraid you missed the equivalent 32bit declaration. >> > > Christoffer had spotted that as well, and indicated he would not mind > fixing it up when applying. Ah, missed that. In which case: Acked-by: Marc Zyngier <marc.zyngier@arm.com> M.
diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c index c093e95ff7ef..fe53c3a30383 100644 --- a/arch/arm/kvm/mmu.c +++ b/arch/arm/kvm/mmu.c @@ -674,7 +674,7 @@ static int stage2_set_pte(struct kvm *kvm, struct kvm_mmu_memory_cache *cache, * @size: The size of the mapping */ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa, - phys_addr_t pa, unsigned long size) + phys_addr_t pa, unsigned long size, bool writable) { phys_addr_t addr, end; int ret = 0; @@ -687,6 +687,9 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa, for (addr = guest_ipa; addr < end; addr += PAGE_SIZE) { pte_t pte = pfn_pte(pfn, PAGE_S2_DEVICE); + if (writable) + kvm_set_s2pte_writable(&pte); + ret = mmu_topup_memory_cache(&cache, 2, 2); if (ret) goto out; diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h index 737da742b293..7474e611bb2a 100644 --- a/arch/arm64/include/asm/kvm_mmu.h +++ b/arch/arm64/include/asm/kvm_mmu.h @@ -78,7 +78,7 @@ void free_hyp_pgds(void); int kvm_alloc_stage2_pgd(struct kvm *kvm); void kvm_free_stage2_pgd(struct kvm *kvm); int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa, - phys_addr_t pa, unsigned long size); + phys_addr_t pa, unsigned long size, bool writable); int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run); diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index 73eba793b17f..c2bdbf4e25c2 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -1628,7 +1628,8 @@ int kvm_vgic_init(struct kvm *kvm) } ret = kvm_phys_addr_ioremap(kvm, kvm->arch.vgic.vgic_cpu_base, - vgic->vcpu_base, KVM_VGIC_V2_CPU_SIZE); + vgic->vcpu_base, KVM_VGIC_V2_CPU_SIZE, + true); if (ret) { kvm_err("Unable to remap VGIC CPU to VCPU\n"); goto out;
Add support for read-only MMIO passthrough mappings by adding a 'writable' parameter to kvm_phys_addr_ioremap. For the moment, mappings will be read-write even if 'writable' is false, but once the definition of PAGE_S2_DEVICE gets changed, those mappings will be created read-only. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- arch/arm/kvm/mmu.c | 5 ++++- arch/arm64/include/asm/kvm_mmu.h | 2 +- virt/kvm/arm/vgic.c | 3 ++- 3 files changed, 7 insertions(+), 3 deletions(-)