| Message ID | 3cc31df5191ae6b03e060ccd8e82df74416a3ef5.1596768588.git.haibo.xu@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/arm: Add vSPE support to KVM guest | expand |
On 8/7/20 10:10 AM, Haibo Xu wrote: > Rename kvm_arm_pmu_set_attr() to kvm_arm_dev_set_attr(), Maybe rename kvm_arm_device_set_attr() to match the structure name? > So both the vPMU and vSPE device can share the same API. > > Signed-off-by: Haibo Xu <haibo.xu@linaro.org> Regardless, with the typo "operation" in patch subject fixed: Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > target/arm/kvm64.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c > index 1169237905..75a417d65c 100644 > --- a/target/arm/kvm64.c > +++ b/target/arm/kvm64.c > @@ -398,7 +398,7 @@ static CPUWatchpoint *find_hw_watchpoint(CPUState *cpu, target_ulong addr) > return NULL; > } > > -static bool kvm_arm_pmu_set_attr(CPUState *cs, struct kvm_device_attr *attr) > +static bool kvm_arm_dev_set_attr(CPUState *cs, struct kvm_device_attr *attr) > { > int err; > > @@ -427,7 +427,7 @@ void kvm_arm_pmu_init(CPUState *cs) > if (!ARM_CPU(cs)->has_pmu) { > return; > } > - if (!kvm_arm_pmu_set_attr(cs, &attr)) { > + if (!kvm_arm_dev_set_attr(cs, &attr)) { > error_report("failed to init PMU"); > abort(); > } > @@ -444,7 +444,7 @@ void kvm_arm_pmu_set_irq(CPUState *cs, int irq) > if (!ARM_CPU(cs)->has_pmu) { > return; > } > - if (!kvm_arm_pmu_set_attr(cs, &attr)) { > + if (!kvm_arm_dev_set_attr(cs, &attr)) { > error_report("failed to set irq for PMU"); > abort(); > } >
On Fri, 7 Aug 2020 at 16:19, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > On 8/7/20 10:10 AM, Haibo Xu wrote: > > Rename kvm_arm_pmu_set_attr() to kvm_arm_dev_set_attr(), > > Maybe rename kvm_arm_device_set_attr() to match the structure > name? > Thanks for the review! I will update it in the next version. > > So both the vPMU and vSPE device can share the same API. > > > > Signed-off-by: Haibo Xu <haibo.xu@linaro.org> > > Regardless, with the typo "operation" in patch subject fixed: > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > > --- > > target/arm/kvm64.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c > > index 1169237905..75a417d65c 100644 > > --- a/target/arm/kvm64.c > > +++ b/target/arm/kvm64.c > > @@ -398,7 +398,7 @@ static CPUWatchpoint *find_hw_watchpoint(CPUState *cpu, target_ulong addr) > > return NULL; > > } > > > > -static bool kvm_arm_pmu_set_attr(CPUState *cs, struct kvm_device_attr *attr) > > +static bool kvm_arm_dev_set_attr(CPUState *cs, struct kvm_device_attr *attr) > > { > > int err; > > > > @@ -427,7 +427,7 @@ void kvm_arm_pmu_init(CPUState *cs) > > if (!ARM_CPU(cs)->has_pmu) { > > return; > > } > > - if (!kvm_arm_pmu_set_attr(cs, &attr)) { > > + if (!kvm_arm_dev_set_attr(cs, &attr)) { > > error_report("failed to init PMU"); > > abort(); > > } > > @@ -444,7 +444,7 @@ void kvm_arm_pmu_set_irq(CPUState *cs, int irq) > > if (!ARM_CPU(cs)->has_pmu) { > > return; > > } > > - if (!kvm_arm_pmu_set_attr(cs, &attr)) { > > + if (!kvm_arm_dev_set_attr(cs, &attr)) { > > error_report("failed to set irq for PMU"); > > abort(); > > } > > >
On Mon, Aug 10, 2020 at 10:48:41AM +0800, Haibo Xu wrote: > On Fri, 7 Aug 2020 at 16:19, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > > > On 8/7/20 10:10 AM, Haibo Xu wrote: > > > Rename kvm_arm_pmu_set_attr() to kvm_arm_dev_set_attr(), > > > > Maybe rename kvm_arm_device_set_attr() to match the structure > > name? > > > > Thanks for the review! I will update it in the next version. I've already renamed it to kvm_arm_set_device_attr() in [1]. Also, it's not enough to just rename the function. The error messages the function may generate have "PMU" embedded in them. [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg727590.html Thanks, drew > > > > So both the vPMU and vSPE device can share the same API. > > > > > > Signed-off-by: Haibo Xu <haibo.xu@linaro.org> > > > > Regardless, with the typo "operation" in patch subject fixed: > > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > > > > --- > > > target/arm/kvm64.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c > > > index 1169237905..75a417d65c 100644 > > > --- a/target/arm/kvm64.c > > > +++ b/target/arm/kvm64.c > > > @@ -398,7 +398,7 @@ static CPUWatchpoint *find_hw_watchpoint(CPUState *cpu, target_ulong addr) > > > return NULL; > > > } > > > > > > -static bool kvm_arm_pmu_set_attr(CPUState *cs, struct kvm_device_attr *attr) > > > +static bool kvm_arm_dev_set_attr(CPUState *cs, struct kvm_device_attr *attr) > > > { > > > int err; > > > > > > @@ -427,7 +427,7 @@ void kvm_arm_pmu_init(CPUState *cs) > > > if (!ARM_CPU(cs)->has_pmu) { > > > return; > > > } > > > - if (!kvm_arm_pmu_set_attr(cs, &attr)) { > > > + if (!kvm_arm_dev_set_attr(cs, &attr)) { > > > error_report("failed to init PMU"); > > > abort(); > > > } > > > @@ -444,7 +444,7 @@ void kvm_arm_pmu_set_irq(CPUState *cs, int irq) > > > if (!ARM_CPU(cs)->has_pmu) { > > > return; > > > } > > > - if (!kvm_arm_pmu_set_attr(cs, &attr)) { > > > + if (!kvm_arm_dev_set_attr(cs, &attr)) { > > > error_report("failed to set irq for PMU"); > > > abort(); > > > } > > > > > >
On Mon, 10 Aug 2020 at 18:29, Andrew Jones <drjones@redhat.com> wrote: > > On Mon, Aug 10, 2020 at 10:48:41AM +0800, Haibo Xu wrote: > > On Fri, 7 Aug 2020 at 16:19, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > > > > > On 8/7/20 10:10 AM, Haibo Xu wrote: > > > > Rename kvm_arm_pmu_set_attr() to kvm_arm_dev_set_attr(), > > > > > > Maybe rename kvm_arm_device_set_attr() to match the structure > > > name? > > > > > > > Thanks for the review! I will update it in the next version. > > I've already renamed it to kvm_arm_set_device_attr() in [1]. Also, it's > not enough to just rename the function. The error messages the function > may generate have "PMU" embedded in them. > > [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg727590.html > > Thanks, > drew > Thanks for your review, Andrew! Will rebase on your patches in the next version. > > > > > > So both the vPMU and vSPE device can share the same API. > > > > > > > > Signed-off-by: Haibo Xu <haibo.xu@linaro.org> > > > > > > Regardless, with the typo "operation" in patch subject fixed: > > > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > > > > > > --- > > > > target/arm/kvm64.c | 6 +++--- > > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c > > > > index 1169237905..75a417d65c 100644 > > > > --- a/target/arm/kvm64.c > > > > +++ b/target/arm/kvm64.c > > > > @@ -398,7 +398,7 @@ static CPUWatchpoint *find_hw_watchpoint(CPUState *cpu, target_ulong addr) > > > > return NULL; > > > > } > > > > > > > > -static bool kvm_arm_pmu_set_attr(CPUState *cs, struct kvm_device_attr *attr) > > > > +static bool kvm_arm_dev_set_attr(CPUState *cs, struct kvm_device_attr *attr) > > > > { > > > > int err; > > > > > > > > @@ -427,7 +427,7 @@ void kvm_arm_pmu_init(CPUState *cs) > > > > if (!ARM_CPU(cs)->has_pmu) { > > > > return; > > > > } > > > > - if (!kvm_arm_pmu_set_attr(cs, &attr)) { > > > > + if (!kvm_arm_dev_set_attr(cs, &attr)) { > > > > error_report("failed to init PMU"); > > > > abort(); > > > > } > > > > @@ -444,7 +444,7 @@ void kvm_arm_pmu_set_irq(CPUState *cs, int irq) > > > > if (!ARM_CPU(cs)->has_pmu) { > > > > return; > > > > } > > > > - if (!kvm_arm_pmu_set_attr(cs, &attr)) { > > > > + if (!kvm_arm_dev_set_attr(cs, &attr)) { > > > > error_report("failed to set irq for PMU"); > > > > abort(); > > > > } > > > > > > > > > >
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c index 1169237905..75a417d65c 100644 --- a/target/arm/kvm64.c +++ b/target/arm/kvm64.c @@ -398,7 +398,7 @@ static CPUWatchpoint *find_hw_watchpoint(CPUState *cpu, target_ulong addr) return NULL; } -static bool kvm_arm_pmu_set_attr(CPUState *cs, struct kvm_device_attr *attr) +static bool kvm_arm_dev_set_attr(CPUState *cs, struct kvm_device_attr *attr) { int err; @@ -427,7 +427,7 @@ void kvm_arm_pmu_init(CPUState *cs) if (!ARM_CPU(cs)->has_pmu) { return; } - if (!kvm_arm_pmu_set_attr(cs, &attr)) { + if (!kvm_arm_dev_set_attr(cs, &attr)) { error_report("failed to init PMU"); abort(); } @@ -444,7 +444,7 @@ void kvm_arm_pmu_set_irq(CPUState *cs, int irq) if (!ARM_CPU(cs)->has_pmu) { return; } - if (!kvm_arm_pmu_set_attr(cs, &attr)) { + if (!kvm_arm_dev_set_attr(cs, &attr)) { error_report("failed to set irq for PMU"); abort(); }
Rename kvm_arm_pmu_set_attr() to kvm_arm_dev_set_attr(), So both the vPMU and vSPE device can share the same API. Signed-off-by: Haibo Xu <haibo.xu@linaro.org> --- target/arm/kvm64.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)