Message ID | 1621914605-14724-4-git-send-email-wangxingang5@huawei.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | IOMMU: Add support for IOMMU Bypass Feature | expand |
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;
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 .
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; + /* 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;