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

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

On Thu, Aug 09, 2018 at 01:36:57AM -0600, Jan Beulich wrote:
> >>> On 08.08.18 at 18:18, <roger.pau@xxxxxxxxxx> wrote:
> > On Wed, Aug 08, 2018 at 07:15:54AM -0600, Jan Beulich wrote:
> >> >>> On 08.08.18 at 12:07, <roger.pau@xxxxxxxxxx> wrote:
> >> > @@ -134,13 +135,67 @@ void arch_iommu_domain_destroy(struct domain *d)
> >> >  {
> >> >  }
> >> >  
> >> > +static bool __hwdom_init hwdom_iommu_map(const struct domain *d, 
> >> > unsigned long pfn,
> >> > +                                         unsigned long max_pfn)
> >> > +{
> >> > +    unsigned int i;
> >> > +
> >> > +    /*
> >> > +     * Ignore any address below 1MB, that's already identity mapped by 
> >> > the
> >> > +     * domain builder for HVM.
> >> > +     */
> >> > +    if ( (is_hvm_domain(d) && pfn < PFN_DOWN(MB(1))) ||
> >> 
> >> Careful again here with the distinction between Dom0 and hwdom:
> >> The domain builder you refer to is - aiui - the in-Xen one, i.e. the
> >> one _only_ dealing with Dom0.
> > 
> > So this should instead be:
> > 
> > if ( (is_control_domain(d) && is_hvm_domain(d) && pfn < PFN_DOWN(MB(1))) ||
> 
> From an abstract perspective (long term plans) is_control_domain()
> can be true for multiple domains, none of which may be Dom0 or
> hwdom. So no, I don't think the addition would help in any way.
> With the reference to the in-Xen domain builder, I think you really
> mean Dom0 here.

OK, I will check against the domid then.

> >> > +    /*
> >> > +     * If dom0-strict mode is enabled or the guest type is PVH/HVM then 
> >> > exclude
> >> > +     * conventional RAM and let the common code map dom0's pages.
> >> > +     */
> >> > +    if ( page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL) &&
> >> > +         (iommu_dom0_strict || is_hvm_domain(d)) )
> >> > +        return false;
> >> > +    if ( page_is_ram_type(pfn, RAM_TYPE_RESERVED) &&
> >> > +         !iommu_dom0_reserved && !iommu_dom0_inclusive )
> >> > +        return false;
> >> > +    if ( !page_is_ram_type(pfn, RAM_TYPE_UNUSABLE) &&
> >> > +         !page_is_ram_type(pfn, RAM_TYPE_RESERVED) &&
> >> > +         !page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL) &&
> >> > +         (!iommu_dom0_inclusive || pfn > max_pfn) )
> >> > +        return false;
> >> 
> >> As page_is_ram_type() is, especially on systems with many E820
> >> entries, not really cheap, I think at least a minimum amount of
> >> optimization is on order here - after all you do this for every
> >> single page of the system. Hence minimally the result of the first
> >> CONVENTIONAL and RESERVED queries should be latched and
> >> re-used (or "else" be used suitably). Ideally an approach would
> >> be used which involved just a single iteration through the E820
> >> map, but I realize this may be more than is feasible here.
> > 
> > This would be quite better if I could just fetch the type, then I
> > could add a switch (type) { ... and it would be better IMO.
> 
> Except that, as said, there's no "the" type for an entire page.
> Only a single byte can have an exact type.

Right, I don0t think the original code was much better in that regard
anyway, neither I'm sure about how to handle this any better.

> >> Furthermore I'm unconvinced the !page_is_ram_type() uses
> >> are really what you want: The function returning false means
> >> "don't know", not "no". Perhaps the function needs to be made
> >> return a tristate (yes, no, or part of it).
> > 
> > Right, that's why I have three different !page_is_ram_type checks in
> > the last branch of the if, so that I can make sure it's not one of the
> > previous types and also account for holes.
> 
> I'm afraid I don't understand. Take the example of a single
> page being split in an unusable and a reserved part. Both
> respective function invocations will return false. Yet you
> want to exclude both unusable and reserved types when
> !iommu_dom0_inclusive, and hence your goal would be to
> exclude that page here.

Right, but I'm not sure how to fix this given the current interfaces.
I plan to introduce something like page_get_type() that returns the
whole type for a page in a similar fashion to what page_is_ram_type
currently does, but this is not going to solve the issue of a page
having different types.

> As to unusable - don't you break original behavior here
> anyway? Shouldn't the function return false when
> page_is_ram_type(pfn, RAM_TYPE_UNUSABLE), effectively
> independent of any command line options?

Yes, I think so, will fix it in the next version.

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