| Message ID | 20200417102650.20083-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/vtd: fix EPT page table sharing check | expand |
On 17.04.2020 12:26, Roger Pau Monne wrote: > The EPT page tables can be shared with the IOMMU as long as the page > sizes supported by EPT are also supported by the IOMMU. > > Current code checks that both the IOMMU and EPT support the same page > sizes, but this is not strictly required, the IOMMU supporting more > page sizes than EPT is fine and shouldn't block page sharing. Meaning the check isn't wrong, just too strict. Hence maybe "relax" instead of "fix" in the subject? Also "... page table sharing." > --- a/xen/drivers/passthrough/vtd/iommu.c > +++ b/xen/drivers/passthrough/vtd/iommu.c > @@ -1914,8 +1914,10 @@ static int __init vtd_ept_page_compatible(struct vtd_iommu *iommu) > if ( rdmsr_safe(MSR_IA32_VMX_EPT_VPID_CAP, ept_cap) != 0 ) > return 0; > > - return (ept_has_2mb(ept_cap) && opt_hap_2mb) == cap_sps_2mb(vtd_cap) && > - (ept_has_1gb(ept_cap) && opt_hap_1gb) == cap_sps_1gb(vtd_cap); > + return ((ept_has_2mb(ept_cap) && opt_hap_2mb) ? cap_sps_2mb(vtd_cap) > + : true) && > + ((ept_has_1gb(ept_cap) && opt_hap_1gb) ? cap_sps_1gb(vtd_cap) > + : true); I have to admit that I always find it odd to have "true" or "false" as the 2nd or 3rd operand of a ternary. How about simply changing == to <= in the original expression? Jan
On Fri, Apr 17, 2020 at 12:57:16PM +0200, Jan Beulich wrote: > On 17.04.2020 12:26, Roger Pau Monne wrote: > > The EPT page tables can be shared with the IOMMU as long as the page > > sizes supported by EPT are also supported by the IOMMU. > > > > Current code checks that both the IOMMU and EPT support the same page > > sizes, but this is not strictly required, the IOMMU supporting more > > page sizes than EPT is fine and shouldn't block page sharing. > > Meaning the check isn't wrong, just too strict. Hence maybe > "relax" instead of "fix" in the subject? > > Also "... page table sharing." Sure. > > --- a/xen/drivers/passthrough/vtd/iommu.c > > +++ b/xen/drivers/passthrough/vtd/iommu.c > > @@ -1914,8 +1914,10 @@ static int __init vtd_ept_page_compatible(struct vtd_iommu *iommu) > > if ( rdmsr_safe(MSR_IA32_VMX_EPT_VPID_CAP, ept_cap) != 0 ) > > return 0; > > > > - return (ept_has_2mb(ept_cap) && opt_hap_2mb) == cap_sps_2mb(vtd_cap) && > > - (ept_has_1gb(ept_cap) && opt_hap_1gb) == cap_sps_1gb(vtd_cap); > > + return ((ept_has_2mb(ept_cap) && opt_hap_2mb) ? cap_sps_2mb(vtd_cap) > > + : true) && > > + ((ept_has_1gb(ept_cap) && opt_hap_1gb) ? cap_sps_1gb(vtd_cap) > > + : true); > > I have to admit that I always find it odd to have "true" or "false" > as the 2nd or 3rd operand of a ternary. How about simply changing > == to <= in the original expression? I find it weird to use <= to compare booleans, but I can change it. Let me post a new version with the adjustments to the commit message and this requested change. Thanks, Roger.
On 17.04.2020 13:16, Roger Pau Monné wrote: > On Fri, Apr 17, 2020 at 12:57:16PM +0200, Jan Beulich wrote: >> On 17.04.2020 12:26, Roger Pau Monne wrote: >>> --- a/xen/drivers/passthrough/vtd/iommu.c >>> +++ b/xen/drivers/passthrough/vtd/iommu.c >>> @@ -1914,8 +1914,10 @@ static int __init vtd_ept_page_compatible(struct vtd_iommu *iommu) >>> if ( rdmsr_safe(MSR_IA32_VMX_EPT_VPID_CAP, ept_cap) != 0 ) >>> return 0; >>> >>> - return (ept_has_2mb(ept_cap) && opt_hap_2mb) == cap_sps_2mb(vtd_cap) && >>> - (ept_has_1gb(ept_cap) && opt_hap_1gb) == cap_sps_1gb(vtd_cap); >>> + return ((ept_has_2mb(ept_cap) && opt_hap_2mb) ? cap_sps_2mb(vtd_cap) >>> + : true) && >>> + ((ept_has_1gb(ept_cap) && opt_hap_1gb) ? cap_sps_1gb(vtd_cap) >>> + : true); >> >> I have to admit that I always find it odd to have "true" or "false" >> as the 2nd or 3rd operand of a ternary. How about simply changing >> == to <= in the original expression? > > I find it weird to use <= to compare booleans, but I can change it. In the end Kevin will have to judge what variant to use. > Let me post a new version with the adjustments to the commit message > and this requested change. Thanks. Jan
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c index 07d40b37fe..63298d3a99 100644 --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -1914,8 +1914,10 @@ static int __init vtd_ept_page_compatible(struct vtd_iommu *iommu) if ( rdmsr_safe(MSR_IA32_VMX_EPT_VPID_CAP, ept_cap) != 0 ) return 0; - return (ept_has_2mb(ept_cap) && opt_hap_2mb) == cap_sps_2mb(vtd_cap) && - (ept_has_1gb(ept_cap) && opt_hap_1gb) == cap_sps_1gb(vtd_cap); + return ((ept_has_2mb(ept_cap) && opt_hap_2mb) ? cap_sps_2mb(vtd_cap) + : true) && + ((ept_has_1gb(ept_cap) && opt_hap_1gb) ? cap_sps_1gb(vtd_cap) + : true); } /*
The EPT page tables can be shared with the IOMMU as long as the page sizes supported by EPT are also supported by the IOMMU. Current code checks that both the IOMMU and EPT support the same page sizes, but this is not strictly required, the IOMMU supporting more page sizes than EPT is fine and shouldn't block page sharing. This is likely not a common case (IOMMU supporting more page sizes than EPT), but should still be fixed for correctness. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/drivers/passthrough/vtd/iommu.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)