Re: [PATCH v3 01/11] x86/hvm: drop vcpu parameter from vlapic EOI callbacks

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Apr 01, 2021 at 01:06:35PM +0200, Jan Beulich wrote:
> On 31.03.2021 12:32, Roger Pau Monne wrote:
> > EOIs are always executed in guest vCPU context, so there's no reason to
> > pass a vCPU parameter around as can be fetched from current.
> 
> While not overly problematic, I'd like to point out that there's not a
> single vcpu parameter being dropped here - in both cases it's struct
> domain *.
> 
> > --- a/xen/arch/x86/hvm/vlapic.c
> > +++ b/xen/arch/x86/hvm/vlapic.c
> > @@ -459,13 +459,10 @@ void vlapic_EOI_set(struct vlapic *vlapic)
> >  
> >  void vlapic_handle_EOI(struct vlapic *vlapic, u8 vector)
> >  {
> > -    struct vcpu *v = vlapic_vcpu(vlapic);
> > -    struct domain *d = v->domain;
> > -
> >      if ( vlapic_test_vector(vector, &vlapic->regs->data[APIC_TMR]) )
> > -        vioapic_update_EOI(d, vector);
> > +        vioapic_update_EOI(vector);
> >  
> > -    hvm_dpci_msi_eoi(d, vector);
> > +    hvm_dpci_msi_eoi(vector);
> >  }
> 
> The Viridian path pointed out before was only an example. I'm afraid
> the call from vlapic_has_pending_irq() to vlapic_EOI_set() is also
> far from obvious that it always has "v == current". What we end up
> with here is a mix of passed in value (vlapic) and assumption of the
> call being for the vCPU / domain we're running on. At the very least
> I think this would want documenting here in some way (maybe ASSERT(),
> definitely mentioning in the description), but even better would
> perhaps be if the parameter of the function here as well as further
> ones involved would also be dropped then.

I've kind of attempted to purge the vlapic parameter further, but the
proper way to do it would be to audit all vlapic functions.

For example I've removed the parameter from vlapic_EOI_set and
vlapic_handle_EOI, but I'm afraid that would also raise questions
about purging it vlapic_has_pending_irq for example.

Let me know if the patch below would be acceptable, or if I should
rather not make the EOI callbacks depends on this cleanup, as I could
certainly do the cleanup later.

Thanks, Roger.
---8<---
>From 4719f5a827d3154ef763e078956792855ca84e04 Mon Sep 17 00:00:00 2001
From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
Date: Tue, 18 Aug 2020 16:30:06 +0200
Subject: [PATCH] x86/hvm: drop vlapic parameter from vlapic EOI helpers
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

EOIs are always executed in guest vCPU context, so there's no reason to
pass a vlapic parameter around as can be fetched from current.

This also allows to drop the domain parameter from the EOI callbacks,
and while there make the vector parameter of both callbacks unsigned
int.

The callers from vmx.c (vmx_handle_eoi_write, vmx_vmexit_handler) are
clearly using v == current, as one is the vmexit handler, and the
other is called directly from such handler.

The only caller from Viridian code halso has a check that v == current
before calling vlapic_EOI_set - so it's safe.

Finally the callers from vlapic.c itself: vlapic_reg_write will only
get called with the APIC_EOI register as a parameter from the MMIO/MSR
intercepts which run in guest context and vlapic_has_pending_irq only
gets called as part of the vm entry path to guest.

No functional change intended.

Suggested-by: Paul Durrant <pdurrant@xxxxxxxxxx>
Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
---
Changes since v3:
 - Purge passing the vcpu in the vlapic eoi call paths.

Changes since v1:
 - New in this version.
---
 xen/arch/x86/hvm/vioapic.c        |  5 +++--
 xen/arch/x86/hvm/viridian/synic.c |  2 +-
 xen/arch/x86/hvm/vlapic.c         | 31 +++++++++++++++++++++----------
 xen/arch/x86/hvm/vmx/vmx.c        |  4 ++--
 xen/drivers/passthrough/x86/hvm.c |  4 +++-
 xen/include/asm-x86/hvm/io.h      |  2 +-
 xen/include/asm-x86/hvm/vioapic.h |  2 +-
 xen/include/asm-x86/hvm/vlapic.h  |  4 ++--
 8 files changed, 34 insertions(+), 20 deletions(-)

diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index 87370dd4172..91e5f892787 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -372,7 +372,7 @@ static int vioapic_write(
 
 #if VIOAPIC_VERSION_ID >= 0x20
     case VIOAPIC_REG_EOI:
-        vioapic_update_EOI(v->domain, val);
+        vioapic_update_EOI(val);
         break;
 #endif
 
@@ -514,8 +514,9 @@ void vioapic_irq_positive_edge(struct domain *d, unsigned 
int irq)
     }
 }
 
-void vioapic_update_EOI(struct domain *d, u8 vector)
+void vioapic_update_EOI(unsigned int vector)
 {
+    struct domain *d = current->domain;
     struct hvm_irq *hvm_irq = hvm_domain_irq(d);
     union vioapic_redir_entry *ent;
     unsigned int i;
diff --git a/xen/arch/x86/hvm/viridian/synic.c 
b/xen/arch/x86/hvm/viridian/synic.c
index e18538c60a6..33bd9c9bd13 100644
--- a/xen/arch/x86/hvm/viridian/synic.c
+++ b/xen/arch/x86/hvm/viridian/synic.c
@@ -93,7 +93,7 @@ int viridian_synic_wrmsr(struct vcpu *v, uint32_t idx, 
uint64_t val)
             ASSERT_UNREACHABLE();
             return X86EMUL_EXCEPTION;
         }
-        vlapic_EOI_set(vcpu_vlapic(v));
+        vlapic_EOI_set();
         break;
 
     case HV_X64_MSR_ICR:
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 5e21fb4937d..111b6d902f5 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -413,9 +413,10 @@ struct vlapic *vlapic_lowest_prio(
     return target;
 }
 
-void vlapic_EOI_set(struct vlapic *vlapic)
+void vlapic_EOI_set(void)
 {
-    struct vcpu *v = vlapic_vcpu(vlapic);
+    struct vcpu *v = current;
+    struct vlapic *vlapic = vcpu_vlapic(v);
     /*
      * If APIC assist was set then an EOI may have been avoided and
      * hence this EOI actually relates to a lower priority vector.
@@ -448,7 +449,7 @@ void vlapic_EOI_set(struct vlapic *vlapic)
         alternative_vcall(hvm_funcs.handle_eoi, vector,
                           vlapic_find_highest_isr(vlapic));
 
-    vlapic_handle_EOI(vlapic, vector);
+    vlapic_handle_EOI(vector);
 
     if ( missed_eoi )
     {
@@ -457,15 +458,14 @@ void vlapic_EOI_set(struct vlapic *vlapic)
     }
 }
 
-void vlapic_handle_EOI(struct vlapic *vlapic, u8 vector)
+void vlapic_handle_EOI(uint8_t vector)
 {
-    struct vcpu *v = vlapic_vcpu(vlapic);
-    struct domain *d = v->domain;
+    struct vlapic *vlapic = vcpu_vlapic(current);
 
     if ( vlapic_test_vector(vector, &vlapic->regs->data[APIC_TMR]) )
-        vioapic_update_EOI(d, vector);
+        vioapic_update_EOI(vector);
 
-    hvm_dpci_msi_eoi(d, vector);
+    hvm_dpci_msi_eoi(vector);
 }
 
 static bool_t is_multicast_dest(struct vlapic *vlapic, unsigned int short_hand,
@@ -796,7 +796,12 @@ void vlapic_reg_write(struct vcpu *v, unsigned int reg, 
uint32_t val)
         break;
 
     case APIC_EOI:
-        vlapic_EOI_set(vlapic);
+        if ( v != current )
+        {
+            ASSERT_UNREACHABLE();
+            break;
+        }
+        vlapic_EOI_set();
         break;
 
     case APIC_LDR:
@@ -1303,6 +1308,12 @@ int vlapic_has_pending_irq(struct vcpu *v)
     struct vlapic *vlapic = vcpu_vlapic(v);
     int irr, isr;
 
+    if ( v != current )
+    {
+        ASSERT_UNREACHABLE();
+        return -1;
+    }
+
     if ( !vlapic_enabled(vlapic) )
         return -1;
 
@@ -1327,7 +1338,7 @@ int vlapic_has_pending_irq(struct vcpu *v)
      * with IRR.
      */
     if ( viridian_apic_assist_completed(v) )
-        vlapic_EOI_set(vlapic);
+        vlapic_EOI_set();
 
     isr = vlapic_find_highest_isr(vlapic);
 
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index b52824677e9..22e406285cf 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3715,7 +3715,7 @@ static int vmx_handle_eoi_write(void)
          ((exit_qualification & 0xfff) == APIC_EOI) )
     {
         update_guest_eip(); /* Safe: APIC data write */
-        vlapic_EOI_set(vcpu_vlapic(current));
+        vlapic_EOI_set();
         HVMTRACE_0D(VLAPIC);
         return 1;
     }
@@ -4364,7 +4364,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
 
         ASSERT(cpu_has_vmx_virtual_intr_delivery);
 
-        vlapic_handle_EOI(vcpu_vlapic(v), exit_qualification);
+        vlapic_handle_EOI(exit_qualification);
         break;
 
     case EXIT_REASON_IO_INSTRUCTION:
diff --git a/xen/drivers/passthrough/x86/hvm.c 
b/xen/drivers/passthrough/x86/hvm.c
index 351daafdc9b..2f6c81b1e2c 100644
--- a/xen/drivers/passthrough/x86/hvm.c
+++ b/xen/drivers/passthrough/x86/hvm.c
@@ -796,8 +796,10 @@ static int _hvm_dpci_msi_eoi(struct domain *d,
     return 0;
 }
 
-void hvm_dpci_msi_eoi(struct domain *d, int vector)
+void hvm_dpci_msi_eoi(unsigned int vector)
 {
+    struct domain *d = current->domain;
+
     if ( !is_iommu_enabled(d) ||
          (!hvm_domain_irq(d)->dpci && !is_hardware_domain(d)) )
        return;
diff --git a/xen/include/asm-x86/hvm/io.h b/xen/include/asm-x86/hvm/io.h
index 54e0161b492..8b8392ec59e 100644
--- a/xen/include/asm-x86/hvm/io.h
+++ b/xen/include/asm-x86/hvm/io.h
@@ -142,7 +142,7 @@ struct hvm_hw_stdvga {
 void stdvga_init(struct domain *d);
 void stdvga_deinit(struct domain *d);
 
-extern void hvm_dpci_msi_eoi(struct domain *d, int vector);
+extern void hvm_dpci_msi_eoi(unsigned int vector);
 
 /* Decode a PCI port IO access into a bus/slot/func/reg. */
 unsigned int hvm_pci_decode_addr(unsigned int cf8, unsigned int addr,
diff --git a/xen/include/asm-x86/hvm/vioapic.h 
b/xen/include/asm-x86/hvm/vioapic.h
index 36b64d20d60..882548c13b7 100644
--- a/xen/include/asm-x86/hvm/vioapic.h
+++ b/xen/include/asm-x86/hvm/vioapic.h
@@ -63,7 +63,7 @@ int vioapic_init(struct domain *d);
 void vioapic_deinit(struct domain *d);
 void vioapic_reset(struct domain *d);
 void vioapic_irq_positive_edge(struct domain *d, unsigned int irq);
-void vioapic_update_EOI(struct domain *d, u8 vector);
+void vioapic_update_EOI(unsigned int vector);
 
 int vioapic_get_mask(const struct domain *d, unsigned int gsi);
 int vioapic_get_vector(const struct domain *d, unsigned int gsi);
diff --git a/xen/include/asm-x86/hvm/vlapic.h b/xen/include/asm-x86/hvm/vlapic.h
index 8f908928c35..b693696eccb 100644
--- a/xen/include/asm-x86/hvm/vlapic.h
+++ b/xen/include/asm-x86/hvm/vlapic.h
@@ -133,8 +133,8 @@ uint32_t vlapic_set_ppr(struct vlapic *vlapic);
 
 void vlapic_adjust_i8259_target(struct domain *d);
 
-void vlapic_EOI_set(struct vlapic *vlapic);
-void vlapic_handle_EOI(struct vlapic *vlapic, u8 vector);
+void vlapic_EOI_set(void);
+void vlapic_handle_EOI(uint8_t vector);
 
 void vlapic_ipi(struct vlapic *vlapic, uint32_t icr_low, uint32_t icr_high);
 
-- 
2.30.1