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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.09.2019 13:47, Paul Durrant wrote:
>> -----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>

Thanks.

> ...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;

Yes, I did change this intentionally, as I think the "sharept" option
(in its positive or negative form) should not be specified to Xen
built with this tied to a fixed value.

Jan

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