Re: X server in dom0: Bad VBT signature

["Cherry G. Mathew" <cherry.g.mathew%gmail.com@localhost>]

On 2 September 2012 18:48, Manuel Bouyer <bouyer%antioche.eu.org@localhost> 
wrote:
> On Sun, Sep 02, 2012 at 06:36:20PM +0100, Cherry G. Mathew wrote:
>> On 2 September 2012 10:41, Manuel Bouyer <bouyer%antioche.eu.org@localhost> 
>> wrote:
>> > On Sun, Sep 02, 2012 at 08:37:34AM +0100, Cherry G. Mathew wrote:
>> >> Index: arch/xen/include/xenfunc.h
>> >> ===================================================================
>> >> RCS file: /cvsroot/src/sys/arch/xen/include/xenfunc.h,v
>> >> retrieving revision 1.15
>> >> diff -u -r1.15 xenfunc.h
>> >> --- arch/xen/include/xenfunc.h        23 Oct 2009 02:32:33 -0000      1.15
>> >> +++ arch/xen/include/xenfunc.h        2 Sep 2012 07:32:24 -0000
>> >> @@ -43,4 +43,17 @@
>> >>  #endif
>> >>
>> >>  void xen_set_ldt(vaddr_t, uint32_t);
>> >> +
>> >> +#if defined(DOM0OPS)
>> >> +
>> >> +/* This is part of a hack to allow dom0 to access isa i/o "hole"s */
>> >> +static __inline bool
>> >> +xen_dom0_pa_isa_conflict(paddr_t pa)
>> >> +{
>> >> +     extern paddr_t pmap_pa_end;
>> >> +     return __predict_false(pa < 0x100000 || pa >= pmap_pa_end);
>> >> +}
>> >> +
>> >> +#endif /* DOM0OPS */
>> >> +
>> >>  #endif /* _XEN_XENFUNC_H_ */
>> >> Index: arch/x86/x86/pmap.c
>> >> ===================================================================
>> >> RCS file: /cvsroot/src/sys/arch/x86/x86/pmap.c,v
>> >> retrieving revision 1.178
>> >> diff -u -r1.178 pmap.c
>> >> --- arch/x86/x86/pmap.c       15 Jun 2012 13:53:40 -0000      1.178
>> >> +++ arch/x86/x86/pmap.c       2 Sep 2012 07:32:29 -0000
>> >> @@ -985,7 +985,12 @@
>> >>       else
>> >>               pte = kvtopte(va);
>> >>  #ifdef DOM0OPS
>> >> -     if (pa < pmap_pa_start || pa >= pmap_pa_end) {
>> >> +     if (xen_dom0_pa_isa_conflict(pa)) { /* dom0 wants to access ISA mem,
>> >> not ram */
>> >> +             /*
>> >> +              * This is a hack. The right thing to do would be to
>> >> +              * have bus_space_map() et al. manage the p2m/m2p
>> >> +              * tables.
>> >
>> > I don't understand what you mean here; bus_space_map() already properly
>> > calls pmap_enter_ma() with a machine address.
>> > This problem only exists for userland accessing devices through /dev/mem,
>> > because we want /dev/mem to map both physical or machine addresses,
>> > depending on what the caller means. The right fix would be to have a
>> > way to express the address space at this level. But this is intrusive.
>> >
>>
>> yep, I was thinking of a ioctl() initially.
>>
>> I mulled this over a bit, and seeing as you say that only userland is
>> involved, we probably don't need to fit it to bus_space_map()
>>
>> What I mean is:
>> On xen we have ma, pa tuples that are managed by the p2m/m2p tables.
>> The way we export the translation to lookups is via xen specific
>> functions in xenpmap.h
>> We also need to maintain aliased pmap_kenter/extract_pa() et. al
>> symbols between pmap.c and xen_pmap.c
>>
>> We also seem to need to export pmap_kenter_ma() and pmap_enter_ma()
>> which are xen abstractions to native code.
>> I was wondering if it's possible to further cleanly separate the
>> xen/native cases.
>>
>> > Another problem with your patch is that you don't prevent the memory in
>> > range pa < 0x100000 from being used by UVM. So you'll end up with
>>
>> Should I have used: IOM_BEGIN/END instead ?
>
> that won't help: for xen, the available RAM range passed to UVM
> is 0 -> pmap_pa_end (pa_start is, or close to 0, if I remember properly).
> All adresses in this range are assumed to be physical addresses.
> If you want to use part of this range for something that is not RAM,
> you need to exclude this range from the memory map passed to UVM.
> See uvm_page_physload() calls in i386 and amd64 machdep.
>

This was no different from the original code. As far as I understand,
here's what happens (upto machdep.c -r 1.168)
- xen_pmap_bootstrap() returns what roughly becomes avail_start
- avail_start to avail_end is uvm_page_physload()ed as one chunk into uvm
- pmap_pa_start/end are used as shims to intercept pmap_[k]enter() to
demarcate the physical address of RAM. Everything outside of this is
mapped in as machine frames
- Xen seems to load RAM mappings in the physical address space of dom0
(ie; the ISA hole is actual addressable RAM in the p2m/m2p tables of
dom0).

Currently, this last point is the only issue. Hotplug is going to
complicate things, but I'm not going to open that can of worms here.


> And I'm not sure the addresse X wants to map are in the ISA hole.
> it may be in PCI memory space too, I guess.
>

I'm guessing both! ktrace shows opening /dev/pcix for device config
probes and /dev/mem for VRAM (0xa000). This needs to be whittled down
to the lowest possible set of apertures, imho.


>>
>> > regular pages mapped in device's memory space, which will likely lead to
>> > panic or spontaneous reboot.
>> >
>>
>> Will the following fix that: (I know your original code just removed
>> the uvm management of that range entirely, I'm just looking for an
>> alternative implementation)
>
> No, this has nothing to do with bus_space. I guess the memory addresses
> that X wants to use are never seen by bus_space, because no driver
> claims them.

So I recalled where I saw this: dev/pci_usrreq.c

I still think that we can make the abstractions fit better with MI
ones such as bus_space() and remove the redundant alias-ed pmap_xxx()
functions in x86/x86/pmap.c and xen/x86/xen_pmap.c

I'll see what falls out of the shakedown for hotplug..

Cheers,
-- 
~Cherry