Re: [Xen-devel] [PATCH] xen: workaround for 64-bit size/alignment bitops

[Pranavkumar Sawargaonkar <pranavkumar@xxxxxxxxxx>]

On 27 April 2014 14:39, Vladimir Murzin <murzin.v@xxxxxxxxx> wrote:
> Xen assumes that bit operations are able to operate on 32-bit size and
> alignment. For arm64 bitops are based on atomic exclusive load/store
> instructions to guarantee that changes are made atomically. However, these
> instructions require that address to be aligned to the data size. Because, by
> default, bitops operates on 64-bit size it implies that address should be
> aligned appropriately. All these lead to breakage of Xen assumption for bitops
> properties.
>
> As a workaround address is aligned forcefully along with adjustment for bit
> position.
>
> Cc: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
> Cc: David Vrabel <david.vrabel@xxxxxxxxxx>
>
> Signed-off-by: Vladimir Murzin <murzin.v@xxxxxxxxx>
> ---
>
>  I followed the way suggested by Ian for making workaround on case side. Not
>  sure how much it might affect other parts, but I was able to boot both Dom0
>  and DomU with this patch applied. Additionally, I've folded my other
>  "starter" patch [1].
>
>  @Pranav: Could I have your Tested-by on this patch?
>
>  [1] http://lists.xen.org/archives/html/xen-devel/2014-04/msg02109.html
>
>  drivers/xen/events/events_fifo.c | 30 ++++++++++++++++++------------
>  1 file changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/xen/events/events_fifo.c 
> b/drivers/xen/events/events_fifo.c
> index 96109a9..bff9841 100644
> --- a/drivers/xen/events/events_fifo.c
> +++ b/drivers/xen/events/events_fifo.c
> @@ -66,7 +66,9 @@ static DEFINE_PER_CPU(struct evtchn_fifo_queue, cpu_queue);
>  static event_word_t *event_array[MAX_EVENT_ARRAY_PAGES] __read_mostly;
>  static unsigned event_array_pages __read_mostly;
>
> -#define BM(w) ((unsigned long *)(w))
> +
> +#define BM(w) (unsigned long *)((unsigned long)w & ~0x7UL)
> +#define EVTCHN_FIFO_BIT(b, w) (((unsigned long)w & 0x4UL) ? (EVTCHN_FIFO_ 
> ##b + 32) : EVTCHN_FIFO_ ##b)
>
>  static inline event_word_t *event_word_from_port(unsigned port)
>  {
> @@ -161,33 +163,38 @@ static void evtchn_fifo_bind_to_cpu(struct irq_info 
> *info, unsigned cpu)
>  static void evtchn_fifo_clear_pending(unsigned port)
>  {
>         event_word_t *word = event_word_from_port(port);
> -       sync_clear_bit(EVTCHN_FIFO_PENDING, BM(word));
> +       sync_clear_bit(EVTCHN_FIFO_BIT(PENDING, word), BM(word));
>  }
>
>  static void evtchn_fifo_set_pending(unsigned port)
>  {
>         event_word_t *word = event_word_from_port(port);
> -       sync_set_bit(EVTCHN_FIFO_PENDING, BM(word));
> +       sync_set_bit(EVTCHN_FIFO_BIT(PENDING, word), BM(word));
>  }
>
>  static bool evtchn_fifo_is_pending(unsigned port)
>  {
>         event_word_t *word = event_word_from_port(port);
> -       return sync_test_bit(EVTCHN_FIFO_PENDING, BM(word));
> +       return sync_test_bit(EVTCHN_FIFO_BIT(PENDING, word), BM(word));
>  }
>
>  static bool evtchn_fifo_test_and_set_mask(unsigned port)
>  {
>         event_word_t *word = event_word_from_port(port);
> -       return sync_test_and_set_bit(EVTCHN_FIFO_MASKED, BM(word));
> +       return sync_test_and_set_bit(EVTCHN_FIFO_BIT(MASKED, word), BM(word));
>  }
>
>  static void evtchn_fifo_mask(unsigned port)
>  {
>         event_word_t *word = event_word_from_port(port);
> -       sync_set_bit(EVTCHN_FIFO_MASKED, BM(word));
> +       sync_set_bit(EVTCHN_FIFO_BIT(MASKED, word), BM(word));
>  }
>
> +static bool evtchn_fifo_is_masked(unsigned port)
> +{
> +       event_word_t *word = event_word_from_port(port);
> +       return sync_test_bit(EVTCHN_FIFO_BIT(MASKED, word), BM(word));
> +}
>  /*
>   * Clear MASKED, spinning if BUSY is set.
>   */
> @@ -211,7 +218,7 @@ static void evtchn_fifo_unmask(unsigned port)
>         BUG_ON(!irqs_disabled());
>
>         clear_masked(word);
> -       if (sync_test_bit(EVTCHN_FIFO_PENDING, BM(word))) {
> +       if (evtchn_fifo_is_pending(port)) {
>                 struct evtchn_unmask unmask = { .port = port };
>                 (void)HYPERVISOR_event_channel_op(EVTCHNOP_unmask, &unmask);
>         }
> @@ -243,7 +250,7 @@ static void handle_irq_for_port(unsigned port)
>
>  static void consume_one_event(unsigned cpu,
>                               struct evtchn_fifo_control_block *control_block,
> -                             unsigned priority, uint32_t *ready)
> +                             unsigned priority, unsigned long *ready)
>  {
>         struct evtchn_fifo_queue *q = &per_cpu(cpu_queue, cpu);
>         uint32_t head;
> @@ -273,10 +280,9 @@ static void consume_one_event(unsigned cpu,
>          * copy of the ready word.
>          */
>         if (head == 0)
> -               clear_bit(priority, BM(ready));
> +               clear_bit(priority, ready);
>
> -       if (sync_test_bit(EVTCHN_FIFO_PENDING, BM(word))
> -           && !sync_test_bit(EVTCHN_FIFO_MASKED, BM(word)))
> +       if (evtchn_fifo_is_pending(port) && !evtchn_fifo_is_masked(port))
>                 handle_irq_for_port(port);
>
>         q->head[priority] = head;
> @@ -285,7 +291,7 @@ static void consume_one_event(unsigned cpu,
>  static void evtchn_fifo_handle_events(unsigned cpu)
>  {
>         struct evtchn_fifo_control_block *control_block;
> -       uint32_t ready;
> +       unsigned long ready;
>         unsigned q;
>
>         control_block = per_cpu(cpu_control_block, cpu);
> --
> 1.8.3.2
>

I have tried this patch on XGENE and works fine, I have managed to get
dom0 and domU working with it.

Tested-by: Pranavkumar Sawargaonkar <pranavkumar@xxxxxxxxxx>

Thanks,
Pranav

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