Re: [Xen-devel] [PATCH v11.1 3/6] sysctl / libxl: report whether IOMMU/HAP page table sharing is supported

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 13 September 2019 12:10
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>; Julien Grall 
> <julien.grall@xxxxxxx>; Andrew
> Cooper <Andrew.Cooper3@xxxxxxxxxx>; Anthony Perard 
> <anthony.perard@xxxxxxxxxx>; Christian Lindig
> <christian.lindig@xxxxxxxxxx>; Roger Pau Monne <roger.pau@xxxxxxxxxx>; George 
> Dunlap
> <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Kevin Tian 
> <kevin.tian@xxxxxxxxx>;
> Stefano Stabellini <sstabellini@xxxxxxxxxx>; Konrad Rzeszutek Wilk 
> <konrad.wilk@xxxxxxxxxx>; David
> Scott <dave@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>; WeiLiu <wl@xxxxxxx>
> Subject: [PATCH v11.1 3/6] sysctl / libxl: report whether IOMMU/HAP page 
> table sharing is supported
> 
> This patch defines a new bit reported in the hw_cap field of struct
> xen_sysctl_physinfo to indicate whether the platform supports sharing of
> HAP page tables (i.e. the P2M) with the IOMMU. This informs the toolstack
> whether the domain needs extra memory to store discrete IOMMU page tables
> or not.
> 
> NOTE: This patch makes sure iommu_hap_pt_shared is clear if HAP is not
>       supported or the IOMMU is disabled, and defines it to false if
>       !CONFIG_HVM.
> 
> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> Acked-by: Christian Lindig <christian.lindig@xxxxxxxxxx>

Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx>

...with one observation...

[snip]
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -49,7 +49,11 @@ int8_t __hwdom_initdata iommu_hwdom_rese
>   * default until we find a good solution to resolve it.
>   */
>  bool_t __read_mostly iommu_intpost;
> -bool_t __read_mostly iommu_hap_pt_share = 1;
> +
> +#ifndef iommu_hap_pt_share
> +bool __read_mostly iommu_hap_pt_share = true;
> +#endif
> +
>  bool_t __read_mostly iommu_debug;
>  bool_t __read_mostly amd_iommu_perdev_intremap = 1;
> 
> @@ -102,8 +106,10 @@ static int __init parse_iommu_param(cons
>              iommu_hwdom_passthrough = val;
>          else if ( (val = parse_boolean("dom0-strict", s, ss)) >= 0 )
>              iommu_hwdom_strict = val;
> +#ifndef iommu_hap_pt_share
>          else if ( (val = parse_boolean("sharept", s, ss)) >= 0 )
>              iommu_hap_pt_share = val;
> +#endif
>          else
>              rc = -EINVAL;
> 

With this change there will be a command line parse error if 'no-sharept' is 
passed on the command line to a hypervisor built without CONFIG_HVM. I don't 
know whether you really want that behaviour, which is why my patch did:

@@ -103,7 +107,14 @@ static int __init parse_iommu_param(const char *s)
         else if ( (val = parse_boolean("dom0-strict", s, ss)) >= 0 )
             iommu_hwdom_strict = val;
         else if ( (val = parse_boolean("sharept", s, ss)) >= 0 )
+        {
+#ifndef iommu_hap_pt_share
             iommu_hap_pt_share = val;
+#else
+            if (val != iommu_hap_pt_share)
+                rc = -EINVAL;
+#endif
+        }
         else
             rc = -EINVAL;

  Paul
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel