We are going to factor out the TOD into a separate device and use const
pointers for device class functions where possible. We are passing right
now ordinary pointers that should never be touched when setting the TOD.
Let's just pass the values directly.
Signed-off-by: David Hildenbrand <david@redhat.com>
---
target/s390x/cpu.c | 4 ++--
target/s390x/kvm-stub.c | 4 ++--
target/s390x/kvm.c | 12 ++++++------
target/s390x/kvm_s390x.h | 4 ++--
4 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index c268065887..68512e3e54 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -413,9 +413,9 @@ int s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)
int r = 0;
if (kvm_enabled()) {
- r = kvm_s390_set_clock_ext(tod_high, tod_low);
+ r = kvm_s390_set_clock_ext(*tod_high, *tod_low);
if (r == -ENXIO) {
- return kvm_s390_set_clock(tod_high, tod_low);
+ return kvm_s390_set_clock(*tod_high, *tod_low);
}
}
/* Fixme TCG */
diff --git a/target/s390x/kvm-stub.c b/target/s390x/kvm-stub.c
index 29b10542cc..bf7795e47a 100644
--- a/target/s390x/kvm-stub.c
+++ b/target/s390x/kvm-stub.c
@@ -60,12 +60,12 @@ int kvm_s390_get_clock_ext(uint8_t *tod_high, uint64_t *tod_low)
return -ENOSYS;
}
-int kvm_s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)
+int kvm_s390_set_clock(uint8_t tod_high, uint64_t tod_low)
{
return -ENOSYS;
}
-int kvm_s390_set_clock_ext(uint8_t *tod_high, uint64_t *tod_low)
+int kvm_s390_set_clock_ext(uint8_t tod_high, uint64_t tod_low)
{
return -ENOSYS;
}
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index ac370da281..8bcd832123 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -666,13 +666,13 @@ int kvm_s390_get_clock_ext(uint8_t *tod_high, uint64_t *tod_low)
return r;
}
-int kvm_s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)
+int kvm_s390_set_clock(uint8_t tod_high, uint64_t tod_low)
{
int r;
struct kvm_device_attr attr = {
.group = KVM_S390_VM_TOD,
.attr = KVM_S390_VM_TOD_LOW,
- .addr = (uint64_t)tod_low,
+ .addr = (uint64_t)&tod_low,
};
r = kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
@@ -681,15 +681,15 @@ int kvm_s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)
}
attr.attr = KVM_S390_VM_TOD_HIGH;
- attr.addr = (uint64_t)tod_high;
+ attr.addr = (uint64_t)&tod_high;
return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
}
-int kvm_s390_set_clock_ext(uint8_t *tod_high, uint64_t *tod_low)
+int kvm_s390_set_clock_ext(uint8_t tod_high, uint64_t tod_low)
{
struct kvm_s390_vm_tod_clock gtod = {
- .epoch_idx = *tod_high,
- .tod = *tod_low,
+ .epoch_idx = tod_high,
+ .tod = tod_low,
};
struct kvm_device_attr attr = {
.group = KVM_S390_VM_TOD,
diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h
index c383bf4ee9..36eb34bf99 100644
--- a/target/s390x/kvm_s390x.h
+++ b/target/s390x/kvm_s390x.h
@@ -25,8 +25,8 @@ int kvm_s390_get_ri(void);
int kvm_s390_get_gs(void);
int kvm_s390_get_clock(uint8_t *tod_high, uint64_t *tod_clock);
int kvm_s390_get_clock_ext(uint8_t *tod_high, uint64_t *tod_clock);
-int kvm_s390_set_clock(uint8_t *tod_high, uint64_t *tod_clock);
-int kvm_s390_set_clock_ext(uint8_t *tod_high, uint64_t *tod_clock);
+int kvm_s390_set_clock(uint8_t tod_high, uint64_t tod_clock);
+int kvm_s390_set_clock_ext(uint8_t tod_high, uint64_t tod_clock);
void kvm_s390_enable_css_support(S390CPU *cpu);
int kvm_s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch,
int vq, bool assign);
--
2.17.1
On Mon, 25 Jun 2018 13:53:45 +0200
David Hildenbrand <david@redhat.com> wrote:
> We are going to factor out the TOD into a separate device and use const
> pointers for device class functions where possible. We are passing right
> now ordinary pointers that should never be touched when setting the TOD.
> Let's just pass the values directly.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> target/s390x/cpu.c | 4 ++--
> target/s390x/kvm-stub.c | 4 ++--
> target/s390x/kvm.c | 12 ++++++------
> target/s390x/kvm_s390x.h | 4 ++--
> 4 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index c268065887..68512e3e54 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -413,9 +413,9 @@ int s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)
Any reason why you keep the pointers here?
> int r = 0;
>
> if (kvm_enabled()) {
> - r = kvm_s390_set_clock_ext(tod_high, tod_low);
> + r = kvm_s390_set_clock_ext(*tod_high, *tod_low);
> if (r == -ENXIO) {
> - return kvm_s390_set_clock(tod_high, tod_low);
> + return kvm_s390_set_clock(*tod_high, *tod_low);
Especially as it would be more clean to check for !NULL before
dereferencing...
> }
> }
> /* Fixme TCG */
On 25.06.2018 17:50, Cornelia Huck wrote:
> On Mon, 25 Jun 2018 13:53:45 +0200
> David Hildenbrand <david@redhat.com> wrote:
>
>> We are going to factor out the TOD into a separate device and use const
>> pointers for device class functions where possible. We are passing right
>> now ordinary pointers that should never be touched when setting the TOD.
>> Let's just pass the values directly.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>> target/s390x/cpu.c | 4 ++--
>> target/s390x/kvm-stub.c | 4 ++--
>> target/s390x/kvm.c | 12 ++++++------
>> target/s390x/kvm_s390x.h | 4 ++--
>> 4 files changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
>> index c268065887..68512e3e54 100644
>> --- a/target/s390x/cpu.c
>> +++ b/target/s390x/cpu.c
>> @@ -413,9 +413,9 @@ int s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)
>
> Any reason why you keep the pointers here?
>
>> int r = 0;
>>
>> if (kvm_enabled()) {
>> - r = kvm_s390_set_clock_ext(tod_high, tod_low);
>> + r = kvm_s390_set_clock_ext(*tod_high, *tod_low);
>> if (r == -ENXIO) {
>> - return kvm_s390_set_clock(tod_high, tod_low);
>> + return kvm_s390_set_clock(*tod_high, *tod_low);
>
> Especially as it would be more clean to check for !NULL before
> dereferencing...
See the next patch :)
(I assume that refactoring code in order to rip it out does not make sense)
>
>> }
>> }
>> /* Fixme TCG */
--
Thanks,
David / dhildenb
On Mon, 25 Jun 2018 17:54:42 +0200
David Hildenbrand <david@redhat.com> wrote:
> On 25.06.2018 17:50, Cornelia Huck wrote:
> > On Mon, 25 Jun 2018 13:53:45 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >
> >> We are going to factor out the TOD into a separate device and use const
> >> pointers for device class functions where possible. We are passing right
> >> now ordinary pointers that should never be touched when setting the TOD.
> >> Let's just pass the values directly.
> >>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >> target/s390x/cpu.c | 4 ++--
> >> target/s390x/kvm-stub.c | 4 ++--
> >> target/s390x/kvm.c | 12 ++++++------
> >> target/s390x/kvm_s390x.h | 4 ++--
> >> 4 files changed, 12 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> >> index c268065887..68512e3e54 100644
> >> --- a/target/s390x/cpu.c
> >> +++ b/target/s390x/cpu.c
> >> @@ -413,9 +413,9 @@ int s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)
> >
> > Any reason why you keep the pointers here?
> >
> >> int r = 0;
> >>
> >> if (kvm_enabled()) {
> >> - r = kvm_s390_set_clock_ext(tod_high, tod_low);
> >> + r = kvm_s390_set_clock_ext(*tod_high, *tod_low);
> >> if (r == -ENXIO) {
> >> - return kvm_s390_set_clock(tod_high, tod_low);
> >> + return kvm_s390_set_clock(*tod_high, *tod_low);
> >
> > Especially as it would be more clean to check for !NULL before
> > dereferencing...
>
> See the next patch :)
>
> (I assume that refactoring code in order to rip it out does not make sense)
Add a comment in the commit message?
"Note that s390_set_clock() will be removed in a follow-on patch and
therefore its calling convention is not changed."
On 25.06.2018 18:03, Cornelia Huck wrote:
> On Mon, 25 Jun 2018 17:54:42 +0200
> David Hildenbrand <david@redhat.com> wrote:
>
>> On 25.06.2018 17:50, Cornelia Huck wrote:
>>> On Mon, 25 Jun 2018 13:53:45 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>
>>>> We are going to factor out the TOD into a separate device and use const
>>>> pointers for device class functions where possible. We are passing right
>>>> now ordinary pointers that should never be touched when setting the TOD.
>>>> Let's just pass the values directly.
>>>>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>> target/s390x/cpu.c | 4 ++--
>>>> target/s390x/kvm-stub.c | 4 ++--
>>>> target/s390x/kvm.c | 12 ++++++------
>>>> target/s390x/kvm_s390x.h | 4 ++--
>>>> 4 files changed, 12 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
>>>> index c268065887..68512e3e54 100644
>>>> --- a/target/s390x/cpu.c
>>>> +++ b/target/s390x/cpu.c
>>>> @@ -413,9 +413,9 @@ int s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)
>>>
>>> Any reason why you keep the pointers here?
>>>
>>>> int r = 0;
>>>>
>>>> if (kvm_enabled()) {
>>>> - r = kvm_s390_set_clock_ext(tod_high, tod_low);
>>>> + r = kvm_s390_set_clock_ext(*tod_high, *tod_low);
>>>> if (r == -ENXIO) {
>>>> - return kvm_s390_set_clock(tod_high, tod_low);
>>>> + return kvm_s390_set_clock(*tod_high, *tod_low);
>>>
>>> Especially as it would be more clean to check for !NULL before
>>> dereferencing...
>>
>> See the next patch :)
>>
>> (I assume that refactoring code in order to rip it out does not make sense)
>
> Add a comment in the commit message?
>
> "Note that s390_set_clock() will be removed in a follow-on patch and
> therefore its calling convention is not changed."
>
Sure I can do that. Thanks!
--
Thanks,
David / dhildenb
On Tue, 26 Jun 2018 11:54:32 +0200
David Hildenbrand <david@redhat.com> wrote:
> On 25.06.2018 18:03, Cornelia Huck wrote:
> > On Mon, 25 Jun 2018 17:54:42 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >
> >> On 25.06.2018 17:50, Cornelia Huck wrote:
> >>> On Mon, 25 Jun 2018 13:53:45 +0200
> >>> David Hildenbrand <david@redhat.com> wrote:
> >>>
> >>>> We are going to factor out the TOD into a separate device and use const
> >>>> pointers for device class functions where possible. We are passing right
> >>>> now ordinary pointers that should never be touched when setting the TOD.
> >>>> Let's just pass the values directly.
> >>>>
> >>>> Signed-off-by: David Hildenbrand <david@redhat.com>
> >>>> ---
> >>>> target/s390x/cpu.c | 4 ++--
> >>>> target/s390x/kvm-stub.c | 4 ++--
> >>>> target/s390x/kvm.c | 12 ++++++------
> >>>> target/s390x/kvm_s390x.h | 4 ++--
> >>>> 4 files changed, 12 insertions(+), 12 deletions(-)
> >>>>
> >>>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> >>>> index c268065887..68512e3e54 100644
> >>>> --- a/target/s390x/cpu.c
> >>>> +++ b/target/s390x/cpu.c
> >>>> @@ -413,9 +413,9 @@ int s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)
> >>>
> >>> Any reason why you keep the pointers here?
> >>>
> >>>> int r = 0;
> >>>>
> >>>> if (kvm_enabled()) {
> >>>> - r = kvm_s390_set_clock_ext(tod_high, tod_low);
> >>>> + r = kvm_s390_set_clock_ext(*tod_high, *tod_low);
> >>>> if (r == -ENXIO) {
> >>>> - return kvm_s390_set_clock(tod_high, tod_low);
> >>>> + return kvm_s390_set_clock(*tod_high, *tod_low);
> >>>
> >>> Especially as it would be more clean to check for !NULL before
> >>> dereferencing...
> >>
> >> See the next patch :)
> >>
> >> (I assume that refactoring code in order to rip it out does not make sense)
> >
> > Add a comment in the commit message?
> >
> > "Note that s390_set_clock() will be removed in a follow-on patch and
> > therefore its calling convention is not changed."
> >
>
> Sure I can do that. Thanks!
>
Well, no need to respin if nothing else comes up, I can add that myself.
© 2016 - 2025 Red Hat, Inc.