Re: [Xen-devel] [PATCH v4 2/2] iommu: add rmrr Xen command line option for misc rmrrs

[Elena Ufimtseva <elena.ufimtseva@xxxxxxxxxx>]

On Thu, May 07, 2015 at 06:47:12AM +0100, Jan Beulich wrote:
> >>> Elena Ufimtseva <elena.ufimtseva@xxxxxxxxxx> 05/06/15 6:50 PM >>>
> >On Tue, May 05, 2015 at 11:14:30AM +0100, Jan Beulich wrote:
> >> >>> On 28.04.15 at 01:50, <elena.ufimtseva@xxxxxxxxxx> wrote:
> >> > +        if ( mfn_valid(rmrru->base_address << PAGE_SHIFT) ||
> >> > +             mfn_valid(rmrru->end_address << PAGE_SHIFT) )
> >> 
> >> Please use PFN_UP() and PFN_DOWN() as appropriate. Also, how
> >> large do we expect custom RMRRs to reasonably get? If no more
> >> than a few pages, I think it would be quite reasonable to check
> >> all involved pages here rather than just first and last one. (And
> >> even if there can be many pages here, leaving at least a comment
> >> saying that this could do with improvement would be nice.)
> >
> >Hm, thats a hard part not having these regions documented.                   
> >   
> >But I have found this thread  https://lkml.org/lkml/2014/6/18/683.           
> >   
> >If I am reading it right, for GPU that region maybe as big as 1024M.         
> >   
> >For USB I think its smaller, but hard to prove without docs.                 
> >   
> >I think we should just limit it to some max amount per RMRR and document     
> >   
> >it. What you think?
> 
> Real RMRRs can be basically unlimited, but I'm not convinced we need them
> to be unlimited in this workaround. In particular I think we shouldn't aim at
> working around GPU related deficiencies of the firmware (at least until we
> know we absolutely need to), and hence yes, documenting a limit of a couple
> of pages (say 16 for starters) per region would seem reasonable.

Yes, sounds good.

> 
> >> But what I'm worried about is invalid uses like
> >> 
> >> rmrr=<addr>=0001:bb:dd.f,0000:bb:dd.f
> >> 
> >> (as opposed to the valid
> >> 
> >> rmrr=<addr>=0001:bb:dd.f,bb:dd.f
> >> 
> >> ). On an earlier version I had specifically pointed out that dealing
> >> with an omitted vs a zero segment requires special care.
> >
> >Yes, you did.                                                                
> >   
> >This is just a very easy way to verify segments by making sure               
> >   
> >they are all equal if multiple devices specified with minimum code           
> >   
> >change.                                                                      
> >   
> >But that can be done in other way as well.                                   
> >   
> >In your proposed second valid case, segment 0001 should be assumed for       
> >   
> >second device as its own segment is not specified? 
> 

Hi Jan

> Exactly. Or (less optimal for the user, but easier to implement) require the 
> segment
> number to repeated when non-zero (i.e. only adjust the documentation part 
> here).

You mean that when segment is non-zero, just specify it explicitly even
when duplicating seg number from first sbdf?
like this:
rmrr=<addr>=0001:bb:dd.f,0001:bb:dd.f

We can use more general approach when adding extra RMRRs. 
I cannot find from Intel spec if one RMRR region cannot be shared
among multiple devices from multiple segments.
Thus we can allow to have any segments for devices specified on RMRR
command line and add RMRRs indexing into the list acpi_rmrr_units by pci
segment (and checking address ranges).
There is also check for overlapping RMRRs in acpi_parse_one_rmrr 
which does not take into accound different pci segments. If one RMRR can
be shared by multiple devices from different segments, this check may need
to be modified as well.
Any advise on this?

Thank you!

> 
> Jan
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel