If run Xen with PVH dom0 and hvm domU, hvm will map a pirq for
a passthrough device by using gsi, see qemu code
xen_pt_realize->xc_physdev_map_pirq and libxl code
pci_add_dm_done->xc_physdev_map_pirq. Then xc_physdev_map_pirq
will call into Xen, but in hvm_physdev_op, PHYSDEVOP_map_pirq
is not allowed because currd is PVH dom0 and PVH has no
X86_EMU_USE_PIRQ flag, it will fail at has_pirq check.
So, allow PHYSDEVOP_map_pirq when dom0 is PVH and also allow
PHYSDEVOP_unmap_pirq for the failed path to unmap pirq. And
add a new check to prevent self map when subject domain has no
PIRQ flag.
Signed-off-by: Huang Rui <ray.huang@amd.com>
Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
xen/arch/x86/hvm/hypercall.c | 6 ++++++
xen/arch/x86/physdev.c | 24 ++++++++++++++++++++++++
2 files changed, 30 insertions(+)
diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index 0fab670a4871..fa5d50a0dd22 100644
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -71,8 +71,14 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
switch ( cmd )
{
+ /*
+ * Only being permitted for management of other domains.
+ * Further restrictions are enforced in do_physdev_op.
+ */
case PHYSDEVOP_map_pirq:
case PHYSDEVOP_unmap_pirq:
+ break;
+
case PHYSDEVOP_eoi:
case PHYSDEVOP_irq_status_query:
case PHYSDEVOP_get_free_pirq:
diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index 7efa17cf4c1e..61999882f836 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -305,11 +305,23 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
case PHYSDEVOP_map_pirq: {
physdev_map_pirq_t map;
struct msi_info msi;
+ struct domain *d;
ret = -EFAULT;
if ( copy_from_guest(&map, arg, 1) != 0 )
break;
+ d = rcu_lock_domain_by_any_id(map.domid);
+ if ( d == NULL )
+ return -ESRCH;
+ /* Prevent self-map when domain has no X86_EMU_USE_PIRQ flag */
+ if ( is_hvm_domain(d) && !has_pirq(d) && d == current->domain )
+ {
+ rcu_unlock_domain(d);
+ return -EOPNOTSUPP;
+ }
+ rcu_unlock_domain(d);
+
switch ( map.type )
{
case MAP_PIRQ_TYPE_MSI_SEG:
@@ -343,11 +355,23 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
case PHYSDEVOP_unmap_pirq: {
struct physdev_unmap_pirq unmap;
+ struct domain *d;
ret = -EFAULT;
if ( copy_from_guest(&unmap, arg, 1) != 0 )
break;
+ d = rcu_lock_domain_by_any_id(unmap.domid);
+ if ( d == NULL )
+ return -ESRCH;
+ /* Prevent self-unmap when domain has no X86_EMU_USE_PIRQ flag */
+ if ( is_hvm_domain(d) && !has_pirq(d) && d == current->domain )
+ {
+ rcu_unlock_domain(d);
+ return -EOPNOTSUPP;
+ }
+ rcu_unlock_domain(d);
+
ret = physdev_unmap_pirq(unmap.domid, unmap.pirq);
break;
}
--
2.34.1On 07.06.2024 10:11, Jiqian Chen wrote:
> If run Xen with PVH dom0 and hvm domU, hvm will map a pirq for
> a passthrough device by using gsi, see qemu code
> xen_pt_realize->xc_physdev_map_pirq and libxl code
> pci_add_dm_done->xc_physdev_map_pirq. Then xc_physdev_map_pirq
> will call into Xen, but in hvm_physdev_op, PHYSDEVOP_map_pirq
> is not allowed because currd is PVH dom0 and PVH has no
> X86_EMU_USE_PIRQ flag, it will fail at has_pirq check.
>
> So, allow PHYSDEVOP_map_pirq when dom0 is PVH and also allow
> PHYSDEVOP_unmap_pirq for the failed path to unmap pirq. And
> add a new check to prevent self map when subject domain has no
> PIRQ flag.
>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
What's imo missing in the description is a clarification / justification of
why it is going to be a good idea (or at least an acceptable one) to expose
the concept of PIRQs to PVH. If I'm not mistaken that concept so far has
been entirely a PV one.
> --- a/xen/arch/x86/hvm/hypercall.c
> +++ b/xen/arch/x86/hvm/hypercall.c
> @@ -71,8 +71,14 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>
> switch ( cmd )
> {
> + /*
> + * Only being permitted for management of other domains.
> + * Further restrictions are enforced in do_physdev_op.
> + */
> case PHYSDEVOP_map_pirq:
> case PHYSDEVOP_unmap_pirq:
> + break;
Nit: Imo such a comment ought to be indented like code (statements), not
like the case labels.
Jan
On 2024/6/10 23:58, Jan Beulich wrote:
> On 07.06.2024 10:11, Jiqian Chen wrote:
>> If run Xen with PVH dom0 and hvm domU, hvm will map a pirq for
>> a passthrough device by using gsi, see qemu code
>> xen_pt_realize->xc_physdev_map_pirq and libxl code
>> pci_add_dm_done->xc_physdev_map_pirq. Then xc_physdev_map_pirq
>> will call into Xen, but in hvm_physdev_op, PHYSDEVOP_map_pirq
>> is not allowed because currd is PVH dom0 and PVH has no
>> X86_EMU_USE_PIRQ flag, it will fail at has_pirq check.
>>
>> So, allow PHYSDEVOP_map_pirq when dom0 is PVH and also allow
>> PHYSDEVOP_unmap_pirq for the failed path to unmap pirq. And
>> add a new check to prevent self map when subject domain has no
>> PIRQ flag.
>>
>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>
> What's imo missing in the description is a clarification / justification of
> why it is going to be a good idea (or at least an acceptable one) to expose
> the concept of PIRQs to PVH. If I'm not mistaken that concept so far has
> been entirely a PV one.
I didn't want to expose the concept of PIRQs to PVH.
I did this patch is for HVM that use PIRQs, what I said in commit message is HVM will map a pirq for gsi, not PVH.
For the original code, it checks " !has_pirq(currd)", but currd is PVH dom0, so it failed. So I need to allow PHYSDEVOP_map_pirq
even currd has no PIRQs, but the subject domain has.
>
>> --- a/xen/arch/x86/hvm/hypercall.c
>> +++ b/xen/arch/x86/hvm/hypercall.c
>> @@ -71,8 +71,14 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>
>> switch ( cmd )
>> {
>> + /*
>> + * Only being permitted for management of other domains.
>> + * Further restrictions are enforced in do_physdev_op.
>> + */
>> case PHYSDEVOP_map_pirq:
>> case PHYSDEVOP_unmap_pirq:
>> + break;
>
> Nit: Imo such a comment ought to be indented like code (statements), not
> like the case labels.
Thanks, I will change in next version.
>
> Jan
--
Best regards,
Jiqian Chen.
On 12.06.2024 04:43, Chen, Jiqian wrote: > On 2024/6/10 23:58, Jan Beulich wrote: >> On 07.06.2024 10:11, Jiqian Chen wrote: >>> If run Xen with PVH dom0 and hvm domU, hvm will map a pirq for >>> a passthrough device by using gsi, see qemu code >>> xen_pt_realize->xc_physdev_map_pirq and libxl code >>> pci_add_dm_done->xc_physdev_map_pirq. Then xc_physdev_map_pirq >>> will call into Xen, but in hvm_physdev_op, PHYSDEVOP_map_pirq >>> is not allowed because currd is PVH dom0 and PVH has no >>> X86_EMU_USE_PIRQ flag, it will fail at has_pirq check. >>> >>> So, allow PHYSDEVOP_map_pirq when dom0 is PVH and also allow >>> PHYSDEVOP_unmap_pirq for the failed path to unmap pirq. And >>> add a new check to prevent self map when subject domain has no >>> PIRQ flag. >>> >>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> >>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> >> >> What's imo missing in the description is a clarification / justification of >> why it is going to be a good idea (or at least an acceptable one) to expose >> the concept of PIRQs to PVH. If I'm not mistaken that concept so far has >> been entirely a PV one. > I didn't want to expose the concept of PIRQs to PVH. > I did this patch is for HVM that use PIRQs, what I said in commit message is HVM will map a pirq for gsi, not PVH. > For the original code, it checks " !has_pirq(currd)", but currd is PVH dom0, so it failed. So I need to allow PHYSDEVOP_map_pirq > even currd has no PIRQs, but the subject domain has. But that's not what you're enforcing in do_physdev_op(). There you only prevent self-mapping. If I'm not mistaken all you need to do is drop the "d == current->domain" checks from those conditionals. Further see also https://lists.xen.org/archives/html/xen-devel/2024-06/msg00540.html. Jan
On 2024/6/12 16:53, Jan Beulich wrote: > On 12.06.2024 04:43, Chen, Jiqian wrote: >> On 2024/6/10 23:58, Jan Beulich wrote: >>> On 07.06.2024 10:11, Jiqian Chen wrote: >>>> If run Xen with PVH dom0 and hvm domU, hvm will map a pirq for >>>> a passthrough device by using gsi, see qemu code >>>> xen_pt_realize->xc_physdev_map_pirq and libxl code >>>> pci_add_dm_done->xc_physdev_map_pirq. Then xc_physdev_map_pirq >>>> will call into Xen, but in hvm_physdev_op, PHYSDEVOP_map_pirq >>>> is not allowed because currd is PVH dom0 and PVH has no >>>> X86_EMU_USE_PIRQ flag, it will fail at has_pirq check. >>>> >>>> So, allow PHYSDEVOP_map_pirq when dom0 is PVH and also allow >>>> PHYSDEVOP_unmap_pirq for the failed path to unmap pirq. And >>>> add a new check to prevent self map when subject domain has no >>>> PIRQ flag. >>>> >>>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> >>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> >>> >>> What's imo missing in the description is a clarification / justification of >>> why it is going to be a good idea (or at least an acceptable one) to expose >>> the concept of PIRQs to PVH. If I'm not mistaken that concept so far has >>> been entirely a PV one. >> I didn't want to expose the concept of PIRQs to PVH. >> I did this patch is for HVM that use PIRQs, what I said in commit message is HVM will map a pirq for gsi, not PVH. >> For the original code, it checks " !has_pirq(currd)", but currd is PVH dom0, so it failed. So I need to allow PHYSDEVOP_map_pirq >> even currd has no PIRQs, but the subject domain has. > > But that's not what you're enforcing in do_physdev_op(). There you only > prevent self-mapping. If I'm not mistaken all you need to do is drop the > "d == current->domain" checks from those conditionals. What I want is to allow PHYSDEVOP_map_pirq when currd doesn't have PIRQs, but subject domain has. Then I just add "break" in hvm_physdev_op without any checks, that will cause self-mapping problems. And in previous mail thread, you suggested me to prevent self-mapping when subject domain doesn't have PIRQs. So I added checks in do_physdev_op. > > Further see also > https://lists.xen.org/archives/html/xen-devel/2024-06/msg00540.html. > > Jan -- Best regards, Jiqian Chen.
On 12.06.2024 11:07, Chen, Jiqian wrote: > On 2024/6/12 16:53, Jan Beulich wrote: >> On 12.06.2024 04:43, Chen, Jiqian wrote: >>> On 2024/6/10 23:58, Jan Beulich wrote: >>>> On 07.06.2024 10:11, Jiqian Chen wrote: >>>>> If run Xen with PVH dom0 and hvm domU, hvm will map a pirq for >>>>> a passthrough device by using gsi, see qemu code >>>>> xen_pt_realize->xc_physdev_map_pirq and libxl code >>>>> pci_add_dm_done->xc_physdev_map_pirq. Then xc_physdev_map_pirq >>>>> will call into Xen, but in hvm_physdev_op, PHYSDEVOP_map_pirq >>>>> is not allowed because currd is PVH dom0 and PVH has no >>>>> X86_EMU_USE_PIRQ flag, it will fail at has_pirq check. >>>>> >>>>> So, allow PHYSDEVOP_map_pirq when dom0 is PVH and also allow >>>>> PHYSDEVOP_unmap_pirq for the failed path to unmap pirq. And >>>>> add a new check to prevent self map when subject domain has no >>>>> PIRQ flag. >>>>> >>>>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> >>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> >>>> >>>> What's imo missing in the description is a clarification / justification of >>>> why it is going to be a good idea (or at least an acceptable one) to expose >>>> the concept of PIRQs to PVH. If I'm not mistaken that concept so far has >>>> been entirely a PV one. >>> I didn't want to expose the concept of PIRQs to PVH. >>> I did this patch is for HVM that use PIRQs, what I said in commit message is HVM will map a pirq for gsi, not PVH. >>> For the original code, it checks " !has_pirq(currd)", but currd is PVH dom0, so it failed. So I need to allow PHYSDEVOP_map_pirq >>> even currd has no PIRQs, but the subject domain has. >> >> But that's not what you're enforcing in do_physdev_op(). There you only >> prevent self-mapping. If I'm not mistaken all you need to do is drop the >> "d == current->domain" checks from those conditionals. > What I want is to allow PHYSDEVOP_map_pirq when currd doesn't have PIRQs, but subject domain has. > Then I just add "break" in hvm_physdev_op without any checks, that will cause self-mapping problems. > And in previous mail thread, you suggested me to prevent self-mapping when subject domain doesn't have PIRQs. > So I added checks in do_physdev_op. Self-mapping was a primary concern of mine. Yet why deal with only a subset of what needs preventing, when generalizing things actually can be done by having less code. Jan
On 2024/6/12 17:21, Jan Beulich wrote: > On 12.06.2024 11:07, Chen, Jiqian wrote: >> On 2024/6/12 16:53, Jan Beulich wrote: >>> On 12.06.2024 04:43, Chen, Jiqian wrote: >>>> On 2024/6/10 23:58, Jan Beulich wrote: >>>>> On 07.06.2024 10:11, Jiqian Chen wrote: >>>>>> If run Xen with PVH dom0 and hvm domU, hvm will map a pirq for >>>>>> a passthrough device by using gsi, see qemu code >>>>>> xen_pt_realize->xc_physdev_map_pirq and libxl code >>>>>> pci_add_dm_done->xc_physdev_map_pirq. Then xc_physdev_map_pirq >>>>>> will call into Xen, but in hvm_physdev_op, PHYSDEVOP_map_pirq >>>>>> is not allowed because currd is PVH dom0 and PVH has no >>>>>> X86_EMU_USE_PIRQ flag, it will fail at has_pirq check. >>>>>> >>>>>> So, allow PHYSDEVOP_map_pirq when dom0 is PVH and also allow >>>>>> PHYSDEVOP_unmap_pirq for the failed path to unmap pirq. And >>>>>> add a new check to prevent self map when subject domain has no >>>>>> PIRQ flag. >>>>>> >>>>>> Signed-off-by: Huang Rui <ray.huang@amd.com> >>>>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> >>>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> >>>>> >>>>> What's imo missing in the description is a clarification / justification of >>>>> why it is going to be a good idea (or at least an acceptable one) to expose >>>>> the concept of PIRQs to PVH. If I'm not mistaken that concept so far has >>>>> been entirely a PV one. >>>> I didn't want to expose the concept of PIRQs to PVH. >>>> I did this patch is for HVM that use PIRQs, what I said in commit message is HVM will map a pirq for gsi, not PVH. >>>> For the original code, it checks " !has_pirq(currd)", but currd is PVH dom0, so it failed. So I need to allow PHYSDEVOP_map_pirq >>>> even currd has no PIRQs, but the subject domain has. >>> >>> But that's not what you're enforcing in do_physdev_op(). There you only >>> prevent self-mapping. If I'm not mistaken all you need to do is drop the >>> "d == current->domain" checks from those conditionals. >> What I want is to allow PHYSDEVOP_map_pirq when currd doesn't have PIRQs, but subject domain has. >> Then I just add "break" in hvm_physdev_op without any checks, that will cause self-mapping problems. >> And in previous mail thread, you suggested me to prevent self-mapping when subject domain doesn't have PIRQs. >> So I added checks in do_physdev_op. > > Self-mapping was a primary concern of mine. Yet why deal with only a subset > of what needs preventing, when generalizing things actually can be done by > having less code. Make sense. I will rebase the branch once your codes are merged. > > Jan -- Best regards, Jiqian Chen.
© 2016 - 2024 Red Hat, Inc.