diff mbox series

[v7,4/4] vfio: convey kvm that the vfio-pci device is wc safe

Message ID 20240211174705.31992-5-ankita@nvidia.com
State New
Headers
Series kvm: arm64: allow the VM to select DEVICE_* and NORMAL_NC for IO memory |

Commit Message

Ankit Agrawal Feb. 11, 2024, 5:47 p.m. UTC 
  From: Ankit Agrawal <ankita@nvidia.com>

The code to map the MMIO in S2 as NormalNC is enabled when conveyed
that the device is WC safe using a new flag VM_ALLOW_ANY_UNCACHED.

Make vfio-pci set the VM_ALLOW_ANY_UNCACHED flag.

This could be extended to other devices in the future once that
is deemed safe.

Suggested-by: Catalin Marinas <catalin.marinas@arm.com>
Acked-by: Jason Gunthorpe <jgg@nvidia.com>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Ankit Agrawal <ankita@nvidia.com>
---
 drivers/vfio/pci/vfio_pci_core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

David Hildenbrand Feb. 12, 2024, 1:16 p.m. UTC | #1 
On 11.02.24 18:47, ankita@nvidia.com wrote:
> From: Ankit Agrawal <ankita@nvidia.com>
> 
> The code to map the MMIO in S2 as NormalNC is enabled when conveyed
> that the device is WC safe using a new flag VM_ALLOW_ANY_UNCACHED.
> 
> Make vfio-pci set the VM_ALLOW_ANY_UNCACHED flag.
> 
> This could be extended to other devices in the future once that
> is deemed safe.

Maybe add some more details how one could make a decision whether it 
would be safe (either here or in patch #2).

> 
> Suggested-by: Catalin Marinas <catalin.marinas@arm.com>
> Acked-by: Jason Gunthorpe <jgg@nvidia.com>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Ankit Agrawal <ankita@nvidia.com>
> ---
>   drivers/vfio/pci/vfio_pci_core.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 1cbc990d42e0..eba2146202f9 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -1862,8 +1862,12 @@ int vfio_pci_core_mmap(struct vfio_device *core_vdev, struct vm_area_struct *vma
>   	/*
>   	 * See remap_pfn_range(), called from vfio_pci_fault() but we can't
>   	 * change vm_flags within the fault handler.  Set them now.
> +	 *
> +	 * Set an additional flag VM_ALLOW_ANY_UNCACHED to convey kvm that
> +	 * the device is wc safe.
>   	 */
> -	vm_flags_set(vma, VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP);
> +	vm_flags_set(vma, VM_ALLOW_ANY_UNCACHED | VM_IO | VM_PFNMAP |
> +			VM_DONTEXPAND | VM_DONTDUMP);
>   	vma->vm_ops = &vfio_pci_mmap_ops;
>   
>   	return 0;

Reviewed-by: David Hildenbrand <david@redhat.com>
Alex Williamson Feb. 12, 2024, 5:05 p.m. UTC | #2 
On Sun, 11 Feb 2024 23:17:05 +0530
<ankita@nvidia.com> wrote:

> From: Ankit Agrawal <ankita@nvidia.com>
> 
> The code to map the MMIO in S2 as NormalNC is enabled when conveyed
> that the device is WC safe using a new flag VM_ALLOW_ANY_UNCACHED.
> 
> Make vfio-pci set the VM_ALLOW_ANY_UNCACHED flag.
> 
> This could be extended to other devices in the future once that
> is deemed safe.
> 
> Suggested-by: Catalin Marinas <catalin.marinas@arm.com>
> Acked-by: Jason Gunthorpe <jgg@nvidia.com>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Ankit Agrawal <ankita@nvidia.com>
> ---
>  drivers/vfio/pci/vfio_pci_core.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 1cbc990d42e0..eba2146202f9 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -1862,8 +1862,12 @@ int vfio_pci_core_mmap(struct vfio_device *core_vdev, struct vm_area_struct *vma
>  	/*
>  	 * See remap_pfn_range(), called from vfio_pci_fault() but we can't
>  	 * change vm_flags within the fault handler.  Set them now.
> +	 *
> +	 * Set an additional flag VM_ALLOW_ANY_UNCACHED to convey kvm that
> +	 * the device is wc safe.
>  	 */

That's a pretty superficial comment.  Check that this is accurate, but
maybe something like:

	The VM_ALLOW_ANY_UNCACHED flag is implemented for ARM64,
	allowing stage 2 device mapping attributes to use Normal-NC
	rather than DEVICE_nGnRE, which allows guest mappings
	supporting combining attributes (WC).  This attribute has
	potential risks with the GICv2 VCPU interface, but is expected
	to be safe for vfio-pci use cases.

And specifically, I think these other devices that may be problematic
as described in the cover letter is a warning against use for
vfio-platform, is that correct?

Thanks,
Alex

> -	vm_flags_set(vma, VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP);
> +	vm_flags_set(vma, VM_ALLOW_ANY_UNCACHED | VM_IO | VM_PFNMAP |
> +			VM_DONTEXPAND | VM_DONTDUMP);
>  	vma->vm_ops = &vfio_pci_mmap_ops;
>  
>  	return 0;
Jason Gunthorpe Feb. 12, 2024, 5:20 p.m. UTC | #3 
On Mon, Feb 12, 2024 at 10:05:02AM -0700, Alex Williamson wrote:

> > --- a/drivers/vfio/pci/vfio_pci_core.c
> > +++ b/drivers/vfio/pci/vfio_pci_core.c
> > @@ -1862,8 +1862,12 @@ int vfio_pci_core_mmap(struct vfio_device *core_vdev, struct vm_area_struct *vma
> >  	/*
> >  	 * See remap_pfn_range(), called from vfio_pci_fault() but we can't
> >  	 * change vm_flags within the fault handler.  Set them now.
> > +	 *
> > +	 * Set an additional flag VM_ALLOW_ANY_UNCACHED to convey kvm that
> > +	 * the device is wc safe.
> >  	 */
> 
> That's a pretty superficial comment.  Check that this is accurate, but
> maybe something like:
> 
> 	The VM_ALLOW_ANY_UNCACHED flag is implemented for ARM64,
> 	allowing stage 2 device mapping attributes to use Normal-NC
               ^^^^ 

> 	rather than DEVICE_nGnRE, which allows guest mappings
> 	supporting combining attributes (WC).  This attribute has
> 	potential risks with the GICv2 VCPU interface, but is expected
> 	to be safe for vfio-pci use cases.

Sure, if you want to elaborate more

  The VM_ALLOW_ANY_UNCACHED flag is implemented for ARM64,
  allowing KVM stage 2 device mapping attributes to use Normal-NC
  rather than DEVICE_nGnRE, which allows guest mappings
  supporting combining attributes (WC). ARM does not architecturally
  guarentee this is safe, and indeed some MMIO regions like the GICv2
  VCPU interface can trigger uncontained faults if Normal-NC is used.

  Even worse we expect there are platforms where even DEVICE_nGnRE can
  allow uncontained faults in conercases. Unfortunately existing ARM
  IP requires platform integration to take responsibility to prevent
  this.

  To safely use VFIO in KVM the platform must guarantee full safety
  in the guest where no action taken against a MMIO mapping can
  trigger an uncontainer failure. We belive that most VFIO PCI
  platforms support this for both mapping types, at least in common
  flows, based on some expectations of how PCI IP is integrated. This
  can be enabled more broadly, for instance into vfio-platform
  drivers, but only after the platform vendor completes auditing for
  safety.
 
> And specifically, I think these other devices that may be problematic
> as described in the cover letter is a warning against use for
> vfio-platform, is that correct?

Maybe more like "we have a general consensus that vfio-pci is likely
safe due to how PCI IP is typically integrated, but it is much less
obvious for other VFIO bus types. As there is no known WC user for
vfio-platform drivers be conservative and do not enable it."

Jason
Alex Williamson Feb. 12, 2024, 5:27 p.m. UTC | #4 
On Mon, 12 Feb 2024 13:20:01 -0400
Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Mon, Feb 12, 2024 at 10:05:02AM -0700, Alex Williamson wrote:
> 
> > > --- a/drivers/vfio/pci/vfio_pci_core.c
> > > +++ b/drivers/vfio/pci/vfio_pci_core.c
> > > @@ -1862,8 +1862,12 @@ int vfio_pci_core_mmap(struct vfio_device *core_vdev, struct vm_area_struct *vma
> > >  	/*
> > >  	 * See remap_pfn_range(), called from vfio_pci_fault() but we can't
> > >  	 * change vm_flags within the fault handler.  Set them now.
> > > +	 *
> > > +	 * Set an additional flag VM_ALLOW_ANY_UNCACHED to convey kvm that
> > > +	 * the device is wc safe.
> > >  	 */  
> > 
> > That's a pretty superficial comment.  Check that this is accurate, but
> > maybe something like:
> > 
> > 	The VM_ALLOW_ANY_UNCACHED flag is implemented for ARM64,
> > 	allowing stage 2 device mapping attributes to use Normal-NC  
>                ^^^^ 
> 
> > 	rather than DEVICE_nGnRE, which allows guest mappings
> > 	supporting combining attributes (WC).  This attribute has
> > 	potential risks with the GICv2 VCPU interface, but is expected
> > 	to be safe for vfio-pci use cases.  
> 
> Sure, if you want to elaborate more
> 
>   The VM_ALLOW_ANY_UNCACHED flag is implemented for ARM64,
>   allowing KVM stage 2 device mapping attributes to use Normal-NC
>   rather than DEVICE_nGnRE, which allows guest mappings
>   supporting combining attributes (WC). ARM does not architecturally
>   guarentee this is safe, and indeed some MMIO regions like the GICv2
>   VCPU interface can trigger uncontained faults if Normal-NC is used.
> 
>   Even worse we expect there are platforms where even DEVICE_nGnRE can
>   allow uncontained faults in conercases. Unfortunately existing ARM
                                ^^^^^^^^^^

*corner cases


>   IP requires platform integration to take responsibility to prevent
>   this.
> 
>   To safely use VFIO in KVM the platform must guarantee full safety
>   in the guest where no action taken against a MMIO mapping can
>   trigger an uncontainer failure. We belive that most VFIO PCI
>   platforms support this for both mapping types, at least in common
>   flows, based on some expectations of how PCI IP is integrated. This
>   can be enabled more broadly, for instance into vfio-platform
>   drivers, but only after the platform vendor completes auditing for
>   safety.

I like it, please incorporate into the next version.
  
> > And specifically, I think these other devices that may be problematic
> > as described in the cover letter is a warning against use for
> > vfio-platform, is that correct?  
> 
> Maybe more like "we have a general consensus that vfio-pci is likely
> safe due to how PCI IP is typically integrated, but it is much less
> obvious for other VFIO bus types. As there is no known WC user for
> vfio-platform drivers be conservative and do not enable it."

Ok.  Thanks for the clarification.

Alex
Ankit Agrawal Feb. 13, 2024, 1:59 a.m. UTC | #5 
>>   IP requires platform integration to take responsibility to prevent
>>   this.
>>
>>   To safely use VFIO in KVM the platform must guarantee full safety
>>   in the guest where no action taken against a MMIO mapping can
>>   trigger an uncontainer failure. We belive that most VFIO PCI
>>   platforms support this for both mapping types, at least in common
>>   flows, based on some expectations of how PCI IP is integrated. This
>>   can be enabled more broadly, for instance into vfio-platform
>>   drivers, but only after the platform vendor completes auditing for
>>   safety.
>
> I like it, please incorporate into the next version.

Yes, will fix the typos and add it.
diff mbox series

Patch

diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 1cbc990d42e0..eba2146202f9 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -1862,8 +1862,12 @@  int vfio_pci_core_mmap(struct vfio_device *core_vdev, struct vm_area_struct *vma
 	/*
 	 * See remap_pfn_range(), called from vfio_pci_fault() but we can't
 	 * change vm_flags within the fault handler.  Set them now.
+	 *
+	 * Set an additional flag VM_ALLOW_ANY_UNCACHED to convey kvm that
+	 * the device is wc safe.
 	 */
-	vm_flags_set(vma, VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP);
+	vm_flags_set(vma, VM_ALLOW_ANY_UNCACHED | VM_IO | VM_PFNMAP |
+			VM_DONTEXPAND | VM_DONTDUMP);
 	vma->vm_ops = &vfio_pci_mmap_ops;
 
 	return 0;