| Message ID | 20200930104108.35969-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/intr: introduce EOI callbacks and fix vPT | expand |
> -----Original Message----- > From: Roger Pau Monne <roger.pau@citrix.com> > Sent: 30 September 2020 11:41 > To: xen-devel@lists.xenproject.org > Cc: Roger Pau Monne <roger.pau@citrix.com>; Jan Beulich <jbeulich@suse.com>; Andrew Cooper > <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Paul Durrant <paul@xen.org>; Paul Durrant > <pdurrant@amazon.com> > Subject: [PATCH v2 01/11] x86/hvm: drop vcpu parameter from vlapic EOI callbacks > > 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. > --- > xen/arch/x86/hvm/vioapic.c | 5 +++-- > xen/arch/x86/hvm/vlapic.c | 7 ++----- > xen/drivers/passthrough/io.c | 4 +++- > xen/include/asm-x86/hvm/io.h | 2 +- > xen/include/asm-x86/hvm/vioapic.h | 2 +- > 5 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c > index 67d4a6237f..0fb9147d99 100644 > --- a/xen/arch/x86/hvm/vioapic.c > +++ b/xen/arch/x86/hvm/vioapic.c > @@ -353,7 +353,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 > > @@ -495,8 +495,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 4e3861eb7d..ae737403f3 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/io.c b/xen/drivers/passthrough/io.c > index 6b1305a3e5..54f3e7b540 100644 > --- a/xen/drivers/passthrough/io.c > +++ b/xen/drivers/passthrough/io.c > @@ -874,8 +874,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 558426b772..adec0f566a 100644 > --- a/xen/include/asm-x86/hvm/io.h > +++ b/xen/include/asm-x86/hvm/io.h > @@ -159,7 +159,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 d6f4e12d54..fd602f8830 100644 > --- a/xen/include/asm-x86/hvm/vioapic.h > +++ b/xen/include/asm-x86/hvm/vioapic.h > @@ -64,7 +64,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); > -- > 2.28.0
On 30.09.2020 12:40, Roger Pau Monne wrote: > --- 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); > } What about viridian_synic_wrmsr() -> vlapic_EOI_set() -> vlapic_handle_EOI()? You'd probably have noticed this if you had tried to (consistently) drop the respective parameters from the intermediate functions as well. Question of course is in how far viridian_synic_wrmsr() for HV_X64_MSR_EOI makes much sense when v != current. Paul, Wei? A secondary question of course is whether passing around the pointers isn't really cheaper than the obtaining of 'current'. Jan
> -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 02 October 2020 09:48 > To: Roger Pau Monne <roger.pau@citrix.com>; Wei Liu <wl@xen.org>; Paul Durrant <paul@xen.org> > Cc: xen-devel@lists.xenproject.org; Andrew Cooper <andrew.cooper3@citrix.com>; Durrant, Paul > <pdurrant@amazon.co.uk> > Subject: RE: [EXTERNAL] [PATCH v2 01/11] x86/hvm: drop vcpu parameter from vlapic EOI callbacks > > CAUTION: This email originated from outside of the organization. Do not click links or open > attachments unless you can confirm the sender and know the content is safe. > > > > On 30.09.2020 12:40, Roger Pau Monne wrote: > > --- 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); > > } > > What about viridian_synic_wrmsr() -> vlapic_EOI_set() -> > vlapic_handle_EOI()? You'd probably have noticed this if you > had tried to (consistently) drop the respective parameters from > the intermediate functions as well. > > Question of course is in how far viridian_synic_wrmsr() for > HV_X64_MSR_EOI makes much sense when v != current. Paul, Wei? > I don't think it makes any sense. I think it would be fine to only do it if v == current. Paul > A secondary question of course is whether passing around the > pointers isn't really cheaper than the obtaining of 'current'. > > Jan
On Fri, Oct 02, 2020 at 09:24:57AM +0000, Durrant, Paul wrote: > > -----Original Message----- > > From: Jan Beulich <jbeulich@suse.com> > > Sent: 02 October 2020 09:48 > > To: Roger Pau Monne <roger.pau@citrix.com>; Wei Liu <wl@xen.org>; Paul Durrant <paul@xen.org> > > Cc: xen-devel@lists.xenproject.org; Andrew Cooper <andrew.cooper3@citrix.com>; Durrant, Paul > > <pdurrant@amazon.co.uk> > > Subject: RE: [EXTERNAL] [PATCH v2 01/11] x86/hvm: drop vcpu parameter from vlapic EOI callbacks > > > > CAUTION: This email originated from outside of the organization. Do not click links or open > > attachments unless you can confirm the sender and know the content is safe. > > > > > > > > On 30.09.2020 12:40, Roger Pau Monne wrote: > > > --- 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); > > > } > > > > What about viridian_synic_wrmsr() -> vlapic_EOI_set() -> > > vlapic_handle_EOI()? You'd probably have noticed this if you > > had tried to (consistently) drop the respective parameters from > > the intermediate functions as well. > > > > Question of course is in how far viridian_synic_wrmsr() for > > HV_X64_MSR_EOI makes much sense when v != current. Paul, Wei? > > > > I don't think it makes any sense. I think it would be fine to only do it if v == current. Yes, I agree. Wei.
On Fri, Oct 02, 2020 at 10:48:07AM +0200, Jan Beulich wrote: > On 30.09.2020 12:40, Roger Pau Monne wrote: > > --- 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); > > } > > What about viridian_synic_wrmsr() -> vlapic_EOI_set() -> > vlapic_handle_EOI()? You'd probably have noticed this if you > had tried to (consistently) drop the respective parameters from > the intermediate functions as well. > > Question of course is in how far viridian_synic_wrmsr() for > HV_X64_MSR_EOI makes much sense when v != current. Paul, Wei? There's already an assert at the top of viridian_synic_wrmsr of v == current, which I assume is why I thought this change was fine. I can purge the passing of v (current) further, but it wasn't really needed for the rest of the series. > A secondary question of course is whether passing around the > pointers isn't really cheaper than the obtaining of 'current'. Well, while there's indeed a performance aspect here, I think using current is much clearer than passing a vcpu around. I could rename the parameter to curr or some such, but I think using current and not passing a vcpu parameter is clearer. Thanks, Roger.
On 13.10.2020 16:08, Roger Pau Monné wrote: > On Fri, Oct 02, 2020 at 10:48:07AM +0200, Jan Beulich wrote: >> On 30.09.2020 12:40, Roger Pau Monne wrote: >>> --- 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); >>> } >> >> What about viridian_synic_wrmsr() -> vlapic_EOI_set() -> >> vlapic_handle_EOI()? You'd probably have noticed this if you >> had tried to (consistently) drop the respective parameters from >> the intermediate functions as well. >> >> Question of course is in how far viridian_synic_wrmsr() for >> HV_X64_MSR_EOI makes much sense when v != current. Paul, Wei? > > There's already an assert at the top of viridian_synic_wrmsr of v == > current, which I assume is why I thought this change was fine. I can > purge the passing of v (current) further, but it wasn't really needed > for the rest of the series. To a large degree that's up to you. It's just that, as said, if you had done so, you'd likely have noticed the issue, and hence doing so here and elsewhere may provide reassurance that there's no further similar case lurking anywhere. >> A secondary question of course is whether passing around the >> pointers isn't really cheaper than the obtaining of 'current'. > > Well, while there's indeed a performance aspect here, I think > using current is much clearer than passing a vcpu around. I could > rename the parameter to curr or some such, but I think using > current and not passing a vcpu parameter is clearer. Personally I'd prefer "curr" named function parameters. But if Andrew or Wei agree with your approach, I'm not going to object. Jan
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c index 67d4a6237f..0fb9147d99 100644 --- a/xen/arch/x86/hvm/vioapic.c +++ b/xen/arch/x86/hvm/vioapic.c @@ -353,7 +353,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 @@ -495,8 +495,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 4e3861eb7d..ae737403f3 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/io.c b/xen/drivers/passthrough/io.c index 6b1305a3e5..54f3e7b540 100644 --- a/xen/drivers/passthrough/io.c +++ b/xen/drivers/passthrough/io.c @@ -874,8 +874,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 558426b772..adec0f566a 100644 --- a/xen/include/asm-x86/hvm/io.h +++ b/xen/include/asm-x86/hvm/io.h @@ -159,7 +159,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 d6f4e12d54..fd602f8830 100644 --- a/xen/include/asm-x86/hvm/vioapic.h +++ b/xen/include/asm-x86/hvm/vioapic.h @@ -64,7 +64,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);
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> --- Changes since v1: - New in this version. --- xen/arch/x86/hvm/vioapic.c | 5 +++-- xen/arch/x86/hvm/vlapic.c | 7 ++----- xen/drivers/passthrough/io.c | 4 +++- xen/include/asm-x86/hvm/io.h | 2 +- xen/include/asm-x86/hvm/vioapic.h | 2 +- 5 files changed, 10 insertions(+), 10 deletions(-)