| Message ID | 20240630123344.20623-3-Jiqian.Chen@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support device passthrough when dom0 is PVH on Xen | expand |
On 30.06.2024 14:33, 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 removal device path to unmap pirq. > And add a new check to prevent (un)map when the subject domain > has no X86_EMU_USE_PIRQ flag. > > So that the interrupt of a passthrough device can be > successfully mapped to pirq for domU with X86_EMU_USE_PIRQ flag > when dom0 is PVH > > Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> > 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> You keep carrying this R-b, despite making functional changes. This can't be quite right. While functionally I'm now okay with the change, I still have a code structure concern: > --- a/xen/arch/x86/physdev.c > +++ b/xen/arch/x86/physdev.c > @@ -323,6 +323,13 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > if ( !d ) > break; > > + /* Prevent mapping when the subject domain has no X86_EMU_USE_PIRQ */ > + if ( is_hvm_domain(d) && !has_pirq(d) ) > + { > + rcu_unlock_domain(d); > + return -EOPNOTSUPP; > + } > + > ret = physdev_map_pirq(d, map.type, &map.index, &map.pirq, &msi); > > rcu_unlock_domain(d); > @@ -346,6 +353,13 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > if ( !d ) > break; > > + /* Prevent unmapping when the subject domain has no X86_EMU_USE_PIRQ */ > + if ( is_hvm_domain(d) && !has_pirq(d) ) > + { > + rcu_unlock_domain(d); > + return -EOPNOTSUPP; > + } > + > ret = physdev_unmap_pirq(d, unmap.pirq); > > rcu_unlock_domain(d); If you did go look, you will have noticed that we use "return" in the middle of this function only very sparingly (when alternatives would result in more complicated code elsewhere). I think you want to avoid "return" here, too, and probably go even further and avoid the extra rcu_unlock_domain() as well. That's easily possible to arrange for (taking the latter case as example): /* Prevent unmapping when the subject domain has no X86_EMU_USE_PIRQ */ if ( !is_hvm_domain(d) || has_pirq(d) ) ret = physdev_unmap_pirq(d, unmap.pirq); else ret = -EOPNOTSUPP; rcu_unlock_domain(d); Personally I would even use a conditional operator here, but I believe others might dislike its use in situations like this one. The re-arrangement make a little more noticeable though that the comment isn't quite right either: PV domains necessarily have no X86_EMU_USE_PIRQ. Maybe "... has no notion of pIRQ"? Jan
On 2024/7/1 15:44, Jan Beulich wrote: > On 30.06.2024 14:33, 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 removal device path to unmap pirq. >> And add a new check to prevent (un)map when the subject domain >> has no X86_EMU_USE_PIRQ flag. >> >> So that the interrupt of a passthrough device can be >> successfully mapped to pirq for domU with X86_EMU_USE_PIRQ flag >> when dom0 is PVH >> >> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> >> 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> > > You keep carrying this R-b, despite making functional changes. This can't be > quite right. Will remove in next version. > > While functionally I'm now okay with the change, I still have a code structure > concern: > >> --- a/xen/arch/x86/physdev.c >> +++ b/xen/arch/x86/physdev.c >> @@ -323,6 +323,13 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >> if ( !d ) >> break; >> >> + /* Prevent mapping when the subject domain has no X86_EMU_USE_PIRQ */ >> + if ( is_hvm_domain(d) && !has_pirq(d) ) >> + { >> + rcu_unlock_domain(d); >> + return -EOPNOTSUPP; >> + } >> + >> ret = physdev_map_pirq(d, map.type, &map.index, &map.pirq, &msi); >> >> rcu_unlock_domain(d); >> @@ -346,6 +353,13 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >> if ( !d ) >> break; >> >> + /* Prevent unmapping when the subject domain has no X86_EMU_USE_PIRQ */ >> + if ( is_hvm_domain(d) && !has_pirq(d) ) >> + { >> + rcu_unlock_domain(d); >> + return -EOPNOTSUPP; >> + } >> + >> ret = physdev_unmap_pirq(d, unmap.pirq); >> >> rcu_unlock_domain(d); > > If you did go look, you will have noticed that we use "return" in the middle > of this function only very sparingly (when alternatives would result in more > complicated code elsewhere). I think you want to avoid "return" here, too, > and probably go even further and avoid the extra rcu_unlock_domain() as well. > That's easily possible to arrange for (taking the latter case as example): > > /* Prevent unmapping when the subject domain has no X86_EMU_USE_PIRQ */ > if ( !is_hvm_domain(d) || has_pirq(d) ) > ret = physdev_unmap_pirq(d, unmap.pirq); > else > ret = -EOPNOTSUPP; > > rcu_unlock_domain(d); > > Personally I would even use a conditional operator here, but I believe > others might dislike its use in situations like this one. > > The re-arrangement make a little more noticeable though that the comment > isn't quite right either: PV domains necessarily have no > X86_EMU_USE_PIRQ. Maybe "... has no notion of pIRQ"? Or just like below? /* * Prevent unmapping when the subject hvm domain has no * X86_EMU_USE_PIRQ */ if ( is_hvm_domain(d) && !has_pirq(d) ) ret = -EOPNOTSUPP; else ret = physdev_unmap_pirq(d, unmap.pirq); > > Jan
On 02.07.2024 05:15, Chen, Jiqian wrote: > On 2024/7/1 15:44, Jan Beulich wrote: >> On 30.06.2024 14:33, 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 removal device path to unmap pirq. >>> And add a new check to prevent (un)map when the subject domain >>> has no X86_EMU_USE_PIRQ flag. >>> >>> So that the interrupt of a passthrough device can be >>> successfully mapped to pirq for domU with X86_EMU_USE_PIRQ flag >>> when dom0 is PVH >>> >>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> >>> 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> >> >> You keep carrying this R-b, despite making functional changes. This can't be >> quite right. > Will remove in next version. > >> >> While functionally I'm now okay with the change, I still have a code structure >> concern: >> >>> --- a/xen/arch/x86/physdev.c >>> +++ b/xen/arch/x86/physdev.c >>> @@ -323,6 +323,13 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >>> if ( !d ) >>> break; >>> >>> + /* Prevent mapping when the subject domain has no X86_EMU_USE_PIRQ */ >>> + if ( is_hvm_domain(d) && !has_pirq(d) ) >>> + { >>> + rcu_unlock_domain(d); >>> + return -EOPNOTSUPP; >>> + } >>> + >>> ret = physdev_map_pirq(d, map.type, &map.index, &map.pirq, &msi); >>> >>> rcu_unlock_domain(d); >>> @@ -346,6 +353,13 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >>> if ( !d ) >>> break; >>> >>> + /* Prevent unmapping when the subject domain has no X86_EMU_USE_PIRQ */ >>> + if ( is_hvm_domain(d) && !has_pirq(d) ) >>> + { >>> + rcu_unlock_domain(d); >>> + return -EOPNOTSUPP; >>> + } >>> + >>> ret = physdev_unmap_pirq(d, unmap.pirq); >>> >>> rcu_unlock_domain(d); >> >> If you did go look, you will have noticed that we use "return" in the middle >> of this function only very sparingly (when alternatives would result in more >> complicated code elsewhere). I think you want to avoid "return" here, too, >> and probably go even further and avoid the extra rcu_unlock_domain() as well. >> That's easily possible to arrange for (taking the latter case as example): >> >> /* Prevent unmapping when the subject domain has no X86_EMU_USE_PIRQ */ >> if ( !is_hvm_domain(d) || has_pirq(d) ) >> ret = physdev_unmap_pirq(d, unmap.pirq); >> else >> ret = -EOPNOTSUPP; >> >> rcu_unlock_domain(d); >> >> Personally I would even use a conditional operator here, but I believe >> others might dislike its use in situations like this one. >> >> The re-arrangement make a little more noticeable though that the comment >> isn't quite right either: PV domains necessarily have no >> X86_EMU_USE_PIRQ. Maybe "... has no notion of pIRQ"? > > Or just like below? > > /* > * Prevent unmapping when the subject hvm domain has no > * X86_EMU_USE_PIRQ > */ > if ( is_hvm_domain(d) && !has_pirq(d) ) > ret = -EOPNOTSUPP; > else > ret = physdev_unmap_pirq(d, unmap.pirq); No objection to the slightly changed comment. The code alternative you present is of course functionally identical, yet personally I prefer to have the "good" case on the "if" branch and the "bad" one following "else". I wouldn't insist, though. Jan
On 2024/7/2 16:44, Jan Beulich wrote: > On 02.07.2024 05:15, Chen, Jiqian wrote: >> On 2024/7/1 15:44, Jan Beulich wrote: >>> On 30.06.2024 14:33, 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 removal device path to unmap pirq. >>>> And add a new check to prevent (un)map when the subject domain >>>> has no X86_EMU_USE_PIRQ flag. >>>> >>>> So that the interrupt of a passthrough device can be >>>> successfully mapped to pirq for domU with X86_EMU_USE_PIRQ flag >>>> when dom0 is PVH >>>> >>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> >>>> 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> >>> >>> You keep carrying this R-b, despite making functional changes. This can't be >>> quite right. >> Will remove in next version. >> >>> >>> While functionally I'm now okay with the change, I still have a code structure >>> concern: >>> >>>> --- a/xen/arch/x86/physdev.c >>>> +++ b/xen/arch/x86/physdev.c >>>> @@ -323,6 +323,13 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >>>> if ( !d ) >>>> break; >>>> >>>> + /* Prevent mapping when the subject domain has no X86_EMU_USE_PIRQ */ >>>> + if ( is_hvm_domain(d) && !has_pirq(d) ) >>>> + { >>>> + rcu_unlock_domain(d); >>>> + return -EOPNOTSUPP; >>>> + } >>>> + >>>> ret = physdev_map_pirq(d, map.type, &map.index, &map.pirq, &msi); >>>> >>>> rcu_unlock_domain(d); >>>> @@ -346,6 +353,13 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >>>> if ( !d ) >>>> break; >>>> >>>> + /* Prevent unmapping when the subject domain has no X86_EMU_USE_PIRQ */ >>>> + if ( is_hvm_domain(d) && !has_pirq(d) ) >>>> + { >>>> + rcu_unlock_domain(d); >>>> + return -EOPNOTSUPP; >>>> + } >>>> + >>>> ret = physdev_unmap_pirq(d, unmap.pirq); >>>> >>>> rcu_unlock_domain(d); >>> >>> If you did go look, you will have noticed that we use "return" in the middle >>> of this function only very sparingly (when alternatives would result in more >>> complicated code elsewhere). I think you want to avoid "return" here, too, >>> and probably go even further and avoid the extra rcu_unlock_domain() as well. >>> That's easily possible to arrange for (taking the latter case as example): >>> >>> /* Prevent unmapping when the subject domain has no X86_EMU_USE_PIRQ */ >>> if ( !is_hvm_domain(d) || has_pirq(d) ) >>> ret = physdev_unmap_pirq(d, unmap.pirq); >>> else >>> ret = -EOPNOTSUPP; >>> >>> rcu_unlock_domain(d); >>> >>> Personally I would even use a conditional operator here, but I believe >>> others might dislike its use in situations like this one. >>> >>> The re-arrangement make a little more noticeable though that the comment >>> isn't quite right either: PV domains necessarily have no >>> X86_EMU_USE_PIRQ. Maybe "... has no notion of pIRQ"? >> >> Or just like below? >> >> /* >> * Prevent unmapping when the subject hvm domain has no >> * X86_EMU_USE_PIRQ >> */ >> if ( is_hvm_domain(d) && !has_pirq(d) ) >> ret = -EOPNOTSUPP; >> else >> ret = physdev_unmap_pirq(d, unmap.pirq); > > No objection to the slightly changed comment. The code alternative you > present is of course functionally identical, yet personally I prefer to > have the "good" case on the "if" branch and the "bad" one following > "else". I wouldn't insist, though. OK, will change "good" case on the "if" branch. Do I need to change "!is_hvm_domain(d)" to "is_pv_domain(d)" ? And then have: /* Only unmapping when the subject domain has a notion of PIRQ */ if ( is_pv_domain(d) || has_pirq(d) ) ret = physdev_unmap_pirq(d, unmap.pirq); else ret = -EOPNOTSUPP; > > Jan
On 04.07.2024 04:56, Chen, Jiqian wrote: > On 2024/7/2 16:44, Jan Beulich wrote: >> On 02.07.2024 05:15, Chen, Jiqian wrote: >>> On 2024/7/1 15:44, Jan Beulich wrote: >>>> On 30.06.2024 14:33, Jiqian Chen wrote: >>>>> --- a/xen/arch/x86/physdev.c >>>>> +++ b/xen/arch/x86/physdev.c >>>>> @@ -323,6 +323,13 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >>>>> if ( !d ) >>>>> break; >>>>> >>>>> + /* Prevent mapping when the subject domain has no X86_EMU_USE_PIRQ */ >>>>> + if ( is_hvm_domain(d) && !has_pirq(d) ) >>>>> + { >>>>> + rcu_unlock_domain(d); >>>>> + return -EOPNOTSUPP; >>>>> + } >>>>> + >>>>> ret = physdev_map_pirq(d, map.type, &map.index, &map.pirq, &msi); >>>>> >>>>> rcu_unlock_domain(d); >>>>> @@ -346,6 +353,13 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >>>>> if ( !d ) >>>>> break; >>>>> >>>>> + /* Prevent unmapping when the subject domain has no X86_EMU_USE_PIRQ */ >>>>> + if ( is_hvm_domain(d) && !has_pirq(d) ) >>>>> + { >>>>> + rcu_unlock_domain(d); >>>>> + return -EOPNOTSUPP; >>>>> + } >>>>> + >>>>> ret = physdev_unmap_pirq(d, unmap.pirq); >>>>> >>>>> rcu_unlock_domain(d); >>>> >>>> If you did go look, you will have noticed that we use "return" in the middle >>>> of this function only very sparingly (when alternatives would result in more >>>> complicated code elsewhere). I think you want to avoid "return" here, too, >>>> and probably go even further and avoid the extra rcu_unlock_domain() as well. >>>> That's easily possible to arrange for (taking the latter case as example): >>>> >>>> /* Prevent unmapping when the subject domain has no X86_EMU_USE_PIRQ */ >>>> if ( !is_hvm_domain(d) || has_pirq(d) ) >>>> ret = physdev_unmap_pirq(d, unmap.pirq); >>>> else >>>> ret = -EOPNOTSUPP; >>>> >>>> rcu_unlock_domain(d); >>>> >>>> Personally I would even use a conditional operator here, but I believe >>>> others might dislike its use in situations like this one. >>>> >>>> The re-arrangement make a little more noticeable though that the comment >>>> isn't quite right either: PV domains necessarily have no >>>> X86_EMU_USE_PIRQ. Maybe "... has no notion of pIRQ"? >>> >>> Or just like below? >>> >>> /* >>> * Prevent unmapping when the subject hvm domain has no >>> * X86_EMU_USE_PIRQ >>> */ >>> if ( is_hvm_domain(d) && !has_pirq(d) ) >>> ret = -EOPNOTSUPP; >>> else >>> ret = physdev_unmap_pirq(d, unmap.pirq); >> >> No objection to the slightly changed comment. The code alternative you >> present is of course functionally identical, yet personally I prefer to >> have the "good" case on the "if" branch and the "bad" one following >> "else". I wouldn't insist, though. > OK, will change "good" case on the "if" branch. > Do I need to change "!is_hvm_domain(d)" to "is_pv_domain(d)" ? > And then have: > > /* Only unmapping when the subject domain has a notion of PIRQ */ > if ( is_pv_domain(d) || has_pirq(d) ) > ret = physdev_unmap_pirq(d, unmap.pirq); > else > ret = -EOPNOTSUPP; I for one would prefer if you kept using is_hvm_domain(), for being more precise in this situation. Jan
On 2024/7/4 14:38, Jan Beulich wrote: > On 04.07.2024 04:56, Chen, Jiqian wrote: >> On 2024/7/2 16:44, Jan Beulich wrote: >>> On 02.07.2024 05:15, Chen, Jiqian wrote: >>>> On 2024/7/1 15:44, Jan Beulich wrote: >>>>> On 30.06.2024 14:33, Jiqian Chen wrote: >>>>>> --- a/xen/arch/x86/physdev.c >>>>>> +++ b/xen/arch/x86/physdev.c >>>>>> @@ -323,6 +323,13 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >>>>>> if ( !d ) >>>>>> break; >>>>>> >>>>>> + /* Prevent mapping when the subject domain has no X86_EMU_USE_PIRQ */ >>>>>> + if ( is_hvm_domain(d) && !has_pirq(d) ) >>>>>> + { >>>>>> + rcu_unlock_domain(d); >>>>>> + return -EOPNOTSUPP; >>>>>> + } >>>>>> + >>>>>> ret = physdev_map_pirq(d, map.type, &map.index, &map.pirq, &msi); >>>>>> >>>>>> rcu_unlock_domain(d); >>>>>> @@ -346,6 +353,13 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >>>>>> if ( !d ) >>>>>> break; >>>>>> >>>>>> + /* Prevent unmapping when the subject domain has no X86_EMU_USE_PIRQ */ >>>>>> + if ( is_hvm_domain(d) && !has_pirq(d) ) >>>>>> + { >>>>>> + rcu_unlock_domain(d); >>>>>> + return -EOPNOTSUPP; >>>>>> + } >>>>>> + >>>>>> ret = physdev_unmap_pirq(d, unmap.pirq); >>>>>> >>>>>> rcu_unlock_domain(d); >>>>> >>>>> If you did go look, you will have noticed that we use "return" in the middle >>>>> of this function only very sparingly (when alternatives would result in more >>>>> complicated code elsewhere). I think you want to avoid "return" here, too, >>>>> and probably go even further and avoid the extra rcu_unlock_domain() as well. >>>>> That's easily possible to arrange for (taking the latter case as example): >>>>> >>>>> /* Prevent unmapping when the subject domain has no X86_EMU_USE_PIRQ */ >>>>> if ( !is_hvm_domain(d) || has_pirq(d) ) >>>>> ret = physdev_unmap_pirq(d, unmap.pirq); >>>>> else >>>>> ret = -EOPNOTSUPP; >>>>> >>>>> rcu_unlock_domain(d); >>>>> >>>>> Personally I would even use a conditional operator here, but I believe >>>>> others might dislike its use in situations like this one. >>>>> >>>>> The re-arrangement make a little more noticeable though that the comment >>>>> isn't quite right either: PV domains necessarily have no >>>>> X86_EMU_USE_PIRQ. Maybe "... has no notion of pIRQ"? >>>> >>>> Or just like below? >>>> >>>> /* >>>> * Prevent unmapping when the subject hvm domain has no >>>> * X86_EMU_USE_PIRQ >>>> */ >>>> if ( is_hvm_domain(d) && !has_pirq(d) ) >>>> ret = -EOPNOTSUPP; >>>> else >>>> ret = physdev_unmap_pirq(d, unmap.pirq); >>> >>> No objection to the slightly changed comment. The code alternative you >>> present is of course functionally identical, yet personally I prefer to >>> have the "good" case on the "if" branch and the "bad" one following >>> "else". I wouldn't insist, though. >> OK, will change "good" case on the "if" branch. >> Do I need to change "!is_hvm_domain(d)" to "is_pv_domain(d)" ? >> And then have: >> >> /* Only unmapping when the subject domain has a notion of PIRQ */ >> if ( is_pv_domain(d) || has_pirq(d) ) >> ret = physdev_unmap_pirq(d, unmap.pirq); >> else >> ret = -EOPNOTSUPP; > > I for one would prefer if you kept using is_hvm_domain(), for being more > precise in this situation. OK, thanks. Will change in next version. > > Jan
diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c index 0fab670a4871..03ada3c880bd 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 d6dd622952a9..a165f68225c1 100644 --- a/xen/arch/x86/physdev.c +++ b/xen/arch/x86/physdev.c @@ -323,6 +323,13 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) if ( !d ) break; + /* Prevent mapping when the subject domain has no X86_EMU_USE_PIRQ */ + if ( is_hvm_domain(d) && !has_pirq(d) ) + { + rcu_unlock_domain(d); + return -EOPNOTSUPP; + } + ret = physdev_map_pirq(d, map.type, &map.index, &map.pirq, &msi); rcu_unlock_domain(d); @@ -346,6 +353,13 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) if ( !d ) break; + /* Prevent unmapping when the subject domain has no X86_EMU_USE_PIRQ */ + if ( is_hvm_domain(d) && !has_pirq(d) ) + { + rcu_unlock_domain(d); + return -EOPNOTSUPP; + } + ret = physdev_unmap_pirq(d, unmap.pirq); rcu_unlock_domain(d);