[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [XEN v9 2/4] xen/arm64: io: Support instructions (for which ISS is not valid) on emulated MMIO region using MMIO/ioreq handler



Hi,

On 04/03/2022 11:27, Ayan Kumar Halder wrote:

+        {
+            rc = decode_instruction(regs, info);
+            if ( rc )
+            {
+                gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
+                info->dabt_instr.state = INSTR_ERROR;
+        }
+        return;
+    }
+
+    /*
+     * Armv8 processor does not provide a valid syndrome for decoding some +     * instructions. So in order to process these instructions, Xen must
+     * decode them.
+     */
+    rc = decode_instruction(regs, info);
+    if ( rc )
+    {
+        gprintk(XENLOG_ERR, "Unable to decode instruction\n");
+        info->dabt_instr.state = INSTR_ERROR;
+    }
+}
+
  enum io_state try_handle_mmio(struct cpu_user_regs *regs,
-                              const union hsr hsr,
-                              paddr_t gpa)
+                              mmio_info_t *info)
  {
      struct vcpu *v = current;
      const struct mmio_handler *handler = NULL;
-    const struct hsr_dabt dabt = hsr.dabt;
-    mmio_info_t info = {
-        .gpa = gpa,
-        .dabt = dabt
-    };
+    int rc;
  -    ASSERT(hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL);
+    ASSERT(info->dabt.ec == HSR_EC_DATA_ABORT_LOWER_EL);
  -    handler = find_mmio_handler(v->domain, info.gpa);
-    if ( !handler )
+    if ( !((info->dabt_instr.state == INSTR_VALID) || (info->dabt_instr.state == INSTR_LDR_STR_POSTINDEXING)) )

This check will become quite large if we decode more class. I would instead set the dabt.valid bit whenever we successfully decoded the instruction and check that if dabt.valid here.

Actually the main reason to introduce INSTR_LDR_STR_POSTINDEXING is to distinguish the scenario where the ISS was valid vs when instruction was decoded manually.

In the later scenario, one would need to do the post increment of the rn.

It makes sense to me to have a unque 'info->dabt_instr.state' for each type of instruction decoded as the post processing will vary. In this case, the post processing logic checks that the instruction is ldr_str_postindexing.

So I agree we want to have a unique state for type of instruction. I wasn't suggesting to remove it. Instead, I was suggesting to use dabt.valid as "This is a valid instruction for accessing an emulated MMIO".


However your concern that the check will become large is valid. I would introduce a function as follows :-

bool check_instr_is_valid(enum instr_decode_state state)

{

    if (state == INSTR_VALID) || (state == INSTR_LDR_STR_POSTINDEXING) || ...)

         return true;

     else

         return false;

}

And then in

enum io_state try_handle_mmio(struct cpu_user_regs *regs, ...)

{

...

     if ( !check_instr_is_valid(info->dabt_instr.state) )

     {

         ASSERT_UNREACHABLE();
         return IO_ABORT;

     }

...

}

Please let me know your thoughts,

This is only moving the check to a separate function. It doesn't help with the fact that the check in check_instr_is_valid() is going to grow.

I can see two options:
  * Using dabt.valid as "The instruction was fully decoded".
  * Check that the state is not INSTR_ERROR

Above, I was suggesting the former. But I am open to use latter.

Cheers,

--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.