| Message ID | 20210331103303.79705-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/intr: introduce EOI callbacks and fix vPT | expand |
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 there make the vector parameter of both callbacks unsigned int. > > No functional change intended. > > Suggested-by: Paul Durrant <pdurrant@amazon.com> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > Reviewed-by: Paul Durrant <paul@xen.org> > --- > Changes since v1: > - New in this version. I'm afraid the situation with viridian_synic_wrmsr() hasn't changed since v2, and hence my previous comment still applies. Jan
On 31/03/2021 17:02, 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 there make the vector parameter of both callbacks unsigned int. >> >> No functional change intended. >> >> Suggested-by: Paul Durrant <pdurrant@amazon.com> >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >> Reviewed-by: Paul Durrant <paul@xen.org> >> --- >> Changes since v1: >> - New in this version. > I'm afraid the situation with viridian_synic_wrmsr() hasn't changed > since v2, and hence my previous comment still applies. Only just spotted that line of reasoning. Longterm, I do want to remove all the Viridian special cases, and handle all of the state via architectural mechanisms (cpu policy for static settings, and the existing MSR records for dynamic content). A consequence of this cleanup is that guest_{rd,wr}msr() will be eventually be used to save/restore dynamic state in the migrate stream, which is why I've been engineering guest_{rd,wr}msr() to work for MSRs in "current || !scheduled" context. As such, it is important to retain a vcpu pointer because it will not be current on the save/restore hypercalls, which execute in control domain context. How much is keeping the vcpu pointer going to interfere with this series? My expectation is that this patch would need reverting to continue the cleanup plans. ~Andrew
On Wed, Mar 31, 2021 at 05:24:24PM +0100, Andrew Cooper wrote: > On 31/03/2021 17:02, 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 there make the vector parameter of both callbacks unsigned int. > >> > >> No functional change intended. > >> > >> Suggested-by: Paul Durrant <pdurrant@amazon.com> > >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > >> Reviewed-by: Paul Durrant <paul@xen.org> > >> --- > >> Changes since v1: > >> - New in this version. > > I'm afraid the situation with viridian_synic_wrmsr() hasn't changed > > since v2, and hence my previous comment still applies. > > Only just spotted that line of reasoning. > > Longterm, I do want to remove all the Viridian special cases, and handle > all of the state via architectural mechanisms (cpu policy for static > settings, and the existing MSR records for dynamic content). > > A consequence of this cleanup is that guest_{rd,wr}msr() will be > eventually be used to save/restore dynamic state in the migrate stream, > which is why I've been engineering guest_{rd,wr}msr() to work for MSRs > in "current || !scheduled" context. > > As such, it is important to retain a vcpu pointer because it will not be > current on the save/restore hypercalls, which execute in control domain > context. But you are never going to restore an HV_X64_MSR_EOI MSR, as such should never be part of the migrate stream in the first place. It doesn't have a value itself - it's only used as an alternative way to EOI an interrupt. I still think the EOIs will always be performed on the affected vCPU context, but I don't want this discussion the block the current series, so I'm just going to drop this patch. Roger.
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. Jan
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@citrix.com> 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@amazon.com> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- 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);
On 07.04.2021 09:41, Roger Pau Monné wrote: > 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. While I'm not opposed in principle, the patch moves us further away from what Andrew has asked for (to retain the vcpu pointers), if I understood him correctly. I'm also not entirely certain if there couldn't be, down the road, emulators needing to signal an EOI to Xen on behalf of a guest. Jan
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/vlapic.c b/xen/arch/x86/hvm/vlapic.c index 5e21fb4937d..98e4ba67d79 100644 --- 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); } static bool_t is_multicast_dest(struct vlapic *vlapic, unsigned int short_hand, 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);