On Wed, May 07, 2014 at 03:55:57PM +0100, Christoffer Dall wrote:
> On Wed, May 07, 2014 at 10:00:21AM +0100, Marc Zyngier wrote:
> > On Tue, May 06 2014 at 7:04:48 pm BST, Christoffer Dall <[email protected]> wrote:
> > > On Tue, Mar 25, 2014 at 05:08:14PM -0500, Kim Phillips wrote:
> > >> Use the correct memory type for device MMIO mappings: PAGE_S2_DEVICE.
> > >>
> > >> Signed-off-by: Kim Phillips <[email protected]>
> > >> ---
> > >> arch/arm/kvm/mmu.c | 11 ++++++++---
> > >> 1 file changed, 8 insertions(+), 3 deletions(-)
[...]
> > > I think this looks reasonable.
> > >
> > > Acked-by: Christoffer Dall <[email protected]>
> >
> > I feel like I'm missing some context here, and the commit message is way
> > too terse for me to make sense of it.
> >
> > So far, we can only get into user_mem_abort on a Stage-2 fault
> > (translation or permission) for memory. How can we suddenly get here for
> > a *device* fault? Do we get a special kind of memslot?
> >
> > I'm not saying the patch does anything wrong, but I'd like to understand
> > the rationale behind it. On its own, it doesn't make much sense.
> >
> Think device passthrough. There's nothing preventing user space from
> setting up a memory region to point to device memory (through VFIO or
> /dev/mem). If that's done, we should enforce device memory properties
> so writes don't linger around in the cache to be written some time later
> when that device memory potentially doesn't belong to the VM anymore.
>
> This is just one tiny piece of all of them to make device passthrough
> work, and we could hold off with this patch until we have something more
> complete. On the other hand, we need to start somewhere, and this is
> hardly intrusive and is functionally correct even though you don't have
> a full device passthrough setup.
Please can you queue this patch up? I need it for my VFIO work, where I'm
registering the PCI BARs using KVM_SET_USER_MEMORY_REGION.
Without this, I'd have to trap all accesses and do pread/pwrite from
kvmtool instead of mmaping the regions straight through.
Cheers,
Will
On 24/06/14 11:23, Will Deacon wrote:
> On Wed, May 07, 2014 at 03:55:57PM +0100, Christoffer Dall wrote:
>> On Wed, May 07, 2014 at 10:00:21AM +0100, Marc Zyngier wrote:
>>> On Tue, May 06 2014 at 7:04:48 pm BST, Christoffer Dall <[email protected]> wrote:
>>>> On Tue, Mar 25, 2014 at 05:08:14PM -0500, Kim Phillips wrote:
>>>>> Use the correct memory type for device MMIO mappings: PAGE_S2_DEVICE.
>>>>>
>>>>> Signed-off-by: Kim Phillips <[email protected]>
>>>>> ---
>>>>> arch/arm/kvm/mmu.c | 11 ++++++++---
>>>>> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> [...]
>
>>>> I think this looks reasonable.
>>>>
>>>> Acked-by: Christoffer Dall <[email protected]>
>>>
>>> I feel like I'm missing some context here, and the commit message is way
>>> too terse for me to make sense of it.
>>>
>>> So far, we can only get into user_mem_abort on a Stage-2 fault
>>> (translation or permission) for memory. How can we suddenly get here for
>>> a *device* fault? Do we get a special kind of memslot?
>>>
>>> I'm not saying the patch does anything wrong, but I'd like to understand
>>> the rationale behind it. On its own, it doesn't make much sense.
>>>
>> Think device passthrough. There's nothing preventing user space from
>> setting up a memory region to point to device memory (through VFIO or
>> /dev/mem). If that's done, we should enforce device memory properties
>> so writes don't linger around in the cache to be written some time later
>> when that device memory potentially doesn't belong to the VM anymore.
>>
>> This is just one tiny piece of all of them to make device passthrough
>> work, and we could hold off with this patch until we have something more
>> complete. On the other hand, we need to start somewhere, and this is
>> hardly intrusive and is functionally correct even though you don't have
>> a full device passthrough setup.
>
> Please can you queue this patch up? I need it for my VFIO work, where I'm
> registering the PCI BARs using KVM_SET_USER_MEMORY_REGION.
>
> Without this, I'd have to trap all accesses and do pread/pwrite from
> kvmtool instead of mmaping the regions straight through.
I'm afraid there as been quite a bit of churn in this department, and
the patch doesn't apply any more.
Kim, any chance you could respin this patch on top of mainline?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
From: Kim Phillips <[email protected]>
A userspace process can map device MMIO memory via VFIO or /dev/mem,
e.g., for platform device passthrough support in QEMU.
During early development, we found the PAGE_S2 memory type being used
for MMIO mappings. This patch corrects that by using the more strongly
ordered memory type for device MMIO mappings: PAGE_S2_DEVICE.
Signed-off-by: Kim Phillips <[email protected]>
Acked-by: Christoffer Dall <[email protected]>
---
Hi, here's a v2, upon request:
- rebased onto today's mainline ToT
- mmu.o-build tested only (ToT build doesn't complete)
- made commit text less terse
- added Christoffer's ack
Cheers,
Kim
arch/arm/kvm/mmu.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 16f8049..69af021 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -748,6 +748,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
struct kvm_mmu_memory_cache *memcache = &vcpu->arch.mmu_page_cache;
struct vm_area_struct *vma;
pfn_t pfn;
+ pgprot_t mem_type = PAGE_S2;
write_fault = kvm_is_write_fault(kvm_vcpu_get_hsr(vcpu));
if (fault_status == FSC_PERM && !write_fault) {
@@ -798,6 +799,9 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
if (is_error_pfn(pfn))
return -EFAULT;
+ if (kvm_is_mmio_pfn(pfn))
+ mem_type = PAGE_S2_DEVICE;
+
spin_lock(&kvm->mmu_lock);
if (mmu_notifier_retry(kvm, mmu_seq))
goto out_unlock;
@@ -805,7 +809,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
hugetlb = transparent_hugepage_adjust(&pfn, &fault_ipa);
if (hugetlb) {
- pmd_t new_pmd = pfn_pmd(pfn, PAGE_S2);
+ pmd_t new_pmd = pfn_pmd(pfn, mem_type);
new_pmd = pmd_mkhuge(new_pmd);
if (writable) {
kvm_set_s2pmd_writable(&new_pmd);
@@ -814,13 +818,14 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
coherent_cache_guest_page(vcpu, hva & PMD_MASK, PMD_SIZE);
ret = stage2_set_pmd_huge(kvm, memcache, fault_ipa, &new_pmd);
} else {
- pte_t new_pte = pfn_pte(pfn, PAGE_S2);
+ pte_t new_pte = pfn_pte(pfn, mem_type);
if (writable) {
kvm_set_s2pte_writable(&new_pte);
kvm_set_pfn_dirty(pfn);
}
coherent_cache_guest_page(vcpu, hva, PAGE_SIZE);
- ret = stage2_set_pte(kvm, memcache, fault_ipa, &new_pte, false);
+ ret = stage2_set_pte(kvm, memcache, fault_ipa, &new_pte,
+ mem_type == PAGE_S2_DEVICE);
}
--
2.0.0
On Thu, Jun 26, 2014 at 01:45:51AM +0100, Kim Phillips wrote:
> From: Kim Phillips <[email protected]>
>
> A userspace process can map device MMIO memory via VFIO or /dev/mem,
> e.g., for platform device passthrough support in QEMU.
>
> During early development, we found the PAGE_S2 memory type being used
> for MMIO mappings. This patch corrects that by using the more strongly
> ordered memory type for device MMIO mappings: PAGE_S2_DEVICE.
>
> Signed-off-by: Kim Phillips <[email protected]>
> Acked-by: Christoffer Dall <[email protected]>
> ---
> Hi, here's a v2, upon request:
>
> - rebased onto today's mainline ToT
> - mmu.o-build tested only (ToT build doesn't complete)
> - made commit text less terse
> - added Christoffer's ack
Thanks for reposting this so quickly!
Acked-by: Will Deacon <[email protected]>
Will
On Thu, Jun 26, 2014 at 09:46:26AM +0100, Will Deacon wrote:
> On Thu, Jun 26, 2014 at 01:45:51AM +0100, Kim Phillips wrote:
> > From: Kim Phillips <[email protected]>
> >
> > A userspace process can map device MMIO memory via VFIO or /dev/mem,
> > e.g., for platform device passthrough support in QEMU.
> >
> > During early development, we found the PAGE_S2 memory type being used
> > for MMIO mappings. This patch corrects that by using the more strongly
> > ordered memory type for device MMIO mappings: PAGE_S2_DEVICE.
> >
> > Signed-off-by: Kim Phillips <[email protected]>
> > Acked-by: Christoffer Dall <[email protected]>
> > ---
> > Hi, here's a v2, upon request:
> >
> > - rebased onto today's mainline ToT
> > - mmu.o-build tested only (ToT build doesn't complete)
> > - made commit text less terse
> > - added Christoffer's ack
>
> Thanks for reposting this so quickly!
>
> Acked-by: Will Deacon <[email protected]>
>
Thanks,
Marc, will you apply this one to kvmarm/queue [and kvmarm/next]?
-Christoffer
On 30/06/14 10:08, Christoffer Dall wrote:
> On Thu, Jun 26, 2014 at 09:46:26AM +0100, Will Deacon wrote:
>> On Thu, Jun 26, 2014 at 01:45:51AM +0100, Kim Phillips wrote:
>>> From: Kim Phillips <[email protected]>
>>>
>>> A userspace process can map device MMIO memory via VFIO or /dev/mem,
>>> e.g., for platform device passthrough support in QEMU.
>>>
>>> During early development, we found the PAGE_S2 memory type being used
>>> for MMIO mappings. This patch corrects that by using the more strongly
>>> ordered memory type for device MMIO mappings: PAGE_S2_DEVICE.
>>>
>>> Signed-off-by: Kim Phillips <[email protected]>
>>> Acked-by: Christoffer Dall <[email protected]>
>>> ---
>>> Hi, here's a v2, upon request:
>>>
>>> - rebased onto today's mainline ToT
>>> - mmu.o-build tested only (ToT build doesn't complete)
>>> - made commit text less terse
>>> - added Christoffer's ack
>>
>> Thanks for reposting this so quickly!
>>
>> Acked-by: Will Deacon <[email protected]>
>>
> Thanks,
>
> Marc, will you apply this one to kvmarm/queue [and kvmarm/next]?
Yup, adding it.
M.
--
Jazz is not dead. It just smells funny...