Re: [Xen-devel] [PATCH v6 6/6] x86/iommu: add map-reserved dom0-iommu option to map reserved memory ranges

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.08.18 at 17:22, <roger.pau@xxxxxxxxxx> wrote:
> On Tue, Aug 21, 2018 at 07:36:17AM -0600, Jan Beulich wrote:
>> >>> On 21.08.18 at 09:49, <roger.pau@xxxxxxxxxx> wrote:
>> > --- a/docs/misc/xen-command-line.markdown
>> > +++ b/docs/misc/xen-command-line.markdown
>> > @@ -704,6 +704,15 @@ This list of booleans controls the iommu usage by 
>> > Dom0:
>> >    option is only applicable to a PV Dom0 and is enabled by default on 
>> > Intel
>> >    hardware.
>> >  
>> > +* `map-reserved`: sets up DMA remapping for all the reserved regions in 
>> > the
>> > +  memory map for Dom0. Use this to work around firmware issues providing
>> > +  incorrect RMRR/IVMD entries. Rather than only mapping RAM pages for 
>> > IOMMU
>> > +  accesses for Dom0, all memory regions marked as reserved in the memory 
>> > map
>> > +  that don't overlap with any MMIO region from emulated devices will be
>> > +  identity mapped. This option maps a subset of the memory that would be
>> > +  mapped when using the `map-inclusive` option. This option is available 
>> > to a
>> > +  PVH Dom0 and is enabled by default on Intel hardware.
>> 
>> This sounds as if the option was meaningless for PV, but I can't seem
>> to see this being the case. The places setting iommu_hwdom_reserved
>> don't look at domain type afaics, and the change to the default case
>> in hwdom_iommu_map()'s switch() block has the is_hvm_domain() check
>> independent of the iommu_hwdom_reserved one.
> 
> The option should be functional for PV also, so that a user could
> have:
> 
> dom0-iommu=map-inclusive=0,map-reserved
> 
>> I also wonder about the wording "is available to": For a domain type
>> restriction, would "only takes effect on" or some such be more to the
>> point?
> 
> What about:
> 
> "This option is available to all Dom0 modes and is enabled by default
> on Intel hardware."

Yes please.

>> > @@ -138,16 +139,24 @@ static bool __hwdom_init hwdom_iommu_map(const 
>> > struct domain *d,
>> >                                           unsigned long pfn,
>> >                                           unsigned long max_pfn)
>> >  {
>> > +    unsigned int i, type;
>> > +
>> >      /*
>> >       * Set up 1:1 mapping for dom0. Default to include only conventional 
>> > RAM
>> >       * areas and let RMRRs include needed reserved regions. When set, the
>> >       * inclusive mapping additionally maps in every pfn up to 4GB except 
>> > those
>> > -     * that fall in unusable ranges.
>> > +     * that fall in unusable ranges for PV Dom0.
>> >       */
>> > -    if ( (pfn > max_pfn && !mfn_valid(_mfn(pfn))) || xen_in_range(pfn) )
>> > +    if ( (pfn > max_pfn && !mfn_valid(_mfn(pfn))) || xen_in_range(pfn) ||
>> > +         /*
>> > +          * Ignore any address below 1MB, that's already identity mapped 
>> > by the
>> > +          * Dom0 builder for HVM.
>> > +          */
>> > +         (!d->domain_id && is_hvm_domain(d) && pfn < PFN_DOWN(MB(1))) )
>> >          return false;
>> >  
>> > -    switch ( page_get_ram_type(pfn) )
>> > +    type = page_get_ram_type(pfn);
>> > +    switch ( type )
>> 
>> Any reason not to keep this a single line, putting the assignment inside
>> the switch()?
> 
> I don't like to put assignments inside of expressions, but I can
> certainly do that.

We do this in a number of places, so it would be nice if it was done
this way here as well.

Jan



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