| Message ID | 20240823132137.336874-21-aik@amd.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Secure VFIO, TDISP, SEV TIO | expand |
On Fri, Aug 23, 2024 at 11:21:34PM +1000, Alexey Kardashevskiy wrote: > Add another resource#d_enc to allow mapping MMIO as > an encrypted/private region. > > Unlike resourceN_wc, the node is added always as ability to > map MMIO as private depends on negotiation with the TSM which > happens quite late. Capitalize subject prefix. Wrap to fill 75 columns. > +++ b/include/linux/pci.h > @@ -2085,7 +2085,7 @@ pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs, > */ > int pci_mmap_resource_range(struct pci_dev *dev, int bar, > struct vm_area_struct *vma, > - enum pci_mmap_state mmap_state, int write_combine); > + enum pci_mmap_state mmap_state, int write_combine, int enc); This interface is only used in drivers/pci and look like it should be moved to drivers/pci/pci.h. > @@ -46,6 +46,15 @@ int pci_mmap_resource_range(struct pci_dev *pdev, int bar, > > vma->vm_ops = &pci_phys_vm_ops; > > + /* > + * Calling remap_pfn_range() directly as io_remap_pfn_range() > + * enforces shared mapping. s/Calling/Call/ Needs some additional context about why io_remap_pfn_range() can't be used here. > + */ > + if (enc) > + return remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff, > + vma->vm_end - vma->vm_start, > + vma->vm_page_prot); > + > return io_remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff, > vma->vm_end - vma->vm_start, > vma->vm_page_prot);
On 24/8/24 08:37, Bjorn Helgaas wrote: > On Fri, Aug 23, 2024 at 11:21:34PM +1000, Alexey Kardashevskiy wrote: >> Add another resource#d_enc to allow mapping MMIO as >> an encrypted/private region. >> >> Unlike resourceN_wc, the node is added always as ability to >> map MMIO as private depends on negotiation with the TSM which >> happens quite late. > > Capitalize subject prefix. > > Wrap to fill 75 columns. > >> +++ b/include/linux/pci.h >> @@ -2085,7 +2085,7 @@ pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs, >> */ >> int pci_mmap_resource_range(struct pci_dev *dev, int bar, >> struct vm_area_struct *vma, >> - enum pci_mmap_state mmap_state, int write_combine); >> + enum pci_mmap_state mmap_state, int write_combine, int enc); > > This interface is only used in drivers/pci and look like it should be > moved to drivers/pci/pci.h. > >> @@ -46,6 +46,15 @@ int pci_mmap_resource_range(struct pci_dev *pdev, int bar, >> >> vma->vm_ops = &pci_phys_vm_ops; >> >> + /* >> + * Calling remap_pfn_range() directly as io_remap_pfn_range() >> + * enforces shared mapping. > > s/Calling/Call/ > > Needs some additional context about why io_remap_pfn_range() can't be > used here. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f8f6ae5d077a9bdaf5cbf2ac960a5d1a04b47482 added this. "IO devices do not understand encryption, so this memory must always be decrypted" it says. But devices do understand encryption so forcing decryption is not wanted. What additional context is missing here, that "shared" means "non-encrypted"? Thanks, > >> + */ >> + if (enc) >> + return remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff, >> + vma->vm_end - vma->vm_start, >> + vma->vm_page_prot); >> + >> return io_remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff, >> vma->vm_end - vma->vm_start, >> vma->vm_page_prot);
On Mon, Sep 02, 2024 at 06:22:00PM +1000, Alexey Kardashevskiy wrote: > On 24/8/24 08:37, Bjorn Helgaas wrote: > > On Fri, Aug 23, 2024 at 11:21:34PM +1000, Alexey Kardashevskiy wrote: > > > Add another resource#d_enc to allow mapping MMIO as > > > an encrypted/private region. > > > > > > Unlike resourceN_wc, the node is added always as ability to > > > map MMIO as private depends on negotiation with the TSM which > > > happens quite late. > > > @@ -46,6 +46,15 @@ int pci_mmap_resource_range(struct pci_dev *pdev, int bar, > > > vma->vm_ops = &pci_phys_vm_ops; > > > + /* > > > + * Calling remap_pfn_range() directly as io_remap_pfn_range() > > > + * enforces shared mapping. > > > > s/Calling/Call/ > > > > Needs some additional context about why io_remap_pfn_range() can't be > > used here. > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f8f6ae5d077a9bdaf5cbf2ac960a5d1a04b47482 > added this. > > "IO devices do not understand encryption, so this memory must always be > decrypted" it says. Thanks for the pointer. Given that hint, the pgprot_decrypted() inside io_remap_pfn_range() is ... at least *there*, if not obvious. io_remap_pfn_range() probably could benefit from a simple comment to highlight that. > But devices do understand encryption so forcing decryption is not wanted. > What additional context is missing here, that "shared" means > "non-encrypted"? Thanks, If "shared" means "non-encrypted", that would be useful. That wasn't obvious to me. IIUC, in the "enc" case, you *want* the mapping to remain encrypted? In that case, it would be helpful to say something like "io_remap_pfn_range() always produces decrypted mappings, so use remap_pfn_range() directly to avoid the decryption". Renaming "enc" to "encrypted" would also be a nice hint. > > > + */ > > > + if (enc) > > > + return remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff, > > > + vma->vm_end - vma->vm_start, > > > + vma->vm_page_prot); > > > + > > > return io_remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff, > > > vma->vm_end - vma->vm_start, > > > vma->vm_page_prot); > > -- > Alexey >
diff --git a/include/linux/pci.h b/include/linux/pci.h index 053b7c506818..3c44542f66df 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -2085,7 +2085,7 @@ pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs, */ int pci_mmap_resource_range(struct pci_dev *dev, int bar, struct vm_area_struct *vma, - enum pci_mmap_state mmap_state, int write_combine); + enum pci_mmap_state mmap_state, int write_combine, int enc); #ifndef arch_can_pci_mmap_wc #define arch_can_pci_mmap_wc() 0 diff --git a/drivers/pci/mmap.c b/drivers/pci/mmap.c index 8da3347a95c4..4fd522aeb767 100644 --- a/drivers/pci/mmap.c +++ b/drivers/pci/mmap.c @@ -23,7 +23,7 @@ static const struct vm_operations_struct pci_phys_vm_ops = { int pci_mmap_resource_range(struct pci_dev *pdev, int bar, struct vm_area_struct *vma, - enum pci_mmap_state mmap_state, int write_combine) + enum pci_mmap_state mmap_state, int write_combine, int enc) { unsigned long size; int ret; @@ -46,6 +46,15 @@ int pci_mmap_resource_range(struct pci_dev *pdev, int bar, vma->vm_ops = &pci_phys_vm_ops; + /* + * Calling remap_pfn_range() directly as io_remap_pfn_range() + * enforces shared mapping. + */ + if (enc) + return remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff, + vma->vm_end - vma->vm_start, + vma->vm_page_prot); + return io_remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff, vma->vm_end - vma->vm_start, vma->vm_page_prot); diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c index bf019371ef9a..1b0eca1751c2 100644 --- a/drivers/pci/pci-sysfs.c +++ b/drivers/pci/pci-sysfs.c @@ -1032,7 +1032,7 @@ void pci_remove_legacy_files(struct pci_bus *b) * Use the regular PCI mapping routines to map a PCI resource into userspace. */ static int pci_mmap_resource(struct kobject *kobj, struct bin_attribute *attr, - struct vm_area_struct *vma, int write_combine) + struct vm_area_struct *vma, int write_combine, int enc) { struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj)); int bar = (unsigned long)attr->private; @@ -1052,21 +1052,28 @@ static int pci_mmap_resource(struct kobject *kobj, struct bin_attribute *attr, mmap_type = res->flags & IORESOURCE_MEM ? pci_mmap_mem : pci_mmap_io; - return pci_mmap_resource_range(pdev, bar, vma, mmap_type, write_combine); + return pci_mmap_resource_range(pdev, bar, vma, mmap_type, write_combine, enc); } static int pci_mmap_resource_uc(struct file *filp, struct kobject *kobj, struct bin_attribute *attr, struct vm_area_struct *vma) { - return pci_mmap_resource(kobj, attr, vma, 0); + return pci_mmap_resource(kobj, attr, vma, 0, 0); } static int pci_mmap_resource_wc(struct file *filp, struct kobject *kobj, struct bin_attribute *attr, struct vm_area_struct *vma) { - return pci_mmap_resource(kobj, attr, vma, 1); + return pci_mmap_resource(kobj, attr, vma, 1, 0); +} + +static int pci_mmap_resource_enc(struct file *filp, struct kobject *kobj, + struct bin_attribute *attr, + struct vm_area_struct *vma) +{ + return pci_mmap_resource(kobj, attr, vma, 0, 1); } static ssize_t pci_resource_io(struct file *filp, struct kobject *kobj, @@ -1160,7 +1167,7 @@ static void pci_remove_resource_files(struct pci_dev *pdev) } } -static int pci_create_attr(struct pci_dev *pdev, int num, int write_combine) +static int pci_create_attr(struct pci_dev *pdev, int num, int write_combine, int enc) { /* allocate attribute structure, piggyback attribute name */ int name_len = write_combine ? 13 : 10; @@ -1178,6 +1185,9 @@ static int pci_create_attr(struct pci_dev *pdev, int num, int write_combine) if (write_combine) { sprintf(res_attr_name, "resource%d_wc", num); res_attr->mmap = pci_mmap_resource_wc; + } else if (enc) { + sprintf(res_attr_name, "resource%d_enc", num); + res_attr->mmap = pci_mmap_resource_enc; } else { sprintf(res_attr_name, "resource%d", num); if (pci_resource_flags(pdev, num) & IORESOURCE_IO) { @@ -1234,11 +1244,14 @@ static int pci_create_resource_files(struct pci_dev *pdev) if (!pci_resource_len(pdev, i)) continue; - retval = pci_create_attr(pdev, i, 0); + retval = pci_create_attr(pdev, i, 0, 0); /* for prefetchable resources, create a WC mappable file */ if (!retval && arch_can_pci_mmap_wc() && pdev->resource[i].flags & IORESOURCE_PREFETCH) - retval = pci_create_attr(pdev, i, 1); + retval = pci_create_attr(pdev, i, 1, 0); + /* Add node for private MMIO mapping */ + if (!retval) + retval = pci_create_attr(pdev, i, 0, 1); if (retval) { pci_remove_resource_files(pdev); return retval; diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c index f967709082d6..62992c8234f1 100644 --- a/drivers/pci/proc.c +++ b/drivers/pci/proc.c @@ -284,7 +284,7 @@ static int proc_bus_pci_mmap(struct file *file, struct vm_area_struct *vma) /* Adjust vm_pgoff to be the offset within the resource */ vma->vm_pgoff -= start >> PAGE_SHIFT; ret = pci_mmap_resource_range(dev, i, vma, - fpriv->mmap_state, write_combine); + fpriv->mmap_state, write_combine, 0); if (ret < 0) return ret;
Add another resource#d_enc to allow mapping MMIO as an encrypted/private region. Unlike resourceN_wc, the node is added always as ability to map MMIO as private depends on negotiation with the TSM which happens quite late. Signed-off-by: Alexey Kardashevskiy <aik@amd.com> --- include/linux/pci.h | 2 +- drivers/pci/mmap.c | 11 +++++++- drivers/pci/pci-sysfs.c | 27 +++++++++++++++----- drivers/pci/proc.c | 2 +- 4 files changed, 32 insertions(+), 10 deletions(-)