Re: [PATCH rfcv1 6/6] intel_iommu: add a framework to check and sync host IOMMU cap/ecap

2024-01-23 Thread Eric Auger 



On 1/19/24 12:55, Duan, Zhenzhong wrote:
>
>> -Original Message-
>> From: Eric Auger 
>> Subject: Re: [PATCH rfcv1 6/6] intel_iommu: add a framework to check and
>> sync host IOMMU cap/ecap
>>
>>
>>
>> On 1/18/24 10:30, Duan, Zhenzhong wrote:
>>> Hi Eric,
>>>
 -Original Message-
 From: Eric Auger 
 Subject: Re: [PATCH rfcv1 6/6] intel_iommu: add a framework to check
>> and
 sync host IOMMU cap/ecap

 Hi Zhenzhong,

 On 1/15/24 11:13, Zhenzhong Duan wrote:
> From: Yi Liu 
>
> Add a framework to check and synchronize host IOMMU cap/ecap with
> vIOMMU cap/ecap.
>
> Currently only stage-2 translation is supported which is backed by
> shadow page table on host side. So we don't need exact matching of
> each bit of cap/ecap between vIOMMU and host. However, we can still
> utilize this framework to ensure compatibility of host and vIOMMU's
> address width at least, i.e., vIOMMU's aw_bits <= host aw_bits,
> which is missed before.
>
> When stage-1 translation is supported in future, a.k.a. scalable
> modern mode, we need to ensure compatibility of each bits. Some
> bits are user controllable, they should be checked with host side
> to ensure compatibility. Other bits are not, they should be synced
> into vIOMMU cap/ecap for compatibility.
>
> The sequence will be:
>
> vtd_cap_init() initializes iommu->cap/ecap.  vtd_cap_init()
> iommu->host_cap/ecap is initialized as iommu->cap/ecap.   vtd_init()
> iommu->host_cap/ecap is checked and updated some bits with host
 cap/ecap.  vtd_sync_hw_info()
> iommu->cap/ecap is finalized as iommu->host_cap/ecap.  
 vtd_machine_done_hook()
> iommu->host_cap/ecap is a temporary storage to hold intermediate
>> value
> when synthesize host cap/ecap and vIOMMU's initial configured
>> cap/ecap.
> Signed-off-by: Yi Liu 
> Signed-off-by: Yi Sun 
> Signed-off-by: Zhenzhong Duan 
> ---
>  include/hw/i386/intel_iommu.h |  4 ++
>  hw/i386/intel_iommu.c | 78
 +++
>  2 files changed, 75 insertions(+), 7 deletions(-)
>
> diff --git a/include/hw/i386/intel_iommu.h
 b/include/hw/i386/intel_iommu.h
> index c65fdde56f..b8abbcce12 100644
> --- a/include/hw/i386/intel_iommu.h
> +++ b/include/hw/i386/intel_iommu.h
> @@ -292,6 +292,9 @@ struct IntelIOMMUState {
>  uint64_t cap;   /* The value of capability reg */
>  uint64_t ecap;  /* The value of extended capability 
> reg */
>
> +uint64_t host_cap;  /* The value of host capability reg 
> */
> +uint64_t host_ecap; /* The value of host ext-capability 
> reg */
> +
>  uint32_t context_cache_gen; /* Should be in [1,MAX] */
>  GHashTable *iotlb;  /* IOTLB */
>
> @@ -314,6 +317,7 @@ struct IntelIOMMUState {
>  bool dma_translation;   /* Whether DMA translation supported
>> */
>  bool pasid; /* Whether to support PASID */
>
> +bool cap_finalized; /* Whether VTD capability finalized 
> */
>  /*
>   * Protects IOMMU states in general.  Currently it protects the
>   * per-IOMMU IOTLB cache, and context entry cache in
 VTDAddressSpace.
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 4c1d058ebd..be03fcbf52 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3819,6 +3819,47 @@ VTDAddressSpace
 *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>  return vtd_dev_as;
>  }
>
> +static bool vtd_sync_hw_info(IntelIOMMUState *s, struct
 iommu_hw_info_vtd *vtd,
> + Error **errp)
> +{
> +uint64_t addr_width;
> +
> +addr_width = (vtd->cap_reg >> 16) & 0x3fULL;
> +if (s->aw_bits > addr_width) {
> +error_setg(errp, "User aw-bits: %u > host address width: %lu",
> +   s->aw_bits, addr_width);
> +return false;
> +}
> +
> +/* TODO: check and sync host cap/ecap into vIOMMU cap/ecap */
> +
> +return true;
> +}
> +
> +/*
> + * virtual VT-d which wants nested needs to check the host IOMMU
> + * nesting cap info behind the assigned devices. Thus that vIOMMU
> + * could bind guest page table to host.
> + */
> +static bool vtd_check_idev(IntelIOMMUState *s, IOMMUFDDevice
>> *idev,
> +   Error **errp)
> +{
> +struct iommu_hw_info_vtd vtd;
> +enum iommu_hw_info_type type =
 IOMMU_HW_INFO_TYPE_INTEL_VTD;
> +
> +if (iommufd_device_get_info(idev, , sizeof(vtd), )) {
> +error_setg(errp, "Failed to get IOMMU capability!!!");
> +

RE: [PATCH rfcv1 6/6] intel_iommu: add a framework to check and sync host IOMMU cap/ecap

2024-01-23 Thread Duan, Zhenzhong 


>-Original Message-
>From: Cédric Le Goater 
>Subject: Re: [PATCH rfcv1 6/6] intel_iommu: add a framework to check and
>sync host IOMMU cap/ecap
>
>On 1/15/24 11:13, Zhenzhong Duan wrote:
>> From: Yi Liu 
>>
>> Add a framework to check and synchronize host IOMMU cap/ecap with
>> vIOMMU cap/ecap.
>>
>> Currently only stage-2 translation is supported which is backed by
>> shadow page table on host side. So we don't need exact matching of
>> each bit of cap/ecap between vIOMMU and host. However, we can still
>> utilize this framework to ensure compatibility of host and vIOMMU's
>> address width at least, i.e., vIOMMU's aw_bits <= host aw_bits,
>> which is missed before.
>>
>> When stage-1 translation is supported in future, a.k.a. scalable
>> modern mode, we need to ensure compatibility of each bits. Some
>> bits are user controllable, they should be checked with host side
>> to ensure compatibility. Other bits are not, they should be synced
>> into vIOMMU cap/ecap for compatibility.
>>
>> The sequence will be:
>>
>> vtd_cap_init() initializes iommu->cap/ecap.  vtd_cap_init()
>> iommu->host_cap/ecap is initialized as iommu->cap/ecap.   vtd_init()
>> iommu->host_cap/ecap is checked and updated some bits with host
>cap/ecap.  vtd_sync_hw_info()
>> iommu->cap/ecap is finalized as iommu->host_cap/ecap.  
>vtd_machine_done_hook()
>>
>> iommu->host_cap/ecap is a temporary storage to hold intermediate value
>> when synthesize host cap/ecap and vIOMMU's initial configured cap/ecap.
>
>
>The above "sequence" paragraph is not very clear. The patch may need to
>be split further.

OK, will do.

>
>
>
>> Signed-off-by: Yi Liu 
>> Signed-off-by: Yi Sun 
>> Signed-off-by: Zhenzhong Duan 
>> ---
>>   include/hw/i386/intel_iommu.h |  4 ++
>>   hw/i386/intel_iommu.c | 78
>+++
>>   2 files changed, 75 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/hw/i386/intel_iommu.h
>b/include/hw/i386/intel_iommu.h
>> index c65fdde56f..b8abbcce12 100644
>> --- a/include/hw/i386/intel_iommu.h
>> +++ b/include/hw/i386/intel_iommu.h
>> @@ -292,6 +292,9 @@ struct IntelIOMMUState {
>>   uint64_t cap;   /* The value of capability reg */
>>   uint64_t ecap;  /* The value of extended capability 
>> reg */
>>
>> +uint64_t host_cap;  /* The value of host capability reg */
>> +uint64_t host_ecap; /* The value of host ext-capability reg 
>> */
>> +
>>   uint32_t context_cache_gen; /* Should be in [1,MAX] */
>>   GHashTable *iotlb;  /* IOTLB */
>>
>> @@ -314,6 +317,7 @@ struct IntelIOMMUState {
>>   bool dma_translation;   /* Whether DMA translation supported */
>>   bool pasid; /* Whether to support PASID */
>>
>> +bool cap_finalized; /* Whether VTD capability finalized */
>>   /*
>>* Protects IOMMU states in general.  Currently it protects the
>>* per-IOMMU IOTLB cache, and context entry cache in
>VTDAddressSpace.
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index 4c1d058ebd..be03fcbf52 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -3819,6 +3819,47 @@ VTDAddressSpace
>*vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>>   return vtd_dev_as;
>>   }
>>
>> +static bool vtd_sync_hw_info(IntelIOMMUState *s, struct
>iommu_hw_info_vtd *vtd,
>> + Error **errp)
>> +{
>> +uint64_t addr_width;
>> +
>> +addr_width = (vtd->cap_reg >> 16) & 0x3fULL;
>
>Virek uses the same kind of macro in :
>
>   https://lore.kernel.org/qemu-devel/20240118192049.1796763-1-
>vivek.kasire...@intel.com/
>
>What about the + 1 ? Looks like it's missing here, according to 11.4.2
>Capability Register.
>
>Could we introduce a common macro in intel_iommu_internal.h ?

Sure.

>
>
>> +if (s->aw_bits > addr_width) {
>> +error_setg(errp, "User aw-bits: %u > host address width: %lu",
>
>I think %lu should be PRId64. This is a general comment. You should avoid
>%llx, %lx, etc. in the code.

Got it.

>
>> +   s->aw_bits, addr_width);
>> +return false;
>> +}
>> +
>> +/* TODO: check and sync host cap/ecap into vIOMMU cap/ecap */
>> +
>> +return true;
>> +}
>> +
>> +/*
>> + * virtual VT-d which wants nested needs to check the host IOMMU
>> + * nesting cap info behind the assigned devices. Thus that vIOMMU
>> + * could bind guest page table to host.
>> + */
>> +static bool vtd_check_idev(IntelIOMMUState *s, IOMMUFDDevice *idev,
>> +   Error **errp)
>> +{
>> +struct iommu_hw_info_vtd vtd;
>> +enum iommu_hw_info_type type =
>IOMMU_HW_INFO_TYPE_INTEL_VTD;
>> +
>> +if (iommufd_device_get_info(idev, , sizeof(vtd), )) {
>> +error_setg(errp, "Failed to get IOMMU capability!!!");
>> +return false;
>> +}
>> +
>> +if (type != IOMMU_HW_INFO_TYPE_INTEL_VTD) {
>> +

Re: [PATCH rfcv1 6/6] intel_iommu: add a framework to check and sync host IOMMU cap/ecap

2024-01-23 Thread Cédric Le Goater 

On 1/15/24 11:13, Zhenzhong Duan wrote:

From: Yi Liu 

Add a framework to check and synchronize host IOMMU cap/ecap with
vIOMMU cap/ecap.

Currently only stage-2 translation is supported which is backed by
shadow page table on host side. So we don't need exact matching of
each bit of cap/ecap between vIOMMU and host. However, we can still
utilize this framework to ensure compatibility of host and vIOMMU's
address width at least, i.e., vIOMMU's aw_bits <= host aw_bits,
which is missed before.

When stage-1 translation is supported in future, a.k.a. scalable
modern mode, we need to ensure compatibility of each bits. Some
bits are user controllable, they should be checked with host side
to ensure compatibility. Other bits are not, they should be synced
into vIOMMU cap/ecap for compatibility.

The sequence will be:

vtd_cap_init() initializes iommu->cap/ecap.  vtd_cap_init()
iommu->host_cap/ecap is initialized as iommu->cap/ecap.   vtd_init()
iommu->host_cap/ecap is checked and updated some bits with host cap/ecap.  
vtd_sync_hw_info()
iommu->cap/ecap is finalized as iommu->host_cap/ecap.   
vtd_machine_done_hook()

iommu->host_cap/ecap is a temporary storage to hold intermediate value
when synthesize host cap/ecap and vIOMMU's initial configured cap/ecap.



The above "sequence" paragraph is not very clear. The patch may need to
be split further.




Signed-off-by: Yi Liu 
Signed-off-by: Yi Sun 
Signed-off-by: Zhenzhong Duan 
---
  include/hw/i386/intel_iommu.h |  4 ++
  hw/i386/intel_iommu.c | 78 +++
  2 files changed, 75 insertions(+), 7 deletions(-)

diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index c65fdde56f..b8abbcce12 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -292,6 +292,9 @@ struct IntelIOMMUState {
  uint64_t cap;   /* The value of capability reg */
  uint64_t ecap;  /* The value of extended capability reg */
  
+uint64_t host_cap;  /* The value of host capability reg */

+uint64_t host_ecap; /* The value of host ext-capability reg */
+
  uint32_t context_cache_gen; /* Should be in [1,MAX] */
  GHashTable *iotlb;  /* IOTLB */
  
@@ -314,6 +317,7 @@ struct IntelIOMMUState {

  bool dma_translation;   /* Whether DMA translation supported */
  bool pasid; /* Whether to support PASID */
  
+bool cap_finalized; /* Whether VTD capability finalized */

  /*
   * Protects IOMMU states in general.  Currently it protects the
   * per-IOMMU IOTLB cache, and context entry cache in VTDAddressSpace.
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 4c1d058ebd..be03fcbf52 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3819,6 +3819,47 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, 
PCIBus *bus,
  return vtd_dev_as;
  }
  
+static bool vtd_sync_hw_info(IntelIOMMUState *s, struct iommu_hw_info_vtd *vtd,

+ Error **errp)
+{
+uint64_t addr_width;
+
+addr_width = (vtd->cap_reg >> 16) & 0x3fULL;


Virek uses the same kind of macro in :

  
https://lore.kernel.org/qemu-devel/20240118192049.1796763-1-vivek.kasire...@intel.com/

What about the + 1 ? Looks like it's missing here, according to 11.4.2
Capability Register.

Could we introduce a common macro in intel_iommu_internal.h ?



+if (s->aw_bits > addr_width) {
+error_setg(errp, "User aw-bits: %u > host address width: %lu",


I think %lu should be PRId64. This is a general comment. You should avoid
%llx, %lx, etc. in the code.


+   s->aw_bits, addr_width);
+return false;
+}
+
+/* TODO: check and sync host cap/ecap into vIOMMU cap/ecap */
+
+return true;
+}
+
+/*
+ * virtual VT-d which wants nested needs to check the host IOMMU
+ * nesting cap info behind the assigned devices. Thus that vIOMMU
+ * could bind guest page table to host.
+ */
+static bool vtd_check_idev(IntelIOMMUState *s, IOMMUFDDevice *idev,
+   Error **errp)
+{
+struct iommu_hw_info_vtd vtd;
+enum iommu_hw_info_type type = IOMMU_HW_INFO_TYPE_INTEL_VTD;
+
+if (iommufd_device_get_info(idev, , sizeof(vtd), )) {
+error_setg(errp, "Failed to get IOMMU capability!!!");
+return false;
+}
+
+if (type != IOMMU_HW_INFO_TYPE_INTEL_VTD) {
+error_setg(errp, "IOMMU hardware is not compatible!!!");
+return false;
+}
+
+return vtd_sync_hw_info(s, , errp);
+}
+
  static int vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int32_t devfn,
  IOMMUFDDevice *idev, Error **errp)
  {
@@ -3837,6 +3878,10 @@ static int vtd_dev_set_iommu_device(PCIBus *bus, void 
*opaque, int32_t devfn,
  return 0;
  }
  
+if (!vtd_check_idev(s, idev, errp)) {

+return -1;
+}
+

RE: [PATCH rfcv1 6/6] intel_iommu: add a framework to check and sync host IOMMU cap/ecap

2024-01-19 Thread Duan, Zhenzhong 


>-Original Message-
>From: Eric Auger 
>Subject: Re: [PATCH rfcv1 6/6] intel_iommu: add a framework to check and
>sync host IOMMU cap/ecap
>
>
>
>On 1/18/24 10:30, Duan, Zhenzhong wrote:
>> Hi Eric,
>>
>>> -Original Message-
>>> From: Eric Auger 
>>> Subject: Re: [PATCH rfcv1 6/6] intel_iommu: add a framework to check
>and
>>> sync host IOMMU cap/ecap
>>>
>>> Hi Zhenzhong,
>>>
>>> On 1/15/24 11:13, Zhenzhong Duan wrote:
 From: Yi Liu 

 Add a framework to check and synchronize host IOMMU cap/ecap with
 vIOMMU cap/ecap.

 Currently only stage-2 translation is supported which is backed by
 shadow page table on host side. So we don't need exact matching of
 each bit of cap/ecap between vIOMMU and host. However, we can still
 utilize this framework to ensure compatibility of host and vIOMMU's
 address width at least, i.e., vIOMMU's aw_bits <= host aw_bits,
 which is missed before.

 When stage-1 translation is supported in future, a.k.a. scalable
 modern mode, we need to ensure compatibility of each bits. Some
 bits are user controllable, they should be checked with host side
 to ensure compatibility. Other bits are not, they should be synced
 into vIOMMU cap/ecap for compatibility.

 The sequence will be:

 vtd_cap_init() initializes iommu->cap/ecap.  vtd_cap_init()
 iommu->host_cap/ecap is initialized as iommu->cap/ecap.   vtd_init()
 iommu->host_cap/ecap is checked and updated some bits with host
>>> cap/ecap.  vtd_sync_hw_info()
 iommu->cap/ecap is finalized as iommu->host_cap/ecap.  
>>> vtd_machine_done_hook()
 iommu->host_cap/ecap is a temporary storage to hold intermediate
>value
 when synthesize host cap/ecap and vIOMMU's initial configured
>cap/ecap.

 Signed-off-by: Yi Liu 
 Signed-off-by: Yi Sun 
 Signed-off-by: Zhenzhong Duan 
 ---
  include/hw/i386/intel_iommu.h |  4 ++
  hw/i386/intel_iommu.c | 78
>>> +++
  2 files changed, 75 insertions(+), 7 deletions(-)

 diff --git a/include/hw/i386/intel_iommu.h
>>> b/include/hw/i386/intel_iommu.h
 index c65fdde56f..b8abbcce12 100644
 --- a/include/hw/i386/intel_iommu.h
 +++ b/include/hw/i386/intel_iommu.h
 @@ -292,6 +292,9 @@ struct IntelIOMMUState {
  uint64_t cap;   /* The value of capability reg */
  uint64_t ecap;  /* The value of extended capability 
 reg */

 +uint64_t host_cap;  /* The value of host capability reg */
 +uint64_t host_ecap; /* The value of host ext-capability 
 reg */
 +
  uint32_t context_cache_gen; /* Should be in [1,MAX] */
  GHashTable *iotlb;  /* IOTLB */

 @@ -314,6 +317,7 @@ struct IntelIOMMUState {
  bool dma_translation;   /* Whether DMA translation supported
>*/
  bool pasid; /* Whether to support PASID */

 +bool cap_finalized; /* Whether VTD capability finalized */
  /*
   * Protects IOMMU states in general.  Currently it protects the
   * per-IOMMU IOTLB cache, and context entry cache in
>>> VTDAddressSpace.
 diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
 index 4c1d058ebd..be03fcbf52 100644
 --- a/hw/i386/intel_iommu.c
 +++ b/hw/i386/intel_iommu.c
 @@ -3819,6 +3819,47 @@ VTDAddressSpace
>>> *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
  return vtd_dev_as;
  }

 +static bool vtd_sync_hw_info(IntelIOMMUState *s, struct
>>> iommu_hw_info_vtd *vtd,
 + Error **errp)
 +{
 +uint64_t addr_width;
 +
 +addr_width = (vtd->cap_reg >> 16) & 0x3fULL;
 +if (s->aw_bits > addr_width) {
 +error_setg(errp, "User aw-bits: %u > host address width: %lu",
 +   s->aw_bits, addr_width);
 +return false;
 +}
 +
 +/* TODO: check and sync host cap/ecap into vIOMMU cap/ecap */
 +
 +return true;
 +}
 +
 +/*
 + * virtual VT-d which wants nested needs to check the host IOMMU
 + * nesting cap info behind the assigned devices. Thus that vIOMMU
 + * could bind guest page table to host.
 + */
 +static bool vtd_check_idev(IntelIOMMUState *s, IOMMUFDDevice
>*idev,
 +   Error **errp)
 +{
 +struct iommu_hw_info_vtd vtd;
 +enum iommu_hw_info_type type =
>>> IOMMU_HW_INFO_TYPE_INTEL_VTD;
 +
 +if (iommufd_device_get_info(idev, , sizeof(vtd), )) {
 +error_setg(errp, "Failed to get IOMMU capability!!!");
 +return false;
 +}
 +
 +if (type != IOMMU_HW_INFO_TYPE_INTEL_VTD) {
 +error_setg(errp, "IOMMU hardware is not compatible!!!");
 +return false;
 +

Re: [PATCH rfcv1 6/6] intel_iommu: add a framework to check and sync host IOMMU cap/ecap

2024-01-18 Thread Eric Auger 



On 1/18/24 10:30, Duan, Zhenzhong wrote:
> Hi Eric,
>
>> -Original Message-
>> From: Eric Auger 
>> Subject: Re: [PATCH rfcv1 6/6] intel_iommu: add a framework to check and
>> sync host IOMMU cap/ecap
>>
>> Hi Zhenzhong,
>>
>> On 1/15/24 11:13, Zhenzhong Duan wrote:
>>> From: Yi Liu 
>>>
>>> Add a framework to check and synchronize host IOMMU cap/ecap with
>>> vIOMMU cap/ecap.
>>>
>>> Currently only stage-2 translation is supported which is backed by
>>> shadow page table on host side. So we don't need exact matching of
>>> each bit of cap/ecap between vIOMMU and host. However, we can still
>>> utilize this framework to ensure compatibility of host and vIOMMU's
>>> address width at least, i.e., vIOMMU's aw_bits <= host aw_bits,
>>> which is missed before.
>>>
>>> When stage-1 translation is supported in future, a.k.a. scalable
>>> modern mode, we need to ensure compatibility of each bits. Some
>>> bits are user controllable, they should be checked with host side
>>> to ensure compatibility. Other bits are not, they should be synced
>>> into vIOMMU cap/ecap for compatibility.
>>>
>>> The sequence will be:
>>>
>>> vtd_cap_init() initializes iommu->cap/ecap.  vtd_cap_init()
>>> iommu->host_cap/ecap is initialized as iommu->cap/ecap.   vtd_init()
>>> iommu->host_cap/ecap is checked and updated some bits with host
>> cap/ecap.  vtd_sync_hw_info()
>>> iommu->cap/ecap is finalized as iommu->host_cap/ecap.  
>> vtd_machine_done_hook()
>>> iommu->host_cap/ecap is a temporary storage to hold intermediate value
>>> when synthesize host cap/ecap and vIOMMU's initial configured cap/ecap.
>>>
>>> Signed-off-by: Yi Liu 
>>> Signed-off-by: Yi Sun 
>>> Signed-off-by: Zhenzhong Duan 
>>> ---
>>>  include/hw/i386/intel_iommu.h |  4 ++
>>>  hw/i386/intel_iommu.c | 78
>> +++
>>>  2 files changed, 75 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/include/hw/i386/intel_iommu.h
>> b/include/hw/i386/intel_iommu.h
>>> index c65fdde56f..b8abbcce12 100644
>>> --- a/include/hw/i386/intel_iommu.h
>>> +++ b/include/hw/i386/intel_iommu.h
>>> @@ -292,6 +292,9 @@ struct IntelIOMMUState {
>>>  uint64_t cap;   /* The value of capability reg */
>>>  uint64_t ecap;  /* The value of extended capability 
>>> reg */
>>>
>>> +uint64_t host_cap;  /* The value of host capability reg */
>>> +uint64_t host_ecap; /* The value of host ext-capability 
>>> reg */
>>> +
>>>  uint32_t context_cache_gen; /* Should be in [1,MAX] */
>>>  GHashTable *iotlb;  /* IOTLB */
>>>
>>> @@ -314,6 +317,7 @@ struct IntelIOMMUState {
>>>  bool dma_translation;   /* Whether DMA translation supported */
>>>  bool pasid; /* Whether to support PASID */
>>>
>>> +bool cap_finalized; /* Whether VTD capability finalized */
>>>  /*
>>>   * Protects IOMMU states in general.  Currently it protects the
>>>   * per-IOMMU IOTLB cache, and context entry cache in
>> VTDAddressSpace.
>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>> index 4c1d058ebd..be03fcbf52 100644
>>> --- a/hw/i386/intel_iommu.c
>>> +++ b/hw/i386/intel_iommu.c
>>> @@ -3819,6 +3819,47 @@ VTDAddressSpace
>> *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>>>  return vtd_dev_as;
>>>  }
>>>
>>> +static bool vtd_sync_hw_info(IntelIOMMUState *s, struct
>> iommu_hw_info_vtd *vtd,
>>> + Error **errp)
>>> +{
>>> +uint64_t addr_width;
>>> +
>>> +addr_width = (vtd->cap_reg >> 16) & 0x3fULL;
>>> +if (s->aw_bits > addr_width) {
>>> +error_setg(errp, "User aw-bits: %u > host address width: %lu",
>>> +   s->aw_bits, addr_width);
>>> +return false;
>>> +}
>>> +
>>> +/* TODO: check and sync host cap/ecap into vIOMMU cap/ecap */
>>> +
>>> +return true;
>>> +}
>>> +
>>> +/*
>>> + * virtual VT-d which wants nested needs to check the host IOMMU
>>> + * nesting cap info behind the assigned devices. Thus that vIOMMU
>>> + * could bind guest page table to host.
>>> + */
>>> +static bool vtd_check_idev(IntelIOMMUState *s, IOMMUFDDevice *idev,
>>> +   Error **errp)
>>> +{
>>> +struct iommu_hw_info_vtd vtd;
>>> +enum iommu_hw_info_type type =
>> IOMMU_HW_INFO_TYPE_INTEL_VTD;
>>> +
>>> +if (iommufd_device_get_info(idev, , sizeof(vtd), )) {
>>> +error_setg(errp, "Failed to get IOMMU capability!!!");
>>> +return false;
>>> +}
>>> +
>>> +if (type != IOMMU_HW_INFO_TYPE_INTEL_VTD) {
>>> +error_setg(errp, "IOMMU hardware is not compatible!!!");
>>> +return false;
>>> +}
>>> +
>>> +return vtd_sync_hw_info(s, , errp);
>>> +}
>>> +
>>>  static int vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int32_t
>> devfn,
>>>  IOMMUFDDevice *idev, Error **errp)
>>>  {
>>> @@ -3837,6 +3878,10 @@ static int

RE: [PATCH rfcv1 6/6] intel_iommu: add a framework to check and sync host IOMMU cap/ecap

2024-01-18 Thread Duan, Zhenzhong 
Hi Eric,

>-Original Message-
>From: Eric Auger 
>Subject: Re: [PATCH rfcv1 6/6] intel_iommu: add a framework to check and
>sync host IOMMU cap/ecap
>
>Hi Zhenzhong,
>
>On 1/15/24 11:13, Zhenzhong Duan wrote:
>> From: Yi Liu 
>>
>> Add a framework to check and synchronize host IOMMU cap/ecap with
>> vIOMMU cap/ecap.
>>
>> Currently only stage-2 translation is supported which is backed by
>> shadow page table on host side. So we don't need exact matching of
>> each bit of cap/ecap between vIOMMU and host. However, we can still
>> utilize this framework to ensure compatibility of host and vIOMMU's
>> address width at least, i.e., vIOMMU's aw_bits <= host aw_bits,
>> which is missed before.
>>
>> When stage-1 translation is supported in future, a.k.a. scalable
>> modern mode, we need to ensure compatibility of each bits. Some
>> bits are user controllable, they should be checked with host side
>> to ensure compatibility. Other bits are not, they should be synced
>> into vIOMMU cap/ecap for compatibility.
>>
>> The sequence will be:
>>
>> vtd_cap_init() initializes iommu->cap/ecap.  vtd_cap_init()
>> iommu->host_cap/ecap is initialized as iommu->cap/ecap.   vtd_init()
>> iommu->host_cap/ecap is checked and updated some bits with host
>cap/ecap.  vtd_sync_hw_info()
>> iommu->cap/ecap is finalized as iommu->host_cap/ecap.  
>vtd_machine_done_hook()
>>
>> iommu->host_cap/ecap is a temporary storage to hold intermediate value
>> when synthesize host cap/ecap and vIOMMU's initial configured cap/ecap.
>>
>> Signed-off-by: Yi Liu 
>> Signed-off-by: Yi Sun 
>> Signed-off-by: Zhenzhong Duan 
>> ---
>>  include/hw/i386/intel_iommu.h |  4 ++
>>  hw/i386/intel_iommu.c | 78
>+++
>>  2 files changed, 75 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/hw/i386/intel_iommu.h
>b/include/hw/i386/intel_iommu.h
>> index c65fdde56f..b8abbcce12 100644
>> --- a/include/hw/i386/intel_iommu.h
>> +++ b/include/hw/i386/intel_iommu.h
>> @@ -292,6 +292,9 @@ struct IntelIOMMUState {
>>  uint64_t cap;   /* The value of capability reg */
>>  uint64_t ecap;  /* The value of extended capability reg 
>> */
>>
>> +uint64_t host_cap;  /* The value of host capability reg */
>> +uint64_t host_ecap; /* The value of host ext-capability reg 
>> */
>> +
>>  uint32_t context_cache_gen; /* Should be in [1,MAX] */
>>  GHashTable *iotlb;  /* IOTLB */
>>
>> @@ -314,6 +317,7 @@ struct IntelIOMMUState {
>>  bool dma_translation;   /* Whether DMA translation supported */
>>  bool pasid; /* Whether to support PASID */
>>
>> +bool cap_finalized; /* Whether VTD capability finalized */
>>  /*
>>   * Protects IOMMU states in general.  Currently it protects the
>>   * per-IOMMU IOTLB cache, and context entry cache in
>VTDAddressSpace.
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index 4c1d058ebd..be03fcbf52 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -3819,6 +3819,47 @@ VTDAddressSpace
>*vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>>  return vtd_dev_as;
>>  }
>>
>> +static bool vtd_sync_hw_info(IntelIOMMUState *s, struct
>iommu_hw_info_vtd *vtd,
>> + Error **errp)
>> +{
>> +uint64_t addr_width;
>> +
>> +addr_width = (vtd->cap_reg >> 16) & 0x3fULL;
>> +if (s->aw_bits > addr_width) {
>> +error_setg(errp, "User aw-bits: %u > host address width: %lu",
>> +   s->aw_bits, addr_width);
>> +return false;
>> +}
>> +
>> +/* TODO: check and sync host cap/ecap into vIOMMU cap/ecap */
>> +
>> +return true;
>> +}
>> +
>> +/*
>> + * virtual VT-d which wants nested needs to check the host IOMMU
>> + * nesting cap info behind the assigned devices. Thus that vIOMMU
>> + * could bind guest page table to host.
>> + */
>> +static bool vtd_check_idev(IntelIOMMUState *s, IOMMUFDDevice *idev,
>> +   Error **errp)
>> +{
>> +struct iommu_hw_info_vtd vtd;
>> +enum iommu_hw_info_type type =
>IOMMU_HW_INFO_TYPE_INTEL_VTD;
>> +
>> +if (iommufd_device_get_info(idev, , sizeof(vtd), )) {
>> +error_setg(errp, "Failed to get IOMMU capability!!!");
>> +return false;
>> +}
>> +
>> +if (type != IOMMU_HW_INFO_TYPE_INTEL_VTD) {
>> +error_setg(errp, "IOMMU hardware is not compatible!!!");
>> +return false;
>> +}
>> +
>> +return vtd_sync_hw_info(s, , errp);
>> +}
>> +
>>  static int vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int32_t
>devfn,
>>  IOMMUFDDevice *idev, Error **errp)
>>  {
>> @@ -3837,6 +3878,10 @@ static int vtd_dev_set_iommu_device(PCIBus
>*bus, void *opaque, int32_t devfn,
>>  return 0;
>>  }
>>
>> +if (!vtd_check_idev(s, idev, errp)) {In
>In
>[RFC 0/7] VIRTIO-IOMMU/VFIO: Fix host

Re: [PATCH rfcv1 6/6] intel_iommu: add a framework to check and sync host IOMMU cap/ecap

2024-01-17 Thread Eric Auger 
Hi Zhenzhong,

On 1/15/24 11:13, Zhenzhong Duan wrote:
> From: Yi Liu 
>
> Add a framework to check and synchronize host IOMMU cap/ecap with
> vIOMMU cap/ecap.
>
> Currently only stage-2 translation is supported which is backed by
> shadow page table on host side. So we don't need exact matching of
> each bit of cap/ecap between vIOMMU and host. However, we can still
> utilize this framework to ensure compatibility of host and vIOMMU's
> address width at least, i.e., vIOMMU's aw_bits <= host aw_bits,
> which is missed before.
>
> When stage-1 translation is supported in future, a.k.a. scalable
> modern mode, we need to ensure compatibility of each bits. Some
> bits are user controllable, they should be checked with host side
> to ensure compatibility. Other bits are not, they should be synced
> into vIOMMU cap/ecap for compatibility.
>
> The sequence will be:
>
> vtd_cap_init() initializes iommu->cap/ecap.  vtd_cap_init()
> iommu->host_cap/ecap is initialized as iommu->cap/ecap.   vtd_init()
> iommu->host_cap/ecap is checked and updated some bits with host cap/ecap. 
>  vtd_sync_hw_info()
> iommu->cap/ecap is finalized as iommu->host_cap/ecap.   
> vtd_machine_done_hook()
>
> iommu->host_cap/ecap is a temporary storage to hold intermediate value
> when synthesize host cap/ecap and vIOMMU's initial configured cap/ecap.
>
> Signed-off-by: Yi Liu 
> Signed-off-by: Yi Sun 
> Signed-off-by: Zhenzhong Duan 
> ---
>  include/hw/i386/intel_iommu.h |  4 ++
>  hw/i386/intel_iommu.c | 78 +++
>  2 files changed, 75 insertions(+), 7 deletions(-)
>
> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> index c65fdde56f..b8abbcce12 100644
> --- a/include/hw/i386/intel_iommu.h
> +++ b/include/hw/i386/intel_iommu.h
> @@ -292,6 +292,9 @@ struct IntelIOMMUState {
>  uint64_t cap;   /* The value of capability reg */
>  uint64_t ecap;  /* The value of extended capability reg 
> */
>  
> +uint64_t host_cap;  /* The value of host capability reg */
> +uint64_t host_ecap; /* The value of host ext-capability reg 
> */
> +
>  uint32_t context_cache_gen; /* Should be in [1,MAX] */
>  GHashTable *iotlb;  /* IOTLB */
>  
> @@ -314,6 +317,7 @@ struct IntelIOMMUState {
>  bool dma_translation;   /* Whether DMA translation supported */
>  bool pasid; /* Whether to support PASID */
>  
> +bool cap_finalized; /* Whether VTD capability finalized */
>  /*
>   * Protects IOMMU states in general.  Currently it protects the
>   * per-IOMMU IOTLB cache, and context entry cache in VTDAddressSpace.
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 4c1d058ebd..be03fcbf52 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3819,6 +3819,47 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, 
> PCIBus *bus,
>  return vtd_dev_as;
>  }
>  
> +static bool vtd_sync_hw_info(IntelIOMMUState *s, struct iommu_hw_info_vtd 
> *vtd,
> + Error **errp)
> +{
> +uint64_t addr_width;
> +
> +addr_width = (vtd->cap_reg >> 16) & 0x3fULL;
> +if (s->aw_bits > addr_width) {
> +error_setg(errp, "User aw-bits: %u > host address width: %lu",
> +   s->aw_bits, addr_width);
> +return false;
> +}
> +
> +/* TODO: check and sync host cap/ecap into vIOMMU cap/ecap */
> +
> +return true;
> +}
> +
> +/*
> + * virtual VT-d which wants nested needs to check the host IOMMU
> + * nesting cap info behind the assigned devices. Thus that vIOMMU
> + * could bind guest page table to host.
> + */
> +static bool vtd_check_idev(IntelIOMMUState *s, IOMMUFDDevice *idev,
> +   Error **errp)
> +{
> +struct iommu_hw_info_vtd vtd;
> +enum iommu_hw_info_type type = IOMMU_HW_INFO_TYPE_INTEL_VTD;
> +
> +if (iommufd_device_get_info(idev, , sizeof(vtd), )) {
> +error_setg(errp, "Failed to get IOMMU capability!!!");
> +return false;
> +}
> +
> +if (type != IOMMU_HW_INFO_TYPE_INTEL_VTD) {
> +error_setg(errp, "IOMMU hardware is not compatible!!!");
> +return false;
> +}
> +
> +return vtd_sync_hw_info(s, , errp);
> +}
> +
>  static int vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int32_t devfn,
>  IOMMUFDDevice *idev, Error **errp)
>  {
> @@ -3837,6 +3878,10 @@ static int vtd_dev_set_iommu_device(PCIBus *bus, void 
> *opaque, int32_t devfn,
>  return 0;
>  }
>  
> +if (!vtd_check_idev(s, idev, errp)) {In
In
[RFC 0/7] VIRTIO-IOMMU/VFIO: Fix host iommu geometry handling for
hotplugged devices
https://lore.kernel.org/all/20240117080414.316890-1-eric.au...@redhat.com/

I also attempt to pass host iommu info to the virtio-iommu but with
legacy BE. In my case I want to pass the reserved memory regions which

[PATCH rfcv1 6/6] intel_iommu: add a framework to check and sync host IOMMU cap/ecap

2024-01-15 Thread Zhenzhong Duan 
From: Yi Liu 

Add a framework to check and synchronize host IOMMU cap/ecap with
vIOMMU cap/ecap.

Currently only stage-2 translation is supported which is backed by
shadow page table on host side. So we don't need exact matching of
each bit of cap/ecap between vIOMMU and host. However, we can still
utilize this framework to ensure compatibility of host and vIOMMU's
address width at least, i.e., vIOMMU's aw_bits <= host aw_bits,
which is missed before.

When stage-1 translation is supported in future, a.k.a. scalable
modern mode, we need to ensure compatibility of each bits. Some
bits are user controllable, they should be checked with host side
to ensure compatibility. Other bits are not, they should be synced
into vIOMMU cap/ecap for compatibility.

The sequence will be:

vtd_cap_init() initializes iommu->cap/ecap.  vtd_cap_init()
iommu->host_cap/ecap is initialized as iommu->cap/ecap.   vtd_init()
iommu->host_cap/ecap is checked and updated some bits with host cap/ecap.  
vtd_sync_hw_info()
iommu->cap/ecap is finalized as iommu->host_cap/ecap.   
vtd_machine_done_hook()

iommu->host_cap/ecap is a temporary storage to hold intermediate value
when synthesize host cap/ecap and vIOMMU's initial configured cap/ecap.

Signed-off-by: Yi Liu 
Signed-off-by: Yi Sun 
Signed-off-by: Zhenzhong Duan 
---
 include/hw/i386/intel_iommu.h |  4 ++
 hw/i386/intel_iommu.c | 78 +++
 2 files changed, 75 insertions(+), 7 deletions(-)

diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index c65fdde56f..b8abbcce12 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -292,6 +292,9 @@ struct IntelIOMMUState {
 uint64_t cap;   /* The value of capability reg */
 uint64_t ecap;  /* The value of extended capability reg */
 
+uint64_t host_cap;  /* The value of host capability reg */
+uint64_t host_ecap; /* The value of host ext-capability reg */
+
 uint32_t context_cache_gen; /* Should be in [1,MAX] */
 GHashTable *iotlb;  /* IOTLB */
 
@@ -314,6 +317,7 @@ struct IntelIOMMUState {
 bool dma_translation;   /* Whether DMA translation supported */
 bool pasid; /* Whether to support PASID */
 
+bool cap_finalized; /* Whether VTD capability finalized */
 /*
  * Protects IOMMU states in general.  Currently it protects the
  * per-IOMMU IOTLB cache, and context entry cache in VTDAddressSpace.
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 4c1d058ebd..be03fcbf52 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3819,6 +3819,47 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, 
PCIBus *bus,
 return vtd_dev_as;
 }
 
+static bool vtd_sync_hw_info(IntelIOMMUState *s, struct iommu_hw_info_vtd *vtd,
+ Error **errp)
+{
+uint64_t addr_width;
+
+addr_width = (vtd->cap_reg >> 16) & 0x3fULL;
+if (s->aw_bits > addr_width) {
+error_setg(errp, "User aw-bits: %u > host address width: %lu",
+   s->aw_bits, addr_width);
+return false;
+}
+
+/* TODO: check and sync host cap/ecap into vIOMMU cap/ecap */
+
+return true;
+}
+
+/*
+ * virtual VT-d which wants nested needs to check the host IOMMU
+ * nesting cap info behind the assigned devices. Thus that vIOMMU
+ * could bind guest page table to host.
+ */
+static bool vtd_check_idev(IntelIOMMUState *s, IOMMUFDDevice *idev,
+   Error **errp)
+{
+struct iommu_hw_info_vtd vtd;
+enum iommu_hw_info_type type = IOMMU_HW_INFO_TYPE_INTEL_VTD;
+
+if (iommufd_device_get_info(idev, , sizeof(vtd), )) {
+error_setg(errp, "Failed to get IOMMU capability!!!");
+return false;
+}
+
+if (type != IOMMU_HW_INFO_TYPE_INTEL_VTD) {
+error_setg(errp, "IOMMU hardware is not compatible!!!");
+return false;
+}
+
+return vtd_sync_hw_info(s, , errp);
+}
+
 static int vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int32_t devfn,
 IOMMUFDDevice *idev, Error **errp)
 {
@@ -3837,6 +3878,10 @@ static int vtd_dev_set_iommu_device(PCIBus *bus, void 
*opaque, int32_t devfn,
 return 0;
 }
 
+if (!vtd_check_idev(s, idev, errp)) {
+return -1;
+}
+
 vtd_iommu_lock(s);
 
 vtd_idev = g_hash_table_lookup(s->vtd_iommufd_dev, );
@@ -4071,10 +4116,11 @@ static void vtd_init(IntelIOMMUState *s)
 {
 X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
 
-memset(s->csr, 0, DMAR_REG_SIZE);
-memset(s->wmask, 0, DMAR_REG_SIZE);
-memset(s->w1cmask, 0, DMAR_REG_SIZE);
-memset(s->womask, 0, DMAR_REG_SIZE);
+/* CAP/ECAP are initialized in machine create done stage */
+memset(s->csr + DMAR_GCMD_REG, 0, DMAR_REG_SIZE - DMAR_GCMD_REG);
+memset(s->wmask + DMAR_GCMD_REG, 0, DMAR_REG_SIZE -