Re: [XEN v10 4/4] xen/arm64: io: Handle data abort due to cache maintenance instructions

[Julien Grall <julien@xxxxxxx>]

Hi,

On 11/03/2022 13:21, Ayan Kumar Halder wrote:
> enum io_state try_handle_mmio(struct cpu_user_regs *regs,
>                                mmio_info_t *info)
> {
>      struct vcpu *v = current;
>      const struct mmio_handler *handler = NULL;
>      int rc;
>
>      ASSERT(info->dabt.ec == HSR_EC_DATA_ABORT_LOWER_EL);
>
>      if ( !info->dabt.valid )
>      {
>          ASSERT_UNREACHABLE();
>          return IO_ABORT;
>      }
>
>      handler = find_mmio_handler(v->domain, info->gpa);
>      if ( !handler )
>      {
>
> #ifdef CONFIG_IOREQ_SERVER
>
>      struct vcpu_io *vio = &v->io;
>      const struct instr_details instr = info->dabt_instr;
>      struct hsr_dabt dabt = info->dabt;
>      ioreq_t p = {
>          .type = IOREQ_TYPE_COPY,
>          .addr = info->gpa,
>          .size = 1 << info->dabt.size,
>          .count = 1,
>          .dir = !info->dabt.write,
>          /*
>           * On x86, df is used by 'rep' instruction to tell the direction
>           * to iterate (forward or backward).
>           * On Arm, all the accesses to MMIO region will do a single
>           * memory access. So for now, we can safely always set to 0.
>           */
>          .df = 0,
>          .data = get_user_reg(regs, info->dabt.reg),
>          .state = STATE_IOREQ_READY,
>      };
>      struct ioreq_server *s = NULL;
>      enum io_state rc;
>
>      if ( vio->req.state != STATE_IOREQ_NONE )
>      {
>          gdprintk(XENLOG_ERR, "wrong state %u\n", vio->req.state);
>          return IO_ABORT;
>      }
>
>      s = ioreq_server_select(v->domain, &p);
>      if ( !s )
>          return IO_UNHANDLED;
>
>      if ( instr.state == INSTR_CACHE )
>      {
>          return IO_HANDLED;
>      }
>
>      rc = try_fwd_ioserv(vio, &p, s);
>      if ( rc == IO_HANDLED )
>          return handle_ioserv(regs, v);
>
> #endif
>          return rc;
>      }
>
> I am not be inclined to have "#ifdef CONFIG_IOREQ_SERVER" in 
> xen/xen/arch/arm/io.c as the file is generic.
+1. I much prefer your first approach.

Cheers,

--
Julien Grall