Re: [PATCH v12 04/13] virtio-iommu: Add the iommu regions
On Tue, Jan 14, 2020 at 09:37:51AM +0100, Auger Eric wrote: > > In all cases, I see that virtio_iommu_mr() is introduced but not used. > > Would be good to put it into the patch where it's firstly used. > OK fair enough, I will put the helper in the same patch as the user as > you have requested that since the beginning ;-) The resulting patch may > be huge. Just hope nobody will request me to split it back ;-) We can wait for a few more days and see whether someone will have a different answer before you start to squash things. :) In all cases, I think we should get rid of the compiler trick at least. Thanks, -- Peter Xu
Re: [PATCH v12 04/13] virtio-iommu: Add the iommu regions
Hi Peter,
On 1/13/20 9:06 PM, Peter Xu wrote:
> On Thu, Jan 09, 2020 at 03:43:10PM +0100, Eric Auger wrote:
>> +/**
>> + * The bus number is used for lookup when SID based operations occur.
>> + * In that case we lazily populate the IOMMUPciBus array from the bus hash
>> + * table. At the time the IOMMUPciBus is created (iommu_find_add_as), the
>> bus
>> + * numbers may not be always initialized yet.
>> + */
>> +static IOMMUPciBus *iommu_find_iommu_pcibus(VirtIOIOMMU *s, uint8_t bus_num)
>> +{
>> +IOMMUPciBus *iommu_pci_bus = s->iommu_pcibus_by_bus_num[bus_num];
>> +
>> +if (!iommu_pci_bus) {
>> +GHashTableIter iter;
>> +
>> +g_hash_table_iter_init(&iter, s->as_by_busptr);
>> +while (g_hash_table_iter_next(&iter, NULL, (void
>> **)&iommu_pci_bus)) {
>> +if (pci_bus_num(iommu_pci_bus->bus) == bus_num) {
>> +s->iommu_pcibus_by_bus_num[bus_num] = iommu_pci_bus;
>> +return iommu_pci_bus;
>> +}
>> +}
>
> Btw, we may need to:
>
>return NULL;
Yes. By the way Yi's patch "intel_iommu: a fix to
vtd_find_as_from_bus_num()" also applies to SMMU code. I will send a patch.
Thanks
Eric
>
> here.
>
>> +}
>> +return iommu_pci_bus;
>> +}
>
Re: [PATCH v12 04/13] virtio-iommu: Add the iommu regions
Hi Peter, On 1/13/20 8:53 PM, Peter Xu wrote: > On Thu, Jan 09, 2020 at 03:43:10PM +0100, Eric Auger wrote: >> Implement a callback called on PCI bus enumeration that >> initializes for a given device on the bus hierarchy >> an IOMMU memory region. The PCI bus hierarchy is stored >> locally in IOMMUPciBus and IOMMUDevice objects. >> >> At the time of the enumeration, the bus number may not be >> computed yet. >> >> So operations that will need to retrieve the IOMMUdevice >> and its IOMMU memory region from the bus number and devfn, >> once the bus number is garanteed to be frozen, >> use an array of IOMMUPciBus, lazily populated. >> >> virtio_iommu_mr() is the top helper that allows to retrieve >> the IOMMU memory region from the requester ID. >> >> Signed-off-by: Eric Auger >> >> --- >> v11 -> v12: >> - add the iommu_find_iommu_pcibus() mechanics. Without it, >> when attaching t device to a domain we could not check >> the device is effectively protected by this IOMMU > > Sorry I probably lost the context again after read the previous > version... Could you hint me what does this used for? In v11 Jean pointed out that as_by_bus_num was not used in my series. I first planned to remove it and then noticed that it could be useful to test on "attach" whether the RID of the device effectively corresponds to a device protected by the IOMMU and in the negative, return an error. In https://patchwork.kernel.org/patch/11258269/#23067995 This is the same mechanics used in intel_iommu/smmu. > > In all cases, I see that virtio_iommu_mr() is introduced but not used. > Would be good to put it into the patch where it's firstly used. OK fair enough, I will put the helper in the same patch as the user as you have requested that since the beginning ;-) The resulting patch may be huge. Just hope nobody will request me to split it back ;-) Thanks Eric > > Thanks, >
Re: [PATCH v12 04/13] virtio-iommu: Add the iommu regions
On Thu, Jan 09, 2020 at 03:43:10PM +0100, Eric Auger wrote:
> +/**
> + * The bus number is used for lookup when SID based operations occur.
> + * In that case we lazily populate the IOMMUPciBus array from the bus hash
> + * table. At the time the IOMMUPciBus is created (iommu_find_add_as), the bus
> + * numbers may not be always initialized yet.
> + */
> +static IOMMUPciBus *iommu_find_iommu_pcibus(VirtIOIOMMU *s, uint8_t bus_num)
> +{
> +IOMMUPciBus *iommu_pci_bus = s->iommu_pcibus_by_bus_num[bus_num];
> +
> +if (!iommu_pci_bus) {
> +GHashTableIter iter;
> +
> +g_hash_table_iter_init(&iter, s->as_by_busptr);
> +while (g_hash_table_iter_next(&iter, NULL, (void **)&iommu_pci_bus))
> {
> +if (pci_bus_num(iommu_pci_bus->bus) == bus_num) {
> +s->iommu_pcibus_by_bus_num[bus_num] = iommu_pci_bus;
> +return iommu_pci_bus;
> +}
> +}
Btw, we may need to:
return NULL;
here.
> +}
> +return iommu_pci_bus;
> +}
--
Peter Xu
Re: [PATCH v12 04/13] virtio-iommu: Add the iommu regions
On Thu, Jan 09, 2020 at 03:43:10PM +0100, Eric Auger wrote: > Implement a callback called on PCI bus enumeration that > initializes for a given device on the bus hierarchy > an IOMMU memory region. The PCI bus hierarchy is stored > locally in IOMMUPciBus and IOMMUDevice objects. > > At the time of the enumeration, the bus number may not be > computed yet. > > So operations that will need to retrieve the IOMMUdevice > and its IOMMU memory region from the bus number and devfn, > once the bus number is garanteed to be frozen, > use an array of IOMMUPciBus, lazily populated. > > virtio_iommu_mr() is the top helper that allows to retrieve > the IOMMU memory region from the requester ID. > > Signed-off-by: Eric Auger > > --- > v11 -> v12: > - add the iommu_find_iommu_pcibus() mechanics. Without it, > when attaching t device to a domain we could not check > the device is effectively protected by this IOMMU Sorry I probably lost the context again after read the previous version... Could you hint me what does this used for? In all cases, I see that virtio_iommu_mr() is introduced but not used. Would be good to put it into the patch where it's firstly used. Thanks, -- Peter Xu
