Re: [Qemu-ppc] [PATCH 2/3] spapr/xive: fix EQ page addresses above 64GB

[Greg Kurz]

On Wed,  8 May 2019 19:19:45 +0200
Cédric Le Goater <address@hidden> wrote:

> The high order bits of the address of the OS event queue is stored in
> bits [4-31] of word2 of the XIVE END internal structures and the low
> order bits in word3. This structure is using Big Endian ordering and
> computing the value requires some simple arithmetic which happens to
> be wrong. The mask removing bits [0-3] of word2 is applied to the
> wrong value and the resulting address is bogus when above 64GB.
> 
> Guests with more than 64GB of RAM will allocate pages for the OS event
> queues which will reside above the 64GB limit. In this case, the XIVE
> device model will wake up the CPUs in case of a notification, such as
> IPIs, but the update of the event queue will be written at the wrong
> place in memory. The result is uncertain as the guest memory is
> trashed and IPI are not delivered.
> 
> Introduce a helper xive_end_qaddr() to compute this value correctly in
> all places where it is used.
> 
> Signed-off-by: Cédric Le Goater <address@hidden>
> ---

I guess this patch should have a Fixes: tag and Cc qemu-stable as well
since QEMU 4.0 has the issue.

Apart from that, LGTM.

Reviewed-by: Greg Kurz <address@hidden>

>  include/hw/ppc/xive_regs.h | 6 ++++++
>  hw/intc/spapr_xive.c       | 3 +--
>  hw/intc/xive.c             | 9 +++------
>  3 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/include/hw/ppc/xive_regs.h b/include/hw/ppc/xive_regs.h
> index bf36678a242c..1a8c5b5e64f0 100644
> --- a/include/hw/ppc/xive_regs.h
> +++ b/include/hw/ppc/xive_regs.h
> @@ -208,6 +208,12 @@ typedef struct XiveEND {
>  #define xive_end_is_backlog(end)  (be32_to_cpu((end)->w0) & END_W0_BACKLOG)
>  #define xive_end_is_escalate(end) (be32_to_cpu((end)->w0) & 
> END_W0_ESCALATE_CTL)
>  
> +static inline uint64_t xive_end_qaddr(XiveEND *end)
> +{
> +    return ((uint64_t) be32_to_cpu(end->w2) & 0x0fffffff) << 32 |
> +        be32_to_cpu(end->w3);
> +}
> +
>  /* Notification Virtual Target (NVT) */
>  typedef struct XiveNVT {
>          uint32_t        w0;
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index 666e24e9b447..810435c30cc7 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -1150,8 +1150,7 @@ static target_ulong h_int_get_queue_config(PowerPCCPU 
> *cpu,
>      }
>  
>      if (xive_end_is_enqueue(end)) {
> -        args[1] = (uint64_t) be32_to_cpu(end->w2 & 0x0fffffff) << 32
> -            | be32_to_cpu(end->w3);
> +        args[1] = xive_end_qaddr(end);
>          args[2] = xive_get_field32(END_W0_QSIZE, end->w0) + 12;
>      } else {
>          args[1] = 0;
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index a0b87001da25..dcf2fcd10893 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -1042,8 +1042,7 @@ static const TypeInfo xive_source_info = {
>  
>  void xive_end_queue_pic_print_info(XiveEND *end, uint32_t width, Monitor 
> *mon)
>  {
> -    uint64_t qaddr_base = (uint64_t) be32_to_cpu(end->w2 & 0x0fffffff) << 32
> -        | be32_to_cpu(end->w3);
> +    uint64_t qaddr_base = xive_end_qaddr(end);
>      uint32_t qsize = xive_get_field32(END_W0_QSIZE, end->w0);
>      uint32_t qindex = xive_get_field32(END_W1_PAGE_OFF, end->w1);
>      uint32_t qentries = 1 << (qsize + 10);
> @@ -1072,8 +1071,7 @@ void xive_end_queue_pic_print_info(XiveEND *end, 
> uint32_t width, Monitor *mon)
>  
>  void xive_end_pic_print_info(XiveEND *end, uint32_t end_idx, Monitor *mon)
>  {
> -    uint64_t qaddr_base = (uint64_t) be32_to_cpu(end->w2 & 0x0fffffff) << 32
> -        | be32_to_cpu(end->w3);
> +    uint64_t qaddr_base = xive_end_qaddr(end);
>      uint32_t qindex = xive_get_field32(END_W1_PAGE_OFF, end->w1);
>      uint32_t qgen = xive_get_field32(END_W1_GENERATION, end->w1);
>      uint32_t qsize = xive_get_field32(END_W0_QSIZE, end->w0);
> @@ -1101,8 +1099,7 @@ void xive_end_pic_print_info(XiveEND *end, uint32_t 
> end_idx, Monitor *mon)
>  
>  static void xive_end_enqueue(XiveEND *end, uint32_t data)
>  {
> -    uint64_t qaddr_base = (uint64_t) be32_to_cpu(end->w2 & 0x0fffffff) << 32
> -        | be32_to_cpu(end->w3);
> +    uint64_t qaddr_base = xive_end_qaddr(end);
>      uint32_t qsize = xive_get_field32(END_W0_QSIZE, end->w0);
>      uint32_t qindex = xive_get_field32(END_W1_PAGE_OFF, end->w1);
>      uint32_t qgen = xive_get_field32(END_W1_GENERATION, end->w1);