Re: [Xen-devel] RMRR Fix Design for Xen

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

I'll work out a new design proposal based on below content and previous
discussions. Thanks Tiejun for your hard-working and Jan for your careful
reviews so far.

For below comment:
> > Also there's clearly an alternative proposal: Drop support for sharing
> > page tables. Your colleagues will surely have told you that we've
> > been considering this for quite some time, and had actually hoped
> > for them to do the necessary VT-d side work to allow for this without
> > causing performance regressions.

let's separate it from RMRR discussion, because RMRR issues are about
general p2m and thus orthogonal to the implementation difference between
shared or not-shared fact (though it did lead to different behaviors w/ current
bogus logic).

Thanks
Kevin

> From: Chen, Tiejun
> Sent: Monday, December 22, 2014 10:12 AM
> 
> Jan,
> 
> Thanks for your time but I'm not going to address your comments here.
> Because I heard this design is totally not satisfied your expectation.
> But this really was reviewed with several revisions by Kevin and Yang
> before sending in public...
> 
> Anyway, I guess the only thing what I can do is that, Kevin and Yang, or
> other appropriate guys should finish this design as you expect. So now
> I'd better not say anything to avoid bringing any inconvenience.
> 
> Tiejun
> 
> On 2014/12/19 23:13, Jan Beulich wrote:
> >>>> On 19.12.14 at 02:21, <tiejun.chen@xxxxxxxxx> wrote:
> >> #4 Something like USB, is still restricted to current RMRR implementation.
> We
> >>     should work out this case.
> >
> > This can mean all or nothing. My understanding is that right now code
> > assumes that USB devices won't use their RMRR-specified memory
> > regions post-boot (kind of contrary to your earlier statement that in
> > such a case the regions shouldn't be listed in RMRRs in the first place).
> >
> >> Design Overview
> >> ===============
> >>
> >> First of all we need to make sure all resources don't overlap RMRR. And
> then
> >> in case of shared ept, we can set these identity entries. And Certainly we
> >> will
> >> group all devices associated to one same RMRR entry, then make sure all
> >> group
> >> devices should be assigned to same VM.
> >>
> >> 1. Setup RMRR identity mapping
> >>
> >> current status:
> >>      * identity mapping only setup in non-shared ept case
> >>
> >> proposal:
> >>
> >> In non-shared ept case, IOMMU stuff always set those entries and RMRR is
> >> already marked reserved in host so its fine enough.
> >
> > Is it? Where? Or am I misunderstanding the whole statement, likely
> > due to me silently replacing "host" by "guest" (since reservation in
> > host address spaces is of no interest here afaict)?
> >
> >> But in shared ept case, we need to
> >> check any conflit, so we should follow up
> >>
> >>    - gfn space unoccupied
> >>          -> insert mapping: success.
> >>              gfn:_mfn(gfn), PAGE_ORDER_4K, p2m_mmio_direct,
> p2m_access_rw
> >>    - gfn space already occupied by 1:1 RMRR mapping
> >>          -> do nothing; success.
> >>    - gfn space already occupied by other mapping
> >>          -> fail.
> >>
> >> expectation:
> >>      * only devices w/ non-conflicting RMRR can be assigned
> >>      * fortunately this achieves the very initial intention to support IGD
> >>        pass-through on BDW
> >
> > Are you trying to say here that doing the above is all you need for
> > your specific machine? If so, that's clearly not something to go into
> > a design document.
> >
> > Also there's clearly an alternative proposal: Drop support for sharing
> > page tables. Your colleagues will surely have told you that we've
> > been considering this for quite some time, and had actually hoped
> > for them to do the necessary VT-d side work to allow for this without
> > causing performance regressions.
> >
> >> 2.1 Expose RMRR to user space
> >>
> >> current status:
> >>      * Xen always record RMRR info into one list, acpi_rmrr_units, while
> >> parsing
> >>        acpi. So we can retrieve these info by lookup that list.
> >>
> >> proposal:
> >>      * RMRR would be exposed by a new hypercall, which Jan already
> finished
> >> in
> >>        current version but just expose all RMRR info unconditionally.
> >>      * Furthermore we can expose RMRR on demand to diminish
> shrinking guest
> >>        RAM/MMIO space.
> >>      * So we will introduce a new parameter, 'rdm_forcecheck' and to
> >> collaborate
> >>        with SBDFs to control which RMRR should be exposed:
> >>
> >>          - We can set this parameter in .cfg file like,
> >>
> >>              rdm_forcecheck = 1 => Of course this should be 0 by
> default.
> >>
> >>          '1' means we should force check to reserve all ranges
> >> unconditionally.
> >>          and if failed VM wouldn't be created successfully. This also can
> >> give
> >>          user a chance to work well with later hotplug, even if not a
> device
> >>          assignment while creating VM.
> >>
> >>          If 0, we just check those assigned pci devices. As you know we
> already
> >
> > assigned? Wasn't the plan to have a separate potentially-to-be-
> > assigned list? And I can only re-iterate that the name
> > "rdm_forcecheck" doesn't really express what you mean. Since
> > your intention is to check all devices (rather than do a check that
> > otherwise wouldn't be done), "rdm_all" or "rdm_check_all" would
> > seem closer to the intended behavior.
> >
> >>          have such an existing hypercall to assign PCI devices, looks we
> can
> >> work
> >>          directly under this hypercall to get that necessary SBDF to sort
> >> which
> >>          RMRR should be handled. But obviously, we need to get these
> info
> >> before
> >>          we populate guest memory to make sure these RMRR ranges
> should be
> >>          excluded from guest memory. But unfortunately the memory
> populating
> >>          takes place before a device assignment, so we can't live on that
> >>          directly.
> >>
> >>          But as we discussed it just benefit that assigned case to reorder
> >> that
> >>          order, but not good to hotplug case. So we have to introduce a
> new
> >>          DOMCTL to pass that global parameter with SBDF at the same
> time.
> >>
> >>          For example, if we own these two RMRR entries,
> >
> > "own" is confusing here, I assume you mean if there are such entries.
> >
> >>          [00:14.0]    RMRR region: base_addr ab80a000 end_address
> ab81dfff
> >>          [00:02.0]    RMRR region: base_addr ad000000 end_address
> af7fffff
> >>
> >>          If 'rdm_forcecheck = 1', any caller to that hypercall may get
> these two
> >
> > s/may/will/ I suppose.
> >
> >>          entries. But if 'rdm_forcecheck = 0', and in .cfg file,
> >>
> >>          #1 'pci=[00:14.0, 00:02.0]' -> The caller still get these two
> entries.
> >>          #2 'pci=[00:02.0]' -> The caller just get one entry,
> >> 0xad000000:0xaf7fffff
> >>          #2 'pci=[00:14.0]' -> The caller just get one entry,
> >> 0xab80a000:0xab81dfff
> >>          #4 'pci=[others]' or no any pci configuration -> The caller get
> >> nothing.
> >
> > Oh, interesting, you dropped the idea of a separate list of possibly-
> > to-be-assigned devices. Why that?
> >
> >>          And ultimately, if we expose any RMRR entry at gfn,
> >>
> >>          in non-shared ept case,
> >>
> >>          p2m table:  valid non-identity mapping, gfn:mfn != 1:1
> >
> > This would mean ...
> >
> >>          VT-D table: no such a mapping until set identity mapping,
> >>                      gfn:_mfn(gfn) == 1:1 when we have a associated
> device
> >>                      assignment.
> >
> > ... the two mappings to go out of sync when such a 1:1 mapping gets
> > established. I think we should avoid such a situation if at all possible.
> >
> >>          in shared ept case,
> >>
> >>          p2m table\VT-D table:  always INVALID until set identity
> mapping,
> >>                                 gfn:_mfn(gfn) == 1:1 when we have
> a
> >> associated
> >>                                 device assignment.
> >>
> >>          But if we don't expose any RMRR entry,
> >>
> >>          in non-shared ept case,
> >>
> >>          p2m table:  valid non-identity mapping, gfn:mfn != 1:1
> >>          VT-D table: no such a mapping until set identity mapping,
> >>                      gfn:_mfn(gfn) == 1:1 when we have a associated
> device
> >>                      assignment.
> >>
> >>          in shared ept case,
> >>
> >>          p2m table\VT-D table:  If guest RAM already cover gfn, we
> have sunc a
> >>                                 valid non-identity mapping,
> gfn:mfn != 1:1,
> >> but
> >>                                 we can't set any identity mapping
> again then
> >>                                 that associated device can't be
> assigned
> >>                                 successfully. If not, we'll set identity
> >> mapping,
> >>                                 gfn:_mfn(gfn) == 1:1, to work well.
> >
> > So in the end we'd still have various cases of different behavior,
> > when really it would be much better (if not a requirement) if guest
> > observable behavior wouldn't depend on implementation details
> > like (non-)shared page tables.
> >
> >> 2.2 Detect and avoid RMRR conflictions with guest RAM/MMIO
> >>
> >> current status:
> >>      * Currently Xen doesn't detect anything to avoid any conflict.
> >>
> >> proposal:
> >>      * We need to cover all points to check any conflict. Below lists 
> >> places
> >>        where reserved region conflictions should be detected:
> >>
> >>          1>. libxc:setup_guest():modules_init()
> >>
> >>          There are some modules, like acpi_module and smbios_module.
> They may
> >>          occupy some ranges so we need to check if they're conflicting
> with
> >>          all ranges from that new hypercall above.
> >
> > I'm missing of what conflict resolution you expect to do here.
> > Following earlier discussion in the context of patch review made
> > pretty clear that this isn't really straightforward, having the
> > potential to prevent a guest from booting (e.g. when the virtual
> > BIOS conflicts with an RMRR).
> >
> >>          2>. libxc:setup_guest():xc_domain_populate_physmap()
> >>
> >>          There are two ways to exclude RMRR ranges here:
> >>
> >>              #1 Before we populate guest RAM without any RMRR
> range from that
> >>                 new hypercall to skip RMRR ranges when populating
> guest RAM.
> >>              #2 In Xen we can fliter RMRR range while calling
> >>                 XENMEM_populate_physmap, its no change to libxc,
> but skip
> >>                 setting p2m entry for RMRR ranges in Xen hypervisor
> >>
> >>          But to compare #1, #2 is not better since Xen still allocate those
> >>          range, and we have to sync somewhere else in Xen. And #1 is
> cleaner
> >>          because #2 actually shrinks the available memory size to the
> guest.
> >>
> >>          3>. hvmloader:pci_setup()
> >>
> >>          Here we should populate guest MMIO excluding all ranges
> from that
> >> new
> >>          hypercall to detect RMRR conflictions for allocating PCI MMIO
> BAR.
> >>
> >>          4>. hvmloader:mem_hole_alloc()
> >>
> >>          Here we should populate mem holw excluding all ranges from
> that new
> >>          hypercall to detect RMRR conflictions for dynamic allocation in
> >>          hvmloader, e.g. as used for IGD opregion
> >>
> >>          5>. hvmloader:build_e820_table():
> >>
> >>          Finally we need let VM know that RMRR regions are reserved
> through
> >> e820
> >>          table
> >>
> >>      Its a little bit tricky to handle this inside hvmloader since as you
> >> know,
> >>      struct hvm_info_table is only a approach between libxc and
> hvmloader.
> >> But
> >>      however, making up all above places will bring some duplicated
> logic.
> >>      Especially between libxc and hvmloader, which skip same regions in
> guest
> >>      physical layer(one in populating guest RAM, the other in
> constructing
> >> e820)
> >>      But current design has some limitation to pass information between
> libxc
> >> and
> >>      hvmloader,
> >>
> >> struct hvm_info_table {
> >>      ...
> >>      /*
> >>       *  0x0 to page_to_phys(low_mem_pgend)-1:
> >>       *    RAM below 4GB (except for VGA hole 0xA0000-0xBFFFF)
> >>       */
> >>      uint32_t    low_mem_pgend;
> >>      ...
> >>      /*
> >>       *  0x100000000 to page_to_phys(high_mem_pgend)-1:
> >>       *    RAM above 4GB
> >>       */
> >>      uint32_t    high_mem_pgend;
> >>      ...
> >> }
> >>
> >>      nothing valuable is passed to hvmloader so we have to figure out a
> way
> >> to
> >>      handle those points in hvmloader. Currently,
> >>
> >>              #1 Reconstruct hvm_info_table{}
> >>
> >>              We can store all necessary RMRR info into hvm_info_table
> as
> >> well,
> >>              then we can pick them conveniently but oftentimes these
> RMRR
> >>              entries are scattered and the number is also undertimined,
> so
> >> its
> >>              a little bit ugly to do.
> >>
> >>              #2 Xenstore
> >>
> >>              We may store all necessary RMRR info as Xenstore then
> pick them
> >> in
> >>              hvmloader.
> >>
> >>              #3 A hypercall
> >>
> >>              We may have to call our new hypercall again to pick them,
> but
> >>              obviously this may introduce some duplicated codes.
> >>
> >>              #4 XENMEM_{set_,}memory_map pair of hypercalls
> >>
> >>              As Jan commented it "could be used(and is readily
> available to
> >> be
> >>              extended that way, since for HVM domains
> XENMEM_set_memory_map
> >>              returns -EPERM at present). The only potentially
> problematic
> >> aspect
> >>              I can see with using it might be its limiting of the entry
> count
> >> to
> >>              E820MAX." So a preparation patch is required before
> RMRR, and
> >>              hvmloader still needs to query RMRR information.
> >
> > Somewhere above I'm missing the mentioning of the option to reorder
> > operations of hvmloader, to e.g. base PCI BAR assignment on the
> > previously set up E820 table, rather than assuming a (mostly) fixed
> > size window where to put these.
> >
> >> 3. Group multiple devices sharing same RMRR entry
> >>
> >> current status:
> >>      * Xen doesn't distinguish if multiple devices share same RMRR entry.
> >> This
> >>        may lead a leak to corruption, e.g. Device A and device B share
> same
> >> entry
> >>        and then device A is assigned to domain A, device B is assigned to
> >> domain
> >>        B. So domain A can read something from that range, even rewrite
> >>        maliciously that range to corrupt device B inside domain B.
> >>
> >> proposal:
> >>      * We will group all devices by means of one same RMRR entry.
> >> Theoretically,
> >>        we should make sure all devices in one group are allowed to
> assign to
> >> one
> >>        VM. But in Xen side the hardware domain owns all devices at first,
> we
> >>        should unbound all group devies before assign one of group device.
> But
> >> its
> >>        hard to do such thing in current VT-d stuff. And actually its rare 
> >> to
> >> have
> >>        the group device in real world so we just prevent assigning any
> group
> >>        device simply.
> >>      * We will introduce two field, gid, in struct, acpi_rmrr_unit:
> >>          gid: indicate if this device belongs a group.
> >>        Then when we add or attach device in iommu side, we will check
> this
> >> field
> >>        if we're assigning a group device then determine if we assign that.
> >>
> >>
> >> expectation:
> >>      * Make all device associated to one RMRR to be assigned same VM.
> >
> > A few lines up you say you're intending to refuse such assignments
> > rather than making them happen in a consistent way - what's the
> > plan in the end?
> >
> >> 4. Handle in case of force accessing to RMRR regions
> >>
> >> proposal:
> >>      * Although we already reserve these ranges in guest e820 table, its
> >>        possible to access these ranges.
> >>        In non-shared ept case it will issue such EPT violation since we
> have
> >> no
> >>        p2m table actually. But its same to access other reserved range so
> >> just
> >>        live on Xen's default behavior. In shared-ept case guest can read
> or
> >>        write anything from this range, but such a leak or corruption just
> >>        happens inside same domain so this behavior is also same as a
> native
> >>        case, it should be fine.
> >>
> >> expectation:
> >>      * Don't broken normal RMRR usage.
> >
> > I'm not getting the purpose of this whole section.
> >
> >> 5. Re-enable USB
> >>
> >> current status:
> >>      * Currently Xen doesn't allow USB passthrough.
> >
> > ???
> >
> >> 6. Hotplug case
> >>
> >> current status:
> >>      * only work well in non-shared ept case or in case of shared ept & all
> >>        associated gfns are free.
> >>
> >> proposal:
> >>      * If user ensure there'll be a hotplug device in advace,
> >> 'rdm_forcecheck'
> >>        can be set to reserve all RMRR range as we describe above. Then
> any
> >>        device can be hotplugged successfully.
> >>      * If not, there are still two scenarios here:
> >>        in non-shared ept case it still work well as original;
> >
> > So you continue to assume that the two tables going out of sync is
> > acceptable.
> >
> >>        in shared ept case, its just going a case of "1. Setup RMRR identity
> >>        mapping"
> >>          - gfn space unoccupied -> insert mapping: success.
> >>          - gfn space already occupied by 1:1 RMRR mapping -> do
> nothing;
> >> success.
> >>          - gfn space already occupied by other mapping -> fail.
> >>
> >> expectation:
> >>      * provide a way to let hotplug work in case of shared ept totally
> >>
> >> Open Issue
> >> ==========
> >>
> >> When other stuffs like ballon mechanism, to populate memory accessing
> RMRR
> >> range, or others like qemu, force map RMRR range, we may need to avoid
> such
> >> a
> >> possibility but we're not 100% sure this so just open this to double check.
> >
> > I think a HVM guest can be expected to play by what E820 table
> > says is reserved. That includes not using such ranges for ballooning.
> > If it still does, it'll get what it deserves.
> >
> > Jan
> >

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