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

[Xingang Wang]

Hi Eric,

On 2021/6/2 20:25, Eric Auger wrote:
> 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

Thanks, maybe I should replace the name to "default_rootbus_bypass_iommu".
I just considered that the name might be too long,
anyway, explicitness is important, I will fix this.


> > 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

Thanks, I will make this annotation clearer.

> > +
> >       /* 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;
>
> .

Xingang

.