Re: [Xen-devel] Help with test_and_clear_bit on events

[Daniel Castro <evil.dani@xxxxxxxxx>]

On Tue, Oct 4, 2011 at 11:11 PM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> On Tue, 2011-10-04 at 14:19 +0100, Ian Campbell wrote:
>
> Two more bugs:
>
> A domid is 16 bits not 64, see struct xen_add_to_physmap. So the
> structure passed to the hypercall doesn't line up and you don't really
> end up with the shared info in the right place.
>
> Secondly in:
>
>        int wait = test_and_clear_bit(event, shinfo->evtchn_pending);
>        int ret = 1;
>        while (!wait || ret){
>                ret = hypercall_sched_op(SCHEDOP_poll, &poll);
>                wait = test_and_clear_bit(event, shinfo->evtchn_pending);
>        }
>
> This will always go through the loop at least once, even if wait is
> already true because ret == 1. But if wait was already one then you have
> cleared the bit so wait will then become 0 on the first time through the
> loop and since you won't get any more events you get stuck in the loop.
> I think you meant && !ret or something, but really you should be doing
> BUG on !ret.
>
> With all those fixed things seem to work for me. I do
> xenstore-write /local/domain/$(xl domid dHVM-1)/device/vbd/768/test foo
> twice to get through the test code and it continues on to boot the
> guest.

Thank you Ian, it works flawlessly... Sorry about the bug. Github repo
is up to date.
With this, do you consider that any other xenbus functionality should
be added? It can read, write, directory and watch. Maybe, unwatch?

Shall I continue with the blk rings?

>
> My patch (with debug and hacks included) is below.
>
> Ian.
>
> diff --git a/src/xen-xs.c b/src/xen-xs.c
> index 9f583f9..4c208d4 100644
> --- a/src/xen-xs.c
> +++ b/src/xen-xs.c
> @@ -126,14 +126,21 @@ static void ring_wait(void)
>        memset(&poll, 0, sizeof(poll));
>        set_xen_guest_handle(poll.ports, &event);
>        poll.nr_ports = 1;
> -       dprintf(1,"evtchn_pending 0x%p , 0x%lx at event %d 
> \n",shinfo->evtchn_pending,shinfo->evtchn_pending[event],event);
> +       dprintf(1, "shared info %p\n", shinfo);
> +       dprintf(1,"evtchn_pending %p , 0x%lx at event %d 
> \n",shinfo->evtchn_pending,shinfo->evtchn_pending[0],event);
> +       while ( !test_and_clear_bit(event, shinfo->evtchn_pending) )
> +               hypercall_sched_op(SCHEDOP_poll, &poll);
> +
> +#if 0
>        int wait = test_and_clear_bit(event, shinfo->evtchn_pending);
> -       int ret = 1;
> +       int ret = 0;
> +       dprintf(1,"DEBUG bit clear is %d and ret %d\n",wait,ret);
>        while (!wait || ret){
>                ret = hypercall_sched_op(SCHEDOP_poll, &poll);
>                wait = test_and_clear_bit(event, shinfo->evtchn_pending);
>                dprintf(1,"DEBUG bit clear is %d and ret %d\n",wait,ret);
>        }
> +#endif
>  }
>
>  static void ring_wait2(void)
> diff --git a/src/xen.c b/src/xen.c
> index e8469b0..312c206 100644
> --- a/src/xen.c
> +++ b/src/xen.c
> @@ -154,11 +154,20 @@ struct shared_info *get_shared_info(void)
>     xatp.domid = DOMID_SELF;
>     xatp.space = XENMAPSPACE_shared_info;
>     xatp.idx   = 0;
> -    shared_info = malloc_high(sizeof(shared_info));
> -    xatp.gpfn  = ((unsigned long)shared_info << PAGE_SHIFT);
> +    shared_info = memalign_high(PAGE_SIZE, PAGE_SIZE);

Here was my big mistake...

> +    memset(shared_info, 0, PAGE_SIZE);
> +    xatp.gpfn  = ((unsigned long)shared_info >> PAGE_SHIFT);
> +    dprintf(1, "allocated shared info %d bytes at %p, gpfn 0x%lx\n",
> +           sizeof(*shared_info), shared_info, xatp.gpfn);
>     //xatp.gpfn  = malloc_high(sizeof(shared_info));
>     //shared_info = (struct shared_info *)(xatp.gpfn << PAGE_SHIFT);
>     if (hypercall_memory_op(XENMEM_add_to_physmap, &xatp) != 0)
>         panic("MAP info page fail");
> +    dprintf(1, "time is %d\n", shared_info->wc_sec);
> +    dprintf(1, "evtchn_pending[0] 0x%08lx\n", 
> shared_info->evtchn_pending[0]);
> +    dprintf(1, "evtchn_mask[0] 0x%08lx\n", shared_info->evtchn_mask[0]);
> +    dprintf(1, "VCPU0 evtchn_upcall_pending 0x%x\n", 
> shared_info->vcpu_info[0].evtchn_upcall_pending);
> +    dprintf(1, "VCPU0 evtchn_upcall_mask 0x%x\n", 
> shared_info->vcpu_info[0].evtchn_upcall_mask);
> +    dprintf(1, "VCPU0 evtchn_pending_sel 0x%08lx\n", 
> shared_info->vcpu_info[0].evtchn_pending_sel);
>     return shared_info;
>  }
> diff --git a/src/xen.h b/src/xen.h
> index 1317af9..d838a7c 100644
> --- a/src/xen.h
> +++ b/src/xen.h
> @@ -480,7 +480,7 @@ static struct xsd_errors __attribute__ ((unused)) 
> xsd_errors[]
>  #define XENMEM_add_to_physmap      7
>  struct xen_add_to_physmap {
>     /* Which domain to change the mapping for. */
> -    u64 domid;
> +    u16 domid;
>
>     /* Source mapping space. */
>  #define XENMAPSPACE_shared_info 0 /* shared info page */
> @@ -503,7 +503,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_add_to_physmap_t);
>  /*
>  * Wrappers for hypercalls
>  */
> -static int hypercall_xen_version( int cmd, void *arg)
> +static inline int hypercall_xen_version( int cmd, void *arg)
>  {
>        return _hypercall2(int, xen_version, cmd, arg);
>  }
> @@ -520,7 +520,7 @@ static inline int hypercall_event_channel_op(int cmd, 
> void *arg)
>
>  static inline int hypercall_memory_op(int cmd ,void *arg)
>  {
> -       return _hypercall2(int, memory_op, cmd ,arg);
> +       return _hypercall2(int, memory_op, cmd, arg);
>  }
>
>  static inline int hypercall_sched_op(int cmd, void *arg)
>
>
>



-- 
+-=====---------------------------+
| +---------------------------------+ | This space intentionally blank
for notetaking.
| |   | Daniel Castro,                |
| |   | Consultant/Programmer.|
| |   | U Andes                         |
+-------------------------------------+

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel