Re: [PATCH] x86/CPUID: suppress IOMMU related hypervisor leaf data

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Jan 04, 2021 at 04:16:18PM +0100, Jan Beulich wrote:
> On 04.01.2021 15:56, Roger Pau Monné wrote:
> > On Mon, Jan 04, 2021 at 02:43:52PM +0100, Jan Beulich wrote:
> >> On 28.12.2020 11:54, Roger Pau Monné wrote:
> >>> On Mon, Nov 09, 2020 at 11:54:09AM +0100, Jan Beulich wrote:
> >>>> Now that the IOMMU for guests can't be enabled "on demand" anymore,
> >>>> there's also no reason to expose the related CPUID bit "just in case".
> >>>>
> >>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >>>
> >>> I'm not sure this is helpful from a guest PoV.
> >>>
> >>> How does the guest know whether it has pass through devices, and thus
> >>> whether it needs to check if this flag is present or not in order to
> >>> safely pass foreign mapped pages (or grants) to the underlying devices?
> >>>
> >>> Ie: prior to this change I would just check whether the flag is
> >>> present in CPUID to know whether FreeBSD needs to use a bounce buffer
> >>> in blkback and netback when running as a domU. If this is now
> >>> conditionally set only when the IOMMU is enabled for the guest I
> >>> also need to figure a way to know whether the domU has any passed
> >>> through device or not, which doesn't seem trivial.
> >>
> >> I'm afraid I don't understand your concern and/or description of
> >> the scenario. Prior to the change, the bit was set unconditionally.
> >> To me, _that_ was making the bit useless - no point in checking
> >> something which is always set anyway (leaving aside old Xen
> >> versions).
> > 
> > This bit was used to differentiate between versions of Xen that don't
> > create IOMMU mappings for grants/foreign maps (and so are broken) vs
> > versions of Xen that do create such mappings. If the bit is not set
> > HVM domains need a bounce buffer in blkback/netback in order to avoid
> > sending grants to devices.
> 
> Neither the comment in cpuid.h nor that in traps.c have any mention
> of this, and the constant's name also doesn't imply such.
> 
> > Now it's my understand that with this change sometimes the bit might
> > not be set not because we are running on an unfixed Xen version, but
> > because there's no IOMMU assigned to the domain, so the guest will
> > fallback to use a bounce buffer.
> 
> ... if it expects to ever map a foreign domain's pages.
> 
> I can see that reverting the change is one way to address the issue.
> Such a revert shouldn't be a plain one then, but one adjusting one
> or both of the the comments to indicate the _real_ purpose of this
> flag. (We probably better don't rename the constant, as we can't
> easily drop the old name from the public interface anyway.)

I'm happy to send the revert, but do you have any suggestion about the
fixed comments?

Maybe adding something like:

/*
 * Unditionally set the flag to notice this version of Xen has been
 * fixed to create IOMMU mappings for grant/foreign maps.
 */

Thanks, Roger.