Re: [PATCH OLK-6.6 1/1] kvm: add coda feature

On 2024/8/17 9:54, JunBin Li wrote:

virtcca inclusion category: feature bugzilla: https://gitee.com/openeuler/kernel/issues/IADD42

---

virtCCA feature

What is virtCCA? How it works? and why do you want to introduce this feature?

Please give some detail information about what, why, how.

Signed-off-by: JunBin Li lijunbin4@huawei.com

arch/arm64/kvm/mmu.c | 23 +++ arch/arm64/kvm/tmi.c | 24 ++++ arch/arm64/kvm/virtcca_cvm.c | 231 ++++++++++++++++++++++++++++++- drivers/iommu/io-pgtable-arm.c | 81 +++++++++++ drivers/iommu/iommu.c | 11 ++ drivers/pci/access.c | 33 +++++ drivers/pci/msi/msi.c | 85 ++++++++++-- drivers/pci/msi/msi.h | 29 +++- drivers/vfio/group.c | 2 +- drivers/vfio/pci/vfio_pci_core.c | 16 ++- drivers/vfio/pci/vfio_pci_rdwr.c | 61 ++++++++ drivers/vfio/vfio_iommu_type1.c | 82 +++++++++++ drivers/vfio/vfio_main.c | 18 +++ include/linux/iommu.h | 5 + include/linux/pci.h | 4 + include/linux/vfio.h | 5 + include/uapi/linux/vfio.h | 3 + virt/kvm/vfio.c | 127 ++++++++++++++++- virt/kvm/vfio.h | 11 ++ 19 files changed, 836 insertions(+), 15 deletions(-)

How can I review this? It's a big patch for review!

+u64 iova_to_pa(void *addr) +{

• uint64_t pa, par_el1;
• asm volatile(

• "AT S1E1W, %0\n"
  

• ::"r"((uint64_t)(addr))
  

• );
• isb();
• asm volatile(

• "mrs %0, par_el1\n"
  

• : "=r"(par_el1)
  

• );
• pa = ((uint64_t)(addr) & (PAGE_SIZE - 1)) |

• (par_el1 & ULL(0x000ffffffffff000));
  

• if (par_el1 & UL(1 << 0))

• return (uint64_t)(addr);
  

• else

• return pa;
  

+} +EXPORT_SYMBOL(iova_to_pa);

Why not use some functions in IOVA subsystem? and why use some specfic registers? does it rely on specific hardware?

• u64 tmi_version(void) { struct arm_smccc_res res;

diff --git a/arch/arm64/kvm/virtcca_cvm.c b/arch/arm64/kvm/virtcca_cvm.c index 367bbf4fa3..75c1d2de3f 100644 --- a/arch/arm64/kvm/virtcca_cvm.c +++ b/arch/arm64/kvm/virtcca_cvm.c @@ -4,6 +4,7 @@ */ #include <linux/kvm_host.h> #include <linux/kvm.h> +#include <linux/vfio.h> #include <asm/kvm_tmi.h> #include <asm/kvm_pgtable.h> #include <asm/kvm_emulate.h> @@ -12,6 +13,7 @@ #include <linux/arm-smccc.h> #include <kvm/arm_hypercalls.h> #include <kvm/arm_psci.h> +#include "../virt/kvm/vfio.h"

This is not the right way to include the head file. it shows your architecture is messed up.

static int arm_lpae_map_pages(struct io_pgtable_ops *ops, unsigned long iova, phys_addr_t paddr, size_t pgsize, size_t pgcount, int iommu_prot, gfp_t gfp, size_t *mapped) @@ -522,12 +578,24 @@ static int arm_lpae_map_pages(struct io_pgtable_ops *ops, unsigned long iova, return 0;

prot = arm_lpae_prot_to_pte(data, iommu_prot);

+#ifdef CONFIG_HISI_VIRTCCA_HOST

• ret = arm_lpae_cvm_map_unmap_pages(data, iova, paddr, pgsize * pgcount, true, mapped);
• struct kvm *kvm = arm_smmu_domain_get_kvm(data);
• if (kvm && kvm_is_virtcca_cvm(kvm))

• goto out;
  

+#endif

• ret = __arm_lpae_map(data, iova, paddr, pgsize, pgcount, prot, lvl, ptep, gfp, mapped); /*
  • Synchronise all PTE updates for the new mapping before there's
  • a chance for anything to kick off a table walk for the new iova.
  */

+#ifdef CONFIG_HISI_VIRTCCA_HOST +out: +#endif wmb();

return ret; @@ -708,6 +776,19 @@ static size_t arm_lpae_unmap_pages(struct io_pgtable_ops *ops, unsigned long iov if (WARN_ON(iaext)) return 0;

+#ifdef CONFIG_HISI_VIRTCCA_HOST

• int ret;
• ret = arm_lpae_cvm_map_unmap_pages(data, iova, 0, pgsize * pgcount, false, NULL);
• if (ret)

• pr_err("%s %d failed to unmap pages, iova %lx, size %lx\n",
  

• 	__func__, __LINE__, iova, pgsize);
  

• struct kvm *kvm = arm_smmu_domain_get_kvm(data);
• if (kvm && kvm_is_virtcca_cvm(kvm))

• return 0;
  

+#endif

So virtcca and normal vm will not be available at the same time, I mean in a single kernel image, right?

• return __arm_lpae_unmap(data, gather, iova, pgsize, pgcount, data->start_level, ptep); }

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 28f63ad432..07a084c9d9 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -592,6 +592,17 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list return ret; }

+#ifdef CONFIG_HISI_VIRTCCA_HOST +struct iommu_domain *iommu_group_get_domain(struct iommu_group *iommu_group) +{

• if (iommu_group)

• return iommu_group->domain;
  

• return NULL;

+} +EXPORT_SYMBOL_GPL(iommu_group_get_domain);

I think we already have some interface like this.

+#endif

• int iommu_probe_device(struct device *dev) { const struct iommu_ops *ops;

diff --git a/drivers/pci/access.c b/drivers/pci/access.c index 6554a2e89d..1e616d7768 100644 --- a/drivers/pci/access.c +++ b/drivers/pci/access.c @@ -7,6 +7,10 @@

#include "pci.h"

+#ifdef CONFIG_HISI_VIRTCCA_HOST +#include <asm/kvm_tmi.h> +#endif

• /*
  • This interrupt-safe spinlock protects all accesses to PCI
  • configuration space.

@@ -86,6 +90,19 @@ int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn, if (!addr) return PCIBIOS_DEVICE_NOT_FOUND;

+#ifdef CONFIG_HISI_VIRTCCA_HOST

• if (is_cc_dev((bus->number << 8) | devfn)) {

• if (size == 1)
  

• 	*val = tmi_mmio_read(iova_to_pa(addr), 8, ((bus->number << 8) | devfn));
  

• else if (size == 2)
  

• 	*val = tmi_mmio_read(iova_to_pa(addr), 16, ((bus->number << 8) | devfn));
  

• else
  

• 	*val = tmi_mmio_read(iova_to_pa(addr), 32, ((bus->number << 8) | devfn));
  

• return PCIBIOS_SUCCESSFUL;
  

• }

why? if the hardware has no isolation on the device, how can you make sure it's a CC device?

+#endif

• if (size == 1) *val = readb(addr); else if (size == 2)

@@ -106,6 +123,22 @@ int pci_generic_config_write(struct pci_bus *bus, unsigned int devfn, if (!addr) return PCIBIOS_DEVICE_NOT_FOUND;

+#ifdef CONFIG_HISI_VIRTCCA_HOST

• if (is_cc_dev((bus->number << 8) | devfn)) {

• if (size == 1)
  

• 	WARN_ON(tmi_mmio_write(iova_to_pa(addr), val,
  

• 		8, ((bus->number << 8) | devfn)));
  

• else if (size == 2)
  

• 	WARN_ON(tmi_mmio_write(iova_to_pa(addr), val,
  

• 		16, ((bus->number << 8) | devfn)));
  

• else
  

• 	WARN_ON(tmi_mmio_write(iova_to_pa(addr), val,
  

• 		32, ((bus->number << 8) | devfn)));
  

• return PCIBIOS_SUCCESSFUL;
  

• }

+#endif

• if (size == 1) writeb(val, addr); else if (size == 2)

diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c index 161c3ac171..c48ff83f65 100644 --- a/drivers/pci/msi/msi.c +++ b/drivers/pci/msi/msi.c @@ -159,9 +159,23 @@ void __pci_read_msi_msg(struct msi_desc *entry, struct msi_msg *msg) if (WARN_ON_ONCE(entry->pci.msi_attrib.is_virtual)) return;

• msg->address_lo = readl(base + PCI_MSIX_ENTRY_LOWER_ADDR);
  

• msg->address_hi = readl(base + PCI_MSIX_ENTRY_UPPER_ADDR);
  

• msg->data = readl(base + PCI_MSIX_ENTRY_DATA);
  

+#ifdef CONFIG_HISI_VIRTCCA_HOST

• u64 pbase = iova_to_pa(base);
  

• if (virtcca_is_available() && dev != NULL && is_cc_dev(pci_dev_id(dev))) {
  

• 	msg->address_lo = tmi_mmio_read(pbase + PCI_MSIX_ENTRY_LOWER_ADDR,
  

• 		32, pci_dev_id(dev));
  

• 	msg->address_hi = tmi_mmio_read(pbase + PCI_MSIX_ENTRY_UPPER_ADDR,
  

• 		32, pci_dev_id(dev));
  

• 	msg->data = tmi_mmio_read(pbase + PCI_MSIX_ENTRY_DATA, 32, pci_dev_id(dev));
  

• } else {
  

+#endif

• 	msg->address_lo = readl(base + PCI_MSIX_ENTRY_LOWER_ADDR);
  

• 	msg->address_hi = readl(base + PCI_MSIX_ENTRY_UPPER_ADDR);
  

• 	msg->data = readl(base + PCI_MSIX_ENTRY_DATA);
  

+#ifdef CONFIG_HISI_VIRTCCA_HOST

• }
  

+#endif

You just ignore everything and hack your code to make it work, I can't review you code anymore.

NACK for the whole solution.

Thanks Hanjun