| Message ID | 1459259051-4943-2-git-send-email-joao.m.martins@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Joao Martins writes ("[PATCH v2 1/6] public/xen.h: add flags field to vcpu_time_info"):
> This field has two possible flags (as of latest pvclock ABI
> shared with KVM).
>
> flags: bits in this field indicate extended capabilities
> coordinated between the guest and the hypervisor. Specifically
> on KVM, availability of specific flags has to be checked in
> 0x40000001 cpuid leaf. On Xen, we don't have that but we can
> still check some of the flags after registering the time info
> page since a force_update_vcpu_system_time is performed.
Earlier in the thread there was a Reviewed-by from Andrew Cooper.
Shouldn't that have been preserved here ?
Also, Andrew made some points about the linux.git MAINTAINERS file.
Ideally I would like to commit this patch ASAP.
Thanks,
Ian.
On 03/30/2016 04:49 PM, Ian Jackson wrote: > Joao Martins writes ("[PATCH v2 1/6] public/xen.h: add flags field to vcpu_time_info"): >> This field has two possible flags (as of latest pvclock ABI >> shared with KVM). >> >> flags: bits in this field indicate extended capabilities >> coordinated between the guest and the hypervisor. Specifically >> on KVM, availability of specific flags has to be checked in >> 0x40000001 cpuid leaf. On Xen, we don't have that but we can >> still check some of the flags after registering the time info >> page since a force_update_vcpu_system_time is performed. > > Earlier in the thread there was a Reviewed-by from Andrew Cooper. > Shouldn't that have been preserved here ? I dropped both of the Reviewed-by because I updated the patch with changes that weren't noted in earlier reviews - I had in mind that is the protocol. > Also, Andrew made some points about the linux.git MAINTAINERS file. Yes, the MAINTAINERS file will be updated with the linux v2 series and the point is to have xen-devel be notified whenever changes are made to the pvclock ABI (that is shared with KVM). Joao > Ideally I would like to commit this patch ASAP. > > Thanks, > Ian. >
>>> On 30.03.16 at 17:49, <Ian.Jackson@eu.citrix.com> wrote: > Joao Martins writes ("[PATCH v2 1/6] public/xen.h: add flags field to > vcpu_time_info"): >> This field has two possible flags (as of latest pvclock ABI >> shared with KVM). >> >> flags: bits in this field indicate extended capabilities >> coordinated between the guest and the hypervisor. Specifically >> on KVM, availability of specific flags has to be checked in >> 0x40000001 cpuid leaf. On Xen, we don't have that but we can >> still check some of the flags after registering the time info >> page since a force_update_vcpu_system_time is performed. > > Earlier in the thread there was a Reviewed-by from Andrew Cooper. > Shouldn't that have been preserved here ? > > Also, Andrew made some points about the linux.git MAINTAINERS file. > > Ideally I would like to commit this patch ASAP. Is there a particular reason you see a need for this to go in ahead of the rest of the series? This change alone can certainly have my ack, but I lack the time right now to look at the other patches. Jan
>>> On 29.03.16 at 15:44, <joao.m.martins@oracle.com> wrote: > This field has two possible flags (as of latest pvclock ABI > shared with KVM). > > flags: bits in this field indicate extended capabilities > coordinated between the guest and the hypervisor. Specifically > on KVM, availability of specific flags has to be checked in > 0x40000001 cpuid leaf. On Xen, we don't have that but we can > still check some of the flags after registering the time info > page since a force_update_vcpu_system_time is performed. > > Current flags are: > > flag bit | cpuid bit | meaning > ------------------------------------------------------------- > | | time measures taken across > 0 | 24 | multiple cpus are guaranteed to > | | be monotonic > ------------------------------------------------------------- > | | guest vcpu has been paused by > 1 | N/A | the host > | | > ------------------------------------------------------------- > > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> With one further adjustment (which could be done while committing) Acked-by: Jan Beulich <jbeulich@suse.com> > --- a/xen/include/public/xen.h > +++ b/xen/include/public/xen.h > @@ -614,10 +614,14 @@ struct vcpu_time_info { > */ > uint32_t tsc_to_system_mul; > int8_t tsc_shift; > - int8_t pad1[3]; > + uint8_t flags; > + uint8_t pad1[2]; > }; /* 32 bytes */ > typedef struct vcpu_time_info vcpu_time_info_t; > > +#define PVCLOCK_TSC_STABLE_BIT (1 << 0) > +#define PVCLOCK_GUEST_STOPPED (1 << 1) No new identifiers not properly prefixed by XEN_ (or, elsewhere, xen_) in the canonical public headers please (whether downstream consumers like Linux elect to strip such prefixes is an independent aspect). Jan
On 03/31/2016 08:13 AM, Jan Beulich wrote: >>>> On 29.03.16 at 15:44, <joao.m.martins@oracle.com> wrote: >> This field has two possible flags (as of latest pvclock ABI >> shared with KVM). >> >> flags: bits in this field indicate extended capabilities >> coordinated between the guest and the hypervisor. Specifically >> on KVM, availability of specific flags has to be checked in >> 0x40000001 cpuid leaf. On Xen, we don't have that but we can >> still check some of the flags after registering the time info >> page since a force_update_vcpu_system_time is performed. >> >> Current flags are: >> >> flag bit | cpuid bit | meaning >> ------------------------------------------------------------- >> | | time measures taken across >> 0 | 24 | multiple cpus are guaranteed to >> | | be monotonic >> ------------------------------------------------------------- >> | | guest vcpu has been paused by >> 1 | N/A | the host >> | | >> ------------------------------------------------------------- >> >> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > > With one further adjustment (which could be done while committing) > Acked-by: Jan Beulich <jbeulich@suse.com> > Thanks! >> --- a/xen/include/public/xen.h >> +++ b/xen/include/public/xen.h >> @@ -614,10 +614,14 @@ struct vcpu_time_info { >> */ >> uint32_t tsc_to_system_mul; >> int8_t tsc_shift; >> - int8_t pad1[3]; >> + uint8_t flags; >> + uint8_t pad1[2]; >> }; /* 32 bytes */ >> typedef struct vcpu_time_info vcpu_time_info_t; >> >> +#define PVCLOCK_TSC_STABLE_BIT (1 << 0) >> +#define PVCLOCK_GUEST_STOPPED (1 << 1) I noticed from seeing different indentation on Jan's reply that there should be spaces and no tabs here between the macro name and value. Fixed that too. > No new identifiers not properly prefixed by XEN_ (or, elsewhere, > xen_) in the canonical public headers please (whether > downstream consumers like Linux elect to strip such prefixes is > an independent aspect). > OK, fixed and note taken. If desired, I can submit v3 of this one in separate if Ian J. would like it committed beforehand. Though what's added here, only get used later on this series. João
>>> On 29.03.16 at 15:44, <joao.m.martins@oracle.com> wrote: > --- a/xen/include/public/xen.h > +++ b/xen/include/public/xen.h > @@ -614,10 +614,14 @@ struct vcpu_time_info { > */ > uint32_t tsc_to_system_mul; > int8_t tsc_shift; > - int8_t pad1[3]; > + uint8_t flags; > + uint8_t pad1[2]; > }; /* 32 bytes */ I've just noticed there's another issue here: The new structure layout should be surfaced only conditionally (evaluating __XEN_INTERFACE_VERSION__). This of course could again be addressed while committing, is - as Ian had asked for - this is to go in ahead of the rest of the series. Jan
On 04/05/2016 11:16 AM, Jan Beulich wrote: >>>> On 29.03.16 at 15:44, <joao.m.martins@oracle.com> wrote: >> --- a/xen/include/public/xen.h >> +++ b/xen/include/public/xen.h >> @@ -614,10 +614,14 @@ struct vcpu_time_info { >> */ >> uint32_t tsc_to_system_mul; >> int8_t tsc_shift; >> - int8_t pad1[3]; >> + uint8_t flags; >> + uint8_t pad1[2]; >> }; /* 32 bytes */ > > I've just noticed there's another issue here: The new structure > layout should be surfaced only conditionally (evaluating > __XEN_INTERFACE_VERSION__). This of course could again be > addressed while committing, is - as Ian had asked for - this is to > go in ahead of the rest of the series. > I will fix this too on my end if this isn't going ahead of the rest of the series.
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h index 64ba7ab..b263fe3 100644 --- a/xen/include/public/xen.h +++ b/xen/include/public/xen.h @@ -614,10 +614,14 @@ struct vcpu_time_info { */ uint32_t tsc_to_system_mul; int8_t tsc_shift; - int8_t pad1[3]; + uint8_t flags; + uint8_t pad1[2]; }; /* 32 bytes */ typedef struct vcpu_time_info vcpu_time_info_t; +#define PVCLOCK_TSC_STABLE_BIT (1 << 0) +#define PVCLOCK_GUEST_STOPPED (1 << 1) + struct vcpu_info { /* * 'evtchn_upcall_pending' is written non-zero by Xen to indicate
This field has two possible flags (as of latest pvclock ABI shared with KVM). flags: bits in this field indicate extended capabilities coordinated between the guest and the hypervisor. Specifically on KVM, availability of specific flags has to be checked in 0x40000001 cpuid leaf. On Xen, we don't have that but we can still check some of the flags after registering the time info page since a force_update_vcpu_system_time is performed. Current flags are: flag bit | cpuid bit | meaning ------------------------------------------------------------- | | time measures taken across 0 | 24 | multiple cpus are guaranteed to | | be monotonic ------------------------------------------------------------- | | guest vcpu has been paused by 1 | N/A | the host | | ------------------------------------------------------------- Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Keir Fraser <keir@xen.org> Cc: Tim Deegan <tim@xen.org> Changes since v1: - flags and pad are both uint8_t. - fix indentation with the other fields in the struct. --- xen/include/public/xen.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)