Xen devel - Mar 2013 - [PATCH 6/18 V2]: PVH xen: Introduce PVH guest type

This patch introduces the concept of pvh guest. There also other basic
changes like creating macros to check for pvh vcpu/domain, and creating
new macros to see if it''s pvh/hvm domain/vcpu. Also, modify copy macros
to test for pvh. Lastly, we introduce that PVH uses HVM style event
delivery.

Chagnes in V2:
  - make is_pvh/is_hvm enum instead of adding is_pvh as a new flag.
  - fix indentation and spacing in guest_kernel_mode macro.
  - add debug only BUG() in GUEST_KERNEL_RPL macro as it should no longer
    be called in any PVH paths.

Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
---
 xen/arch/x86/debug.c               |    2 +-
 xen/arch/x86/domain.c              |    7 +++++++
 xen/common/domain.c                |    2 +-
 xen/include/asm-x86/desc.h         |    5 +++++
 xen/include/asm-x86/domain.h       |    9 ++++-----
 xen/include/asm-x86/event.h        |    2 +-
 xen/include/asm-x86/guest_access.h |   12 ++++++------
 xen/include/asm-x86/x86_64/regs.h  |    9 +++++----
 xen/include/xen/sched.h            |   17 ++++++++++++++---
 9 files changed, 44 insertions(+), 21 deletions(-)

diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
index e67473e..502edbc 100644
--- a/xen/arch/x86/debug.c
+++ b/xen/arch/x86/debug.c
@@ -158,7 +158,7 @@ dbg_rw_guest_mem(dbgva_t addr, dbgbyte_t *buf, int len,
struct domain *dp,
 
         pagecnt = min_t(long, PAGE_SIZE - (addr & ~PAGE_MASK), len);
 
-        mfn = (dp->is_hvm
+        mfn = (is_hvm_domain(dp)
                ? dbg_hvm_va2mfn(addr, dp, toaddr, &gfn)
                : dbg_pv_va2mfn(addr, dp, pgd3));
         if ( mfn == INVALID_MFN ) 
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index e9549e0..768c19d 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -649,6 +649,13 @@ int arch_set_info_guest(
     unsigned int i;
     int rc = 0, compat;
 
+    /* This removed when all patches are checked in */
+    if ( is_pvh_vcpu(v) )
+    {
+        printk("PVH: You don''t have the correct xen version for
PVH\n");
+        return -EINVAL;
+    }
+
     /* The context is a compat-mode one if the target domain is compat-mode;
      * we expect the tools to DTRT even in compat-mode callers. */
     compat = is_pv_32on64_domain(d);
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 64ee29d..b6f10b7 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -231,7 +231,7 @@ struct domain *domain_create(
         goto fail;
 
     if ( domcr_flags & DOMCRF_hvm )
-        d->is_hvm = 1;
+        d->guest_type = hvm_guest;
 
     if ( domid == 0 )
     {
diff --git a/xen/include/asm-x86/desc.h b/xen/include/asm-x86/desc.h
index 354b889..4dca0a3 100644
--- a/xen/include/asm-x86/desc.h
+++ b/xen/include/asm-x86/desc.h
@@ -38,7 +38,12 @@
 
 #ifndef __ASSEMBLY__
 
+#ifndef NDEBUG
+#define GUEST_KERNEL_RPL(d) (is_pvh_domain(d) ? ({ BUG(); 0; }) :    \
+                                                is_pv_32bit_domain(d) ? 1 : 3)
+#else
 #define GUEST_KERNEL_RPL(d) (is_pv_32bit_domain(d) ? 1 : 3)
+#endif
 
 /* Fix up the RPL of a guest segment selector. */
 #define __fixup_guest_selector(d, sel)                             \
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 97e09ca..ecb3058 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -16,7 +16,7 @@
 #define is_pv_32on64_domain(d) (is_pv_32bit_domain(d))
 #define is_pv_32on64_vcpu(v)   (is_pv_32on64_domain((v)->domain))
 
-#define is_hvm_pv_evtchn_domain(d) (is_hvm_domain(d) && \
+#define is_hvm_pv_evtchn_domain(d) (is_hvm_or_pvh_domain(d) && \
         d->arch.hvm_domain.irq.callback_via_type == HVMIRQ_callback_vector)
 #define is_hvm_pv_evtchn_vcpu(v) (is_hvm_pv_evtchn_domain(v->domain))
 
@@ -254,10 +254,9 @@ struct arch_domain
 
     struct list_head pdev_list;
 
-    union {
-        struct pv_domain pv_domain;
-        struct hvm_domain hvm_domain;
-    };
+    /* PVH : pvh uses fields from both pv and hvm, so separate the union */
+    struct pv_domain pv_domain;
+    struct hvm_domain hvm_domain;
 
     struct paging_domain paging;
     struct p2m_domain *p2m;
diff --git a/xen/include/asm-x86/event.h b/xen/include/asm-x86/event.h
index 06057c7..9187606 100644
--- a/xen/include/asm-x86/event.h
+++ b/xen/include/asm-x86/event.h
@@ -18,7 +18,7 @@ int hvm_local_events_need_delivery(struct vcpu *v);
 static inline int local_events_need_delivery(void)
 {
     struct vcpu *v = current;
-    return (is_hvm_vcpu(v) ? hvm_local_events_need_delivery(v) :
+    return (is_hvm_or_pvh_vcpu(v) ? hvm_local_events_need_delivery(v) :
             (vcpu_info(v, evtchn_upcall_pending) &&
              !vcpu_info(v, evtchn_upcall_mask)));
 }
diff --git a/xen/include/asm-x86/guest_access.h
b/xen/include/asm-x86/guest_access.h
index ca700c9..1839fa4 100644
--- a/xen/include/asm-x86/guest_access.h
+++ b/xen/include/asm-x86/guest_access.h
@@ -14,27 +14,27 @@
 
 /* Raw access functions: no type checking. */
 #define raw_copy_to_guest(dst, src, len)        \
-    (is_hvm_vcpu(current) ?                     \
+    (is_hvm_or_pvh_vcpu(current) ?              \
      copy_to_user_hvm((dst), (src), (len)) :    \
      copy_to_user((dst), (src), (len)))
 #define raw_copy_from_guest(dst, src, len)      \
-    (is_hvm_vcpu(current) ?                     \
+    (is_hvm_or_pvh_vcpu(current) ?              \
      copy_from_user_hvm((dst), (src), (len)) :  \
      copy_from_user((dst), (src), (len)))
 #define raw_clear_guest(dst,  len)              \
-    (is_hvm_vcpu(current) ?                     \
+    (is_hvm_or_pvh_vcpu(current) ?              \
      clear_user_hvm((dst), (len)) :             \
      clear_user((dst), (len)))
 #define __raw_copy_to_guest(dst, src, len)      \
-    (is_hvm_vcpu(current) ?                     \
+    (is_hvm_or_pvh_vcpu(current) ?              \
      copy_to_user_hvm((dst), (src), (len)) :    \
      __copy_to_user((dst), (src), (len)))
 #define __raw_copy_from_guest(dst, src, len)    \
-    (is_hvm_vcpu(current) ?                     \
+    (is_hvm_or_pvh_vcpu(current) ?              \
      copy_from_user_hvm((dst), (src), (len)) :  \
      __copy_from_user((dst), (src), (len)))
 #define __raw_clear_guest(dst,  len)            \
-    (is_hvm_vcpu(current) ?                     \
+    (is_hvm_or_pvh_vcpu(current) ?              \
      clear_user_hvm((dst), (len)) :             \
      clear_user((dst), (len)))
 
diff --git a/xen/include/asm-x86/x86_64/regs.h
b/xen/include/asm-x86/x86_64/regs.h
index 3cdc702..bb475cf 100644
--- a/xen/include/asm-x86/x86_64/regs.h
+++ b/xen/include/asm-x86/x86_64/regs.h
@@ -10,10 +10,11 @@
 #define ring_2(r)    (((r)->cs & 3) == 2)
 #define ring_3(r)    (((r)->cs & 3) == 3)
 
-#define guest_kernel_mode(v, r)                                 \
-    (!is_pv_32bit_vcpu(v) ?                                     \
-     (ring_3(r) && ((v)->arch.flags & TF_kernel_mode)) :       
\
-     (ring_1(r)))
+#define guest_kernel_mode(v, r)                                   \
+    (is_pvh_vcpu(v) ? ({ ASSERT(v == current); ring_0(r); }) :    \
+     (!is_pv_32bit_vcpu(v) ?                                      \
+      (ring_3(r) && ((v)->arch.flags & TF_kernel_mode)) :       
\
+      (ring_1(r))))
 
 #define permit_softint(dpl, v, r) \
     ((dpl) >= (guest_kernel_mode(v, r) ? 1 : 3))
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 569e76e..079daff 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -230,6 +230,9 @@ struct mem_event_per_domain
     struct mem_event_domain access;
 };
 
+/* PVH is a PV guest running in HVM container. While is_hvm is false for it, 
+ * it uses many of the HVM data structs.
+ */
 struct domain
 {
     domid_t          domain_id;
@@ -277,8 +280,8 @@ struct domain
     struct rangeset *iomem_caps;
     struct rangeset *irq_caps;
 
-    /* Is this an HVM guest? */
-    bool_t           is_hvm;
+    /* !is_pvh && !is_hvm ==> PV, else PVH or HVM */
+    enum {hvm_guest=1, pvh_guest} guest_type;
 #ifdef HAS_PASSTHROUGH
     /* Does this guest need iommu mappings? */
     bool_t           need_iommu;
@@ -450,6 +453,10 @@ struct domain *domain_create(
  /* DOMCRF_oos_off: dont use out-of-sync optimization for shadow page tables */
 #define _DOMCRF_oos_off         4
 #define DOMCRF_oos_off          (1U<<_DOMCRF_oos_off)
+ /* DOMCRF_pvh: Create PV domain in HVM container */
+#define _DOMCRF_pvh            5
+#define DOMCRF_pvh             (1U<<_DOMCRF_pvh)
+
 
 /*
  * rcu_lock_domain_by_id() is more efficient than get_domain_by_id().
@@ -718,10 +725,14 @@ void watchdog_domain_destroy(struct domain *d);
 
 #define VM_ASSIST(_d,_t) (test_bit((_t), &(_d)->vm_assist))
 
-#define is_hvm_domain(d) ((d)->is_hvm)
+#define is_hvm_domain(d) ((d)->guest_type == hvm_guest)
 #define is_hvm_vcpu(v)   (is_hvm_domain(v->domain))
+#define is_pvh_domain(d) ((d)->guest_type == pvh_guest)
+#define is_pvh_vcpu(v)   (is_pvh_domain(v->domain))
 #define is_pinned_vcpu(v) ((v)->domain->is_pinned || \
                            cpumask_weight((v)->cpu_affinity) == 1)
+#define is_hvm_or_pvh_domain(d) (is_hvm_domain(d) || is_pvh_domain(d))
+#define is_hvm_or_pvh_vcpu(v) (is_hvm_or_pvh_domain(v->domain))
 #ifdef HAS_PASSTHROUGH
 #define need_iommu(d)    ((d)->need_iommu)
 #else
-- 
1.7.2.3

>>> On 16.03.13 at 01:32, Mukesh Rathor
<mukesh.rathor@oracle.com> wrote:
> @@ -277,8 +280,8 @@ struct domain
>      struct rangeset *iomem_caps;
>      struct rangeset *irq_caps;
>  
> -    /* Is this an HVM guest? */
> -    bool_t           is_hvm;
> +    /* !is_pvh && !is_hvm ==> PV, else PVH or HVM */
> +    enum {hvm_guest=1, pvh_guest} guest_type;
Even if not used explicitly anywhere right now, please make the PV
guest case explicit here too (at once avoiding the =1).

Quite likely we''ll want to also have the enum have a tag, so we
can eventually pass value from this enumeration to functions.

I''m also mildly puzzled by you using "_guest" suffixes here
rather
than, as usual, a common prefix.

And of course, please properly format this.
> @@ -718,10 +725,14 @@ void watchdog_domain_destroy(struct domain *d);
>  
>  #define VM_ASSIST(_d,_t) (test_bit((_t), &(_d)->vm_assist))
>  
> -#define is_hvm_domain(d) ((d)->is_hvm)
> +#define is_hvm_domain(d) ((d)->guest_type == hvm_guest)
>  #define is_hvm_vcpu(v)   (is_hvm_domain(v->domain))
> +#define is_pvh_domain(d) ((d)->guest_type == pvh_guest)
> +#define is_pvh_vcpu(v)   (is_pvh_domain(v->domain))
>  #define is_pinned_vcpu(v) ((v)->domain->is_pinned || \
>                             cpumask_weight((v)->cpu_affinity) == 1)
> +#define is_hvm_or_pvh_domain(d) (is_hvm_domain(d) || is_pvh_domain(d))
> +#define is_hvm_or_pvh_vcpu(v) (is_hvm_or_pvh_domain(v->domain))
These surely can have better names, if they''re needed at all:
Wouldn''t !is_pv_domain() do what you need?

Jan

On Mon, 18 Mar 2013 11:54:29 +0000
"Jan Beulich" <JBeulich@suse.com> wrote:
> >>> On 16.03.13 at 01:32, Mukesh Rathor
<mukesh.rathor@oracle.com>
> >>> wrote:
> > @@ -277,8 +280,8 @@ struct domain
> >      struct rangeset *iomem_caps;
> >      struct rangeset *irq_caps;
> >  
> > -    /* Is this an HVM guest? */
> > -    bool_t           is_hvm;
> > +    /* !is_pvh && !is_hvm ==> PV, else PVH or HVM */
> > +    enum {hvm_guest=1, pvh_guest} guest_type;
> 
> Even if not used explicitly anywhere right now, please make the PV
> guest case explicit here too (at once avoiding the =1).
Ok.
> Quite likely we''ll want to also have the enum have a tag, so we
> can eventually pass value from this enumeration to functions.
 
Ok.
> I''m also mildly puzzled by you using "_guest" suffixes
here rather
> than, as usual, a common prefix.
> 
> And of course, please properly format this.
Not sure I follow what needs formatting?
 
> > @@ -718,10 +725,14 @@ void watchdog_domain_destroy(struct domain
> > *d); 
> >  #define VM_ASSIST(_d,_t) (test_bit((_t), &(_d)->vm_assist))
> >  
> > -#define is_hvm_domain(d) ((d)->is_hvm)
> > +#define is_hvm_domain(d) ((d)->guest_type == hvm_guest)
> >  #define is_hvm_vcpu(v)   (is_hvm_domain(v->domain))
> > +#define is_pvh_domain(d) ((d)->guest_type == pvh_guest)
> > +#define is_pvh_vcpu(v)   (is_pvh_domain(v->domain))
> >  #define is_pinned_vcpu(v) ((v)->domain->is_pinned || \
> >                             cpumask_weight((v)->cpu_affinity) == 1)
> > +#define is_hvm_or_pvh_domain(d) (is_hvm_domain(d) ||
> > is_pvh_domain(d)) +#define is_hvm_or_pvh_vcpu(v)
> > (is_hvm_or_pvh_domain(v->domain))
> 
> These surely can have better names, if they''re needed at all:
> Wouldn''t !is_pv_domain() do what you need?
Nop, that''s more confusing, since PVH is a PV domain. So, I suggest
we leave it as is. is_hvm_or_pvh_domain is nicely readable, what name
do you suggest?

Thanks,
Mukesh

>>> On 19.03.13 at 01:21, Mukesh Rathor
<mukesh.rathor@oracle.com> wrote:
> On Mon, 18 Mar 2013 11:54:29 +0000
> "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>> >>> On 16.03.13 at 01:32, Mukesh Rathor
<mukesh.rathor@oracle.com>
>> >>> wrote:
>> > @@ -277,8 +280,8 @@ struct domain
>> >      struct rangeset *iomem_caps;
>> >      struct rangeset *irq_caps;
>> >  
>> > -    /* Is this an HVM guest? */
>> > -    bool_t           is_hvm;
>> > +    /* !is_pvh && !is_hvm ==> PV, else PVH or HVM */
>> > +    enum {hvm_guest=1, pvh_guest} guest_type;
>> 
>> And of course, please properly format this.
> 
> Not sure I follow what needs formatting?
A number of blanks need to be inserted. And the whole enum
declaration probably doesn''t belong on a single line anyway.

And as pointed out elsewhere - your whole series is in need of
making it conform to coding conventions.
>> > @@ -718,10 +725,14 @@ void watchdog_domain_destroy(struct domain
>> > *d); 
>> >  #define VM_ASSIST(_d,_t) (test_bit((_t),
&(_d)->vm_assist))
>> >  
>> > -#define is_hvm_domain(d) ((d)->is_hvm)
>> > +#define is_hvm_domain(d) ((d)->guest_type == hvm_guest)
>> >  #define is_hvm_vcpu(v)   (is_hvm_domain(v->domain))
>> > +#define is_pvh_domain(d) ((d)->guest_type == pvh_guest)
>> > +#define is_pvh_vcpu(v)   (is_pvh_domain(v->domain))
>> >  #define is_pinned_vcpu(v) ((v)->domain->is_pinned || \
>> >                             cpumask_weight((v)->cpu_affinity)
== 1)
>> > +#define is_hvm_or_pvh_domain(d) (is_hvm_domain(d) ||
>> > is_pvh_domain(d)) +#define is_hvm_or_pvh_vcpu(v)
>> > (is_hvm_or_pvh_domain(v->domain))
>> 
>> These surely can have better names, if they''re needed at all:
>> Wouldn''t !is_pv_domain() do what you need?
> 
> Nop, that''s more confusing, since PVH is a PV domain. So, I
suggest
> we leave it as is. is_hvm_or_pvh_domain is nicely readable, what name
> do you suggest?
No. The three kinds should be fully distinct, such that when
meaning one you can use is_xyz_domain() and when meaning
two, you can use !is_abc_domain().

is_hvm_or_pvh_domain() isn''t nicely readable to me, in particular
because this kind of naming doesn''t scale. And it''s certainly
more
typing than !is_pv_domain().

Jan

On Tue, Mar 19, 2013 at 08:48:53AM +0000, Jan Beulich
wrote:
> >>> On 19.03.13 at 01:21, Mukesh Rathor
<mukesh.rathor@oracle.com> wrote:
> > On Mon, 18 Mar 2013 11:54:29 +0000
> > "Jan Beulich" <JBeulich@suse.com> wrote:
> > 
> >> >>> On 16.03.13 at 01:32, Mukesh Rathor
<mukesh.rathor@oracle.com>
> >> >>> wrote:
> >> > @@ -277,8 +280,8 @@ struct domain
> >> >      struct rangeset *iomem_caps;
> >> >      struct rangeset *irq_caps;
> >> >  
> >> > -    /* Is this an HVM guest? */
> >> > -    bool_t           is_hvm;
> >> > +    /* !is_pvh && !is_hvm ==> PV, else PVH or HVM
*/
> >> > +    enum {hvm_guest=1, pvh_guest} guest_type;
> >> 
> >> And of course, please properly format this.
> > 
> > Not sure I follow what needs formatting?
> 
> A number of blanks need to be inserted. And the whole enum
> declaration probably doesn''t belong on a single line anyway.
The single line looks to be already there:

enum { DOMDYING_alive, DOMDYING_dying, DOMDYING_dead } is_dying;

So I think Mukesh is just following the same convention in that
file. Should he depart from it?

>>> On 19.03.13 at 14:03, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Tue, Mar 19, 2013 at 08:48:53AM +0000, Jan Beulich wrote:
>> >>> On 19.03.13 at 01:21, Mukesh Rathor
<mukesh.rathor@oracle.com> wrote:
>> > On Mon, 18 Mar 2013 11:54:29 +0000
>> > "Jan Beulich" <JBeulich@suse.com> wrote:
>> > 
>> >> >>> On 16.03.13 at 01:32, Mukesh Rathor
<mukesh.rathor@oracle.com>
>> >> >>> wrote:
>> >> > @@ -277,8 +280,8 @@ struct domain
>> >> >      struct rangeset *iomem_caps;
>> >> >      struct rangeset *irq_caps;
>> >> >  
>> >> > -    /* Is this an HVM guest? */
>> >> > -    bool_t           is_hvm;
>> >> > +    /* !is_pvh && !is_hvm ==> PV, else PVH or
HVM */
>> >> > +    enum {hvm_guest=1, pvh_guest} guest_type;
>> >> 
>> >> And of course, please properly format this.
>> > 
>> > Not sure I follow what needs formatting?
>> 
>> A number of blanks need to be inserted. And the whole enum
>> declaration probably doesn''t belong on a single line anyway.
> 
> The single line looks to be already there:
> 
> enum { DOMDYING_alive, DOMDYING_dying, DOMDYING_dead } is_dying;
> 
> So I think Mukesh is just following the same convention in that
> file. Should he depart from it?
Not necessarily, provided the line isn''t getting overly long.

Jan

On Tue, 19 Mar 2013 08:48:53 +0000
"Jan Beulich" <JBeulich@suse.com> wrote:
> >>> On 19.03.13 at 01:21, Mukesh Rathor
<mukesh.rathor@oracle.com>
> >>> wrote:
> > On Mon, 18 Mar 2013 11:54:29 +0000
> > "Jan Beulich" <JBeulich@suse.com> wrote:
>And as pointed out elsewhere - your whole series is in need of
>making it conform to coding conventions.
For the most part I looked at existing code in the same or different 
module for coding conventions.
> >> >  #define VM_ASSIST(_d,_t) (test_bit((_t),
&(_d)->vm_assist))
> >> >  
> >> > -#define is_hvm_domain(d) ((d)->is_hvm)
> >> > +#define is_hvm_domain(d) ((d)->guest_type == hvm_guest)
> >> >  #define is_hvm_vcpu(v)   (is_hvm_domain(v->domain))
> >> > +#define is_pvh_domain(d) ((d)->guest_type == pvh_guest)
> >> > +#define is_pvh_vcpu(v)   (is_pvh_domain(v->domain))
> >> >  #define is_pinned_vcpu(v) ((v)->domain->is_pinned || \
> >> >                            
cpumask_weight((v)->cpu_affinity) => >> > 1) +#define
is_hvm_or_pvh_domain(d) (is_hvm_domain(d) ||
> >> > is_pvh_domain(d)) +#define is_hvm_or_pvh_vcpu(v)
> >> > (is_hvm_or_pvh_domain(v->domain))
> >> 
> >> These surely can have better names, if they''re needed at
all:
> >> Wouldn''t !is_pv_domain() do what you need?
> > 
> > Nop, that''s more confusing, since PVH is a PV domain. So, I
suggest
> > we leave it as is. is_hvm_or_pvh_domain is nicely readable, what
> > name do you suggest?
> 
> No. The three kinds should be fully distinct, such that when
> meaning one you can use is_xyz_domain() and when meaning
> two, you can use !is_abc_domain().
> is_hvm_or_pvh_domain() isn''t nicely readable to me, in particular
> because this kind of naming doesn''t scale. And it''s
certainly more
> typing than !is_pv_domain().
Since, pvh is a pv domain, I don''t like using pv_guest for non PVH PV.
But perhaps I could use the name pv_mmu and have something like
following:

enum guest_type { is_mmu_pv, is_pvh_pv, is_hvm } guest_type;

Then:  is_hvm_or_pvh_domain()  becomes:  !is_mmu_pv().

Alternative to is_mmu_pv: is_pure_pv, is_orig_pv, ....

Let me know what you think.

thanks,
m-

>>> On 23.03.13 at 02:13, Mukesh Rathor
<mukesh.rathor@oracle.com> wrote:
> On Tue, 19 Mar 2013 08:48:53 +0000
> "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>> >>> On 19.03.13 at 01:21, Mukesh Rathor
<mukesh.rathor@oracle.com>
>> >>> wrote:
>> > On Mon, 18 Mar 2013 11:54:29 +0000
>> > "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>>And as pointed out elsewhere - your whole series is in need of
>>making it conform to coding conventions.
> 
> For the most part I looked at existing code in the same or different 
> module for coding conventions.
Which is the right approach, except that you should ignore bad
examples (namely "real" Xen sources not conforming to the
conventions, where "real" means not cloned from another source,
which most frequently would have been Linux).
>> >> >  #define VM_ASSIST(_d,_t) (test_bit((_t),
&(_d)->vm_assist))
>> >> >  
>> >> > -#define is_hvm_domain(d) ((d)->is_hvm)
>> >> > +#define is_hvm_domain(d) ((d)->guest_type ==
hvm_guest)
>> >> >  #define is_hvm_vcpu(v)   (is_hvm_domain(v->domain))
>> >> > +#define is_pvh_domain(d) ((d)->guest_type ==
pvh_guest)
>> >> > +#define is_pvh_vcpu(v)   (is_pvh_domain(v->domain))
>> >> >  #define is_pinned_vcpu(v) ((v)->domain->is_pinned
|| \
>> >> >                            
cpumask_weight((v)->cpu_affinity) =>> >> > 1) +#define
is_hvm_or_pvh_domain(d) (is_hvm_domain(d) ||
>> >> > is_pvh_domain(d)) +#define is_hvm_or_pvh_vcpu(v)
>> >> > (is_hvm_or_pvh_domain(v->domain))
>> >> 
>> >> These surely can have better names, if they''re needed
at all:
>> >> Wouldn''t !is_pv_domain() do what you need?
>> > 
>> > Nop, that''s more confusing, since PVH is a PV domain. So,
I suggest
>> > we leave it as is. is_hvm_or_pvh_domain is nicely readable, what
>> > name do you suggest?
>> 
>> No. The three kinds should be fully distinct, such that when
>> meaning one you can use is_xyz_domain() and when meaning
>> two, you can use !is_abc_domain().
> 
>> is_hvm_or_pvh_domain() isn''t nicely readable to me, in
particular
>> because this kind of naming doesn''t scale. And it''s
certainly more
>> typing than !is_pv_domain().
> 
> Since, pvh is a pv domain, I don''t like using pv_guest for non PVH
PV.
> But perhaps I could use the name pv_mmu and have something like
> following:
> 
> enum guest_type { is_mmu_pv, is_pvh_pv, is_hvm } guest_type;
> 
> Then:  is_hvm_or_pvh_domain()  becomes:  !is_mmu_pv().
> 
> Alternative to is_mmu_pv: is_pure_pv, is_orig_pv, ....
These are all ugly, and I don''t see why the triplet I suggested
(is_pv, is_pvh, and is_hvm), including their intended use, wouldn''t
be acceptable.

Jan

On Mon, 25 Mar 2013 09:26:51 +0000
"Jan Beulich" <JBeulich@suse.com> wrote:
> >>> On 23.03.13 at 02:13, Mukesh Rathor
<mukesh.rathor@oracle.com>
> >>> wrote:
> > On Tue, 19 Mar 2013 08:48:53 +0000
> > "Jan Beulich" <JBeulich@suse.com> wrote:
> > 
> >> > suggest we leave it as is. is_hvm_or_pvh_domain is nicely
> >> > readable, what name do you suggest?
> >> 
> >> No. The three kinds should be fully distinct, such that when
> >> meaning one you can use is_xyz_domain() and when meaning
> >> two, you can use !is_abc_domain().
> > 
> >> is_hvm_or_pvh_domain() isn''t nicely readable to me, in
particular
> >> because this kind of naming doesn''t scale. And
it''s certainly more
> >> typing than !is_pv_domain().
> > 
> > Since, pvh is a pv domain, I don''t like using pv_guest for
non PVH
> > PV. But perhaps I could use the name pv_mmu and have something like
> > following:
> > 
> > enum guest_type { is_mmu_pv, is_pvh_pv, is_hvm } guest_type;
> > 
> > Then:  is_hvm_or_pvh_domain()  becomes:  !is_mmu_pv().
> > 
> > Alternative to is_mmu_pv: is_pure_pv, is_orig_pv, ....
> 
> These are all ugly, and I don''t see why the triplet I suggested
> (is_pv, is_pvh, and is_hvm), including their intended use,
wouldn''t
> be acceptable.
Because this implies pvh is a new type, whereas like I said before,
PVH is a PV guest. Ok, lets go with your suggestion above, and if
people find it confusing, we can change in future.

Thanks,
Mukesh

On 25/03/2013 19:05, "Mukesh Rathor" <mukesh.rathor@oracle.com>
wrote:
>> These are all ugly, and I don''t see why the triplet I
suggested
>> (is_pv, is_pvh, and is_hvm), including their intended use,
wouldn''t
>> be acceptable.
> 
> Because this implies pvh is a new type, whereas like I said before,
> PVH is a PV guest. Ok, lets go with your suggestion above, and if
> people find it confusing, we can change in future.
It''s not really PV -- the interfaces and execution environment are
somewhat
different, evidence being that a legacy PV guest will not boot in PVH mode!
There are certainly similarities, but then there are between HVM and PV too
(e.g., many hypercalls), so at the end of the day a guest is one of
PV/PVH/HVM. So I have to agree with Jan.

 -- Keir

On Mon, 25 Mar 2013 20:07:10 +0000
Keir Fraser <keir.xen@gmail.com> wrote:
> On 25/03/2013 19:05, "Mukesh Rathor"
<mukesh.rathor@oracle.com> wrote:
> 
> >> These are all ugly, and I don''t see why the triplet I
suggested
> >> (is_pv, is_pvh, and is_hvm), including their intended use,
wouldn''t
> >> be acceptable.
> > 
> > Because this implies pvh is a new type, whereas like I said before,
> > PVH is a PV guest. Ok, lets go with your suggestion above, and if
> > people find it confusing, we can change in future.
> 
> It''s not really PV -- the interfaces and execution environment are
> somewhat different, evidence being that a legacy PV guest will not
> boot in PVH mode! There are certainly similarities, but then there
> are between HVM and PV too (e.g., many hypercalls), so at the end of
> the day a guest is one of PV/PVH/HVM. So I have to agree with Jan.
LOL... which is why originally I had called it hybrid (neither PV nor
HVM), but then many here convinced me it should be called a PV something
because it was a PV. Anyways, no more on this.

     { is_pv, is_pvh, is_hvm }
it is.

Thanks,
Mukesh