RE: [PATCH rfcv2 10/18] hw/pci: Introduce pci_device_set/unset_iommu_device()

2024-02-25 Thread Duan, Zhenzhong


>-Original Message-
>From: Eric Auger 
>Subject: Re: [PATCH rfcv2 10/18] hw/pci: Introduce
>pci_device_set/unset_iommu_device()
>
>Hi Zhenzhong,
>
>On 2/1/24 08:28, Zhenzhong Duan wrote:
>> From: Yi Liu 
>>
>> This adds pci_device_set/unset_iommu_device() to set/unset
>> HostIOMMUDevice for a given PCIe device. Caller of set
>> should fail if set operation fails.
>>
>> Extract out pci_device_get_iommu_bus_devfn() to facilitate
>> implementation of pci_device_set/unset_iommu_device().
>>
>> Signed-off-by: Yi Liu 
>> Signed-off-by: Yi Sun 
>> Signed-off-by: Nicolin Chen 
>> Signed-off-by: Zhenzhong Duan 
>> ---
>>  include/hw/pci/pci.h | 38 ++-
>>  hw/pci/pci.c | 62
>+---
>>  2 files changed, 96 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>> index fa6313aabc..5b471fd380 100644
>> --- a/include/hw/pci/pci.h
>> +++ b/include/hw/pci/pci.h
>> @@ -3,6 +3,7 @@
>>
>>  #include "exec/memory.h"
>>  #include "sysemu/dma.h"
>> +#include "sysemu/host_iommu_device.h"
>>
>>  /* PCI includes legacy ISA access.  */
>>  #include "hw/isa/isa.h"
>> @@ -384,10 +385,45 @@ typedef struct PCIIOMMUOps {
>>   *
>>   * @devfn: device and function number
>>   */
>> -   AddressSpace * (*get_address_space)(PCIBus *bus, void *opaque, int
>devfn);
>> +AddressSpace * (*get_address_space)(PCIBus *bus, void *opaque, int
>devfn);
>> +/**
>> + * @set_iommu_device: set iommufd device for a PCI device to
>vIOMMU
>attach a HostIOMMUDevice to a vIOMMU

Will do.

>> + *
>> + * Optional callback, if not implemented in vIOMMU, then vIOMMU
>can't
>> + * utilize iommufd specific features.
>looks too iommufd specific. Then vIOMMU can't retrieve host information
>from the associated HostIOMMUDevice

Will do.

>> + *
>> + * Return true if iommufd device is accepted, or else return false with
>s/accepted/attached

Will do.

>> + * errp set.
>> + *
>> + * @bus: the #PCIBus of the PCI device.
>> + *
>> + * @opaque: the data passed to pci_setup_iommu().
>> + *
>> + * @devfn: device and function number of the PCI device.
>> + *
>> + * @dev: the data structure representing host assigned device.
>> + *
>> + */
>> +int (*set_iommu_device)(PCIBus *bus, void *opaque, int devfn,
>> +HostIOMMUDevice *dev, Error **errp);
>> +/**
>> + * @unset_iommu_device: unset iommufd device for a PCI device from
>vIOMMU
>same suggestion here

Will do.

Thanks
Zhenzhong


Re: [PATCH rfcv2 10/18] hw/pci: Introduce pci_device_set/unset_iommu_device()

2024-02-19 Thread Eric Auger
Hi Zhenzhong,

On 2/1/24 08:28, Zhenzhong Duan wrote:
> From: Yi Liu 
>
> This adds pci_device_set/unset_iommu_device() to set/unset
> HostIOMMUDevice for a given PCIe device. Caller of set
> should fail if set operation fails.
>
> Extract out pci_device_get_iommu_bus_devfn() to facilitate
> implementation of pci_device_set/unset_iommu_device().
>
> Signed-off-by: Yi Liu 
> Signed-off-by: Yi Sun 
> Signed-off-by: Nicolin Chen 
> Signed-off-by: Zhenzhong Duan 
> ---
>  include/hw/pci/pci.h | 38 ++-
>  hw/pci/pci.c | 62 +---
>  2 files changed, 96 insertions(+), 4 deletions(-)
>
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index fa6313aabc..5b471fd380 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -3,6 +3,7 @@
>  
>  #include "exec/memory.h"
>  #include "sysemu/dma.h"
> +#include "sysemu/host_iommu_device.h"
>  
>  /* PCI includes legacy ISA access.  */
>  #include "hw/isa/isa.h"
> @@ -384,10 +385,45 @@ typedef struct PCIIOMMUOps {
>   *
>   * @devfn: device and function number
>   */
> -   AddressSpace * (*get_address_space)(PCIBus *bus, void *opaque, int devfn);
> +AddressSpace * (*get_address_space)(PCIBus *bus, void *opaque, int 
> devfn);
> +/**
> + * @set_iommu_device: set iommufd device for a PCI device to vIOMMU
attach a HostIOMMUDevice to a vIOMMU
> + *
> + * Optional callback, if not implemented in vIOMMU, then vIOMMU can't
> + * utilize iommufd specific features.
looks too iommufd specific. Then vIOMMU can't retrieve host information
from the associated HostIOMMUDevice
> + *
> + * Return true if iommufd device is accepted, or else return false with
s/accepted/attached
> + * errp set.
> + *
> + * @bus: the #PCIBus of the PCI device.
> + *
> + * @opaque: the data passed to pci_setup_iommu().
> + *
> + * @devfn: device and function number of the PCI device.
> + *
> + * @dev: the data structure representing host assigned device.
> + *
> + */
> +int (*set_iommu_device)(PCIBus *bus, void *opaque, int devfn,
> +HostIOMMUDevice *dev, Error **errp);
> +/**
> + * @unset_iommu_device: unset iommufd device for a PCI device from vIOMMU
same suggestion here
> + *
> + * Optional callback.
> + *
> + * @bus: the #PCIBus of the PCI device.
> + *
> + * @opaque: the data passed to pci_setup_iommu().
> + *
> + * @devfn: device and function number of the PCI device.
> + */
> +void (*unset_iommu_device)(PCIBus *bus, void *opaque, int devfn);
>  } PCIIOMMUOps;
>  
>  AddressSpace *pci_device_iommu_address_space(PCIDevice *dev);
> +int pci_device_set_iommu_device(PCIDevice *dev, HostIOMMUDevice *base_dev,
> +Error **errp);
> +void pci_device_unset_iommu_device(PCIDevice *dev);
>  
>  /**
>   * pci_setup_iommu: Initialize specific IOMMU handlers for a PCIBus
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 76080af580..8078307963 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2672,11 +2672,14 @@ static void pci_device_class_base_init(ObjectClass 
> *klass, void *data)
>  }
>  }
>  
> -AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
> +static void pci_device_get_iommu_bus_devfn(PCIDevice *dev,
> +   PCIBus **aliased_bus,
> +   PCIBus **piommu_bus,
> +   int *aliased_devfn)
>  {
>  PCIBus *bus = pci_get_bus(dev);
>  PCIBus *iommu_bus = bus;
> -uint8_t devfn = dev->devfn;
> +int devfn = dev->devfn;
>  
>  while (iommu_bus && !iommu_bus->iommu_ops && iommu_bus->parent_dev) {
>  PCIBus *parent_bus = pci_get_bus(iommu_bus->parent_dev);
> @@ -2717,13 +2720,66 @@ AddressSpace 
> *pci_device_iommu_address_space(PCIDevice *dev)
>  
>  iommu_bus = parent_bus;
>  }
> -if (!pci_bus_bypass_iommu(bus) && iommu_bus->iommu_ops) {
> +
> +assert(0 <= devfn && devfn < PCI_DEVFN_MAX);
> +assert(iommu_bus);
> +
> +if (pci_bus_bypass_iommu(bus) || !iommu_bus->iommu_ops) {
> +iommu_bus = NULL;
> +}
> +
> +*piommu_bus = iommu_bus;
> +
> +if (aliased_bus) {
> +*aliased_bus = bus;
> +}
> +
> +if (aliased_devfn) {
> +*aliased_devfn = devfn;
> +}
> +}
> +
> +AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
> +{
> +PCIBus *bus;
> +PCIBus *iommu_bus;
> +int devfn;
> +
> +pci_device_get_iommu_bus_devfn(dev, &bus, &iommu_bus, &devfn);
> +if (iommu_bus) {
>  return iommu_bus->iommu_ops->get_address_space(bus,
>   iommu_bus->iommu_opaque, devfn);
>  }
>  return &address_space_memory;
>  }
>  
> +int pci_device_set_iommu_device(PCIDevice *dev, HostIOMMUDevice *base_dev,
> +Error **errp)
> +{
> +