Message ID | 20230622214845.3980-7-joao.m.martins@oracle.com |
---|---|
State | New |
Headers | show |
Series | vfio: VFIO migration support with vIOMMU | expand |
On 6/23/2023 5:48 AM, Joao Martins wrote: > Implement IOMMU MR get_attr() method and use the dma_translation > property to report the IOMMU_ATTR_DMA_TRANSLATION attribute. > Additionally add the necessary get_iommu_attr into the PCIIOMMUOps to > support pci_device_iommu_get_attr(). > > The callback in there acts as a IOMMU-specific address space walker > which will call get_attr in the IOMMUMemoryRegion backing the device to > fetch the desired attribute. > > Signed-off-by: Avihai Horon <avihaih@nvidia.com> > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > --- > hw/i386/intel_iommu.c | 40 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index 1606d1b952d0..ed2a46e008df 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -3861,6 +3861,29 @@ static void vtd_iommu_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n) > return; > } > > +static int vtd_iommu_get_attr(IOMMUMemoryRegion *iommu_mr, > + enum IOMMUMemoryRegionAttr attr, void *data) > +{ > + VTDAddressSpace *vtd_as = container_of(iommu_mr, VTDAddressSpace, iommu); > + IntelIOMMUState *s = vtd_as->iommu_state; > + int ret = 0; > + > + switch (attr) { > + case IOMMU_ATTR_DMA_TRANSLATION: > + { > + bool *enabled = (bool *)(uintptr_t) data; > + > + *enabled = s->dma_translation; > + break; > + } > + default: > + ret = -EINVAL; > + break; > + } > + > + return ret; > +} > + > /* Do the initialization. It will also be called when reset, so pay > * attention when adding new initialization stuff. > */ > @@ -4032,8 +4055,24 @@ static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) > return &vtd_as->as; > } > > +static int vtd_get_iommu_attr(PCIBus *bus, void *opaque, int32_t devfn, > + enum IOMMUMemoryRegionAttr attr, void *data) > +{ > + IntelIOMMUState *s = opaque; > + VTDAddressSpace *vtd_as; > + > + assert(0 <= devfn && devfn < PCI_DEVFN_MAX); > + > + vtd_as = vtd_find_add_as(s, bus, devfn, PCI_NO_PASID); > + if (!vtd_as) > + return -EINVAL; > + > + return memory_region_iommu_get_attr(&vtd_as->iommu, attr, data); > +} > + > static PCIIOMMUOps vtd_iommu_ops = { > .get_address_space = vtd_host_dma_iommu, > + .get_iommu_attr = vtd_get_iommu_attr, > }; > > static bool vtd_decide_config(IntelIOMMUState *s, Error **errp) > @@ -4197,6 +4236,7 @@ static void vtd_iommu_memory_region_class_init(ObjectClass *klass, > imrc->translate = vtd_iommu_translate; > imrc->notify_flag_changed = vtd_iommu_notify_flag_changed; > imrc->replay = vtd_iommu_replay; > + imrc->get_attr = vtd_iommu_get_attr; Do we have other user of imrc->get_attr? If not, what about squashing vtd_iommu_get_attr into vtd_get_iommu_attr and have imrc->get_attr removed? Thanks Zhenzhong > } > > static const TypeInfo vtd_iommu_memory_region_info = {
On 08/09/2023 07:23, Duan, Zhenzhong wrote: > > On 6/23/2023 5:48 AM, Joao Martins wrote: >> Implement IOMMU MR get_attr() method and use the dma_translation >> property to report the IOMMU_ATTR_DMA_TRANSLATION attribute. >> Additionally add the necessary get_iommu_attr into the PCIIOMMUOps to >> support pci_device_iommu_get_attr(). >> >> The callback in there acts as a IOMMU-specific address space walker >> which will call get_attr in the IOMMUMemoryRegion backing the device to >> fetch the desired attribute. >> >> Signed-off-by: Avihai Horon <avihaih@nvidia.com> >> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >> --- >> hw/i386/intel_iommu.c | 40 ++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 40 insertions(+) >> >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >> index 1606d1b952d0..ed2a46e008df 100644 >> --- a/hw/i386/intel_iommu.c >> +++ b/hw/i386/intel_iommu.c >> @@ -3861,6 +3861,29 @@ static void vtd_iommu_replay(IOMMUMemoryRegion >> *iommu_mr, IOMMUNotifier *n) >> return; >> } >> +static int vtd_iommu_get_attr(IOMMUMemoryRegion *iommu_mr, >> + enum IOMMUMemoryRegionAttr attr, void *data) >> +{ >> + VTDAddressSpace *vtd_as = container_of(iommu_mr, VTDAddressSpace, iommu); >> + IntelIOMMUState *s = vtd_as->iommu_state; >> + int ret = 0; >> + >> + switch (attr) { >> + case IOMMU_ATTR_DMA_TRANSLATION: >> + { >> + bool *enabled = (bool *)(uintptr_t) data; >> + >> + *enabled = s->dma_translation; >> + break; >> + } >> + default: >> + ret = -EINVAL; >> + break; >> + } >> + >> + return ret; >> +} >> + >> /* Do the initialization. It will also be called when reset, so pay >> * attention when adding new initialization stuff. >> */ >> @@ -4032,8 +4055,24 @@ static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, >> void *opaque, int devfn) >> return &vtd_as->as; >> } >> +static int vtd_get_iommu_attr(PCIBus *bus, void *opaque, int32_t devfn, >> + enum IOMMUMemoryRegionAttr attr, void *data) >> +{ >> + IntelIOMMUState *s = opaque; >> + VTDAddressSpace *vtd_as; >> + >> + assert(0 <= devfn && devfn < PCI_DEVFN_MAX); >> + >> + vtd_as = vtd_find_add_as(s, bus, devfn, PCI_NO_PASID); >> + if (!vtd_as) >> + return -EINVAL; >> + >> + return memory_region_iommu_get_attr(&vtd_as->iommu, attr, data); >> +} >> + >> static PCIIOMMUOps vtd_iommu_ops = { >> .get_address_space = vtd_host_dma_iommu, >> + .get_iommu_attr = vtd_get_iommu_attr, >> }; >> static bool vtd_decide_config(IntelIOMMUState *s, Error **errp) >> @@ -4197,6 +4236,7 @@ static void >> vtd_iommu_memory_region_class_init(ObjectClass *klass, >> imrc->translate = vtd_iommu_translate; >> imrc->notify_flag_changed = vtd_iommu_notify_flag_changed; >> imrc->replay = vtd_iommu_replay; >> + imrc->get_attr = vtd_iommu_get_attr; > > Do we have other user of imrc->get_attr? If not, what about squashing > > vtd_iommu_get_attr into vtd_get_iommu_attr and have imrc->get_attr removed? > There's an user, and that's VFIO, but it's later in the series. It felt more natural for bisection to do things this way.
On 6/22/23 23:48, Joao Martins wrote: > Implement IOMMU MR get_attr() method and use the dma_translation > property to report the IOMMU_ATTR_DMA_TRANSLATION attribute. > Additionally add the necessary get_iommu_attr into the PCIIOMMUOps to > support pci_device_iommu_get_attr(). > > The callback in there acts as a IOMMU-specific address space walker > which will call get_attr in the IOMMUMemoryRegion backing the device to > fetch the desired attribute. This looks like two patches to me and the previous should be merged with the one introducing vtd_iommu_get_attr(). Thanks, C. > Signed-off-by: Avihai Horon <avihaih@nvidia.com> > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > --- > hw/i386/intel_iommu.c | 40 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index 1606d1b952d0..ed2a46e008df 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -3861,6 +3861,29 @@ static void vtd_iommu_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n) > return; > } > > +static int vtd_iommu_get_attr(IOMMUMemoryRegion *iommu_mr, > + enum IOMMUMemoryRegionAttr attr, void *data) > +{ > + VTDAddressSpace *vtd_as = container_of(iommu_mr, VTDAddressSpace, iommu); > + IntelIOMMUState *s = vtd_as->iommu_state; > + int ret = 0; > + > + switch (attr) { > + case IOMMU_ATTR_DMA_TRANSLATION: > + { > + bool *enabled = (bool *)(uintptr_t) data; > + > + *enabled = s->dma_translation; > + break; > + } > + default: > + ret = -EINVAL; > + break; > + } > + > + return ret; > +} > + > /* Do the initialization. It will also be called when reset, so pay > * attention when adding new initialization stuff. > */ > @@ -4032,8 +4055,24 @@ static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) > return &vtd_as->as; > } > > +static int vtd_get_iommu_attr(PCIBus *bus, void *opaque, int32_t devfn, > + enum IOMMUMemoryRegionAttr attr, void *data) > +{ > + IntelIOMMUState *s = opaque; > + VTDAddressSpace *vtd_as; > + > + assert(0 <= devfn && devfn < PCI_DEVFN_MAX); > + > + vtd_as = vtd_find_add_as(s, bus, devfn, PCI_NO_PASID); > + if (!vtd_as) > + return -EINVAL; > + > + return memory_region_iommu_get_attr(&vtd_as->iommu, attr, data); > +} > + > static PCIIOMMUOps vtd_iommu_ops = { > .get_address_space = vtd_host_dma_iommu, > + .get_iommu_attr = vtd_get_iommu_attr, > }; > > static bool vtd_decide_config(IntelIOMMUState *s, Error **errp) > @@ -4197,6 +4236,7 @@ static void vtd_iommu_memory_region_class_init(ObjectClass *klass, > imrc->translate = vtd_iommu_translate; > imrc->notify_flag_changed = vtd_iommu_notify_flag_changed; > imrc->replay = vtd_iommu_replay; > + imrc->get_attr = vtd_iommu_get_attr; > } > > static const TypeInfo vtd_iommu_memory_region_info = {
On 02/10/2023 16:23, Cédric Le Goater wrote: > On 6/22/23 23:48, Joao Martins wrote: >> Implement IOMMU MR get_attr() method and use the dma_translation >> property to report the IOMMU_ATTR_DMA_TRANSLATION attribute. >> Additionally add the necessary get_iommu_attr into the PCIIOMMUOps to >> support pci_device_iommu_get_attr(). >> >> The callback in there acts as a IOMMU-specific address space walker >> which will call get_attr in the IOMMUMemoryRegion backing the device to >> fetch the desired attribute. > > This looks like two patches to me and the previous should be merged with > the one introducing vtd_iommu_get_attr(). > The reason was that one is a bit useless without the other. But I can spread into two patches. And I can merge the previous to the one introducing vtd_iommu_get_attr() > Thanks, > > C. > >> Signed-off-by: Avihai Horon <avihaih@nvidia.com> >> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >> --- >> hw/i386/intel_iommu.c | 40 ++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 40 insertions(+) >> >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >> index 1606d1b952d0..ed2a46e008df 100644 >> --- a/hw/i386/intel_iommu.c >> +++ b/hw/i386/intel_iommu.c >> @@ -3861,6 +3861,29 @@ static void vtd_iommu_replay(IOMMUMemoryRegion >> *iommu_mr, IOMMUNotifier *n) >> return; >> } >> +static int vtd_iommu_get_attr(IOMMUMemoryRegion *iommu_mr, >> + enum IOMMUMemoryRegionAttr attr, void *data) >> +{ >> + VTDAddressSpace *vtd_as = container_of(iommu_mr, VTDAddressSpace, iommu); >> + IntelIOMMUState *s = vtd_as->iommu_state; >> + int ret = 0; >> + >> + switch (attr) { >> + case IOMMU_ATTR_DMA_TRANSLATION: >> + { >> + bool *enabled = (bool *)(uintptr_t) data; >> + >> + *enabled = s->dma_translation; >> + break; >> + } >> + default: >> + ret = -EINVAL; >> + break; >> + } >> + >> + return ret; >> +} >> + >> /* Do the initialization. It will also be called when reset, so pay >> * attention when adding new initialization stuff. >> */ >> @@ -4032,8 +4055,24 @@ static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, >> void *opaque, int devfn) >> return &vtd_as->as; >> } >> +static int vtd_get_iommu_attr(PCIBus *bus, void *opaque, int32_t devfn, >> + enum IOMMUMemoryRegionAttr attr, void *data) >> +{ >> + IntelIOMMUState *s = opaque; >> + VTDAddressSpace *vtd_as; >> + >> + assert(0 <= devfn && devfn < PCI_DEVFN_MAX); >> + >> + vtd_as = vtd_find_add_as(s, bus, devfn, PCI_NO_PASID); >> + if (!vtd_as) >> + return -EINVAL; >> + >> + return memory_region_iommu_get_attr(&vtd_as->iommu, attr, data); >> +} >> + >> static PCIIOMMUOps vtd_iommu_ops = { >> .get_address_space = vtd_host_dma_iommu, >> + .get_iommu_attr = vtd_get_iommu_attr, >> }; >> static bool vtd_decide_config(IntelIOMMUState *s, Error **errp) >> @@ -4197,6 +4236,7 @@ static void >> vtd_iommu_memory_region_class_init(ObjectClass *klass, >> imrc->translate = vtd_iommu_translate; >> imrc->notify_flag_changed = vtd_iommu_notify_flag_changed; >> imrc->replay = vtd_iommu_replay; >> + imrc->get_attr = vtd_iommu_get_attr; >> } >> static const TypeInfo vtd_iommu_memory_region_info = { >
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 1606d1b952d0..ed2a46e008df 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -3861,6 +3861,29 @@ static void vtd_iommu_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n) return; } +static int vtd_iommu_get_attr(IOMMUMemoryRegion *iommu_mr, + enum IOMMUMemoryRegionAttr attr, void *data) +{ + VTDAddressSpace *vtd_as = container_of(iommu_mr, VTDAddressSpace, iommu); + IntelIOMMUState *s = vtd_as->iommu_state; + int ret = 0; + + switch (attr) { + case IOMMU_ATTR_DMA_TRANSLATION: + { + bool *enabled = (bool *)(uintptr_t) data; + + *enabled = s->dma_translation; + break; + } + default: + ret = -EINVAL; + break; + } + + return ret; +} + /* Do the initialization. It will also be called when reset, so pay * attention when adding new initialization stuff. */ @@ -4032,8 +4055,24 @@ static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) return &vtd_as->as; } +static int vtd_get_iommu_attr(PCIBus *bus, void *opaque, int32_t devfn, + enum IOMMUMemoryRegionAttr attr, void *data) +{ + IntelIOMMUState *s = opaque; + VTDAddressSpace *vtd_as; + + assert(0 <= devfn && devfn < PCI_DEVFN_MAX); + + vtd_as = vtd_find_add_as(s, bus, devfn, PCI_NO_PASID); + if (!vtd_as) + return -EINVAL; + + return memory_region_iommu_get_attr(&vtd_as->iommu, attr, data); +} + static PCIIOMMUOps vtd_iommu_ops = { .get_address_space = vtd_host_dma_iommu, + .get_iommu_attr = vtd_get_iommu_attr, }; static bool vtd_decide_config(IntelIOMMUState *s, Error **errp) @@ -4197,6 +4236,7 @@ static void vtd_iommu_memory_region_class_init(ObjectClass *klass, imrc->translate = vtd_iommu_translate; imrc->notify_flag_changed = vtd_iommu_notify_flag_changed; imrc->replay = vtd_iommu_replay; + imrc->get_attr = vtd_iommu_get_attr; } static const TypeInfo vtd_iommu_memory_region_info = {