On 13.09.2019 13:47, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <[email protected]>
>> Sent: 13 September 2019 12:10
>> To: Paul Durrant <[email protected]>; [email protected]
>> Cc: Suravee Suthikulpanit <[email protected]>; Julien Grall
>> <[email protected]>; Andrew
>> Cooper <[email protected]>; Anthony Perard
>> <[email protected]>; Christian Lindig
>> <[email protected]>; Roger Pau Monne <[email protected]>;
>> George Dunlap
>> <[email protected]>; Ian Jackson <[email protected]>; Kevin Tian
>> <[email protected]>;
>> Stefano Stabellini <[email protected]>; Konrad Rzeszutek Wilk
>> <[email protected]>; David
>> Scott <[email protected]>; Tim (Xen.org) <[email protected]>; WeiLiu <[email protected]>
>> 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 <[email protected]>
>> Signed-off-by: Jan Beulich <[email protected]>
>> Acked-by: Christian Lindig <[email protected]>
>
> Reviewed-by: Paul Durrant <[email protected]>
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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
