Re: [PATCH v4 3/8] hw/arm/virt: Add a machine option to bypass iommu for primary bus

From: Eric Auger <eric.auger@redhat.com>
To: Wang Xingang <wangxingang5@huawei.com>,
	qemu-devel@nongnu.org, qemu-arm@nongnu.org,
	shannon.zhaosl@gmail.com, imammedo@redhat.com, mst@redhat.com,
	marcel.apfelbaum@gmail.com, peter.maydell@linaro.org,
	ehabkost@redhat.com, richard.henderson@linaro.org,
	pbonzini@redhat.com
Cc: xieyingtai@huawei.com
Subject: Re: [PATCH v4 3/8] hw/arm/virt: Add a machine option to bypass iommu for primary bus
Date: Wed, 2 Jun 2021 14:25:50 +0200	[thread overview]
Message-ID: <b1f26e3a-d678-aa50-4e82-71c76c9775b8@redhat.com> (raw)
In-Reply-To: <1621914605-14724-4-git-send-email-wangxingang5@huawei.com>

Hi Xingang,

On 5/25/21 5:50 AM, Wang Xingang wrote:
> From: Xingang Wang <wangxingang5@huawei.com>
>
> This add a bypass_iommu option for arm virt machine,
> the option can be used in this manner:
> qemu -machine virt,iommu=smmuv3,bypass_iommu=true
This still looks confusing to me. On one hand we say that for the virt
machine the iommu is set to smmuv3 and we say bypass_iommu=true on the
virt machine option line
It is not straightforward that the bypass_iommu only relates to devices
plugged onto the "default" root bus.

At least the name of the property should reflect that I think

> Signed-off-by: Xingang Wang <wangxingang5@huawei.com>
> ---
>  hw/arm/virt.c         | 26 ++++++++++++++++++++++++++
>  include/hw/arm/virt.h |  1 +
>  2 files changed, 27 insertions(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 840758666d..49d8a801ed 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1364,6 +1364,7 @@ static void create_pcie(VirtMachineState *vms)
>      }
>  
>      pci = PCI_HOST_BRIDGE(dev);
> +    pci->bypass_iommu = vms->bypass_iommu;
>      vms->bus = pci->bus;
>      if (vms->bus) {
>          for (i = 0; i < nb_nics; i++) {
> @@ -2319,6 +2320,21 @@ static void virt_set_iommu(Object *obj, const char *value, Error **errp)
>      }
>  }
>  
> +static bool virt_get_bypass_iommu(Object *obj, Error **errp)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> +    return vms->bypass_iommu;
> +}
> +
> +static void virt_set_bypass_iommu(Object *obj, bool value,
> +                                              Error **errp)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> +    vms->bypass_iommu = value;
> +}
> +
>  static CpuInstanceProperties
>  virt_cpu_index_to_props(MachineState *ms, unsigned cpu_index)
>  {
> @@ -2656,6 +2672,13 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>                                            "Set the IOMMU type. "
>                                            "Valid values are none and smmuv3");
>  
> +    object_class_property_add_bool(oc, "bypass_iommu",
> +                                   virt_get_bypass_iommu,
> +                                   virt_set_bypass_iommu);
> +    object_class_property_set_description(oc, "bypass_iommu",
> +                                          "Set on/off to enable/disable "
> +                                          "bypass_iommu for primary bus");
> +
>      object_class_property_add_bool(oc, "ras", virt_get_ras,
>                                     virt_set_ras);
>      object_class_property_set_description(oc, "ras",
> @@ -2723,6 +2746,9 @@ static void virt_instance_init(Object *obj)
>      /* Default disallows iommu instantiation */
>      vms->iommu = VIRT_IOMMU_NONE;
>  
> +    /* The primary bus is attached to iommu by default */
> +    vms->bypass_iommu = false;
I don't fully master the PCI topology but I think you should clarify
which primary bus we talk about. AFAIU PXB's bus also is a primary bus.

Thanks

Eric
> +
>      /* Default disallows RAS instantiation */
>      vms->ras = false;
>  
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 921416f918..82bceadb82 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -147,6 +147,7 @@ struct VirtMachineState {
>      OnOffAuto acpi;
>      VirtGICType gic_version;
>      VirtIOMMUType iommu;
> +    bool bypass_iommu;
>      VirtMSIControllerType msi_controller;
>      uint16_t virtio_iommu_bdf;
>      struct arm_boot_info bootinfo;