Re: X server in dom0: Bad VBT signature

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

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 ?

> 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)

diff -u -r1.38 bus_space.c
--- arch/x86/x86/bus_space.c    27 Jan 2012 18:53:07 -0000      1.38
+++ arch/x86/x86/bus_space.c    2 Sep 2012 17:29:59 -0000
@@ -209,12 +209,10 @@
                return 0;
        }

-#ifndef XEN
        if (bpa >= IOM_BEGIN && (bpa + size) != 0 && (bpa + size) <= IOM_END) {
                *bshp = (bus_space_handle_t)ISA_HOLE_VADDR(bpa);
                return 0;
        }
-#endif /* !XEN */

        /*
         * For memory space, map the bus physical address to
@@ -445,17 +443,10 @@
                panic("x86_mem_add_mapping: overflow");
 #endif

-#ifdef XEN
-       if (bpa >= IOM_BEGIN && (bpa + size) != 0 && (bpa + size) <= IOM_END) {
-               sva = (vaddr_t)ISA_HOLE_VADDR(pa);
-       } else
-#endif /* XEN */
-       {
-               sva = uvm_km_alloc(kernel_map, endpa - pa, 0,
-                   UVM_KMF_VAONLY | UVM_KMF_NOWAIT);
-               if (sva == 0)
-                       return (ENOMEM);
-       }
+       sva = uvm_km_alloc(kernel_map, endpa - pa, 0,
+           UVM_KMF_VAONLY | UVM_KMF_NOWAIT);
+       if (sva == 0)
+               return (ENOMEM);

        *bshp = (bus_space_handle_t)(sva + (bpa & PGOFSET));


-- 
~Cherry