Re: [PATCH v7 3/6] iommu: make iommu_inclusive_mapping a suboption of dom0-iommu

[Julien Grall <julien.grall@arm.com>]

Hi,

On 22/08/18 16:22, Roger Pau Monné wrote:
> On Wed, Aug 22, 2018 at 04:07:26PM +0100, Julien Grall wrote:
>> Hi Roger,
>>
>> On 22/08/18 08:51, Roger Pau Monne wrote:
>>> Introduce a new dom0-iommu=map-inclusive generic option that
>>> supersedes iommu_inclusive_mapping. The previous behavior is preserved
>>> and the option should only be enabled by default on Intel hardware.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
>>> ---
>>> Changes since v4:
>>>    - Use an if to set the default option value.
>>>    - Set the option to false unconditionally on ARM.
>>>
>>> Changes since v2:
>>>    - Fix typo in commit message.
>>>    - Change style and text of the documentation in xen command line.
>>>    - Set the defaults in {intel/amd}_iommu_hwdom_init for inclusive.
>>>    - Re-add the iommu_dom0_passthrough || !is_pv_domain(d) check.
>>>
>>> Changes since v1:
>>>    - Use dom0-iommu instead of the iommu option.
>>>    - Only enable by default on Intel hardware.
>>> ---
>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
>>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>>> Cc: Jan Beulich <jbeulich@suse.com>
>>> Cc: Julien Grall <julien.grall@arm.com>
>>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>>> Cc: Tim Deegan <tim@xen.org>
>>> Cc: Wei Liu <wei.liu2@citrix.com>
>>> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>> Cc: Brian Woods <brian.woods@amd.com>
>>> Cc: Kevin Tian <kevin.tian@intel.com>
>>> ---
>>>    docs/misc/xen-command-line.markdown         | 13 ++++-
>>>    xen/drivers/passthrough/amd/pci_amd_iommu.c |  4 ++
>>>    xen/drivers/passthrough/arm/iommu.c         |  4 ++
>>>    xen/drivers/passthrough/arm/smmu.c          |  2 +
>>>    xen/drivers/passthrough/iommu.c             | 12 +++++
>>>    xen/drivers/passthrough/vtd/extern.h        |  2 -
>>>    xen/drivers/passthrough/vtd/iommu.c         |  8 ++-
>>>    xen/drivers/passthrough/vtd/x86/vtd.c       | 58 +-------------------
>>>    xen/drivers/passthrough/x86/iommu.c         | 59 +++++++++++++++++++++
>>>    xen/include/xen/iommu.h                     |  2 +
>>>    10 files changed, 99 insertions(+), 65 deletions(-)
>>>
>>> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
>>> index cd57960ede..98f0f3b68b 100644
>>> --- a/docs/misc/xen-command-line.markdown
>>> +++ b/docs/misc/xen-command-line.markdown
>>> @@ -682,7 +682,7 @@ Flag that makes a dom0 use shadow paging. Only works when "pvh" is
>>>    enabled.
>>>    ### dom0-iommu
>>> -> `= List of [ passthrough | strict ]`
>>> +> `= List of [ passthrough | strict | map-inclusive ]`
>>>    This list of booleans controls the iommu usage by Dom0:
>>> @@ -696,6 +696,14 @@ This list of booleans controls the iommu usage by Dom0:
>>>      `true` for a PVH Dom0 and any attempt to overwrite it from the command line
>>>      is ignored.
>>> +* `map-inclusive`: sets up DMA remapping for all the non-RAM regions below 4GB
>>> +  except for unusable ranges. Use this to work around firmware issues providing
>>> +  incorrect RMRR/IVMD entries. Rather than only mapping RAM pages for IOMMU
>>> +  accesses for Dom0, with this option all pages up to 4GB, not marked as
>>> +  unusable in the E820 table, will get a mapping established. Note that this
>>> +  option is only applicable to a PV Dom0 and is enabled by default on Intel
>>> +  hardware.
>>> +
>>>    ### dom0\_ioports\_disable (x86)
>>>    > `= List of <hex>-<hex>`
>>> @@ -1233,6 +1241,9 @@ wait descriptor timed out', try increasing this value.
>>>    ### iommu\_inclusive\_mapping (VT-d)
>>>    > `= <boolean>`
>>> +**WARNING: This command line option is deprecated, and superseded by
>>> +_dom0-iommu=map-inclusive_ - using both options in combination is undefined.**
>>> +
>>>    > Default: `true`
>>>    Use this to work around firmware issues providing incorrect RMRR entries.
>>> diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>>> index ab39e7500d..27eb49619d 100644
>>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
>>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>>> @@ -253,6 +253,10 @@ static void __hwdom_init amd_iommu_hwdom_init(struct domain *d)
>>>        unsigned long i;
>>>        const struct amd_iommu *iommu;
>>> +    /* Inclusive IOMMU mappings are disabled by default on AMD hardware. */
>>> +    if ( iommu_hwdom_inclusive == -1 )
>>> +        iommu_hwdom_inclusive = false;
>>
>> This is a quite bad practice to mix boolean and integer. Can you please
>> rework the code to avoid that?
> 
> I don't think I can fix that. An integer is used so that the default
> value is set to -1. Then at runtime a default value based on the
> hardware is set unless the user has already specified a value on the
> command line.
> 
> It needs to be a tristate in order to detect whether the user has
> specified a value on the command line.

In that case the solution is to use -1, 0, 1. So replacing false with 0 
and true with 1.

Cheers,

-- 
Julien Grall

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