[Xen-devel] [PATCH] passthrough/pci: properly qualify the mem_sharing_enabled check...

Paul Durrant posted 1 patch 16 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/20190701131750.37855-1-paul.durrant@citrix.com
xen/arch/x86/mm/mem_sharing.c    | 3 ---
xen/drivers/passthrough/pci.c    | 2 +-
xen/include/asm-x86/hvm/domain.h | 3 +++
3 files changed, 4 insertions(+), 4 deletions(-)

[Xen-devel] [PATCH] passthrough/pci: properly qualify the mem_sharing_enabled check...

Posted by Paul Durrant 16 weeks ago
...in assign_device().

The check of arch.hvm.mem_sharing_enabled flag at the top of
drivers/passthrough/pci.c:assign_device() does not make sure that the
domain is actually an HVM domain.

This patch fixes the issue by moving the definition of the
mem_sharing_enabled() macro from mem_sharing.c into asm-86/hvm/domain.h and
then using that instead.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Tamas K Lengyel <tamas@tklengyel.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
 xen/arch/x86/mm/mem_sharing.c    | 3 ---
 xen/drivers/passthrough/pci.c    | 2 +-
 xen/include/asm-x86/hvm/domain.h | 3 +++
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index f16a3f5324..db02f4dfd5 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -149,9 +149,6 @@ static inline shr_handle_t get_next_handle(void)
     return x + 1;
 }
 
-#define mem_sharing_enabled(d) \
-    (is_hvm_domain(d) && (d)->arch.hvm.mem_sharing_enabled)
-
 static atomic_t nr_saved_mfns   = ATOMIC_INIT(0); 
 static atomic_t nr_shared_mfns  = ATOMIC_INIT(0);
 
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index e88689425d..7697ec440d 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1450,7 +1450,7 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
 
     /* Prevent device assign if mem paging or mem sharing have been 
      * enabled for this domain */
-    if ( unlikely(d->arch.hvm.mem_sharing_enabled ||
+    if ( unlikely(mem_sharing_enabled(d) ||
                   vm_event_check_ring(d->vm_event_paging) ||
                   p2m_get_hostp2m(d)->global_logdirty) )
         return -EXDEV;
diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
index 6c7c4f5aa6..1e6e3ce078 100644
--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -197,8 +197,11 @@ struct hvm_domain {
 
 #ifdef CONFIG_HVM
 #define hap_enabled(d)  (is_hvm_domain(d) && (d)->arch.hvm.hap_enabled)
+#define mem_sharing_enabled(d) \
+    (is_hvm_domain(d) && (d)->arch.hvm.mem_sharing_enabled)
 #else
 #define hap_enabled(d)  ({(void)(d); false;})
+#define mem_sharing_enabled(d)  ({(void)(d); false;})
 #endif
 
 #endif /* __ASM_X86_HVM_DOMAIN_H__ */
-- 
2.20.1.2.gb21ebb671


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] passthrough/pci: properly qualify the mem_sharing_enabled check...

Posted by Jan Beulich 16 weeks ago
On 01.07.2019 15:17, Paul Durrant wrote:
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1450,7 +1450,7 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
>   
>       /* Prevent device assign if mem paging or mem sharing have been
>        * enabled for this domain */
> -    if ( unlikely(d->arch.hvm.mem_sharing_enabled ||
> +    if ( unlikely(mem_sharing_enabled(d) ||
>                     vm_event_check_ring(d->vm_event_paging) ||
>                     p2m_get_hostp2m(d)->global_logdirty) )
>           return -EXDEV;

This change is redundant with the more extensive one by
"x86/HVM: p2m_ram_ro is incompatible with device pass-through",
of which I've sent v2 earlier today, and v1 of which has been
pending for quite some time without having heard back from
other than you.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] passthrough/pci: properly qualify the mem_sharing_enabled check...

Posted by Paul Durrant 16 weeks ago
> -----Original Message-----
> From: Jan Beulich <JBeulich@suse.com>
> Sent: 03 July 2019 14:20
> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Tamas K Lengyel <tamas@tklengyel.com>; WeiLiu <wl@xen.org>
> Subject: Re: [PATCH] passthrough/pci: properly qualify the mem_sharing_enabled check...
> 
> On 01.07.2019 15:17, Paul Durrant wrote:
> > --- a/xen/drivers/passthrough/pci.c
> > +++ b/xen/drivers/passthrough/pci.c
> > @@ -1450,7 +1450,7 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32
> flag)
> >
> >       /* Prevent device assign if mem paging or mem sharing have been
> >        * enabled for this domain */
> > -    if ( unlikely(d->arch.hvm.mem_sharing_enabled ||
> > +    if ( unlikely(mem_sharing_enabled(d) ||
> >                     vm_event_check_ring(d->vm_event_paging) ||
> >                     p2m_get_hostp2m(d)->global_logdirty) )
> >           return -EXDEV;
> 
> This change is redundant with the more extensive one by
> "x86/HVM: p2m_ram_ro is incompatible with device pass-through",
> of which I've sent v2 earlier today, and v1 of which has been
> pending for quite some time without having heard back from
> other than you.

Agreed, but I still think it's a good idea to move the mem_sharing_enabled() macro into domain.h, so maybe incorporate that into your patch? I'll take a look at v2 as soon as I can.

  Paul

> 
> Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] passthrough/pci: properly qualify the mem_sharing_enabled check...

Posted by Jan Beulich 16 weeks ago
On 03.07.2019 15:25, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <JBeulich@suse.com>
>> Sent: 03 July 2019 14:20
>> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; George Dunlap
>> <George.Dunlap@citrix.com>; Tamas K Lengyel <tamas@tklengyel.com>; WeiLiu <wl@xen.org>
>> Subject: Re: [PATCH] passthrough/pci: properly qualify the mem_sharing_enabled check...
>>
>> On 01.07.2019 15:17, Paul Durrant wrote:
>>> --- a/xen/drivers/passthrough/pci.c
>>> +++ b/xen/drivers/passthrough/pci.c
>>> @@ -1450,7 +1450,7 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32
>> flag)
>>>
>>>        /* Prevent device assign if mem paging or mem sharing have been
>>>         * enabled for this domain */
>>> -    if ( unlikely(d->arch.hvm.mem_sharing_enabled ||
>>> +    if ( unlikely(mem_sharing_enabled(d) ||
>>>                      vm_event_check_ring(d->vm_event_paging) ||
>>>                      p2m_get_hostp2m(d)->global_logdirty) )
>>>            return -EXDEV;
>>
>> This change is redundant with the more extensive one by
>> "x86/HVM: p2m_ram_ro is incompatible with device pass-through",
>> of which I've sent v2 earlier today, and v1 of which has been
>> pending for quite some time without having heard back from
>> other than you.
> 
> Agreed, but I still think it's a good idea to move the
> mem_sharing_enabled() macro into domain.h, so maybe incorporate
> that into your patch?

Hmm, that would actually have been a question of mine if I hadn't
recognized the redundancy: Why domain.h rather than mem_sharing.h?
Furthermore I'd rather not use it here anyway, to avoid two
consecutive is_hvm_*() checks (which have become slightly more
expensive with the LFENCE addition).

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] passthrough/pci: properly qualify the mem_sharing_enabled check...

Posted by Paul Durrant 16 weeks ago
> -----Original Message-----
> From: Jan Beulich <JBeulich@suse.com>
> Sent: 03 July 2019 14:30
> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; Roger Pau
> Monne <roger.pau@citrix.com>; Tamas KLengyel <tamas@tklengyel.com>; WeiLiu <wl@xen.org>
> Subject: Re: [PATCH] passthrough/pci: properly qualify the mem_sharing_enabled check...
> 
> On 03.07.2019 15:25, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <JBeulich@suse.com>
> >> Sent: 03 July 2019 14:20
> >> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
> >> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; George
> Dunlap
> >> <George.Dunlap@citrix.com>; Tamas K Lengyel <tamas@tklengyel.com>; WeiLiu <wl@xen.org>
> >> Subject: Re: [PATCH] passthrough/pci: properly qualify the mem_sharing_enabled check...
> >>
> >> On 01.07.2019 15:17, Paul Durrant wrote:
> >>> --- a/xen/drivers/passthrough/pci.c
> >>> +++ b/xen/drivers/passthrough/pci.c
> >>> @@ -1450,7 +1450,7 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32
> >> flag)
> >>>
> >>>        /* Prevent device assign if mem paging or mem sharing have been
> >>>         * enabled for this domain */
> >>> -    if ( unlikely(d->arch.hvm.mem_sharing_enabled ||
> >>> +    if ( unlikely(mem_sharing_enabled(d) ||
> >>>                      vm_event_check_ring(d->vm_event_paging) ||
> >>>                      p2m_get_hostp2m(d)->global_logdirty) )
> >>>            return -EXDEV;
> >>
> >> This change is redundant with the more extensive one by
> >> "x86/HVM: p2m_ram_ro is incompatible with device pass-through",
> >> of which I've sent v2 earlier today, and v1 of which has been
> >> pending for quite some time without having heard back from
> >> other than you.
> >
> > Agreed, but I still think it's a good idea to move the
> > mem_sharing_enabled() macro into domain.h, so maybe incorporate
> > that into your patch?
> 
> Hmm, that would actually have been a question of mine if I hadn't
> recognized the redundancy: Why domain.h rather than mem_sharing.h?
> Furthermore I'd rather not use it here anyway, to avoid two
> consecutive is_hvm_*() checks (which have become slightly more
> expensive with the LFENCE addition).

Maybe mem_sharing.h is a more logical location, but I think providing a macro to make sure the is_hvm_domain() protection is there is a good thing. As for using it here, yes I guess the addition of the speculation barrier makes it more expensive but then is not really performance critical code so I'd go for tidiness.

  Paul


> 
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel